[PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Chris Packham]

Hi,

I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
to one of our boards which uses the Marvell NFCv2 controller.

This particular chip is a bit odd in that the datasheet states support
for ONFI 1.0 but the revision number field is 00 00. It also is marked
ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
which cannot be disabled.

The existing test in micron_supports_on_die_ecc() determines that on-die
ECC is supported but not mandatory but I know for this chip it is
mandatory despite what set_features returns.

In order for this to work I need to set nand-ecc-mode = "on-die" in my
dts. Ideally I'd like it to be automatic based on what the hardware can
support but that may be asking too much at the moment.

Here's a dump of the parameter page from the chip I have

00000000: 4f 4e 46 49 00 00 18 00 3f 00 00 00 00 00 00 00 ONFI....?.......
00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00000020: 4d 49 43 52 4f 4e 20 20 20 20 20 20 4d 54 32 39  MICRON MT29
00000030: 46 31 47 30 38 41 42 41 47 41 57 50 20 20 20 20  F1G08ABAGAWP    
00000040: 2c 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ,...............
00000050: 00 08 00 00 80 00 00 02 00 00 20 00 40 00 00 00  ..........  .@...
00000060: 00 04 00 00 01 22 01 14 00 01 05 08 00 00 04 00 ....."..........
00000070: 08 01 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00000080: 08 3f 00 3f 00 58 02 10 27 46 00 64 00 00 00 00 .?.?.X..'F.d....
00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000a0: 00 00 00 00 01 00 00 00 00 02 04 80 01 81 04 03 ................
000000b0: 02 01 1e 90 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
000000f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 85 a6 ................

Series changes in v3:
- No longer RFC
- dropped "mtd: rawnand: micron: add ONFI_FEATURE_ON_DIE_ECC to supported
  features" which Boris has already picked up
- dropped "mtd: rawnand: marvell: Support page size of 2048 with 8-bit ECC"
  since I can't test it.

Series changes in v4:
- based on top of http://patchwork.ozlabs.org/patch/932006/

Series changes in v5:
- address review comments from Boris on patches 5 and 6

Series changes in v6:
- Update commit message on 6/6

Chris Packham (6):
  mtd: rawnand: marvell: Handle on-die ECC
  mtd: rawnand: add manufacturer fixup for ONFI parameter page
  mtd: rawnand: add defines for ONFI version bits
  mtd: rawnand: micron: add fixup for ONFI revision
  mtd: rawnand: micron: support 8/512 on-die ECC
  mtd: rawnand: micron: detect forced on-die ECC

 drivers/mtd/nand/raw/marvell_nand.c |   1 +
 drivers/mtd/nand/raw/nand_base.c    |  14 +--
 drivers/mtd/nand/raw/nand_micron.c  | 129 +++++++++++++++++++++++-----
 include/linux/mtd/rawnand.h         |  14 +++
 4 files changed, 131 insertions(+), 27 deletions(-)

-- 
2.18.0

[PATCH v6 1/6] mtd: rawnand: marvell: Handle on-die ECC

[Chris Packham]

From the controllers point of view this is the same as no or
software only ECC.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- New
Changes in v3:
- Add review from Boris
Changes in v4:
- None
Changes in v5:
- None
Changes in v6:
- None

 drivers/mtd/nand/raw/marvell_nand.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index ebb1d141b900..ba6889bbe802 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2157,6 +2157,7 @@ static int marvell_nand_ecc_init(struct mtd_info *mtd,
 		break;
 	case NAND_ECC_NONE:
 	case NAND_ECC_SOFT:
+	case NAND_ECC_ON_DIE:
 		if (!nfc->caps->is_nfcv2 && mtd->writesize != SZ_512 &&
 		    mtd->writesize != SZ_2K) {
 			dev_err(nfc->dev, "NFCv1 cannot write %d bytes pages\n",
-- 
2.18.0

[PATCH v6 2/6] mtd: rawnand: add manufacturer fixup for ONFI parameter page

[Chris Packham]

This is called after the ONFI parameter page checksum is verified
and allows us to override the contents of the parameter page.

Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- New
Changes in v3:
- Add doc comment and review from Boris
Changes in v4:
- None
Changes in v5:
- None
Changes in v6:
- None

 drivers/mtd/nand/raw/nand_base.c | 4 ++++
 include/linux/mtd/rawnand.h      | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 0cd3e216b95c..65250308c82d 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5172,6 +5172,10 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 		}
 	}
 
+	if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
+	    chip->manufacturer.desc->ops->fixup_onfi_param_page)
+		chip->manufacturer.desc->ops->fixup_onfi_param_page(chip, p);
+
 	/* Check version */
 	val = le16_to_cpu(p->revision);
 	if (val & (1 << 5))
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 3e8ec3b8a39c..ef7e3b4e91ea 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -778,11 +778,14 @@ nand_get_sdr_timings(const struct nand_data_interface *conf)
  *	  implementation) if any.
  * @cleanup: the ->init() function may have allocated resources, ->cleanup()
  *	     is here to let vendor specific code release those resources.
+ * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI
+ *	parameter page. This is called after the checksum is verified.
  */
 struct nand_manufacturer_ops {
 	void (*detect)(struct nand_chip *chip);
 	int (*init)(struct nand_chip *chip);
 	void (*cleanup)(struct nand_chip *chip);
+	void (*fixup_onfi_param_page)(struct nand_chip *chip, struct nand_onfi_params *p);
 };
 
 /**
-- 
2.18.0

[PATCH v6 3/6] mtd: rawnand: add defines for ONFI version bits

[Chris Packham]

Add defines for the ONFI version bits and use them in
nand_flash_detect_onfi().

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v4:
- New
Changes in v5:
- Add review from Boris
Changes in v6:
- None

 drivers/mtd/nand/raw/nand_base.c | 10 +++++-----
 include/linux/mtd/rawnand.h      | 11 +++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 65250308c82d..36048e0cf1f5 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5178,15 +5178,15 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
 
 	/* Check version */
 	val = le16_to_cpu(p->revision);
