[PATCH 1/7] mtd: rawnand: micron: Fix on-die ECC detection logic

[PATCH 1/7] mtd: rawnand: micron: Fix on-die ECC detection logic

[Boris Brezillon]

Basing the "mandatory on-die" detection on ID byte 2 does not work,
because Micron has plenty of NANDs using the same device ID code, and
not all of them have forcibly enabled on-die ECC.

Since the "Array Operation" feature does not provide the "ECC
enabled/disabled" bit when the ECC can't be disabled, let's try to use
the "ECC enabled/disabled" bit in the READ_ID bytes.

It seems that this bit is dynamically updated on NANDs where on-die ECC
can freely be enabled/disabled, so let's hope it stays at one when we
have a NAND with on-die ECC forcibly enabled.

Fixes: 51f3b3970a8c ("mtd: rawnand: micron: detect forced on-die ECC")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/mtd/nand/raw/nand_micron.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index d30bd4df9b12..d85bb4d42686 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -196,6 +196,9 @@ enum {
 	MICRON_ON_DIE_MANDATORY,
 };
 
+#define MICRON_ID_INTERNAL_ECC_MASK	GENMASK(1, 0)
+#define MICRON_ID_ECC_ENABLED		BIT(7)
+
 /*
  * 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
@@ -208,7 +211,7 @@ enum {
  */
 static int micron_supports_on_die_ecc(struct nand_chip *chip)
 {
-	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
+	u8 id[5];
 	int ret;
 
 	if (!chip->parameters.onfi.version)
@@ -217,26 +220,37 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 	if (chip->bits_per_cell != 1)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
+	/*
+	 * We only support on-die ECC of 4/512 or 8/512
+	 */
+	if  (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
+		return MICRON_ON_DIE_UNSUPPORTED;
+
+	/* 0x2 means on-die ECC is available. */
+	if (chip->id.len != 5 ||
+	    (chip->id.data[4] & MICRON_ID_INTERNAL_ECC_MASK) != 0x2)
+		return MICRON_ON_DIE_UNSUPPORTED;
+
 	ret = micron_nand_on_die_ecc_setup(chip, true);
 	if (ret)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
-	ret = nand_get_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
-	if (ret < 0)
-		return ret;
+	ret = nand_readid_op(chip, 0, id, sizeof(id));
+	if (ret)
+		return MICRON_ON_DIE_UNSUPPORTED;
 
-	if ((feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN) == 0)
+	if (!(id[4] & MICRON_ID_ECC_ENABLED))
 		return MICRON_ON_DIE_UNSUPPORTED;
 
 	ret = micron_nand_on_die_ecc_setup(chip, false);
 	if (ret)
 		return MICRON_ON_DIE_UNSUPPORTED;
 
-	ret = nand_get_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
-	if (ret < 0)
-		return ret;
+	ret = nand_readid_op(chip, 0, id, sizeof(id));
+	if (ret)
+		return MICRON_ON_DIE_UNSUPPORTED;
 
-	if (feature[0] & ONFI_FEATURE_ON_DIE_ECC_EN)
+	if (id[4] & MICRON_ID_ECC_ENABLED)
 		return MICRON_ON_DIE_MANDATORY;
 
 	/*
-- 
2.14.1

[PATCH 2/7] mtd: rawnand: micron: support 8/512 on-die ECC

[Boris Brezillon]

From: Chris Packham <chris.packham@alliedtelesis.co.nz>

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>
---
 drivers/mtd/nand/raw/nand_micron.c | 161 ++++++++++++++++++++++++++++++-------
 1 file changed, 131 insertions(+), 30 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index d85bb4d42686..f8839c7f7464 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;
@@ -74,8 +94,9 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
 	return 0;
 }
 
-static int micron_nand_on_die_ooblayout_ecc(struct mtd_info *mtd, int section,
-					    struct mtd_oob_region *oobregion)
+static int micron_nand_on_die_4_ooblayout_ecc(struct mtd_info *mtd,
+					      int section,
+					      struct mtd_oob_region *oobregion)
 {
 	if (section >= 4)
 		return -ERANGE;
@@ -86,8 +107,9 @@ static int micron_nand_on_die_ooblayout_ecc(struct mtd_info *mtd, int section,
 	return 0;
 }
 
-static int micron_nand_on_die_ooblayout_free(struct mtd_info *mtd, int section,
-					     struct mtd_oob_region *oobregion)
+static int micron_nand_on_die_4_ooblayout_free(struct mtd_info *mtd,
+					       int section,
+					       struct mtd_oob_region *oobregion)
 {
 	if (section >= 4)
 		return -ERANGE;
@@ -98,9 +120,44 @@ static int micron_nand_on_die_ooblayout_free(struct mtd_info *mtd, int section,
 	return 0;
 }
 
-static const struct mtd_ooblayout_ops micron_nand_on_die_ooblayout_ops = {
-	.ecc = micron_nand_on_die_ooblayout_ecc,
-	.free = micron_nand_on_die_ooblayout_free,
+static const struct mtd_ooblayout_ops micron_nand_on_die_4_ooblayout_ops = {
+	.ecc = micron_nand_on_die_4_ooblayout_ecc,
+	.free = micron_nand_on_die_4_ooblayout_free,
+};
+
+static int micron_nand_on_die_8_ooblayout_ecc(struct mtd_info *mtd,
+					      int section,
+					      struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = mtd->oobsize - chip->ecc.total;
+	oobregion->length = chip->ecc.total;
+
+	return 0;
+}
+
+static int micron_nand_on_die_8_ooblayout_free(struct mtd_info *mtd,
+					       int section,
+					       struct mtd_oob_region *oobregion)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (section)
+		return -ERANGE;
+
+	oobregion->offset = 2;
+	oobregion->length = mtd->oobsize - chip->ecc.total - 2;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops micron_nand_on_die_8_ooblayout_ops = {
+	.ecc = micron_nand_on_die_8_ooblayout_ecc,
+	.free = micron_nand_on_die_8_ooblayout_free,
 };
 
 static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
@@ -113,6 +170,55 @@ 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 nand_chip *chip, u8 status)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	/*
+	 * 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 nand_chip *chip, u8 status)
+{
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	/*
+	 * 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 +243,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(chip, status);
+	else
+		max_bitflips = micron_nand_on_die_ecc_status_8(chip, status);
 
 	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
 	if (!ret && oob_required)
@@ -254,10 +351,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;
@@ -289,16 +385,21 @@ static int micron_nand_init(struct nand_chip *chip)
 			return -EINVAL;
 		}
 
-		chip->ecc.bytes = 8;
+		if (chip->ecc_strength_ds == 4)
+			mtd_set_ooblayout(mtd,
+					  &micron_nand_on_die_4_ooblayout_ops);
+		else
+			mtd_set_ooblayout(mtd,
+					  &micron_nand_on_die_8_ooblayout_ops);
+
+		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;
 		chip->ecc.read_page_raw = nand_read_page_raw;
 		chip->ecc.write_page_raw = nand_write_page_raw;
-
-		mtd_set_ooblayout(mtd, &micron_nand_on_die_ooblayout_ops);
 	}
 
 	return 0;
-- 
2.14.1

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

[Boris Brezillon]

On Wed, 18 Jul 2018 10:42:16 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> From: Chris Packham <chris.packham@alliedtelesis.co.nz>
> 
> 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>

Forgot to add

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/mtd/nand/raw/nand_micron.c | 161 ++++++++++++++++++++++++++++++-------
>  1 file changed, 131 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index d85bb4d42686..f8839c7f7464 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;
> @@ -74,8 +94,9 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
>  	return 0;
>  }
>  
> -static int micron_nand_on_die_ooblayout_ecc(struct mtd_info *mtd, int section,
> -					    struct mtd_oob_region *oobregion)
> +static int micron_nand_on_die_4_ooblayout_ecc(struct mtd_info *mtd,
> +					      int section,
> +					      struct mtd_oob_region *oobregion)
>  {
>  	if (section >= 4)
>  		return -ERANGE;
> @@ -86,8 +107,9 @@ static int micron_nand_on_die_ooblayout_ecc(struct mtd_info *mtd, int section,
>  	return 0;
>  }
>  
> -static int micron_nand_on_die_ooblayout_free(struct mtd_info *mtd, int section,
> -					     struct mtd_oob_region *oobregion)
> +static int micron_nand_on_die_4_ooblayout_free(struct mtd_info *mtd,
> +					       int section,
> +					       struct mtd_oob_region *oobregion)
>  {
>  	if (section >= 4)
>  		return -ERANGE;
> @@ -98,9 +120,44 @@ static int micron_nand_on_die_ooblayout_free(struct mtd_info *mtd, int section,
>  	return 0;
>  }
>  
> -static const struct mtd_ooblayout_ops micron_nand_on_die_ooblayout_ops = {
> -	.ecc = micron_nand_on_die_ooblayout_ecc,
> -	.free = micron_nand_on_die_ooblayout_free,
> +static const struct mtd_ooblayout_ops micron_nand_on_die_4_ooblayout_ops = {
> +	.ecc = micron_nand_on_die_4_ooblayout_ecc,
> +	.free = micron_nand_on_die_4_ooblayout_free,
> +};
> +
> +static int micron_nand_on_die_8_ooblayout_ecc(struct mtd_info *mtd,
> +					      int section,
> +					      struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->offset = mtd->oobsize - chip->ecc.total;
> +	oobregion->length = chip->ecc.total;
> +
> +	return 0;
> +}
> +
> +static int micron_nand_on_die_8_ooblayout_free(struct mtd_info *mtd,
> +					       int section,
> +					       struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->offset = 2;
> +	oobregion->length = mtd->oobsize - chip->ecc.total - 2;
> +
> +	return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops micron_nand_on_die_8_ooblayout_ops = {
> +	.ecc = micron_nand_on_die_8_ooblayout_ecc,
> +	.free = micron_nand_on_die_8_ooblayout_free,
>  };
>  
>  static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
> @@ -113,6 +170,55 @@ 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 nand_chip *chip, u8 status)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	/*
> +	 * 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 nand_chip *chip, u8 status)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +
> +	/*
> +	 * 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 +243,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(chip, status);
> +	else
> +		max_bitflips = micron_nand_on_die_ecc_status_8(chip, status);
>  
>  	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
>  	if (!ret && oob_required)
> @@ -254,10 +351,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;
> @@ -289,16 +385,21 @@ static int micron_nand_init(struct nand_chip *chip)
>  			return -EINVAL;
>  		}
>  
> -		chip->ecc.bytes = 8;
> +		if (chip->ecc_strength_ds == 4)
> +			mtd_set_ooblayout(mtd,
> +					  &micron_nand_on_die_4_ooblayout_ops);
> +		else
> +			mtd_set_ooblayout(mtd,
> +					  &micron_nand_on_die_8_ooblayout_ops);
> +
> +		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;
>  		chip->ecc.read_page_raw = nand_read_page_raw;
>  		chip->ecc.write_page_raw = nand_write_page_raw;
> -
> -		mtd_set_ooblayout(mtd, &micron_nand_on_die_ooblayout_ops);
>  	}
>  
>  	return 0;

[PATCH 3/7] mtd: rawnand: Expose _notsupp() helpers for raw page accessors

[Boris Brezillon]

Some implementations simply can't disable their ECC engine. Expose
helpers returning -ENOTSUPP so that the caller knows that raw accesses
are not supported instead of silently falling back to non-raw
accessors.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/mtd/rawnand.h      |  4 ++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 4fa5e20d9690..323a900f3697 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -2966,6 +2966,23 @@ int nand_check_erased_ecc_chunk(void *data, int datalen,
 }
 EXPORT_SYMBOL(nand_check_erased_ecc_chunk);
 
+/**
+ * nand_read_page_raw_notsupp - dummy read raw page function
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @buf: buffer to store read data
+ * @oob_required: caller requires OOB data read to chip->oob_poi
+ * @page: page number to read
+ *
+ * Returns -ENOTSUPP unconditionally.
+ */
+int nand_read_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
+			       u8 *buf, int oob_required, int page)
+{
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL(nand_read_page_raw_notsupp);
+
 /**
  * nand_read_page_raw - [INTERN] read raw page data without ecc
  * @mtd: mtd info structure
@@ -3960,6 +3977,22 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
 	return ret;
 }
 
+/**
+ * nand_write_page_raw - dummy raw page write function
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @buf: data buffer
+ * @oob_required: must write chip->oob_poi to OOB
+ * @page: page number to write
+ *
+ * Returns -ENOTSUPP unconditionally.
+ */
+int nand_write_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
+				const u8 *buf, int oob_required, int page)
+{
+	return -ENOTSUPP;
+}
+EXPORT_SYMBOL(nand_write_page_raw_notsupp);
 
 /**
  * nand_write_page_raw - [INTERN] raw page write function
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 11c2426fc363..f60fad29eae6 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1681,10 +1681,14 @@ int nand_get_set_features_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
 /* Default read_page_raw implementation */
 int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
 		       uint8_t *buf, int oob_required, int page);
