[PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Michael Walle]

Am 2022-03-03 15:41, schrieb Tudor.Ambarus at microchip.com:
> On 3/1/22 23:52, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-02-28 14:45, schrieb Tudor Ambarus:
>>> A typical differentiator between flashes whose ID collide is whether
>>> they
>>> support SFDP or not. For such a collision there will be a single
>>> flash_info entry where the developer should specify:
>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize 
>>> its
>>>    parameters by parsing the SFDP tables
>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the
>>>    flash parameters via the static no_sfdp_flags for the flash that
>>>    doesn't support SFDP.
>>> Use the logic the above to handle ID collisions between SFDP & 
>>> non-SFDP
>>> flashes.
>>> 
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>>> ---
>>>  drivers/mtd/spi-nor/core.c | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index fbf3278ba29a..aef00151c116 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor
>>> *nor)
>>>       if (nor->info->parse_sfdp) {
>>>               ret = spi_nor_parse_sfdp(nor);
>> 
>> Can we return -ENOENT here if sfdp isn't supported, so we
>> can differentiate between "no sfdp present" and other errors?
> 
> I'll take a look.
> 
>> 
>>>               if (ret) {
>>> -                     dev_err(nor->dev, "BFPT parsing failed. Please 
>>> consider using
>>> SPI_NOR_SKIP_SFDP when declaring the flash\n");
>>> -                     return ret;
>>> +                     /*
>>> +                      * Handle ID collisions between flashes that 
>>> support
>>> +                      * SFDP and flashes that don't. Initialize 
>>> parameters
>>> +                      * for the flash that doesn't support SFDP.
>>> +                      */
>>> +                     if (nor->info->no_sfdp_flags & 
>>> ~SPI_NOR_SKIP_SFDP) {
>> 
>> Shouldn't this be
>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))

Ahh I misread that for the "skip no sftp handling". But doesn't render
my point below invalid.

> No, because this will be true when no_sfdp_flags is zero, and the 
> method
> from below will be called. I would like to call it when any of the
> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag.

You should add a comment then.

> So when one
> declares a flash like:
> +      { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
> +             /* ID collision with mx25l3233f. */
> +             PARSE_SFDP
> +             NO_SFDP_FLAGS(SECT_4K)

But what about
+      { "differentflash",  INFO(0xc22016, 0, 64 * 1024,  64)
+             /* ID collision with mx25l3233f. */
+             PARSE_SFDP

Thats also valid, no? Why is having 4k sectors special? FWIW, the
function not only handles no_sfdp_flags but also erase related
things.

So in summary, the nosfdp handling is always called when parsing
fails (that is when there is no SFDP, not due to read errors or
similar). I don't see why that shouldn't be the case.

-michael

> First we will try to retrieve the flash params from SFDP. If SFDP 
> fails,
> then we'll init the flash based on the no_sfdp_flags. If SFDP succeeds
> the no_sfdp_flags is ignored.