[PATCH] mtd: spi-nor: read 6 bytes for the ID

[PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

Currently, we read 5 bytes for ID, but s25fl128s has the same ext_id(0x4d01)
with s25fl129p1. The s25fl128s can support the DDR Quad read, while s25fl129p1
does not. So we have to distinguish the two NOR flashs.

This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search the
right flash_info.

The detail of the patch is:
  [1] change the "ext_id" from u16 to u32.
      We can store two bytes or three bytes with the @ext_id now.

  [2] search the right flash_info with the 6byte ID and the new @ext_id.
      We use "matched" variable to track the legacy two bytes @ext_id.
      If the flash_info's @ext_id is three bytes, we will use the
      sixth byte of the ID to check it.

  [3] add the new item to spi_nor_ids for s25fl128s.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
based on Brian's patch set.
---
 drivers/mtd/spi-nor/spi-nor.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d6f44d5..cd4b277 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -383,7 +383,7 @@ struct flash_info {
 	 * then a two byte device id.
 	 */
 	u32		jedec_id;
-	u16             ext_id;
+	u32		ext_id;
 
 	/* The size listed here is what works with SPINOR_OP_SE, which isn't
 	 * necessarily called a "sector" by the vendor.
@@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
+	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
 	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
 	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
 	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
@@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
 static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
 {
 	int			tmp;
-	u8			id[5];
+	u8			id[6];
 	u32			jedec;
-	u16                     ext_jedec;
+	u32                     ext_jedec;
 	struct flash_info	*info;
+	int			matched = -1;
 
-	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
+	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
 	if (tmp < 0) {
 		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
 		return ERR_PTR(tmp);
@@ -614,8 +616,23 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
 	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
 		info = (void *)spi_nor_ids[tmp].driver_data;
 		if (info->jedec_id == jedec) {
-			if (info->ext_id == 0 || info->ext_id == ext_jedec)
+			if (info->ext_id == 0)
 				return &spi_nor_ids[tmp];
+
+			/* the legacy two bytes ext_id */
+			if ((info->ext_id >> 16) == 0) {
+				if (info->ext_id == ext_jedec)
+					matched = tmp;
+			} else {
+				/* check the sixth byte now */
+				ext_jedec = ext_jedec << 8 | id[5];
+				if (info->ext_id == ext_jedec)
+					return &spi_nor_ids[tmp];
+			}
+		} else {
+			/* shortcut */
+			if (matched != -1)
+				return &spi_nor_ids[matched];
 		}
 	}
 	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);
-- 
1.7.2.rc3

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Monday, April 14, 2014 at 12:09:34 PM, Huang Shijie wrote:

[...]

> @@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
>  	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>  	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>  	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> +	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256,
> SPI_NOR_QUAD_READ) }, { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 
> 64, 0) },

Support for new hardware should go in separatelly.

