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

[Narnakaje, Snehaprabha]

Dave,

> -----Original Message-----
> From: David Brownell [mailto:david-b at pacbell.net]
> Sent: Thursday, May 07, 2009 3:17 AM
> To: Narnakaje, Snehaprabha
> Cc: davinci-linux-open-source at linux.davincidsp.com; linux-
> mtd at lists.infradead.org; dwmw2 at infradead.org; tglx at linutronix.de;
> akpm at linux-foundation.org
> Subject: Re: [PATCH 2/2] NAND on DM355: Add 4-bit ECC support for large
> page NAND chips
> 
> 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.

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

> (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?

> 
> This patch is sufficient to support development boards for
> the dm355, dm357, and dm365 ... right?  They all have 2 GByte
> NAND chips, ISTR with 2K pages, and haven't yet had to switch
> to more current parts with 4K pages.
> 
> 
> On Wednesday 06 May 2009, nsnehaprabha at ti.com wrote:
> > --- a/drivers/mtd/nand/davinci_nand.c
> > +++ b/drivers/mtd/nand/davinci_nand.c
> > @@ -500,6 +500,21 @@ static struct nand_ecclayout hwecc4_small
> __initconst
> > = { },
> >  };
> >
> > +/* An ECC layout for using 4-bit ECC with large-page (2048bytes) flash,
> > + * storing ten ECC bytes plus the manufacturer's bad block marker byte,
> > + * and not overlapping the default BBT markers.
> > + */
> > +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. 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.

> 
> 
> > +       .eccpos = { 0, 1, 2, 3, 4, 6, 7, 8, 9, 10,
> > +               11, 12, 13, 14, 15, 24, 25, 26, 27, 28,
> > +               29, 30, 31, 32, 33, 34, 35, 36, 37, 38,
> > +               39, 40, 41, 42, 43, 44, 45, 46, 47, 48, },
> > +       .oobfree = {
> > +               {.offset = 16, .length = 8, },
> > +               {.offset = 49, },
> 
> Comments would be good, highlighting (a) byte 5 is reserved,
> it's the manufacturer bad block marker, (b) 8 bytes @16 are
> expected by JFFS2.  Not everyone will "just know" those.

OK, will add in the next patch.

> 
> 
> > +       },
> > +};
> >
> >  static int __init nand_davinci_probe(struct platform_device *pdev)
> >  {
> 
> > @@ -690,6 +705,13 @@ static int __init nand_davinci_probe(struct
> platform_device *pdev)
> >                                 info->mtd.oobsize - 16;
> >                         goto syndrome_done;
> >                 }
> > +               if (chunks == 4) {
> > +                       info->ecclayout = hwecc4_2048;
> > +                       info->ecclayout.oobfree[1].length =
> > +                               info->mtd.oobsize - 49;
> > +                       info->chip.ecc.mode = NAND_ECC_HW_OOB_FIRST;
> > +                       goto syndrome_done;
> > +               }
> >
> >                 /* For large page chips we'll be wanting to use a
> >                  * not-yet-implemented mode that reads OOB data
> 
> You should update that comment ... that not-yet-implemented mode
> is now called "NAND_ECC_HW_OOB_FIRST", from patch 1/2 ... ;)

Yes, but will have a comment about 4K though.

Thanks
Sneha

>