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

[Michael Walle <michael@walle.cc>]

Hi,

Am 2023-07-13 05:32, schrieb Tudor Ambarus:
> 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.

This is an entry matching various flash families from Winbond, see my
reply in v1. I'm not sure we should remove these as we could break the
older ones, which might or might not have SFDP tables. We don't know.

> 
>> -		NO_SFDP_FLAGS(SECT_4K) },

Thus, I'd also keep this one.

-michael

>> +		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