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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

Hi, Linus,

On 13.07.2023 00:59, Linus Walleij wrote:
> 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
> 
> 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.

I guess you refer to the locking flags. Probably your flash has the non
volatile block protection (BP) bits from the Status Register set, which
means the entire flash is write protected. The factory default for these
bits is 0/disabled on this flash so someone must have played with them.
The reason why one may want write protection set is to avoid inadvertent
writes during power-up.
One can control whether to disable the software write protection at boot
time with the MTD_SPI_NOR_SWP_ configs.
> 
> 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

The fasted mode is chosen after SFDP parsing, so you should use quad
reads if your controller also supports 4 I/O lines.
> 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.
> 
> Cc: stable@vger.kernel.org
> Suggested-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> 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@linaro.org
> ---
>  drivers/mtd/spi-nor/winbond.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 834d6ba5ce70..6c82e525c801 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -121,7 +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)

while here try, using INFO with INFO(0xef4018, 0, 0, 0), those
parameters shall be discovered at run-time, so we prepare to get rid of
explicitly setting them sooner or later.

> -		NO_SFDP_FLAGS(SECT_4K) },
> +		PARSE_SFDP
> +		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },

Looks good. Also I would like you to run a small sanity test, just to
make sure the flash works after your changes. You can do that with
mtd_debug utility, see an example on Miquel's commit message from:
https://lore.kernel.org/linux-mtd/d479489736ee193609816dc2003bd0fb@walle.cc/T/#m3550973e0884ec4a288d344fabd4a9c3b64af46e

Cheers,
ta