Re: [U-Boot] [PATCH v4 1/5] mtd: nand: omap: enable BCH ECC scheme using ELM for generic platform
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@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: selectedOMAP_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: selectedOMAP_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 orsupported\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");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
participants (1)
-
Scott Wood