Re: [PATCH v2 08/35] mtd: spi-nor: core: Introduce the ate_init() hook

Pratyush Yadav
2021-08-16T18:55:56Z

Hi Tudor,

> Subject: [PATCH v2 08/35] mtd: spi-nor: core: Introduce the ate_init() hook
^^^^^^^^
You ate the 'l' ;-)

On 27/07/21 07:51AM, Tudor Ambarus wrote:
> The goal is to get rid of the spaghetti way of initializing the flash
> parameters and settings. late_init() hook will be used to tweak various
> parameters that are not defined by the SFDP standard. Can be used by
> non SFDP compliant flashes in order to tweak flash parameters that
> are not/shouldn't be handled by the flash_info flags. Will replace the
> default_init() hook.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> drivers/mtd/spi-nor/core.c | 15 +++++++++++----
> drivers/mtd/spi-nor/core.h | 8 ++++++++
> 2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index d30c8f350dc1..15ccc9994215 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2667,11 +2667,18 @@ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)
> * spi_nor_late_init_params() - Late initialization of default flash parameters.
> * @nor: pointer to a 'struct spi_nor'
> *
> - * Used to set default flash parameters and settings when the ->default_init()
> - * hook or the SFDP parser let voids.
> + * Used to tweak various flash parameters that are not defined by the SFDP
> + * standard. Can be used by non SFDP compliant flashes in order to tweek flash
> + * parameters that are not handled by the flash_info flags.

What about flashes that do have the SFDP table, but not all of them? For
example, the Micron MT35XU512ABA flash currently uses the post_sfdp()
hook to populate 8D-8D-8D fast read settings, command extension type,
etc. These are supposed to be obtained from the xSPI Profile 1.0 table
(like we do for Spansion/Cypress S28HS512T). But the flash does not
populate this table. Should these go into the late_init() hook or the
post_sfdp() hook?

FWIW, I think it should go into late_init(). post_sfdp() should only be
used for correcting info obtained from the SFDP table. For populating
the info not present in SFDP at all, late_init() should be used.

Thoughts?

> */
> static void spi_nor_late_init_params(struct spi_nor *nor)
> {
> + if (nor->manufacturer && nor->manufacturer->late_init)
> + nor->manufacturer->late_init(nor);
> +
> + if (nor->info->late_init)
> + nor->info->late_init(nor);

Manufacturer late_init goes before flash late_init. Makes sense.

> +
> /*
> * NOR protection support. When locking_ops are not provided, we pick
> * the default ones.
> @@ -2713,8 +2720,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
> * wrong).
> * spi_nor_post_sfdp_fixups()
> *
> - * 5/ Late default flash parameters initialization, used when the
> - * ->default_init() hook or the SFDP parser do not set specific params.
> + * 5/ Late flash parameters initialization, used to initialize flash
> + * parameters that are not declared in the JESD216 SFDP standard.
> * spi_nor_late_init_params()
> */
> static int spi_nor_init_params(struct spi_nor *nor)
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index e9896cd60695..13d5c5edfd27 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -391,6 +391,11 @@ struct flash_info {
>
> /* Part specific fixup hooks. */
> const struct spi_nor_fixups *fixups;
> + /*
> + * Init flash parameters that are not declared in the JESD216 SFDP
> + * standard.
> + */
> + void (* const late_init)(struct spi_nor *nor);
> };
>
> /* Used when the "_ext_id" is two bytes at most */
> @@ -457,12 +462,15 @@ struct flash_info {
> * @parts: array of parts supported by this manufacturer
> * @nparts: number of entries in the parts array
> * @fixups: hooks called at various points in time during spi_nor_scan()
> + * @late_init: used to init flash parameters that are not declared in the
> + * JESD216 SFDP standard.
> */
> struct spi_nor_manufacturer {
> const char *name;
> const struct flash_info *parts;
> unsigned int nparts;
> const struct spi_nor_fixups *fixups;
> + void (* const late_init)(struct spi_nor *nor);

The patch looks good to me other than the comments above.

> };
>
> /**
> --
> 2.25.1
>

--
Regards,
Pratyush Yadav
Texas Instruments Inc.