+int nand_read_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
+			       u8 *buf, int oob_required, int page);
 
 /* Default write_page_raw implementation */
 int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
 			const uint8_t *buf, int oob_required, int page);
+int nand_write_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
+				const u8 *buf, int oob_required, int page);
 
 /* Reset and initialize a NAND device */
 int nand_reset(struct nand_chip *chip, int chipnr);
-- 
2.14.1

Re: [PATCH 3/7] mtd: rawnand: Expose _notsupp() helpers for raw page accessors

[Boris Brezillon]

On Wed, 18 Jul 2018 10:42:17 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Some implementations simply can't disable their ECC engine. Expose
> helpers returning -ENOTSUPP so that the caller knows that raw accesses
> are not supported instead of silently falling back to non-raw
> accessors.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 33 +++++++++++++++++++++++++++++++++
>  include/linux/mtd/rawnand.h      |  4 ++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 4fa5e20d9690..323a900f3697 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -2966,6 +2966,23 @@ int nand_check_erased_ecc_chunk(void *data, int datalen,
>  }
>  EXPORT_SYMBOL(nand_check_erased_ecc_chunk);
>  
> +/**
> + * nand_read_page_raw_notsupp - dummy read raw page function
> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @buf: buffer to store read data
> + * @oob_required: caller requires OOB data read to chip->oob_poi
> + * @page: page number to read
> + *
> + * Returns -ENOTSUPP unconditionally.
> + */
> +int nand_read_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
> +			       u8 *buf, int oob_required, int page)
> +{
> +	return -ENOTSUPP;
> +}
> +EXPORT_SYMBOL(nand_read_page_raw_notsupp);
> +
>  /**
>   * nand_read_page_raw - [INTERN] read raw page data without ecc
>   * @mtd: mtd info structure
> @@ -3960,6 +3977,22 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
>  	return ret;
>  }
>  
> +/**
> + * nand_write_page_raw - dummy raw page write function

Crap, I forgot to add the _notsupp suffix in the doc.

> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @buf: data buffer
> + * @oob_required: must write chip->oob_poi to OOB
> + * @page: page number to write
> + *
> + * Returns -ENOTSUPP unconditionally.
> + */
> +int nand_write_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
> +				const u8 *buf, int oob_required, int page)
> +{
> +	return -ENOTSUPP;
> +}
> +EXPORT_SYMBOL(nand_write_page_raw_notsupp);
>  
>  /**
>   * nand_write_page_raw - [INTERN] raw page write function
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 11c2426fc363..f60fad29eae6 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1681,10 +1681,14 @@ int nand_get_set_features_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
>  /* Default read_page_raw implementation */
>  int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>  		       uint8_t *buf, int oob_required, int page);
> +int nand_read_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
> +			       u8 *buf, int oob_required, int page);
>  
>  /* Default write_page_raw implementation */
>  int nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>  			const uint8_t *buf, int oob_required, int page);
> +int nand_write_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
> +				const u8 *buf, int oob_required, int page);
>  
>  /* Reset and initialize a NAND device */
>  int nand_reset(struct nand_chip *chip, int chipnr);

