[PATCH] mtd: spi-nor: Fix w25q01jv flags

[PATCH] mtd: spi-nor: Fix w25q01jv flags

[Marc Olberding]

The previous support for w25q01jv was causing read/write
miscompares on flashing with the aspeed ast2600. Add in
extra flags to allow the chip to identify as having 4K
sectors if the sfdp tables aren't present, as well
as reporting the name and adding the LOCK and TB flag.

After this change, no miscompares were seen.

Fixes: 9b4db032fb2b ("mtd: spi-nor: winbond: Add support for w25q01jv")
Signed-off-by: Marc Olberding <molberding@nvidia.com>
---
The initial commit[1] that came in for w25q01jv [2] support
lacks the 4K sector flags in the case that the sfdp flags don't
exist on the chip. This caused filesystem corruption upon writes.
Add in the flags as well as the chips size and name.

Testing:
After this patch, BMC's backed by the w25q01jv succeed on boot
as well as self-flash

[1] https://lore.kernel.org/all/20251014050108.665338-1-chou.cosmo@gmail.com/
[2] https://www.winbond.com/resource-files/W25Q01JV%20SPI%20RevE%2003042024%20Plus.pdf
---
 drivers/mtd/spi-nor/winbond.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 63a93c9eb9174b073e19c41eeada33b23a99b184..7d4119afc8c6b4135b5a055b52b1ade7e3ef926c 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -227,8 +227,11 @@ static const struct flash_info winbond_nor_parts[] = {
 		.size = SZ_64M,
 		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
 	}, {
-		/* W25Q01JV */
 		.id = SNOR_ID(0xef, 0x40, 0x21),
+		.name = "w25q01jv",
+		.size = SZ_128M,
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB,
+		.no_sfdp_flags = SECT_4K,
 		.fixups = &winbond_nor_multi_die_fixups,
 	}, {
 		.id = SNOR_ID(0xef, 0x50, 0x12),

---
base-commit: fd95357fd8c6778ac7dea6c57a19b8b182b6e91f
change-id: 20251121-w25q01jv_fixup-468539f2a777

Best regards,
-- 
Marc Olberding <molberding@nvidia.com>

Re: [PATCH] mtd: spi-nor: Fix w25q01jv flags

[Miquel Raynal]

Hi Marc,

On 21/11/2025 at 14:35:34 -08, Marc Olberding <molberding@nvidia.com> wrote:

> The previous support for w25q01jv was causing read/write
> miscompares on flashing with the aspeed ast2600. Add in
> extra flags to allow the chip to identify as having 4K
> sectors if the sfdp tables aren't present, as well

Are we sure about the reason? Normally this is a non-sfdp flag, which
means even if you have sfdp tables, you must mark this flag
manually. These chips should always have the sfdp tables. Did you face
cases where they are not populated?

> as reporting the name and adding the LOCK and TB flag.
>
> After this change, no miscompares were seen.
>
> Fixes: 9b4db032fb2b ("mtd: spi-nor: winbond: Add support for w25q01jv")
> Signed-off-by: Marc Olberding <molberding@nvidia.com>
> ---
> The initial commit[1] that came in for w25q01jv [2] support
> lacks the 4K sector flags in the case that the sfdp flags don't
> exist on the chip. This caused filesystem corruption upon writes.
> Add in the flags as well as the chips size and name.
>
> Testing:
> After this patch, BMC's backed by the w25q01jv succeed on boot
> as well as self-flash
>
> [1] https://lore.kernel.org/all/20251014050108.665338-1-chou.cosmo@gmail.com/
> [2] https://www.winbond.com/resource-files/W25Q01JV%20SPI%20RevE%2003042024%20Plus.pdf
> ---
>  drivers/mtd/spi-nor/winbond.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 63a93c9eb9174b073e19c41eeada33b23a99b184..7d4119afc8c6b4135b5a055b52b1ade7e3ef926c 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -227,8 +227,11 @@ static const struct flash_info winbond_nor_parts[] = {
>  		.size = SZ_64M,
>  		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>  	}, {
> -		/* W25Q01JV */
>  		.id = SNOR_ID(0xef, 0x40, 0x21),
> +		.name = "w25q01jv",

Changing the comment into a name is not relevant here for two reasons:
- this is deprecated
- this patch is a fix, any further additions should be in a dedicated patch.

> +		.size = SZ_128M,

Do you really need this field? How did the flash even worked before if
you need this?

> +		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB,

I have not used nor tested block protection on that chip. If it's an
addition you must put that in a separated patch. Fixes will be queued
faster than feature additions, you cannot mix both.

> +		.no_sfdp_flags = SECT_4K,

This one is the right fix and should stand alone in its own patch (first
in the series if you add support for the block protection).

>  		.fixups = &winbond_nor_multi_die_fixups,
>  	}, {
>  		.id = SNOR_ID(0xef, 0x50, 0x12),

Thanks,
Miquèl

Re: [PATCH] mtd: spi-nor: Fix w25q01jv flags

[Michael Walle]

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

Hi,

>> +		.no_sfdp_flags = SECT_4K,
>
> This one is the right fix and should stand alone in its own patch (first
> in the series if you add support for the block protection).

Only if that flash really doesn't have SFDP. But since the entry
didn't have a size property the flash *must* have SFDP in the first
place.  Otherwise it won't even be probed. Please provide a dump of
the SFDP tables, see [1]. Also please provide the contents of
/sys/kernel/debug/spi-nor/spiN.N/params.

-michael

[1] https://docs.kernel.org/driver-api/mtd/spi-nor.html#minimum-testing-requirements

>
>>  		.fixups = &winbond_nor_multi_die_fixups,
>>  	}, {
>>  		.id = SNOR_ID(0xef, 0x50, 0x12),
>
> Thanks,
> Miquèl


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

Re: [PATCH] mtd: spi-nor: Fix w25q01jv flags

[Miquel Raynal]

Hi,

On 24/11/2025 at 09:12:38 +01, "Michael Walle" <mwalle@kernel.org> wrote:

> Hi,
>
>>> +		.no_sfdp_flags = SECT_4K,
>>
>> This one is the right fix and should stand alone in its own patch (first
>> in the series if you add support for the block protection).
>
> Only if that flash really doesn't have SFDP. But since the entry
> didn't have a size property the flash *must* have SFDP in the first
> place.  Otherwise it won't even be probed. Please provide a dump of
> the SFDP tables, see [1].

SFDP data is in lore, but not the params which are missing (?) Marc, can
you compare with your data?
https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/

> Also please provide the contents of
> /sys/kernel/debug/spi-nor/spiN.N/params.
>
> -michael

My understanding (which may clearly be erroneous) is that most of these
flashes support 4K blocks but somehow don't advertise it in their SFDP
data, so every time we describe a chip we must remember to tick that
flag. I guess all^Wmost chips have 4k blocks compatibility support, but in
general we prefer to use bigger blocks (the ones advertised in the SFDP
data). Michael, am I being mislead by the decades of history that went
through the spi-nor core? :)

> [1] https://docs.kernel.org/driver-api/mtd/spi-nor.html#minimum-testing-requirements
>
>>
>>>  		.fixups = &winbond_nor_multi_die_fixups,
>>>  	}, {
>>>  		.id = SNOR_ID(0xef, 0x50, 0x12),
>>
>> Thanks,
>> Miquèl

Re: [PATCH] mtd: spi-nor: Fix w25q01jv flags

[Michael Walle]

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

On Mon Nov 24, 2025 at 9:25 AM CET, Miquel Raynal wrote:
> Hi,
>
> On 24/11/2025 at 09:12:38 +01, "Michael Walle" <mwalle@kernel.org> wrote:
>
>> Hi,
>>
>>>> +		.no_sfdp_flags = SECT_4K,
>>>
>>> This one is the right fix and should stand alone in its own patch (first
>>> in the series if you add support for the block protection).
>>
>> Only if that flash really doesn't have SFDP. But since the entry
>> didn't have a size property the flash *must* have SFDP in the first
>> place.  Otherwise it won't even be probed. Please provide a dump of
>> the SFDP tables, see [1].
>
> SFDP data is in lore

At least yours :) And if I decode that correctly by hand, it has the
4k erase size bit set as well as the correct opcode 20h or 21h for
4byte addressing.

