[PATCH v2 11/41] mtd: spi-nor: drop .parse_sfdp

[Tudor Ambarus]

On 8/22/23 08:09, Michael Walle wrote:
> Drop the size parameter to indicate we need to do SFDP, we can do that
> because it is guaranteed that the size will be set by SFDP and because
> PARSE_SFDP forced the SFDP parsing it must be overwritten.
> 
> Signed-off-by: Michael Walle <mwalle at kernel.org>
> ---
>  drivers/mtd/spi-nor/core.c       |  3 +--
>  drivers/mtd/spi-nor/core.h       | 23 ++++++++++++++++-------
>  drivers/mtd/spi-nor/eon.c        |  3 +--
>  drivers/mtd/spi-nor/gigadevice.c |  3 +--
>  drivers/mtd/spi-nor/issi.c       |  4 +---
>  drivers/mtd/spi-nor/macronix.c   |  1 -
>  drivers/mtd/spi-nor/spansion.c   | 12 ------------
>  drivers/mtd/spi-nor/sst.c        |  1 -
>  drivers/mtd/spi-nor/winbond.c    |  7 ++-----
>  9 files changed, 22 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 368851ff9f40..4ba1778eda4b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2017,7 +2017,6 @@ static const struct spi_nor_manufacturer *manufacturers[] = {
>  
>  static const struct flash_info spi_nor_generic_flash = {
>  	.name = "spi-nor-generic",
> -	.parse_sfdp = true,
>  };
>  
>  static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,
> @@ -3069,7 +3068,7 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  
>  	spi_nor_init_default_params(nor);
>  
> -	if (nor->info->parse_sfdp) {
> +	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");
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index fba3ea8536a5..9e6ad02b03b0 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -460,9 +460,6 @@ struct spi_nor_fixups {
>   * @page_size:      (optional) the flash's page size. Defaults to 256.
>   * @addr_nbytes:    number of address bytes to send.
>   *
> - * @parse_sfdp:     true when flash supports SFDP tables. The false value has no
> - *                  meaning. If one wants to skip the SFDP tables, one should
> - *                  instead use the SPI_NOR_SKIP_SFDP sfdp_flag.
>   * @flags:          flags that indicate support that is not defined by the
>   *                  JESD216 standard in its SFDP tables. Flag meanings:
>   *   SPI_NOR_HAS_LOCK:        flash supports lock/unlock via SR
> @@ -521,7 +518,6 @@ struct flash_info {
>  	u8 n_banks;
>  	u8 addr_nbytes;
>  
> -	bool parse_sfdp;
>  	u16 flags;
>  #define SPI_NOR_HAS_LOCK		BIT(0)
>  #define SPI_NOR_HAS_TB			BIT(1)
> @@ -598,9 +594,6 @@ struct flash_info {
>  			.n_regions = (_n_regions),			\
>  		},
>  
> -#define PARSE_SFDP							\
> -	.parse_sfdp = true,						\
> -
>  #define FLAGS(_flags)							\
>  		.flags = (_flags),					\
>  
> @@ -740,6 +733,22 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return container_of(mtd, struct spi_nor, mtd);
>  }
>  
> +/**
> + * spi_nor_needs_sfdp() - returns true if SFDP parsing is used for this flash.
> + *
> + * Return: true if SFDP parsing is needed
> + */
> +static inline bool spi_nor_needs_sfdp(const struct spi_nor *nor)
> +{
> +	/*
> +	 * The flash size is one property parsed by the SFDP. We use it as an
> +	 * indicator whether we need SFDP parsing for a particular flash. I.e.
> +	 * non-legacy flash entries in flash_info will have a size of zero iff
> +	 * SFDP should be used.
> +	 */
> +	return nor->info->size;
> +}
> +
>  #ifdef CONFIG_DEBUG_FS
>  void spi_nor_debugfs_register(struct spi_nor *nor);
>  void spi_nor_debugfs_shutdown(void);
> diff --git a/drivers/mtd/spi-nor/eon.c b/drivers/mtd/spi-nor/eon.c
> index 50a11053711f..434aaf155856 100644
> --- a/drivers/mtd/spi-nor/eon.c
> +++ b/drivers/mtd/spi-nor/eon.c
> @@ -25,8 +25,7 @@ static const struct flash_info eon_nor_parts[] = {
>  	{ "en25qh64",   INFO(0x1c7017, 0, 64 * 1024,  128)
>  		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ) },
>  	{ "en25qh128",  INFO(0x1c7018, 0, 64 * 1024,  256) },
> -	{ "en25qh256",  INFO(0x1c7019, 0, 64 * 1024,  512)
> -		PARSE_SFDP },
> +	{ "en25qh256",  INFO(0x1c7019, 0, 64 * 1024,  0) },
>  	{ "en25s64",	INFO(0x1c3817, 0, 64 * 1024,  128)
>  		NO_SFDP_FLAGS(SECT_4K) },
>  };
> diff --git a/drivers/mtd/spi-nor/gigadevice.c b/drivers/mtd/spi-nor/gigadevice.c
> index d57ddaf1525b..7cf142c75529 100644
> --- a/drivers/mtd/spi-nor/gigadevice.c
> +++ b/drivers/mtd/spi-nor/gigadevice.c
> @@ -62,8 +62,7 @@ static const struct flash_info gigadevice_nor_parts[] = {
>  		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>  		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
>  			      SPI_NOR_QUAD_READ) },
> -	{ "gd25q256", INFO(0xc84019, 0, 64 * 1024, 512)
> -		PARSE_SFDP
> +	{ "gd25q256", INFO(0xc84019, 0, 64 * 1024, 0)
>  		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_TB_SR_BIT6)
>  		FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
>  		.fixups = &gd25q256_fixups },

We might get in trouble here if SFDP advertises a wrong flash size. And
this is because in BP, instead of relying on the info->sector_size, we
now compute locally the sector size based on the size advertised by
SFDP. And if the SFDP flash size is wrong, we'll break BP for this
flash. I'm ok taking this risk, but please update the commit message and
inform readers about what they may hit, and what would be the fix. With
that feel free to add:

Reviewed-by: Tudor Ambarus <tudor.ambarus at linaro.org>