Re: [PATCH 3/7] mtd: rawnand: Expose _notsupp() helpers for raw page accessors

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Thu, 19 Jul 2018
11:22:17 +0200:

> On Wed, 18 Jul 2018 10:42:17 +0200
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
> > Some implementations simply can't disable their ECC engine. Expose
> > helpers returning -ENOTSUPP so that the caller knows that raw accesses
> > are not supported instead of silently falling back to non-raw
> > accessors.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/mtd/rawnand.h      |  4 ++++
> >  2 files changed, 37 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 4fa5e20d9690..323a900f3697 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -2966,6 +2966,23 @@ int nand_check_erased_ecc_chunk(void *data, int datalen,
> >  }
> >  EXPORT_SYMBOL(nand_check_erased_ecc_chunk);
> >  
> > +/**
> > + * nand_read_page_raw_notsupp - dummy read raw page function
> > + * @mtd: mtd info structure
> > + * @chip: nand chip info structure
> > + * @buf: buffer to store read data
> > + * @oob_required: caller requires OOB data read to chip->oob_poi
> > + * @page: page number to read
> > + *
> > + * Returns -ENOTSUPP unconditionally.
> > + */
> > +int nand_read_page_raw_notsupp(struct mtd_info *mtd, struct nand_chip *chip,
> > +			       u8 *buf, int oob_required, int page)
> > +{
> > +	return -ENOTSUPP;
> > +}
> > +EXPORT_SYMBOL(nand_read_page_raw_notsupp);
> > +
> >  /**
> >   * nand_read_page_raw - [INTERN] read raw page data without ecc
> >   * @mtd: mtd info structure
> > @@ -3960,6 +3977,22 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from,
> >  	return ret;
> >  }
> >  
> > +/**
> > + * nand_write_page_raw - dummy raw page write function  
> 
> Crap, I forgot to add the _notsupp suffix in the doc.

Suffix "_notsupp" added.

Miquèl

[PATCH 4/7] mtd: rawnand: micron: allow forced on-die ECC

[Boris Brezillon]

From: Chris Packham <chris.packham@alliedtelesis.co.nz>

Some Micron NAND chips have on-die ECC forceably enabled. Allow such
chips to be used as long as the controller has set chip->ecc.mode to
NAND_ECC_ON_DIE.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_micron.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index f8839c7f7464..fd3f68e0909f 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -374,7 +374,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;
 	}
@@ -398,8 +399,14 @@ static int micron_nand_init(struct nand_chip *chip)
 		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;
-		chip->ecc.read_page_raw = nand_read_page_raw;
-		chip->ecc.write_page_raw = nand_write_page_raw;
+
+		if (ondie == MICRON_ON_DIE_MANDATORY) {
+			chip->ecc.read_page_raw = nand_read_page_raw_notsupp;
+			chip->ecc.write_page_raw = nand_write_page_raw_notsupp;
+		} else {
+			chip->ecc.read_page_raw = nand_read_page_raw;
+			chip->ecc.write_page_raw = nand_write_page_raw;
+		}
 	}
 
 	return 0;
-- 
2.14.1

Re: [PATCH 4/7] mtd: rawnand: micron: allow forced on-die ECC

[Boris Brezillon]

On Wed, 18 Jul 2018 10:42:18 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> From: Chris Packham <chris.packham@alliedtelesis.co.nz>
> 
> Some Micron NAND chips have on-die ECC forceably enabled. Allow such
> chips to be used as long as the controller has set chip->ecc.mode to
> NAND_ECC_ON_DIE.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Missing

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
>  drivers/mtd/nand/raw/nand_micron.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index f8839c7f7464..fd3f68e0909f 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -374,7 +374,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;
>  	}
> @@ -398,8 +399,14 @@ static int micron_nand_init(struct nand_chip *chip)
>  		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;
> -		chip->ecc.read_page_raw = nand_read_page_raw;
> -		chip->ecc.write_page_raw = nand_write_page_raw;
> +
> +		if (ondie == MICRON_ON_DIE_MANDATORY) {
> +			chip->ecc.read_page_raw = nand_read_page_raw_notsupp;
> +			chip->ecc.write_page_raw = nand_write_page_raw_notsupp;
> +		} else {
> +			chip->ecc.read_page_raw = nand_read_page_raw;
> +			chip->ecc.write_page_raw = nand_write_page_raw;
> +		}
>  	}
>  
>  	return 0;

[PATCH 5/7] mtd: rawnand: micron: Get the actual number of bitflips

[Boris Brezillon]

The MT29F2Gxxx chips with 4bits/512byte on-die ECC let us know when
some bitflips were corrected by the on-die ECC, but they do not report
the actual number of bitflips that were present in the data+ECC chunk.

We initially decided to always return ecc->strength to avoid re-reading
the page in raw mode + comparing it to the corrected buffer to extract
the real number of bitflips, but this forces UBI to move data around as
soon as one bitflip is present in a page.

This not only wears the NAND out faster, but also degrades
performances, since reading a full PEB + writing it back to a different
PEB + erasing the old one is much more expensive than re-reading the
faulty page in raw mode and comparing it to the corrected buffer.
In most cases, the actual number of bitflips does not exceed the
bitflips threshold, and UBI won't have to move data around. Otherwise,
we can assume the time spent re-reading the page and doing the
comparison is negligible compared to the time UBI spends moving a full
PEB to another PEB.

Note that this logic is not applied to chips with 8bits/512byte on-die
ECC, because those chips provide fine-grained information (the maximum
error is 1 bit, and it will not force UBI to move blocks around at the
first bitflip).

Suggested-by: Bean Huo <beanhuo@micron.com>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/nand/raw/nand_micron.c | 139 +++++++++++++++++++++++++++++++------
 1 file changed, 119 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index fd3f68e0909f..2cff25f7b48b 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/mtd/rawnand.h>
+#include <linux/slab.h>
 
 /*
  * Special Micron status bit 3 indicates that the block has been
@@ -63,6 +64,14 @@ struct nand_onfi_vendor_micron {
 	u8 param_revision;
 } __packed;
 
+struct micron_on_die_ecc {
+	void *rawbuf;
+};
+
+struct micron_nand {
+	struct micron_on_die_ecc ecc;
+};
+
 static int micron_nand_setup_read_retry(struct mtd_info *mtd, int retry_mode)
 {
 	struct nand_chip *chip = mtd_to_nand(mtd);
@@ -170,25 +179,71 @@ 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 nand_chip *chip, u8 status)
+static int micron_nand_on_die_ecc_status_4(struct nand_chip *chip, u8 status,
+					   void *buf, int page,
+					   int oob_required)
 {
+	struct micron_nand *micron = nand_get_manufacturer_data(chip);
 	struct mtd_info *mtd = nand_to_mtd(chip);
+	unsigned int step, max_bitflips = 0;
+	int ret;
+
+	if (!(status & NAND_ECC_STATUS_WRITE_RECOMMENDED)) {
+		if (status & NAND_STATUS_FAIL)
+			mtd->ecc_stats.failed++;
+
+		return 0;
+	}
 
 	/*
-	 * 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.
+	 * 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, we need to read the page in raw mode and compare
+	 * its content to the corrected version to extract the actual number of
+	 * bitflips.
+	 * But before we do that, we must make sure we have all OOB bytes read
+	 * in non-raw mode, even if the user did not request those bytes.
 	 */