> , but not the params which are missing (?) Marc, can
> you compare with your data?
> https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
>
>> Also please provide the contents of
>> /sys/kernel/debug/spi-nor/spiN.N/params.
>>
>> -michael
>
> My understanding (which may clearly be erroneous) is that most of these
> flashes support 4K blocks but somehow don't advertise it in their SFDP
> data, so every time we describe a chip we must remember to tick that
> flag.

Which flag? SECT_4K? I don't think that will be used at all, does
it? It's only used in spi_nor_no_sfdp_init_params() which in turn is
only called in spi_nor_init_params_deprecated() (or if SKIP_SFDP is
set). 

> I guess all^Wmost chips have 4k blocks compatibility support, but in
> general we prefer to use bigger blocks (the ones advertised in the SFDP
> data). Michael, am I being mislead by the decades of history that went
> through the spi-nor core? :)

You mean CONFIG_MTD_SPI_NOR_USE_4K_SECTORS? But that has nothing to
to with the flashdb/sfdp parsing.

-michael

>
>> [1] https://docs.kernel.org/driver-api/mtd/spi-nor.html#minimum-testing-requirements
>>
>>>
>>>>  		.fixups = &winbond_nor_multi_die_fixups,
>>>>  	}, {
>>>>  		.id = SNOR_ID(0xef, 0x50, 0x12),
>>>
>>> Thanks,
>>> Miquèl


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

Re: [PATCH] mtd: spi-nor: Fix w25q01jv flags

[Miquel Raynal]

On 24/11/2025 at 09:50:27 +01, "Michael Walle" <mwalle@kernel.org> wrote:

> On Mon Nov 24, 2025 at 9:25 AM CET, Miquel Raynal wrote:
>> Hi,
>>
>> On 24/11/2025 at 09:12:38 +01, "Michael Walle" <mwalle@kernel.org> wrote:
>>
>>> Hi,
>>>
>>>>> +		.no_sfdp_flags = SECT_4K,
>>>>
>>>> This one is the right fix and should stand alone in its own patch (first
>>>> in the series if you add support for the block protection).
>>>
>>> Only if that flash really doesn't have SFDP. But since the entry
>>> didn't have a size property the flash *must* have SFDP in the first
>>> place.  Otherwise it won't even be probed. Please provide a dump of
>>> the SFDP tables, see [1].
>>
>> SFDP data is in lore
>
> At least yours :) And if I decode that correctly by hand, it has the
> 4k erase size bit set as well as the correct opcode 20h or 21h for
> 4byte addressing.
>
>> , but not the params which are missing (?) Marc, can
>> you compare with your data?
>> https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
>>
>>> Also please provide the contents of
>>> /sys/kernel/debug/spi-nor/spiN.N/params.
>>>
>>> -michael
>>
>> My understanding (which may clearly be erroneous) is that most of these
>> flashes support 4K blocks but somehow don't advertise it in their SFDP
>> data, so every time we describe a chip we must remember to tick that
>> flag.
>
> Which flag? SECT_4K? I don't think that will be used at all, does
> it? It's only used in spi_nor_no_sfdp_init_params() which in turn is
> only called in spi_nor_init_params_deprecated() (or if SKIP_SFDP is
> set). 
>
>> I guess all^Wmost chips have 4k blocks compatibility support, but in
>> general we prefer to use bigger blocks (the ones advertised in the SFDP
>> data). Michael, am I being mislead by the decades of history that went
>> through the spi-nor core? :)
>
> You mean CONFIG_MTD_SPI_NOR_USE_4K_SECTORS? But that has nothing to
> to with the flashdb/sfdp parsing.

