[RFC] mtd: nand: Preparatory patch for adding on-die ECC controller support. This patch adds NAND_ECC_HW_ON_DIE and all other changes to generic code.
Gupta, Pekon
pekon at ti.com
Fri Mar 28 01:48:23 EDT 2014
Hi David,
>From: David Mosberger
>Pekon,
>
>Before I go any further with this, could you confirm that what is
>below is what you had in mind as far as the generic portion of the
>patch is concerned. If so, I'll go ahead and create the second,
>Micron-specific part next.
>
>Thanks,
>
> --david
>
>PS: This patch adds a one-liner to of_mtd.c that I forgot about previously.
>
>Signed-off-by: David Mosberger <davidm at egauge.net>
>---
> drivers/mtd/nand/nand_base.c | 36 +++++++++++++++++++++++++++++++++---
> drivers/of/of_mtd.c | 1 +
> include/linux/mtd/nand.h | 7 +++++++
> 3 files changed, 41 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>index 5826da3..b94e2e9 100644
>--- a/drivers/mtd/nand/nand_base.c
>+++ b/drivers/mtd/nand/nand_base.c
>@@ -3783,22 +3783,46 @@ EXPORT_SYMBOL(nand_scan_ident);
> int nand_scan_tail(struct mtd_info *mtd)
> {
> int i;
>+ u8 features[ONFI_SUBFEATURE_PARAM_LEN];
> struct nand_chip *chip = mtd->priv;
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> struct nand_buffers *nbuf;
>
>+ if (chip->onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_OP_MODE,
>+ features) >= 0) {
>+ if (features[0] & ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC) {
>+ /*
>+ * If the chip has on-die ECC enabled, we kind
>+ * of have to do the same...
>+ */
>+ chip->ecc.mode = NAND_ECC_HW_ON_DIE;
>+ pr_info("Using on-die ECC\n");
>+ }
>+ }
>+
Here, I'm bit skeptical on enabling "NAND_ECC_HW_ON_DIE" mode automatically,
based on ONFI parameters. ONFI params just advertise the current NAND device's
capability and features, whether to use that feature or not should be under
control of individual vendor drivers or passed by user via DT | platform-data.
Example: "MT29F4G16ABADAWP" is supported on-die 4-bit ECC correction. But user
can use 8-bit ECC correction on same device, using ECC-schemes supported by
vendor controller driver.
(1) So it would be nice if instead of enabling chip->ecc.mode=NAND_ECC_HW_ON_DIE
in nand_scan_tail(), you advertise the availability of this feature in nand_scan_ident(),
and then let user choose whether to enable 'on-die' ECC or not.
(2) At-least in the datasheet "m60a_4gb_8gb_16gb_ecc_nand.pdf" I have,
- "internal ECC" is _not_ mentioned in vendor specific ONFI params. Rather
- "internal ECC" is mentioned in 4th byte of READ_ID command.
So, I don't know if using chip->onfi_get_feature() is correct API to use here,
because it uses NAND_CMD_GET_FEATURES (0xeeh) not READ_ID (0x90h)
(3) If "internal ECC" is mentioned via some vendor specific ONFI params,
Then you should put this code under nand_base.c : nand_onfi_detect_micron()
> /* New bad blocks should be marked in OOB, flash-based BBT, or both */
> BUG_ON((chip->bbt_options & NAND_BBT_NO_OOB_BBM) &&
> !(chip->bbt_options & NAND_BBT_USE_FLASH));
>
> if (!(chip->options & NAND_OWN_BUFFERS)) {
>+ size_t on_die_bufsz = 0;
>+
>+ if (chip->ecc.mode == NAND_ECC_HW_ON_DIE)
>+ on_die_bufsz = 2*(mtd->writesize + mtd->oobsize);
>+
> nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
>- + mtd->oobsize * 3, GFP_KERNEL);
>+ + mtd->oobsize * 3 + on_die_bufsz, GFP_KERNEL);
> if (!nbuf)
> return -ENOMEM;
> nbuf->ecccalc = (uint8_t *)(nbuf + 1);
> nbuf->ecccode = nbuf->ecccalc + mtd->oobsize;
> nbuf->databuf = nbuf->ecccode + mtd->oobsize;
>+ if (chip->ecc.mode == NAND_ECC_HW_ON_DIE) {
>+ nbuf->chkbuf = (nbuf->databuf + mtd->writesize
>+ + mtd->oobsize);
>+ nbuf->rawbuf = (nbuf->chkbuf + mtd->writesize
>+ + mtd->oobsize);
>+ }
>
> chip->buffers = nbuf;
> } else {
>@@ -3956,6 +3980,7 @@ int nand_scan_tail(struct mtd_info *mtd)
> ecc->strength = ecc->bytes * 8 / fls(8 * ecc->size);
> break;
>
>+ case NAND_ECC_HW_ON_DIE:
> case NAND_ECC_NONE:
> pr_warn("NAND_ECC_NONE selected by board driver. "
> "This is not recommended!\n");
>@@ -4023,8 +4048,13 @@ int nand_scan_tail(struct mtd_info *mtd)
> /* Invalidate the pagebuffer reference */
> chip->pagebuf = -1;
>
>- /* Large page NAND with SOFT_ECC should support subpage reads */
>- if ((ecc->mode == NAND_ECC_SOFT) && (chip->page_shift > 9))
>+ /*
>+ * Large page NAND with SOFT_ECC or on-die ECC should support
>+ * subpage reads.
>+ */
>+ if (((ecc->mode == NAND_ECC_SOFT)
>+ || (chip->ecc.mode == NAND_ECC_HW_ON_DIE))
>+ && (chip->page_shift > 9))
> chip->options |= NAND_SUBPAGE_READ;
>
> /* Fill in remaining MTD driver data */
>diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c
>index b7361ed..c844c84 100644
>--- a/drivers/of/of_mtd.c
>+++ b/drivers/of/of_mtd.c
>@@ -23,6 +23,7 @@ static const char *nand_ecc_modes[] = {
> [NAND_ECC_HW_SYNDROME] = "hw_syndrome",
> [NAND_ECC_HW_OOB_FIRST] = "hw_oob_first",
> [NAND_ECC_SOFT_BCH] = "soft_bch",
>+ [NAND_ECC_HW_ON_DIE] = "hw_on_die",
> };
>
> /**
>diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>index 450d61e..a1cc980 100644
>--- a/include/linux/mtd/nand.h
>+++ b/include/linux/mtd/nand.h
>@@ -115,6 +115,7 @@ typedef enum {
> NAND_ECC_HW_SYNDROME,
> NAND_ECC_HW_OOB_FIRST,
> NAND_ECC_SOFT_BCH,
>+ NAND_ECC_HW_ON_DIE,
> } nand_ecc_modes_t;
>
> /*
>@@ -214,6 +215,10 @@ struct nand_chip;
> /* Vendor-specific feature address (Micron) */
> #define ONFI_FEATURE_ADDR_READ_RETRY 0x89
>
>+/* Vendor-specific array operation mode (Micron) */
>+#define ONFI_FEATURE_ADDR_OP_MODE 0x90
>+#define ONFI_FEATURE_OP_MODE_ENABLE_ON_DIE_ECC 0x08
>+
> /* ONFI subfeature parameters length */
> #define ONFI_SUBFEATURE_PARAM_LEN 4
>
>@@ -516,6 +521,8 @@ struct nand_buffers {
> uint8_t *ecccalc;
> uint8_t *ecccode;
> uint8_t *databuf;
>+ uint8_t *chkbuf;
>+ uint8_t *rawbuf;
> };
>
I'm not sure if its good to extend struct nand_buffers, you could have
as well re-used nbuf->databuf, instead of nbuf->chkbuf, as you only
need to get number of bit-flips. Right ?
And again re-use nbuf->databuf on re-reads, in read_page_on_die().
I tried to be elaborate here, so that it cuts down the number of iterations
of your patch. So if you have any disagreements or discussion, lets
clear that out before you send next patch version.
with regards, pekon
More information about the linux-mtd
mailing list