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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