Well I expect CONFIG_MTD_SPI_NOR_USE_4K_SECTORS to have no effect
if the SECT_4K flag is unset. I believe Marc is using the same chip as I
am, but enabled this option for compatibility reasons?

But as you say, this 4K capability is advertised by "my" chip, so if
Marc faces an issue with it, it may indicate that we are having an ID
collision?

Thanks,
Miquèl

Re: [PATCH] mtd: spi-nor: Fix w25q01jv flags

[Marc Olberding]

On Mon, Nov 24, 2025 at 10:15:28AM +0100, Miquel Raynal wrote:
> On 24/11/2025 at 09:50:27 +01, "Michael Walle" <mwalle@kernel.org> wrote:
> 
> > On Mon Nov 24, 2025 at 9:25 AM CET, Miquel Raynal wrote:
> >> Hi,
> >>
> >> On 24/11/2025 at 09:12:38 +01, "Michael Walle" <mwalle@kernel.org> wrote:
> >>
> >>> Hi,
> >>>
> >>>>> +		.no_sfdp_flags = SECT_4K,
> >>>>
> >>>> This one is the right fix and should stand alone in its own patch (first
> >>>> in the series if you add support for the block protection).
> >>>
> >>> Only if that flash really doesn't have SFDP. But since the entry
> >>> didn't have a size property the flash *must* have SFDP in the first
> >>> place.  Otherwise it won't even be probed. Please provide a dump of
> >>> the SFDP tables, see [1].
> >>
> >> SFDP data is in lore
> >
> > At least yours :) And if I decode that correctly by hand, it has the
> > 4k erase size bit set as well as the correct opcode 20h or 21h for
> > 4byte addressing.
> >
> >> , but not the params which are missing (?) Marc, can
> >> you compare with your data?
> >> https://lore.kernel.org/all/20250110-winbond-6-12-rc1-nor-volatile-bit-v3-1-735363f8cc7d@bootlin.com/
> >>
> >>> Also please provide the contents of
> >>> /sys/kernel/debug/spi-nor/spiN.N/params.
> >>>
> >>> -michael
Thanks all for the discussion, I appreciate the insight. After investigating some more,
it looks like the sfdp table is identical. I'll be honest I'm not a spi expert.

