| Message ID | 1414576675-7792-1-git-send-email-Chunhe.Lan@freescale.com |
|---|---|
| State | Accepted |
| Commit | 4414d3efe57df6168d1ea564d56557d76b5ad8c7 |
| Headers | show |
On Wednesday, October 29, 2014 at 10:57:55 AM, Chunhe Lan wrote: [...] There are problems with this patch. Firstly, it misses any description explaining why the change took place at all. From an outside observer point of view, this change seems random at best. Secondly, the change in itself makes no sense -- it just reorders the entries in an array. I can only speculate here, that your SPI NOR was recognised as some other part, right ? That's why moving the n25q032 higher resolved the problem for you, right ? The problem is with the fragility of this code which matches the JEDEC ID and type of the SPI NOR. I recall Huang had some patches which tried to resolve this, not sure what the status of those patches is though. > Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com> > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Huang Shijie <shijie8@gmail.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index ae16aa2..1fa5e05 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -530,6 +530,7 @@ const struct spi_device_id spi_nor_ids[] = { > { "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) }, > > /* Micron */ > + { "n25q032", INFO(0x20ba16, 0, 64 * 1024, 64, 0) }, > { "n25q064", INFO(0x20ba17, 0, 64 * 1024, 128, 0) }, > { "n25q128a11", INFO(0x20bb18, 0, 64 * 1024, 256, 0) }, > { "n25q128a13", INFO(0x20ba18, 0, 64 * 1024, 256, 0) }, > @@ -586,7 +587,6 @@ const struct spi_device_id spi_nor_ids[] = { > { "m25p32", INFO(0x202016, 0, 64 * 1024, 64, 0) }, > { "m25p64", INFO(0x202017, 0, 64 * 1024, 128, 0) }, > { "m25p128", INFO(0x202018, 0, 256 * 1024, 64, 0) }, > - { "n25q032", INFO(0x20ba16, 0, 64 * 1024, 64, 0) }, > > { "m25p05-nonjedec", INFO(0, 0, 32 * 1024, 2, 0) }, > { "m25p10-nonjedec", INFO(0, 0, 32 * 1024, 4, 0) }, Best regards, Marek Vasut
On 29 October 2014 15:05, Marek Vasut <marex@denx.de> wrote: > On Wednesday, October 29, 2014 at 10:57:55 AM, Chunhe Lan wrote: > > [...] > > There are problems with this patch. Firstly, it misses any description > explaining why the change took place at all. From an outside observer > point of view, this change seems random at best. This looks like a normal cleaning for me. > Secondly, the change in itself makes no sense -- it just reorders the > entries in an array. It moves Micron entry to the Micron pseudo-group, which makes sense to me. > I can only speculate here, that your SPI NOR was > recognised as some other part, right ? That's why moving the n25q032 > higher resolved the problem for you, right ? > > The problem is with the fragility of this code which matches the JEDEC > ID and type of the SPI NOR. I recall Huang had some patches which tried > to resolve this, not sure what the status of those patches is though. Wait, what? OK, this makes things tricky. Does this patch really change any behavior? How does it happen? I don't see any duplicated entry with the same JEDEC ID (0x20ba16). Could you give us some more details?
On Wednesday, October 29, 2014 at 06:40:46 PM, Rafał Miłecki wrote: > On 29 October 2014 15:05, Marek Vasut <marex@denx.de> wrote: > > On Wednesday, October 29, 2014 at 10:57:55 AM, Chunhe Lan wrote: > > > > [...] > > > > There are problems with this patch. Firstly, it misses any description > > explaining why the change took place at all. From an outside observer > > point of view, this change seems random at best. > > This looks like a normal cleaning for me. We can only guess, since the commit message is missing. > > Secondly, the change in itself makes no sense -- it just reorders the > > entries in an array. > > It moves Micron entry to the Micron pseudo-group, which makes sense to me. I hope it's just that. > > I can only speculate here, that your SPI NOR was > > recognised as some other part, right ? That's why moving the n25q032 > > higher resolved the problem for you, right ? > > > > The problem is with the fragility of this code which matches the JEDEC > > ID and type of the SPI NOR. I recall Huang had some patches which tried > > to resolve this, not sure what the status of those patches is though. > > Wait, what? OK, this makes things tricky. Does this patch really > change any behavior? How does it happen? I don't see any duplicated > entry with the same JEDEC ID (0x20ba16). > > Could you give us some more details? Please see this thread: http://lists.infradead.org/pipermail/linux-mtd/2014-April/053308.html This is where the discussion about ordering in the table took place and where the patch for implementing the support for length of READID return value was proposed. Best regards, Marek Vasut
On 29 October 2014 20:19, Marek Vasut <marex@denx.de> wrote: > On Wednesday, October 29, 2014 at 06:40:46 PM, Rafał Miłecki wrote: >> On 29 October 2014 15:05, Marek Vasut <marex@denx.de> wrote: >> > On Wednesday, October 29, 2014 at 10:57:55 AM, Chunhe Lan wrote: >> > >> > [...] >> > >> > There are problems with this patch. Firstly, it misses any description >> > explaining why the change took place at all. From an outside observer >> > point of view, this change seems random at best. >> >> This looks like a normal cleaning for me. > > We can only guess, since the commit message is missing. Maybe the topic says everything the patch does :) >> > I can only speculate here, that your SPI NOR was >> > recognised as some other part, right ? That's why moving the n25q032 >> > higher resolved the problem for you, right ? >> > >> > The problem is with the fragility of this code which matches the JEDEC >> > ID and type of the SPI NOR. I recall Huang had some patches which tried >> > to resolve this, not sure what the status of those patches is though. >> >> Wait, what? OK, this makes things tricky. Does this patch really >> change any behavior? How does it happen? I don't see any duplicated >> entry with the same JEDEC ID (0x20ba16). >> >> Could you give us some more details? > > Please see this thread: > http://lists.infradead.org/pipermail/linux-mtd/2014-April/053308.html > > This is where the discussion about ordering in the table took place > and where the patch for implementing the support for length of READID > return value was proposed. As I said, entry "n25q032" doesn't share JEDEC with anything else and does not even have an ext_id. So I don't think it's related to the s25fl128s vs. s25fl129p1 problem.
On Wednesday, October 29, 2014 at 08:43:32 PM, Rafał Miłecki wrote: > On 29 October 2014 20:19, Marek Vasut <marex@denx.de> wrote: > > On Wednesday, October 29, 2014 at 06:40:46 PM, Rafał Miłecki wrote: > >> On 29 October 2014 15:05, Marek Vasut <marex@denx.de> wrote: > >> > On Wednesday, October 29, 2014 at 10:57:55 AM, Chunhe Lan wrote: > >> > > >> > [...] > >> > > >> > There are problems with this patch. Firstly, it misses any description > >> > explaining why the change took place at all. From an outside observer > >> > point of view, this change seems random at best. > >> > >> This looks like a normal cleaning for me. > > > > We can only guess, since the commit message is missing. > > Maybe the topic says everything the patch does :) Maybe :) > >> > I can only speculate here, that your SPI NOR was > >> > recognised as some other part, right ? That's why moving the n25q032 > >> > higher resolved the problem for you, right ? > >> > > >> > The problem is with the fragility of this code which matches the JEDEC > >> > ID and type of the SPI NOR. I recall Huang had some patches which > >> > tried to resolve this, not sure what the status of those patches is > >> > though. > >> > >> Wait, what? OK, this makes things tricky. Does this patch really > >> change any behavior? How does it happen? I don't see any duplicated > >> entry with the same JEDEC ID (0x20ba16). > >> > >> Could you give us some more details? > > > > Please see this thread: > > http://lists.infradead.org/pipermail/linux-mtd/2014-April/053308.html > > > > This is where the discussion about ordering in the table took place > > and where the patch for implementing the support for length of READID > > return value was proposed. > > As I said, entry "n25q032" doesn't share JEDEC with anything else and > does not even have an ext_id. So I don't think it's related to the > s25fl128s vs. s25fl129p1 problem. I hope that's the case. Clearly, proper commit message would help clarify this case ;-) Best regards, Marek Vasut
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index ae16aa2..1fa5e05 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -530,6 +530,7 @@ const struct spi_device_id spi_nor_ids[] = { { "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) }, /* Micron */ + { "n25q032", INFO(0x20ba16, 0, 64 * 1024, 64, 0) }, { "n25q064", INFO(0x20ba17, 0, 64 * 1024, 128, 0) }, { "n25q128a11", INFO(0x20bb18, 0, 64 * 1024, 256, 0) }, { "n25q128a13", INFO(0x20ba18, 0, 64 * 1024, 256, 0) }, @@ -586,7 +587,6 @@ const struct spi_device_id spi_nor_ids[] = { { "m25p32", INFO(0x202016, 0, 64 * 1024, 64, 0) }, { "m25p64", INFO(0x202017, 0, 64 * 1024, 128, 0) }, { "m25p128", INFO(0x202018, 0, 256 * 1024, 64, 0) }, - { "n25q032", INFO(0x20ba16, 0, 64 * 1024, 64, 0) }, { "m25p05-nonjedec", INFO(0, 0, 32 * 1024, 2, 0) }, { "m25p10-nonjedec", INFO(0, 0, 32 * 1024, 4, 0) },
Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com> Cc: Brian Norris <computersforpeace@gmail.com> Cc: Marek Vasut <marex@denx.de> Cc: Huang Shijie <shijie8@gmail.com> --- drivers/mtd/spi-nor/spi-nor.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)