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

[Scott Wood]

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?

> +   CONFIG_BCH
> +	Enables software based BCH ECC algorithm present in lib/bch.c
> +	This is used by SoC platforms which do not have in-build hardware
> +	engine to calculate and correct BCH ECC.

s/in-build/a built-in/

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

> +#define BADBLOCK_MARKER_LENGTH	0x2
> +#define SECTOR_BYTES		512

Why hex, especially since you don't use it on the larger number?
 
> +/**
> + * 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) {
> +	struct nand_bch_priv	*bch		= nand->priv;
> +	struct nand_ecclayout	*ecclayout	= nand->ecc.layout;
> +	int i;
> +
> +	/* Reset ecc interface */
> +	nand->ecc.mode			= NAND_ECC_NONE;
> +	nand->ecc.read_page		= NULL;
> +	nand->ecc.write_page		= NULL;
> +	nand->ecc.read_oob		= NULL;
> +	nand->ecc.write_oob		= NULL;
> +	nand->ecc.hwctl			= NULL;
> +	nand->ecc.correct		= NULL;
> +	nand->ecc.calculate		= NULL;
> +
> +	switch (ecc_scheme) {
> +	case OMAP_ECC_HAM1_CODE_SW:
> +		debug("nand: selected OMAP_ECC_HAM1_CODE_SW\n");
> +		nand->ecc.mode		= NAND_ECC_SOFT;
> +		nand->ecc.layout	= NULL;
> +		nand->ecc.size		= 0;
> +		nand->ecc.strength	= 1;
> +		bch->ecc_scheme		= OMAP_ECC_HAM1_CODE_SW;
> +		break;
> +	case OMAP_ECC_HAM1_CODE_HW_ROMCODE:
> +		debug("nand: selected OMAP_ECC_HAM1_CODE_HW_ROMCODE\n");
> +		nand->ecc.mode		= NAND_ECC_HW;
> +		nand->ecc.strength	= 1;
> +		nand->ecc.size		= 512;
> +		nand->ecc.bytes		= 3;
> +		nand->ecc.hwctl		= omap_enable_hwecc;
> +		nand->ecc.correct	= omap_correct_data;
> +		nand->ecc.calculate	= omap_calculate_ecc;
> +		/* define ecc-layout */
> +		ecclayout->eccbytes	= nand->ecc.bytes *
> +						(pagesize / SECTOR_BYTES);
> +		for (i = 0; i < ecclayout->eccbytes; i++)
> +			ecclayout->eccpos[i] = i +
> +						BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].offset = i +
> +						BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].length = oobsize - nand->ecc.bytes -
> +						BADBLOCK_MARKER_LENGTH;
> +		bch->ecc_scheme		= OMAP_ECC_HAM1_CODE_HW_ROMCODE;
> +		break;
> +#ifdef CONFIG_BCH
> +	case OMAP_ECC_BCH8_CODE_HW_DETECTION_SW:
> +		debug("nand: selected OMAP_ECC_BCH8_CODE_HW_DETECTION_SW\n");
> +		nand->ecc.mode		= NAND_ECC_HW;
> +		nand->ecc.strength	= 8;
> +		nand->ecc.size		= 512;
> +		nand->ecc.bytes		= 13;
> +		nand->ecc.hwctl		= omap_enable_ecc_bch;
> +		nand->ecc.correct	= omap_correct_data_bch_sw;
> +		nand->ecc.calculate	= omap_calculate_ecc_bch_sw;
> +		/* BCH SW library is used for error detection */
> +		bch_priv.control	= init_bch(13, 8, 0x201b);
> +		if (!bch_priv.control) {
> +			printf("nand: error: could not init_bch()\n");
> +			return -ENODEV;
> +		}
> +		/* define ecc-layout */
> +		ecclayout->eccbytes	= nand->ecc.bytes *
> +						(pagesize / SECTOR_BYTES);
> +		for (i = 0; i < ecclayout->eccbytes; i++)
> +			ecclayout->eccpos[i] = i + (oobsize -
> +						ecclayout->eccbytes);
> +		ecclayout->oobfree[0].offset = BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].length = oobsize - nand->ecc.bytes -
> +						BADBLOCK_MARKER_LENGTH;
> +		omap_hwecc_init_bch(nand, NAND_ECC_READ);
> +		bch->ecc_scheme		= OMAP_ECC_BCH8_CODE_HW_DETECTION_SW;
> +		break;
> +#endif
> +	case OMAP_ECC_BCH8_CODE_HW:
> +		debug("nand: selected OMAP_ECC_BCH8_CODE_HW\n");
> +		nand->ecc.mode		= NAND_ECC_HW;
> +		nand->ecc.strength	= 8;
> +		nand->ecc.size		= 512;
> +		nand->ecc.bytes		= 14;
> +		nand->ecc.hwctl		= omap_enable_ecc_bch;
> +		nand->ecc.correct	= omap_correct_data_bch;
> +		nand->ecc.calculate	= omap_calculate_ecc_bch;
> +		/* ELM is used for ECC error detection */
> +		elm_init();
> +		/* define ecc-layout */
> +		ecclayout->eccbytes	= nand->ecc.bytes *
> +						(pagesize / SECTOR_BYTES);
> +		for (i = 0; i < ecclayout->eccbytes; i++)
> +			ecclayout->eccpos[i] = i +
> +						BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].offset = i +
> +						BADBLOCK_MARKER_LENGTH;
> +		ecclayout->oobfree[0].length = oobsize - nand->ecc.bytes -
> +						BADBLOCK_MARKER_LENGTH;
> +		bch->ecc_scheme		= OMAP_ECC_BCH8_CODE_HW;
> +		break;
> +	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.

> @@ -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?
 
Better would be something like:
	printf("%s: no devices available\n", __func__);

>  	/* Setup the ecc configurations again */
> -	if (hardware) {
> +	if (hardware)
>  		if (eccstrength == 1) {

The brace is required due to multiline bodies.


> diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
> index e0a87f8..c2d25a4 100644
> --- a/include/configs/am335x_evm.h
> +++ b/include/configs/am335x_evm.h
> @@ -259,6 +259,8 @@
>  
>  #define CONFIG_SYS_NAND_ECCSIZE		512
>  #define CONFIG_SYS_NAND_ECCBYTES	14
> +#define CONFIG_SYS_NAND_ECCSCHEME	3
> +#define CONFIG_SYS_NAND_ONFI_DETECTION
>  
>  #define CONFIG_SYS_NAND_U_BOOT_START	CONFIG_SYS_TEXT_BASE
>  #define CONFIG_SYS_NAND_U_BOOT_OFFS	0x80000
> 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.

-Scott