>  	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>  	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
> @@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
>  static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>  {
>  	int			tmp;
> -	u8			id[5];
> +	u8			id[6];
>  	u32			jedec;
> -	u16                     ext_jedec;
> +	u32                     ext_jedec;
>  	struct flash_info	*info;
> +	int			matched = -1;
> 
> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
> +	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
>  	if (tmp < 0) {
>  		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
>  		return ERR_PTR(tmp);
> @@ -614,8 +616,23 @@ static const struct spi_device_id
> *spi_nor_read_id(struct spi_nor *nor) for (tmp = 0; tmp <
> ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>  		info = (void *)spi_nor_ids[tmp].driver_data;
>  		if (info->jedec_id == jedec) {
> -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> +			if (info->ext_id == 0)
>  				return &spi_nor_ids[tmp];
> +
> +			/* the legacy two bytes ext_id */
> +			if ((info->ext_id >> 16) == 0) {
> +				if (info->ext_id == ext_jedec)
> +					matched = tmp;
> +			} else {
> +				/* check the sixth byte now */
> +				ext_jedec = ext_jedec << 8 | id[5];
> +				if (info->ext_id == ext_jedec)
> +					return &spi_nor_ids[tmp];
> +			}
> +		} else {
> +			/* shortcut */
> +			if (matched != -1)
> +				return &spi_nor_ids[matched];

I wonder if the ID-bytes wraparound cannot cause us trouble here. For example if 
we try to detect a SPI NOR which has 5-byte ID code, but in the table, we'd also 
have a SPI NOR with has a 6-byte code where the last byte of ext-jedec matches 
the first byte of JEDEC ID , this would actually match on the later.

Shall we not add an ID code length field into the table instead ?

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Mon, Apr 14, 2014 at 01:53:07PM +0200, Marek Vasut wrote:
> > @@ -614,8 +616,23 @@ static const struct spi_device_id
> > *spi_nor_read_id(struct spi_nor *nor) for (tmp = 0; tmp <
> > ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
> >  		info = (void *)spi_nor_ids[tmp].driver_data;
> >  		if (info->jedec_id == jedec) {
> > -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> > +			if (info->ext_id == 0)
> >  				return &spi_nor_ids[tmp];
> > +
> > +			/* the legacy two bytes ext_id */
> > +			if ((info->ext_id >> 16) == 0) {
> > +				if (info->ext_id == ext_jedec)
> > +					matched = tmp;
> > +			} else {
> > +				/* check the sixth byte now */
> > +				ext_jedec = ext_jedec << 8 | id[5];
> > +				if (info->ext_id == ext_jedec)
> > +					return &spi_nor_ids[tmp];
> > +			}
> > +		} else {
> > +			/* shortcut */
> > +			if (matched != -1)
> > +				return &spi_nor_ids[matched];
> 
> I wonder if the ID-bytes wraparound cannot cause us trouble here. For example if 
> we try to detect a SPI NOR which has 5-byte ID code, but in the table, we'd also 
> have a SPI NOR with has a 6-byte code where the last byte of ext-jedec matches 
> the first byte of JEDEC ID , this would actually match on the later.
could you give me detail example?

I feel sorry that i do not quit understand your meaning.

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Monday, April 14, 2014 at 04:44:01 PM, Huang Shijie wrote:
> On Mon, Apr 14, 2014 at 01:53:07PM +0200, Marek Vasut wrote:
> > > @@ -614,8 +616,23 @@ static const struct spi_device_id
> > > *spi_nor_read_id(struct spi_nor *nor) for (tmp = 0; tmp <
> > > ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
> > > 
> > >  		info = (void *)spi_nor_ids[tmp].driver_data;
> > >  		if (info->jedec_id == jedec) {
> > > 
> > > -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> > > +			if (info->ext_id == 0)
> > > 
> > >  				return &spi_nor_ids[tmp];
> > > 
> > > +
> > > +			/* the legacy two bytes ext_id */
> > > +			if ((info->ext_id >> 16) == 0) {
> > > +				if (info->ext_id == ext_jedec)
> > > +					matched = tmp;
> > > +			} else {
> > > +				/* check the sixth byte now */
> > > +				ext_jedec = ext_jedec << 8 | id[5];
> > > +				if (info->ext_id == ext_jedec)
> > > +					return &spi_nor_ids[tmp];
> > > +			}
> > > +		} else {
> > > +			/* shortcut */
> > > +			if (matched != -1)
> > > +				return &spi_nor_ids[matched];
> > 
> > I wonder if the ID-bytes wraparound cannot cause us trouble here. For
> > example if we try to detect a SPI NOR which has 5-byte ID code, but in
> > the table, we'd also have a SPI NOR with has a 6-byte code where the
> > last byte of ext-jedec matches the first byte of JEDEC ID , this would
> > actually match on the later.
> 
> could you give me detail example?
> 
> I feel sorry that i do not quit understand your meaning.

Imagine two chips with two IDs:
Chip 1 has IDs: 0xf00b42 0x4242f0 and readID[6] returns 0x420bf0f04242
Chip 2 has IDs: 0xf00b42   0x42f0 and readID[6] returns 0x420bf0f04242
This is because in the second chips' case the ID wraps around at 5 bytes. But 
chip #1 matches the ID, so if chip #1 is earlier in the list of SPI NOR flashes, 
we will get an incorrect detection of that chip.

Does it make sense now please ?

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Mon, Apr 14, 2014 at 08:23:47PM +0200, Marek Vasut wrote:
> On Monday, April 14, 2014 at 04:44:01 PM, Huang Shijie wrote:
> > On Mon, Apr 14, 2014 at 01:53:07PM +0200, Marek Vasut wrote:
> > > > @@ -614,8 +616,23 @@ static const struct spi_device_id
> > > > *spi_nor_read_id(struct spi_nor *nor) for (tmp = 0; tmp <
> > > > ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
> > > > 
> > > >  		info = (void *)spi_nor_ids[tmp].driver_data;
> > > >  		if (info->jedec_id == jedec) {
> > > > 
> > > > -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> > > > +			if (info->ext_id == 0)
> > > > 
> > > >  				return &spi_nor_ids[tmp];
> > > > 
> > > > +
> > > > +			/* the legacy two bytes ext_id */
> > > > +			if ((info->ext_id >> 16) == 0) {
> > > > +				if (info->ext_id == ext_jedec)
> > > > +					matched = tmp;
> > > > +			} else {
> > > > +				/* check the sixth byte now */
> > > > +				ext_jedec = ext_jedec << 8 | id[5];
> > > > +				if (info->ext_id == ext_jedec)
> > > > +					return &spi_nor_ids[tmp];
> > > > +			}
> > > > +		} else {
> > > > +			/* shortcut */
> > > > +			if (matched != -1)
> > > > +				return &spi_nor_ids[matched];
> > > 
> > > I wonder if the ID-bytes wraparound cannot cause us trouble here. For
> > > example if we try to detect a SPI NOR which has 5-byte ID code, but in
> > > the table, we'd also have a SPI NOR with has a 6-byte code where the
> > > last byte of ext-jedec matches the first byte of JEDEC ID , this would
> > > actually match on the later.
> > 
> > could you give me detail example?
> > 
> > I feel sorry that i do not quit understand your meaning.
> 
> Imagine two chips with two IDs:
> Chip 1 has IDs: 0xf00b42 0x4242f0 and readID[6] returns 0x420bf0f04242
It will not return 0x420bf0f04242.

The readID[6] should be: f0, 0b, 42, 42, 42, f0.


> Chip 2 has IDs: 0xf00b42   0x42f0 and readID[6] returns 0x420bf0f04242
the readID[6] should be: f0, 0b, 42, 42, f0, XX.

 "XX" stands for the sixth byte.

The current patch can distinguish these two chips.

> This is because in the second chips' case the ID wraps around at 5 bytes. But 
> chip #1 matches the ID, so if chip #1 is earlier in the list of SPI NOR flashes, 
> we will get an incorrect detection of that chip.
> 
I guess your meaning is that the chip 2 has IDs: 0xf00b42   0x4242
and the sixth byte is 0xf0 which wraps the first byte.

Currently, only the Spansion uses the ext_id, please see the items:
 ---------------------------------------------------------------------
                 .................
	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
                 .................
 ---------------------------------------------------------------------
The s25fl128s is the first NOR chip which shares the same 5 bytes:
     the jedec_id and the ext_id.

What are you worry about is that the s25fl129p1 has the same 6th byte as
the s25fl128s. The sixth byte is the "Family ID". For s25fl128s, the sixth byte
of the ID is 0x80.  I do not have s25fl129p1, I only have its datasheet. 

Can someone have the s25fl129p1? if you have this chip, could you please
read out the sixth byte of its ID?

I think the s25fl129p1 will not have the same "Family ID" as the s25fl128s.

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Tuesday, April 15, 2014 at 07:22:39 AM, Huang Shijie wrote:
> On Mon, Apr 14, 2014 at 08:23:47PM +0200, Marek Vasut wrote:

[...]

> > > > I wonder if the ID-bytes wraparound cannot cause us trouble here. For
> > > > example if we try to detect a SPI NOR which has 5-byte ID code, but
> > > > in the table, we'd also have a SPI NOR with has a 6-byte code where
> > > > the last byte of ext-jedec matches the first byte of JEDEC ID , this
> > > > would actually match on the later.
> > > 
> > > could you give me detail example?
> > > 
> > > I feel sorry that i do not quit understand your meaning.
> > 
> > Imagine two chips with two IDs:
> > Chip 1 has IDs: 0xf00b42 0x4242f0 and readID[6] returns 0x420bf0f04242
> 
> It will not return 0x420bf0f04242.
> 
> The readID[6] should be: f0, 0b, 42, 42, 42, f0.
> 
> > Chip 2 has IDs: 0xf00b42   0x42f0 and readID[6] returns 0x420bf0f04242
> 
> the readID[6] should be: f0, 0b, 42, 42, f0, XX.
> 
>  "XX" stands for the sixth byte.
> 
> The current patch can distinguish these two chips.
> 
> > This is because in the second chips' case the ID wraps around at 5 bytes.
> > But chip #1 matches the ID, so if chip #1 is earlier in the list of SPI
> > NOR flashes, we will get an incorrect detection of that chip.
> 
> I guess your meaning is that the chip 2 has IDs: 0xf00b42   0x4242
> and the sixth byte is 0xf0 which wraps the first byte.

Huang, what I meant is that if you read 6 bytes of ID from a chip which wraps 
the READID command output on 5 bytes AND the first and last byte match in the 
table for some 6-byte chip, then this 6-byte chip will be used as a 
configuration for the different 5-byte chip.

This code should be future-proof, but if we keep adding such special cases, we 
will end up with false matches sooner or later anyway I'm afraid.

What do you say we add the READID length field into the table ?

[...]

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Tue, Apr 15, 2014 at 03:35:05PM +0200, Marek Vasut wrote:
> On Tuesday, April 15, 2014 at 07:22:39 AM, Huang Shijie wrote:
> > On Mon, Apr 14, 2014 at 08:23:47PM +0200, Marek Vasut wrote:
> 
> [...]
> 
> > > > > I wonder if the ID-bytes wraparound cannot cause us trouble here. For
> > > > > example if we try to detect a SPI NOR which has 5-byte ID code, but
> > > > > in the table, we'd also have a SPI NOR with has a 6-byte code where
> > > > > the last byte of ext-jedec matches the first byte of JEDEC ID , this
> > > > > would actually match on the later.
> > > > 
> > > > could you give me detail example?
> > > > 
> > > > I feel sorry that i do not quit understand your meaning.
> > > 
> > > Imagine two chips with two IDs:
> > > Chip 1 has IDs: 0xf00b42 0x4242f0 and readID[6] returns 0x420bf0f04242
> > 
> > It will not return 0x420bf0f04242.
> > 
> > The readID[6] should be: f0, 0b, 42, 42, 42, f0.
> > 
> > > Chip 2 has IDs: 0xf00b42   0x42f0 and readID[6] returns 0x420bf0f04242
> > 
> > the readID[6] should be: f0, 0b, 42, 42, f0, XX.
> > 
> >  "XX" stands for the sixth byte.
> > 
> > The current patch can distinguish these two chips.
> > 
> > > This is because in the second chips' case the ID wraps around at 5 bytes.
> > > But chip #1 matches the ID, so if chip #1 is earlier in the list of SPI
> > > NOR flashes, we will get an incorrect detection of that chip.
> > 
> > I guess your meaning is that the chip 2 has IDs: 0xf00b42   0x4242
> > and the sixth byte is 0xf0 which wraps the first byte.
> 
> Huang, what I meant is that if you read 6 bytes of ID from a chip which wraps 
> the READID command output on 5 bytes AND the first and last byte match in the 
> table for some 6-byte chip, then this 6-byte chip will be used as a 
> configuration for the different 5-byte chip.

Does the chip vendor so silly to produce such chips? :)

> 
> This code should be future-proof, but if we keep adding such special cases, we 
> will end up with false matches sooner or later anyway I'm afraid.
> 
> What do you say we add the READID length field into the table ?
If we add the length field into the table, we have to sort the table by
some kind of order. Btw: I do not object to add the length field. 

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Tuesday, April 15, 2014 at 06:04:05 PM, Huang Shijie wrote:
> On Tue, Apr 15, 2014 at 03:35:05PM +0200, Marek Vasut wrote:
> > On Tuesday, April 15, 2014 at 07:22:39 AM, Huang Shijie wrote:
> > > On Mon, Apr 14, 2014 at 08:23:47PM +0200, Marek Vasut wrote:
> > [...]
> > 
> > > > > > I wonder if the ID-bytes wraparound cannot cause us trouble here.
> > > > > > For example if we try to detect a SPI NOR which has 5-byte ID
> > > > > > code, but in the table, we'd also have a SPI NOR with has a
> > > > > > 6-byte code where the last byte of ext-jedec matches the first
> > > > > > byte of JEDEC ID , this would actually match on the later.
> > > > > 
> > > > > could you give me detail example?
> > > > > 
> > > > > I feel sorry that i do not quit understand your meaning.
> > > > 
> > > > Imagine two chips with two IDs:
> > > > Chip 1 has IDs: 0xf00b42 0x4242f0 and readID[6] returns
> > > > 0x420bf0f04242
> > > 
> > > It will not return 0x420bf0f04242.
> > > 
> > > The readID[6] should be: f0, 0b, 42, 42, 42, f0.
> > > 
> > > > Chip 2 has IDs: 0xf00b42   0x42f0 and readID[6] returns
> > > > 0x420bf0f04242
> > > 
> > > the readID[6] should be: f0, 0b, 42, 42, f0, XX.
> > > 
> > >  "XX" stands for the sixth byte.
> > > 
> > > The current patch can distinguish these two chips.
> > > 
> > > > This is because in the second chips' case the ID wraps around at 5
> > > > bytes. But chip #1 matches the ID, so if chip #1 is earlier in the
> > > > list of SPI NOR flashes, we will get an incorrect detection of that
> > > > chip.
> > > 
> > > I guess your meaning is that the chip 2 has IDs: 0xf00b42   0x4242
> > > and the sixth byte is 0xf0 which wraps the first byte.
> > 
> > Huang, what I meant is that if you read 6 bytes of ID from a chip which
> > wraps the READID command output on 5 bytes AND the first and last byte
> > match in the table for some 6-byte chip, then this 6-byte chip will be
> > used as a configuration for the different 5-byte chip.
> 
> Does the chip vendor so silly to produce such chips? :)

I don't quite understand the meaning of this sentence, but the approach where we 
try heuristics doesn't scale.

> > This code should be future-proof, but if we keep adding such special
> > cases, we will end up with false matches sooner or later anyway I'm
> > afraid.
> > 
> > What do you say we add the READID length field into the table ?
> 
> If we add the length field into the table, we have to sort the table by
> some kind of order.

Why, please elaborate.

> Btw: I do not object to add the length field.

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Tue, Apr 15, 2014 at 08:48:50PM +0200, Marek Vasut wrote:
> On Tuesday, April 15, 2014 at 06:04:05 PM, Huang Shijie wrote:
> > On Tue, Apr 15, 2014 at 03:35:05PM +0200, Marek Vasut wrote:
> > > On Tuesday, April 15, 2014 at 07:22:39 AM, Huang Shijie wrote:
> > > > On Mon, Apr 14, 2014 at 08:23:47PM +0200, Marek Vasut wrote:
> > > [...]
> > > 
> > > > > > > I wonder if the ID-bytes wraparound cannot cause us trouble here.
> > > > > > > For example if we try to detect a SPI NOR which has 5-byte ID
> > > > > > > code, but in the table, we'd also have a SPI NOR with has a
> > > > > > > 6-byte code where the last byte of ext-jedec matches the first
> > > > > > > byte of JEDEC ID , this would actually match on the later.
> > > > > > 
> > > > > > could you give me detail example?
> > > > > > 
> > > > > > I feel sorry that i do not quit understand your meaning.
> > > > > 
> > > > > Imagine two chips with two IDs:
> > > > > Chip 1 has IDs: 0xf00b42 0x4242f0 and readID[6] returns
> > > > > 0x420bf0f04242
> > > > 
> > > > It will not return 0x420bf0f04242.
> > > > 
> > > > The readID[6] should be: f0, 0b, 42, 42, 42, f0.
> > > > 
> > > > > Chip 2 has IDs: 0xf00b42   0x42f0 and readID[6] returns
> > > > > 0x420bf0f04242
> > > > 
> > > > the readID[6] should be: f0, 0b, 42, 42, f0, XX.
> > > > 
> > > >  "XX" stands for the sixth byte.
> > > > 
> > > > The current patch can distinguish these two chips.
> > > > 
> > > > > This is because in the second chips' case the ID wraps around at 5
> > > > > bytes. But chip #1 matches the ID, so if chip #1 is earlier in the
> > > > > list of SPI NOR flashes, we will get an incorrect detection of that
> > > > > chip.
> > > > 
> > > > I guess your meaning is that the chip 2 has IDs: 0xf00b42   0x4242
> > > > and the sixth byte is 0xf0 which wraps the first byte.
> > > 
> > > Huang, what I meant is that if you read 6 bytes of ID from a chip which
> > > wraps the READID command output on 5 bytes AND the first and last byte
> > > match in the table for some 6-byte chip, then this 6-byte chip will be
> > > used as a configuration for the different 5-byte chip.
> > 
> > Does the chip vendor so silly to produce such chips? :)
> 
> I don't quite understand the meaning of this sentence, but the approach where we 
> try heuristics doesn't scale.

If two chips share the same jedec_id, it means the two chips are produced from
the same chip vendor, such as 0x012018 means the chip is from Spansion.

If two chips share the same 5 bytes ID, the two chips definitely are produced
from the same vendor.

So my meaning is the case what are you mentioned will _not_ exit in the real world.
Spansion will not so silly.

> 
> > > This code should be future-proof, but if we keep adding such special
> > > cases, we will end up with false matches sooner or later anyway I'm

> > > afraid.
> > > 
> > > What do you say we add the READID length field into the table ?
> > 
> > If we add the length field into the table, we have to sort the table by
> > some kind of order.
> 
> Why, please elaborate.
pleas see:
http://lists.infradead.org/pipermail/linux-mtd/2013-December/050670.html

If you are interested in this issue, please give us a patch.
What I want is making the kernel can support the s25fl128s as soon as possible.

thanks
Huang Shijie
   

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Wednesday, April 16, 2014 at 03:52:03 AM, Huang Shijie wrote:

[...]

> > > Does the chip vendor so silly to produce such chips? :)
> > 
> > I don't quite understand the meaning of this sentence, but the approach
> > where we try heuristics doesn't scale.
> 
> If two chips share the same jedec_id, it means the two chips are produced
> from the same chip vendor, such as 0x012018 means the chip is from
> Spansion.
> 
> If two chips share the same 5 bytes ID, the two chips definitely are
> produced from the same vendor.
> 
> So my meaning is the case what are you mentioned will _not_ exit in the
> real world. Spansion will not so silly.

You do have an awful amount of trust for those things. I am better of with 
"better safe than sorry".
 
> > > > This code should be future-proof, but if we keep adding such special
> > > > cases, we will end up with false matches sooner or later anyway I'm
> > > > 
> > > > afraid.
> > > > 
> > > > What do you say we add the READID length field into the table ?
> > > 
> > > If we add the length field into the table, we have to sort the table by
> > > some kind of order.
> > 
> > Why, please elaborate.
> 
> pleas see:
> http://lists.infradead.org/pipermail/linux-mtd/2013-December/050670.html

Sorry, but this really doesn't answer my question. It's only a matter of 
correctly implementing the matching function to find a proper match.

Can we get insight on this from the others please ?

> If you are interested in this issue, please give us a patch.
> What I want is making the kernel can support the s25fl128s as soon as
> possible.

Perfect, I'd prefer to support it as correctly as possible.

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Wed, Apr 16, 2014 at 12:18:28PM +0200, Marek Vasut wrote:
> On Wednesday, April 16, 2014 at 03:52:03 AM, Huang Shijie wrote:
> 
> [...]
> 
> > > > Does the chip vendor so silly to produce such chips? :)
> > > 
> > > I don't quite understand the meaning of this sentence, but the approach
> > > where we try heuristics doesn't scale.
> > 
> > If two chips share the same jedec_id, it means the two chips are produced
> > from the same chip vendor, such as 0x012018 means the chip is from
> > Spansion.
> > 
> > If two chips share the same 5 bytes ID, the two chips definitely are
> > produced from the same vendor.
> > 
> > So my meaning is the case what are you mentioned will _not_ exit in the
> > real world. Spansion will not so silly.
> 
> You do have an awful amount of trust for those things. I am better of with 
> "better safe than sorry".

okay.
>  
> > > > > This code should be future-proof, but if we keep adding such special
> > > > > cases, we will end up with false matches sooner or later anyway I'm
> > > > > 
> > > > > afraid.
> > > > > 
> > > > > What do you say we add the READID length field into the table ?
> > > > 
> > > > If we add the length field into the table, we have to sort the table by
> > > > some kind of order.
> > > 
> > > Why, please elaborate.
> > 
> > pleas see:
> > http://lists.infradead.org/pipermail/linux-mtd/2013-December/050670.html
> 
> Sorry, but this really doesn't answer my question. It's only a matter of 
> correctly implementing the matching function to find a proper match.
> 
> Can we get insight on this from the others please ?
> 
> > If you are interested in this issue, please give us a patch.
> > What I want is making the kernel can support the s25fl128s as soon as
> > possible.
> 
> Perfect, I'd prefer to support it as correctly as possible.
wait for your patch.

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Wednesday, April 16, 2014 at 03:56:43 PM, Huang Shijie wrote:
> On Wed, Apr 16, 2014 at 12:18:28PM +0200, Marek Vasut wrote:
> > On Wednesday, April 16, 2014 at 03:52:03 AM, Huang Shijie wrote:
> > 
> > [...]
> > 
> > > > > Does the chip vendor so silly to produce such chips? :)
> > > > 
> > > > I don't quite understand the meaning of this sentence, but the
> > > > approach where we try heuristics doesn't scale.
> > > 
> > > If two chips share the same jedec_id, it means the two chips are
> > > produced from the same chip vendor, such as 0x012018 means the chip is
> > > from Spansion.
> > > 
> > > If two chips share the same 5 bytes ID, the two chips definitely are
> > > produced from the same vendor.
> > > 
> > > So my meaning is the case what are you mentioned will _not_ exit in the
> > > real world. Spansion will not so silly.
> > 
> > You do have an awful amount of trust for those things. I am better of
> > with "better safe than sorry".
> 
> okay.
> 
> > > > > > This code should be future-proof, but if we keep adding such
> > > > > > special cases, we will end up with false matches sooner or later
> > > > > > anyway I'm
> > > > > > 
> > > > > > afraid.
> > > > > > 
> > > > > > What do you say we add the READID length field into the table ?
> > > > > 
> > > > > If we add the length field into the table, we have to sort the
> > > > > table by some kind of order.
> > > > 
> > > > Why, please elaborate.
> > > 
> > > pleas see:
> > > http://lists.infradead.org/pipermail/linux-mtd/2013-December/050670.htm
> > > l
> > 
> > Sorry, but this really doesn't answer my question. It's only a matter of
> > correctly implementing the matching function to find a proper match.
> > 
> > Can we get insight on this from the others please ?
> > 
> > > If you are interested in this issue, please give us a patch.
> > > What I want is making the kernel can support the s25fl128s as soon as
> > > possible.
> > 
> > Perfect, I'd prefer to support it as correctly as possible.
> 
> wait for your patch.

Please keep the discussion professional and avoid threats.

Thank you

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Thu, Apr 17, 2014 at 01:23:31AM +0200, Marek Vasut wrote:
> On Wednesday, April 16, 2014 at 03:56:43 PM, Huang Shijie wrote:
> > On Wed, Apr 16, 2014 at 12:18:28PM +0200, Marek Vasut wrote:
> > > On Wednesday, April 16, 2014 at 03:52:03 AM, Huang Shijie wrote:
> > > 
> > > [...]
> > > 
> > > > > > Does the chip vendor so silly to produce such chips? :)
> > > > > 
> > > > > I don't quite understand the meaning of this sentence, but the
> > > > > approach where we try heuristics doesn't scale.
> > > > 
> > > > If two chips share the same jedec_id, it means the two chips are
> > > > produced from the same chip vendor, such as 0x012018 means the chip is
> > > > from Spansion.
> > > > 
> > > > If two chips share the same 5 bytes ID, the two chips definitely are
> > > > produced from the same vendor.
> > > > 
> > > > So my meaning is the case what are you mentioned will _not_ exit in the
> > > > real world. Spansion will not so silly.
> > > 
> > > You do have an awful amount of trust for those things. I am better of
> > > with "better safe than sorry".
> > 
> > okay.
> > 
> > > > > > > This code should be future-proof, but if we keep adding such
> > > > > > > special cases, we will end up with false matches sooner or later
> > > > > > > anyway I'm
> > > > > > > 
> > > > > > > afraid.
> > > > > > > 
> > > > > > > What do you say we add the READID length field into the table ?
> > > > > > 
> > > > > > If we add the length field into the table, we have to sort the
> > > > > > table by some kind of order.
> > > > > 
> > > > > Why, please elaborate.
> > > > 
> > > > pleas see:
> > > > http://lists.infradead.org/pipermail/linux-mtd/2013-December/050670.htm
> > > > l
> > > 
> > > Sorry, but this really doesn't answer my question. It's only a matter of
> > > correctly implementing the matching function to find a proper match.
> > > 
> > > Can we get insight on this from the others please ?
> > > 
> > > > If you are interested in this issue, please give us a patch.
> > > > What I want is making the kernel can support the s25fl128s as soon as
> > > > possible.
> > > 
> > > Perfect, I'd prefer to support it as correctly as possible.
> > 
> > wait for your patch.
> 
> Please keep the discussion professional and avoid threats.
> 
Please do not make mistake about me. :) it is not a threat.

If add the readid length field to the table, i will implement nearly the
same code as Clark did. I thought You have a better idea maybe.



thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Tue, Apr 15, 2014 at 03:35:05PM +0200, Marek Vasut wrote:
> On Tuesday, April 15, 2014 at 07:22:39 AM, Huang Shijie wrote:
> > On Mon, Apr 14, 2014 at 08:23:47PM +0200, Marek Vasut wrote:
> 
> [...]
> 
> > > > > I wonder if the ID-bytes wraparound cannot cause us trouble here. For
> > > > > example if we try to detect a SPI NOR which has 5-byte ID code, but
> > > > > in the table, we'd also have a SPI NOR with has a 6-byte code where
> > > > > the last byte of ext-jedec matches the first byte of JEDEC ID , this
> > > > > would actually match on the later.
> > > > 
> > > > could you give me detail example?
> > > > 
> > > > I feel sorry that i do not quit understand your meaning.
> > > 
> > > Imagine two chips with two IDs:
> > > Chip 1 has IDs: 0xf00b42 0x4242f0 and readID[6] returns 0x420bf0f04242
> > 
> > It will not return 0x420bf0f04242.
> > 
> > The readID[6] should be: f0, 0b, 42, 42, 42, f0.
> > 
> > > Chip 2 has IDs: 0xf00b42   0x42f0 and readID[6] returns 0x420bf0f04242
> > 
> > the readID[6] should be: f0, 0b, 42, 42, f0, XX.
> > 
> >  "XX" stands for the sixth byte.
> > 
> > The current patch can distinguish these two chips.
> > 
> > > This is because in the second chips' case the ID wraps around at 5 bytes.
> > > But chip #1 matches the ID, so if chip #1 is earlier in the list of SPI
> > > NOR flashes, we will get an incorrect detection of that chip.
> > 
> > I guess your meaning is that the chip 2 has IDs: 0xf00b42   0x4242
> > and the sixth byte is 0xf0 which wraps the first byte.
> 
> Huang, what I meant is that if you read 6 bytes of ID from a chip which wraps 
> the READID command output on 5 bytes AND the first and last byte match in the 
> table for some 6-byte chip, then this 6-byte chip will be used as a 
> configuration for the different 5-byte chip.
what are you saying is that we meet the same 6 bytes for two different NOR flashes.


> 
> This code should be future-proof, but if we keep adding such special cases, we 
> will end up with false matches sooner or later anyway I'm afraid.
> 
> What do you say we add the READID length field into the table ?

could you tell us how do you implement the "add the READID length field .."?

IMHO, even we add the readid length field, we also can not avoid the case you
mentioned.

hi Brian & Clark, what's your opinion about this?

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Mon, Apr 14, 2014 at 06:09:34PM +0800, Huang Shijie wrote:
> Currently, we read 5 bytes for ID, but s25fl128s has the same ext_id(0x4d01)
> with s25fl129p1. The s25fl128s can support the DDR Quad read, while s25fl129p1
> does not. So we have to distinguish the two NOR flashs.
> 
> This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search the
> right flash_info.
> 
> The detail of the patch is:
>   [1] change the "ext_id" from u16 to u32.
>       We can store two bytes or three bytes with the @ext_id now.
> 
>   [2] search the right flash_info with the 6byte ID and the new @ext_id.
>       We use "matched" variable to track the legacy two bytes @ext_id.
>       If the flash_info's @ext_id is three bytes, we will use the
>       sixth byte of the ID to check it.
> 
>   [3] add the new item to spi_nor_ids for s25fl128s.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
> based on Brian's patch set.
> ---
>  drivers/mtd/spi-nor/spi-nor.c |   27 ++++++++++++++++++++++-----
>  1 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d6f44d5..cd4b277 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -383,7 +383,7 @@ struct flash_info {
>  	 * then a two byte device id.
>  	 */
>  	u32		jedec_id;
> -	u16             ext_id;
> +	u32		ext_id;
>  
>  	/* The size listed here is what works with SPINOR_OP_SE, which isn't
>  	 * necessarily called a "sector" by the vendor.
> @@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
>  	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>  	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>  	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> +	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
>  	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
>  	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>  	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
> @@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
>  static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>  {
>  	int			tmp;
> -	u8			id[5];
> +	u8			id[6];
>  	u32			jedec;
> -	u16                     ext_jedec;
> +	u32                     ext_jedec;
>  	struct flash_info	*info;
> +	int			matched = -1;
>  
> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
> +	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
>  	if (tmp < 0) {
>  		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
>  		return ERR_PTR(tmp);
> @@ -614,8 +616,23 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>  	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>  		info = (void *)spi_nor_ids[tmp].driver_data;
>  		if (info->jedec_id == jedec) {
> -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> +			if (info->ext_id == 0)
>  				return &spi_nor_ids[tmp];
> +
> +			/* the legacy two bytes ext_id */
> +			if ((info->ext_id >> 16) == 0) {
> +				if (info->ext_id == ext_jedec)
> +					matched = tmp;
> +			} else {
> +				/* check the sixth byte now */
> +				ext_jedec = ext_jedec << 8 | id[5];
> +				if (info->ext_id == ext_jedec)
> +					return &spi_nor_ids[tmp];
> +			}
> +		} else {
> +			/* shortcut */
> +			if (matched != -1)
> +				return &spi_nor_ids[matched];
>  		}
>  	}
>  	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);
Hi Brian:
	any comment about this patch?

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Christophe KERELLO]