-	if (val & (1 << 5))
+	if (val & ONFI_VERSION_2_3)
 		chip->parameters.onfi.version = 23;
-	else if (val & (1 << 4))
+	else if (val & ONFI_VERSION_2_2)
 		chip->parameters.onfi.version = 22;
-	else if (val & (1 << 3))
+	else if (val & ONFI_VERSION_2_1)
 		chip->parameters.onfi.version = 21;
-	else if (val & (1 << 2))
+	else if (val & ONFI_VERSION_2_0)
 		chip->parameters.onfi.version = 20;
-	else if (val & (1 << 1))
+	else if (val & ONFI_VERSION_1_0)
 		chip->parameters.onfi.version = 10;
 
 	if (!chip->parameters.onfi.version) {
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index ef7e3b4e91ea..015bc3f2fc13 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -230,6 +230,17 @@ enum nand_ecc_algo {
 /* Keep gcc happy */
 struct nand_chip;
 
+/* ONFI version bits */
+#define ONFI_VERSION_1_0		BIT(1)
+#define ONFI_VERSION_2_0		BIT(2)
+#define ONFI_VERSION_2_1		BIT(3)
+#define ONFI_VERSION_2_2		BIT(4)
+#define ONFI_VERSION_2_3		BIT(5)
+#define ONFI_VERSION_3_0		BIT(6)
+#define ONFI_VERSION_3_1		BIT(7)
+#define ONFI_VERSION_3_2		BIT(8)
+#define ONFI_VERSION_4_0		BIT(9)
+
 /* ONFI features */
 #define ONFI_FEATURE_16_BIT_BUS		(1 << 0)
 #define ONFI_FEATURE_EXT_PARAM_PAGE	(1 << 7)
-- 
2.18.0

[PATCH v6 4/6] mtd: rawnand: micron: add fixup for ONFI revision

[Chris Packham]

Some Micron NAND chips (MT29F1G08ABAFAWP-ITE:F) report 00 00 for the
revision number field of the ONFI parameter page. Rather than rejecting
these outright assume ONFI version 1.0 if the revision number is 00 00.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
This is now qualified on vendor == MICRON. I haven't qualified this
based on specific chips the ABAFA (id=d1) and ABBFA (id=a1) variants are
documented to have this behaviour.

Changes in v2:
- use fixup_onfi_param_page
Changes in v3:
- add code comment next to workaround
Changes in v4:
- use define for ONFI_VERSION
Changes in v5:
- None
Changes in v6:
- None

 drivers/mtd/nand/raw/nand_micron.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index 203faba0a9c1..d30bd4df9b12 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -290,6 +290,19 @@ static int micron_nand_init(struct nand_chip *chip)
 	return 0;
 }
 
+static void micron_fixup_onfi_param_page(struct nand_chip *chip,
+					 struct nand_onfi_params *p)
+{
+	/*
+	 * MT29F1G08ABAFAWP-ITE:F and possibly others report 00 00 for the
+	 * revision number field of the ONFI parameter page. Assume ONFI
+	 * version 1.0 if the revision number is 00 00.
+	 */
+	if (le16_to_cpu(p->revision) == 0)
+		p->revision = cpu_to_le16(ONFI_VERSION_1_0);
+}
+
 const struct nand_manufacturer_ops micron_nand_manuf_ops = {
 	.init = micron_nand_init,
+	.fixup_onfi_param_page = micron_fixup_onfi_param_page,
 };
-- 
2.18.0

[PATCH v6 5/6] mtd: rawnand: micron: support 8/512 on-die ECC

[Chris Packham]

Micron MT29F1G08ABAFAWP-ITE:F supports an on-die ECC with 8 bits
per 512 bytes. Add support for this combination.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v2:
- New
Changes in v3:
- Handle reporting of corrected errors that don't require a rewrite, expand
  comment for the ECC status bits.
Changes in v4:
- Use a switch statement for handling ECC status
- Update ecc_stats.corrected
Changes in v5:
- Move status checking to different routines for 4/512 and 8/512 assume
  the highest number of bit flips for a given status value.
Changes in v6:
- Add review from Boris

 drivers/mtd/nand/raw/nand_micron.c | 100 +++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index d30bd4df9b12..f83053562925 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -18,10 +18,30 @@
 #include <linux/mtd/rawnand.h>
 
 /*
- * Special Micron status bit that indicates when the block has been
- * corrected by on-die ECC and should be rewritten
+ * Special Micron status bit 3 indicates that the block has been
+ * corrected by on-die ECC and should be rewritten.
  */
-#define NAND_STATUS_WRITE_RECOMMENDED	BIT(3)
+#define NAND_ECC_STATUS_WRITE_RECOMMENDED	BIT(3)
+
+/*
+ * On chips with 8-bit ECC and additional bit can be used to distinguish
+ * cases where a errors were corrected without needing a rewrite
+ *
+ * Bit 4 Bit 3 Bit 0 Description
+ * ----- ----- ----- -----------
+ * 0     0     0     No Errors
+ * 0     0     1     Multiple uncorrected errors
+ * 0     1     0     4 - 6 errors corrected, recommend rewrite
+ * 0     1     1     Reserved
+ * 1     0     0     1 - 3 errors corrected
+ * 1     0     1     Reserved
+ * 1     1     0     7 - 8 errors corrected, recommend rewrite
+ */
+#define NAND_ECC_STATUS_MASK		(BIT(4) | BIT(3) | BIT(0))
+#define NAND_ECC_STATUS_UNCORRECTABLE	BIT(0)
+#define NAND_ECC_STATUS_4_6_CORRECTED	BIT(3)
+#define NAND_ECC_STATUS_1_3_CORRECTED	BIT(4)
+#define NAND_ECC_STATUS_7_8_CORRECTED	(BIT(4) | BIT(3))
 
 struct nand_onfi_vendor_micron {
 	u8 two_plane_read;
@@ -113,6 +133,54 @@ static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
 	return nand_set_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
 }
 
+
+static int micron_nand_on_die_ecc_status_4(struct mtd_info *mtd,
+					   struct nand_chip *chip, u8 status)
+{
+	/*
+	 * The internal ECC doesn't tell us the number of bitflips
+	 * that have been corrected, but tells us if it recommends to
+	 * rewrite the block. If it's the case, then we pretend we had
+	 * a number of bitflips equal to the ECC strength, which will
+	 * hint the NAND core to rewrite the block.
+	 */
+	if (status & NAND_STATUS_FAIL) {
+		mtd->ecc_stats.failed++;
+	} else if (status & NAND_ECC_STATUS_WRITE_RECOMMENDED) {
+		mtd->ecc_stats.corrected += chip->ecc.strength;
+		return chip->ecc.strength;
+	}
+
+	return 0;
+}
+
+static int micron_nand_on_die_ecc_status_8(struct mtd_info *mtd,
+					   struct nand_chip *chip, u8 status)
+{
+	/*
+	 * With 8/512 we have more information but still don't know precisely
+	 * how many bit-flips were seen.
+	 */
+	switch (status & NAND_ECC_STATUS_MASK) {
+	case NAND_ECC_STATUS_UNCORRECTABLE:
+		mtd->ecc_stats.failed++;
+		return 0;
+	case NAND_ECC_STATUS_1_3_CORRECTED:
+		mtd->ecc_stats.corrected += 3;
+		return 3;
+	case NAND_ECC_STATUS_4_6_CORRECTED:
+		mtd->ecc_stats.corrected += 6;
+		/* rewrite recommended */
+		return 6;
+	case NAND_ECC_STATUS_7_8_CORRECTED:
+		mtd->ecc_stats.corrected += 8;
+		/* rewrite recommended */
+		return 8;
+	default:
+		return 0;
+	}
+}
+
 static int
 micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
 				 uint8_t *buf, int oob_required,
@@ -137,19 +205,10 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
 	if (ret)
 		goto out;
 
-	if (status & NAND_STATUS_FAIL) {
-		mtd->ecc_stats.failed++;
-	} else if (status & NAND_STATUS_WRITE_RECOMMENDED) {
-		/*
-		 * The internal ECC doesn't tell us the number of bitflips
-		 * that have been corrected, but tells us if it recommends to
-		 * rewrite the block. If it's the case, then we pretend we had
-		 * a number of bitflips equal to the ECC strength, which will
-		 * hint the NAND core to rewrite the block.
-		 */
-		mtd->ecc_stats.corrected += chip->ecc.strength;
-		max_bitflips = chip->ecc.strength;
-	}
+	if (chip->ecc.strength == 4)
+		max_bitflips = micron_nand_on_die_ecc_status_4(mtd, chip, status);
+	else
+		max_bitflips = micron_nand_on_die_ecc_status_8(mtd, chip, status);
 
 	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
 	if (!ret && oob_required)
@@ -240,10 +299,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 		return MICRON_ON_DIE_MANDATORY;
 
 	/*
-	 * Some Micron NANDs have an on-die ECC of 4/512, some other
-	 * 8/512. We only support the former.
+	 * We only support on-die ECC of 4/512 or 8/512
 	 */
-	if (chip->ecc_strength_ds != 4)
+	if  (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
 	return MICRON_ON_DIE_SUPPORTED;
@@ -275,9 +333,9 @@ static int micron_nand_init(struct nand_chip *chip)
 			return -EINVAL;
 		}
 
-		chip->ecc.bytes = 8;
+		chip->ecc.bytes = chip->ecc_strength_ds * 2;
 		chip->ecc.size = 512;
-		chip->ecc.strength = 4;
+		chip->ecc.strength = chip->ecc_strength_ds;
 		chip->ecc.algo = NAND_ECC_BCH;
 		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
 		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;
-- 
2.18.0

[PATCH v6 6/6] mtd: rawnand: micron: detect forced on-die ECC

[Chris Packham]

Some Micron NAND chips have on-die ECC forceably enabled. Detect these
based on chip ID as there seems to be no other way of distinguishing
these chips from those that have optional support for on-die ECC.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v4:
- New
Changes in v5:
- fail if on-die ECC is mandatory and the current ecc.mode is not
  NAND_ECC_ON_DIE.
Changes in v6:
- Update commit message, add review from Boris

 drivers/mtd/nand/raw/nand_micron.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index f83053562925..35fa6880a799 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -255,6 +255,14 @@ enum {
 	MICRON_ON_DIE_MANDATORY,
 };
 
+/*
+ * These parts are known to have on-die ECC forceably enabled
+ */
+static u8 micron_on_die_ecc[] = {
+	0xd1, /* MT29F1G08ABAFA */
+	0xa1, /* MT29F1G08ABBFA */
+};
+
 /*
  * Try to detect if the NAND support on-die ECC. To do this, we enable
  * the feature, and read back if it has been enabled as expected. We
@@ -269,6 +277,11 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 {
 	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
 	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(micron_on_die_ecc); i++)
+		if (chip->id.data[1] == micron_on_die_ecc[i])
+			return MICRON_ON_DIE_MANDATORY;
 
 	if (!chip->parameters.onfi.version)
 		return MICRON_ON_DIE_UNSUPPORTED;
@@ -322,7 +335,8 @@ static int micron_nand_init(struct nand_chip *chip)
 
 	ondie = micron_supports_on_die_ecc(chip);
 
-	if (ondie == MICRON_ON_DIE_MANDATORY) {
+	if (ondie == MICRON_ON_DIE_MANDATORY &&
+	    chip->ecc.mode != NAND_ECC_ON_DIE) {
 		pr_err("On-die ECC forcefully enabled, not supported\n");
 		return -EINVAL;
 	}
-- 
2.18.0

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Miquel Raynal]

Hi Chris,

On Mon, 25 Jun 2018 10:44:42 +1200, Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:

> Hi,
> 
> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
> to one of our boards which uses the Marvell NFCv2 controller.
> 
> This particular chip is a bit odd in that the datasheet states support
> for ONFI 1.0 but the revision number field is 00 00. It also is marked
> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
> which cannot be disabled.
> 
> The existing test in micron_supports_on_die_ecc() determines that on-die
> ECC is supported but not mandatory but I know for this chip it is
> mandatory despite what set_features returns.
> 
> In order for this to work I need to set nand-ecc-mode = "on-die" in my
> dts. Ideally I'd like it to be automatic based on what the hardware can
> support but that may be asking too much at the moment.
> 
> Here's a dump of the parameter page from the chip I have
> 
> 00000000: 4f 4e 46 49 00 00 18 00 3f 00 00 00 00 00 00 00 ONFI....?.......
> 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00000020: 4d 49 43 52 4f 4e 20 20 20 20 20 20 4d 54 32 39  MICRON MT29
> 00000030: 46 31 47 30 38 41 42 41 47 41 57 50 20 20 20 20  F1G08ABAGAWP    
> 00000040: 2c 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ,...............
> 00000050: 00 08 00 00 80 00 00 02 00 00 20 00 40 00 00 00  ..........  .@...
> 00000060: 00 04 00 00 01 22 01 14 00 01 05 08 00 00 04 00 ....."..........
> 00000070: 08 01 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00000080: 08 3f 00 3f 00 58 02 10 27 46 00 64 00 00 00 00 .?.?.X..'F.d....
> 00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000a0: 00 00 00 00 01 00 00 00 00 02 04 80 01 81 04 03 ................
> 000000b0: 02 01 1e 90 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 85 a6 ................
> 
> Series changes in v3:
> - No longer RFC
> - dropped "mtd: rawnand: micron: add ONFI_FEATURE_ON_DIE_ECC to supported
>   features" which Boris has already picked up
> - dropped "mtd: rawnand: marvell: Support page size of 2048 with 8-bit ECC"
>   since I can't test it.
> 
> Series changes in v4:
> - based on top of http://patchwork.ozlabs.org/patch/932006/
> 
> Series changes in v5:
> - address review comments from Boris on patches 5 and 6
> 
> Series changes in v6:
> - Update commit message on 6/6
> 
> Chris Packham (6):
>   mtd: rawnand: marvell: Handle on-die ECC
>   mtd: rawnand: add manufacturer fixup for ONFI parameter page
>   mtd: rawnand: add defines for ONFI version bits
>   mtd: rawnand: micron: add fixup for ONFI revision
>   mtd: rawnand: micron: support 8/512 on-die ECC
>   mtd: rawnand: micron: detect forced on-die ECC
> 
>  drivers/mtd/nand/raw/marvell_nand.c |   1 +
>  drivers/mtd/nand/raw/nand_base.c    |  14 +--
>  drivers/mtd/nand/raw/nand_micron.c  | 129 +++++++++++++++++++++++-----
>  include/linux/mtd/rawnand.h         |  14 +++
>  4 files changed, 131 insertions(+), 27 deletions(-)
> 

Applied to nand/next, I just changed the way you aligned/not aligned
some lines to respect the 80 columns rule.

Thanks,
Miquèl

Re: [PATCH v6 5/6] mtd: rawnand: micron: support 8/512 on-die ECC

[Boris Brezillon]

On Mon, 25 Jun 2018 10:44:47 +1200
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> Micron MT29F1G08ABAFAWP-ITE:F supports an on-die ECC with 8 bits
> per 512 bytes. Add support for this combination.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
> Changes in v2:
> - New
> Changes in v3:
> - Handle reporting of corrected errors that don't require a rewrite, expand
>   comment for the ECC status bits.
> Changes in v4:
> - Use a switch statement for handling ECC status
> - Update ecc_stats.corrected
> Changes in v5:
> - Move status checking to different routines for 4/512 and 8/512 assume
>   the highest number of bit flips for a given status value.
> Changes in v6:
> - Add review from Boris
> 
>  drivers/mtd/nand/raw/nand_micron.c | 100 +++++++++++++++++++++++------
>  1 file changed, 79 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index d30bd4df9b12..f83053562925 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -18,10 +18,30 @@
>  #include <linux/mtd/rawnand.h>
>  
>  /*
> - * Special Micron status bit that indicates when the block has been
> - * corrected by on-die ECC and should be rewritten
> + * Special Micron status bit 3 indicates that the block has been
> + * corrected by on-die ECC and should be rewritten.
>   */
> -#define NAND_STATUS_WRITE_RECOMMENDED	BIT(3)
> +#define NAND_ECC_STATUS_WRITE_RECOMMENDED	BIT(3)
> +
> +/*
> + * On chips with 8-bit ECC and additional bit can be used to distinguish
> + * cases where a errors were corrected without needing a rewrite
> + *
> + * Bit 4 Bit 3 Bit 0 Description
> + * ----- ----- ----- -----------
> + * 0     0     0     No Errors
> + * 0     0     1     Multiple uncorrected errors
> + * 0     1     0     4 - 6 errors corrected, recommend rewrite
> + * 0     1     1     Reserved
> + * 1     0     0     1 - 3 errors corrected
> + * 1     0     1     Reserved
> + * 1     1     0     7 - 8 errors corrected, recommend rewrite
> + */
> +#define NAND_ECC_STATUS_MASK		(BIT(4) | BIT(3) | BIT(0))
> +#define NAND_ECC_STATUS_UNCORRECTABLE	BIT(0)
> +#define NAND_ECC_STATUS_4_6_CORRECTED	BIT(3)
> +#define NAND_ECC_STATUS_1_3_CORRECTED	BIT(4)
> +#define NAND_ECC_STATUS_7_8_CORRECTED	(BIT(4) | BIT(3))
>  
>  struct nand_onfi_vendor_micron {
>  	u8 two_plane_read;
> @@ -113,6 +133,54 @@ static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
>  	return nand_set_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
>  }
>  
> +
> +static int micron_nand_on_die_ecc_status_4(struct mtd_info *mtd,
> +					   struct nand_chip *chip, u8 status)
> +{
> +	/*
> +	 * The internal ECC doesn't tell us the number of bitflips
> +	 * that have been corrected, but tells us if it recommends to
> +	 * rewrite the block. If it's the case, then we pretend we had
> +	 * a number of bitflips equal to the ECC strength, which will
> +	 * hint the NAND core to rewrite the block.
> +	 */
> +	if (status & NAND_STATUS_FAIL) {
> +		mtd->ecc_stats.failed++;
> +	} else if (status & NAND_ECC_STATUS_WRITE_RECOMMENDED) {
> +		mtd->ecc_stats.corrected += chip->ecc.strength;
> +		return chip->ecc.strength;
> +	}
> +
> +	return 0;
> +}
> +
> +static int micron_nand_on_die_ecc_status_8(struct mtd_info *mtd,
> +					   struct nand_chip *chip, u8 status)
> +{
> +	/*
> +	 * With 8/512 we have more information but still don't know precisely
> +	 * how many bit-flips were seen.
> +	 */
> +	switch (status & NAND_ECC_STATUS_MASK) {
> +	case NAND_ECC_STATUS_UNCORRECTABLE:
> +		mtd->ecc_stats.failed++;
> +		return 0;
> +	case NAND_ECC_STATUS_1_3_CORRECTED:
> +		mtd->ecc_stats.corrected += 3;
> +		return 3;
> +	case NAND_ECC_STATUS_4_6_CORRECTED:
> +		mtd->ecc_stats.corrected += 6;
> +		/* rewrite recommended */
> +		return 6;
> +	case NAND_ECC_STATUS_7_8_CORRECTED:
> +		mtd->ecc_stats.corrected += 8;
> +		/* rewrite recommended */
> +		return 8;
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static int
>  micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>  				 uint8_t *buf, int oob_required,
> @@ -137,19 +205,10 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>  	if (ret)
>  		goto out;
>  
> -	if (status & NAND_STATUS_FAIL) {
> -		mtd->ecc_stats.failed++;
> -	} else if (status & NAND_STATUS_WRITE_RECOMMENDED) {
> -		/*
> -		 * The internal ECC doesn't tell us the number of bitflips
> -		 * that have been corrected, but tells us if it recommends to
> -		 * rewrite the block. If it's the case, then we pretend we had
> -		 * a number of bitflips equal to the ECC strength, which will
> -		 * hint the NAND core to rewrite the block.
> -		 */
> -		mtd->ecc_stats.corrected += chip->ecc.strength;
> -		max_bitflips = chip->ecc.strength;
> -	}
> +	if (chip->ecc.strength == 4)
> +		max_bitflips = micron_nand_on_die_ecc_status_4(mtd, chip, status);
> +	else
> +		max_bitflips = micron_nand_on_die_ecc_status_8(mtd, chip, status);
>  
>  	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
>  	if (!ret && oob_required)
> @@ -240,10 +299,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
>  		return MICRON_ON_DIE_MANDATORY;
>  
>  	/*
> -	 * Some Micron NANDs have an on-die ECC of 4/512, some other
> -	 * 8/512. We only support the former.
> +	 * We only support on-die ECC of 4/512 or 8/512
>  	 */
> -	if (chip->ecc_strength_ds != 4)
> +	if  (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
>  		return MICRON_ON_DIE_UNSUPPORTED;
>  
>  	return MICRON_ON_DIE_SUPPORTED;
> @@ -275,9 +333,9 @@ static int micron_nand_init(struct nand_chip *chip)
>  			return -EINVAL;
>  		}
>  
> -		chip->ecc.bytes = 8;
> +		chip->ecc.bytes = chip->ecc_strength_ds * 2;

Just had a quick look at the MT29F1G08ABAFAWP datasheet, and the layout
is different: you have all ECC bytes placed at the end of OOB area (64
bytes), and all the free bytes placed at the beginning (64 bytes). You
should define a new mtd_ooblayout_ops for this case.

>  		chip->ecc.size = 512;
> -		chip->ecc.strength = 4;
> +		chip->ecc.strength = chip->ecc_strength_ds;
>  		chip->ecc.algo = NAND_ECC_BCH;
>  		chip->ecc.read_page = micron_nand_read_page_on_die_ecc;
>  		chip->ecc.write_page = micron_nand_write_page_on_die_ecc;

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Boris Brezillon]

On Mon, 25 Jun 2018 10:44:42 +1200
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> Hi,
> 
> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip

Hm, it's even worse than I thought. The model name does not include the
-ITE suffix (E means ECC can't be disabled), which means we have no way
to detect the version with forced on-die ECC.

I see 2 solutions to this problem:
1/ Bean provides us a solution to reliably detect when ECC can be
   de-actived and when it can't
2/ We only ever expose 64 bytes of OOB to the user and consider that
   ECC can be disabled, even if it can't in reality

I wonder when NAND vendors will care about this sort of stuff. It's
really a mess to deal with all these quirks, and it's even worse when
we have no way to detect when the quirks are needed and when they're
not. Device detection is broken in so many ways, and each new chip
brings with it its own brokenness.

> to one of our boards which uses the Marvell NFCv2 controller.
> 
> This particular chip is a bit odd in that the datasheet states support
> for ONFI 1.0 but the revision number field is 00 00. It also is marked
> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
> which cannot be disabled.
> 
> The existing test in micron_supports_on_die_ecc() determines that on-die
> ECC is supported but not mandatory but I know for this chip it is
> mandatory despite what set_features returns.
> 
> In order for this to work I need to set nand-ecc-mode = "on-die" in my
> dts. Ideally I'd like it to be automatic based on what the hardware can
> support but that may be asking too much at the moment.
> 
> Here's a dump of the parameter page from the chip I have
> 
> 00000000: 4f 4e 46 49 00 00 18 00 3f 00 00 00 00 00 00 00 ONFI....?.......
> 00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00000020: 4d 49 43 52 4f 4e 20 20 20 20 20 20 4d 54 32 39  MICRON MT29
> 00000030: 46 31 47 30 38 41 42 41 47 41 57 50 20 20 20 20  F1G08ABAGAWP    
> 00000040: 2c 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ,...............
> 00000050: 00 08 00 00 80 00 00 02 00 00 20 00 40 00 00 00  ..........  .@...
> 00000060: 00 04 00 00 01 22 01 14 00 01 05 08 00 00 04 00 ....."..........
> 00000070: 08 01 0e 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00000080: 08 3f 00 3f 00 58 02 10 27 46 00 64 00 00 00 00 .?.?.X..'F.d....
> 00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000a0: 00 00 00 00 01 00 00 00 00 02 04 80 01 81 04 03 ................
> 000000b0: 02 01 1e 90 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 000000f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 85 a6 ................
> 
> Series changes in v3:
> - No longer RFC
> - dropped "mtd: rawnand: micron: add ONFI_FEATURE_ON_DIE_ECC to supported
>   features" which Boris has already picked up
> - dropped "mtd: rawnand: marvell: Support page size of 2048 with 8-bit ECC"
>   since I can't test it.
> 
> Series changes in v4:
> - based on top of http://patchwork.ozlabs.org/patch/932006/
> 
> Series changes in v5:
> - address review comments from Boris on patches 5 and 6
> 
> Series changes in v6:
> - Update commit message on 6/6
> 
> Chris Packham (6):
>   mtd: rawnand: marvell: Handle on-die ECC
>   mtd: rawnand: add manufacturer fixup for ONFI parameter page
>   mtd: rawnand: add defines for ONFI version bits
>   mtd: rawnand: micron: add fixup for ONFI revision
>   mtd: rawnand: micron: support 8/512 on-die ECC
>   mtd: rawnand: micron: detect forced on-die ECC
> 
>  drivers/mtd/nand/raw/marvell_nand.c |   1 +
>  drivers/mtd/nand/raw/nand_base.c    |  14 +--
>  drivers/mtd/nand/raw/nand_micron.c  | 129 +++++++++++++++++++++++-----
>  include/linux/mtd/rawnand.h         |  14 +++
>  4 files changed, 131 insertions(+), 27 deletions(-)
> 

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Boris Brezillon]

On Fri, 6 Jul 2018 21:27:20 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Mon, 25 Jun 2018 10:44:42 +1200
> Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> 
> > Hi,
> > 
> > I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip  
> 
> Hm, it's even worse than I thought. The model name does not include the
> -ITE suffix (E means ECC can't be disabled), which means we have no way
> to detect the version with forced on-die ECC.
> 
> I see 2 solutions to this problem:
> 1/ Bean provides us a solution to reliably detect when ECC can be
>    de-actived and when it can't
> 2/ We only ever expose 64 bytes of OOB to the user and consider that
>    ECC can be disabled, even if it can't in reality
>

After reading the doc again, I forgot one thing you can try before
deciding to go for option #2.

8th bit in byte 5 of READID's result encodes whether the on-die ECC
state (enabled or not). I remember we had a discussion with Bean where
he told us this was a runtime status reflecting the on-die ECC state,
which is crazy, since READID might return different values depending on
the NAND state, and most of the code in the core assumes READID
provides a fixed ID that encodes the chip characteristics/capabilities,
not its state.

Anyway, if this bit is actually reflecting the on-die ECC state and
on-die cannot be disabled on your chip, it should stay at 1 even after
you have sent the SET_FEATURES(DISABLE_ECC) command. Let's hope this
works as I expect, otherwise we're back to option #2 until Bean suggest
something else.

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Chris Packham]

Hi Boris,

On 07/07/18 09:37, Boris Brezillon wrote:
> On Fri, 6 Jul 2018 21:27:20 +0200
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
>> On Mon, 25 Jun 2018 10:44:42 +1200
>> Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
>>
>>> Hi,
>>>
>>> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
>>
>> Hm, it's even worse than I thought. The model name does not include the
>> -ITE suffix (E means ECC can't be disabled), which means we have no way
>> to detect the version with forced on-die ECC.
>>
>> I see 2 solutions to this problem:
>> 1/ Bean provides us a solution to reliably detect when ECC can be
>>     de-actived and when it can't
>> 2/ We only ever expose 64 bytes of OOB to the user and consider that
>>     ECC can be disabled, even if it can't in reality
>>
> 
> After reading the doc again, I forgot one thing you can try before
> deciding to go for option #2.
> 
> 8th bit in byte 5 of READID's result encodes whether the on-die ECC
> state (enabled or not). I remember we had a discussion with Bean where
> he told us this was a runtime status reflecting the on-die ECC state,
> which is crazy, since READID might return different values depending on
> the NAND state, and most of the code in the core assumes READID
> provides a fixed ID that encodes the chip characteristics/capabilities,
> not its state.
> 
> Anyway, if this bit is actually reflecting the on-die ECC state and
> on-die cannot be disabled on your chip, it should stay at 1 even after
> you have sent the SET_FEATURES(DISABLE_ECC) command. Let's hope this
> works as I expect, otherwise we're back to option #2 until Bean suggest
> something else.
> 

I'm away from work this week so I don't have access to that system. But 
I can take a look when I get back. From memory though there was very 
little that you could tell from the id/params on this chip (FYI we've 
decided to use a chip from a different vendor for production).

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Miquel Raynal]

Hi Bean,

> I encounter one question as below when I reply the email of linux-mtd, but If I reply other mail list 
> Such as scsi mail list, mmc mail list, no any problem. Do you know why? 
> 
> 
> Your mail to 'linux-mtd' with the subject
> 
>     Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F
> 
> Is being held until the list moderator can review it for approval.
> 
> The reason it is being held:
> 
>     Message has a suspicious header

Linus recently reported to a contributor that one of his patches got
marked as SPAM by the kernel ML because it failed the DKIM signature.
This was caused by a bad handling somewhere of the 'To:' field that
ended-up being empty. While this is an arguable handling from mailman,
it might be related to your problems.

Otherwise you could share the headers, but as this message passed
through the mailing list filter, I suspect it's not a severe issue.

Thanks,
Miquèl

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi, Boris
>>
>
>Okay. Can you have a look at the patches I sent an let me know if I do the
>right thing?

I am doing today. And will back to you I could give some advice.


I encounter one question as below when I reply the email of linux-mtd, but If I reply other mail list 
Such as scsi mail list, mmc mail list, no any problem. Do you know why? 


Your mail to 'linux-mtd' with the subject

    Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

Is being held until the list moderator can review it for approval.

The reason it is being held:

    Message has a suspicious header

Either the message will get posted to the list, or you will receive notification of the moderator's decision.  If you would like to cancel this posting, please visit the following URL:

    http://lists.infradead.org/mailman/confirm/linux-mtd/1a28f9a4069a0bbcadfb6b2a170c01dd8b962d0a


//beanhuo

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Boris Brezillon]

On Tue, 10 Jul 2018 11:40:08 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi,  Boris
> >>
> >> Okay, I think we already had this discussion, but I'm asking it again.
> >> What are the possible values for that field and what do they mean?  
> >
> >Still, it's not clear to me what "Internal ECC level" means. It seems that NAND
> >chips having on-die ECC have this field set to 10b (0x2), 00b seems to be
> >reserved for "no on-die ECC", but what are 01b and 11b reserved for?
> >  
> 
> That position identifies the part as having Internal ECC capability.  
> The 01b and 11b are reserved for future definition.  Bit 7 of READ ID Byte 4 identifies the
> device as an ECC OFF 0x0 or ECC ON 0x1.
> 
> >> Also, is it even used to encode the fact that the NAND has on-die ECC
> >> on all your NANDs? We already had the problem of incompatible ID
> >> schemes, so I wouldn't be surprised if that was the case here, hence
> >> my initial suggestion to base the detection on the model name.  
> >
> >I'd really need to have an answer on that one to take a decision. Also, I
> >couldn't find a datasheet for an IT (without E) version of the
> >MT29F1G08ABAFAWP part. Does it exist, or can we assume
> >MT29F1G08ABAFAWP chips always come with forcibly enabled on-die ECC?  
> 
> MT29F1G08ABAFAWP comes in ECC ON only.  We didn’t develop a
> MT29F1G08ABAFAWP ECC OFF version.  Bit 7 of READ ID Byte 4 identifies the
> device as an ECC OFF 0x0 or ECC ON 0x1.
> 

Okay. Can you have a look at the patches I sent an let me know if I do
the right thing?

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi,  Boris
>>
>> Okay, I think we already had this discussion, but I'm asking it again.
>> What are the possible values for that field and what do they mean?
>
>Still, it's not clear to me what "Internal ECC level" means. It seems that NAND
>chips having on-die ECC have this field set to 10b (0x2), 00b seems to be
>reserved for "no on-die ECC", but what are 01b and 11b reserved for?
>

That position identifies the part as having Internal ECC capability.  
The 01b and 11b are reserved for future definition.  Bit 7 of READ ID Byte 4 identifies the
device as an ECC OFF 0x0 or ECC ON 0x1.

>> Also, is it even used to encode the fact that the NAND has on-die ECC
>> on all your NANDs? We already had the problem of incompatible ID
>> schemes, so I wouldn't be surprised if that was the case here, hence
>> my initial suggestion to base the detection on the model name.
>
>I'd really need to have an answer on that one to take a decision. Also, I
>couldn't find a datasheet for an IT (without E) version of the
>MT29F1G08ABAFAWP part. Does it exist, or can we assume
>MT29F1G08ABAFAWP chips always come with forcibly enabled on-die ECC?

MT29F1G08ABAFAWP comes in ECC ON only.  We didn’t develop a
MT29F1G08ABAFAWP ECC OFF version.  Bit 7 of READ ID Byte 4 identifies the
device as an ECC OFF 0x0 or ECC ON 0x1.

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Boris Brezillon]

