[PATCH] mtd: nand: parse the Hynix nand which uses <26nm technology

[PATCH] mtd: nand: parse the Hynix nand which uses <26nm technology

[Huang Shijie]

The Hynix uses different ID parsing rules for <26nm technology.
We should check the id_data[5] for Hynix nand now.

This patch adds the parsing code for the Hynix nand which use <26nm technology,
and it also parses out the datasheet's minimum required ECC.

Tested with H27UBG8T2CTR(8192 + 640).

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |   65 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bd39f7b..4dab696 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3162,7 +3162,7 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
 			(((extid >> 1) & 0x04) | (extid & 0x03));
 		*busw = 0;
 	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
-			!nand_is_slc(chip)) {
+			!nand_is_slc(chip) && (id_data[5] & 0x7) < 3) {
 		unsigned int tmp;
 
 		/* Calc pagesize */
@@ -3202,6 +3202,69 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
 		else
 			mtd->erasesize = (64 * 1024) << tmp;
 		*busw = 0;
+	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
+			!nand_is_slc(chip) && (id_data[5] & 0x7) > 3) {
+		unsigned int tmp;
+
+		/* Calc pagesize */
+		mtd->writesize = 4096 << (extid & 0x03);
+		extid >>= 2;
+		/* Calc oobsize */
+		switch (((extid >> 2) & 0x04) | (extid & 0x03)) {
+		case 0:
+			mtd->oobsize = 640;
+			break;
+		case 1:
+			mtd->oobsize = 448;
+			break;
+		case 2:
+			mtd->oobsize = 224;
+			break;
+		case 3:
+			mtd->oobsize = 128;
+			break;
+		case 4:
+			mtd->oobsize = 64;
+			break;
+		case 5:
+			mtd->oobsize = 32;
+			break;
+		case 6:
+			mtd->oobsize = 16;
+			break;
+		default:
+			mtd->oobsize = 640;
+			break;
+		}
+		extid >>= 2;
+		/* Calc blocksize */
+		tmp = ((extid >> 1) & 0x04) | (extid & 0x03);
+		if (tmp < 0x03)
+			mtd->erasesize = (128 * 1024) << tmp;
+		else if (tmp == 0x03)
+			mtd->erasesize = 768 * 1024;
+		else
+			mtd->erasesize = (64 * 1024) << tmp;
+
+		/* ecc info */
+		tmp = (id_data[4] >> 4) & 0x7;
+
+		if (tmp < 4) {
+			chip->ecc_strength_ds = 1 << tmp;
+			chip->ecc_step_ds = 512;
+		} else {
+			chip->ecc_step_ds = 1024;
+			if (tmp == 4)
+				chip->ecc_strength_ds = 24;
+			else if (tmp == 5)
+				chip->ecc_strength_ds = 32;
+			else if (tmp == 6)
+				chip->ecc_strength_ds = 40;
+			else /* (tmp == 7) */
+				chip->ecc_strength_ds = 100;
+		}
+
+		*busw = 0;
 	} else {
 		/* Calc pagesize */
 		mtd->writesize = 1024 << (extid & 0x03);
-- 
1.7.2.rc3

[PATCH] mtd: nand: parse out the datasheet's required minimum ECC for Hynix(>=26nm)

[Huang Shijie]

Parse out the datasheet's required minimum ECC for Hynix nand chips which
use the >=26 technology and the id length is 6.

Referencd to the H27UBG8T2B.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 4dab696..5c1ffd0 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3201,6 +3201,26 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
 			mtd->erasesize = 768 * 1024;
 		else
 			mtd->erasesize = (64 * 1024) << tmp;
+
+		/* ecc info */
+		tmp = (id_data[4] >> 4) & 0x7;
+
+		if (tmp <= 4) {
+			if (tmp == 0)
+				chip->ecc_strength_ds = 0;
+			else
+				chip->ecc_strength_ds = 1 << (tmp - 1);
+			chip->ecc_step_ds = 512;
+		} else {
+			chip->ecc_step_ds = 1024;
+			if (tmp == 5)
+				chip->ecc_strength_ds = 24;
+			else if (tmp == 6)
+				chip->ecc_strength_ds = 32;
+			else /* (tmp == 7) */
+				chip->ecc_strength_ds = 40;
+		}
+
 		*busw = 0;
 	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
 			!nand_is_slc(chip) && (id_data[5] & 0x7) > 3) {
-- 
1.7.2.rc3

Re: [PATCH] mtd: nand: parse out the datasheet's required minimum ECC for Hynix(>=26nm)

[Brian Norris]

On Mon, Dec 02, 2013 at 05:20:26PM +0800, Huang Shijie wrote:
> Parse out the datasheet's required minimum ECC for Hynix nand chips which
> use the >=26 technology and the id length is 6.
> 
> Referencd to the H27UBG8T2B.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/nand_base.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 4dab696..5c1ffd0 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3201,6 +3201,26 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
>  			mtd->erasesize = 768 * 1024;
>  		else
>  			mtd->erasesize = (64 * 1024) << tmp;
> +
> +		/* ecc info */

s/ecc/ECC/

ECC is an acronym and should be capitalized.

> +		tmp = (id_data[4] >> 4) & 0x7;

This calculation (and several others) are similar between >= and < 26nm.
I think it could help to split out a separate Hynix MLC function for
decoding these together, with the shared calculations pulled outside
(i.e., for extracting the ECC and block size bitfields; and for setting
the buswidth to x8).

> +
> +		if (tmp <= 4) {
> +			if (tmp == 0)
> +				chip->ecc_strength_ds = 0;
> +			else
> +				chip->ecc_strength_ds = 1 << (tmp - 1);

The above 4 lines could be simplified to:

			chip->ecc_strength_ds = (1 << tmp) >> 1;

> +			chip->ecc_step_ds = 512;
> +		} else {
> +			chip->ecc_step_ds = 1024;
> +			if (tmp == 5)
> +				chip->ecc_strength_ds = 24;
> +			else if (tmp == 6)
> +				chip->ecc_strength_ds = 32;
> +			else /* (tmp == 7) */
> +				chip->ecc_strength_ds = 40;

This ECC table looks incorrect. It matches my datasheet for H27UBG8T2B
(26nm) but it is incorrect for my H27UBG8T2A (32nm). My datasheet says:

  0 ==> 1-bit/512-bytes
  1 ==> 2-bits/512-bytes
  2 ==> 4-bits/512-bytes
  3 ==> 8-bits/512-bytes
  4 ==> 16-bits/512-bytes
  5 ==> 24-bits/2048-bytes
  6 ==> 24-bits/1024-bytes
  7 ==> Reserved

Not sure if that's a preliminary version that changed for production
(the datasheet is Rev 0.6, from 2009; I don't have samples on hand for
this one), but this demonstrates the larger problem I have with this
patch series...

...that keeping track of minimum ECC requirements is not a scalable
practice for all flash, especially those that avoid using any proper
standards. It's difficult enough just getting OOB/page/block sizes
correct without adding this to the mix. If we add one more thing to get
wrong (ECC strength/step), we become increasingly fragile.

Furthermore, it seems that GPMI is the only driver that needs this
information in Linux. All other systems determine these things within
their bootloader, or through some restriction on the devices they
support (e.g., only support Hamming ECC). So it doesn't seem 100% clear
that we *must* detect the minimum ECC properties in nand_base. In fact,
you even agreed (in another thread) that gpmi-nand should probably
define a more precise nand-ecc-strength and nand-ecc-step-size binding;
such a binding could eliminate the need for runtime auto-detection.

Now, I'm not against progressively adding ECC information that we are
sure to get correct -- like we did with ONFI and with the full-ID table
entries we add. But unless we can get better guarantees from vendors
like Hynix [1], I don't want to commit to this approach, and I'd prefer
not committing any drivers to relying on autodetecting this information.

[1] For instance, can you (or Hynix) explain why my H27UBG8T2A conflicts
    with this table you provided? Do we have to generate a new ECC table
    for every generation?? (48nm, 41nm, 32nm, 26nm, ...)

> +		}
> +
>  		*busw = 0;
>  	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
>  			!nand_is_slc(chip) && (id_data[5] & 0x7) > 3) {

Brian

Re: [PATCH] mtd: nand: parse out the datasheet's required minimum ECC for Hynix(>=26nm)

[Huang Shijie]

On Thu, Dec 19, 2013 at 10:44:07PM -0800, Brian Norris wrote:
> On Mon, Dec 02, 2013 at 05:20:26PM +0800, Huang Shijie wrote:
> > Parse out the datasheet's required minimum ECC for Hynix nand chips which
> > use the >=26 technology and the id length is 6.
> > 
> > Referencd to the H27UBG8T2B.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> >  drivers/mtd/nand/nand_base.c |   20 ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 4dab696..5c1ffd0 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > +			chip->ecc_step_ds = 512;
> > +		} else {
> > +			chip->ecc_step_ds = 1024;
> > +			if (tmp == 5)
> > +				chip->ecc_strength_ds = 24;
> > +			else if (tmp == 6)
> > +				chip->ecc_strength_ds = 32;
> > +			else /* (tmp == 7) */
> > +				chip->ecc_strength_ds = 40;
> 
> This ECC table looks incorrect. It matches my datasheet for H27UBG8T2B
> (26nm) but it is incorrect for my H27UBG8T2A (32nm). My datasheet says:

I do not a H27UBG8T2A chip, it is a bad news to me.
why Hynix can not keep the same syntax?
I will abandon the ECC info in the next version.

> 
>   0 ==> 1-bit/512-bytes
>   1 ==> 2-bits/512-bytes
>   2 ==> 4-bits/512-bytes
>   3 ==> 8-bits/512-bytes
>   4 ==> 16-bits/512-bytes
>   5 ==> 24-bits/2048-bytes
>   6 ==> 24-bits/1024-bytes
>   7 ==> Reserved
> 
> Not sure if that's a preliminary version that changed for production
> (the datasheet is Rev 0.6, from 2009; I don't have samples on hand for
> this one), but this demonstrates the larger problem I have with this
> patch series...
> 
> ...that keeping track of minimum ECC requirements is not a scalable
> practice for all flash, especially those that avoid using any proper
> standards. It's difficult enough just getting OOB/page/block sizes
> correct without adding this to the mix. If we add one more thing to get
> wrong (ECC strength/step), we become increasingly fragile.
> 
> Furthermore, it seems that GPMI is the only driver that needs this
not the only. The atmel-nand.c also uses it.

> information in Linux. All other systems determine these things within
> their bootloader, or through some restriction on the devices they
> support (e.g., only support Hamming ECC). So it doesn't seem 100% clear
> that we *must* detect the minimum ECC properties in nand_base. In fact,
> you even agreed (in another thread) that gpmi-nand should probably
> define a more precise nand-ecc-strength and nand-ecc-step-size binding;
> such a binding could eliminate the need for runtime auto-detection.

yes. you can add the new DTs when you are free. Or I add it when i
finish my job in my hands.

thanks
Huang Shijie

Re: [PATCH] mtd: nand: parse the Hynix nand which uses <26nm technology

[Huang Shijie]

于 2013年12月02日 17:01, Huang Shijie 写道:
> The Hynix uses different ID parsing rules for <26nm technology.
> We should check the id_data[5] for Hynix nand now.
>
> This patch adds the parsing code for the Hynix nand which use <26nm technology,
> and it also parses out the datasheet's minimum required ECC.
>
> Tested with H27UBG8T2CTR(8192 + 640).
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/nand_base.c |   65 +++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 64 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bd39f7b..4dab696 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3162,7 +3162,7 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
>  			(((extid >> 1) & 0x04) | (extid & 0x03));
>  		*busw = 0;
>  	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
> -			!nand_is_slc(chip)) {
> +			!nand_is_slc(chip) && (id_data[5] & 0x7) < 3) {
>  		unsigned int tmp;
>  
>  		/* Calc pagesize */
> @@ -3202,6 +3202,69 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
>  		else
>  			mtd->erasesize = (64 * 1024) << tmp;
>  		*busw = 0;
> +	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
> +			!nand_is_slc(chip) && (id_data[5] & 0x7) > 3) {
> +		unsigned int tmp;
> +
> +		/* Calc pagesize */
> +		mtd->writesize = 4096 << (extid & 0x03);
> +		extid >>= 2;
> +		/* Calc oobsize */
> +		switch (((extid >> 2) & 0x04) | (extid & 0x03)) {
> +		case 0:
> +			mtd->oobsize = 640;
> +			break;
> +		case 1:
> +			mtd->oobsize = 448;
> +			break;
> +		case 2:
> +			mtd->oobsize = 224;
> +			break;
> +		case 3:
> +			mtd->oobsize = 128;
> +			break;
> +		case 4:
> +			mtd->oobsize = 64;
> +			break;
> +		case 5:
> +			mtd->oobsize = 32;
> +			break;
> +		case 6:
> +			mtd->oobsize = 16;
> +			break;
> +		default:
> +			mtd->oobsize = 640;
> +			break;
> +		}
> +		extid >>= 2;
> +		/* Calc blocksize */
> +		tmp = ((extid >> 1) & 0x04) | (extid & 0x03);
> +		if (tmp < 0x03)
> +			mtd->erasesize = (128 * 1024) << tmp;
> +		else if (tmp == 0x03)
> +			mtd->erasesize = 768 * 1024;
> +		else
> +			mtd->erasesize = (64 * 1024) << tmp;
> +
> +		/* ecc info */
> +		tmp = (id_data[4] >> 4) & 0x7;
> +
> +		if (tmp < 4) {
> +			chip->ecc_strength_ds = 1 << tmp;
> +			chip->ecc_step_ds = 512;
> +		} else {
> +			chip->ecc_step_ds = 1024;
> +			if (tmp == 4)
> +				chip->ecc_strength_ds = 24;
> +			else if (tmp == 5)
> +				chip->ecc_strength_ds = 32;
> +			else if (tmp == 6)
> +				chip->ecc_strength_ds = 40;
> +			else /* (tmp == 7) */
> +				chip->ecc_strength_ds = 100;
> +		}
> +
> +		*busw = 0;
>  	} else {
>  		/* Calc pagesize */
>  		mtd->writesize = 1024 << (extid & 0x03);
just a ping.

any comment about these two patches?


thanks
Huang Shijie

Re: [PATCH] mtd: nand: parse the Hynix nand which uses <26nm technology

[Brian Norris]

Hi Huang,

On Mon, Dec 02, 2013 at 05:01:46PM +0800, Huang Shijie wrote:
> The Hynix uses different ID parsing rules for <26nm technology.
> We should check the id_data[5] for Hynix nand now.
> 
> This patch adds the parsing code for the Hynix nand which use <26nm technology,
> and it also parses out the datasheet's minimum required ECC.
> 
> Tested with H27UBG8T2CTR(8192 + 640).

I don't actually have a datasheet for the < 26nm generation yet. Are
there any publicly available? Or do I need to track down one myself? Not
sure if I have any contacts at Hynix...

> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3162,7 +3162,7 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
>  			(((extid >> 1) & 0x04) | (extid & 0x03));
>  		*busw = 0;
>  	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
> -			!nand_is_slc(chip)) {
> +			!nand_is_slc(chip) && (id_data[5] & 0x7) < 3) {

This should be <= 3, since '3' is for 26nm.

Also, I recommended (on the other patch) that you split Hynix out into
its own function, since it is growing too much and it shares many of the
same checks.

And please add a comment about the 'id_data[5] & 0x7' bitfield
represents in the new Hynix ext_id function, like in the comments near
the top of nand_decode_ext_id() that already describe the two different
kinds of Samsung and the Hynix MLC.

>  		unsigned int tmp;
>  
>  		/* Calc pagesize */
> @@ -3202,6 +3202,69 @@ static void nand_decode_ext_id(struct mtd_info *mtd, struct nand_chip *chip,
>  		else
>  			mtd->erasesize = (64 * 1024) << tmp;
>  		*busw = 0;
> +	} else if (id_len == 6 && id_data[0] == NAND_MFR_HYNIX &&
> +			!nand_is_slc(chip) && (id_data[5] & 0x7) > 3) {
> +		unsigned int tmp;
> +
> +		/* Calc pagesize */
> +		mtd->writesize = 4096 << (extid & 0x03);
> +		extid >>= 2;
> +		/* Calc oobsize */
> +		switch (((extid >> 2) & 0x04) | (extid & 0x03)) {

If you create a Hynix ext-ID function, the above bitfield calculation
can be shared.

> +		case 0:
> +			mtd->oobsize = 640;
> +			break;
> +		case 1:
> +			mtd->oobsize = 448;
> +			break;
> +		case 2:
> +			mtd->oobsize = 224;
> +			break;
> +		case 3:
> +			mtd->oobsize = 128;
> +			break;
> +		case 4:
> +			mtd->oobsize = 64;
> +			break;
> +		case 5:
> +			mtd->oobsize = 32;
> +			break;
> +		case 6:
> +			mtd->oobsize = 16;
> +			break;
> +		default:
> +			mtd->oobsize = 640;
> +			break;
> +		}
> +		extid >>= 2;
> +		/* Calc blocksize */
> +		tmp = ((extid >> 1) & 0x04) | (extid & 0x03);
> +		if (tmp < 0x03)
> +			mtd->erasesize = (128 * 1024) << tmp;
> +		else if (tmp == 0x03)
> +			mtd->erasesize = 768 * 1024;
> +		else
> +			mtd->erasesize = (64 * 1024) << tmp;

This entire erasesize calculation can be shared with the >=26nm version.

> +
> +		/* ecc info */

s/ecc/ECC/

> +		tmp = (id_data[4] >> 4) & 0x7;
> +
> +		if (tmp < 4) {
> +			chip->ecc_strength_ds = 1 << tmp;
> +			chip->ecc_step_ds = 512;
> +		} else {
> +			chip->ecc_step_ds = 1024;
> +			if (tmp == 4)
> +				chip->ecc_strength_ds = 24;
> +			else if (tmp == 5)
> +				chip->ecc_strength_ds = 32;
> +			else if (tmp == 6)
> +				chip->ecc_strength_ds = 40;
> +			else /* (tmp == 7) */
> +				chip->ecc_strength_ds = 100;
> +		}

My same criticism about parsing ECC info applies here. How can I trust
that this table will be at all maintainable in the future? I'm not
convinced it's actually necessary information to put here, and if it's
low-utility, then it's not worth maintaining, IMO. (Let's keep the ECC
discussion on the patch 2, where I responded in fuller detail though,
please.)

Brian