Hello Huang,

I have one remark on this patch.

S25FL128S flash has 2 variants:
  - Uniform 256-kB sectors (ext_id = 0x4D00)
  - 4-kB parameter sectors with uniform 64-kB sectors (ext_id = 0x4D01)

If i would like to distinguish these 2 variants in spi_nor_ids array, i 
replace:
{ "s25fl128s", INFO(0x012018, 0x4d0180, 64 * 1024, 256, 
SPI_NOR_QUAD_READ) },
with
{ "s25fl128s0", INFO(0x012018, 0x4d0080, 256 * 1024, 64, 
SPI_NOR_QUAD_READ) },
{ "s25fl128s1", INFO(0x012018, 0x4d0180, 64 * 1024, 256, 
SPI_NOR_QUAD_READ) },

In this case, i fail to find s25fl128s1 device.

The problem is coming from ext_jedec calculation.
first try:
     - jedec = 0x012018,
     - ext_jedec = 0x4d0180,
     - info->ext_id = 0x4d0080,
     =>s25fl128s0 doesn't match

second try:
     - jedec = 0x012018,
     - ext_jedec = 0x4d018080,
     - info->ext_id = 0x4d0180,
     => s25fl128s1 doesn't match

I think there is similar issue with s25fl129p1 (if i use this patch 
without modifying it).
first try:
     - jedec = 0x012018,
     - ext_jedec = 0x4d01xx, (xx = probably 0x00 or 0xff)
     - info->ext_id = 0x4d0180,
     =>s25fl128s doesn't match

