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

[Gupta, Pekon]

Hi,
Please see the replies inline..

> From: Scott Wood [mailto:scottwood at 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