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

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

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 new fields to nand_flash_dev{}
  mtd: add 4 Toshiba nand chips for the full-id case
  mtd: add the support to parse out the full-id nand type

 drivers/mtd/nand/nand_base.c |   36 +++++++++++++++++++++++++++++++++---
 drivers/mtd/nand/nand_ids.c  |   15 +++++++++++++++
 include/linux/mtd/nand.h     |    4 ++++
 3 files changed, 52 insertions(+), 3 deletions(-)

[PATCH V4 1/3] mtd: add new fields to 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 some new fields to the nand_flash_dev{}:
  @id_len: the valid length of the id data. See the comments in
           nand_id_has_period()
  @oobsize: the oob size.

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

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 591eeeb..f0a9d93 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -578,6 +578,8 @@ struct nand_chip {
  * @erasesize: eraseblock size in bytes (determined from the extended ID if 0)
  * @chipsize: total chip size in MiB
  * @options: stores various chip bit options
+ * @id_len: The valid length of the @id.
+ * @oobsize: OOB size
  */
 struct nand_flash_dev {
 	char *name;
@@ -592,6 +594,8 @@ struct nand_flash_dev {
 	unsigned long chipsize;
 	unsigned long erasesize;
 	unsigned long options;
+	unsigned long id_len;
+	unsigned long oobsize;
 };
 
 /**
-- 
1.7.1

Re: [PATCH V4 1/3] mtd: add new fields to nand_flash_dev{}

[Artem Bityutskiy]

On Thu, 2013-03-07 at 18:49 +0800, Huang Shijie wrote:
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 591eeeb..f0a9d93 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -578,6 +578,8 @@ struct nand_chip {
>   * @erasesize: eraseblock size in bytes (determined from the extended ID if 0)
>   * @chipsize: total chip size in MiB
>   * @options: stores various chip bit options
> + * @id_len: The valid length of the @id.
> + * @oobsize: OOB size
>   */
>  struct nand_flash_dev {
>  	char *name;
> @@ -592,6 +594,8 @@ struct nand_flash_dev {
>  	unsigned long chipsize;
>  	unsigned long erasesize;
>  	unsigned long options;
> +	unsigned long id_len;
> +	unsigned long oobsize;
>  };

Why are these of type 'long', which is 64 bits in 64-bit architectures,
which seems to be unnecessarily big. Wouldn't 'unsigned int' be enough?

Also, can we avoid having the 'id_len' field? Can the end of the
sequence of ID's be marked with a '0' or '0xFF' marker instead?

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH V4 1/3] mtd: add new fields to nand_flash_dev{}

[Huang Shijie]

On Wed, Mar 13, 2013 at 6:54 PM, Artem Bityutskiy
<artem.bityutskiy@linux.intel.com> wrote:
> On Thu, 2013-03-07 at 18:49 +0800, Huang Shijie wrote:
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index 591eeeb..f0a9d93 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -578,6 +578,8 @@ struct nand_chip {
>>   * @erasesize: eraseblock size in bytes (determined from the extended ID if 0)
>>   * @chipsize: total chip size in MiB
>>   * @options: stores various chip bit options
>> + * @id_len: The valid length of the @id.
>> + * @oobsize: OOB size
>>   */
>>  struct nand_flash_dev {
>>       char *name;
>> @@ -592,6 +594,8 @@ struct nand_flash_dev {
>>       unsigned long chipsize;
>>       unsigned long erasesize;
>>       unsigned long options;
>> +     unsigned long id_len;
>> +     unsigned long oobsize;
>>  };
>
> Why are these of type 'long', which is 64 bits in 64-bit architectures,
> which seems to be unnecessarily big. Wouldn't 'unsigned int' be enough?
>
Frankly speaking, "uint16_t" is enough.
"unsigned int" is too long for both the fields.

> Also, can we avoid having the 'id_len' field? Can the end of the

For these 4 toshiba nand, we do not need the id_len field.

> sequence of ID's be marked with a '0' or '0xFF' marker instead?

Are you sure that 0 or 0xff are not the valid data during the 8byte id data?
I am not sure.

Maybe in future, we may meet a nand which use the 0xff as a valid id-data.

But we can remove this id_len field now. It's ok to me.

thanks
Huang Shijie

>
> --
> Best Regards,
> Artem Bityutskiy
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V4 1/3] mtd: add new fields to nand_flash_dev{}

[Artem Bityutskiy]

On Wed, 2013-03-13 at 21:04 +0800, Huang Shijie wrote:
> On Wed, Mar 13, 2013 at 6:54 PM, Artem Bityutskiy
> <artem.bityutskiy@linux.intel.com> wrote:
> > On Thu, 2013-03-07 at 18:49 +0800, Huang Shijie wrote:
> >> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> >> index 591eeeb..f0a9d93 100644
> >> --- a/include/linux/mtd/nand.h
> >> +++ b/include/linux/mtd/nand.h
> >> @@ -578,6 +578,8 @@ struct nand_chip {
> >>   * @erasesize: eraseblock size in bytes (determined from the extended ID if 0)
> >>   * @chipsize: total chip size in MiB
> >>   * @options: stores various chip bit options
> >> + * @id_len: The valid length of the @id.
> >> + * @oobsize: OOB size
> >>   */
> >>  struct nand_flash_dev {
> >>       char *name;
> >> @@ -592,6 +594,8 @@ struct nand_flash_dev {
> >>       unsigned long chipsize;
> >>       unsigned long erasesize;
> >>       unsigned long options;
> >> +     unsigned long id_len;
> >> +     unsigned long oobsize;
> >>  };
> >
> > Why are these of type 'long', which is 64 bits in 64-bit architectures,
> > which seems to be unnecessarily big. Wouldn't 'unsigned int' be enough?
> >
> Frankly speaking, "uint16_t" is enough.
> "unsigned int" is too long for both the fields.

Adding a single uint16_t is useless because GCC will pad it to 32 bits
anyway. I send a patch which turns all the longs to ints. Just add
another int. And yes, do not add id_len unless you need it. Thanks!

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH V4 1/3] mtd: add new fields to nand_flash_dev{}

[Brian Norris]

On Wed, Mar 13, 2013 at 6:04 AM, Huang Shijie <shijie8@gmail.com> wrote:
> On Wed, Mar 13, 2013 at 6:54 PM, Artem Bityutskiy
> <artem.bityutskiy@linux.intel.com> wrote:
>> On Thu, 2013-03-07 at 18:49 +0800, Huang Shijie wrote:
>>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>>> index 591eeeb..f0a9d93 100644
>>> --- a/include/linux/mtd/nand.h
>>> +++ b/include/linux/mtd/nand.h
>>> @@ -578,6 +578,8 @@ struct nand_chip {
>>>   * @erasesize: eraseblock size in bytes (determined from the extended ID if 0)
>>>   * @chipsize: total chip size in MiB
>>>   * @options: stores various chip bit options
>>> + * @id_len: The valid length of the @id.
>>> + * @oobsize: OOB size
>>>   */
>>>  struct nand_flash_dev {
>>>       char *name;
>>> @@ -592,6 +594,8 @@ struct nand_flash_dev {
>>>       unsigned long chipsize;
>>>       unsigned long erasesize;
>>>       unsigned long options;
>>> +     unsigned long id_len;
>>> +     unsigned long oobsize;
>>>  };
>>
>> Why are these of type 'long', which is 64 bits in 64-bit architectures,
>> which seems to be unnecessarily big. Wouldn't 'unsigned int' be enough?
>>
> Frankly speaking, "uint16_t" is enough.
> "unsigned int" is too long for both the fields.
>
>> Also, can we avoid having the 'id_len' field? Can the end of the
>
> For these 4 toshiba nand, we do not need the id_len field.
>
>> sequence of ID's be marked with a '0' or '0xFF' marker instead?
>
> Are you sure that 0 or 0xff are not the valid data during the 8byte id data?
> I am not sure.

I am sure that 0x00 is valid data. But I do not see any with 0xFF. Check:

http://www.linux-mtd.infradead.org/nand-data/nanddata.html

Anyway, this sort of magic ID byte is no good, IMO.

> Maybe in future, we may meet a nand which use the 0xff as a valid id-data.
>
> But we can remove this id_len field now. It's ok to me.

As I noted elsewhere, I would prefer an id_len field. But I can also
add one if/when it comes up, if we really must.

BTW, I was just thinking; if you really want to save data space, we
should have just made two totally separate tables: one with full IDs
and one dev-id patterns. The first table should (hopefully) have fewer
entries but can have a few extra columns, for a good purpose.

But I'm not sure that space is worth it. We waste more (?) space with
our static nand_ecclayout structs which aren't even going to be used
by many drivers. Even those that are "used" aren't very useful, as no
one really uses OOB for FS data much. (And to go off on more
tangents...those structs could by shrunk easily with a few 'uint16_t',
as they are no longer directly exported as the user ABI.)

Brian

[PATCH V4 2/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.

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

diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index 625bc89..7805271 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,20 @@
  * extended chip ID.
  */
 struct nand_flash_dev nand_flash_ids[] = {
+	/* TOSHIBA */
+	{"TC58NVG2S0F 4G 3.3V 8-bit",
+		{ .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08}},
+			SZ_4K, SZ_512, SZ_256K, 0, 8, 224},
+	{"TC58NVG3S0F 8G 3.3V 8-bit",
+		{ .id = {0x98, 0xd3, 0x90, 0x26, 0x76, 0x15, 0x02, 0x08}},
+			SZ_4K, SZ_1K, SZ_256K, 0, 8, 232},
+	{"TC58NVG5D2 32G 3.3V 8-bit",
+		{ .id = {0x98, 0xd7, 0x94, 0x32, 0x76, 0x56, 0x09, 0x00}},
+			SZ_8K, SZ_4K, SZ_1M, 0, 8, 640},
+	{"TC58NVG6D2 64G 3.3V 8-bit",
+		{ .id = {0x98, 0xde, 0x94, 0x82, 0x76, 0x56, 0x04, 0x20}},
+			SZ_8K, SZ_8K, SZ_2M, 0, 8, 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 V4 2/3] mtd: add 4 Toshiba nand chips for the full-id case

[Artem Bityutskiy]

On Thu, 2013-03-07 at 18:49 +0800, Huang Shijie wrote:
> +	/* TOSHIBA */
> +	{"TC58NVG2S0F 4G 3.3V 8-bit",
> +		{ .id = {0x98, 0xdc, 0x90, 0x26, 0x76, 0x15, 0x01, 0x08}},
> +			SZ_4K, SZ_512, SZ_256K, 0, 8, 224},
> +	{"TC58NVG3S0F 8G 3.3V 8-bit",
> +		{ .id = {0x98, 0xd3, 0x90, 0x26, 0x76, 0x15, 0x02, 0x08}},
> +			SZ_4K, SZ_1K, SZ_256K, 0, 8, 232},
> +	{"TC58NVG5D2 32G 3.3V 8-bit",
> +		{ .id = {0x98, 0xd7, 0x94, 0x32, 0x76, 0x56, 0x09, 0x00}},
> +			SZ_8K, SZ_4K, SZ_1M, 0, 8, 640},
> +	{"TC58NVG6D2 64G 3.3V 8-bit",
> +		{ .id = {0x98, 0xde, 0x94, 0x82, 0x76, 0x56, 0x04, 0x20}},
> +			SZ_8K, SZ_8K, SZ_2M, 0, 8, 640},

We need a useful comment above this block explaining that it is
important to have full ID records first.

-- 
Best Regards,
Artem Bityutskiy

[PATCH V4 3/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 29c0378..b9a24ff 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3074,6 +3074,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, type->id_len)) {
+		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.
  */
@@ -3125,9 +3149,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