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

Tudor Ambarus [off-list ref] writes:

Hiya, Esben,

On 7/11/24 2:00 PM, Esben Haabendal wrote:

quoted

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?

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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