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

[Michael Walle]

Am 2022-03-03 16:25, schrieb Tudor.Ambarus at microchip.com:
> On 3/3/22 16:51, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> 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
> 
> no, because just the first entry with the 0xc22016 ID will be hit, the
> second one will be ignored. We use a single flash entry for all the
> flashes that collide.

Not in addition but instead of yours, of course.

> PARSE_SFDP together with any of the:
> #define SECT_4K                         BIT(1)
> #define SECT_4K_PMC                     BIT(2)
> #define SPI_NOR_DUAL_READ               BIT(3)
> #define SPI_NOR_QUAD_READ               BIT(4)
> #define SPI_NOR_OCTAL_READ              BIT(5)
> #define SPI_NOR_OCTAL_DTR_READ          BIT(6)
> #define SPI_NOR_OCTAL_DTR_PP            BIT(7)
> 
> suggests that we'd like to differentiate between a flash that supports
> SFDP and can discover its params via SFDP (no_sfdp_flags will be 
> ignored),
> and a flash that doesn't support SFDP and it's forced to initialize the
> params via the no_sfdp_flags.

I get that, but what if I have a flash, which doesn't have 4k
sectors but ordinary 64k sectors (to stick to your example). In
general, what if I have a flash where none of the above flags
are set. You only call that function if there are any no_sfdp flags
set, but they are all optional, no? Who is setting the erase opcode
for flashes with that ID but without SFDP, then?

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

-- 
-michael