second try:
     - jedec = 0x012018,
     - ext_jedec = 0x4d01xx,
     - info->ext_id = 0x4d01,
     => s25fl129p1 doesn't match

If I reset ext_jedec after each try, it works.

ext_jedec = ext_jedec << 8 | id[5];
if (info->ext_id == ext_jedec)
     return &spi_nor_ids[tmp];
else
     /* reset ext_jdec for next try */
     ext_jedec = ext_jedec >> 8;

Regards,
Christophe Kerello.

On 05/27/2014 03:12 AM, Huang Shijie wrote:
> On Mon, Apr 14, 2014 at 06:09:34PM +0800, Huang Shijie wrote:
>> Currently, we read 5 bytes for ID, but s25fl128s has the same ext_id(0x4d01)
>> with s25fl129p1. The s25fl128s can support the DDR Quad read, while s25fl129p1
>> does not. So we have to distinguish the two NOR flashs.
>>
>> This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search the
>> right flash_info.
>>
>> The detail of the patch is:
>>    [1] change the "ext_id" from u16 to u32.
>>        We can store two bytes or three bytes with the @ext_id now.
>>
>>    [2] search the right flash_info with the 6byte ID and the new @ext_id.
>>        We use "matched" variable to track the legacy two bytes @ext_id.
>>        If the flash_info's @ext_id is three bytes, we will use the
>>        sixth byte of the ID to check it.
>>
>>    [3] add the new item to spi_nor_ids for s25fl128s.
>>
>> Signed-off-by: Huang Shijie <b32955@freescale.com>
>> ---
>> based on Brian's patch set.
>> ---
>>   drivers/mtd/spi-nor/spi-nor.c |   27 ++++++++++++++++++++++-----
>>   1 files changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index d6f44d5..cd4b277 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -383,7 +383,7 @@ struct flash_info {
>>   	 * then a two byte device id.
>>   	 */
>>   	u32		jedec_id;
>> -	u16             ext_id;
>> +	u32		ext_id;
>>   
>>   	/* The size listed here is what works with SPINOR_OP_SE, which isn't
>>   	 * necessarily called a "sector" by the vendor.
>> @@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
>>   	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>>   	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>>   	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>> +	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
>>   	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
>>   	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>>   	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
>> @@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
>>   static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>>   {
>>   	int			tmp;
>> -	u8			id[5];
>> +	u8			id[6];
>>   	u32			jedec;
>> -	u16                     ext_jedec;
>> +	u32                     ext_jedec;
>>   	struct flash_info	*info;
>> +	int			matched = -1;
>>   
>> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
>> +	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
>>   	if (tmp < 0) {
>>   		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
>>   		return ERR_PTR(tmp);
>> @@ -614,8 +616,23 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>>   	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>>   		info = (void *)spi_nor_ids[tmp].driver_data;
>>   		if (info->jedec_id == jedec) {
>> -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
>> +			if (info->ext_id == 0)
>>   				return &spi_nor_ids[tmp];
>> +
>> +			/* the legacy two bytes ext_id */
>> +			if ((info->ext_id >> 16) == 0) {
>> +				if (info->ext_id == ext_jedec)
>> +					matched = tmp;
>> +			} else {
>> +				/* check the sixth byte now */
>> +				ext_jedec = ext_jedec << 8 | id[5];
>> +				if (info->ext_id == ext_jedec)
>> +					return &spi_nor_ids[tmp];
>> +			}
>> +		} else {
>> +			/* shortcut */
>> +			if (matched != -1)
>> +				return &spi_nor_ids[matched];
>>   		}
>>   	}
>>   	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);
> Hi Brian:
> 	any comment about this patch?
>
> thanks
> Huang Shijie
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Tue, May 27, 2014 at 05:57:31PM +0200, Christophe KERELLO wrote:
> Hello Huang,
> 
> I have one remark on this patch.
> 
> S25FL128S flash has 2 variants:
>  - Uniform 256-kB sectors (ext_id = 0x4D00)
>  - 4-kB parameter sectors with uniform 64-kB sectors (ext_id = 0x4D01)
> 
> If i would like to distinguish these 2 variants in spi_nor_ids
> array, i replace:
> { "s25fl128s", INFO(0x012018, 0x4d0180, 64 * 1024, 256,
> SPI_NOR_QUAD_READ) },
> with
> { "s25fl128s0", INFO(0x012018, 0x4d0080, 256 * 1024, 64,
> SPI_NOR_QUAD_READ) },
> { "s25fl128s1", INFO(0x012018, 0x4d0180, 64 * 1024, 256,
> SPI_NOR_QUAD_READ) },
> 
> In this case, i fail to find s25fl128s1 device.
> 
> The problem is coming from ext_jedec calculation.
> first try:
>     - jedec = 0x012018,
>     - ext_jedec = 0x4d0180,
>     - info->ext_id = 0x4d0080,
>     =>s25fl128s0 doesn't match
> 
> second try:
>     - jedec = 0x012018,
>     - ext_jedec = 0x4d018080,
>     - info->ext_id = 0x4d0180,
>     => s25fl128s1 doesn't match
> 
> I think there is similar issue with s25fl129p1 (if i use this patch
> without modifying it).
> first try:
>     - jedec = 0x012018,
>     - ext_jedec = 0x4d01xx, (xx = probably 0x00 or 0xff)
>     - info->ext_id = 0x4d0180,
>     =>s25fl128s doesn't match
> 
> second try:
>     - jedec = 0x012018,
>     - ext_jedec = 0x4d01xx,
>     - info->ext_id = 0x4d01,
>     => s25fl129p1 doesn't match
> 
> If I reset ext_jedec after each try, it works.
> 
> ext_jedec = ext_jedec << 8 | id[5];
> if (info->ext_id == ext_jedec)
>     return &spi_nor_ids[tmp];
> else
>     /* reset ext_jdec for next try */
>     ext_jedec = ext_jedec >> 8;
> 
hi Christophe:
  thanks for pointing this!

  Please try the new version.