-	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;
+	if (!oob_required) {
+		ret = nand_read_data_op(chip, chip->oob_poi, mtd->oobsize,
+					false);
+		if (ret)
+			return ret;
 	}
 
-	return 0;
+	micron_nand_on_die_ecc_setup(chip, false);
+
+	ret = nand_read_page_op(chip, page, 0, micron->ecc.rawbuf,
+				mtd->writesize + mtd->oobsize);
+	if (ret)
+		return ret;
+
+	for (step = 0; step < chip->ecc.steps; step++) {
+		unsigned int offs, i, nbitflips = 0;
+		u8 *rawbuf, *corrbuf;
+
+		offs = step * chip->ecc.size;
+		rawbuf = micron->ecc.rawbuf + offs;
+		corrbuf = buf + offs;
+
+		for (i = 0; i < chip->ecc.size; i++)
+			nbitflips += hweight8(corrbuf[i] ^ rawbuf[i]);
+
+		offs = (step * 16) + 4;
+		rawbuf = micron->ecc.rawbuf + mtd->writesize + offs;
+		corrbuf = chip->oob_poi + offs;
+
+		for (i = 0; i < chip->ecc.bytes + 4; i++)
+			nbitflips += hweight8(corrbuf[i] ^ rawbuf[i]);
+
+		if (WARN_ON(nbitflips > chip->ecc.strength))
+			return -EINVAL;
+
+		max_bitflips = max(nbitflips, max_bitflips);
+		mtd->ecc_stats.corrected += nbitflips;
+	}
+
+	return max_bitflips;
 }
 
 static int micron_nand_on_die_ecc_status_8(struct nand_chip *chip, u8 status)
