[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[John Rigby]

Sorry for the late comments.  We have been trying to use this code with the
associated davinci 4-bit ecc patches and have some questions (inline).

.....
> +       uint8_t *ecc_code = chip->buffers->ecccode;
> +       uint32_t *eccpos = chip->ecc.layout->eccpos;
> +       uint8_t *ecc_calc = chip->buffers->ecccalc;
> +
> +       /* Read the OOB area first */
> +       chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
>

What about chips that do not support the  NAND_CMD_READOOB?  Do I need to
provide my own read routine for that case?

+       chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +       chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> +
> +       for (i = 0; i < chip->ecc.total; i++)
> +               ecc_code[i] = chip->oob_poi[eccpos[i]];
> +
> +       for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
> +               int stat;
> +
> +               chip->ecc.hwctl(mtd, NAND_ECC_READ);
> +               chip->read_buf(mtd, p, eccsize);
> +               chip->ecc.calculate(mtd, p, &ecc_calc[i]);
>

Here you calculate ecc then never use the result?

+
> +               stat = chip->ecc.correct(mtd, p, &ecc_code[i], NULL);
> +               if (stat < 0)
> +                       mtd->ecc_stats.failed++;
> +               else
> +                       mtd->ecc_stats.corrected += stat;
> +       }
> +       return 0;
> +}
> +
>

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

John Rigby wrote:
> Sorry for the late comments.  We have been trying to use this code with 
> the associated davinci 4-bit ecc patches and have some questions (inline).
> 
>     .....
>     +       uint8_t *ecc_code = chip->buffers->ecccode;
>     +       uint32_t *eccpos = chip->ecc.layout->eccpos;
>     +       uint8_t *ecc_calc = chip->buffers->ecccalc;
>     +
>     +       /* Read the OOB area first */
>     +       chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> 
>  
> What about chips that do not support the  NAND_CMD_READOOB?  Do I need 
> to provide my own read routine for that case?

cmdfunc is supposed to fix that up.  This is already the case with 
existing code.

>     +       chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
>     +       chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
>     +
>     +       for (i = 0; i < chip->ecc.total; i++)
>     +               ecc_code[i] = chip->oob_poi[eccpos[i]];
>     +
>     +       for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
>     +               int stat;
>     +
>     +               chip->ecc.hwctl(mtd, NAND_ECC_READ);
>     +               chip->read_buf(mtd, p, eccsize);
>     +               chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> 
>  
> Here you calculate ecc then never use the result?

Hmm, that looks wrong, both here and in the davinci driver.  Are the two 
calls to nand_davinci_4bit_readecc reading different things?  Does the 
calculate function have any side effects beyond producing data that is 
never used?

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Paulraj, Sandeep]

