Re: [PATCH v5 09/14] mtd: spi-nor: core: Init all flash parameters based on SFDP where possible

[Pratyush Yadav <p.yadav@ti.com>]

On 03/12/21 04:22PM, Tudor Ambarus wrote:
> New flash additions that support SFDP should be declared with
> PARSE_SFDP and with all the other flags that are not SFDP
> discoverable.
> 
> Keep the old way of initializing the flash, until all the flashes
> are converted to use either PARSE_SFDP or SPI_NOR_SKIP_SFDP.
> 
> Flashes that declare PARSE_SFDP do not have a roll-back mechanism
> because if spi_nor_parse_sfdp() returns an error it means that either
> BFPT is not supported, thus SFDP is not supported and the user didn't
> correctly declared the flash_info entry, or some memalloc failed.
> Either way we should return an error. The rest of the SFDP tables are
> optional, if one of the optional SFDP tables fails, we just continue.
> We would like to get rid of the default_init() hook, so the
> spi_nor_manufacturer_init_params() is not called in the new sequnce
> of flash initialization that is based solely on SFDP.
> 
> Split spi_nor_info_init_params() in spi_nor_init_default_params()
> and spi_nor_no_sfdp_init_params(). spi_nor_init_default_params() is
> called for all the flashes regardless if they support SFDP or not.
> spi_nor_no_sfdp_init_params() is called just for the flashes that
> do not define SFDP and initializes parameters and setting solely
> based on flash_info data.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 194 ++++++++++++++++++++++---------------
>  1 file changed, 116 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 86bbd1ca22fc..d5eaf9a705ae 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2509,74 +2509,21 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>  }
>  
>  /**
> - * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
> - * based on JESD216 SFDP standard.
> + * 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

s/fixup hooks/fixup flags/ ?

> + * bits.
>   * @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.
>   */
> -static void spi_nor_sfdp_init_params(struct spi_nor *nor)
> -{
> -	struct spi_nor_flash_parameter sfdp_params;
> -
> -	memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
> -
> -	if (spi_nor_parse_sfdp(nor)) {
> -		memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> -		nor->addr_width = 0;
> -		nor->flags &= ~SNOR_F_4B_OPCODES;
> -	}
> -}
[...]
> +
> +/**
> + * 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 ((SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_OCTAL_READ |
> +	     SPI_NOR_OCTAL_DTR_READ) &&
> +	    !(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))
> +		spi_nor_sfdp_init_params_deprecated(nor);
> +}
> +
[...]
> @@ -2773,24 +2807,28 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>   * parameters that are not declared in the JESD216 SFDP standard, or where SFDP
>   * tables are not defined at all.
>   *		spi_nor_late_init_params()
> + *
> + * Return: 0 on success, -errno otherwise.
>   */
>  static int spi_nor_init_params(struct spi_nor *nor)
>  {
> +	int ret;
> +
>  	nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params), GFP_KERNEL);
>  	if (!nor->params)
>  		return -ENOMEM;
>  
> -	spi_nor_info_init_params(nor);
> -
> -	spi_nor_manufacturer_init_params(nor);
> +	spi_nor_init_default_params(nor);
>  
> -	if ((nor->info->parse_sfdp ||
> -	     (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
> -					  SPI_NOR_QUAD_READ |
> -					  SPI_NOR_OCTAL_READ |
> -					  SPI_NOR_OCTAL_DTR_READ))) &&
> -	    !(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))
> -		spi_nor_sfdp_init_params(nor);
> +	if (nor->info->parse_sfdp) {
> +		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 {
> +		spi_nor_init_params_deprecated(nor);

Hmm, a flash without SFDP would always go via the "deprecated" path even 
if it is a new entry. It makes no difference in practice, but is just 
slightly confusing logically. Can we make this a bit clearer? How about 
this:

  if (nor->info->parse_sfdp) {
	...
  } else if (nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP) {
	spi_nor_no_sfdp_init_params();
	spi_nor_manufacturer_init_params();
  } else {
	spi_nor_init_params_deprecated();
  }

> +	}
>  
>  	spi_nor_late_init_params(nor);
>  
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel