Re: [RFC PATCH 3/3] mtd: spi-nor: rework flash parameter initialization

[Tudor Ambarus]

On 6/1/26 3:52 PM, Michael Walle wrote:
> Rework how the flash parameters are initialized. It used to make a
> difference whether a flash is in the in-kernel database, whether it
> supports multi I/O and so on. Recently, that makes more and more
> problems because flash IDs are reused among legacy flashes (w/o SFDP
> support) and newer flashes [1,2,3,4].
> 
> Simplify the whole parameter initialization, by starting with the
> parameters we have in the in-kernel database and the try to parse SFDP
> (and let it override any settings).

You need to specify here as well about the potential problem of issuing
an unsupported command (rdsfdp), although practice showed us it's okay.

I can't remember who raised first the potential of issuing unsupported
commands. Was it macronix, sst?
> 
> [1] https://lore.kernel.org/linux-mtd/CAAyq3SYX9UPwhC_Ume_S2yxhQwimRvB=Y6O_+FFqokhNmw7jQg@xxxxxxxxxxxxxx/
> [2] https://lore.kernel.org/linux-mtd/DGB4745HRCFI.1DRYTHXURWZJO@xxxxxxxxxx/
> [3] https://lore.kernel.org/linux-mtd/DD10GE4EOCD7.CPTN7198QFUV@xxxxxxxxxx/
> [4] https://lore.kernel.org/linux-mtd/DD6SI06QNEE4.2YCRTWJHEAAQM@xxxxxxxxxx/
> 
> Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/core.c | 65 +++++++++++++-------------------------
>  drivers/mtd/spi-nor/core.h |  1 -
>  2 files changed, 22 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index fbf8c2d9c6b5..67e0377b606f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2868,11 +2868,10 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>  
>  /**
>   * spi_nor_no_sfdp_init_params() - Initialize the flash's parameters and
> - * settings based on nor->info->sfdp_flags. This method should be called only by
> - * flashes that do not define SFDP tables. If the flash supports SFDP but the
> - * information is wrong and the settings from this function can not be retrieved
> - * by parsing SFDP, one should instead use the fixup hooks and update the wrong
> - * bits.
> + * settings based on nor->info->sfdp_flags.
> + * If the flash supports SFDP but the information is wrong and the settings from
> + * this function can not be retrieved by parsing SFDP, one should instead use
> + * the fixup hooks and update the wrong bits.
>   * @nor:	pointer to a 'struct spi_nor'.
>   */
>  static void spi_nor_no_sfdp_init_params(struct spi_nor *nor)
> @@ -3053,43 +3052,25 @@ static int spi_nor_late_init_params(struct spi_nor *nor)
>  }
>  
>  /**
> - * spi_nor_sfdp_init_params_deprecated() - Deprecated way of initializing flash
> - * parameters and settings based on JESD216 SFDP standard.
> + * spi_nor_try_parse_sfdp() - Tries to parse flash parameters based
> + * on JESD216 SFDP standard.
>   * @nor:	pointer to a 'struct spi_nor'.
>   *
>   * The method has a roll-back mechanism: in case the SFDP parsing fails, the
> - * legacy flash parameters and settings will be restored.
> + * flash parameters and settings will be restored.
>   */
> -static void spi_nor_sfdp_init_params_deprecated(struct spi_nor *nor)
> +static int spi_nor_try_parse_sfdp(struct spi_nor *nor)
>  {
>  	struct spi_nor_flash_parameter sfdp_params;
> +	int ret;
>  
>  	memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
>  
> -	if (spi_nor_parse_sfdp(nor))
> +	ret = spi_nor_parse_sfdp(nor);
> +	if (ret)
>  		memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> -}
> -
> -/**
> - * spi_nor_init_params_deprecated() - Deprecated way of initializing flash
> - * parameters and settings.
> - * @nor:	pointer to a 'struct spi_nor'.
> - *
> - * The method assumes that flash doesn't support SFDP so it initializes flash
> - * parameters in spi_nor_no_sfdp_init_params() which later on can be overwritten
> - * when parsing SFDP, if supported.
> - */
> -static void spi_nor_init_params_deprecated(struct spi_nor *nor)
> -{
> -	spi_nor_no_sfdp_init_params(nor);
>  
> -	spi_nor_manufacturer_init_params(nor);
> -
> -	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
> -					SPI_NOR_QUAD_READ |
> -					SPI_NOR_OCTAL_READ |
> -					SPI_NOR_OCTAL_DTR_READ))
> -		spi_nor_sfdp_init_params_deprecated(nor);
> +	return ret;
>  }
>  
>  /**
> @@ -3152,7 +3133,8 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
>   *
>   * 1/ Default flash parameters initialization. The initializations are done
>   *    based on nor->info data:
> - *		spi_nor_info_init_params()
> + *		spi_nor_init_default_params()
> + *		spi_nor_no_sfdp_init_params()
>   *
>   * which can be overwritten by:
>   * 2/ Manufacturer flash parameters initialization. The initializations are
> @@ -3163,7 +3145,7 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
>   * which can be overwritten by:
>   * 3/ SFDP flash parameters initialization. JESD216 SFDP is a standard and
>   *    should be more accurate that the above.
> - *		spi_nor_parse_sfdp() or spi_nor_no_sfdp_init_params()
> + *		spi_nor_try_parse_sfdp()
>   *
>   *    Please note that there is a ->post_bfpt() fixup hook that can overwrite
>   *    the flash parameters and settings immediately after parsing the Basic
> @@ -3189,17 +3171,14 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  		return -ENOMEM;
>  
>  	spi_nor_init_default_params(nor);
> +	spi_nor_no_sfdp_init_params(nor);
> +	spi_nor_manufacturer_init_params(nor);
>  
> -	if (spi_nor_needs_sfdp(nor)) {
> -		ret = spi_nor_parse_sfdp(nor);
> -		if (ret) {
> -			dev_err(nor->dev, "BFPT parsing failed. Please consider using SPI_NOR_SKIP_SFDP when declaring the flash\n");
> -			return ret;
> -		}
> -	} else if (nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP) {
> -		spi_nor_no_sfdp_init_params(nor);
> -	} else {
> -		spi_nor_init_params_deprecated(nor);
> +	ret = spi_nor_try_parse_sfdp(nor);

I find it fine. I'm concerned about the inconsistent rollback
mechanism, but this is a preexisting problem.

Reviewed-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>


> +	if (ret && spi_nor_needs_sfdp(nor)) {
> +		dev_err(nor->dev,
> +			"SFDP parsing failed. You need to manually declare the flash parameters.\n");
> +		return ret;
>  	}
>  
>  	ret = spi_nor_late_init_params(nor);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 2ebc5c3caca1..3d7c31bff5cb 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -548,7 +548,6 @@ struct flash_info {
>  #define SPI_NOR_HAS_CMP			BIT(10)
>  
>  	u8 no_sfdp_flags;
> -#define SPI_NOR_SKIP_SFDP		BIT(0)

sashiko says you need to grep this further and remove all instances.

>  #define SECT_4K				BIT(1)
>  #define SPI_NOR_DUAL_READ		BIT(3)
>  #define SPI_NOR_QUAD_READ		BIT(4)