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

[Tudor Ambarus]

On 13.07.2023 10:01, Michael Walle wrote:
> 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@xxxxxxxxxxxxxxx
>>> Suggested-by: Michael Walle <michael@xxxxxxxx>
>>> Signed-off-by: Linus Walleij <linus.walleij@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 | 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.

I'd take the risk and break the older ones if there are some that don't
define SFDP indeed, just to handle the conflict properly. We can't
encourage code based on assumptions otherwise we'll get back to the
knotted spi-nor code that we tried to untie in the last years.

> 
>>
>>> -        NO_SFDP_FLAGS(SECT_4K) },
> 
> Thus, I'd also keep this one.
> 

Keeping this one does not have the effect that you want as SECT_4K is
used in spi_nor_no_sfdp_init_params() which is not called when
PARSE_SFDP is set, which makes perfectly sense. Let's drop this and if
bugs will be reported, I commit I'll fix them in the same release cycle.

If both of you agree, I'll amend Linus's v4 patch when applying.

Cheers,
ta