Definition for flash w25q128 is wrong

[Michael Walle]

On Mon Jun 10, 2024 at 9:44 AM CEST, Esben Haabendal wrote:
> e9hack <e9hack at gmail.com> writes:
>
> > Am 07.06.2024 um 13:44 schrieb Esben Haabendal:
> >> "Michael Walle" <michael at walle.cc> writes:
> >> 
> >>> On Thu Jun 6, 2024 at 9:50 PM CEST, e9hack wrote:
> >>>> I'm using a TP-LINK WDR3600 with a bigger flash. Since some time the
> >>>> router hangs in an endless boot loop. I see the following message:
> >>>>
> >>>> ...
> >>>> [    0.402716] spi-nor spi0.0: BFPT parsing failed. Please consider
> >>>> using SPI_NOR_SKIP_SFDP when declaring the flash
> >>>
> >>> Looks like your flash doesn't support SFDP. Relevant threads are:
> >>> https://lore.kernel.org/r/d99d87e7-47ba-d6fe-735f-16de2a2ec280@linaro.org/
> >>> https://lore.kernel.org/r/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/
> >>>
> >>>> [    0.413217] spi-nor: probe of spi0.0 failed with error -22
> >>>> ...
> >>>> [    0.926180] /dev/root: Can't open blockdev
> >>>> [    0.930427] VFS: Cannot open root device "(null)" or
> >>>> unknown-block(0,0): error -6
> >>>> [    0.938037] Please append a correct "root=" boot option; here are
> >>>> the available partitions:
> >>>> [    0.946520] Kernel panic - not syncing: VFS: Unable to mount root
> >>>> fs on unknown-block(0,0)
> >>>> [    0.954914] Rebooting in 1 seconds..
> >>>>
> >>>> It looks like the definition for the flash is wrong:
> >>>>
> >>>> --- a/drivers/mtd/spi-nor/winbond.
> >>>> c     2024-03-15 19:27:50.000000000 +0100
> >>>> +++ b/drivers/mtd/spi-nor/winbond.c     2024-04-01 05:59:17.166780732 +0200
> >>>> @@ -120,8 +120,8 @@ static const struct flash_info winbond_n
> >>>>                  NO_SFDP_FLAGS(SECT_4K) },
> >>>>          { "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16)
> >>>>                  NO_SFDP_FLAGS(SECT_4K) },
> >>>> -       { "w25q128", INFO(0xef4018, 0, 0, 0)
> >>>> -               PARSE_SFDP
> >>>> +       { "w25q128", INFO(0xef4018, 0, 64 * 1024, 256)
> >>>> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> >>>>                  FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> >>>>          { "w25q256", INFO(0xef4019, 0, 64 * 1024, 512)
> >>>>                  NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> >>>>
> >>>> With these changes, the flash will be detected properly. The chip is
> >>>> marked with:
> >>>> The chip (SOIC8) is marked with:
> >>>> winbond
> >>>> 25Q128FVSG
> >>>> 1327
> >>>>
> >>>> I use another TP-LINK router. This is an Archer C7 v2. It has the same
> >>>> flash chip with date code 1528. This router doesn't have this issue.
> >>>
> >>> Looks like some of these flashes support SFDP and some don't :/
> >
> > Related to the spec
> > 	https://www.winbond.com/hq/support/documentation/levelOne.jsp?__locale=en&DocNo=DA00-W25Q128FV
> > chapter 8.2.30, older flashes don't support SFDP. Newer flashes may support SFDP. From my two versions, one doesn't support SFDP, the other does support it.
>
> Ok. That is clear. Sort of. No concrete information on which flashes
> exactly supports SFDP. Thank you Winbond. Looks like we really need the
> SFDP with fallback to handle these flashes according to their
> "specification".

Shoot, I forgot to add you on CC:
https://lore.kernel.org/r/20240610074809.2180535-1-mwalle@kernel.org/

That's for the quick fix (and the backports). And yes we really need
that case (4) IMHO.

-michael

> /Esben
>
> >> Which means that for those (old) flashes without SFDP was broken by
> >> 83e824a4a595 ("mtd: spi-nor: Correct flags for Winbond w25q128").
> >> During patch review, the following was said:
> >> 
> >>>>> 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.
> >> I guess it is not an assumption anymore. There are flashes out there
> >> with this jedec id which does not have SFDP tables.
> >> So while parsing SFDP is good, keeping a fallback to some known "good"
> >> values are needed. Something similar to what we are discussing in
> >> https://lore.kernel.org/all/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/