Re: [PATCH v3 00/15] mtd: spi-nor: macronix: workaround for device id re-use

[Esben Haabendal <esben@geanix.com>]

Tudor Ambarus <tudor.ambarus@linaro.org> writes:

> Hiya, Esben,
>
> On 7/11/24 2:00 PM, Esben Haabendal wrote:
>> Following up to various discussions, this series have now been
>> modified so that it gets rid of the old deprecated approach
>> for detecting when to do optional SFDP parsing.
>> 
>> Before these changes, spi-nor flashes were handled in 4 different
>> ways:
>> 
>
> I'm adding a bit of extra context on each point you made. All your
> points contain static init, you missed spi_nor_init_default_params()

I might be even more confused now :) Let me try to understand...

> There's a 0/ case where we have indeed just SFDP initialized flashes,
> and that is the generic flash driver.
>
> 0/ SFDP only, generic flash driver

Ok. So this is when spi_nor_detect() falls back to
spi_nor_generic_flash, right?

>> (1) SFDP only [size==0]
>
> 1/ static + SFDP

So we have found a matching flash_info, and it has size==0. Right?

> spi_nor_init_default_params(nor);

I guess we can assume that flash_info in this case will always have
something set (other than .name and .size=0)?

> spi_nor_parse_sfdp();

>> (2a) static config only [size!=0 && no_sfdp_flags & SPI_NOR_SKIP_SFDP]
>
> 2a/ is
> spi_nor_init_default_params(nor);
> spi_nor_no_sfdp_init_params(nor);

Got it.

>> (2b) static config only
>>        [size!=0 &&
>>         !(no_sfdp_flags & (SPI_NOR_SKIP_SFDP | 
>>            SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]
>
> 2b/ is
> spi_nor_init_default_params(nor);
> spi_nor_init_params_deprecated(nor); //where parse SFDP is not called
> 	spi_nor_no_sfdp_init_params(nor);
> 	spi_nor_manufacturer_init_params(nor);>

Got it.

>> (3) SFDP with fallback to static config
>>        [size!=0 &&
>>         !(no_sfdp_flags & SPI_NOR_SKIP_SFDP) &&
>>         (no_sfdp_flags & SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>            SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ))]
>> 
>
> 3/ is 2b/ with parse SFDP called and rollback mechanism
> spi_nor_init_default_params(nor);
> spi_nor_init_params_deprecated(nor);
> 	spi_nor_no_sfdp_init_params(nor);
> 	spi_nor_manufacturer_init_params(nor);
> 	spi_nor_sfdp_init_params_deprecated(nor);
>
> All cases from above are followed by a call to spi_nor_late_init_params().

Got it.

Should I move your annotations to the cover letter? If I find it
helpful, I think it might also be helpful to somebody else :)

>> Cases (2a) and (2b) have been handled slightly different, with
>> manufacturer and flash_info ->default_init() hooks being called in
>> case (2b) but not in case (2a).
>
> default_init() was a mistake and we shall remove it and replace it with
> late_init(). The challenge is to do that without affecting backwards
> compatibility. But let's move this aside for the moment
>> 
>> With this series, that is changed to this simpler approach instead:
>> 
>> (1) SFDP only [size==0]
>> 
>> (2) static config only
>>       [size!=0 && !(no_sfdp_flags & SPI_NOR_TRY_SFDP)]
>> 
>> (3) SFDP with fallback to static config
>>       [size!=0 && (no_sfdp_flags & SPI_NOR_TRY_SFDP)]
>> 
>> Existing struct flash_info entries are modified, so that all those
>> that was case (2a) or (2b) are now case (2), and those that were (1)
>> and (3) are still (1) and (3).
>
> We indeed want 2a/ and 2b/ to be squashed, ideally by removing the
> default_init() hook.

But you think we should not remove default_init() hook as part of this
series?

> And if we really want SFDP-only init, we shall not call
> spi_nor_init_default_params() in this case.

So move spi_nor_init_default_params() into the if (spi_nor_needs_sfdp())
else section?
I have pushed a fixup commit doing this to my branch. Should I amend
patch 1/15 with it?

/Esben