> John Rigby wrote:
> > Sorry for the late comments.  We have been trying to use this code with
> > the associated davinci 4-bit ecc patches and have some questions
We use this internally and it works. Are you having any issues because we don't see any!!
J-C has to apply a patch and you will need that for this to work properly.
That patch updates the DM355 Config
> (inline).
> >
> >     .....
> >     +       uint8_t *ecc_code = chip->buffers->ecccode;
> >     +       uint32_t *eccpos = chip->ecc.layout->eccpos;
> >     +       uint8_t *ecc_calc = chip->buffers->ecccalc;
> >     +
> >     +       /* Read the OOB area first */
> >     +       chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> >
> >
> > What about chips that do not support the  NAND_CMD_READOOB?  Do I need
> > to provide my own read routine for that case?
> 
> cmdfunc is supposed to fix that up.  This is already the case with
> existing code.
> 
> >     +       chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> >     +       chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> >     +
> >     +       for (i = 0; i < chip->ecc.total; i++)
> >     +               ecc_code[i] = chip->oob_poi[eccpos[i]];
> >     +
> >     +       for (i = 0; eccsteps; eccsteps--, i += eccbytes, p +=
> eccsize) {
> >     +               int stat;
> >     +
> >     +               chip->ecc.hwctl(mtd, NAND_ECC_READ);
> >     +               chip->read_buf(mtd, p, eccsize);
> >     +               chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> >
> >
> > Here you calculate ecc then never use the result?
> 
> Hmm, that looks wrong, both here and in the davinci driver.  Are the two
> calls to nand_davinci_4bit_readecc reading different things?  Does the
> calculate function have any side effects beyond producing data that is
> never used?
Have you reads the patch description. Maybe that might help a bit

This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
read_page API that first reads the OOB area, reads the data in chunks,
feeds the ECC from OOB area to the ECC hw engine and perform any
correction on the data as per the ECC status reported by the engine.

> 
> -Scott
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[John Rigby]

Scott answered my question about chips that don't support NAND_CMD_READOOB,
we need to take care of it in cmdfunc.

I still don't see why the calculate method is called.  The results are
ignored.

On Tue, Sep 1, 2009 at 10:03 AM, Paulraj, Sandeep <s-paulraj@ti.com> wrote:

>
> > John Rigby wrote:
> > > Sorry for the late comments.  We have been trying to use this code with
> > > the associated davinci 4-bit ecc patches and have some questions
> We use this internally and it works. Are you having any issues because we
> don't see any!!
> J-C has to apply a patch and you will need that for this to work properly.
> That patch updates the DM355 Config
> > (inline).
> > >
> > >     .....
> > >     +       uint8_t *ecc_code = chip->buffers->ecccode;
> > >     +       uint32_t *eccpos = chip->ecc.layout->eccpos;
> > >     +       uint8_t *ecc_calc = chip->buffers->ecccalc;
> > >     +
> > >     +       /* Read the OOB area first */
> > >     +       chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> > >
> > >
> > > What about chips that do not support the  NAND_CMD_READOOB?  Do I need
> > > to provide my own read routine for that case?
> >
> > cmdfunc is supposed to fix that up.  This is already the case with
> > existing code.
> >
> > >     +       chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> > >     +       chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> > >     +
> > >     +       for (i = 0; i < chip->ecc.total; i++)
> > >     +               ecc_code[i] = chip->oob_poi[eccpos[i]];
> > >     +
> > >     +       for (i = 0; eccsteps; eccsteps--, i += eccbytes, p +=
> > eccsize) {
> > >     +               int stat;
> > >     +
> > >     +               chip->ecc.hwctl(mtd, NAND_ECC_READ);
> > >     +               chip->read_buf(mtd, p, eccsize);
> > >     +               chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> > >
> > >
> > > Here you calculate ecc then never use the result?
> >
> > Hmm, that looks wrong, both here and in the davinci driver.  Are the two
> > calls to nand_davinci_4bit_readecc reading different things?  Does the
> > calculate function have any side effects beyond producing data that is
> > never used?
> Have you reads the patch description. Maybe that might help a bit
>
> This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
> support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
> chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
> read_page API that first reads the OOB area, reads the data in chunks,
> feeds the ECC from OOB area to the ECC hw engine and perform any
> correction on the data as per the ECC status reported by the engine.
>
> >
> > -Scott
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
>
>

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[John Rigby]

Thanks for the info.

On Tue, Sep 1, 2009 at 10:33 AM, Paulraj, Sandeep <s-paulraj@ti.com> wrote:

> This patch is fine.
>
> I'll send an updated NAND Davinci driver patch soon which adds to what is
> already there. I have made a mistake and testing did not show it.
>
> Have you noticed that not all e-mails reach the mailing list in the correct
> order? This is true for the conversation you two were having.
>
> Thanks,
> Sandeep
>
> > -----Original Message-----
> > From: Paulraj, Sandeep
> > Sent: Tuesday, September 01, 2009 12:04 PM
> > To: Scott Wood; John Rigby
> > Cc: u-boot at lists.denx.de; Paulraj, Sandeep; Narnakaje, Snehaprabha
> > Subject: RE: [U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode
> > NAND_ECC_HW_OOB_FIRST
> >
> >
> > > John Rigby wrote:
> > > > Sorry for the late comments.  We have been trying to use this code
> > with
> > > > the associated davinci 4-bit ecc patches and have some questions
> > We use this internally and it works. Are you having any issues because we
> > don't see any!!
> > J-C has to apply a patch and you will need that for this to work
> properly.
> > That patch updates the DM355 Config
> > > (inline).
> > > >
> > > >     .....
> > > >     +       uint8_t *ecc_code = chip->buffers->ecccode;
> > > >     +       uint32_t *eccpos = chip->ecc.layout->eccpos;
> > > >     +       uint8_t *ecc_calc = chip->buffers->ecccalc;
> > > >     +
> > > >     +       /* Read the OOB area first */
> > > >     +       chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> > > >
> > > >
> > > > What about chips that do not support the  NAND_CMD_READOOB?  Do I
> need
> > > > to provide my own read routine for that case?
> > >
> > > cmdfunc is supposed to fix that up.  This is already the case with
> > > existing code.
> > >
> > > >     +       chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> > > >     +       chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
> > > >     +
> > > >     +       for (i = 0; i < chip->ecc.total; i++)
> > > >     +               ecc_code[i] = chip->oob_poi[eccpos[i]];
> > > >     +
> > > >     +       for (i = 0; eccsteps; eccsteps--, i += eccbytes, p +=
> > > eccsize) {
> > > >     +               int stat;
> > > >     +
> > > >     +               chip->ecc.hwctl(mtd, NAND_ECC_READ);
> > > >     +               chip->read_buf(mtd, p, eccsize);
> > > >     +               chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> > > >
> > > >
> > > > Here you calculate ecc then never use the result?
> > >
> > > Hmm, that looks wrong, both here and in the davinci driver.  Are the
> two
> > > calls to nand_davinci_4bit_readecc reading different things?  Does the
> > > calculate function have any side effects beyond producing data that is
> > > never used?
> > Have you reads the patch description. Maybe that might help a bit
> >
> > This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
> > support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
> > chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
> > read_page API that first reads the OOB area, reads the data in chunks,
> > feeds the ECC from OOB area to the ECC hw engine and perform any
> > correction on the data as per the ECC status reported by the engine.
> >
> > >
> > > -Scott
> > > _______________________________________________
> > > U-Boot mailing list
> > > U-Boot at lists.denx.de
> > > http://lists.denx.de/mailman/listinfo/u-boot
>
>

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

Paulraj, Sandeep wrote:
>> John Rigby wrote:
>>> Sorry for the late comments.  We have been trying to use this code with
>>> the associated davinci 4-bit ecc patches and have some questions
> We use this internally and it works. Are you having any issues because we don't see any!!

Calm down, just because it works doesn't mean nobody should ask 
questions about the code.

>>> Here you calculate ecc then never use the result?
>> Hmm, that looks wrong, both here and in the davinci driver.  Are the two
>> calls to nand_davinci_4bit_readecc reading different things?  Does the
>> calculate function have any side effects beyond producing data that is
>> never used?
> Have you reads the patch description. Maybe that might help a bit
> 
> This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
> support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
> chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
> read_page API that first reads the OOB area, reads the data in chunks,
> feeds the ECC from OOB area to the ECC hw engine and perform any
> correction on the data as per the ECC status reported by the engine.

Yes, I've read that.  That doesn't explain why calculate_ecc is 
producing data, *after* everything has been read, that is never consumed 
by anything (hardware or software) AFAICT.  It doesn't explain why the 
"generic" code is passing NULL to ecc.correct.

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Matt Waddel]

Hi Scott and Sandeep,

As long as we're looking at these patches again I had a question about 
patch 1 of 2 in this series.  

The following part of the patch added a definition for: 

chip->ecc.read_page = nand_read_page_hwecc_oob_first

but since there aren't any "break" statements until "case NAND_ECC_SOFT:" 
it looks like the chip->ecc.read_page definition ends up being set to:

chip->ecc.read_page = nand_read_page_swecc.

Maybe this is explains why things seem to be working???

Best regards,
Matt

+   case NAND_ECC_HW_OOB_FIRST:
+           /* Similar to NAND_ECC_HW, but a separate read_page handle */
+           if (!chip->ecc.calculate || !chip->ecc.correct ||
+                !chip->ecc.hwctl) {
+                   printk(KERN_WARNING "No ECC functions supplied, "
+                          "Hardware ECC not possible\n");
+                   BUG();
+           }
+           if (!chip->ecc.read_page)
+                   chip->ecc.read_page = nand_read_page_hwecc_oob_first;
+


On Tuesday 01 September 2009 01:19:09 pm Scott Wood wrote:
> Paulraj, Sandeep wrote:
> >> John Rigby wrote:
> >>> Sorry for the late comments.  We have been trying to use this code with
> >>> the associated davinci 4-bit ecc patches and have some questions
> > We use this internally and it works. Are you having any issues because we don't see any!!
> 
> Calm down, just because it works doesn't mean nobody should ask 
> questions about the code.
> 
> >>> Here you calculate ecc then never use the result?
> >> Hmm, that looks wrong, both here and in the davinci driver.  Are the two
> >> calls to nand_davinci_4bit_readecc reading different things?  Does the
> >> calculate function have any side effects beyond producing data that is
> >> never used?
> > Have you reads the patch description. Maybe that might help a bit
> > 
> > This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
> > support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
> > chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
> > read_page API that first reads the OOB area, reads the data in chunks,
> > feeds the ECC from OOB area to the ECC hw engine and perform any
> > correction on the data as per the ECC status reported by the engine.
> 
> Yes, I've read that.  That doesn't explain why calculate_ecc is 
> producing data, *after* everything has been read, that is never consumed 
> by anything (hardware or software) AFAICT.  It doesn't explain why the 
> "generic" code is passing NULL to ecc.correct.
> 
> -Scott
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

Matt Waddel wrote:
> Hi Scott and Sandeep,
> 
> As long as we're looking at these patches again I had a question about 
> patch 1 of 2 in this series.  
> 
> The following part of the patch added a definition for: 
> 
> chip->ecc.read_page = nand_read_page_hwecc_oob_first
> 
> but since there aren't any "break" statements until "case NAND_ECC_SOFT:" 
> it looks like the chip->ecc.read_page definition ends up being set to:
> 
> chip->ecc.read_page = nand_read_page_swecc.
> 
> Maybe this is explains why things seem to be working???

No, that's OK -- it stops before that on the
"if (mtd->writesize >= chip->ecc.size) break;" line (or if that 
condition is not true, it prints a warning that it is falling back on 
soft ecc).

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

On Mon, Aug 10, 2009 at 01:27:56PM -0400, s-paulraj at dal.design.ti.com wrote:
> From: Sandeep Paulraj <s-paulraj@ti.com>
> 
> This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
> support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
> chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
> read_page API that first reads the OOB area, reads the data in chunks,
> feeds the ECC from OOB area to the ECC hw engine and perform any
> correction on the data as per the ECC status reported by the engine.
> 
> This patch has been accepted by Andrew Morton and can be found at
> 
> http://userweb.kernel.org/~akpm/mmotm/broken-out/mtd-nand-add-new-ecc-mode-ecc_hw_oob_first.patch
> 
> Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
> Signed-off-by: Sneha Narnakaje <nsnehaprabha@ti.com>

Applied to u-boot-nand-flash/next.

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

On Mon, Aug 10, 2009 at 01:27:56PM -0400, s-paulraj at dal.design.ti.com wrote:
>  /**
> + * nand_read_page_hwecc_oob_first - [REPLACABLE] hw ecc, read oob first
> + * @mtd:	mtd info structure
> + * @chip:	nand chip info structure
> + * @buf:	buffer to store read data
> + *
> + * Hardware ECC for large page chips, require OOB to be read first.

That statement may be true on some controllers, but not all.

> + * For this ECC mode, the write_page method is re-used from ECC_HW.
> + * These methods read/write ECC from the OOB area, unlike the
> + * ECC_HW_SYNDROME support with multiple ECC steps, follows the
> + * "infix ECC" scheme and reads/writes ECC from the data area, by
> + * overwriting the NAND manufacturer bad block markings.
> + */
> +static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd,
> +	struct nand_chip *chip, uint8_t *buf, int page)
> +{
> +	int i, eccsize = chip->ecc.size;
> +	int eccbytes = chip->ecc.bytes;
> +	int eccsteps = chip->ecc.steps;
> +	uint8_t *p = buf;
> +	uint8_t *ecc_code = chip->buffers->ecccode;
> +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> +	uint8_t *ecc_calc = chip->buffers->ecccalc;
> +
> +	/* Read the OOB area first */
> +	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);

Note that a READ0 command will have already been issued at this point. 

I guess it looks to the NAND chip as a zero-byte read, but still things
are getting quite ugly.  The "generic" interface is one big layering
violation that assumes a certain type of very simple controller, and
we're accumulating hacks to deal with less straightforward controllers.
:-(

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Paulraj, Sandeep]

> -----Original Message-----
> From: Scott Wood [mailto:scottwood at freescale.com]
> Sent: Tuesday, August 11, 2009 6:35 PM
> To: Paulraj, Sandeep
> Cc: u-boot at lists.denx.de; Narnakaje, Snehaprabha
> Subject: Re: [U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode
> NAND_ECC_HW_OOB_FIRST
> 
> On Mon, Aug 10, 2009 at 01:27:56PM -0400, s-paulraj at dal.design.ti.com
> wrote:
> >  /**
> > + * nand_read_page_hwecc_oob_first - [REPLACABLE] hw ecc, read oob first
> > + * @mtd:	mtd info structure
> > + * @chip:	nand chip info structure
> > + * @buf:	buffer to store read data
> > + *
> > + * Hardware ECC for large page chips, require OOB to be read first.
> 
> That statement may be true on some controllers, but not all.
[Sandeep] Yes, this is true for TI DaVinci controllers
> 
> > + * For this ECC mode, the write_page method is re-used from ECC_HW.
> > + * These methods read/write ECC from the OOB area, unlike the
> > + * ECC_HW_SYNDROME support with multiple ECC steps, follows the
> > + * "infix ECC" scheme and reads/writes ECC from the data area, by
> > + * overwriting the NAND manufacturer bad block markings.
> > + */
> > +static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd,
> > +	struct nand_chip *chip, uint8_t *buf, int page)
> > +{
> > +	int i, eccsize = chip->ecc.size;
> > +	int eccbytes = chip->ecc.bytes;
> > +	int eccsteps = chip->ecc.steps;
> > +	uint8_t *p = buf;
> > +	uint8_t *ecc_code = chip->buffers->ecccode;
> > +	uint32_t *eccpos = chip->ecc.layout->eccpos;
> > +	uint8_t *ecc_calc = chip->buffers->ecccalc;
> > +
> > +	/* Read the OOB area first */
> > +	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> 
> Note that a READ0 command will have already been issued at this point.
> 
> I guess it looks to the NAND chip as a zero-byte read, but still things
> are getting quite ugly.  The "generic" interface is one big layering
> violation that assumes a certain type of very simple controller, and
> we're accumulating hacks to deal with less straightforward controllers.
> :-(
[Sandeep] I agree.At the beginning of the year we had a way to do it with the existing modes. WE had are own read and write funtions in the Davinci NAND driver to achieve what were are doing in this patch. But the solution was not accepted by the MTD community. I have given the link for the original kernel patches. This current solution had the blessings of Thomas Gleixner and David Brownell.
> 
> -Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

Paulraj, Sandeep wrote:
> [Sandeep] I agree.At the beginning of the year we had a way to do it
> with the existing modes. WE had are own read and write funtions in
> the Davinci NAND driver to achieve what were are doing in this patch.
> But the solution was not accepted by the MTD community. I have given
> the link for the original kernel patches. This current solution had
> the blessings of Thomas Gleixner and David Brownell.

OK.  We'll do the same for now, then, but at some point (the 
ever-elusive "when I get time") I'd like to try to straighten out the 
interface, and shrink the footprint.

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

On Wed, Aug 12, 2009 at 11:48:27AM -0500, Paulraj, Sandeep wrote:
> There are other issues. The NAND manufacturers have changed the format
> of the 4Th ID byte and some other are supporting ONFI. Current NAND
> driver has no support for these

There are always other issues. :-)

Unless the issues show up on hardware I'm working with, there's not much
I can do other than apply patches from others.

> Shrinking the footprint will first need to be done in the MTD GIT and
> then the changes should trickle down to other trees including U-Boot.

That would be preferable, but we are not obligated to follow Linux's lead
if they aren't interested.

> So I take it that you will add these 2 patches to the u-boot-nand-flash/next git soon.

Assuming no other issues, yes.

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Paulraj, Sandeep]

> 
> There are always other issues. :-)
> 
> Unless the issues show up on hardware I'm working with, there's not much
> I can do other than apply patches from others.
> 
> > Shrinking the footprint will first need to be done in the MTD GIT and
> > then the changes should trickle down to other trees including U-Boot.
> 
> That would be preferable, but we are not obligated to follow Linux's lead
> if they aren't interested.
> 
> > So I take it that you will add these 2 patches to the u-boot-nand-
> flash/next git soon.
> 
> Assuming no other issues, yes.
Ok Thanks
> 
> -Scott

