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

Pratyush Yadav
2021-08-24T17:53:04Z

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.