Re: [PATCH v2 27/35] mtd: spi-nor: core: Init flash params based on SFDP first for new flash additions

[<Tudor.Ambarus@microchip.com>]

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.
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/