thanks
Huang Shijie

---------------------------------------
>From 7b920141899e4e7249bd23792dc8d5f1558cb7ca Mon Sep 17 00:00:00 2001
From: Huang Shijie <b32955@freescale.com>
Date: Mon, 14 Apr 2014 18:09:34 +0800
Subject: [PATCH v2] mtd: spi-nor: read 6 bytes for the ID

Currently, we read 5 bytes for ID, but s25fl128s has the same ext_id(0x4d01)
with s25fl129p1. The s25fl128s can support the DDR Quad read, while s25fl129p1
does not. So we have to distinguish the two NOR flashs.

This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search the
right flash_info.

The detail of the patch is:
  [1] change the "ext_id" from u16 to u32.
      We can store two bytes or three bytes with the @ext_id now.

  [2] search the right flash_info with the 6byte ID and the new @ext_id.
      We use "matched" variable to track the legacy two bytes @ext_id.
      If the flash_info's @ext_id is three bytes, we will use the
      sixth byte of the ID to check it.

  [3] add the new item to spi_nor_ids for s25fl128s.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c713c86..f21d3ef 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -383,7 +383,7 @@ struct flash_info {
 	 * then a two byte device id.
 	 */
 	u32		jedec_id;
-	u16             ext_id;
+	u32		ext_id;
 
 	/* The size listed here is what works with SPINOR_OP_SE, which isn't
 	 * necessarily called a "sector" by the vendor.
@@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
+	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
 	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
 	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
 	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
@@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
 static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
 {
 	int			tmp;
-	u8			id[5];
+	u8			id[6];
 	u32			jedec;
-	u16                     ext_jedec;
+	u32                     ext_jedec;
 	struct flash_info	*info;
+	int			matched = -1;
 
-	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
+	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
 	if (tmp < 0) {
 		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
 		return ERR_PTR(tmp);
@@ -614,8 +616,26 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
 	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
 		info = (void *)spi_nor_ids[tmp].driver_data;
 		if (info->jedec_id == jedec) {
-			if (info->ext_id == 0 || info->ext_id == ext_jedec)
+			if (info->ext_id == 0)
 				return &spi_nor_ids[tmp];
+
+			/* the legacy two bytes ext_id */
+			if ((info->ext_id >> 16) == 0) {
+				if (info->ext_id == ext_jedec)
+					matched = tmp;
+			} else {
+				/* check the sixth byte now */
+				ext_jedec = ext_jedec << 8 | id[5];
+				if (info->ext_id == ext_jedec)
+					return &spi_nor_ids[tmp];
+
+				/* reset back the ext_jedec */
+				ext_jedec >>= 8;
+			}
+		} else {
+			/* shortcut */
+			if (matched != -1)
+				return &spi_nor_ids[matched];
 		}
 	}
 	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);
