Patchwork [V4,1/3] mtd: add new fields to nand_flash_dev{}

login
register
mail settings
Submitter Huang Shijie
Date March 7, 2013, 10:49 a.m.
Message ID <1362653364-29993-2-git-send-email-b32955@freescale.com>
Download mbox | patch
Permalink /patch/225800/
State New
Headers show

Comments

Huang Shijie - March 7, 2013, 10:49 a.m.
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(-)
Artem Bityutskiy - March 13, 2013, 10:54 a.m.
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?
Huang Shijie - March 13, 2013, 1:04 p.m.
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/
Artem Bityutskiy - March 13, 2013, 1:13 p.m.
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!
Brian Norris - March 14, 2013, 5:30 a.m.
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

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