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

[Scott Wood]

On Sat, 2013-10-05 at 06:11 +0000, Gupta, Pekon wrote:
> 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.

I disagree.  The user should specify OMAP_ECC_BCH8_CODE_SW.  It's more
greppable, and it's more likely that the number will change than that
the name will.

>  This is similar to DT binding approach used in linux. Internal symbols are not
> exposed to users.

This is a shortcoming of device trees that we should not emulate in C
code.

> > > +/**
> > > + * 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.

It certainly can be done before you make any changes to the ECC struct.

You could either do it in each individual case, or you could have two
separate switch statements with this code in between them.

> > > +		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.

If you really can't avoid the damage in error cases, move the cleanup
code into omap_select_ecc_scheme().

-Scott