On Mon, 9 Jul 2018 18:31:24 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hi Bean,
> 
> On Mon, 9 Jul 2018 15:54:11 +0000
> "Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:
> 
> > Hi, Boris and Chris
> >   
> > >>
> > >> I see 2 solutions to this problem:
> > >> 1/ Bean provides us a solution to reliably detect when ECC can be
> > >>    de-actived and when it can't
> > >> 2/ We only ever expose 64 bytes of OOB to the user and consider that
> > >>    ECC can be disabled, even if it can't in reality
> > >>    
> > >
> > >After reading the doc again, I forgot one thing you can try before deciding to
> > >go for option #2.
> > >
> > >8th bit in byte 5 of READID's result encodes whether the on-die ECC state
> > >(enabled or not). I remember we had a discussion with Bean where he told us
> > >this was a runtime status reflecting the on-die ECC state, which is crazy, since
> > >READID might return different values depending on the NAND state, and most
> > >of the code in the core assumes READID provides a fixed ID that encodes the
> > >chip characteristics/capabilities, not its state.
> > >
> > >Anyway, if this bit is actually reflecting the on-die ECC state and on-die
> > >cannot be disabled on your chip, it should stay at 1 even after you have sent
> > >the SET_FEATURES(DISABLE_ECC) command. Let's hope this works as I expect,
> > >otherwise we're back to option #2 until Bean suggest something else.    
> > 
> > MT29F1G08ABAFAWP-ITE:F is Micron 70s SLC NAND with on-die ECC.
> > If the bit7 in the byte 5 of READID default is 1(ECC Enabled), it is true on-die ECC
> > Cannot be disabled by SET Feature command.
> > In case sending Set Feature command, you can check previous command is success or not by
> > Get Feature  
> 
> Chris tried that already and it didn't work. GET_FEATURES(ECC) returns a
> value with the bit cleared (meaning that ECC is supposedly disabled)
> after SET_FEATURES(ECC, DISABLE) has been sent. Are you sure that
> GET_FEATURES(ECC) is supposed to return the actual state of the ECC
> engine and not the last value you wrote in the internal reg (would be
> the case if this reg is just a dummy reg when on-die ECC is forcibly
> enabled).

