[PATCH 2/2] NAND on DM355: Add 4-bit ECC support for large page NAND chips

[David Brownell]

On Thursday 07 May 2009, Narnakaje, Snehaprabha wrote:

> > I'm glad to see this patch is so small ... basically, just
> > adding a special case for 2K pages, and keeping the core of
> > this NAND driver the same.  Not needing to change the 4-bit
> > ECC support from the patch I sent earlier seems a good sign.
> 
> Yes, I had to use the .calculate in the new read_page handler
> to stop the ECC though. 

Oh, now I see what you were talking about off-list.  That
doesn't seem quite right.  I'll take a look.


> > Two comments though:  (a) the board-dm355-evm.c file isn't
> > yet in mainline, so the MTD folk can't take this patch as-is;
> 
> Sorry, I didn't realize this. I will separate the board-dm355-evm.c
> from this patch and submit the patch #3 for board-dm355-evm.c to go
> to davinci-linux-open-source.  

Let's wait until we see the previous davinci_nand.c patches get
queued up somewhere "official" though ... in fact, I don't see
why they shouldn't merge for 2.6.30-soon.


> > (b) as noted elsewhere, there are still issues with 4K pages
> > and the NAND core infrastructure.
> 
> Yes, this is still an issue, if we make the read_page handler
> use .eccpos[] for positioning the ECC bytes in the OOB area.
> If we had fixed prepad+ecc nand_ecclayout we would avoid using
> eccpos[] (This is what my initial set of 4-bit ECC patches did
> to support 4K). But this is not a generic solution.    
> 
> The main problem is with nand_ooblayout structure that is not
> extendable for large pages 4K or more, without breaking the
> userspace IOCTLs that is dependent on the size of this structure.   
> 
> Question to linux-mtd list: Are there plans in the linux-mtd to 
> address this generic issue, now that NAND manufacturers are
> coming up NAND chips > 4K page size?

They've been doing it for a while now, actually.  :)

And it wouldn't be an issue here ... *IF* this 4-bit ECC didn't
need to use 80 bytes (8 chunks of 512 bytes, 10 bytes ECC per
chunk) from a 64 byte field.  Doesn't fit.

Maybe the ecclayout stuff should migrate to sysfs, and just
drop the old ioctl stuff for parts that can't use it ... as
is being done to support chips that are 4 GByte and bigger.


> > > +static struct nand_ecclayout hwecc4_2048 __initconst = {
> > > +       .eccbytes = 10,
> > 
> > Not ".eccbytes = 40"?  This is 4 chunks, 10 ecc bytes each...
> 
> No, .eccbytes is for each chips->ecc.steps. 

So it's the same as ecc.bytes, which is not exported to userspace.


> And all nand read/write 
> APIs, we handle ecc.steps (for loop). There is chips->ecc.total that
> is initialized as (chips->ecc.steps * chips->ecc.bytes).  
> 
> It is strange that .eccbytes is for each chunk, while eccpos[] and
> .oobfree[] have to handle/cover all chunks. 

Also strange:  when ECC_HW_SYNDROME is in use, nothing even
looks at the nand_ecclayout to make sure it matches the actual
layout being used ... which derives entirely from the prepad,
postpad, and bytes fields of "ecc".  (Except that some of the
OOB bytes not used for ECC may be hidden, e.g. manufacturer
bad block markers are usually not listed.)

- Dave