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

[Gupta, Pekon]

(apologies for multiple mails, but mails from my client were rejected
 due to some client configuration)


> On Tue, 2013-09-03 at 11:26 +0530, Pekon Gupta wrote:
> > diff --git a/doc/README.nand b/doc/README.nand
> > index 913e9b5..f72f618 100644
> > --- a/doc/README.nand
> > +++ b/doc/README.nand
> > @@ -169,6 +169,19 @@ Configuration Options:
> >        Please convert your driver even if you don't need the extra
> >        flexibility, so that one day we can eliminate the old mechanism.
> >
> > +   CONFIG_SYS_NAND_ONFI_DETECTION
> > +	Enables detection of ONFI compliant devices during probe.
> > +	And fetching device parameters flashed on device, by parsing
> > +	ONFI parameter page.
> 
> I don't see this used anywhere in the patch (defined in config headers,
> yes, but not ifdeffed).
> 
> What does "DETECTION" add here, versus just CONFIG_NAND_ONFI?
> 
> It should be CONFIG rather than CONFIG_SYS because it just controls
> whether the support is built, not making an assertion about the
> hardware, right?
> 
This is not the new define, it was already there in generic code (nand_base.c)
But not documented, so I documented it and enabled it for am33xx platform
Please refer: $UBOOT/drivers/mtd/nand/nand_base.c
It was added as part of following commit
commit 0272c718ba69c60a9d719db6806971d98db98090
Author:     Florian Fainelli <florian at openwrt.org>
AuthorDate: 2011-02-25

> 
> > +   CONFIG_SYS_NAND_ECCSCHEME
> > +	specifies which ECC scheme to use.
> 
> Specifies it how?  What are the possible values?
> 
> If the answer involves "enum omap_ecc", then OMAP should be
>  somewhere in the name.
> 
I wanted this define to be generic for all NAND drivers. Therefore CONFIG_SYS_
But I don't know how other drivers would use this.
For AM335x its updated in Patch:
[PATCH v5 5/5] board/ti/am335x/README: update for NAND boot 

Instead, should I make it more generic for all SoC ? like
CONFIG_SYS_NAND_ECCSCHEME can take in following strings defining
ECC scheme used by NAND driver in SPL boot:
"ham1" - 1 bit hamming code
"bch4"  - 4-bit BCH ecc code
"bch8"  - 8-bit BCH ecc code
"bch16"  - 16-bit BCH ecc 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
> > + */
[snip]
> > +	default:
> > +		debug("nand: error: ecc scheme not enabled or
> supported\n");
> > +		return -EINVAL;
> > +	}
> 
> This will result in NAND_ECC_NONE and likely an eventual NULL pointer
> dereference if a bad value is passed and the -EINVAL return path is
> taken.
> 
> OK, I now see that you've got the caller patching this up in the error
> case, but that's awkward and error prone.
> 
Sorry, din't understand your feedback here.
omap_select_ecc_scheme() is common function which is used for
- changing ECC scheme if 'nandecc' like utility is needed later
- selecting ECC scheme during probe.
This function populates all necessary fields in struct *nand_chip
for nand driver to work. During probe (both SPL and u-boot), 
'CONFIG_SYS_NAND_ECCSCHEME' is used to determine the ecc-scheme.

'default' statement protects the driver from any invalid or un-supported
ECC scheme selected by user in his board file. So that driver probe cleanly
exits probe instead of causing an exception.
Can you please elaborate, what I'm missing here ?

> > @@ -767,67 +811,41 @@ void omap_nand_switch_ecc(uint32_t hardware,
> uint32_t eccstrength)
> >  {
> >  	struct nand_chip *nand;
> >  	struct mtd_info *mtd;
> > +	struct nand_bch_priv *bch;
> > +	int err = 0;
> >
> >  	if (nand_curr_device < 0 ||
> >  	    nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
> >  	    !nand_info[nand_curr_device].name) {
> > -		printf("Error: Can't switch ecc, no devices available\n");
> > +		printf("nand: error: no devices available\n");
> >  		return;
> >  	}
> 
> Why are you making the error message less specific?
> 
There is already another error message in board_nand_init() which will be
flagged when there is problem in selecting ECC.
printf("nand: error: cannot select ecc scheme\n");
But thanks, you pointed out a bug, I should return -ENODEV here,
Otherwise caller would not know that ecc-scheme could not be changed.


> > diff --git a/include/configs/tricorder.h b/include/configs/tricorder.h
> > index a9b2714..2b01486 100644
> > --- a/include/configs/tricorder.h
> > +++ b/include/configs/tricorder.h
> > @@ -298,6 +298,8 @@
> >
> >  #define CONFIG_SYS_NAND_ECCSIZE		512
> >  #define CONFIG_SYS_NAND_ECCBYTES	13
> > +#define CONFIG_SYS_NAND_ECCSCHEME	3
> > +#define CONFIG_SYS_NAND_ONFI_DETECTION
> 
> What does 3 mean?  Please use a symbolic name.
> 
For AM335x this is updated in board/ti/am335x/README as part of
following patch:
[PATCH v5 5/5] board/ti/am335x/README: update for NAND boot
but would it be better to use strings like "bch8" as mentioned above ?

with regards, pekon