Re: [PATCH v2 27/35] mtd: spi-nor: core: Init flash params based on SFDP first for new flash additions

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

On 27/07/21 07:52AM, Tudor Ambarus wrote:
> Remove the spagetti way of initializing flash parameters and settings,
> at least for the new flash additions (for now). All flash entries should
> be converted to either use SPI_NOR_PARSE_SFDP or SPI_NOR_SKIP_SFDP.
> SPI_NOR_SKIP_SFDP should be set either when the SFDP tables are completely
> wrong and we can't parse relevant data, or when the SFDP tables are not
> defined at all, or when RDSFDP command is not supported by the flash.
> After all the flash entries will be converted to use these flags and after
> the default_init() hook will be removed, the
> spi_nor_init_params_deprecated() will be replaced by
> spi_nor_info_init_params(). The flash parameters and settings will be
> initialized either by parsing SFDP, or via the flash info flags.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 103 +++++++++++++++++++++++++------------
>  1 file changed, 70 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 9193317f897d..ef06a8d6abb8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2627,28 +2627,6 @@ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)
>  		nor->info->fixups->post_sfdp(nor);
>  }
>  
> -/**
> - * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
> - * 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.
> - */
> -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))
> -		return spi_nor_post_sfdp_fixups(nor);
> -
> -	memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> -	nor->addr_width = 0;
> -	nor->flags &= ~SNOR_F_4B_OPCODES;
> -}
> -
>  /**
>   * spi_nor_info_init_params() - Initialize the flash's parameters and settings
>   * based on nor->info data.
> @@ -2722,6 +2700,39 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>  	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
>  }
>  
> +/**
> + * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
> + * based on JESD216 SFDP standard.
> + * @nor:	pointer to a 'struct spi_nor'.

Missing documentation for treat_id_collisions.

Also, I assume this will go away when we get rid of 
spi_nor_init_params_deprecated(), correct?

> + *
> + * 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,
> +				     bool treat_id_collisions)
> +{
> +	struct spi_nor_flash_parameter sfdp_params;
> +
> +	memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
> +
> +	if (!spi_nor_parse_sfdp(nor))
> +		return spi_nor_post_sfdp_fixups(nor);
> +
> +	memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> +	nor->addr_width = 0;
> +	nor->flags &= ~SNOR_F_4B_OPCODES;
> +
> +	if (!treat_id_collisions)
> +		return;

No, this doesn't seem quite right. Why would you not want to treat ID 
collisions for flashes that use spi_nor_init_params_deprecated()? What 
makes this not possible for them?

Anyway, even if we don't want to treat ID collisions for those flashes, 
we can't just return here. In the previous code, nor->params is already 
initialized by spi_nor_info_init_params() so restoring that via memcpy() 
would make sense since it would restore the info-initialized state. Now 
it would be just 0, with no useful information in there. You are bound 
to run into errors somewhere down the line.

So I think you either need to make this function return an error and 
propagate it up the call chain or run spi_nor_info_init_params() for 
both type of flashes to make sure we do our best effort to initialize 
the flash.

> +	/*
> +	 * Fallback to flash info params init in case the SFDP parsing fails.
> +	 * Used to handle ID collisions between flashes that define the SFDP
> +	 * tables and flashes that don't.
> +	 */
> +	spi_nor_info_init_params(nor);
> +	spi_nor_manufacturer_init_params(nor);
> +}
> +
>  /**
>   * spi_nor_late_init_params() - Late initialization of default flash parameters.
>   * @nor:	pointer to a 'struct spi_nor'
> @@ -2797,7 +2808,9 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor)
>  }
>  
>  /**
> - * spi_nor_init_params() - Initialize the flash's parameters and settings.
> + * spi_nor_init_params_deprecated() - Initialize the flash's parameters and
> + * settings. The function is deprecated, it will be removed and replaced with
> + * spi_nor_info_init_params().
>   * @nor:	pointer to a 'struct spi_nor'.
>   *
>   * The flash parameters and settings are initialized based on a sequence of
> @@ -2821,11 +2834,40 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor)
>   *    Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can
>   *    overwrite the flash parameters and settings immediately after table
>   *    parsing.
> + */
> +static void spi_nor_init_params_deprecated(struct spi_nor *nor)
> +{
> +	spi_nor_info_init_params(nor);
> +	spi_nor_manufacturer_init_params(nor);
> +
> +	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
> +	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
> +		spi_nor_sfdp_init_params(nor, false);
> +}
> +
> +/**
> + * spi_nor_init_params() - Initialize the flash's parameters and settings.
> + * @nor:	pointer to a 'struct spi_nor'.
> + *
> + * The flash parameters and settings are initialized based on a sequence of
> + * calls that are ordered by priority:
> + *
> + * 1/ Default flash parameters initialization. The initializations are done
> + *    for all the flashes, regardless if the support SFDP or not.
> + *		spi_nor_init_default_params()
> + * which can be overwritten by:
>   *
> + * 2/ SFDP based or the deprecated way of initializing flash parameters.
> + * Ideally at this step the flash parameters init will be done either by
> + * parsing SFDP, where supported, or statically via flash_info flags.
> + *		spi_nor_sfdp_init_params() or spi_nor_init_params_deprecated()
>   * which can be overwritten by:
> - * 4/ Late flash parameters initialization, used to initialize flash
> + *
> + * 3/ Late flash parameters initialization, used to initialize flash
>   * parameters that are not declared in the JESD216 SFDP standard.
>   *		spi_nor_late_init_params()
> + *

Not really related to this patch, but we need to document the return 
value here as well.

>   */
>  static int spi_nor_init_params(struct spi_nor *nor)
>  {
> @@ -2835,15 +2877,10 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  
>  	spi_nor_init_default_params(nor);
>  
> -	spi_nor_info_init_params(nor);
> -
> -	spi_nor_manufacturer_init_params(nor);
> -
> -	if ((nor->info->flags & (SPI_NOR_PARSE_SFDP |
> -				 SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
> -	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
> -		spi_nor_sfdp_init_params(nor);
> +	if (nor->info->flags & SPI_NOR_PARSE_SFDP)
> +		spi_nor_sfdp_init_params(nor, true);
> +	else
> +		spi_nor_init_params_deprecated(nor);
>  
>  	spi_nor_late_init_params(nor);
>  
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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