[U-Boot-Users] [PATCH RFC] ARM: Davinci: NAND fix for large page ECC and linux compatibility

[Bernard Blackham]

[offlist thread cc'd back to the list - hope you don't mind Scott!]

Hi Scott,

Thanks for your feedback.

On Fri, Jun 27, 2008 at 12:04:00PM -0500, Scott Wood wrote:
>Bernard Blackham wrote: 
>> What I would like to know though, is are you the right person to
>> push this through, and can it make it into U-boot 1.3.4? I'm asking
>> because it contains potential compatibility breaks, and I'd like to
>> document the specific U-boot versions that will be affected.
>
> There is a general ARM tree, but I can take it through mine if that's  
> what is preferred.

That was the suggestion on #uboot. I'd say it's more NAND related
than ARM related too.

>> +/*
>> + * Previous versions of u-boot (1.3.3 and prior) and Montavista Linux kernels
>> + * generated bogus ECCs on large-page NAND. Both large and small page NAND ECCs
>> + * were incompatible with the Linux davinci git tree (since NAND was integrated
>> + * in 2.6.24).
>> + * Don't turn this on if you want backwards compatibility.
>> + * Do turn this on if you want u-boot to be able to read and write NAND
>> + * that can be written or read by the Linux davinci git kernel.
>> + *
>> +#define CFG_LINUX_COMPATIBLE_ECC
>> + */
>
> It seems odd that backwards compatibility requires turning *off* an  
> option with "compatible" in the name...  I'd invert the sense of the  
> ifdef, and have it be something like CFG_BROKEN_ECC_COMPATIBILITY.

The concern with this is people that use their own custom config
files will need to add this #define when they upgrade. How about
just changing the name to CFG_NEW_NAND_ECC_FORMAT then?

> If the old way of doing small page ECC was valid, should we preserve  
> that (and change Linux back)?

That's a little controversial. Basically, the old OOB layout didn't
match any other layout used (except by the MV kernel), the actual
ECC layout meant that the method for correction was overly complex
(with 170 non-obvious lines of code), and allegedly broken:
   http://article.gmane.org/gmane.comp.boot-loaders.u-boot/32035

The new code is about 30 lines, really simple, and I can even prove
it's correctness (which I couldn't even begin to with the old code).
Troy (cc'd) I believe was the original author.  It could probably do
with some comments though to make it dead obvious to the casual
observer what's going on. I'll add them in.

> We should probably default to doing it the right way, not the  
> broken-but-compatible way for large pages, though.

It depends if you put backwards compatibility over reliability
though. Many davinci users are still running the MontaVista-supplied
2.6.10 kernel which has the same broken ECC code and I've heard no
word from MV on fixing it yet (and they're probably struggling to
deal with the same backwards compatibility issue).

How about this solution: in davinci/nand.c, we add something like
this:

#if defined(CFG_NAND_LARGEPAGE) && !defined(CFG_LINUX_COMPATIBLE_ECC)
/* Comment this #error out only if you really really have to. */
#error "You are using old ECC code that is broken on large page devices. See doc/README.davinci"
#endif

This forces the user to make a choice - they'll probably curse while
they're doing it, but they can't plead ignorance when they find
their large page NAND isn't detecting ECC errors.

> Perhaps we could use some currently unused OOB byte as a marker
> for new/old ECC layout?

Could do, but any filesystems which use the OOB bytes might step on
these. It also complicates the code even moreso and creates a lot
more scenarios to test and that could go wrong.

I really do believe it should be a clean switch from one format to
the other, for both small and large page NAND, with no run-time
backwards compatibility. But that's just my POV.

Cheers,
Bernard.