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

[<Tudor.Ambarus@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@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@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.

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.

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/