Hi,
Please see the replies inline..
> From: Scott Wood [mailto:scottw...@freescale.com]
> > On Mon, 2013-09-30 at 19:43 +0530, Pekon Gupta wrote:
> > +Platform specific options
> > +=========================
> > +
> > + CONFIG_NAND_OMAP_ECCSCHEME
> > + On OMAP platforms, this specifies NAND ECC scheme.
> > + 1 - HAM1_SW 1-bit Hamming code using software library
> > + (for legacy devices only)
> > + 2 - HAM1_HW 1-bit Hamming code using GPMC hardware engine
> > + (for legacy devices only)
> > + 3 - BCH4_SW 4-bit BCH code (unsupported)
> > + 4 - BCH4_HW 4-bit BCH code (unsupported)
> > + 5 - BCH8_SW 8-bit BCH code with
> > + - ecc calculation using GPMC hardware engine,
> > + - error detection using software library.
> > + - requires CONFIG_BCH to enable software BCH
> library
> > + (For legacy device which do not have ELM h/w
> engine)
> > + 6 - BCH8_HW 8-bit BCH code with
> > + - ecc calculation using GPMC hardware engine,
> > + - error detection using ELM hardware engine.
>
> You should document the symbols, not the numbers that happen to be
> assigned to them.
>
Sorry din't get you. This is based on your below feedback
http://lists.denx.de/pipermail/u-boot/2013-September/162773.html
Example: "6 - BCH8_HW" means 8-bit BCH ECC scheme using h/w engine.
It is this number is what user needs to specify in include/configs/*.h
Any other internal symbol like "OMAP_ECC_BCH8_CODE_SW" should
not be exposed to user, user-interface should remain constant. This is
similar to DT binding approach used in linux. Internal symbols are not
exposed to users.
> > +/**
> > + * omap_select_ecc_scheme - configures driver for particular ecc-scheme
> > + * @nand: NAND chip device structure
> > + * @ecc_scheme: ecc scheme to configure
> > + * @pagesize: number of main-area bytes per page of NAND device
> > + * @oobsize: number of OOB/spare bytes per page of NAND device
> > + */
> > +static int omap_select_ecc_scheme(struct nand_chip *nand, int
> ecc_scheme,
> > + unsigned int pagesize, unsigned int oobsize) {
>
> s/int ecc_scheme/enum omap_ecc ecc_scheme/
>
If this is only the cosmetic change, may be I'll take it separately
in another patch. Also 'omap_select_ecc_scheme()' has default
statement in last, which gracefully handles all un-supported ecc-schemes.
[snip]
> > + /* check if NAND spare/OOB has enough bytes to accomodate
> ecclayout */
> > + if ((ecclayout->eccbytes + BADBLOCK_MARKER_LENGTH) > oobsize)
> {
> > + printf("nand: error: insufficient OOB bytes. require=%d\n", (
> > + ecclayout->eccbytes +
> BADBLOCK_MARKER_LENGTH));
> > + return -EINVAL;
> > + }
>
> Check this before you make any changes to the current ECC setup.
>
'ecclayout->eccbytes' depends on ECC scheme selected, therefore
this check cannot be done before selecting ECC scheme first.
> > + err = omap_select_ecc_scheme(nand,
> OMAP_ECC_HAM1_CODE_SW,
> > + mtd->writesize, mtd->oobsize);
> > + }
> > + if (err) {
> > + printf("nand: error: could not switch ecc, reverting\n");
> > + omap_select_ecc_scheme(nand, bch->ecc_scheme,
> > + mtd->writesize, mtd->oobsize);
> > + return -EINVAL;
> > }
>
> You won't need to "revert" here if omap_select_ecc_scheme doesn't
> damage anything in error cases.
>
There are two cases when ECC selection could fail half-way:
(1) when if(ecclayout->eccbytes + BADBLOCK > oobsize), as this check
can only be done after selecting ECC scheme so it can fail
(2) if 'bch_priv.control = init_bch(13, 8, 0x201b);' fails.
This check is also is done during ecc-scheme selection.
In both above cases, nand_chip.ecc information is half-configured
so this reverting back to old ecc-scheme is essential.
with regards, pekon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
