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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

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()

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

> (1) SFDP only [size==0]

1/ static + SFDP

spi_nor_init_default_params(nor);
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);

> 
> (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);>
> (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().

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

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

Your patches do not apply on latest SPI NOR, I tried locally. Can you
point me to a branch of yours where I can see them? No need to resend.

Thanks,
ta