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

[Tudor.Ambarus at microchip.com]

On 3/3/22 17:42, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?

this use case is not handled in this proposal, indeed. I'm not sure
if there are such flashes though. Can't think of an example. We search
for a flash that operates only in 1-1-1 and can't do 4k erase.
Is that a NOR? But if you feel we should care about this case too,
I'll address it, probably I will be forced to introduce a new flag or
a bool, meh.

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