-- 
1.7.8

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Christophe KERELLO]

On 05/28/2014 07:22 AM, Huang Shijie wrote:
> On Tue, May 27, 2014 at 05:57:31PM +0200, Christophe KERELLO wrote:
>> Hello Huang,
>>
>> I have one remark on this patch.
>>
>> S25FL128S flash has 2 variants:
>>   - Uniform 256-kB sectors (ext_id = 0x4D00)
>>   - 4-kB parameter sectors with uniform 64-kB sectors (ext_id = 0x4D01)
>>
>> If i would like to distinguish these 2 variants in spi_nor_ids
>> array, i replace:
>> { "s25fl128s", INFO(0x012018, 0x4d0180, 64 * 1024, 256,
>> SPI_NOR_QUAD_READ) },
>> with
>> { "s25fl128s0", INFO(0x012018, 0x4d0080, 256 * 1024, 64,
>> SPI_NOR_QUAD_READ) },
>> { "s25fl128s1", INFO(0x012018, 0x4d0180, 64 * 1024, 256,
>> SPI_NOR_QUAD_READ) },
>>
>> In this case, i fail to find s25fl128s1 device.
>>
>> The problem is coming from ext_jedec calculation.
>> first try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d0180,
>>      - info->ext_id = 0x4d0080,
>>      =>s25fl128s0 doesn't match
>>
>> second try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d018080,
>>      - info->ext_id = 0x4d0180,
>>      => s25fl128s1 doesn't match
>>
>> I think there is similar issue with s25fl129p1 (if i use this patch
>> without modifying it).
>> first try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d01xx, (xx = probably 0x00 or 0xff)
>>      - info->ext_id = 0x4d0180,
>>      =>s25fl128s doesn't match
>>
>> second try:
>>      - jedec = 0x012018,
>>      - ext_jedec = 0x4d01xx,
>>      - info->ext_id = 0x4d01,
>>      => s25fl129p1 doesn't match
>>
>> If I reset ext_jedec after each try, it works.
>>
>> ext_jedec = ext_jedec << 8 | id[5];
>> if (info->ext_id == ext_jedec)
>>      return &spi_nor_ids[tmp];
>> else
>>      /* reset ext_jdec for next try */
>>      ext_jedec = ext_jedec >> 8;
>>
> hi Christophe:
>    thanks for pointing this!
>
>    Please try the new version.
>
> thanks
> Huang Shijie
Hello Huang,

Thanks.
Patch v2 works fine.