This data was taken with the original patch, as sfdp didn't populate as I had provided
the size field in the spi-nor-info table, this seems to cause us to skip any sfdp parsing
and go down the deprecated path. Looking at the info table, it looks like all but 4
spi chips in the winbond table use the deprecated path. Its not clear to me however,
why it works on Miquel's board but not mine. 

/tmp/xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450060101ff00060110800000ff84000102d00000ff03000102f000
00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffe520fbffffffff3f44eb086b083b42bbfeffffffffff
0000ffff40eb0c200f5210d800003602a60082ea14e2e96376337a757a75
f7a2d55c19f74dffe970f9a5ffffffffffffffffffffffffffffffffff0a
f0ff21ffdcff

md5sum ./sfdp 
a7b9dbf76e99a33db99e557b6676588a  ./sfdp

cat /sys/kernel/debug/spi-nor/spi0.0/params
name            (null)
id              ef 40 21 00 00 00
size            128 MiB
write size      1
page size       256
address nbytes  4
flags           4B_OPCODES | HAS_4BAIT | HAS_16BIT_SR | NO_READ_CR | SOFT_RESET

opcodes
 read           0x6c
  dummy cycles  8
 erase          0xdc
 program        0x34
 8D extension   none

protocols
 read           1S-1S-4S
 write          1S-1S-4S
 register       1S-1S-1S

erase commands
 21 (4.00 KiB) [1]
 dc (64.0 KiB) [3]
 c7 (128 MiB)

sector map
 region (in hex)   | erase mask | overlaid
 ------------------+------------+----------
 00000000-07ffffff |     [   3] | no


