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

[Tudor Ambarus]

On 7/18/23 07:25, Michael Walle wrote:
> Hi,
> 
>>>> 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.
> 
> Wait a minute, now I'm the more conservative one of the both of
> us? (:

;)

> 
> Jokes aside, basically you are saying that if there are two flashes
> with the same id, one supports JEDEC one doesn't, we can brake the
> latter one.

I say I don't want to suffocate the code based on assumptions and I'm ready
to take the risk and break some presumably old flashes that don't support
SFDP. If we know for sure that we break old variants of this flash ID,
then we have to rework the core now. Otherwise I'll rework it when a bug
is reported.

> 
>>>>> -        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.
> 
> Mhh, I should have been more curious to why Linus needed the PARSE_SFDP
> flag in the first place. Taking a closer look, that is because in the
> legacy behavior, the SFDP is only read if the chip supports dual or
> quad read (whatever the rationale was for that).

dual or quad reads are params that could be discovered in the first version of
SFDP, that why they associated them with reading SFDP. Not great, but it
worked.

> 
> Also, if PARSE_SFDP is set, none of the no_sfdp_flags are ever handled.
> If the chip doesn't support the SFDP is will just fail probing.
> 
> As I'm reading the code right now, for the new behavior
> it is either
>  * expect the flash supports SFDP, if not, probe fails
>  * expect the flash to don't support SFDP, no SFDP is ever read at all
> 

sort of. It's more elaborate than that. Check spi_nor_init_params().
> Shouldn't we handle the third case in the new behavior, too:
>  * start with the static data we have and try reading SFDP to amend/correct it.

This case is already supported and it's the old way of initializing flash
parameters. Check spi_nor_init_params_deprecated().

I don't want to do that for the SFDP capable flashes for now, otherwise
we'll have again parameters initialized all over the place, which results
in ugly hard to read code. You have the fixup hooks if you need to amend SFDP
data. And since the first table of SFDP is mandatory (BFPT), if you set PARSE_SFDP
and get an error, then you shouldn't have set PARSE_SFDP in the first place.
Optional SFDP tables return void and we have a rollback mechanism for the parameters
set in those optional tables in case of errors.

> 
> If not, will you accept breakage for future flashes, too? Looking at winbond.c
> for example, I guess all chips with 0xef40.. 0xef50.. and 0xef60.. supports
> SFDP nowadays and most of them only have SECT_4K set.

I will. Note that you have to actually have a physical flash to do changes,
I don't queue untested patches.
> 
>> If both of you agree, I'll amend Linus's v4 patch when applying.
> 
> So it would be the first chip without flags at all? Then we could
> drop the entry entirely :) And if we do this, then we should also

No, you have the locking flags that can't be discovered by parsing SFDP,
thus you need to define a flash entry for it.

> drop all the other entries for the newer winbond flashes.

If you can test it, and there's no dedicated compatible for that flash,
I'm ok to drop them.
> 
> If you decide to break older flashes, then I'd prefer to also drop
> the n_sectors and sector_size, i.e. INFO(0xef...., 0, 0, 0).
> 

Isn't v4 already doing this? I'll amend it if not. Waiting for ack from
both you and Linus.

Cheers,
ta