[PATCH v5 0/3] mtd: use the full-id as the keyword for some nand chips

[PATCH v5 0/3] mtd: use the full-id as the keyword for some nand chips

[Huang Shijie]

As time goes on, we begin to meet the situation that we can not
get enough information from some nand chips's id data.
Take some Toshiba's nand chips for example.
I have 4 Toshiba's nand chips in my hand:
	TC58NVG2S0F, TC58NVG3S0F, TC58NVG5D2, TC58NVG6D2

When we read these chips' datasheets, we will get the geometry of these chips:
	TC58NVG2S0F : 4096 + 224
	TC58NVG3S0F : 4096 + 232
	TC58NVG5D2  : 8192 + 640
	TC58NVG6D2  : 8192 + 640

But we can not parse out the correct oob size for these chips from the id data.
So it is time to add some new fields to the nand_flash_dev{},
and update the detection mechanisms.

v4 --> v4:
	[1] remove the id_len field.
	[2] based on Artem "mtd: nand: use more reasonable integer types"
	[3] add more comments.

v3 --> v4:
	[1] rewrite the code based on the latest l2-mtd.
	[2] add the full-id nand in the nand_flash_lds.

v2 --> v3:
	[1] remove the duplicated header.
	[2] remove the field "ecc_len" in nand_flash_dev{}.
	[3] fix some coding style warnings.
	[4] add more comments

Huang Shijie (3):
  mtd: add a new field for nand_flash_dev{}
  mtd: add the support to parse out the full-id nand type
  mtd: add 4 Toshiba nand chips for the full-id case

 drivers/mtd/nand/nand_base.c |   36 +++++++++++++++++++++++++++++++++---
 drivers/mtd/nand/nand_ids.c  |   22 ++++++++++++++++++++++
 include/linux/mtd/nand.h     |    2 ++
 3 files changed, 57 insertions(+), 3 deletions(-)

[PATCH v5 1/3] mtd: add a new field for nand_flash_dev{}

[Huang Shijie]

As time goes on, we begin to meet the situation that we can not get enough
information from some nand chips's id data. Take some Toshiba's nand chips
for example. I have 4 Toshiba's nand chips in my hand:
	TC58NVG2S0F, TC58NVG3S0F, TC58NVG5D2, TC58NVG6D2

When we read these chips' datasheets, we will get the geometry of these chips:
	TC58NVG2S0F : 4096 + 224
	TC58NVG3S0F : 4096 + 232
	TC58NVG5D2  : 8192 + 640
	TC58NVG6D2  : 8192 + 640

But we can not parse out the correct oob size for these chips from the id data.