> >> My understanding (which may clearly be erroneous) is that most of these
> >> flashes support 4K blocks but somehow don't advertise it in their SFDP
> >> data, so every time we describe a chip we must remember to tick that
> >> flag.
> >
> > Which flag? SECT_4K? I don't think that will be used at all, does
> > it? It's only used in spi_nor_no_sfdp_init_params() which in turn is
> > only called in spi_nor_init_params_deprecated() (or if SKIP_SFDP is
> > set). 
> >

See my above comment about setting the size in the tables. This eventually
gets used by spi_nor_needs_sfdp. This ends up being most of the winbond chips
defined today.

All in all, I'm a little confused what the difference is between Miquel's and my
tests. Any direction would be appreciated, in the meantime, I'm going to continue
debugging on my end.

Best regards,
Marc Olberding

Re: [PATCH] mtd: spi-nor: Fix w25q01jv flags

[Michael Walle]

Hi,

> This data was taken with the original patch, as sfdp didn't populate as I had provided
> the size field in the spi-nor-info table, this seems to cause us to skip any sfdp parsing
> and go down the deprecated path.

But the SFDP should still be parsed. Only if you have the SKIP_SFDP
flag set, parsing is turned off. Which kernel are you using? Which
SPI controller are you using?

> Looking at the info table, it looks like all but 4
> spi chips in the winbond table use the deprecated path. Its not clear to me however,
> why it works on Miquel's board but not mine. 
>
> /tmp/xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 53464450060101ff00060110800000ff84000102d00000ff03000102f000
> 00ffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
> ffffffffffffffffe520fbffffffff3f44eb086b083b42bbfeffffffffff
> 0000ffff40eb0c200f5210d800003602a60082ea14e2e96376337a757a75
> f7a2d55c19f74dffe970f9a5ffffffffffffffffffffffffffffffffff0a
> f0ff21ffdcff
>
> md5sum ./sfdp 
> a7b9dbf76e99a33db99e557b6676588a  ./sfdp
>
> cat /sys/kernel/debug/spi-nor/spi0.0/params
> name            (null)
> id              ef 40 21 00 00 00
> size            128 MiB
> write size      1
> page size       256
> address nbytes  4
> flags           4B_OPCODES | HAS_4BAIT | HAS_16BIT_SR | NO_READ_CR | SOFT_RESET
>
> opcodes
>  read           0x6c
>   dummy cycles  8
>  erase          0xdc
>  program        0x34
>  8D extension   none
>
> protocols
>  read           1S-1S-4S
>  write          1S-1S-4S
>  register       1S-1S-1S
>
> erase commands
>  21 (4.00 KiB) [1]

4k erases are parsed correctly as expected..

>  dc (64.0 KiB) [3]
>  c7 (128 MiB)
>
> sector map
>  region (in hex)   | erase mask | overlaid
>  ------------------+------------+----------
>  00000000-07ffffff |     [   3] | no

..but not selected. I guess you don't have
CONFIG_MTD_SPI_NOR_USE_4K_SECTORS set, do you? In that case I'm lost
why it should have been working with this patch applied. Do you see
any differences in the "params" file if the patch is applied?

>> >> My understanding (which may clearly be erroneous) is that most of these
>> >> flashes support 4K blocks but somehow don't advertise it in their SFDP
>> >> data, so every time we describe a chip we must remember to tick that
>> >> flag.
>> >
>> > Which flag? SECT_4K? I don't think that will be used at all, does
>> > it? It's only used in spi_nor_no_sfdp_init_params() which in turn is
>> > only called in spi_nor_init_params_deprecated() (or if SKIP_SFDP is
>> > set). 
>> >
>
> See my above comment about setting the size in the tables. This eventually
> gets used by spi_nor_needs_sfdp. This ends up being most of the winbond chips
> defined today.

Yeah because they were added before we switched to parsing SFDP.

> All in all, I'm a little confused what the difference is between Miquel's and my
> tests. Any direction would be appreciated, in the meantime, I'm going to continue
> debugging on my end.

What are your tests? Did you try the tests in the "minimal testing
requirements" chapter? Did you see any errors?

-michael