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

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

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.

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