Re: [PATCH v2 15/35] mtd: spi-nor: core: Call spi_nor_post_sfdp_fixups() only when SFDP is defined

Pratyush Yadav
2021-08-16T19:32:57Z

On 27/07/21 07:52AM, Tudor Ambarus wrote:
> spi_nor_post_sfdp_fixups() was called even when there were no SFDP
> tables defined and the function name was misleading.
>
> We introduced the late_init() hook which is used to tweak various
> parameters that could not be extracted by other means, i.e. when
> parameters are not defined in the JESD216 SFDP standard, or when
> the flash_info flags are incomplete.
>
> Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp()
> hook is as of now used just by s28hs512t, mt35xu512aba, and both
> support SFDP, there's no functional change with this patch.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++---------------------
> 1 file changed, 29 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 15ccc9994215..1f38fa8ab2fa 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2509,6 +2509,25 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
> nor->info->fixups->default_init(nor);
> }
>
> +/**
> + * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings
> + * after SFDP has been parsed.
> + * @nor: pointer to a 'struct spi_nor'
> + *
> + * Typically used to tweak various parameters that could not be extracted by
> + * other means (i.e. when information provided by the SFDP tables are
> + * incomplete or wrong).

Do we want to keep the "incomplete" here? Wouldn't incomplete
information (like missing parameter tables) be more suited for
late_init()?

> + */
> +static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)
> +{
> + if (nor->manufacturer && nor->manufacturer->fixups &&
> + nor->manufacturer->fixups->post_sfdp)
> + nor->manufacturer->fixups->post_sfdp(nor);
> +
> + if (nor->info->fixups && nor->info->fixups->post_sfdp)
> + nor->info->fixups->post_sfdp(nor);
> +}
> +
> /**
> * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
> * based on JESD216 SFDP standard.
> @@ -2523,11 +2542,12 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor)
>
> 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;
> - }
> + if (!spi_nor_parse_sfdp(nor))
> + return spi_nor_post_sfdp_fixups(nor);

Huh. I didn't know you could do return foo() in a void function if foo()
is also void. Dunno how I feel about this though. It definitely confused
me for a bit.

> +
> + memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> + nor->addr_width = 0;
> + nor->flags &= ~SNOR_F_4B_OPCODES;

I feel like the new flow makes these 3 lines more confusing. Earlier,
these were called under if (spi_nor_parse_sfdp()) so it was a bit easier
to make the connection that these are undoing the changes performed by
that function. Now it is a little harder to spot. I think a comment is
in order.

> }
>
> /**
> @@ -2643,26 +2663,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
> spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
> }
>
> -/**
> - * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings
> - * after SFDP has been parsed (is also called for SPI NORs that do not
> - * support RDSFDP).
> - * @nor: pointer to a 'struct spi_nor'
> - *
> - * Typically used to tweak various parameters that could not be extracted by
> - * other means (i.e. when information provided by the SFDP/flash_info tables
> - * are incomplete or wrong).
> - */
> -static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)
> -{
> - if (nor->manufacturer && nor->manufacturer->fixups &&
> - nor->manufacturer->fixups->post_sfdp)
> - nor->manufacturer->fixups->post_sfdp(nor);
> -
> - if (nor->info->fixups && nor->info->fixups->post_sfdp)
> - nor->info->fixups->post_sfdp(nor);
> -}
> -
> /**
> * spi_nor_late_init_params() - Late initialization of default flash parameters.
> * @nor: pointer to a 'struct spi_nor'
> @@ -2709,18 +2709,12 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
> * should be more accurate that the above.
> * spi_nor_sfdp_init_params()
> *
> - * Please note that there is a ->post_bfpt() fixup hook that can overwrite
> - * the flash parameters and settings immediately after parsing the Basic
> - * Flash Parameter Table.
> + * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can
> + * overwrite the flash parameters and settings immediately after table
> + * parsing.
> *
> * which can be overwritten by:
> - * 4/ Post SFDP flash parameters initialization. Used to tweak various
> - * parameters that could not be extracted by other means (i.e. when
> - * information provided by the SFDP/flash_info tables are incomplete or
> - * wrong).
> - * spi_nor_post_sfdp_fixups()
> - *
> - * 5/ Late flash parameters initialization, used to initialize flash
> + * 4/ Late flash parameters initialization, used to initialize flash
> * parameters that are not declared in the JESD216 SFDP standard.
> * spi_nor_late_init_params()
> */
> @@ -2740,8 +2734,6 @@ static int spi_nor_init_params(struct spi_nor *nor)
> !(nor->info->flags & SPI_NOR_SKIP_SFDP))
> spi_nor_sfdp_init_params(nor);
>
> - spi_nor_post_sfdp_fixups(nor);
> -
> spi_nor_late_init_params(nor);
>
> return 0;
> --
> 2.25.1
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.