Re: [PATCH] mtd: spi-nor: winbond: fix w25q128 regression

[Pratyush Yadav <pratyush@kernel.org>]

On Thu, Jun 20 2024, Michael Walle wrote:

>>>> > Commit 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128")
>>>> That commit did:
>>>> -       { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
>>>> -               NO_SFDP_FLAGS(SECT_4K) },
>>>> +       { "w25q128", INFO(0xef4018, 0, 0, 0)
>>>> +               PARSE_SFDP
>>>> +               FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>>>> 
>> [...]
>>>> > diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
>>>> > index ca67bf2c46c3..6b6dec6f8faf 100644
>>>> > --- a/drivers/mtd/spi-nor/winbond.c
>>>> > +++ b/drivers/mtd/spi-nor/winbond.c
>>>> > @@ -105,7 +105,9 @@ static const struct flash_info winbond_nor_parts[] = {
>>>> >  	}, {
>>>> >  		.id = SNOR_ID(0xef, 0x40, 0x18),
>>>> >  		.name = "w25q128",
>>>> > +		.size = SZ_16M,
>>>> >  		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB,
>>>> > +		.no_sfdp_flags = SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
>>>> and here you add dual and quad to trigger SFDP parsing I guess. All fine
>>>> if the old flash supports dual and quad read. But please update the
>>>> commit message describing the intention. With that ACK. Would be good to
>>>> have this merged soon.
>>> Right. It's not because it will trigger the SFDP parsing, but
>>> because that what was tested by Esben. We're lucky that this will
>>> trigger the SFDP parsing ;) I'll explain that in more detail and add
>>> a Link: to the bug report mail.
>> Should we treat this flash similar to the Macronix ones Esben sent out
>> patches for [0]? It seems that there are some old parts without SFDP
>> support and new ones with SFDP support. From your comment in [1]:
>> 
>>> 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.
>> Since the entry matches multiple families, do _all_ of them support dual
>> and quad read? If not, attempting to enable dual or quad reads on them
>> can cause problems.
>
> I rely on the information Helmut provided. Also the w25q64 and the w25q256
> both have these flags set. So I'd say it's less likely the 128 doesn't
> support it.

Okay, fair enough.

>
>> Also, for parts that _do_ have SFDP available, won't it be better to use
>> the information in SFDP instead of our hard-coded ones anyway? Using
>> SPI_NOR_TRY_SFDP here would let us do that.
>
> Sure, but this is about fixing the referenced commit. A later patch will
> then move that to TRY_SFDP. We can't fix this regression by introducing
> new code IMHO. This seems to be the easiest fix.

New code will make it harder to backport to stable kernels. Beyond that,
I don't see why we can't fix a regression with new code.

Here's why I suggested this: before 83e824a4a595 ("mtd: spi-nor: Correct
flags for Winbond w25q128"), all flashes with this ID got only the
SECT_4K flag -- and thus only single SPI mode. After that commit, all
flashes with this ID got their settings configured via SFDP. Using the
TRY_SFDP approach allows both of those configurations to co-exist. Old
ones still use the old configuration, new ones get to use SFDP.

Now we add a different configuration that adds dual and quad reads to
these old flashes. As mentioned above, this is unlikely to cause
problems, but a new configuration regardless. So _in principle_ I think
TRY_SFDP would be the best balance.

But I get your point -- since both w25q64 and w25q256 have these flags,
it is likely someone just never bothered updating w25q128. So this patch
LGTM. I'll apply it once you send a new version with an updated commit
message.

>
> -michael
>
>> [0]
>> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/
>> [1]
>> https://lore.kernel.org/linux-mtd/0525440a652854a2a575256cd07d3559@walle.cc/
>

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/