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

[Gupta, Pekon]

> From: Scott Wood [mailto:scottwood at freescale.com]
> > 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.
> 
Ok, accepted.


> >  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.
> 
Don't know if its short-coming of device-tree or difference between
u-boot and kernel ideologies. But use of internal symbols (enums) in DT
was  opposed by one of the kernel maintainers. Refer below:
http://lists.infradead.org/pipermail/linux-mtd/2013-May/047030.html


> > > > +/**
> > > > + * 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.
> 
Having 2 switch statements would clutter the code. Isn't it ?
(1) first switch-case would do a dry-run to check the ecc requirements.
(2) second switch-case would do actual assignments.

And omap_select_ecc_scheme() would be called only once in whole
lifetime, because:
(1) omap devices have deprecated the dynamic switching of ecc-schemes.
(2) with falcon-mode where SPL directly loads kernel, most driver configs
   would be statically derived from CONFIGS_xx.

Anyways I would take the changes if you wish so..
But request you to please provide comments on all the patches, before
I send next revision. This would help me consolidate all changes.


> > > > +		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().
> 
As described above, this has dependency on ecc information
So again I have to go by 2 switch statement approach which would
clutter the code. I'm okay if you want it so..

But request you to review and provide comments to other patches
of this series, OR else atleast Ack/Nak them. This way my effort on
re-working and testing the patches again and again would reduce.

So, should I wait for review of other remaining patches before
sending next version v8 of this series ?


with regards, pekon