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

[Amit Virdi <amit.virdi@st.com>]

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.

> 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;


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?

>
>> +	nand->options = 0;
>> +#if defined(CONFIG_SYS_FSMC_NAND_16BIT)
>> +	nand->options |= NAND_BUSWIDTH_16;
>> +#endif
>
> No on-flash BBT?
>

No, not implemented yet.

>> +	/* 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.

>> +/*
>> + * 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.

Thanks
Amit Virdi