Patchwork [12/12] mtd: nand: provision full ID support

login
register
mail settings
Submitter Artem Bityutskiy
Date March 4, 2013, 4:42 p.m.
Message ID <1362415349-7107-13-git-send-email-dedekind1@gmail.com>
Download mbox | patch
Permalink /patch/224757/
State New
Headers show

Comments

Artem Bityutskiy - March 4, 2013, 4:42 p.m.
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 include/linux/mtd/nand.h |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
Jan Luebbe - March 4, 2013, 4:50 p.m.
On Mon, 2013-03-04 at 18:42 +0200, Artem Bityutskiy wrote:
>  /**
>   * struct nand_flash_dev - NAND Flash Device ID Structure
>   * @name:	Identify the device type
> - * @dev_id:	device ID code
> + * @mfr_id:     manufecturer ID part of the full chip ID array (byte 0)

This should be 'manufacturer'.

> + * @dev_id:     device ID part of the full chip ID array (byte 1)
Brian Norris - March 4, 2013, 7:45 p.m.
On Mon, Mar 4, 2013 at 8:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  include/linux/mtd/nand.h |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 7af6600..951ea0d 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -559,7 +559,9 @@ struct nand_chip {
>  /**
>   * struct nand_flash_dev - NAND Flash Device ID Structure
>   * @name:      Identify the device type
> - * @dev_id:    device ID code
> + * @mfr_id:     manufecturer ID part of the full chip ID array (byte 0)
> + * @dev_id:     device ID part of the full chip ID array (byte 1)
> + * @id:         full device ID array
>   * @pagesize:  Pagesize in bytes. Either 256 or 512 or 0
>   *             If the pagesize is 0, then the real pagesize
>   *             and the eraseize are determined from the
> @@ -570,7 +572,13 @@ struct nand_chip {
>   */
>  struct nand_flash_dev {
>         char *name;
> -       int dev_id;
> +       union {
> +               struct {
> +                       uint8_t mfr_id;
> +                       uint8_t dev_id;
> +               };
> +               uint8_t id[8];
> +       };

I'm sorry that I was unable to fully review Huang's code while it was
being discussed earlier. But from what I read, it seemed there was
some disagreement on some of the format of the tables (e.g., Artem
wondered why the extra zeroes). Now, it seems like instead, we're
including duplicate fields. The "mfr_id" and "dev_id" are always the
first two bytes of the "id[8]" field. Does it make sense to repeat
them? And I actually don't ever see where "mfr_id" would be used.

There's also the (yet unanswered?) question of whether we reasonably
would handle in this table both those chips which have their full
information listed in conjunction with their 8-byte ID string and
those chips with minimal information in the table, and the rest must
be decoded from extended ID.

Perhaps the answer to this last question was already assumed to be
"yes." But how would we differentiate the two types of table entries?
Simply by seeing whether they filled out "dev_id" vs. "id[8]"? This
patch doesn't clearly show how the new fields could be used.

Hopefully my comments make some sense. If they don't, then that
probably means we need some more discussion before including this last
patch. If they do make sense, then that probably still means this last
patch needs to be rewritten and/or included only along with a series
that can reasonably make use of these fields (like Huang's series).

>         unsigned long pagesize;
>         unsigned long chipsize;
>         unsigned long erasesize;

Thanks,
Brian
Huang Shijie - March 5, 2013, 6:08 a.m.
On Tue, Mar 5, 2013 at 12:42 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  include/linux/mtd/nand.h |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 7af6600..951ea0d 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -559,7 +559,9 @@ struct nand_chip {
>  /**
>   * struct nand_flash_dev - NAND Flash Device ID Structure
>   * @name:      Identify the device type
> - * @dev_id:    device ID code
> + * @mfr_id:     manufecturer ID part of the full chip ID array (byte 0)
> + * @dev_id:     device ID part of the full chip ID array (byte 1)
> + * @id:         full device ID array
>   * @pagesize:  Pagesize in bytes. Either 256 or 512 or 0
>   *             If the pagesize is 0, then the real pagesize
>   *             and the eraseize are determined from the
> @@ -570,7 +572,13 @@ struct nand_chip {
>   */
>  struct nand_flash_dev {
>         char *name;
> -       int dev_id;
> +       union {
> +               struct {
> +                       uint8_t mfr_id;
> +                       uint8_t dev_id;
> +               };
> +               uint8_t id[8];
> +       };
>
We can add a new helper like compare_nand_with_devid() to remove the
mfr_id and dev_id, only left the id[8].

The compare_nand_with_devid() can be used in places where your patch
11 does. For example:
   ..............................................................
        if (id == nand_flash_ids[i].dev_id) {

   changed to:

        if (compare_nand_with_devid(id)){

  ...............................................................

By this way, we can remove the mfr_id and dev_id.


thanks
Huang Shijie
Artem Bityutskiy - March 5, 2013, 10:37 a.m.
On Mon, 2013-03-04 at 11:45 -0800, Brian Norris wrote:
> >  struct nand_flash_dev {
> >         char *name;
> > -       int dev_id;
> > +       union {
> > +               struct {
> > +                       uint8_t mfr_id;
> > +                       uint8_t dev_id;
> > +               };
> > +               uint8_t id[8];
> > +       };
> 
> I'm sorry that I was unable to fully review Huang's code while it was
> being discussed earlier. But from what I read, it seemed there was
> some disagreement on some of the format of the tables (e.g., Artem
> wondered why the extra zeroes). Now, it seems like instead, we're
> including duplicate fields. The "mfr_id" and "dev_id" are always the
> first two bytes of the "id[8]" field. Does it make sense to repeat
> them? And I actually don't ever see where "mfr_id" would be used.

This is not repeating. Because of the union {}, mfr_id's memory address
is identical to id[0]'s address, and dev_id's memory address is
identical to id[1]'s address. You can consider 'mfr_id' and 'dev_id' to
be alias names for id[0] and id[1]. If one needs to have an alias name
for other ID bytes, they can be added.

My idea was that type->mfr_id and type->dev_id is more readable than
type->id[0] and type->id[1].

> There's also the (yet unanswered?) question of whether we reasonably
> would handle in this table both those chips which have their full
> information listed in conjunction with their 8-byte ID string and
> those chips with minimal information in the table, and the rest must
> be decoded from extended ID.

I was thinking that we have a single table which contains both, and we
make sure that all full ID records go first, and device ID-only records
go last. This would mean that we first match by full ID, and then by
device IDs.

> Perhaps the answer to this last question was already assumed to be
> "yes." But how would we differentiate the two types of table entries?
> Simply by seeing whether they filled out "dev_id" vs. "id[8]"? This
> patch doesn't clearly show how the new fields could be used.

I was thinking about

if (type->mfr_id == 0)
	/* this is device-only ID */
else
	/* this is full ID */

or

static inline bool is_full_nand_id(type) {
	return type->mfr_id;
}

> Hopefully my comments make some sense. If they don't, then that
> probably means we need some more discussion before including this last
> patch. If they do make sense, then that probably still means this last
> patch needs to be rewritten and/or included only along with a series
> that can reasonably make use of these fields (like Huang's series).

Sure, this is why you was in "To:" - you are the main NAND janitor
nowadays, because others mostly care about their little drivers, while
you demonstrated skills and will to make the general layer sane.
Artem Bityutskiy - March 5, 2013, 10:42 a.m.
On Tue, 2013-03-05 at 14:08 +0800, Huang Shijie wrote:
> >  struct nand_flash_dev {
> >         char *name;
> > -       int dev_id;
> > +       union {
> > +               struct {
> > +                       uint8_t mfr_id;
> > +                       uint8_t dev_id;
> > +               };
> > +               uint8_t id[8];
> > +       };
> >
> We can add a new helper like compare_nand_with_devid() to remove the
> mfr_id and dev_id, only left the id[8].

Huang, did you miss the "union { ... }" part or you think using 'id[N]'
directly is readable enough?
Huang Shijie - March 5, 2013, 2:36 p.m.
On Tue, Mar 5, 2013 at 6:42 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2013-03-05 at 14:08 +0800, Huang Shijie wrote:

>> >  struct nand_flash_dev {

>> >         char *name;

>> > -       int dev_id;

>> > +       union {

>> > +               struct {

>> > +                       uint8_t mfr_id;

>> > +                       uint8_t dev_id;

>> > +               };

>> > +               uint8_t id[8];

>> > +       };

>> >

>> We can add a new helper like compare_nand_with_devid() to remove the

>> mfr_id and dev_id, only left the id[8].

>

> Huang, did you miss the "union { ... }" part or you think using 'id[N]'

> directly is readable enough?

i think using the id[] is enough.
the union is redundant.

thanks
Huang Shijie
Artem Bityutskiy - March 5, 2013, 2:55 p.m.
On Tue, 2013-03-05 at 22:36 +0800, Huang Shijie wrote:
> >> We can add a new helper like compare_nand_with_devid() to remove the
> >> mfr_id and dev_id, only left the id[8].
> >
> > Huang, did you miss the "union { ... }" part or you think using 'id[N]'
> > directly is readable enough?
> i think using the id[] is enough.
> the union is redundant.

I just find 'type->dev_id' more readable than 'type->id[1]'...
Huang Shijie - March 6, 2013, 2:17 a.m.
于 2013年03月05日 22:55, Artem Bityutskiy 写道:
> On Tue, 2013-03-05 at 22:36 +0800, Huang Shijie wrote:
>>>> We can add a new helper like compare_nand_with_devid() to remove the
>>>> mfr_id and dev_id, only left the id[8].
>>> Huang, did you miss the "union { ... }" part or you think using 'id[N]'
>>> directly is readable enough?
>> i think using the id[] is enough.
>> the union is redundant.
> I just find 'type->dev_id' more readable than 'type->id[1]'...
Just a suggestion, add more comments to the code will also make it readable.

But it's ok to me if you keep the union.

thanks
Huang Shijie
Huang Shijie - March 6, 2013, 5:32 a.m.
于 2013年03月05日 18:37, Artem Bityutskiy 写道:
> I was thinking about
>
> if (type->mfr_id == 0)
> 	/* this is device-only ID */
> else
> 	/* this is full ID */
>
> or
>
> static inline bool is_full_nand_id(type) {
> 	return type->mfr_id;
> }
okay. If you think we should not add another table for the full-id nand.
I will follow your advice.

thanks
Huang Shijie

Patch

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7af6600..951ea0d 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -559,7 +559,9 @@  struct nand_chip {
 /**
  * struct nand_flash_dev - NAND Flash Device ID Structure
  * @name:	Identify the device type
- * @dev_id:	device ID code
+ * @mfr_id:     manufecturer ID part of the full chip ID array (byte 0)
+ * @dev_id:     device ID part of the full chip ID array (byte 1)
+ * @id:         full device ID array
  * @pagesize:	Pagesize in bytes. Either 256 or 512 or 0
  *		If the pagesize is 0, then the real pagesize
  *		and the eraseize are determined from the
@@ -570,7 +572,13 @@  struct nand_chip {
  */
 struct nand_flash_dev {
 	char *name;
-	int dev_id;
+	union {
+		struct {
+			uint8_t mfr_id;
+			uint8_t dev_id;
+		};
+		uint8_t id[8];
+	};
 	unsigned long pagesize;
 	unsigned long chipsize;
 	unsigned long erasesize;