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

[Tudor.Ambarus at microchip.com]

On 8/16/21 9:54 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
>> Subject: [PATCH v2 08/35] mtd: spi-nor: core: Introduce the ate_init() hook
>                                                               ^^^^^^^^
> You ate the 'l' ;-)

right, thanks!

> 
> 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 at 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.

That's correct. Missing parameter tables are more suited for late_init(),
whereas post_sfdp is to tweak parameters that are wrongly defined in the tables.
spi_nor_post_sfdp_fixups() will be called only when SFDP tables are populated:
patch 15/35

Cheers,
ta
> 
> 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.
>