[PATCH v2 27/35] mtd: spi-nor: core: Init flash params based on SFDP first for new flash additions
Tudor.Ambarus at microchip.com
Tudor.Ambarus at microchip.com
Sun Oct 3 22:01:40 PDT 2021
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 at 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.
>
More information about the linux-arm-kernel
mailing list