Re: [PATCH v3] mtd: spi-nor: Correct flags for Winbond w25q128

[Michael Walle]

Hi,

Am 2023-07-14 20:29, schrieb Linus Walleij:
> The Winbond "w25q128" (actual vendor name W25Q128JV) has
> exactly the same flags as the sibling device "w25q128jv".
> The devices both require unlocking to enable write access.
>
> The actual product naming between devices vs the Linux
> strings in winbond.c:
>
> 0xef4018: "w25q128"   W25Q128JV-IM/JM
> 0xef7018: "w25q128jv" W25Q128JV-IN/IQ/JQ

This is still wrong btw. According to:
https://www.winbond.com/resource-files/W25Q128JV%20RevI%2008232021%20Plus.pdf

This should be
0xef4018: "w25q128"   W25Q128JV-IN/IQ/JQ
0xef7018: "w25q128jv" W25Q128JV-IM/JM

Otherwise, looks good. Either Tudor can change that in
place while picking up the patch or you could send a new
version.

Either way:
Reviewed-by: Michael Walle <michael@xxxxxxxx>

Thanks!
-michael

> The latter device, "w25q128jv" supports features named DTQ
> and QPI, otherwise it is the same.
>
> Not having the right flags has the annoying side effect
> that write access does not work.
>
> After this patch I can write to the flash on the Inteno
> XG6846 router.
>
> The flash memory also supports dual and quad SPI modes.
> This does not currently manifest, but by turning on SFDP
> parsing, the right SPI modes are emitted in
> /sys/kernel/debug/spi-nor/spi1.0/capabilities
> for this chip, so we also turn on this.
>
> Since we suspect that older chips may be using the same
> device ID, we need to keep NO_SFDP_FLAGS(SECT_4K) as these
> older chips may not support SFDP.
>
> cat jedec_id
> ef4018
> cat manufacturer
> winbond
> cat partname
> w25q128
> hexdump -v -C sfdp
> 00000000  53 46 44 50 05 01 00 ff  00 05 01 10 80 00 00 ff
> 00000010  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> 00000020  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> 00000030  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> 00000040  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> 00000050  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> 00000060  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> 00000070  ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> 00000080  e5 20 f9 ff ff ff ff 07  44 eb 08 6b 08 3b 42 bb
> 00000090  fe ff ff ff ff ff 00 00  ff ff 40 eb 0c 20 0f 52
> 000000a0  10 d8 00 00 36 02 a6 00  82 ea 14 c9 e9 63 76 33
> 000000b0  7a 75 7a 75 f7 a2 d5 5c  19 f7 4d ff e9 30 f8 80
>
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Michael Walle <michael@xxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> Changes in v3:
> - Keep NO_SFDP_FLAGS(SECT_4K) around.
> - Update commit message
> - Link to v2: 
> https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v2-1-50c9f1d58d6c@xxxxxxxxxx
>
> Changes in v2:
> - Only add the write access flags.
> - Use SFDP parsing to properly detect the various
>   available SPI modes.
> - Link to v1: 
> https://lore.kernel.org/r/20230712-spi-nor-winbond-w25q128-v1-1-f78f3bb42a1c@xxxxxxxxxx
> ---
>  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 834d6ba5ce70..8f30a67cd27a 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -121,6 +121,8 @@ static const struct flash_info winbond_nor_parts[] 
> = {
>  	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
>  		NO_SFDP_FLAGS(SECT_4K) },
>  	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
> +		PARSE_SFDP
> +		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>  		NO_SFDP_FLAGS(SECT_4K) },
>  	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512)
>  		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>
> ---
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> change-id: 20230711-spi-nor-winbond-w25q128-321a602ee267
>
> Best regards,