[U-Boot] [PATCH 1/3] mtd/NAND: Add FSMC driver support

[Scott Wood]

On 03/02/2012 05:16 AM, Amit Virdi wrote:
> Hello Scott,
> 
> On 3/2/2012 2:57 AM, Scott Wood wrote:
>> On 02/27/2012 03:38 AM, Amit Virdi wrote:
>>> +        /*
>>> +         * This is a temporary erase check. A newly erased page read
>>> +         * would result in an ecc error because the oob data is also
>>> +         * erased to FF and the calculated ecc for an FF data is not
>>> +         * FF..FF.
>>> +         * This is a workaround to skip performing correction in case
>>> +         * data is FF..FF
>>> +         */
>>> +        for (k = 0; k<  eccsize; k++) {
>>> +            if (*(p + k) != 0xff)
>>> +                break;
>>> +        }
>>
>> Shouldn't this apply over the whole page (including the ECC bytes
>> themselves), not just the ECC chunk?  The data could legitimately be all
>> 0xff except for one bit, and that bit could have flipped...
> 
> So you're saying that such a page is wrongly interpreted as "erased
> page" and ecc check is skipped! Yes, you are very right. This check
> should be applied to the whole page and not only the chunk.
> 
>>
>> Will a freshly erased page show up as having correctable errors, or
> 
> A newly erased page contains 0xff in data as well as spare area. So most
> likely, it shows up as having uncorrectable errors.

This is what I saw as well on FSL IFC with 4-bit ECC (see is_blank() and
related code in drivers/mtd/nand/fsl_ifc_nand.c for another driver doing
this sort of check).

>> uncorrectable?  If the latter, just hold off on declaring the page as an
>> ECC fail until you've read the whole thing, and then if you're about to
>> mark it failed, check wheter it's freshly erased.
> 
> So do you mean to say:
> 
>             stat = chip->ecc.correct(mtd, p, &ecc_code[i],
>                     &ecc_calc[i]);
>             if (stat < 0) {
>               if (data as well as the oob is 0xff)
>                 do nothing;
>                           else
>                 mtd->ecc_stats.failed++;
>             } else
>                 mtd->ecc_stats.corrected += stat;

Yes, provided you check the entire page and not just the ecc subpage
after seeing an error.

> Can we have a scenario like this:
> A page has been erased - so it contains 0xff in data area as well as
> spare area. Somehow, a bit in either or say both areas are flipped, then
> the SW will not be able to distinguish if it's and erased page or a page
> with data and uncorrectable errors. How do we take care of such scenarios?

It will show up as an uncorrectable error (which is no big loss, since
the page has no data, but may result in a block being marked bad before
its time).

If you really want, you could keep a count and allow a certain number of
bits to be flipped (up to the ECC threshold) and still consider it blank
(and report the number of corrections as usual).

>>> +    /* Detect NAND chips */
>>> +    if (nand_scan_ident(mtd, 1, NULL))
>>> +        return -ENXIO;
>>
>> You should #define CONFIG_SYS_NAND_SELF_INIT if you want to call this
>> yourself (see the documentation for what else you need to do).
>>
> 
> Ok. I see this is the new philosophy that is encouraged even for the
> existing drivers.

Yes, it would be nice if the old way could go away at some point.

>>> +/*
>>> + * There are 13 bytes of ecc for every 512 byte block and it has to
>>> be read
>>> + * consecutively and immediately after the 512 byte data block for
>>> hardware to
>>> + * generate the error bit offsets
>>> + * Managing the ecc bytes in the following way is easier. This way
>>> is similar to
>>> + * oobfree structure maintained already in u-boot nand driver
>>> + */
>>> +#define MAX_ECCPLACE_ENTRIES    32
>>
>> No FSMC namespace... is/will this file included by anything but the FSMC
>>   driver?
>>
> 
> Sorry, it didn't get you here. This file (fsmc_nand.h) shall be included
> by board files.

I'd either namespace it or (preferably) keep this in the driver file
itself, but as long as nothing outside FSMC-specific code includes this
(directly or indirectly -- e.g. the board config file must not include
it) I won't complain too loudly.

-Scott