[U-Boot] [PATCH v1 1/4] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform

[Scott Wood]

On Mon, 2013-08-12 at 13:31 +0000, Gupta, Pekon wrote:
> Hi,
> > 
> > On Tue, 2013-08-06 at 15:25 +0530, Pekon Gupta wrote:
> > > This patch
> > > - replaces CONFIG_AM33xx define with generic
> > CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
> > >   so that all device families having required h/w capability can use ELM for
> > >   error detection in ECC_BCHx schemes.
> > >
> > > - replaces CONFIG_NAND_OMAP_BCH8 with
> > CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
> > >   and separates out code for above mentioned BCH8_ECC implementations
> > so that
> > >   driver can be build independently using anyone of them.
> > 
> > Please document these CONFIG symbols in the README.
> > 
> [Pekon]: should I add them to doc/README.nand ? 

Yes.

> Also, in broad sense these define suggest which flavor of ECC scheme to use
> BCH8_HW, BCH8_SW, BCH4_HW, BCH4_SW, etc..
> So in-order to make them generic for use across all vendors I can rename
> them to CONFIG_SYS_NAND_ECC_xx (omitting OMAP in name).

I don't think this would be appropriate, especially with the "SYS" in
the name.  The generic approach is that this is configured at runtime.
Whether a specific controller driver uses CONFIG symbols or other
mechanisms to determine what sort of ECC to use is up to the controller
driver (most controller drivers probably only support one type for any
given type of NAND chip).

Plus, this way you only need to focus on the options that are relevant
to OMAP.

> > Is the choice of ECC mode dictated by the hardware, or is it software's
> > choice?  If the former, it should be CONFIG_SYS rather than just CONFIG.
> > 
> [Pekon] Some older OMAP device do not support ELM hardware. So in those
> CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW is default.
> In newer devices, supporting ELM hardware
> CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW is default.

What does "DETECTION_SW" mean?

> > > -#ifdef CONFIG_AM33XX
> > > +#if defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW)
> > >  #include <asm/arch/elm.h>
> > > +#elif
> > defined(CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW)
> > > +#include <linux/bch.h>
> > >  #endif
> > 
> > Normally includes don't get ifdeffed...  and what is the connection
> > between a particular ECC mode and asm/arch/elm.h?
> > 
> [Pekon] Both the Config do the same BCH8 ECC scheme, but implementation
> is different. One uses S/W library, while other uses ELM hardware engine.

Why can't you include both headers?  They don't appear to be exposing
the same interface...

> - CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
> Include API for S/W library (lib/bch.c) so included that here..
> But this ECC scheme should be used only for older OMAP platforms where
>  ELM is not present
> 
> - CONFIG_NAND_OMAP_ECC_BCH8_CODE_HW
> Include declarations for ELM functions. So new OMAP platforms which have
> ELM hardware engine, need not include whole bch.c library in their code.

Maybe it should just be something like CONFIG_SYS_OMAP_ELM?

-Scott