[PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC

[PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC

[Boris Brezillon]

Chris, Bean,

Here are 2 patches for you to review/test. The first one is fixing
the layout definition, and unless I missed something it should be
correct.

The second one is just my understanding of how byte 5 of READ_ID works
based on our experience with the 4bit/512 on-die ECC chip we have
worked on and the other datasheet I had a look at.
I'm not 100% sure this will work for all chips, but might work for the
2 chips we support right now.
So please test and/or review it and let me know if this approach works.

Regards,

Boris

Boris Brezillon (2):
  mtd: rawnand: micron: Define the proper layout for 8bit/512bytes
    on-die ECC
  mtd: rawnand: micron: Fix on-die ECC detection logic

 drivers/mtd/nand/raw/nand_micron.c | 110 +++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 35 deletions(-)

-- 
2.14.1

[PATCH 1/2] mtd: rawnand: micron: Define the proper layout for 8bit/512bytes on-die ECC

[Boris Brezillon]

The MT29F1G08ABAFAWP datasheet says that all ECC bytes are placed at the
end of the OOB area.

Fixes: 386fc2609d15 ("mtd: rawnand: micron: support 8/512 on-die ECC")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/mtd/nand/raw/nand_micron.c | 59 ++++++++++++++++++++++++++++++++------
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index 754e9cdd44ac..00a965ad7ab2 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -103,8 +103,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;
@@ -115,8 +116,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;
@@ -127,9 +129,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)
@@ -425,6 +462,12 @@ static int micron_nand_init(struct nand_chip *chip)
 				ret = -ENOMEM;
 				goto err_free_manuf_data;
 			}
+
+			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;
@@ -435,8 +478,6 @@ static int micron_nand_init(struct nand_chip *chip)
 		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 1/2] mtd: rawnand: micron: Define the proper layout for 8bit/512bytes on-die ECC

[Chris Packham]

On 10/07/18 09:09, Boris Brezillon wrote:
> The MT29F1G08ABAFAWP datasheet says that all ECC bytes are placed at the
> end of the OOB area.
> 
> Fixes: 386fc2609d15 ("mtd: rawnand: micron: support 8/512 on-die ECC")
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

With d73e5d29 reverted,

Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

> ---
>   drivers/mtd/nand/raw/nand_micron.c | 59 ++++++++++++++++++++++++++++++++------
>   1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index 754e9cdd44ac..00a965ad7ab2 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -103,8 +103,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;
> @@ -115,8 +116,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;
> @@ -127,9 +129,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)
> @@ -425,6 +462,12 @@ static int micron_nand_init(struct nand_chip *chip)
>   				ret = -ENOMEM;
>   				goto err_free_manuf_data;
>   			}
> +
> +			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;
> @@ -435,8 +478,6 @@ static int micron_nand_init(struct nand_chip *chip)
>   		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 2/2] 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>
---
 drivers/mtd/nand/raw/nand_micron.c | 51 +++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index 00a965ad7ab2..6ae708fbbd3a 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -345,13 +345,8 @@ 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 */