This patch adds @oobsize for the nand_flash_dev{} to store the oob size for
these nands.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 include/linux/mtd/nand.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index e2c7173..42862e9 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -581,6 +581,7 @@ struct nand_chip {
  * @chipsize: total chip size in MiB
  * @erasesize: eraseblock size in bytes (determined from the extended ID if 0)
  * @options: stores various chip bit options
+ * @oobsize: OOB size
  */
 struct nand_flash_dev {
 	char *name;
@@ -595,6 +596,7 @@ struct nand_flash_dev {
 	unsigned int chipsize;
 	unsigned int erasesize;
 	unsigned int options;
+	unsigned int oobsize;
 };
 
 /**
-- 
1.7.1

[PATCH v5 2/3] mtd: add the support to parse out the full-id nand type

[Huang Shijie]

When we meet a full-id nand type which @mfr_id is true, we can use
the find_full_id_nand() to parse out the neccessary information for a
nand chip.

If we meet a non full-id nand type, we can handle it in the lagacy way.

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

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 72eada2..b7ad9fd 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3123,6 +3123,30 @@ static void nand_decode_bbm_options(struct mtd_info *mtd,
 		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
 }
 
+static inline bool is_full_id_nand(struct nand_flash_dev *type)
+{
+	return type->mfr_id;
+}
+
+static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
+		   struct nand_flash_dev *type, u8 *id_data, int *busw)
+{
+	if (!strncmp(type->id, id_data, 8)) {
+		mtd->writesize = type->pagesize;
+		mtd->erasesize = type->erasesize;
+		mtd->oobsize = type->oobsize;
+
+		chip->cellinfo = id_data[2];
+		chip->chipsize = (uint64_t)type->chipsize << 20;
+		chip->options |= type->options;
+
+		*busw = type->options & NAND_BUSWIDTH_16;
+
+		return true;
+	}
+	return false;
+}
+
 /*
  * Get the flash and manufacturer id and lookup if the type is supported.
  */
@@ -3174,9 +3198,15 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	if (!type)
 		type = nand_flash_ids;
 
-	for (; type->name != NULL; type++)
-		if (*dev_id == type->dev_id)
-			break;
+	for (; type->name != NULL; type++) {
+		if (is_full_id_nand(type)) {
+			if (find_full_id_nand(mtd, chip, type, id_data, &busw))
+				goto ident_done;
+		} else {
+			if (*dev_id == type->dev_id)
+				break;
+		}
+	}
 
 	chip->onfi_version = 0;
 	if (!type->name || !type->pagesize) {
-- 
1.7.1

Re: [PATCH v5 2/3] mtd: add the support to parse out the full-id nand type

[Brian Norris]

Hi Huang,

A few nitpicks, and a few real comments.

On Wed, Mar 13, 2013 at 7:59 PM, Huang Shijie <b32955@freescale.com> wrote:
> When we meet a full-id nand type which @mfr_id is true, we can use
> the find_full_id_nand() to parse out the neccessary information for a

s/neccessary/necessary

> nand chip.
>
> If we meet a non full-id nand type, we can handle it in the lagacy way.

s/lagacy/legacy

> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/nand_base.c |   36 +++++++++++++++++++++++++++++++++---
>  1 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 72eada2..b7ad9fd 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3123,6 +3123,30 @@ static void nand_decode_bbm_options(struct mtd_info *mtd,
>                 chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>  }
>
> +static inline bool is_full_id_nand(struct nand_flash_dev *type)
> +{
> +       return type->mfr_id;
> +}

If we restore the id_len field (as I recommended in reply to the cover
letter), then I think this check would be better performed as:

return !type->id_len;

That would really be a more specific and accurate test and can
compensate for some wildly unlikely case in which a manufacturer uses
manufacturer ID = 0.

> +static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
> +                  struct nand_flash_dev *type, u8 *id_data, int *busw)
> +{
> +       if (!strncmp(type->id, id_data, 8)) {
> +               mtd->writesize = type->pagesize;
> +               mtd->erasesize = type->erasesize;
> +               mtd->oobsize = type->oobsize;
> +
> +               chip->cellinfo = id_data[2];
> +               chip->chipsize = (uint64_t)type->chipsize << 20;
> +               chip->options |= type->options;
> +
> +               *busw = type->options & NAND_BUSWIDTH_16;
> +
> +               return true;
> +       }
> +       return false;
> +}
> +
>  /*
>   * Get the flash and manufacturer id and lookup if the type is supported.
>   */
> @@ -3174,9 +3198,15 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>         if (!type)
>                 type = nand_flash_ids;
>
> -       for (; type->name != NULL; type++)
> -               if (*dev_id == type->dev_id)
> -                       break;
> +       for (; type->name != NULL; type++) {
> +               if (is_full_id_nand(type)) {
> +                       if (find_full_id_nand(mtd, chip, type, id_data, &busw))
> +                               goto ident_done;
> +               } else {
> +                       if (*dev_id == type->dev_id)

Combine the previous two lines to:

} else if (*dev_id == type->dev_id) {

> +                               break;
> +               }
> +       }
>
>         chip->onfi_version = 0;
>         if (!type->name || !type->pagesize) {

Brian

[PATCH v5 3/3] mtd: add 4 Toshiba nand chips for the full-id case

[Huang Shijie]

I have 4 Toshiba nand chips which can not be parsed out by the
id data. We can not get the oob size from the id data. So add them
as the full-id nand chips in the first of nand_flash_ids.

The nand_get_flash_type() scans the full id nands firstly.
If a full-id nand matchs, it will not continue to parse other
non-full-id nand types, else it will continue to parse the non-full-id nands.

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

diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index 625bc89..38b8cee 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -10,6 +10,7 @@
  */
 #include <linux/module.h>
 #include <linux/mtd/nand.h>
+#include <linux/sizes.h>
 
 #define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS
 #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16)
@@ -22,6 +23,27 @@
  * extended chip ID.
  */
 struct nand_flash_dev nand_flash_ids[] = {
+	/*
+	 * The full-id nands may share the same Device ID with the non-full-id
+	 * nands. In order to distinguish the two type nands, we put the
+	 * full-id nands in the first of the table. So the nand_get_flash_type()
+	 * scans the full id nands firstly. If a full-id nand matchs, it will
+	 * not continue to parse other non-full-id nand types, else it will
+	 * continue to parse the non-full-id nands.
+	 */
+	{"TC58NVG2S0F 4G 3.3V 8-bit",
+		{ .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08} },
+			SZ_4K, SZ_512, SZ_256K, 0, 224},
+	{"TC58NVG3S0F 8G 3.3V 8-bit",
+		{ .id = {0x98, 0xd3, 0x90, 0x26, 0x76, 0x15, 0x02, 0x08} },
+			SZ_4K, SZ_1K, SZ_256K, 0, 232},
+	{"TC58NVG5D2 32G 3.3V 8-bit",
+		{ .id = {0x98, 0xd7, 0x94, 0x32, 0x76, 0x56, 0x09, 0x00} },
+			SZ_8K, SZ_4K, SZ_1M, 0, 640},
+	{"TC58NVG6D2 64G 3.3V 8-bit",
+		{ .id = {0x98, 0xde, 0x94, 0x82, 0x76, 0x56, 0x04, 0x20} },
+			SZ_8K, SZ_8K, SZ_2M, 0, 640},
+
 	LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 512, 4, 0x2000, 0),
 	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 512, 4, 0x2000, 0),
 	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE5, 512, 4, 0x2000, 0),
