[PATCH 01/20] mtd: pxa3xx_nand: refuse the flash definition get from platform

Lei Wen adrian.wenl at gmail.com
Wed May 26 09:35:07 EDT 2010


On Tue, May 25, 2010 at 9:21 PM, Eric Miao <eric.y.miao at gmail.com> wrote:
> On Tue, May 25, 2010 at 9:01 PM, Lei Wen <adrian.wenl at gmail.com> wrote:
>> On Tue, May 25, 2010 at 6:21 PM, Mike Rapoport <mike at compulab.co.il> wrote:
>>> Lei Wen wrote:
>>>>
>>>> On Mon, May 24, 2010 at 9:21 PM, Mike Rapoport <mike at compulab.co.il>
>>>> wrote:
>>>>>
>>>>> Lei Wen wrote:
>>>>>>
>>>>>> Hi Mike,
>>>>>>
>>>>>> On Mon, May 24, 2010 at 7:00 PM, Mike Rapoport <mike at compulab.co.il>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Lei,
>>>>>>>
>>>>>>> Lei Wen wrote:
>>>>>>>>
>>>>>>>> Hi Mike,
>>>>>>>>
>>>>>>> 2) I don't like hadrcoding of NAND parameters into the driver. You
>>>>>>> remove
>>>>>>> *deprecetad* CONFIG_MTD_NAND_PXA3xx_BUILTIN configuration option and
>>>>>>> instead
>>>>>>> you enforce use of built-in definitions. The driver in its current
>>>>>>> state
>>>>>>> is
>>>>>>> robust enough to allow platforms to define optimized NAND timings
>>>>>>> either
>>>>>>> in
>>>>>>> the bootloader or in the kernel. If you don't like that multiple
>>>>>>> platforms
>>>>>>> define the same flash chip create an enumeration of built-in types and
>>>>>>> let
>>>>>>> platforms to use this enumeration to select the NAND chip. But, anyway,
>>>>>>> there should be a fallback mode that will support NAND chips that are
>>>>>>> not
>>>>>>> defined in the driver, probably with suboptimal timings.
>>>>>>
>>>>>> Original driver also use hardcoding. And in bootloader, this timing
>>>>>> parameter is also hard coding.
>>>>>> We cannot deduce a parameter set only from the nand id, that is why we
>>>>>> need a table to preset it.
>>>>>> If one nand chip is not listed in that table, the chip id would still
>>>>>> be printed out, so that we can do something for that.
>>>>>> If we encourage people to continue on this, we would not able to
>>>>>> really "driver" that nand.
>>>>>
>>>>> Currently pxa3xx-nand has three operational modes:
>>>>> - use NAND parameters supplied by the platform
>>>>> - use presets configured by the bootloader chain
>>>>> - use built-in NAND parameters, marked as deprecated in favor of the
>>>>> first
>>>>> two
>>>>> You remove the first two modes completely and require that each and every
>>>>> NAND chip used on pxa3xx based platform will be added to the driver. This
>>>>> way you make the driver less robust and harder to use for platform
>>>>> developers, not mentioning you're breaking the existing platforms.
>>>>> In my opinion, the driver *may* support built-in definitions for certain
>>>>> NAND flashes and *must* support configuration of the NAND parameters by
>>>>> the
>>>>> platform code and bootloader.
>>>>>
>>>>
>>>> Hi Mike,
>>>>
>>>> Well... I would submit another patch set which would reserve a way
>>>> that platform could pass its parameter setting.
>>>> Like specify the certain type of nand chip parameter for each chip
>>>> select. Is that ok for you?
>>>>
>>>> For bootloader pass parameter method, I think this way should be
>>>> dropped... For there is attributed which we could not tell from
>>>> registers...
>>>> What do you think of this?
>>>
>>> Can you please elaborate what are the attributes that cannot be detected?
>>
>> The attributes comes from two new features that would be included into
>> pxa3xx_nand: BCH support and 8bit per 512bytes ECC.
>> Originally, our pxa3xx_nand only support Hamming ECC way. So no doubt
>> just read the timing register, and just set use ecc bit in NDCR is
>> enough.
>> But for now there is two type of ECC type. For distinguish, we
>> introduce a new register called NDECCCTRL. NDECCCTRL_BCH_EN in the bit
>> 0 of that register control whether use the BCH ECC.
>> You may ask me why not just read from NDECCCTRL to know the ECC type.
>> It is right, but not always to be true. It the last command executed
>> for nand before kernel take control is not read or write operation,
>> the bootloader has no reason to set the NDECCCTRL register. So our
>> kernel also unable to detect whether to use BCH or HAMMING ECC.
>>
>> This two type of ecc has different character, if use hamming ecc for
>> reading while that page is written by BCH, it would instant report of
>> DB error...
>> This is not the worst case., for we may ask the bootloader to always
>> set the NDECCCTRL. (There is no reason to apply ECC when do command
>> except the read and write although)
>>
>> The worst would comes at the supporting of 8bit per 512 bytes ECC. Our
>> BCH engine only support 16bit corroction per 2k data, aka 4bit per
>> 512bytes.
>> If we need to support 8bit per 512bytes feature, which is needed by a
>> lot of newest flash which require such high ecc strength.
>> Then we have to apply the BCH to 1k data instead of 2k. The layout of
>> content on flash would change.
>> Original BCH apply to 2k nand, the layout may like: | 2k data | + | 30
>> bytes ECC |
>> Now after apply BCH to 1k range, the layout would be like: | 1k data |
>> + | 30 bytes ECC| + | 1k data| + |30 bytes ECC|.
>> Even as I said let the bootloader to always set the NDECCCTRL. The
>> kernel would still cannot distinguish the two use case of 4bit per
>> 512bytes and 8bit per 512 bytes ECC strength requirement.
>> For they both use BCH, the only difference could be told on the nand
>> layout, not by register reading.
>>
>> Those two new feature would lead different nand content layout. So if
>> not distinguish properly, nand would not be able to work functional.
>> And obviously, the two features cannot get from register reading, and
>> that is the reason why I suggest drop the feature.
>>
>
> Then would it be possible to just drop the feature for pxa168 and onward?
> I guess there might be some existing usages out there for pxa3xx already,
> so not to affect existing systems.
>
> Actually, it's now quite easy for a single driver to distinguish this, through
> platform_device_id table please, see drivers/i2c/busses/i2c-pxa.c for an
> example. I'd like to call the device pxa168-nand or something.

Yes, that is also what I want. Just put another attributes like ecc
strength into flash_info structure.
This info could be revealed in the built_in_table or flash info passed
from platform data.

Also this ecc strength could take place the ecc type used in the flash
info structure.

Thanks,
Lei



More information about the linux-arm-kernel mailing list