-};
+#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
@@ -366,12 +361,8 @@ static u8 micron_on_die_ecc[] = {
 static int micron_supports_on_die_ecc(struct nand_chip *chip)
 {
 	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
+	u8 id[5];
 	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;
@@ -379,34 +370,42 @@ 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;
+
+	/*
+	 * We don't know what INTERNAL_ECC means, but it seems that 0x2 matches
+	 * all NANDs with on-die ECC.
+	 */
+	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;
 
-	/*
-	 * 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;
-
 	return MICRON_ON_DIE_SUPPORTED;
 }
 
-- 
2.14.1

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

[Chris Packham]

Hi Boris,

On 10/07/18 09:09, Boris Brezillon wrote:
> 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>

With d73e5d29 reverted,

Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

One compiler warning below.

> ---
>   drivers/mtd/nand/raw/nand_micron.c | 51 +++++++++++++++++++-------------------
>   1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index 00a965ad7ab2..6ae708fbbd3a 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -345,13 +345,8 @@ 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 */
> -};
> +#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
> @@ -366,12 +361,8 @@ static u8 micron_on_die_ecc[] = {
>   static int micron_supports_on_die_ecc(struct nand_chip *chip)
>   {
>   	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };

feature is now unused so this causes a compiler warning.

> +	u8 id[5];
>   	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;
> @@ -379,34 +370,42 @@ 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;
> +
> +	/*
> +	 * We don't know what INTERNAL_ECC means, but it seems that 0x2 matches
> +	 * all NANDs with on-die ECC.
> +	 */
> +	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;
>   
> -	/*
> -	 * 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;
> -
>   	return MICRON_ON_DIE_SUPPORTED;
>   }
>   
> 

Re: [PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC

[Boris Brezillon]

On Mon,  9 Jul 2018 23:09:35 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Chris, Bean,
> 
> Here are 2 patches for you to review/test. The first one is fixing
> the layout definition, and unless I missed something it should be
> correct.
> 
> The second one is just my understanding of how byte 5 of READ_ID works
> based on our experience with the 4bit/512 on-die ECC chip we have
> worked on and the other datasheet I had a look at.
> I'm not 100% sure this will work for all chips, but might work for the
> 2 chips we support right now.

I tested it on a MT29F2G08ABAEAH4 (4bit/512bytes on-die ECC) and it
works as expected. When I do the SET_FEATURES(ECC_EN) the ECC enabled
bit in READID byte 5 is set, and when I do SET_FEATURES(ECC_DIS), the
bit is cleared.

Now we need to make sure it works correctly on MT29F1G08ABAFAWP.

> So please test and/or review it and let me know if this approach works.
> 
> Regards,
> 
> Boris
> 
> Boris Brezillon (2):
>   mtd: rawnand: micron: Define the proper layout for 8bit/512bytes
>     on-die ECC
>   mtd: rawnand: micron: Fix on-die ECC detection logic
> 
>  drivers/mtd/nand/raw/nand_micron.c | 110 +++++++++++++++++++++++++------------
>  1 file changed, 75 insertions(+), 35 deletions(-)
> 

Re: [PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC

[Chris Packham]

Hi Boris,

On 10/07/18 20:11, Boris Brezillon wrote:
> On Mon,  9 Jul 2018 23:09:35 +0200
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
>> Chris, Bean,
>>
>> Here are 2 patches for you to review/test. The first one is fixing
>> the layout definition, and unless I missed something it should be
>> correct.
>>
>> The second one is just my understanding of how byte 5 of READ_ID works
>> based on our experience with the 4bit/512 on-die ECC chip we have
>> worked on and the other datasheet I had a look at.
>> I'm not 100% sure this will work for all chips, but might work for the
>> 2 chips we support right now.
> 
> I tested it on a MT29F2G08ABAEAH4 (4bit/512bytes on-die ECC) and it
> works as expected. When I do the SET_FEATURES(ECC_EN) the ECC enabled
> bit in READID byte 5 is set, and when I do SET_FEATURES(ECC_DIS), the
> bit is cleared.
> 
> Now we need to make sure it works correctly on MT29F1G08ABAFAWP.

I'll test that as soon as I can get access to the hardware (probably 
Monday next week).

> 
>> So please test and/or review it and let me know if this approach works.
>>
>> Regards,
>>
>> Boris
>>
>> Boris Brezillon (2):
>>    mtd: rawnand: micron: Define the proper layout for 8bit/512bytes
>>      on-die ECC
>>    mtd: rawnand: micron: Fix on-die ECC detection logic
>>
>>   drivers/mtd/nand/raw/nand_micron.c | 110 +++++++++++++++++++++++++------------
>>   1 file changed, 75 insertions(+), 35 deletions(-)
>>
> 
> 

Re: [PATCH 0/2] mtd: rawnand: micron: Fix on-die ECC

[Chris Packham]

On 10/07/18 09:09, Boris Brezillon wrote:
> Chris, Bean,
> 
> Here are 2 patches for you to review/test. The first one is fixing
> the layout definition, and unless I missed something it should be
> correct.
> 
> The second one is just my understanding of how byte 5 of READ_ID works
> based on our experience with the 4bit/512 on-die ECC chip we have
> worked on and the other datasheet I had a look at.
> I'm not 100% sure this will work for all chips, but might work for the
> 2 chips we support right now.
> So please test and/or review it and let me know if this approach works.
> 

Hi Boris,

I went to test this today and found that I could no-longer mount the 
Micron MT29F1G08ABAFAWP-ITE chip. However it doesn't appear to these 
changes but d73e5d29 ("mtd: rawnand: micron: Disable ECC earlier in the 
read path").

   [root@linuxbox ~]# ubiformat -q -y /dev/mtd0
   [root@linuxbox ~]# ubiattach -p /dev/mtd0
   ubi0: attaching mtd0
   ubi0: scanning is finished
   ubi0 error: ubi_read_volume_table: the layout volume was not found
   ubi0 error: ubi_attach_mtd_dev: failed to attach mtd0, error -22
   ubiattach: error!: cannot attach "/dev/mtd0"
              error 22 (Invalid argument)
   [root@linuxbox ~]# ubimkvol /dev/ubi0 -N user -m
   libubi: error!: cannot get information about "/dev/ubi0"
           error 2 (No such file or directory)
   ubimkvol: error!: error while probing "/dev/ubi0"
             error 2 (No such file or directory)

I'll try juggling some of the nand/next patches around to see if I can 
test these two without d73e5d29.

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

[Boris Brezillon]

Hi Bean,

On Wed, 11 Jul 2018 10:05:06 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> 
> >+	if (chip->id.len != 5 ||
> >+	    (chip->id.data[4] & MICRON_ID_INTERNAL_ECC_MASK) != 0x2)
> >+		return MICRON_ON_DIE_UNSUPPORTED;
> >+  
> 
> I think, we should firstly check if by default internal ECC is on.
> If default is off, then can try to enable through set/get feature command.
> if default is on, there is no need to set up completely.

No, see my comment below.

>   
> 
> > 	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;
> >  
> 
> Here I say for sure, the value of Bit7 in byte 4 of READID is permanent. It is a reflection of
> the default state of the device. it cannot change by set feature.

That's not what the board I have on my desk says. I have a
MT29F2G08ABAEAH4, and I can tell you for sure this bit changes when I
enable/disable on-die ECC. Do the test, and you'll see...

See, that's what I'm complaining about. You keep saying things that
appear to be untrue when when we check on real HW parts. So, either we
have NANDs that are buggy, or you didn't check yourself.

Regards,

Boris