@@ -243,16 +298,18 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
 	if (ret)
 		goto out;
 
-	if (chip->ecc.strength == 4)
-		max_bitflips = micron_nand_on_die_ecc_status_4(chip, status);
-	else
-		max_bitflips = micron_nand_on_die_ecc_status_8(chip, status);
-
 	ret = nand_read_data_op(chip, buf, mtd->writesize, false);
 	if (!ret && oob_required)
 		ret = nand_read_data_op(chip, chip->oob_poi, mtd->oobsize,
 					false);
 
+	if (chip->ecc.strength == 4)
+		max_bitflips = micron_nand_on_die_ecc_status_4(chip, status,
+							       buf, page,
+							       oob_required);
+	else
+		max_bitflips = micron_nand_on_die_ecc_status_8(chip, status);
+
 out:
 	micron_nand_on_die_ecc_setup(chip, false);
 
@@ -362,12 +419,19 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
 static int micron_nand_init(struct nand_chip *chip)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
+	struct micron_nand *micron;
 	int ondie;
 	int ret;
 
+	micron = kzalloc(sizeof(*micron), GFP_KERNEL);
+	if (!micron)
+		return -ENOMEM;
+
+	nand_set_manufacturer_data(chip, micron);
+
 	ret = micron_nand_onfi_init(chip);
 	if (ret)