When you get time can you also review the following NAND specific patches
[PATCH] NAND: DaVinci: Adding 4 BIT ECC support
[PATCH] ARM: DaVinci DM355: Updating the DM355 EVM config

Thanks,
Sandeep

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[Scott Wood]

On Mon, Aug 10, 2009 at 01:27:56PM -0400, s-paulraj at dal.design.ti.com wrote:
> From: Sandeep Paulraj <s-paulraj@ti.com>
> 
> This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
> support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
> chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
> read_page API that first reads the OOB area, reads the data in chunks,
> feeds the ECC from OOB area to the ECC hw engine and perform any
> correction on the data as per the ECC status reported by the engine.

Is this going to be used by any other NAND controllers?  If it's not a
particularly common thing, perhaps the DaVinci NAND controller driver
should just override the read_page method.

Even if it's shared, the alternative read_page should probably be
provided in library form for the controller driver to use if desired.  We
should move most of the so-called generic NAND stuff in that direction;
currently it bloats u-boot even on hardware that doesn't need it.

> This patch has been accepted by Andrew Morton and can be found at
> 
> http://userweb.kernel.org/~akpm/mmotm/broken-out/mtd-nand-add-new-ecc-mode-ecc_hw_oob_first.patch

That's a testing tree -- it's not actually "accepted" if it's not in the
mtd tree (or Linus's).

> +	case NAND_ECC_HW_OOB_FIRST:
> +		/* Similar to NAND_ECC_HW, but a separate read_page handle */
> +		if (!chip->ecc.calculate || !chip->ecc.correct ||
> +		     !chip->ecc.hwctl) {
> +			printk(KERN_WARNING "No ECC functions supplied, "
> +			       "Hardware ECC not possible\n");
> +			BUG();
> +		}
> +		if (!chip->ecc.read_page)
> +			chip->ecc.read_page = nand_read_page_hwecc_oob_first;

Would anyone ever use this with read_page non-NULL?  It seems like its
whole point is to overide that.

-Scott

[U-Boot] [PATCH 2/2] MTD:NAND: ADD new ECC mode NAND_ECC_HW_OOB_FIRST

[s-paulraj at dal.design.ti.com]

From: Sandeep Paulraj <s-paulraj@ti.com>

This patch adds the new mode NAND_ECC_HW_OOB_FIRST in the nand code to
support 4-bit ECC on TI DaVinci devices with large page (up to 2K) NAND
chips.  This ECC mode is similar to NAND_ECC_HW, with the exception of
read_page API that first reads the OOB area, reads the data in chunks,
feeds the ECC from OOB area to the ECC hw engine and perform any
correction on the data as per the ECC status reported by the engine.

This patch has been accepted by Andrew Morton and can be found at

http://userweb.kernel.org/~akpm/mmotm/broken-out/mtd-nand-add-new-ecc-mode-ecc_hw_oob_first.patch

Signed-off-by: Sandeep Paulraj <s-paulraj@ti.com>
Signed-off-by: Sneha Narnakaje <nsnehaprabha@ti.com>
---
 drivers/mtd/nand/nand_base.c |   59 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |    1 +
 2 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ca02628..426bb95 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1068,6 +1068,54 @@ static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
 }
 
 /**
+ * nand_read_page_hwecc_oob_first - [REPLACABLE] hw ecc, read oob first
+ * @mtd:	mtd info structure
+ * @chip:	nand chip info structure
+ * @buf:	buffer to store read data
+ *
+ * Hardware ECC for large page chips, require OOB to be read first.
+ * For this ECC mode, the write_page method is re-used from ECC_HW.
+ * These methods read/write ECC from the OOB area, unlike the
+ * ECC_HW_SYNDROME support with multiple ECC steps, follows the
+ * "infix ECC" scheme and reads/writes ECC from the data area, by
+ * overwriting the NAND manufacturer bad block markings.
+ */
+static int nand_read_page_hwecc_oob_first(struct mtd_info *mtd,
+	struct nand_chip *chip, uint8_t *buf, int page)
+{
+	int i, eccsize = chip->ecc.size;
+	int eccbytes = chip->ecc.bytes;
+	int eccsteps = chip->ecc.steps;
+	uint8_t *p = buf;
+	uint8_t *ecc_code = chip->buffers->ecccode;
+	uint32_t *eccpos = chip->ecc.layout->eccpos;
+	uint8_t *ecc_calc = chip->buffers->ecccalc;
+
+	/* Read the OOB area first */
+	chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+	chip->cmdfunc(mtd, NAND_CMD_READ0, 0, page);
+
+	for (i = 0; i < chip->ecc.total; i++)
+		ecc_code[i] = chip->oob_poi[eccpos[i]];
+
+	for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) {
+		int stat;
+
+		chip->ecc.hwctl(mtd, NAND_ECC_READ);
+		chip->read_buf(mtd, p, eccsize);
+		chip->ecc.calculate(mtd, p, &ecc_calc[i]);
+
+		stat = chip->ecc.correct(mtd, p, &ecc_code[i], NULL);
+		if (stat < 0)
+			mtd->ecc_stats.failed++;
+		else
+			mtd->ecc_stats.corrected += stat;
+	}
+	return 0;
+}
+
+/**
  * nand_read_page_syndrome - [REPLACABLE] hardware ecc syndrom based page read
  * @mtd:	mtd info structure
  * @chip:	nand chip info structure
@@ -2730,6 +2778,17 @@ int nand_scan_tail(struct mtd_info *mtd)
 		chip->ecc.write_page_raw = nand_write_page_raw;
 
 	switch (chip->ecc.mode) {
+	case NAND_ECC_HW_OOB_FIRST:
+		/* Similar to NAND_ECC_HW, but a separate read_page handle */
+		if (!chip->ecc.calculate || !chip->ecc.correct ||
+		     !chip->ecc.hwctl) {
+			printk(KERN_WARNING "No ECC functions supplied, "
+			       "Hardware ECC not possible\n");
+			BUG();
+		}
+		if (!chip->ecc.read_page)
+			chip->ecc.read_page = nand_read_page_hwecc_oob_first;
+
 	case NAND_ECC_HW:
 		/* Use standard hwecc read page function ? */
 		if (!chip->ecc.read_page)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index d6aa392..cb7c19a 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -128,6 +128,7 @@ typedef enum {
 	NAND_ECC_SOFT,
 	NAND_ECC_HW,
 	NAND_ECC_HW_SYNDROME,
+	NAND_ECC_HW_OOB_FIRST,
 } nand_ecc_modes_t;
 
 /*
-- 
1.6.0.4