Regards,
Christophe Kerello.
>
> ---------------------------------------
>  From 7b920141899e4e7249bd23792dc8d5f1558cb7ca Mon Sep 17 00:00:00 2001
> From: Huang Shijie <b32955@freescale.com>
> Date: Mon, 14 Apr 2014 18:09:34 +0800
> Subject: [PATCH v2] mtd: spi-nor: read 6 bytes for the ID
>
> Currently, we read 5 bytes for ID, but s25fl128s has the same ext_id(0x4d01)
> with s25fl129p1. The s25fl128s can support the DDR Quad read, while s25fl129p1
> does not. So we have to distinguish the two NOR flashs.
>
> This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search the
> right flash_info.
>
> The detail of the patch is:
>    [1] change the "ext_id" from u16 to u32.
>        We can store two bytes or three bytes with the @ext_id now.
>
>    [2] search the right flash_info with the 6byte ID and the new @ext_id.
>        We use "matched" variable to track the legacy two bytes @ext_id.
>        If the flash_info's @ext_id is three bytes, we will use the
>        sixth byte of the ID to check it.
>
>    [3] add the new item to spi_nor_ids for s25fl128s.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>   drivers/mtd/spi-nor/spi-nor.c |   30 +++++++++++++++++++++++++-----
>   1 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c713c86..f21d3ef 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -383,7 +383,7 @@ struct flash_info {
>   	 * then a two byte device id.
>   	 */
>   	u32		jedec_id;
> -	u16             ext_id;
> +	u32		ext_id;
>   
>   	/* The size listed here is what works with SPINOR_OP_SE, which isn't
>   	 * necessarily called a "sector" by the vendor.
> @@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
>   	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>   	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>   	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> +	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
>   	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
>   	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>   	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
> @@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
>   static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>   {
>   	int			tmp;
> -	u8			id[5];
> +	u8			id[6];
>   	u32			jedec;
> -	u16                     ext_jedec;
> +	u32                     ext_jedec;
>   	struct flash_info	*info;
> +	int			matched = -1;
>   
> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
> +	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
>   	if (tmp < 0) {
>   		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
>   		return ERR_PTR(tmp);
> @@ -614,8 +616,26 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>   	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>   		info = (void *)spi_nor_ids[tmp].driver_data;
>   		if (info->jedec_id == jedec) {
> -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> +			if (info->ext_id == 0)
>   				return &spi_nor_ids[tmp];
> +
> +			/* the legacy two bytes ext_id */
> +			if ((info->ext_id >> 16) == 0) {
> +				if (info->ext_id == ext_jedec)
> +					matched = tmp;
> +			} else {
> +				/* check the sixth byte now */
> +				ext_jedec = ext_jedec << 8 | id[5];
> +				if (info->ext_id == ext_jedec)
> +					return &spi_nor_ids[tmp];
> +
> +				/* reset back the ext_jedec */
> +				ext_jedec >>= 8;
> +			}
> +		} else {
> +			/* shortcut */
> +			if (matched != -1)
> +				return &spi_nor_ids[matched];
>   		}
>   	}
>   	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Wednesday, May 28, 2014 at 07:22:40 AM, Huang Shijie wrote:
[...]
> From 7b920141899e4e7249bd23792dc8d5f1558cb7ca Mon Sep 17 00:00:00 2001
> From: Huang Shijie <b32955@freescale.com>
> Date: Mon, 14 Apr 2014 18:09:34 +0800
> Subject: [PATCH v2] mtd: spi-nor: read 6 bytes for the ID
> 
> Currently, we read 5 bytes for ID, but s25fl128s has the same
> ext_id(0x4d01) with s25fl129p1. The s25fl128s can support the DDR Quad
> read, while s25fl129p1 does not. So we have to distinguish the two NOR
> flashs.
> 
> This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search
> the right flash_info.
> 
> The detail of the patch is:
>   [1] change the "ext_id" from u16 to u32.
>       We can store two bytes or three bytes with the @ext_id now.
> 
>   [2] search the right flash_info with the 6byte ID and the new @ext_id.
>       We use "matched" variable to track the legacy two bytes @ext_id.
>       If the flash_info's @ext_id is three bytes, we will use the
>       sixth byte of the ID to check it.
> 
>   [3] add the new item to spi_nor_ids for s25fl128s.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c |   30 +++++++++++++++++++++++++-----
>  1 files changed, 25 insertions(+), 5 deletions(-)

What exactly changed here please ?

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Thu, May 29, 2014 at 10:58:52PM +0200, Marek Vasut wrote:
> On Wednesday, May 28, 2014 at 07:22:40 AM, Huang Shijie wrote:
> [...]
> > From 7b920141899e4e7249bd23792dc8d5f1558cb7ca Mon Sep 17 00:00:00 2001
> > From: Huang Shijie <b32955@freescale.com>
> > Date: Mon, 14 Apr 2014 18:09:34 +0800
> > Subject: [PATCH v2] mtd: spi-nor: read 6 bytes for the ID
> > 
> > Currently, we read 5 bytes for ID, but s25fl128s has the same
> > ext_id(0x4d01) with s25fl129p1. The s25fl128s can support the DDR Quad
> > read, while s25fl129p1 does not. So we have to distinguish the two NOR
> > flashs.
> > 
> > This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search
> > the right flash_info.
> > 
> > The detail of the patch is:
> >   [1] change the "ext_id" from u16 to u32.
> >       We can store two bytes or three bytes with the @ext_id now.
> > 
> >   [2] search the right flash_info with the 6byte ID and the new @ext_id.
> >       We use "matched" variable to track the legacy two bytes @ext_id.
> >       If the flash_info's @ext_id is three bytes, we will use the
> >       sixth byte of the ID to check it.
> > 
> >   [3] add the new item to spi_nor_ids for s25fl128s.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c |   30 +++++++++++++++++++++++++-----
> >  1 files changed, 25 insertions(+), 5 deletions(-)
> 
> What exactly changed here please ?

This patch will reset the ext_jedec to the old value if the sixth-byte-match fails.

------------------------------------------------------------
+				/* reset back the ext_jedec */
+				ext_jedec >>= 8;
------------------------------------------------------------

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Friday, May 30, 2014 at 02:49:30 AM, Huang Shijie wrote:
> On Thu, May 29, 2014 at 10:58:52PM +0200, Marek Vasut wrote:
> > On Wednesday, May 28, 2014 at 07:22:40 AM, Huang Shijie wrote:
> > [...]
> > 
> > > From 7b920141899e4e7249bd23792dc8d5f1558cb7ca Mon Sep 17 00:00:00 2001
> > > From: Huang Shijie <b32955@freescale.com>
> > > Date: Mon, 14 Apr 2014 18:09:34 +0800
> > > Subject: [PATCH v2] mtd: spi-nor: read 6 bytes for the ID
> > > 
> > > Currently, we read 5 bytes for ID, but s25fl128s has the same
> > > ext_id(0x4d01) with s25fl129p1. The s25fl128s can support the DDR Quad
> > > read, while s25fl129p1 does not. So we have to distinguish the two NOR
> > > flashs.
> > > 
> > > This patch reads out 6 bytes for the ID, and use the 6 bytes ID to
> > > search the right flash_info.
> > > 
> > > The detail of the patch is:
> > >   [1] change the "ext_id" from u16 to u32.
> > >   
> > >       We can store two bytes or three bytes with the @ext_id now.
> > >   
> > >   [2] search the right flash_info with the 6byte ID and the new
> > >   @ext_id.
> > >   
> > >       We use "matched" variable to track the legacy two bytes @ext_id.
> > >       If the flash_info's @ext_id is three bytes, we will use the
> > >       sixth byte of the ID to check it.
> > >   
> > >   [3] add the new item to spi_nor_ids for s25fl128s.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > ---
> > > 
> > >  drivers/mtd/spi-nor/spi-nor.c |   30 +++++++++++++++++++++++++-----
> > >  1 files changed, 25 insertions(+), 5 deletions(-)
> > 
> > What exactly changed here please ?
> 
> This patch will reset the ext_jedec to the old value if the
> sixth-byte-match fails.
> 
> ------------------------------------------------------------
> +				/* reset back the ext_jedec */
> +				ext_jedec >>= 8;
> ------------------------------------------------------------

