| Message ID | 20210727045222.905056-9-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
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 >
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 >>
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@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. >
On 9/10/21 12:40 AM, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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 I thought I made it clearer in 15/35. I will revisit the documentation updates and try to be more precise. Maybe it is worth to reorder the patches to, I'll do it if there will not be huge conflicts. Cheers, ta
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. */ 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); + /* * 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 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(-)