[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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

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

[Linus Walleij]

From: Michael Walle <mwalle@kernel.org>

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 wasn't true. Add the no_sfdp_flags as well as the size
again.

We add the additional flags for dual and quad read because they have
been reported to work properly by Hartmut using both older and newer
versions of this flash, the similar flashes with 64Mbit and 256Mbit
already have these flags and because it will (luckily) trigger our
legacy SFDP parsing, so newer versions with SFDP support will still get
the parameters from the SFDP tables.

This was applied to mainline as
commit e49b2731c396 ("mtd: spi-nor: winbond: fix w25q128 regression")
however the code has changed a lot after v6.6 so the patch did
not apply to v6.6 or v6.1 which still has the problem.

This patch fixes the issue in the way of the old API and has
been tested on hardware. Please apply it for v6.1 and v6.6.

Reported-by: Hartmut Birr <e9hack@gmail.com>
Closes: https://lore.kernel.org/r/CALxbwRo_-9CaJmt7r7ELgu+vOcgk=xZcGHobnKf=oT2=u4d4aA@mail.gmail.com/
Fixes: 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128")
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Michael Walle <mwalle@kernel.org>
Acked-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Reviewed-by: Esben Haabendal <esben@geanix.com>
Reviewed-by: Pratyush Yadav <pratyush@kernel.org>
Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
Link: https://lore.kernel.org/r/20240621120929.2670185-1-mwalle@kernel.org
[Backported to v6.6 - vastly different due to upstream changes]
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
This fix backported to stable v6.6 and v6.1 after reports
from OpenWrt users:
https://github.com/openwrt/openwrt/issues/16796
---
 drivers/mtd/spi-nor/winbond.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index cd99c9a1c568..95dd28b9bf14 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -120,9 +120,10 @@ static const struct flash_info winbond_nor_parts[] = {
 		NO_SFDP_FLAGS(SECT_4K) },
 	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
 		NO_SFDP_FLAGS(SECT_4K) },
-	{ "w25q128", INFO(0xef4018, 0, 0, 0)
-		PARSE_SFDP
-		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
+	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
+		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
+			      SPI_NOR_QUAD_READ) },
 	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
 		.fixups = &w25q256_fixups },

---
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
change-id: 20241027-v6-6-7ed05eaccb22

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

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

[Tudor Ambarus]

Thanks Linus for taking care of this!

On 10/28/24 11:44 AM, Linus Walleij wrote:
> From: Michael Walle <mwalle@kernel.org>
> 

nit:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
says we *must* note the upstream commit ID with a separate line above
the commit text like this:

```commit <sha1> upstream.```
or alternatively:
```[ Upstream commit <sha1> ]```

With this addressed:
Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

> 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 wasn't true. Add the no_sfdp_flags as well as the size
> again.
> 
> We add the additional flags for dual and quad read because they have
> been reported to work properly by Hartmut using both older and newer
> versions of this flash, the similar flashes with 64Mbit and 256Mbit
> already have these flags and because it will (luckily) trigger our
> legacy SFDP parsing, so newer versions with SFDP support will still get
> the parameters from the SFDP tables.
> 
> This was applied to mainline as
> commit e49b2731c396 ("mtd: spi-nor: winbond: fix w25q128 regression")

invalid sha1, it must have been: d35df77707bf5ae1221b5ba1c8a88cf4fcdd4901

Cheers,
ta