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

[Miquel Raynal <miquel.raynal@bootlin.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