[U-Boot] [PATCH v6 2/5] nand: lpc32xx: add hardware ECC support

[LEMIEUX, SYLVAIN]

> -----Original Message-----
> From: Vladimir Zapolskiy [mailto:vz at mleia.com]
>
> Hi Sylvain,
>
> On 10.08.2015 15:16, slemieux.tyco at gmail.com wrote:
> > From: Sylvain Lemieux <slemieux at tycoint.com>
> >
> > Incorporate NAND SLC hardware ECC support from legacy
> > LPCLinux NXP BSP.
> > The code taken from the legacy patch is:
> > - lpc32xx SLC NAND driver (hardware ECC support)
> > - lpc3250 header file missing SLC NAND registers definition
> >
> > The legacy driver code was updated to integrate with the existing NAND SLC driver.
> >
> > Signed-off-by: Sylvain Lemieux <slemieux at tycoint.com>
> > ---

[...]

> >
> > diff --git a/drivers/mtd/nand/lpc32xx_nand_slc.c b/drivers/mtd/nand/lpc32xx_nand_slc.c
> > index 719a74d..2a32264 100644
> > --- a/drivers/mtd/nand/lpc32xx_nand_slc.c
> > +++ b/drivers/mtd/nand/lpc32xx_nand_slc.c
> > @@ -3,15 +3,48 @@
> >   *

[...]

> > +
> > +#define CONFIG_SYS_NAND_ECCBYTES   3
> > +#define CONFIG_SYS_NAND_ECCSIZE            256
>
> These defines are OK, but see my bottom comment below related to this
> subject.
>
> Also it's worth to mention that these defines are conflicting with the
> same defines from update to my board:
> http://lists.denx.de/pipermail/u-boot/2015-July/219251.html -- I don't
> understand why my changes are still not present in U-boot/master e.g. to
> catch this kind of problems.
>

> So, CONFIG_SYS_NAND_ECCSIZE, CONFIG_SYS_NAND_ECCBYTES and
> CONFIG_SYS_NAND_OOBSIZE are used outside of the driver code (to be
> precise they are used in drivers/mtd/nand/nand_spl_simple.c), and
> therefore this is not the right place to define them IMHO.

I missed the change; I applied only patch 2/4 of your series (NAND SLC driver).

I can add an #if !defined() statement for both of them, as it was in previous
version of the patch. This way, you don't need to define them in your board
config if you are not using the SPL.

>
> >  struct lpc32xx_nand_slc_regs {
> >     u32 data;
> > @@ -33,11 +66,18 @@ struct lpc32xx_nand_slc_regs {
> >
[...]
> >
> > +#if defined(CONFIG_DMA_LPC32XX)
> > +/* Total ECC bytes and size; refer to board_nand_init() for details. */
> > +#define ECCSTEPS   (CONFIG_SYS_NAND_PAGE_SIZE / CONFIG_SYS_NAND_ECCSIZE)
> > +#define TOTAL_ECCBYTES     (CONFIG_SYS_NAND_ECCBYTES * ECCSTEPS)
> > +#define TOTAL_ECCSIZE      (CONFIG_SYS_NAND_ECCSIZE * ECCSTEPS)
>
> See my comment below regarding these two defines.
>

[...]

> > +
> > +   /*
> > +    * ECC correction is done by page when using the DMA controller
> > +    * scatter/gather mode through linked list;
> > +    * refer to UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3
> > +    * section 9.7 and tables 173 notes for details.
> > +    *
> > +    * The error correction is done on each page and process in multiple
> > +    * steps of 256 bytes inside the driver.
> > +    *
> > +    * The total ECC size and bytes for a page are used;
> > +    * NAND base code (HW ECC) will call the read / write buffer functions
> > +    * using the total ECC size and bytes for a page as a single step.
> > +    */
> > +   lpc32xx_chip->ecc.size          = TOTAL_ECCSIZE;
> > +   lpc32xx_chip->ecc.bytes         = TOTAL_ECCBYTES;
>
> I still don't quite understand, why TOTAL_ECCSIZE and TOTAL_ECCBYTES are
> not equal to CONFIG_SYS_NAND_ECCSIZE and CONFIG_SYS_NAND_ECCBYTES
> correspondingly. IOW why total values are used here?
>
> This indicates a bug.
>

The ECC correction is done by page when using the DMA controller
scatter/gather mode through linked list; the DMA is configured to
process 2 (small) or 8 (large) pages of 256 bytes and the ECC read
without any software intervention, resulting in a single DMA transfer.

As per the "LPC32x0 and LPC32x0/01 User manual"  table 173 notes and
section 9.7, the NAND SLC & DMA allowed single DMA transaction of a
page size (512 or 2048) that manage the ECC within that single transaction,
resulting in an ECCSIZE of 512 (small page) or 2048 (large page) size.

[...]

>
> --
> With best wishes,
> Vladimir

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.