-- 
1.7.1

Re: [PATCH v5 3/3] mtd: add 4 Toshiba nand chips for the full-id case

[Brian Norris]

On Wed, Mar 13, 2013 at 7:59 PM, Huang Shijie <b32955@freescale.com> wrote:
> I have 4 Toshiba nand chips which can not be parsed out by the
> id data. We can not get the oob size from the id data. So add them
> as the full-id nand chips in the first of nand_flash_ids.
>
> The nand_get_flash_type() scans the full id nands firstly.
> If a full-id nand matchs, it will not continue to parse other
> non-full-id nand types, else it will continue to parse the non-full-id nands.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/nand/nand_ids.c |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
> index 625bc89..38b8cee 100644
> --- a/drivers/mtd/nand/nand_ids.c
> +++ b/drivers/mtd/nand/nand_ids.c
> @@ -10,6 +10,7 @@
>   */
>  #include <linux/module.h>
>  #include <linux/mtd/nand.h>
> +#include <linux/sizes.h>
>
>  #define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS
>  #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16)
> @@ -22,6 +23,27 @@
>   * extended chip ID.
>   */
>  struct nand_flash_dev nand_flash_ids[] = {
> +       /*
> +        * The full-id nands may share the same Device ID with the non-full-id
> +        * nands. In order to distinguish the two type nands, we put the
> +        * full-id nands in the first of the table. So the nand_get_flash_type()
> +        * scans the full id nands firstly. If a full-id nand matchs, it will
> +        * not continue to parse other non-full-id nand types, else it will
> +        * continue to parse the non-full-id nands.
> +        */

There are a few grammar/language issues (nands -> NAND chips, id ->
ID, firstly -> first). Also, I don't think you need to explain the
full search here. I would rewrite this whole paragraph as:

"Some incompatible NAND chips share device ID's and so must be listed
by full ID. We list them first so that we can easily identify the most
specific match."

> +       {"TC58NVG2S0F 4G 3.3V 8-bit",
> +               { .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08} },
> +                       SZ_4K, SZ_512, SZ_256K, 0, 224},
> +       {"TC58NVG3S0F 8G 3.3V 8-bit",
> +               { .id = {0x98, 0xd3, 0x90, 0x26, 0x76, 0x15, 0x02, 0x08} },
> +                       SZ_4K, SZ_1K, SZ_256K, 0, 232},
> +       {"TC58NVG5D2 32G 3.3V 8-bit",
> +               { .id = {0x98, 0xd7, 0x94, 0x32, 0x76, 0x56, 0x09, 0x00} },
> +                       SZ_8K, SZ_4K, SZ_1M, 0, 640},
> +       {"TC58NVG6D2 64G 3.3V 8-bit",
> +               { .id = {0x98, 0xde, 0x94, 0x82, 0x76, 0x56, 0x04, 0x20} },
> +                       SZ_8K, SZ_8K, SZ_2M, 0, 640},
> +
>         LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 512, 4, 0x2000, 0),
>         LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 512, 4, 0x2000, 0),
>         LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE5, 512, 4, 0x2000, 0),

