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

From: <Tudor.Ambarus@microchip.com>
To: <michael@walle.cc>
Cc: sr@denx.de, vigneshr@ti.com, jaimeliao@mxic.com.tw,
	richard@nod.at, esben@geanix.com, linux@rasmusvillemoes.dk,
	knaerzche@gmail.com, linux-mtd@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org, macromorgan@hotmail.com,
	miquel.raynal@bootlin.com, heiko.thiery@gmail.com,
	zhengxunli@mxic.com.tw, figgyc@figgyc.uk, p.yadav@ti.com,
	mail@david-bauer.net, code@reto-schneider.ch
Subject: Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes
Date: Thu, 3 Mar 2022 14:41:28 +0000	[thread overview]
Message-ID: <72679fc7-42e4-d7b8-1fb9-463f225217fc@microchip.com> (raw)
In-Reply-To: <328762851786f1158505f9159ba3e5bc@walle.cc>

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel