| Message ID | 20240610074809.2180535-1-mwalle@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: spi-nor: winbond: fix w25q128 regression | expand |
Am 10.06.2024 um 09:48 schrieb Michael Walle: > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") > removed the flags for non-SFDP devices. It was assumed that it wasn't in > use anymore. This turned out to be wrong. Add the no_sfdp_flags as > well as the size again. > > Reported-by: e9hack <e9hack@gmail.com> Reported-by: Hartmut Birr <e9hack@gmail.com> > Fixes: 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > Hartmut, Linus, could you please test it on your boards? Also, do > you have a real name we should put in the Reported-by tag? I cannot test it. I'm using OpenWRT on both routers. OpenWRT uses linux 6.6.32. The differences are to big. Regards, Hartmut > > This will also need a manual backport to the stable kernels due to > the new syntax. But that should be straight forward. > --- > drivers/mtd/spi-nor/winbond.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c > index ca67bf2c46c3..6b6dec6f8faf 100644 > --- a/drivers/mtd/spi-nor/winbond.c > +++ b/drivers/mtd/spi-nor/winbond.c > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { > }, { > .id = SNOR_ID(0xef, 0x40, 0x18), > .name = "w25q128", > + .size = SZ_16M, > .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, > + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, > }, { > .id = SNOR_ID(0xef, 0x40, 0x19), > .name = "w25q256",
On Mon, Jun 10, 2024 at 9:48 AM Michael Walle <mwalle@kernel.org> wrote: > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") > removed the flags for non-SFDP devices. It was assumed that it wasn't in > use anymore. This turned out to be wrong. Add the no_sfdp_flags as > well as the size again. > > Reported-by: e9hack <e9hack@gmail.com> > Fixes: 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > Hartmut, Linus, could you please test it on your boards? Also, do > you have a real name we should put in the Reported-by tag? I'm unable to bring mainline Linux back up on this device, so sadly I can't test it right now. But it sure looks fine to me: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 6/10/24 8:48 AM, Michael Walle wrote: > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") That commit did: - { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) - NO_SFDP_FLAGS(SECT_4K) }, + { "w25q128", INFO(0xef4018, 0, 0, 0) + PARSE_SFDP + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, > removed the flags for non-SFDP devices. It was assumed that it wasn't in > use anymore. This turned out to be wrong. Add the no_sfdp_flags as > well as the size again. > > Reported-by: e9hack <e9hack@gmail.com> > Fixes: 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") > Signed-off-by: Michael Walle <mwalle@kernel.org> > --- > Hartmut, Linus, could you please test it on your boards? Also, do > you have a real name we should put in the Reported-by tag? > > This will also need a manual backport to the stable kernels due to > the new syntax. But that should be straight forward. > --- > drivers/mtd/spi-nor/winbond.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c > index ca67bf2c46c3..6b6dec6f8faf 100644 > --- a/drivers/mtd/spi-nor/winbond.c > +++ b/drivers/mtd/spi-nor/winbond.c > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { > }, { > .id = SNOR_ID(0xef, 0x40, 0x18), > .name = "w25q128", > + .size = SZ_16M, > .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, > + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, and here you add dual and quad to trigger SFDP parsing I guess. All fine if the old flash supports dual and quad read. But please update the commit message describing the intention. With that ACK. Would be good to have this merged soon. > }, { > .id = SNOR_ID(0xef, 0x40, 0x19), > .name = "w25q256",
Hi Tudor, On Tue Jun 18, 2024 at 12:33 PM CEST, Tudor Ambarus wrote: > On 6/10/24 8:48 AM, Michael Walle wrote: > > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") > > That commit did: > - { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) > - NO_SFDP_FLAGS(SECT_4K) }, > + { "w25q128", INFO(0xef4018, 0, 0, 0) > + PARSE_SFDP > + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, > > > removed the flags for non-SFDP devices. It was assumed that it wasn't in > > use anymore. This turned out to be wrong. Add the no_sfdp_flags as > > well as the size again. > > > > Reported-by: e9hack <e9hack@gmail.com> > > Fixes: 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") > > Signed-off-by: Michael Walle <mwalle@kernel.org> > > --- > > Hartmut, Linus, could you please test it on your boards? Also, do > > you have a real name we should put in the Reported-by tag? > > > > This will also need a manual backport to the stable kernels due to > > the new syntax. But that should be straight forward. > > --- > > drivers/mtd/spi-nor/winbond.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c > > index ca67bf2c46c3..6b6dec6f8faf 100644 > > --- a/drivers/mtd/spi-nor/winbond.c > > +++ b/drivers/mtd/spi-nor/winbond.c > > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { > > }, { > > .id = SNOR_ID(0xef, 0x40, 0x18), > > .name = "w25q128", > > + .size = SZ_16M, > > .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, > > + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, > > and here you add dual and quad to trigger SFDP parsing I guess. All fine > if the old flash supports dual and quad read. But please update the > commit message describing the intention. With that ACK. Would be good to > have this merged soon. Right. It's not because it will trigger the SFDP parsing, but because that what was tested by Esben. We're lucky that this will trigger the SFDP parsing ;) I'll explain that in more detail and add a Link: to the bug report mail. -michael > > > }, { > > .id = SNOR_ID(0xef, 0x40, 0x19), > > .name = "w25q256",
On Tue, Jun 18 2024, Michael Walle wrote: > Hi Tudor, > > On Tue Jun 18, 2024 at 12:33 PM CEST, Tudor Ambarus wrote: >> On 6/10/24 8:48 AM, Michael Walle wrote: >> > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") >> >> That commit did: >> - { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) >> - NO_SFDP_FLAGS(SECT_4K) }, >> + { "w25q128", INFO(0xef4018, 0, 0, 0) >> + PARSE_SFDP >> + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, >> [...] >> > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c >> > index ca67bf2c46c3..6b6dec6f8faf 100644 >> > --- a/drivers/mtd/spi-nor/winbond.c >> > +++ b/drivers/mtd/spi-nor/winbond.c >> > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { >> > }, { >> > .id = SNOR_ID(0xef, 0x40, 0x18), >> > .name = "w25q128", >> > + .size = SZ_16M, >> > .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, >> > + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, >> >> and here you add dual and quad to trigger SFDP parsing I guess. All fine >> if the old flash supports dual and quad read. But please update the >> commit message describing the intention. With that ACK. Would be good to >> have this merged soon. > > Right. It's not because it will trigger the SFDP parsing, but > because that what was tested by Esben. We're lucky that this will > trigger the SFDP parsing ;) I'll explain that in more detail and add > a Link: to the bug report mail. Should we treat this flash similar to the Macronix ones Esben sent out patches for [0]? It seems that there are some old parts without SFDP support and new ones with SFDP support. From your comment in [1]: > This is an entry matching various flash families from Winbond, see my > reply in v1. I'm not sure we should remove these as we could break the > older ones, which might or might not have SFDP tables. We don't know. Since the entry matches multiple families, do _all_ of them support dual and quad read? If not, attempting to enable dual or quad reads on them can cause problems. Also, for parts that _do_ have SFDP available, won't it be better to use the information in SFDP instead of our hard-coded ones anyway? Using SPI_NOR_TRY_SFDP here would let us do that. [0] https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/ [1] https://lore.kernel.org/linux-mtd/0525440a652854a2a575256cd07d3559@walle.cc/
>>> > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") >>> >>> That commit did: >>> - { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) >>> - NO_SFDP_FLAGS(SECT_4K) }, >>> + { "w25q128", INFO(0xef4018, 0, 0, 0) >>> + PARSE_SFDP >>> + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, >>> > [...] >>> > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c >>> > index ca67bf2c46c3..6b6dec6f8faf 100644 >>> > --- a/drivers/mtd/spi-nor/winbond.c >>> > +++ b/drivers/mtd/spi-nor/winbond.c >>> > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { >>> > }, { >>> > .id = SNOR_ID(0xef, 0x40, 0x18), >>> > .name = "w25q128", >>> > + .size = SZ_16M, >>> > .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, >>> > + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, >>> >>> and here you add dual and quad to trigger SFDP parsing I guess. All >>> fine >>> if the old flash supports dual and quad read. But please update the >>> commit message describing the intention. With that ACK. Would be good >>> to >>> have this merged soon. >> >> Right. It's not because it will trigger the SFDP parsing, but >> because that what was tested by Esben. We're lucky that this will >> trigger the SFDP parsing ;) I'll explain that in more detail and add >> a Link: to the bug report mail. > > Should we treat this flash similar to the Macronix ones Esben sent out > patches for [0]? It seems that there are some old parts without SFDP > support and new ones with SFDP support. From your comment in [1]: > >> This is an entry matching various flash families from Winbond, see my >> reply in v1. I'm not sure we should remove these as we could break the >> older ones, which might or might not have SFDP tables. We don't know. > > Since the entry matches multiple families, do _all_ of them support > dual > and quad read? If not, attempting to enable dual or quad reads on them > can cause problems. I rely on the information Helmut provided. Also the w25q64 and the w25q256 both have these flags set. So I'd say it's less likely the 128 doesn't support it. > Also, for parts that _do_ have SFDP available, won't it be better to > use > the information in SFDP instead of our hard-coded ones anyway? Using > SPI_NOR_TRY_SFDP here would let us do that. Sure, but this is about fixing the referenced commit. A later patch will then move that to TRY_SFDP. We can't fix this regression by introducing new code IMHO. This seems to be the easiest fix. -michael > > [0] > https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/ > [1] > https://lore.kernel.org/linux-mtd/0525440a652854a2a575256cd07d3559@walle.cc/
On Thu, Jun 20 2024, Michael Walle wrote: >>>> > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") >>>> That commit did: >>>> - { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256) >>>> - NO_SFDP_FLAGS(SECT_4K) }, >>>> + { "w25q128", INFO(0xef4018, 0, 0, 0) >>>> + PARSE_SFDP >>>> + FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) }, >>>> >> [...] >>>> > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c >>>> > index ca67bf2c46c3..6b6dec6f8faf 100644 >>>> > --- a/drivers/mtd/spi-nor/winbond.c >>>> > +++ b/drivers/mtd/spi-nor/winbond.c >>>> > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { >>>> > }, { >>>> > .id = SNOR_ID(0xef, 0x40, 0x18), >>>> > .name = "w25q128", >>>> > + .size = SZ_16M, >>>> > .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, >>>> > + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, >>>> and here you add dual and quad to trigger SFDP parsing I guess. All fine >>>> if the old flash supports dual and quad read. But please update the >>>> commit message describing the intention. With that ACK. Would be good to >>>> have this merged soon. >>> Right. It's not because it will trigger the SFDP parsing, but >>> because that what was tested by Esben. We're lucky that this will >>> trigger the SFDP parsing ;) I'll explain that in more detail and add >>> a Link: to the bug report mail. >> Should we treat this flash similar to the Macronix ones Esben sent out >> patches for [0]? It seems that there are some old parts without SFDP >> support and new ones with SFDP support. From your comment in [1]: >> >>> This is an entry matching various flash families from Winbond, see my >>> reply in v1. I'm not sure we should remove these as we could break the >>> older ones, which might or might not have SFDP tables. We don't know. >> Since the entry matches multiple families, do _all_ of them support dual >> and quad read? If not, attempting to enable dual or quad reads on them >> can cause problems. > > I rely on the information Helmut provided. Also the w25q64 and the w25q256 > both have these flags set. So I'd say it's less likely the 128 doesn't > support it. Okay, fair enough. > >> Also, for parts that _do_ have SFDP available, won't it be better to use >> the information in SFDP instead of our hard-coded ones anyway? Using >> SPI_NOR_TRY_SFDP here would let us do that. > > Sure, but this is about fixing the referenced commit. A later patch will > then move that to TRY_SFDP. We can't fix this regression by introducing > new code IMHO. This seems to be the easiest fix. New code will make it harder to backport to stable kernels. Beyond that, I don't see why we can't fix a regression with new code. Here's why I suggested this: before 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128"), all flashes with this ID got only the SECT_4K flag -- and thus only single SPI mode. After that commit, all flashes with this ID got their settings configured via SFDP. Using the TRY_SFDP approach allows both of those configurations to co-exist. Old ones still use the old configuration, new ones get to use SFDP. Now we add a different configuration that adds dual and quad reads to these old flashes. As mentioned above, this is unlikely to cause problems, but a new configuration regardless. So _in principle_ I think TRY_SFDP would be the best balance. But I get your point -- since both w25q64 and w25q256 have these flags, it is likely someone just never bothered updating w25q128. So this patch LGTM. I'll apply it once you send a new version with an updated commit message. > > -michael > >> [0] >> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/ >> [1] >> https://lore.kernel.org/linux-mtd/0525440a652854a2a575256cd07d3559@walle.cc/ >
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c index ca67bf2c46c3..6b6dec6f8faf 100644 --- a/drivers/mtd/spi-nor/winbond.c +++ b/drivers/mtd/spi-nor/winbond.c @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = { }, { .id = SNOR_ID(0xef, 0x40, 0x18), .name = "w25q128", + .size = SZ_16M, .flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB, + .no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ, }, { .id = SNOR_ID(0xef, 0x40, 0x19), .name = "w25q256",
Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") removed the flags for non-SFDP devices. It was assumed that it wasn't in use anymore. This turned out to be wrong. Add the no_sfdp_flags as well as the size again. Reported-by: e9hack <e9hack@gmail.com> Fixes: 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128") Signed-off-by: Michael Walle <mwalle@kernel.org> --- Hartmut, Linus, could you please test it on your boards? Also, do you have a real name we should put in the Reported-by tag? This will also need a manual backport to the stable kernels due to the new syntax. But that should be straight forward. --- drivers/mtd/spi-nor/winbond.c | 2 ++ 1 file changed, 2 insertions(+)