| Message ID | 20140528052237.GA7984@shlinux1.ap.freescale.net |
|---|---|
| State | Changes Requested |
| Headers | show |
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);
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
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
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
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
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
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
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
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);