| Message ID | 1362653364-29993-2-git-send-email-b32955@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
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?
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/
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!
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
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; }; /**
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(-)