My bad, that's not exactly what Chris reported. He said that the
SET_FEATURES(ECC)/GET_FEATURES(ECC) detection logic was reporting the
NAND as "not supporting on-die ECC", which kind of reflects what's
described in the datasheet (in "Table 10: Feature Address 90h – Array
Operation Mode" of MT29F1G08ABAFAWP-ITE:F spec, there's no ECC
enable/disable bit).

This means the detection logic we have simply doesn't work.

> 
> > or check this bit7 in byte 5 of READID later.  
> 
> This, we'll have to test.
> 
> > 
> > To check if this device supports on-die ECC or not, you can use inter ECC level (bit 0 and bit1 in byte 5 of READID)  
> > http://lists.infradead.org/pipermail/linux-mtd/2017-April/073370.html  
> 
> Okay, I think we already had this discussion, but I'm asking it again.
> What are the possible values for that field and what do they mean?

Still, it's not clear to me what "Internal ECC level" means. It seems
that NAND chips having on-die ECC have this field set to 10b (0x2), 00b
seems to be reserved for "no on-die ECC", but what are 01b and 11b
reserved for?

> Also, is it even used to encode the fact that the NAND has on-die ECC
> on all your NANDs? We already had the problem of incompatible ID
> schemes, so I wouldn't be surprised if that was the case here, hence my
> initial suggestion to base the detection on the model name.

