drivers/mtd/spi-nor/winbond.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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>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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.