| Message ID | 1397470174-27856-1-git-send-email-b32955@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
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 ?
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
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
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
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
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
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
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
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
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
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
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
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
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
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/
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);
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(-)