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

[Michael Walle <michael@walle.cc>]

Am 2021-08-16 20:54, schrieb Pratyush Yadav:
> 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?

Is there any difference between these? According to the description
in spi_nor_init_params() your case should go into post_sfdp(). Mh
or maybe not. It says "incomplete tables", one could interpret it
either way. We should clarify which fixup goes where, esp. this case
here where there should be a table describing it, but there isn't.

-michael

>>   */
>>  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
>> 

-- 
-michael

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