[PATCH v3 0/4] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[PATCH v3 0/4] 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.

Chris Packham (4):
  mtd: rawnand: marvell: Handle on-die ECC
  mtd: rawnand: add manufacturer fixup for ONFI parameter page
  mtd: rawnand: micron: add fixup for ONFI revision
  mtd: rawnand: micron: support 8/512 on-die ECC

 drivers/mtd/nand/raw/marvell_nand.c |  1 +
 drivers/mtd/nand/raw/nand_base.c    |  4 +++
 drivers/mtd/nand/raw/nand_micron.c  | 47 ++++++++++++++++++++++++-----
 include/linux/mtd/rawnand.h         |  3 ++
 4 files changed, 48 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] 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.

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

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

[PATCH v3 2/4] 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>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Changes in v2:
- New
Changes in v3:
- Add doc comment and review from Boris

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

[PATCH v3 3/4] 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.

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
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

 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 5ec4c90a637d..5cec79372181 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -289,6 +289,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(1 << 1);
+}
+
 const struct nand_manufacturer_ops micron_nand_manuf_ops = {
 	.init = micron_nand_init,
+	.fixup_onfi_param_page = micron_fixup_onfi_param_page,
 };
-- 
2.17.1

[PATCH v3 4/4] 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>
---
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.

 drivers/mtd/nand/raw/nand_micron.c | 34 ++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index 5cec79372181..0c2bde4411d7 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -18,10 +18,24 @@
 #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.
+ *
+ * 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     0     1     Reserved
+ * 1     0     0     1 - 3 errors corrected
+ * 1     0     1     Reserved
+ * 1     1     0     7 - 8 errors corrected, recommend rewrite
  */
 #define NAND_STATUS_WRITE_RECOMMENDED	BIT(3)
