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

[Eric Miao]

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.