Uh oh, I think we're getting deeper and deeper into ad-hoc adjustments :-(

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Brian Norris]

Hi Huang,

You asked for a review of this patch...

On Wed, May 28, 2014 at 01:22:40PM +0800, Huang Shijie wrote:
> From 7b920141899e4e7249bd23792dc8d5f1558cb7ca Mon Sep 17 00:00:00 2001
> From: Huang Shijie <b32955@freescale.com>
> Date: Mon, 14 Apr 2014 18:09:34 +0800
> Subject: [PATCH v2] mtd: spi-nor: read 6 bytes for the ID
> 
> Currently, we read 5 bytes for ID, but s25fl128s has the same ext_id(0x4d01)
> with s25fl129p1. The s25fl128s can support the DDR Quad read, while s25fl129p1
> does not. So we have to distinguish the two NOR flashs.
> 
> This patch reads out 6 bytes for the ID, and use the 6 bytes ID to search the
> right flash_info.
> 
> The detail of the patch is:
>   [1] change the "ext_id" from u16 to u32.
>       We can store two bytes or three bytes with the @ext_id now.
> 
>   [2] search the right flash_info with the 6byte ID and the new @ext_id.
>       We use "matched" variable to track the legacy two bytes @ext_id.
>       If the flash_info's @ext_id is three bytes, we will use the
>       sixth byte of the ID to check it.
> 
>   [3] add the new item to spi_nor_ids for s25fl128s.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c |   30 +++++++++++++++++++++++++-----
>  1 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c713c86..f21d3ef 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -383,7 +383,7 @@ struct flash_info {
>  	 * then a two byte device id.
>  	 */
>  	u32		jedec_id;
> -	u16             ext_id;
> +	u32		ext_id;
>  
>  	/* The size listed here is what works with SPINOR_OP_SE, which isn't
>  	 * necessarily called a "sector" by the vendor.
> @@ -505,6 +505,7 @@ const struct spi_device_id spi_nor_ids[] = {
>  	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>  	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>  	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> +	{ "s25fl128s",	INFO(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
>  	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
>  	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>  	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
> @@ -593,12 +594,13 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
>  static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>  {
>  	int			tmp;
> -	u8			id[5];
> +	u8			id[6];
>  	u32			jedec;
> -	u16                     ext_jedec;
> +	u32                     ext_jedec;
>  	struct flash_info	*info;
> +	int			matched = -1;
>  
> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
> +	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 6);
>  	if (tmp < 0) {
>  		dev_dbg(nor->dev, " error %d reading JEDEC ID\n", tmp);
>  		return ERR_PTR(tmp);
> @@ -614,8 +616,26 @@ static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
>  	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>  		info = (void *)spi_nor_ids[tmp].driver_data;
>  		if (info->jedec_id == jedec) {
> -			if (info->ext_id == 0 || info->ext_id == ext_jedec)
> +			if (info->ext_id == 0)
>  				return &spi_nor_ids[tmp];
> +
> +			/* the legacy two bytes ext_id */
> +			if ((info->ext_id >> 16) == 0) {
> +				if (info->ext_id == ext_jedec)
> +					matched = tmp;
> +			} else {
> +				/* check the sixth byte now */
> +				ext_jedec = ext_jedec << 8 | id[5];
> +				if (info->ext_id == ext_jedec)
> +					return &spi_nor_ids[tmp];
> +
> +				/* reset back the ext_jedec */
> +				ext_jedec >>= 8;

As Marek already noted, this is very ad-hoc.

The logic here is just plain convoluted, because we haven't updated the
flash_info struct to contain the ID length. Please fix this. Just like
struct nand_flash_dev, we probably need an 'id_len' field. Bonus points
if you can extend the structure properly without having to modify the
entire existing table... Maybe a new INFO6() macro? (Better name
suggestions are welcome.)

Another reason for adding this field: what if we need to match to a
6-byte ID where the 6th byte is 0x00? I already see a 5-byte ID in our
table where the 5th byte is 0x00.

> +			}
> +		} else {
> +			/* shortcut */
> +			if (matched != -1)
> +				return &spi_nor_ids[matched];
>  		}
>  	}
>  	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);

Brian

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Mon, Aug 04, 2014 at 04:24:07PM -0700, Brian Norris wrote:
> 
> The logic here is just plain convoluted, because we haven't updated the
> flash_info struct to contain the ID length. Please fix this. Just like
> struct nand_flash_dev, we probably need an 'id_len' field. Bonus points
> if you can extend the structure properly without having to modify the
> entire existing table... Maybe a new INFO6() macro? (Better name
> suggestions are welcome.)

ok. I will try to add a "id_len" field in the next patch.
> 
> Another reason for adding this field: what if we need to match to a
> 6-byte ID where the 6th byte is 0x00? I already see a 5-byte ID in our
> table where the 5th byte is 0x00.
I think it is okay if the 6the byte is 0x00. 
Anyway, we do not meet such NOR. If we meet such NOR in future, we
can check it carefully, and see if we need to change the code a little.

thanks
Huang Shijie

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Marek Vasut]

On Tuesday, August 05, 2014 at 02:52:00 AM, Huang Shijie wrote:
> On Mon, Aug 04, 2014 at 04:24:07PM -0700, Brian Norris wrote:
> > The logic here is just plain convoluted, because we haven't updated the
> > flash_info struct to contain the ID length. Please fix this. Just like
> > struct nand_flash_dev, we probably need an 'id_len' field. Bonus points
> > if you can extend the structure properly without having to modify the
> > entire existing table... Maybe a new INFO6() macro? (Better name
> > suggestions are welcome.)
> 
> ok. I will try to add a "id_len" field in the next patch.
> 
> > Another reason for adding this field: what if we need to match to a
> > 6-byte ID where the 6th byte is 0x00? I already see a 5-byte ID in our
> > table where the 5th byte is 0x00.
> 
> I think it is okay if the 6the byte is 0x00.
> Anyway, we do not meet such NOR. If we meet such NOR in future, we
> can check it carefully, and see if we need to change the code a little.

Well, I expressed my disagreement on that matter a few times already. Like Brian 
mentioned, adding an id_len field would be the way to go. It would prevent all 
kinds of crazy wrap-arounds when reading the IDs and other such strange 
behavior.

Best regards,
Marek Vasut

Re: [PATCH] mtd: spi-nor: read 6 bytes for the ID

[Huang Shijie]

On Tue, Aug 05, 2014 at 09:14:11AM +0200, Marek Vasut wrote:
> On Tuesday, August 05, 2014 at 02:52:00 AM, Huang Shijie wrote:
> > On Mon, Aug 04, 2014 at 04:24:07PM -0700, Brian Norris wrote:
> > > The logic here is just plain convoluted, because we haven't updated the
> > > flash_info struct to contain the ID length. Please fix this. Just like
> > > struct nand_flash_dev, we probably need an 'id_len' field. Bonus points
> > > if you can extend the structure properly without having to modify the
> > > entire existing table... Maybe a new INFO6() macro? (Better name
> > > suggestions are welcome.)
> > 
> > ok. I will try to add a "id_len" field in the next patch.
> > 
> > > Another reason for adding this field: what if we need to match to a
> > > 6-byte ID where the 6th byte is 0x00? I already see a 5-byte ID in our
> > > table where the 5th byte is 0x00.
> > 
> > I think it is okay if the 6the byte is 0x00.
> > Anyway, we do not meet such NOR. If we meet such NOR in future, we
> > can check it carefully, and see if we need to change the code a little.
> 
> Well, I expressed my disagreement on that matter a few times already. Like Brian 
> mentioned, adding an id_len field would be the way to go. It would prevent all 
> kinds of crazy wrap-arounds when reading the IDs and other such strange 
> behavior.
thanks Marek, but i need to collect more comments to change patch.
Brian was busy in the past month.. 

thanks again.
Huang Shijie