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

[Scott Wood]

On Tue, 2013-10-08 at 05:30 +0000, Gupta, Pekon wrote:
> > 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

It's a different situation.  The device tree is supposed to be stable
ABI.  This isn't.  The device tree is also supposed to be OS-independent
and thus taking Linux identifiers as-is is frowned upon (as the Linux
identifiers may change, plus device tree properties use a different
style for naming).

It's not a general difference between U-Boot and Linux.

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

It wouldn't be that bad.  It would eliminate the need for recovery code
and substantially reduce the complexity of a (likely little-tested, at
least long term) error path.

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

I'm working on it, but I also have a lot of other patches to review, and
doing nothing but patch reviews all day long can be very draining (not
to mention starving other tasks that need to be done), and reviewing
patches for hardware I'm not familiar with is even worse.  I realize
that the delays can be frustrating from your end, but I only have so
much time and energy to expend.  Is there nobody else who's familiar
with this driver that can help review?  Looking at the commit history no
single person stands out as a logical maintainer for this driver.  It
would be nice to designate someone.

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

Yes.

-Scott