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