Definition for flash w25q128 is wrong

[Esben Haabendal]

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

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