I'd really need to have an answer on that one to take a decision. Also,
I couldn't find a datasheet for an IT (without E) version of the
MT29F1G08ABAFAWP part. Does it exist, or can we assume MT29F1G08ABAFAWP
chips always come with forcibly enabled on-die ECC?

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Boris Brezillon]

Hi Bean,

On Mon, 9 Jul 2018 15:54:11 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi, Boris and Chris
> 
> >>
> >> I see 2 solutions to this problem:
> >> 1/ Bean provides us a solution to reliably detect when ECC can be
> >>    de-actived and when it can't
> >> 2/ We only ever expose 64 bytes of OOB to the user and consider that
> >>    ECC can be disabled, even if it can't in reality
> >>  
> >
> >After reading the doc again, I forgot one thing you can try before deciding to
> >go for option #2.
> >
> >8th bit in byte 5 of READID's result encodes whether the on-die ECC state
> >(enabled or not). I remember we had a discussion with Bean where he told us
> >this was a runtime status reflecting the on-die ECC state, which is crazy, since
> >READID might return different values depending on the NAND state, and most
> >of the code in the core assumes READID provides a fixed ID that encodes the
> >chip characteristics/capabilities, not its state.
> >
> >Anyway, if this bit is actually reflecting the on-die ECC state and on-die
> >cannot be disabled on your chip, it should stay at 1 even after you have sent
> >the SET_FEATURES(DISABLE_ECC) command. Let's hope this works as I expect,
> >otherwise we're back to option #2 until Bean suggest something else.  
> 
> MT29F1G08ABAFAWP-ITE:F is Micron 70s SLC NAND with on-die ECC.
> If the bit7 in the byte 5 of READID default is 1(ECC Enabled), it is true on-die ECC
> Cannot be disabled by SET Feature command.
> In case sending Set Feature command, you can check previous command is success or not by
> Get Feature

