| Message ID | 20210727045222.905056-16-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions and clean params init | expand |
On 27/07/21 07:52AM, Tudor Ambarus wrote: > spi_nor_post_sfdp_fixups() was called even when there were no SFDP > tables defined and the function name was misleading. > > We introduced the late_init() hook which is used to tweak various > parameters that could not be extracted by other means, i.e. when > parameters are not defined in the JESD216 SFDP standard, or when > the flash_info flags are incomplete. > > Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp() > hook is as of now used just by s28hs512t, mt35xu512aba, and both > support SFDP, there's no functional change with this patch. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++--------------------- > 1 file changed, 29 insertions(+), 37 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 15ccc9994215..1f38fa8ab2fa 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2509,6 +2509,25 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor) > nor->info->fixups->default_init(nor); > } > > +/** > + * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings > + * after SFDP has been parsed. > + * @nor: pointer to a 'struct spi_nor' > + * > + * Typically used to tweak various parameters that could not be extracted by > + * other means (i.e. when information provided by the SFDP tables are > + * incomplete or wrong). Do we want to keep the "incomplete" here? Wouldn't incomplete information (like missing parameter tables) be more suited for late_init()? > + */ > +static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) > +{ > + if (nor->manufacturer && nor->manufacturer->fixups && > + nor->manufacturer->fixups->post_sfdp) > + nor->manufacturer->fixups->post_sfdp(nor); > + > + if (nor->info->fixups && nor->info->fixups->post_sfdp) > + nor->info->fixups->post_sfdp(nor); > +} > + > /** > * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings > * based on JESD216 SFDP standard. > @@ -2523,11 +2542,12 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor) > > memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); > > - if (spi_nor_parse_sfdp(nor)) { > - memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); > - nor->addr_width = 0; > - nor->flags &= ~SNOR_F_4B_OPCODES; > - } > + if (!spi_nor_parse_sfdp(nor)) > + return spi_nor_post_sfdp_fixups(nor); Huh. I didn't know you could do return foo() in a void function if foo() is also void. Dunno how I feel about this though. It definitely confused me for a bit. > + > + memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); > + nor->addr_width = 0; > + nor->flags &= ~SNOR_F_4B_OPCODES; I feel like the new flow makes these 3 lines more confusing. Earlier, these were called under if (spi_nor_parse_sfdp()) so it was a bit easier to make the connection that these are undoing the changes performed by that function. Now it is a little harder to spot. I think a comment is in order. > } > > /** > @@ -2643,26 +2663,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > spi_nor_init_uniform_erase_map(map, erase_mask, params->size); > } > > -/** > - * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings > - * after SFDP has been parsed (is also called for SPI NORs that do not > - * support RDSFDP). > - * @nor: pointer to a 'struct spi_nor' > - * > - * Typically used to tweak various parameters that could not be extracted by > - * other means (i.e. when information provided by the SFDP/flash_info tables > - * are incomplete or wrong). > - */ > -static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) > -{ > - if (nor->manufacturer && nor->manufacturer->fixups && > - nor->manufacturer->fixups->post_sfdp) > - nor->manufacturer->fixups->post_sfdp(nor); > - > - if (nor->info->fixups && nor->info->fixups->post_sfdp) > - nor->info->fixups->post_sfdp(nor); > -} > - > /** > * spi_nor_late_init_params() - Late initialization of default flash parameters. > * @nor: pointer to a 'struct spi_nor' > @@ -2709,18 +2709,12 @@ static void spi_nor_late_init_params(struct spi_nor *nor) > * should be more accurate that the above. > * spi_nor_sfdp_init_params() > * > - * Please note that there is a ->post_bfpt() fixup hook that can overwrite > - * the flash parameters and settings immediately after parsing the Basic > - * Flash Parameter Table. > + * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can > + * overwrite the flash parameters and settings immediately after table > + * parsing. > * > * which can be overwritten by: > - * 4/ Post SFDP flash parameters initialization. Used to tweak various > - * parameters that could not be extracted by other means (i.e. when > - * information provided by the SFDP/flash_info tables are incomplete or > - * wrong). > - * spi_nor_post_sfdp_fixups() > - * > - * 5/ Late flash parameters initialization, used to initialize flash > + * 4/ Late flash parameters initialization, used to initialize flash > * parameters that are not declared in the JESD216 SFDP standard. > * spi_nor_late_init_params() > */ > @@ -2740,8 +2734,6 @@ static int spi_nor_init_params(struct spi_nor *nor) > !(nor->info->flags & SPI_NOR_SKIP_SFDP)) > spi_nor_sfdp_init_params(nor); > > - spi_nor_post_sfdp_fixups(nor); > - > spi_nor_late_init_params(nor); > > return 0; > -- > 2.25.1 >
On 8/16/21 10:31 PM, Pratyush Yadav wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 27/07/21 07:52AM, Tudor Ambarus wrote: >> spi_nor_post_sfdp_fixups() was called even when there were no SFDP >> tables defined and the function name was misleading. >> >> We introduced the late_init() hook which is used to tweak various >> parameters that could not be extracted by other means, i.e. when >> parameters are not defined in the JESD216 SFDP standard, or when >> the flash_info flags are incomplete. >> >> Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp() >> hook is as of now used just by s28hs512t, mt35xu512aba, and both >> support SFDP, there's no functional change with this patch. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++--------------------- >> 1 file changed, 29 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 15ccc9994215..1f38fa8ab2fa 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2509,6 +2509,25 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor) >> nor->info->fixups->default_init(nor); >> } >> >> +/** >> + * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings >> + * after SFDP has been parsed. >> + * @nor: pointer to a 'struct spi_nor' >> + * >> + * Typically used to tweak various parameters that could not be extracted by >> + * other means (i.e. when information provided by the SFDP tables are >> + * incomplete or wrong). > > Do we want to keep the "incomplete" here? Wouldn't incomplete > information (like missing parameter tables) be more suited for > late_init()? > will update, thanks >> + */ >> +static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) >> +{ >> + if (nor->manufacturer && nor->manufacturer->fixups && >> + nor->manufacturer->fixups->post_sfdp) >> + nor->manufacturer->fixups->post_sfdp(nor); >> + >> + if (nor->info->fixups && nor->info->fixups->post_sfdp) >> + nor->info->fixups->post_sfdp(nor); >> +} >> + >> /** >> * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings >> * based on JESD216 SFDP standard. >> @@ -2523,11 +2542,12 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor) >> >> memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); >> >> - if (spi_nor_parse_sfdp(nor)) { >> - memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); >> - nor->addr_width = 0; >> - nor->flags &= ~SNOR_F_4B_OPCODES; >> - } >> + if (!spi_nor_parse_sfdp(nor)) >> + return spi_nor_post_sfdp_fixups(nor); > > Huh. I didn't know you could do return foo() in a void function if foo() > is also void. Dunno how I feel about this though. It definitely confused > me for a bit. > >> + >> + memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); >> + nor->addr_width = 0; >> + nor->flags &= ~SNOR_F_4B_OPCODES; > > I feel like the new flow makes these 3 lines more confusing. Earlier, > these were called under if (spi_nor_parse_sfdp()) so it was a bit easier > to make the connection that these are undoing the changes performed by > that function. Now it is a little harder to spot. I think a comment is > in order. > I'll revisit this when reaching patch 27/35. cheers, ta
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 15ccc9994215..1f38fa8ab2fa 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2509,6 +2509,25 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor) nor->info->fixups->default_init(nor); } +/** + * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings + * after SFDP has been parsed. + * @nor: pointer to a 'struct spi_nor' + * + * Typically used to tweak various parameters that could not be extracted by + * other means (i.e. when information provided by the SFDP tables are + * incomplete or wrong). + */ +static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) +{ + if (nor->manufacturer && nor->manufacturer->fixups && + nor->manufacturer->fixups->post_sfdp) + nor->manufacturer->fixups->post_sfdp(nor); + + if (nor->info->fixups && nor->info->fixups->post_sfdp) + nor->info->fixups->post_sfdp(nor); +} + /** * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings * based on JESD216 SFDP standard. @@ -2523,11 +2542,12 @@ static void spi_nor_sfdp_init_params(struct spi_nor *nor) memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); - if (spi_nor_parse_sfdp(nor)) { - memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); - nor->addr_width = 0; - nor->flags &= ~SNOR_F_4B_OPCODES; - } + if (!spi_nor_parse_sfdp(nor)) + return spi_nor_post_sfdp_fixups(nor); + + memcpy(nor->params, &sfdp_params, sizeof(*nor->params)); + nor->addr_width = 0; + nor->flags &= ~SNOR_F_4B_OPCODES; } /** @@ -2643,26 +2663,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor) spi_nor_init_uniform_erase_map(map, erase_mask, params->size); } -/** - * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings - * after SFDP has been parsed (is also called for SPI NORs that do not - * support RDSFDP). - * @nor: pointer to a 'struct spi_nor' - * - * Typically used to tweak various parameters that could not be extracted by - * other means (i.e. when information provided by the SFDP/flash_info tables - * are incomplete or wrong). - */ -static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) -{ - if (nor->manufacturer && nor->manufacturer->fixups && - nor->manufacturer->fixups->post_sfdp) - nor->manufacturer->fixups->post_sfdp(nor); - - if (nor->info->fixups && nor->info->fixups->post_sfdp) - nor->info->fixups->post_sfdp(nor); -} - /** * spi_nor_late_init_params() - Late initialization of default flash parameters. * @nor: pointer to a 'struct spi_nor' @@ -2709,18 +2709,12 @@ static void spi_nor_late_init_params(struct spi_nor *nor) * should be more accurate that the above. * spi_nor_sfdp_init_params() * - * Please note that there is a ->post_bfpt() fixup hook that can overwrite - * the flash parameters and settings immediately after parsing the Basic - * Flash Parameter Table. + * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can + * overwrite the flash parameters and settings immediately after table + * parsing. * * which can be overwritten by: - * 4/ Post SFDP flash parameters initialization. Used to tweak various - * parameters that could not be extracted by other means (i.e. when - * information provided by the SFDP/flash_info tables are incomplete or - * wrong). - * spi_nor_post_sfdp_fixups() - * - * 5/ Late flash parameters initialization, used to initialize flash + * 4/ Late flash parameters initialization, used to initialize flash * parameters that are not declared in the JESD216 SFDP standard. * spi_nor_late_init_params() */ @@ -2740,8 +2734,6 @@ static int spi_nor_init_params(struct spi_nor *nor) !(nor->info->flags & SPI_NOR_SKIP_SFDP)) spi_nor_sfdp_init_params(nor); - spi_nor_post_sfdp_fixups(nor); - spi_nor_late_init_params(nor); return 0;
spi_nor_post_sfdp_fixups() was called even when there were no SFDP tables defined and the function name was misleading. We introduced the late_init() hook which is used to tweak various parameters that could not be extracted by other means, i.e. when parameters are not defined in the JESD216 SFDP standard, or when the flash_info flags are incomplete. Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp() hook is as of now used just by s28hs512t, mt35xu512aba, and both support SFDP, there's no functional change with this patch. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 37 deletions(-)