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

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

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

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.

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