Chris tried that already and it didn't work. GET_FEATURES(ECC) returns a
value with the bit cleared (meaning that ECC is supposedly disabled)
after SET_FEATURES(ECC, DISABLE) has been sent. Are you sure that
GET_FEATURES(ECC) is supposed to return the actual state of the ECC
engine and not the last value you wrote in the internal reg (would be
the case if this reg is just a dummy reg when on-die ECC is forcibly
enabled).

> or check this bit7 in byte 5 of READID later.

This, we'll have to test.

> 
> To check if this device supports on-die ECC or not, you can use inter ECC level (bit 0 and bit1 in byte 5 of READID)  
> http://lists.infradead.org/pipermail/linux-mtd/2017-April/073370.html

Okay, I think we already had this discussion, but I'm asking it again.
What are the possible values for that field and what do they mean?
Also, is it even used to encode the fact that the NAND has on-die ECC
on all your NANDs? We already had the problem of incompatible ID
schemes, so I wouldn't be surprised if that was the case here, hence my
initial suggestion to base the detection on the model name.

Bean, can you check that on a few different NAND parts before giving a
definitive answer, because, at least for the SET_FEATURES(ECC), we
don't see what you describe, and that's not the first time you suggest
things that do not work in real life.

Regards,

Boris

Re: [PATCH v6 0/6] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Bean Huo (beanhuo)]

