[U-Boot] [PATCH v4 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform
Scott Wood
scottwood at freescale.com
Mon Sep 16 23:14:44 CEST 2013
On Fri, 2013-09-13 at 07:02 +0000, Gupta, Pekon wrote:
> (resending to u-boot maillist as previous mail was discarded by mailman
> due to mail-encoding issue.)
>
> > 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
Oops... I looked for it and didn't see it, but maybe I was looking at
the Linux version or something similarly silly.
> > > + 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.
> >
> This is generic define which can be used by all NAND drivers to specify
> ECC scheme for SPL boot. Therefore CONFIG_SYS_
> How each device/driver will use it depends should be mentioned in
> its board level document. For AM335x its updated in Patch:
> [PATCH v5 5/5] board/ti/am335x/README: update for NAND boot
> Do you want it to be made omap specific ?
Yes, it should be omap specific. If the semantics are not generic I
don't see the value in making the name be generic and splitting the
documentation in two places.
> > > +/**
> > > + * 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.
> >
> 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 ?
Before you get into the switch statement you set ecc.mode to
NAND_ECC_NONE and wipe out all of the function pointers. Then, if the
switch hits default:, you'll return leaving the ecc in a bad state,
relying on the caller to clean it up.
> > > @@ -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.
>
> > Better would be something like:
> > printf("%s: no devices available\n", __func__);
Still, it's best for error messages to be unique and include the
function name.
> > > #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
Patches are generally reviewed in order. Don't introduce something bad
in 1/5 and then fix it in 5/5, even if it's just a style issue that
doesn't affect bisect.
-Scott
More information about the U-Boot
mailing list