| Message ID | 20210727045222.905056-28-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: > Remove the spagetti way of initializing flash parameters and settings, > at least for the new flash additions (for now). All flash entries should > be converted to either use SPI_NOR_PARSE_SFDP or SPI_NOR_SKIP_SFDP. > SPI_NOR_SKIP_SFDP should be set either when the SFDP tables are completely > wrong and we can't parse relevant data, or when the SFDP tables are not > defined at all, or when RDSFDP command is not supported by the flash. > After all the flash entries will be converted to use these flags and after > the default_init() hook will be removed, the > spi_nor_init_params_deprecated() will be replaced by > spi_nor_info_init_params(). The flash parameters and settings will be > initialized either by parsing SFDP, or via the flash info flags. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/mtd/spi-nor/core.c | 103 +++++++++++++++++++++++++------------ > 1 file changed, 70 insertions(+), 33 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 9193317f897d..ef06a8d6abb8 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -2627,28 +2627,6 @@ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) > nor->info->fixups->post_sfdp(nor); > } > > -/** > - * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings > - * based on JESD216 SFDP standard. > - * @nor: pointer to a 'struct spi_nor'. > - * > - * The method has a roll-back mechanism: in case the SFDP parsing fails, the > - * legacy flash parameters and settings will be restored. > - */ > -static void spi_nor_sfdp_init_params(struct spi_nor *nor) > -{ > - struct spi_nor_flash_parameter sfdp_params; > - > - memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); > - > - 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; > -} > - > /** > * spi_nor_info_init_params() - Initialize the flash's parameters and settings > * based on nor->info data. > @@ -2722,6 +2700,39 @@ static void spi_nor_info_init_params(struct spi_nor *nor) > spi_nor_init_uniform_erase_map(map, erase_mask, params->size); > } > > +/** > + * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings > + * based on JESD216 SFDP standard. > + * @nor: pointer to a 'struct spi_nor'. Missing documentation for treat_id_collisions. Also, I assume this will go away when we get rid of spi_nor_init_params_deprecated(), correct? > + * > + * The method has a roll-back mechanism: in case the SFDP parsing fails, the > + * legacy flash parameters and settings will be restored. > + */ > +static void spi_nor_sfdp_init_params(struct spi_nor *nor, > + bool treat_id_collisions) > +{ > + struct spi_nor_flash_parameter sfdp_params; > + > + memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); > + > + 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; > + > + if (!treat_id_collisions) > + return; No, this doesn't seem quite right. Why would you not want to treat ID collisions for flashes that use spi_nor_init_params_deprecated()? What makes this not possible for them? Anyway, even if we don't want to treat ID collisions for those flashes, we can't just return here. In the previous code, nor->params is already initialized by spi_nor_info_init_params() so restoring that via memcpy() would make sense since it would restore the info-initialized state. Now it would be just 0, with no useful information in there. You are bound to run into errors somewhere down the line. So I think you either need to make this function return an error and propagate it up the call chain or run spi_nor_info_init_params() for both type of flashes to make sure we do our best effort to initialize the flash. > + /* > + * Fallback to flash info params init in case the SFDP parsing fails. > + * Used to handle ID collisions between flashes that define the SFDP > + * tables and flashes that don't. > + */ > + spi_nor_info_init_params(nor); > + spi_nor_manufacturer_init_params(nor); > +} > + > /** > * spi_nor_late_init_params() - Late initialization of default flash parameters. > * @nor: pointer to a 'struct spi_nor' > @@ -2797,7 +2808,9 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) > } > > /** > - * spi_nor_init_params() - Initialize the flash's parameters and settings. > + * spi_nor_init_params_deprecated() - Initialize the flash's parameters and > + * settings. The function is deprecated, it will be removed and replaced with > + * spi_nor_info_init_params(). > * @nor: pointer to a 'struct spi_nor'. > * > * The flash parameters and settings are initialized based on a sequence of > @@ -2821,11 +2834,40 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) > * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can > * overwrite the flash parameters and settings immediately after table > * parsing. > + */ > +static void spi_nor_init_params_deprecated(struct spi_nor *nor) > +{ > + spi_nor_info_init_params(nor); > + spi_nor_manufacturer_init_params(nor); > + > + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && > + !(nor->info->flags & SPI_NOR_SKIP_SFDP)) > + spi_nor_sfdp_init_params(nor, false); > +} > + > +/** > + * spi_nor_init_params() - Initialize the flash's parameters and settings. > + * @nor: pointer to a 'struct spi_nor'. > + * > + * The flash parameters and settings are initialized based on a sequence of > + * calls that are ordered by priority: > + * > + * 1/ Default flash parameters initialization. The initializations are done > + * for all the flashes, regardless if the support SFDP or not. > + * spi_nor_init_default_params() > + * which can be overwritten by: > * > + * 2/ SFDP based or the deprecated way of initializing flash parameters. > + * Ideally at this step the flash parameters init will be done either by > + * parsing SFDP, where supported, or statically via flash_info flags. > + * spi_nor_sfdp_init_params() or spi_nor_init_params_deprecated() > * which can be overwritten by: > - * 4/ Late flash parameters initialization, used to initialize flash > + * > + * 3/ Late flash parameters initialization, used to initialize flash > * parameters that are not declared in the JESD216 SFDP standard. > * spi_nor_late_init_params() > + * Not really related to this patch, but we need to document the return value here as well. > */ > static int spi_nor_init_params(struct spi_nor *nor) > { > @@ -2835,15 +2877,10 @@ static int spi_nor_init_params(struct spi_nor *nor) > > spi_nor_init_default_params(nor); > > - spi_nor_info_init_params(nor); > - > - spi_nor_manufacturer_init_params(nor); > - > - if ((nor->info->flags & (SPI_NOR_PARSE_SFDP | > - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | > - SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && > - !(nor->info->flags & SPI_NOR_SKIP_SFDP)) > - spi_nor_sfdp_init_params(nor); > + if (nor->info->flags & SPI_NOR_PARSE_SFDP) > + spi_nor_sfdp_init_params(nor, true); > + else > + spi_nor_init_params_deprecated(nor); > > spi_nor_late_init_params(nor); > > -- > 2.25.1 >
On 8/24/21 8:51 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: >> Remove the spagetti way of initializing flash parameters and settings, >> at least for the new flash additions (for now). All flash entries should >> be converted to either use SPI_NOR_PARSE_SFDP or SPI_NOR_SKIP_SFDP. >> SPI_NOR_SKIP_SFDP should be set either when the SFDP tables are completely >> wrong and we can't parse relevant data, or when the SFDP tables are not >> defined at all, or when RDSFDP command is not supported by the flash. >> After all the flash entries will be converted to use these flags and after >> the default_init() hook will be removed, the >> spi_nor_init_params_deprecated() will be replaced by >> spi_nor_info_init_params(). The flash parameters and settings will be >> initialized either by parsing SFDP, or via the flash info flags. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> >> --- >> drivers/mtd/spi-nor/core.c | 103 +++++++++++++++++++++++++------------ >> 1 file changed, 70 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 9193317f897d..ef06a8d6abb8 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -2627,28 +2627,6 @@ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) >> nor->info->fixups->post_sfdp(nor); >> } >> >> -/** >> - * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings >> - * based on JESD216 SFDP standard. >> - * @nor: pointer to a 'struct spi_nor'. >> - * >> - * The method has a roll-back mechanism: in case the SFDP parsing fails, the >> - * legacy flash parameters and settings will be restored. >> - */ >> -static void spi_nor_sfdp_init_params(struct spi_nor *nor) >> -{ >> - struct spi_nor_flash_parameter sfdp_params; >> - >> - memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); >> - >> - 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; >> -} >> - >> /** >> * spi_nor_info_init_params() - Initialize the flash's parameters and settings >> * based on nor->info data. >> @@ -2722,6 +2700,39 @@ static void spi_nor_info_init_params(struct spi_nor *nor) >> spi_nor_init_uniform_erase_map(map, erase_mask, params->size); >> } >> >> +/** >> + * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings >> + * based on JESD216 SFDP standard. >> + * @nor: pointer to a 'struct spi_nor'. > > Missing documentation for treat_id_collisions. ok > > Also, I assume this will go away when we get rid of > spi_nor_init_params_deprecated(), correct? correct > >> + * >> + * The method has a roll-back mechanism: in case the SFDP parsing fails, the >> + * legacy flash parameters and settings will be restored. >> + */ >> +static void spi_nor_sfdp_init_params(struct spi_nor *nor, >> + bool treat_id_collisions) >> +{ >> + struct spi_nor_flash_parameter sfdp_params; >> + >> + memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); >> + >> + 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; >> + >> + if (!treat_id_collisions) >> + return; > > No, this doesn't seem quite right. Why would you not want to treat ID > collisions for flashes that use spi_nor_init_params_deprecated()? What > makes this not possible for them? I think I wanted to motivate people to switch to the parse SFDP first idea. Why would we care for those flashes that use deprecated methods? > > Anyway, even if we don't want to treat ID collisions for those flashes, > we can't just return here. In the previous code, nor->params is already > initialized by spi_nor_info_init_params() so restoring that via memcpy() > would make sense since it would restore the info-initialized state. Now > it would be just 0, with no useful information in there. You are bound > to run into errors somewhere down the line. you're correct! > > So I think you either need to make this function return an error and > propagate it up the call chain or run spi_nor_info_init_params() for > both type of flashes to make sure we do our best effort to initialize > the flash. > >> + /* >> + * Fallback to flash info params init in case the SFDP parsing fails. >> + * Used to handle ID collisions between flashes that define the SFDP >> + * tables and flashes that don't. >> + */ >> + spi_nor_info_init_params(nor); >> + spi_nor_manufacturer_init_params(nor); >> +} >> + >> /** >> * spi_nor_late_init_params() - Late initialization of default flash parameters. >> * @nor: pointer to a 'struct spi_nor' >> @@ -2797,7 +2808,9 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) >> } >> >> /** >> - * spi_nor_init_params() - Initialize the flash's parameters and settings. >> + * spi_nor_init_params_deprecated() - Initialize the flash's parameters and >> + * settings. The function is deprecated, it will be removed and replaced with >> + * spi_nor_info_init_params(). >> * @nor: pointer to a 'struct spi_nor'. >> * >> * The flash parameters and settings are initialized based on a sequence of >> @@ -2821,11 +2834,40 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) >> * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can >> * overwrite the flash parameters and settings immediately after table >> * parsing. >> + */ >> +static void spi_nor_init_params_deprecated(struct spi_nor *nor) >> +{ >> + spi_nor_info_init_params(nor); >> + spi_nor_manufacturer_init_params(nor); >> + >> + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >> + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && >> + !(nor->info->flags & SPI_NOR_SKIP_SFDP)) >> + spi_nor_sfdp_init_params(nor, false); >> +} >> + >> +/** >> + * spi_nor_init_params() - Initialize the flash's parameters and settings. >> + * @nor: pointer to a 'struct spi_nor'. >> + * >> + * The flash parameters and settings are initialized based on a sequence of >> + * calls that are ordered by priority: >> + * >> + * 1/ Default flash parameters initialization. The initializations are done >> + * for all the flashes, regardless if the support SFDP or not. >> + * spi_nor_init_default_params() >> + * which can be overwritten by: >> * >> + * 2/ SFDP based or the deprecated way of initializing flash parameters. >> + * Ideally at this step the flash parameters init will be done either by >> + * parsing SFDP, where supported, or statically via flash_info flags. >> + * spi_nor_sfdp_init_params() or spi_nor_init_params_deprecated() >> * which can be overwritten by: >> - * 4/ Late flash parameters initialization, used to initialize flash >> + * >> + * 3/ Late flash parameters initialization, used to initialize flash >> * parameters that are not declared in the JESD216 SFDP standard. >> * spi_nor_late_init_params() >> + * > > Not really related to this patch, but we need to document the return > value here as well. ok Cheers, ta > >> */ >> static int spi_nor_init_params(struct spi_nor *nor) >> { >> @@ -2835,15 +2877,10 @@ static int spi_nor_init_params(struct spi_nor *nor) >> >> spi_nor_init_default_params(nor); >> >> - spi_nor_info_init_params(nor); >> - >> - spi_nor_manufacturer_init_params(nor); >> - >> - if ((nor->info->flags & (SPI_NOR_PARSE_SFDP | >> - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | >> - SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && >> - !(nor->info->flags & SPI_NOR_SKIP_SFDP)) >> - spi_nor_sfdp_init_params(nor); >> + if (nor->info->flags & SPI_NOR_PARSE_SFDP) >> + spi_nor_sfdp_init_params(nor, true); >> + else >> + spi_nor_init_params_deprecated(nor); >> >> spi_nor_late_init_params(nor); >> >> -- >> 2.25.1 >> > > -- > Regards, > Pratyush Yadav > Texas Instruments Inc. >
On 10/4/21 8:01 AM, Tudor.Ambarus@microchip.com wrote: cut >>> + if (!treat_id_collisions) >>> + return; >> >> No, this doesn't seem quite right. Why would you not want to treat ID >> collisions for flashes that use spi_nor_init_params_deprecated()? What >> makes this not possible for them? > > I think I wanted to motivate people to switch to the parse SFDP first idea. > Why would we care for those flashes that use deprecated methods? > No, no, I'm wrong. cut >>> + if (nor->info->flags & SPI_NOR_PARSE_SFDP) >>> + spi_nor_sfdp_init_params(nor, true); >>> + else >>> + spi_nor_init_params_deprecated(nor); deprecated maybe it's not the best name, because here falls also the non-SFDP compliant flashes, which are not mandatory seen as deprecated. I'll rework this and resubmit.
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 9193317f897d..ef06a8d6abb8 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -2627,28 +2627,6 @@ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor) nor->info->fixups->post_sfdp(nor); } -/** - * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings - * based on JESD216 SFDP standard. - * @nor: pointer to a 'struct spi_nor'. - * - * The method has a roll-back mechanism: in case the SFDP parsing fails, the - * legacy flash parameters and settings will be restored. - */ -static void spi_nor_sfdp_init_params(struct spi_nor *nor) -{ - struct spi_nor_flash_parameter sfdp_params; - - memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); - - 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; -} - /** * spi_nor_info_init_params() - Initialize the flash's parameters and settings * based on nor->info data. @@ -2722,6 +2700,39 @@ static void spi_nor_info_init_params(struct spi_nor *nor) spi_nor_init_uniform_erase_map(map, erase_mask, params->size); } +/** + * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings + * based on JESD216 SFDP standard. + * @nor: pointer to a 'struct spi_nor'. + * + * The method has a roll-back mechanism: in case the SFDP parsing fails, the + * legacy flash parameters and settings will be restored. + */ +static void spi_nor_sfdp_init_params(struct spi_nor *nor, + bool treat_id_collisions) +{ + struct spi_nor_flash_parameter sfdp_params; + + memcpy(&sfdp_params, nor->params, sizeof(sfdp_params)); + + 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; + + if (!treat_id_collisions) + return; + /* + * Fallback to flash info params init in case the SFDP parsing fails. + * Used to handle ID collisions between flashes that define the SFDP + * tables and flashes that don't. + */ + spi_nor_info_init_params(nor); + spi_nor_manufacturer_init_params(nor); +} + /** * spi_nor_late_init_params() - Late initialization of default flash parameters. * @nor: pointer to a 'struct spi_nor' @@ -2797,7 +2808,9 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) } /** - * spi_nor_init_params() - Initialize the flash's parameters and settings. + * spi_nor_init_params_deprecated() - Initialize the flash's parameters and + * settings. The function is deprecated, it will be removed and replaced with + * spi_nor_info_init_params(). * @nor: pointer to a 'struct spi_nor'. * * The flash parameters and settings are initialized based on a sequence of @@ -2821,11 +2834,40 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can * overwrite the flash parameters and settings immediately after table * parsing. + */ +static void spi_nor_init_params_deprecated(struct spi_nor *nor) +{ + spi_nor_info_init_params(nor); + spi_nor_manufacturer_init_params(nor); + + if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && + !(nor->info->flags & SPI_NOR_SKIP_SFDP)) + spi_nor_sfdp_init_params(nor, false); +} + +/** + * spi_nor_init_params() - Initialize the flash's parameters and settings. + * @nor: pointer to a 'struct spi_nor'. + * + * The flash parameters and settings are initialized based on a sequence of + * calls that are ordered by priority: + * + * 1/ Default flash parameters initialization. The initializations are done + * for all the flashes, regardless if the support SFDP or not. + * spi_nor_init_default_params() + * which can be overwritten by: * + * 2/ SFDP based or the deprecated way of initializing flash parameters. + * Ideally at this step the flash parameters init will be done either by + * parsing SFDP, where supported, or statically via flash_info flags. + * spi_nor_sfdp_init_params() or spi_nor_init_params_deprecated() * which can be overwritten by: - * 4/ Late flash parameters initialization, used to initialize flash + * + * 3/ 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) { @@ -2835,15 +2877,10 @@ static int spi_nor_init_params(struct spi_nor *nor) spi_nor_init_default_params(nor); - spi_nor_info_init_params(nor); - - spi_nor_manufacturer_init_params(nor); - - if ((nor->info->flags & (SPI_NOR_PARSE_SFDP | - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | - SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) && - !(nor->info->flags & SPI_NOR_SKIP_SFDP)) - spi_nor_sfdp_init_params(nor); + if (nor->info->flags & SPI_NOR_PARSE_SFDP) + spi_nor_sfdp_init_params(nor, true); + else + spi_nor_init_params_deprecated(nor); spi_nor_late_init_params(nor);
Remove the spagetti way of initializing flash parameters and settings, at least for the new flash additions (for now). All flash entries should be converted to either use SPI_NOR_PARSE_SFDP or SPI_NOR_SKIP_SFDP. SPI_NOR_SKIP_SFDP should be set either when the SFDP tables are completely wrong and we can't parse relevant data, or when the SFDP tables are not defined at all, or when RDSFDP command is not supported by the flash. After all the flash entries will be converted to use these flags and after the default_init() hook will be removed, the spi_nor_init_params_deprecated() will be replaced by spi_nor_info_init_params(). The flash parameters and settings will be initialized either by parsing SFDP, or via the flash info flags. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/mtd/spi-nor/core.c | 103 +++++++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 33 deletions(-)