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

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

[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@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");
+		}
+	}
+
 	/* 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;
 };
 
 /**
-- 
1.7.9.5

RE: [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]

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@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

Re: [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.

[Gerhard Sittig]

On Fri, 2014-03-28 at 05:48 +0000, Gupta, Pekon wrote:
> 
> 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()

There are two things here.  With chip identification you can
learn whether the chip's model _supports_ on-die ECC.  With the
feature get/set requests, you can learn and adjust whether this
mode is _enabled_ within the chip.  Without support you cannot
enable the feature (will reflect "off" upon re-read).  But with
support, it's orthogonal whether the feature is in use.

The motivation might be that when some previous stage in the boot
process did enable on die ECC mode, you have to "pick up" there
and keep using it.  There still is the option to disable it for
yourself and continue without ECC or with differing methods.  But
at first you find the chip using it, which puts constraints on
how you talk to the chip.

There too is the option to find the chip in a specific mode, and
already knowing whether the user wants to run it in this mode,
and adjusting subsequent operation early, during detection
already.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [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.

[David Mosberger]

Pekon,

On Thu, Mar 27, 2014 at 11:48 PM, Gupta, Pekon <pekon@ti.com> wrote:

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

No, the OP_MODE register reflects the actual operational mode of the chip,
it's not a capability flag.  You can't ignore the chip being on
"Internal ECC" mode:
you either have to use on-die ECC or turn it off.  If you ignore it
when it's on and
try to use a different ECC scheme, all hell will break lose, because the chip
will corrupt the OOB area with its own ECC bytes.

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

Again, we are not enabling on-die ECC unsolicited *ever* in that patch.
We just *detect* that it's enabled (by bootstrap loader or at
manufacturing time) and then act accordingly.

> (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)

Yes, READ_ID also has an "Internal ECC" flag.

However, look on page 50, table 14: Array Operation Mode.  It documents the
Disable/Enable ECC bit that can be read via GET FEATURES and
set via SET FEATURES.

I can assure you, the patch I sent actually does work! ;-)

> (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()

Yes, I don't see why that wouldn't work.  nand_onfi_detect_micron() appears
to be a relatively recent addition that I wasn't aware of.  Let me try that.

>>@@ -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().

In theory, yes.  In practice, it got way too complicated.  AFAIR, databuf
may not contain the entire page + OOB, so in general we have to
re-read (some) of the data some of the time anyhow.  It's much more
reliable and cleaner to have separate buffers.  That way, it's guaranteed
that we don't disturb the contents of databuf.

Remember that only gets called for pages that have bitflips, so it's
relatively rare and from a performance-perspective, the extra work
is irrelevant.

Also, the extra buffers only get allocated when internal ECC is
enabled, so it has no discernible effect on other configurations.

  --david
-- 
eGauge Systems LLC, http://egauge.net/, 1.877-EGAUGE1, fax 720.545.9768

Re: [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.

[Brian Norris]

On Fri, Mar 28, 2014 at 10:19:17AM -0600, David Mosberger wrote:
> On Thu, Mar 27, 2014 at 11:48 PM, Gupta, Pekon <pekon@ti.com> wrote:
> >>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,

I am also skeptical, but for different reasons.

> > 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.
> 
> No, the OP_MODE register reflects the actual operational mode of the chip,
> it's not a capability flag.  You can't ignore the chip being on
> "Internal ECC" mode:
> you either have to use on-die ECC or turn it off.  If you ignore it
> when it's on and
> try to use a different ECC scheme, all hell will break lose, because the chip
> will corrupt the OOB area with its own ECC bytes.

OK, but I think turning it off makes a better default.

> > (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.
> 
> Again, we are not enabling on-die ECC unsolicited *ever* in that patch.
> We just *detect* that it's enabled (by bootstrap loader or at
> manufacturing time) and then act accordingly.

Consider this: what if the NAND is not used or configured by any
bootloader (i.e., it's not the boot device), but we still want to use
the on-die ECC (or to disable it, if it was on by default)? Then the
system still needs some way to communicate to nand_base that it should
enable/disable on-die ECC. So to make all these cases consistent, I
think it makes sense to provide some kind of flag (device tree property;
chip->options flag; etc.) that captures the system's expectations.

BTW, do these flash power on in any particular default mode? Can you
order versions that have on-die ECC enabeld/disabled by default? Is that
perhaps what the READ_ID "Internal ECC" flag actually means
(disabled/enabled by default)?

> > (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)
> 
> Yes, READ_ID also has an "Internal ECC" flag.
> 
> However, look on page 50, table 14: Array Operation Mode.  It documents the
> Disable/Enable ECC bit that can be read via GET FEATURES and
> set via SET FEATURES.
> 
> I can assure you, the patch I sent actually does work! ;-)
> 
> > (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()
> 
> Yes, I don't see why that wouldn't work.  nand_onfi_detect_micron() appears
> to be a relatively recent addition that I wasn't aware of.  Let me try that.
> 
> >>@@ -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 ?

I had the same comment on v4 :)

> > And again re-use nbuf->databuf on re-reads, in read_page_on_die().
> 
> In theory, yes.  In practice, it got way too complicated.  AFAIR, databuf
> may not contain the entire page + OOB, so in general we have to
> re-read (some) of the data some of the time anyhow.  It's much more
> reliable and cleaner to have separate buffers.  That way, it's guaranteed
> that we don't disturb the contents of databuf.

I agree that you may have to re-read anyway, but I still don't see
what's wrong with reusing the buffer. You're not really "disturbing"
anyone's data; you're re-reading the same content.

> Remember that only gets called for pages that have bitflips, so it's
> relatively rare and from a performance-perspective, the extra work
> is irrelevant.
> 
> Also, the extra buffers only get allocated when internal ECC is
> enabled, so it has no discernible effect on other configurations.

Brian

Re: [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.

[Gerhard Sittig]

On Tue, 2014-04-01 at 02:12 -0700, Brian Norris wrote:
> 
> BTW, do these flash power on in any particular default mode? Can you
> order versions that have on-die ECC enabeld/disabled by default? Is that
> perhaps what the READ_ID "Internal ECC" flag actually means
> (disabled/enabled by default)?

According to the datasheet the chips come out of RESET with
on-die-ECC disabled.  The setup that I have seen does enable the
in-chip feature within an early loader.  Subsequently running
bootloaders and OSes (here: U-Boot and Linux) find the chip with
on-die-ECC enabled.

I understand that READ_ID will tell you whether on-die-ECC is
supported by the chip.  The GETFEATURE request tells you whether
it's enabled.  And SETFEATURE allows you to control whether it's
enabled or disabled.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de