[PATCH v5 09/14] mtd: spi-nor: core: Init all flash parameters based on SFDP where possible

[Pratyush Yadav]

On 06/12/21 01:52PM, Tudor.Ambarus at microchip.com wrote:
> On 12/6/21 2:22 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 03/12/21 04:22PM, Tudor Ambarus wrote:
> >> New flash additions that support SFDP should be declared with
> >> PARSE_SFDP and with all the other flags that are not SFDP
> >> discoverable.
> >>
> >> Keep the old way of initializing the flash, until all the flashes
> >> are converted to use either PARSE_SFDP or SPI_NOR_SKIP_SFDP.
> >>
> >> Flashes that declare PARSE_SFDP do not have a roll-back mechanism
> >> because if spi_nor_parse_sfdp() returns an error it means that either
> >> BFPT is not supported, thus SFDP is not supported and the user didn't
> >> correctly declared the flash_info entry, or some memalloc failed.
> >> Either way we should return an error. The rest of the SFDP tables are
> >> optional, if one of the optional SFDP tables fails, we just continue.
> >> We would like to get rid of the default_init() hook, so the
> >> spi_nor_manufacturer_init_params() is not called in the new sequnce
> >> of flash initialization that is based solely on SFDP.
> >>
> >> Split spi_nor_info_init_params() in spi_nor_init_default_params()
> >> and spi_nor_no_sfdp_init_params(). spi_nor_init_default_params() is
> >> called for all the flashes regardless if they support SFDP or not.
> >> spi_nor_no_sfdp_init_params() is called just for the flashes that
> >> do not define SFDP and initializes parameters and setting solely
> >> based on flash_info data.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
> >> ---
> >>  drivers/mtd/spi-nor/core.c | 194 ++++++++++++++++++++++---------------
> >>  1 file changed, 116 insertions(+), 78 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >> index 86bbd1ca22fc..d5eaf9a705ae 100644
> >> --- a/drivers/mtd/spi-nor/core.c
> >> +++ b/drivers/mtd/spi-nor/core.c
> >> @@ -2509,74 +2509,21 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
> >>  }
> >>
> >>  /**
> >> - * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
> >> - * based on JESD216 SFDP standard.
> >> + * spi_nor_no_sfdp_init_params() - Initialize the flash's parameters and
> >> + * settings based on nor->info->sfdp_flags. This method should be called only by
> >> + * flashes that do not define SFDP tables. If the flash supports SFDP but the
> >> + * information is wrong and the settings from this function can not be retrieved
> >> + * by parsing SFDP, one should instead use the fixup hooks and update the wrong
> > 
> > s/fixup hooks/fixup flags/ ?
> 
> fixup hooks is fine if we want to fix a wrong SFDP table.
> fixup flags for settings that are not SFDP discoverable, i.e. where the table
> is missing.

Ok.

> 
> 3/ FIXUP_FLAGS: flags that indicate support that can be discovered
>    via SFDP ideally, but can not be discovered for this particular flash
>    because the SFDP table that indicates this support is not defined by
>    the flash. In case the table for this support is defined but has wrong
>    values, one should instead use a post_sfdp() hook to set the SNOR_F
>    equivalent flag.
> > 
> >> + * bits.
> >>   * @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)) {
> >> -             memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
> >> -             nor->addr_width = 0;
> >> -             nor->flags &= ~SNOR_F_4B_OPCODES;
> >> -     }
> >> -}
> > [...]
> >> +
> >> +/**
> >> + * spi_nor_init_params_deprecated() - Deprecated way of initializing flash
> >> + * parameters and settings.
> >> + * @nor:     pointer to a 'struct spi_nor'.
> >> + *
> >> + * The method assumes that flash doesn't support SFDP so it initializes flash
> >> + * parameters in spi_nor_no_sfdp_init_params() which later on can be overwritten
> >> + * when parsing SFDP, if supported.
> >> + */
> >> +static void spi_nor_init_params_deprecated(struct spi_nor *nor)
> >> +{
> >> +     spi_nor_no_sfdp_init_params(nor);
> >> +
> >> +     spi_nor_manufacturer_init_params(nor);
> >> +
> >> +     if ((SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_OCTAL_READ |
> >> +          SPI_NOR_OCTAL_DTR_READ) &&
> >> +         !(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))
> >> +             spi_nor_sfdp_init_params_deprecated(nor);
> >> +}
> >> +
> > [...]
> >> @@ -2773,24 +2807,28 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
> >>   * parameters that are not declared in the JESD216 SFDP standard, or where SFDP
> >>   * tables are not defined at all.
> >>   *           spi_nor_late_init_params()
> >> + *
> >> + * Return: 0 on success, -errno otherwise.
> >>   */
> >>  static int spi_nor_init_params(struct spi_nor *nor)
> >>  {
> >> +     int ret;
> >> +
> >>       nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params), GFP_KERNEL);
> >>       if (!nor->params)
> >>               return -ENOMEM;
> >>
> >> -     spi_nor_info_init_params(nor);
> >> -
> >> -     spi_nor_manufacturer_init_params(nor);
> >> +     spi_nor_init_default_params(nor);
> >>
> >> -     if ((nor->info->parse_sfdp ||
> >> -          (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
> >> -                                       SPI_NOR_QUAD_READ |
> >> -                                       SPI_NOR_OCTAL_READ |
> >> -                                       SPI_NOR_OCTAL_DTR_READ))) &&
> >> -         !(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))
> >> -             spi_nor_sfdp_init_params(nor);
> >> +     if (nor->info->parse_sfdp) {
> >> +             ret = spi_nor_parse_sfdp(nor);
> >> +             if (ret) {
> >> +                     dev_err(nor->dev, "BFPT parsing failed. Please consider using SPI_NOR_SKIP_SFDP when declaring the flash\n");
> >> +                     return ret;
> >> +             }
> >> +     } else {
> >> +             spi_nor_init_params_deprecated(nor);
> > 
> > Hmm, a flash without SFDP would always go via the "deprecated" path even
> > if it is a new entry. It makes no difference in practice, but is just
> > slightly confusing logically. Can we make this a bit clearer? How about
> > this:
> > 
> >   if (nor->info->parse_sfdp) {
> >         ...
> >   } else if (nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP) {
> >         spi_nor_no_sfdp_init_params();
> >         spi_nor_manufacturer_init_params();
> >   } else {
> >         spi_nor_init_params_deprecated();
> >   }
> > 
> >> +     }
> 
> Ok, will do. Can I add your R-b tag on a local v6 with this change that you
> proposed? It will spare me of sending v6.

Sure, go ahead.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.