-		return ret;
+		goto err_free_manuf_data;
 
 	if (mtd->writesize == 2048)
 		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
@@ -377,13 +441,33 @@ static int micron_nand_init(struct nand_chip *chip)
 	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;
+		ret = -EINVAL;
+		goto err_free_manuf_data;
 	}
 
 	if (chip->ecc.mode == NAND_ECC_ON_DIE) {
 		if (ondie == MICRON_ON_DIE_UNSUPPORTED) {
 			pr_err("On-die ECC selected but not supported\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_free_manuf_data;
+		}
+
+		/*
+		 * In case of 4bit on-die ECC, we need a buffer to store a
+		 * page dumped in raw mode so that we can compare its content
+		 * to the same page after ECC correction happened and extract
+		 * the real number of bitflips from this comparison.
+		 * That's not needed for 8-bit ECC, because the status expose
+		 * a better approximation of the number of bitflips in a page.
+		 */
+		if (chip->ecc_strength_ds == 4) {
+			micron->ecc.rawbuf = kmalloc(mtd->writesize +
+						     mtd->oobsize,
+						     GFP_KERNEL);
+			if (!micron->ecc.rawbuf) {
+				ret = -ENOMEM;
+				goto err_free_manuf_data;
+			}
 		}
 
 		if (chip->ecc_strength_ds == 4)
@@ -410,6 +494,20 @@ static int micron_nand_init(struct nand_chip *chip)
 	}
 
 	return 0;
+
+err_free_manuf_data:
+	kfree(micron->ecc.rawbuf);
+	kfree(micron);
+
+	return ret;
+}
+
+static void micron_nand_cleanup(struct nand_chip *chip)
+{
+	struct micron_nand *micron = nand_get_manufacturer_data(chip);
+
+	kfree(micron->ecc.rawbuf);
+	kfree(micron);
 }
 
 static void micron_fixup_onfi_param_page(struct nand_chip *chip,
@@ -426,5 +524,6 @@ static void micron_fixup_onfi_param_page(struct nand_chip *chip,
 
 const struct nand_manufacturer_ops micron_nand_manuf_ops = {
 	.init = micron_nand_init,
+	.cleanup = micron_nand_cleanup,
 	.fixup_onfi_param_page = micron_fixup_onfi_param_page,
 };
-- 
2.14.1

[PATCH 6/7] mtd: rawnand: micron: Avoid enabling/disabling ECC when it can't be disabled

[Boris Brezillon]

Some chips have their on-die ECC forcibly enabled, there's no point in
trying to enable/disable the ECC engine in that case.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/mtd/nand/raw/nand_micron.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index 2cff25f7b48b..9d13b701e581 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -65,6 +65,7 @@ struct nand_onfi_vendor_micron {
 } __packed;
 
 struct micron_on_die_ecc {
+	bool forced;
 	void *rawbuf;
 };
 
@@ -171,8 +172,12 @@ static const struct mtd_ooblayout_ops micron_nand_on_die_8_ooblayout_ops = {
 
 static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
 {
+	struct micron_nand *micron = nand_get_manufacturer_data(chip);
 	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
 
+	if (micron->ecc.forced)
+		return 0;
+
 	if (enable)
 		feature[0] |= ONFI_FEATURE_ON_DIE_ECC_EN;
 
@@ -452,6 +457,9 @@ static int micron_nand_init(struct nand_chip *chip)
 			goto err_free_manuf_data;
 		}
 
+		if (ondie == MICRON_ON_DIE_MANDATORY)
+			micron->ecc.forced = true;
+
 		/*
 		 * In case of 4bit on-die ECC, we need a buffer to store a
 		 * page dumped in raw mode so that we can compare its content
-- 
2.14.1

[PATCH 7/7] mtd: rawnand: micron: Make ECC activation stateful

[Boris Brezillon]

We currently don't store the on-die ECC state (enabled/disabled) which
might force us to re-disable the engine even if it's already been
disabled after we've read the page in raw mode to count the actual
number of bitflips.

Add an "enabled" field to struct micron_on_die_ecc to keep track of the
ECC state.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/mtd/nand/raw/nand_micron.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index 9d13b701e581..656947d91841 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -66,6 +66,7 @@ struct nand_onfi_vendor_micron {
 
 struct micron_on_die_ecc {
 	bool forced;
+	bool enabled;
 	void *rawbuf;
 };
 
@@ -174,14 +175,22 @@ static int micron_nand_on_die_ecc_setup(struct nand_chip *chip, bool enable)
 {
 	struct micron_nand *micron = nand_get_manufacturer_data(chip);
 	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
+	int ret;
 
 	if (micron->ecc.forced)
 		return 0;
 
+	if (micron->ecc.enabled == enable)
+		return 0;
+
 	if (enable)
 		feature[0] |= ONFI_FEATURE_ON_DIE_ECC_EN;
 
-	return nand_set_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
+	ret = nand_set_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature);
+	if (!ret)
+		micron->ecc.enabled = enable;
+
+	return ret;
 }
 
 static int micron_nand_on_die_ecc_status_4(struct nand_chip *chip, u8 status,
@@ -457,8 +466,10 @@ static int micron_nand_init(struct nand_chip *chip)
 			goto err_free_manuf_data;
 		}
 
-		if (ondie == MICRON_ON_DIE_MANDATORY)
+		if (ondie == MICRON_ON_DIE_MANDATORY) {
 			micron->ecc.forced = true;
+			micron->ecc.enabled = true;
+		}
 
 		/*
 		 * In case of 4bit on-die ECC, we need a buffer to store a
-- 
2.14.1

Re: [PATCH 1/7] mtd: rawnand: micron: Fix on-die ECC detection logic

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@bootlin.com> wrote on Wed, 18 Jul 2018
10:42:15 +0200:

> Basing the "mandatory on-die" detection on ID byte 2 does not work,
> because Micron has plenty of NANDs using the same device ID code, and
> not all of them have forcibly enabled on-die ECC.
> 
> Since the "Array Operation" feature does not provide the "ECC
> enabled/disabled" bit when the ECC can't be disabled, let's try to use
> the "ECC enabled/disabled" bit in the READ_ID bytes.
> 
> It seems that this bit is dynamically updated on NANDs where on-die ECC
> can freely be enabled/disabled, so let's hope it stays at one when we
> have a NAND with on-die ECC forcibly enabled.
> 
> Fixes: 51f3b3970a8c ("mtd: rawnand: micron: detect forced on-die ECC")
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---

Series applied to nand/next in place of the previous -buggy- series,
with your SoB added. Actually as you answered the patches with it,
patchwork/git-pwc added it by itself!

Thanks,
Miquèl