+#define NAND_STATUS_ERRORS_CORRECTED	BIT(4)
 
 struct nand_onfi_vendor_micron {
 	u8 two_plane_read;
@@ -141,7 +155,7 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
 		mtd->ecc_stats.failed++;
 
 	/*
-	 * The internal ECC doesn't tell us the number of bitflips
+	 * The internal 4-bit 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
@@ -149,6 +163,12 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
 	 */
 	else if (status & NAND_STATUS_WRITE_RECOMMENDED)
 		max_bitflips = chip->ecc.strength;
+	/*
+	 * Chips with 8-bit internal ECC do tell us if errors 1 to 3 bit
+	 * errors have been corrected without recommending a rewrite.
+	 */
+	else if (status & NAND_STATUS_ERRORS_CORRECTED)
+		max_bitflips = 3;
 
 	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
 	if (!ret && oob_required)
@@ -240,9 +260,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 
 	/*
 	 * Some Micron NANDs have an on-die ECC of 4/512, some other
-	 * 8/512. We only support the former.
+	 * 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;
@@ -274,9 +294,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.17.1

Re: [PATCH v3 1/4] mtd: rawnand: marvell: Handle on-die ECC

[Boris Brezillon]

Hi Chris,

On Wed, 20 Jun 2018 17:05:41 +1200
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> From the controllers point of view this is the same as no or
> software only ECC.
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

I'm nitpicking, but we usually put the R-b/A-b tags after the author
SoB.

> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Changes in v2:
> - New
> Changes in v3:
> - Add review from Boris
> 
>  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",

Re: [PATCH v3 2/4] mtd: rawnand: add manufacturer fixup for ONFI parameter page

[Boris Brezillon]

On Wed, 20 Jun 2018 17:05:42 +1200
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> 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>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

Ditto.

> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Changes in v2:
> - New
> Changes in v3:
> - Add doc comment and review from Boris
> 
>  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.

Can you align the second line on the colon?

>   */
>  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);
>  };
>  
>  /**

Re: [PATCH v3 3/4] mtd: rawnand: micron: add fixup for ONFI revision

[Boris Brezillon]

On Wed, 20 Jun 2018 17:05:43 +1200
Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> 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.
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 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
> 
>  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 5ec4c90a637d..5cec79372181 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -289,6 +289,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(1 << 1);

Would be better to have macros defining all version numbers in rawnand.h

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

> +}
> +
>  const struct nand_manufacturer_ops micron_nand_manuf_ops = {
>  	.init = micron_nand_init,
> +	.fixup_onfi_param_page = micron_fixup_onfi_param_page,
>  };

Re: [PATCH v3 4/4] mtd: rawnand: micron: support 8/512 on-die ECC

[Boris Brezillon]

On Wed, 20 Jun 2018 17:05:44 +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>
> ---
> 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.
> 
>  drivers/mtd/nand/raw/nand_micron.c | 34 ++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index 5cec79372181..0c2bde4411d7 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -18,10 +18,24 @@
>  #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.
> + *
> + * 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     0     1     Reserved
> + * 1     0     0     1 - 3 errors corrected
> + * 1     0     1     Reserved
> + * 1     1     0     7 - 8 errors corrected, recommend rewrite
>   */
>  #define NAND_STATUS_WRITE_RECOMMENDED	BIT(3)
> +#define NAND_STATUS_ERRORS_CORRECTED	BIT(4)
>  
>  struct nand_onfi_vendor_micron {
>  	u8 two_plane_read;
> @@ -141,7 +155,7 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>  		mtd->ecc_stats.failed++;
>  
>  	/*
> -	 * The internal ECC doesn't tell us the number of bitflips
> +	 * The internal 4-bit 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
> @@ -149,6 +163,12 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>  	 */
>  	else if (status & NAND_STATUS_WRITE_RECOMMENDED)
>  		max_bitflips = chip->ecc.strength;
> +	/*
> +	 * Chips with 8-bit internal ECC do tell us if errors 1 to 3 bit
> +	 * errors have been corrected without recommending a rewrite.
> +	 */
> +	else if (status & NAND_STATUS_ERRORS_CORRECTED)
> +		max_bitflips = 3;

Why not masking bit 3, 4 and 0 and having a switch-case block?

Also, you should update ecc_stats.corrected (see the patch I just sent
[1]).

>  
>  	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
>  	if (!ret && oob_required)
> @@ -240,9 +260,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
>  
>  	/*
>  	 * Some Micron NANDs have an on-die ECC of 4/512, some other
> -	 * 8/512. We only support the former.
> +	 * 8/512.
>  	 */
> -	if (chip->ecc_strength_ds != 4)
> +	if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
>  		return MICRON_ON_DIE_UNSUPPORTED;

Given that our on-die-support detection procedure is not reliable, I'd
recommend changing the way we do it and instead base this detection
logic on the model name (in the ONFI param page) or the READ_ID bytes.

>  
>  	return MICRON_ON_DIE_SUPPORTED;
> @@ -274,9 +294,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;

[1]http://patchwork.ozlabs.org/patch/932006/

Re: [PATCH v3 1/4] mtd: rawnand: marvell: Handle on-die ECC

[Chris Packham]

On 20/06/18 19:47, Boris Brezillon wrote:
> Hi Chris,
> 
> On Wed, 20 Jun 2018 17:05:41 +1200
> Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> 
>>  From the controllers point of view this is the same as no or
>> software only ECC.
>>
>> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> I'm nitpicking, but we usually put the R-b/A-b tags after the author
> SoB.

Yeah sorry force of habit from using gerrit internally. I'll try to 
re-train my fingers.

>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>> Changes in v2:
>> - New
>> Changes in v3:
>> - Add review from Boris
>>
>>   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",
> 
> 

Re: [PATCH v3 3/4] mtd: rawnand: micron: add fixup for ONFI revision

[Chris Packham]

On 20/06/18 19:54, Boris Brezillon wrote:
> On Wed, 20 Jun 2018 17:05:43 +1200
> Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> 
>> 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.
>>
>> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>> 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
>>
>>   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 5ec4c90a637d..5cec79372181 100644
>> --- a/drivers/mtd/nand/raw/nand_micron.c
>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>> @@ -289,6 +289,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(1 << 1);
> 
> Would be better to have macros defining all version numbers in rawnand.h
> 
> #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)
> 

Make sense. Do you want me to do so in a v4 or send a new patch and 
clean up the other uses?

>> +}
>> +
>>   const struct nand_manufacturer_ops micron_nand_manuf_ops = {
>>   	.init = micron_nand_init,
>> +	.fixup_onfi_param_page = micron_fixup_onfi_param_page,
>>   };
> 
> 

Re: [PATCH v3 4/4] mtd: rawnand: micron: support 8/512 on-die ECC

[Chris Packham]

