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

Message ID 20180709210937.30150-3-boris.brezillon@bootlin.com
State Changes Requested
Headers show
Series
  • mtd: rawnand: micron: Fix on-die ECC
Related show

Commit Message

Boris Brezillon July 9, 2018, 9:09 p.m.
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(-)

Comments

Chris Packham July 16, 2018, 8:36 a.m. | #1
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;
>   }
>   
>

Patch

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;
 }