Brian

Re: [PATCH v5 3/3] mtd: add 4 Toshiba nand chips for the full-id case

[Huang Shijie]

于 2013年03月14日 01:10, Brian Norris 写道:
> On Wed, Mar 13, 2013 at 7:59 PM, Huang Shijie<b32955@freescale.com>  wrote:
>> I have 4 Toshiba nand chips which can not be parsed out by the
>> id data. We can not get the oob size from the id data. So add them
>> as the full-id nand chips in the first of nand_flash_ids.
>>
>> The nand_get_flash_type() scans the full id nands firstly.
>> If a full-id nand matchs, it will not continue to parse other
>> non-full-id nand types, else it will continue to parse the non-full-id nands.
>>
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>> ---
>>   drivers/mtd/nand/nand_ids.c |   22 ++++++++++++++++++++++
>>   1 files changed, 22 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
>> index 625bc89..38b8cee 100644
>> --- a/drivers/mtd/nand/nand_ids.c
>> +++ b/drivers/mtd/nand/nand_ids.c
>> @@ -10,6 +10,7 @@
>>    */
>>   #include<linux/module.h>
>>   #include<linux/mtd/nand.h>
>> +#include<linux/sizes.h>
>>
>>   #define LP_OPTIONS NAND_SAMSUNG_LP_OPTIONS
>>   #define LP_OPTIONS16 (LP_OPTIONS | NAND_BUSWIDTH_16)
>> @@ -22,6 +23,27 @@
>>    * extended chip ID.
>>    */
>>   struct nand_flash_dev nand_flash_ids[] = {
>> +       /*
>> +        * The full-id nands may share the same Device ID with the non-full-id
>> +        * nands. In order to distinguish the two type nands, we put the
>> +        * full-id nands in the first of the table. So the nand_get_flash_type()
>> +        * scans the full id nands firstly. If a full-id nand matchs, it will
>> +        * not continue to parse other non-full-id nand types, else it will
>> +        * continue to parse the non-full-id nands.
>> +        */
> There are a few grammar/language issues (nands ->  NAND chips, id ->
> ID, firstly ->  first). Also, I don't think you need to explain the
> full search here. I would rewrite this whole paragraph as:
>
sorry for my poor english.

> "Some incompatible NAND chips share device ID's and so must be listed
> by full ID. We list them first so that we can easily identify the most
> specific match."
>
thanks. I will use this description.



Huang Shijie

Re: [PATCH v5 3/3] mtd: add 4 Toshiba nand chips for the full-id case

[Brian Norris]

On 03/14/2013 07:29 PM, Huang Shijie wrote:
> sorry for my poor english.

No need to apologize.

Thanks for braving through the 6 iterations of this! We were going to 
need this eventually anyway. Now that the dust is settling, your v6 is 
looking pretty good. I'll have a closer look later and probably give my 
Acked/Reviewed-by.

Brian

Re: [PATCH v5 0/3] mtd: use the full-id as the keyword for some nand chips

[Brian Norris]

On Wed, Mar 13, 2013 at 7:59 PM, Huang Shijie <b32955@freescale.com> wrote:
> As time goes on, we begin to meet the situation that we can not
> get enough information from some nand chips's id data.
> Take some Toshiba's nand chips for example.
> I have 4 Toshiba's nand chips in my hand:
>         TC58NVG2S0F, TC58NVG3S0F, TC58NVG5D2, TC58NVG6D2
>
> When we read these chips' datasheets, we will get the geometry of these chips:
>         TC58NVG2S0F : 4096 + 224
>         TC58NVG3S0F : 4096 + 232
>         TC58NVG5D2  : 8192 + 640
>         TC58NVG6D2  : 8192 + 640
>
> But we can not parse out the correct oob size for these chips from the id data.
> So it is time to add some new fields to the nand_flash_dev{},
> and update the detection mechanisms.
>
> v4 --> v4:
>         [1] remove the id_len field.
>         [2] based on Artem "mtd: nand: use more reasonable integer types"
>         [3] add more comments.

Sorry for not posting this earlier, but why don't we want an id_len
field? NAND chips often only have a 5-byte or 6-byte ID "defined" in
their datasheet, and so the next few bytes aren't guaranteed to be any
particular value. Wouldn't we want to accommodate for any potential
variation in the "undefined" bytes? (These bytes do typically have a
pattern, and in fact, we currently rely on that fact.) Also, I can
easily foresee a situation in which someone might want to support NAND
based solely on the datasheet, not waiting till they get a sample of
that chip in their hands. In that case, they cannot specify a full 8
bytes in the table; they can (and should) only specify the few
substring found in their datasheet.

Really, the only argument I see for dropping id_len is to save space.
I think this is a bad choice.

... <snip> ...

Brian

Re: [PATCH v5 0/3] mtd: use the full-id as the keyword for some nand chips

[Artem Bityutskiy]

On Wed, 2013-03-13 at 21:48 -0700, Brian Norris wrote:
> On Wed, Mar 13, 2013 at 7:59 PM, Huang Shijie <b32955@freescale.com> wrote:
> > As time goes on, we begin to meet the situation that we can not
> > get enough information from some nand chips's id data.
> > Take some Toshiba's nand chips for example.
> > I have 4 Toshiba's nand chips in my hand:
> >         TC58NVG2S0F, TC58NVG3S0F, TC58NVG5D2, TC58NVG6D2
> >
> > When we read these chips' datasheets, we will get the geometry of these chips:
> >         TC58NVG2S0F : 4096 + 224
> >         TC58NVG3S0F : 4096 + 232
> >         TC58NVG5D2  : 8192 + 640
> >         TC58NVG6D2  : 8192 + 640
> >
> > But we can not parse out the correct oob size for these chips from the id data.
> > So it is time to add some new fields to the nand_flash_dev{},
> > and update the detection mechanisms.
> >
> > v4 --> v4:
> >         [1] remove the id_len field.
> >         [2] based on Artem "mtd: nand: use more reasonable integer types"
> >         [3] add more comments.
> 
> Sorry for not posting this earlier, but why don't we want an id_len
> field? NAND chips often only have a 5-byte or 6-byte ID "defined" in
> their datasheet, and so the next few bytes aren't guaranteed to be any
> particular value. Wouldn't we want to accommodate for any potential
> variation in the "undefined" bytes? (These bytes do typically have a
> pattern, and in fact, we currently rely on that fact.) Also, I can
> easily foresee a situation in which someone might want to support NAND
> based solely on the datasheet, not waiting till they get a sample of
> that chip in their hands. In that case, they cannot specify a full 8
> bytes in the table; they can (and should) only specify the few
> substring found in their datasheet.
> 
> Really, the only argument I see for dropping id_len is to save space.
> I think this is a bad choice.

Let's take a flash which has 5 bytes ID length, suppose it is
{1, 2, 3, 4, 5}. The 8-byte table value for this flash would be:
{1, 2, 3, 4, 5, 0, 0, 0}. When we read the ID sequence, we need to start
with allocating an 8-byte array and initializing it to 0. Then we read
the 5-byte ID and end up with: {1, 2, 3, 4, 5, 0, 0, 0}. And we
successfully match the table entry.

That was my thinking.

There is a potential problem: there may be a flash with 6 bytes ID with
value {1, 2, 3, 4, 5, 0}, in which case the algorithm would not work.

However, I consider this to be very unlikely. And even if this unlikely
situation happens, it will probably be noticed and we could fix this
with adding the ID length field.

My rationale is avoiding over-designing and trying to cover low
probability theoretical cases.

But yes, I did not really strongly opposed the field, it was more that I
asked questions from Huang about why is this needed, and expected to
hear some justification. Huang preferred to answer that he does not
really need this, and I just thought that if this is all he can say,
then he should not add it.

This is often, generally, the role of maintainer who cannot get into all
the details - ask questions and validate the answers. Generally, good
answers have correlation with quality code.

You did provide good arguments thanks! If my rationally is not
convincing enough and you think this is not over-engineering, let's have
the ID length field.

BTW, Huang, I think we should introduce a Macro instead of the '8'
constant. And if ATM we do not have 8-byte ID's in our table, we should
make the macro to contain a smaller number. This number can be increased
when needed.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH v5 0/3] mtd: use the full-id as the keyword for some nand chips