Hi, Boris and Chris

>>
>> I see 2 solutions to this problem:
>> 1/ Bean provides us a solution to reliably detect when ECC can be
>>    de-actived and when it can't
>> 2/ We only ever expose 64 bytes of OOB to the user and consider that
>>    ECC can be disabled, even if it can't in reality
>>
>
>After reading the doc again, I forgot one thing you can try before deciding to
>go for option #2.
>
>8th bit in byte 5 of READID's result encodes whether the on-die ECC state
>(enabled or not). I remember we had a discussion with Bean where he told us
>this was a runtime status reflecting the on-die ECC state, which is crazy, since
>READID might return different values depending on the NAND state, and most
>of the code in the core assumes READID provides a fixed ID that encodes the
>chip characteristics/capabilities, not its state.
>
>Anyway, if this bit is actually reflecting the on-die ECC state and on-die
>cannot be disabled on your chip, it should stay at 1 even after you have sent
>the SET_FEATURES(DISABLE_ECC) command. Let's hope this works as I expect,
>otherwise we're back to option #2 until Bean suggest something else.

MT29F1G08ABAFAWP-ITE:F is Micron 70s SLC NAND with on-die ECC.
If the bit7 in the byte 5 of READID default is 1(ECC Enabled), it is true on-die ECC
Cannot be disabled by SET Feature command.
In case sending Set Feature command, you can check previous command is success or not by
Get Feature or check this bit7 in byte 5 of READID later.

To check if this device supports on-die ECC or not, you can use inter ECC level (bit 0 and bit1 in byte 5 of READID)  
http://lists.infradead.org/pipermail/linux-mtd/2017-April/073370.html

//beanhuo