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

Michael Walle
2021-09-09T21:41:50Z

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