[Brian Norris]

On 03/14/2013 12:07 AM, Artem Bityutskiy wrote:
> On Wed, 2013-03-13 at 21:48 -0700, Brian Norris wrote:
>> On Wed, Mar 13, 2013 at 7:59 PM, Huang Shijie <b32955@freescale.com> wrote:
>>> As time goes on, we begin to meet the situation that we can not
>>> get enough information from some nand chips's id data.
>>> Take some Toshiba's nand chips for example.
>>> I have 4 Toshiba's nand chips in my hand:
>>>          TC58NVG2S0F, TC58NVG3S0F, TC58NVG5D2, TC58NVG6D2
>>>
>>> When we read these chips' datasheets, we will get the geometry of these chips:
>>>          TC58NVG2S0F : 4096 + 224
>>>          TC58NVG3S0F : 4096 + 232
>>>          TC58NVG5D2  : 8192 + 640
>>>          TC58NVG6D2  : 8192 + 640
>>>
>>> But we can not parse out the correct oob size for these chips from the id data.
>>> So it is time to add some new fields to the nand_flash_dev{},
>>> and update the detection mechanisms.
>>>
>>> v4 --> v4:
>>>          [1] remove the id_len field.
>>>          [2] based on Artem "mtd: nand: use more reasonable integer types"
>>>          [3] add more comments.
>>
>> Sorry for not posting this earlier, but why don't we want an id_len
>> field? NAND chips often only have a 5-byte or 6-byte ID "defined" in
>> their datasheet, and so the next few bytes aren't guaranteed to be any
>> particular value. Wouldn't we want to accommodate for any potential
>> variation in the "undefined" bytes? (These bytes do typically have a
>> pattern, and in fact, we currently rely on that fact.) Also, I can
>> easily foresee a situation in which someone might want to support NAND
>> based solely on the datasheet, not waiting till they get a sample of
>> that chip in their hands. In that case, they cannot specify a full 8
>> bytes in the table; they can (and should) only specify the few
>> substring found in their datasheet.
>>
>> Really, the only argument I see for dropping id_len is to save space.
>> I think this is a bad choice.
>
> Let's take a flash which has 5 bytes ID length, suppose it is
> {1, 2, 3, 4, 5}. The 8-byte table value for this flash would be:
> {1, 2, 3, 4, 5, 0, 0, 0}. When we read the ID sequence, we need to start
> with allocating an 8-byte array and initializing it to 0. Then we read
> the 5-byte ID and end up with: {1, 2, 3, 4, 5, 0, 0, 0}. And we

