[PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[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>
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,
 	}, {
 		.id = SNOR_ID(0xef, 0x40, 0x19),
 		.name = "w25q256",
-- 
2.39.2

Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[e9hack]

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",

Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[Linus Walleij]

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

Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[Tudor Ambarus]

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",

Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

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",


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[Pratyush Yadav]

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/

-- 
Regards,
Pratyush Yadav

Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[Michael Walle]

>>> > 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/

Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[Pratyush Yadav]

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/
>

-- 
Regards,
Pratyush Yadav