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

[Michael Walle]

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

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

-michael

> [0] 
> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@xxxxxxxxxx/
> [1] 
> https://lore.kernel.org/linux-mtd/0525440a652854a2a575256cd07d3559@xxxxxxxx/