When we "read the 5-byte ID", are you referring to reading the ID from 
the NAND flash? Unfortunately, things are *never* this nice. Those last 
3 bytes can be anything. Often they wrap around; see my nand_id_len() 
function. But they rarely are 0.

So, we have no way to identify (100% guaranteed) a 5-byte or 6-byte ID 
that comes from the flash. But we *can* compare a substring of it 
against 5 or 6 byte ID in our table, if they are marked with lengths.

> successfully match the table entry.
>
> That was my thinking.
>
> There is a potential problem: there may be a flash with 6 bytes ID with
> value {1, 2, 3, 4, 5, 0}, in which case the algorithm would not work.
>
> However, I consider this to be very unlikely. And even if this unlikely
> situation happens, it will probably be noticed and we could fix this
> with adding the ID length field.
>
> My rationale is avoiding over-designing and trying to cover low
> probability theoretical cases.

I agree about avoiding over-designing. I only would add that my 
scenario's are not so theoretical. It just happens that I (and others) 
have successfully managed to devise heuristics for most of the 5-byte 
and 6-byte IDs I've seen.

> But yes, I did not really strongly opposed the field, it was more that I
> asked questions from Huang about why is this needed, and expected to
> hear some justification. Huang preferred to answer that he does not
> really need this, and I just thought that if this is all he can say,
> then he should not add it.
>
> This is often, generally, the role of maintainer who cannot get into all
> the details - ask questions and validate the answers. Generally, good
> answers have correlation with quality code.

That is entirely reasonable.

> You did provide good arguments thanks! If my rationally is not
> convincing enough and you think this is not over-engineering, let's have
> the ID length field.

I do not think that it is over-engineering, but with the immediate needs 
(Huang's patch set), it is not necessary. I am OK with either result. 
It's not too difficult to add the field as needed.

> BTW, Huang, I think we should introduce a Macro instead of the '8'
> constant. And if ATM we do not have 8-byte ID's in our table, we should
> make the macro to contain a smaller number. This number can be increased
> when needed.

Sounds like a good idea. (But I don't expect that this will need increased.)

Brian