On 20/06/18 20:02, Boris Brezillon wrote:
> On Wed, 20 Jun 2018 17:05:44 +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>
>> ---
>> 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.
>>
>>   drivers/mtd/nand/raw/nand_micron.c | 34 ++++++++++++++++++++++++------
>>   1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
>> index 5cec79372181..0c2bde4411d7 100644
>> --- a/drivers/mtd/nand/raw/nand_micron.c
>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>> @@ -18,10 +18,24 @@
>>   #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.
>> + *
>> + * 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     0     1     Reserved
>> + * 1     0     0     1 - 3 errors corrected
>> + * 1     0     1     Reserved
>> + * 1     1     0     7 - 8 errors corrected, recommend rewrite
>>    */
>>   #define NAND_STATUS_WRITE_RECOMMENDED	BIT(3)
>> +#define NAND_STATUS_ERRORS_CORRECTED	BIT(4)
>>   
>>   struct nand_onfi_vendor_micron {
>>   	u8 two_plane_read;
>> @@ -141,7 +155,7 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>>   		mtd->ecc_stats.failed++;
>>   
>>   	/*
>> -	 * The internal ECC doesn't tell us the number of bitflips
>> +	 * The internal 4-bit 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
>> @@ -149,6 +163,12 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>>   	 */
>>   	else if (status & NAND_STATUS_WRITE_RECOMMENDED)
>>   		max_bitflips = chip->ecc.strength;
>> +	/*
>> +	 * Chips with 8-bit internal ECC do tell us if errors 1 to 3 bit
>> +	 * errors have been corrected without recommending a rewrite.
>> +	 */
>> +	else if (status & NAND_STATUS_ERRORS_CORRECTED)
>> +		max_bitflips = 3;
> 
> Why not masking bit 3, 4 and 0 and having a switch-case block?

Mainly because the existing code was just checking bit 3 and that 
happened to worked for my use-case.

I'm happy to re-work it as you've suggested can anyone point me at the 
datasheet for a 4/512 on-die chip (or just the part number I can lookup 
on mircon's site).

> 
> Also, you should update ecc_stats.corrected (see the patch I just sent
> [1]).
> 

Will do. I'll pull in your patch and base v4 on top of that.

>>   
>>   	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
>>   	if (!ret && oob_required)
>> @@ -240,9 +260,9 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
>>   
>>   	/*
>>   	 * Some Micron NANDs have an on-die ECC of 4/512, some other
>> -	 * 8/512. We only support the former.
>> +	 * 8/512.
>>   	 */
>> -	if (chip->ecc_strength_ds != 4)
>> +	if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
>>   		return MICRON_ON_DIE_UNSUPPORTED;

I was thinking about removing this. The original code excluded 8/512 due 
to lack of access to a chip that implements this, which I now have.

> Given that our on-die-support detection procedure is not reliable, I'd
> recommend changing the way we do it and instead base this detection
> logic on the model name (in the ONFI param page) or the READ_ID bytes.
> 

The problem is I don't know an exhaustive list of IDs that this applies 
to. I guess having a list of known IDs and falling back to the current 
detection is probably the best approach unless Micron get back to us 
with some other method of detecting these and determining if it is 
forceably enabled.

>>   
>>   	return MICRON_ON_DIE_SUPPORTED;
>> @@ -274,9 +294,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;
> 
> [1]http://patchwork.ozlabs.org/patch/932006/
> 

Re: [PATCH v3 3/4] mtd: rawnand: micron: add fixup for ONFI revision

[Boris Brezillon]

On Wed, 20 Jun 2018 21:12:02 +0000
Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:

> On 20/06/18 19:54, Boris Brezillon wrote:
> > On Wed, 20 Jun 2018 17:05:43 +1200
> > Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> >   
> >> 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.
> >>
> >> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> >> ---
> >> 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
> >>
> >>   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 5ec4c90a637d..5cec79372181 100644
> >> --- a/drivers/mtd/nand/raw/nand_micron.c
> >> +++ b/drivers/mtd/nand/raw/nand_micron.c
> >> @@ -289,6 +289,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(1 << 1);  
> > 
> > Would be better to have macros defining all version numbers in rawnand.h
> > 
> > #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)
> >   
> 
> Make sense. Do you want me to do so in a v4 or send a new patch and 
> clean up the other uses?

It can be part of your v4, but yes, the idea is to first introduce the
macros and patch existing users, and then use it in this patch.