[PATCH v2 26/35] mtd: spi-nor: core: Introduce spi_nor_init_default_params()
Tudor.Ambarus at microchip.com
Tudor.Ambarus at microchip.com
Fri Oct 22 05:55:26 PDT 2021
On 10/22/21 3:41 PM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am 2021-10-04 06:17, schrieb Tudor.Ambarus at microchip.com:
>> On 8/24/21 8:30 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:
>>>> Called for all flashes, regardless if they define SFDP tables or not.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus at microchip.com>
>>>> ---
>>>> drivers/mtd/spi-nor/core.c | 92
>>>> +++++++++++++++++++++-----------------
>>>> 1 file changed, 52 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index b3a01d7d6f0b..9193317f897d 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor,
>>>> return spi_nor_set_addr_width(nor);
>>>> }
>>>>
>>>> +/**
>>>> + * spi_nor_init_default_params() - Default initialization of flash
>>>> parameters
>>>> + * and settings. Done for all flashes, regardless is they define
>>>> SFDP tables
>>>> + * or not.
>>>> + * @nor: pointer to a 'struct spi_nor'.
>>>> + */
>>>> +static void spi_nor_init_default_params(struct spi_nor *nor)
>>>> +{
>>>> + struct spi_nor_flash_parameter *params = nor->params;
>>>> + const struct flash_info *info = nor->info;
>>>> + struct device_node *np = spi_nor_get_flash_node(nor);
>>>> +
>>>> + params->quad_enable = spi_nor_sr2_bit1_quad_enable;
>>>> + params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
>>>> + params->setup = spi_nor_default_setup;
>>>> + params->otp.org = &info->otp_org;
>>>> +
>>>> + /* Default to 16-bit Write Status (01h) Command */
>>>> + nor->flags |= SNOR_F_HAS_16BIT_SR;
>>>> +
>>>> + /* Set SPI NOR sizes. */
>>>> + params->writesize = 1;
>>>> + params->size = (u64)info->sector_size * info->n_sectors;
>>>> + params->page_size = info->page_size;
>>>
>>> I think these two lines should go in spi_nor_info_init_params() since
>>> you are using the nor info to initialize these parameters. Otherwise,
>>> what even is the difference between these two functions?
>>
>> I think a better name for spi_nor_info_init_params() is
>> spi_nor_nonsfdp_info_init_params(). This method will eventually be
>> called
>> just for non SFDP flashes. Check conversation in 18/35.
>>
>> And maybe I should rename spi_nor_nonsfdp_flags_init to
>> spi_nor_nonsfdp_info_init_snor_f.
>
> Urgh. I really dislike that suffix. Somehow the flags prefix
:) I'll think of something better then.
> creeps into the function name. If that function is expected
> to only set the SNOR_F_ flags then there should be a comment.
>
>>>
>>>> +
>>>> + if (!(info->flags & SPI_NOR_NO_FR)) {
>>>> + /* Default to Fast Read for DT and non-DT platform
>>>> devices. */
>>>> + params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>>>> +
>>>> + /* Mask out Fast Read if not requested at DT
>>>> instantiation. */
>>>> + if (np && !of_property_read_bool(np, "m25p,fast-read"))
>>>> + params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>>>> + }
>
> Hm, this looks really weird.
>
> why not:
>
> /* default to fast read */
> params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>
> /* unless the flash doesn't support it */
> if (info->flags & SPI_NOR_NO_FR)
> params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>
> /* or the device tree doesn't explicitly request it */
> if (np && !of_property_read_bool(np, "m25p,fast-read"))
> params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>
> I know the old code was the same, but it might go into another
> patch. The SPI_NOR_NO_FR suggests that fast read was always the
right, dedicated patch. I'll check it.
> default. Otherwise, it would have been SPI_NOR_HAS_FAST_READ.
>
>>>> +
>>>> + /* (Fast) Read settings. */
>>>> + params->hwcaps.mask |= SNOR_HWCAPS_READ;
>>>> + spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ],
>>>> + 0, 0, SPINOR_OP_READ,
>>>> + SNOR_PROTO_1_1_1);
>>>> +
>>>> + if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>>>> +
>>>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST],
>>>> + 0, 8, SPINOR_OP_READ_FAST,
>>>> + SNOR_PROTO_1_1_1);
>>>> + /* Page Program settings. */
>>>> + params->hwcaps.mask |= SNOR_HWCAPS_PP;
>>>> + spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP],
>>>> + SPINOR_OP_PP, SNOR_PROTO_1_1_1);
>>>> +}
>>>> +
>>>> /**
>>>> * spi_nor_manufacturer_init_params() - Initialize the flash's
>>>> parameters and
>>>> * settings based on MFR register and ->default_init() hook.
>>>> @@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct
>>>> spi_nor *nor)
>>>> struct spi_nor_flash_parameter *params = nor->params;
>>>> struct spi_nor_erase_map *map = ¶ms->erase_map;
>>>> const struct flash_info *info = nor->info;
>>>> - struct device_node *np = spi_nor_get_flash_node(nor);
>>>> u8 i, erase_mask;
>>>>
>>>> - /* Initialize default flash parameters and settings. */
>>>> - params->quad_enable = spi_nor_sr2_bit1_quad_enable;
>>>> - params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
>>>> - params->setup = spi_nor_default_setup;
>>>> - params->otp.org = &info->otp_org;
>>>> -
>>>> - /* Default to 16-bit Write Status (01h) Command */
>>>> - nor->flags |= SNOR_F_HAS_16BIT_SR;
>>>> -
>>>> - /* Set SPI NOR sizes. */
>>>> - params->writesize = 1;
>>>> - params->size = (u64)info->sector_size * info->n_sectors;
>>>> - params->page_size = info->page_size;
>>>> -
>>>> - if (!(info->flags & SPI_NOR_NO_FR)) {
>>>> - /* Default to Fast Read for DT and non-DT platform
>>>> devices. */
>>>> - params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>>>> -
>>>> - /* Mask out Fast Read if not requested at DT
>>>> instantiation. */
>>>> - if (np && !of_property_read_bool(np, "m25p,fast-read"))
>>>> - params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
>>>> - }
>>>> -
>>>> - /* (Fast) Read settings. */
>>>> - params->hwcaps.mask |= SNOR_HWCAPS_READ;
>>>> - spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ],
>>>> - 0, 0, SPINOR_OP_READ,
>>>> - SNOR_PROTO_1_1_1);
>>>> -
>>>> - if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
>>>> -
>>>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST],
>>>> - 0, 8, SPINOR_OP_READ_FAST,
>>>> - SNOR_PROTO_1_1_1);
>>>> -
>>>> if (info->flags & SPI_NOR_DUAL_READ) {
>>>> params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
>>>>
>>>> spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2],
>>>> @@ -2674,11 +2689,6 @@ static void spi_nor_info_init_params(struct
>>>> spi_nor *nor)
>>>> SNOR_PROTO_8_8_8_DTR);
>>>> }
>>>>
>>>> - /* Page Program settings. */
>>>> - params->hwcaps.mask |= SNOR_HWCAPS_PP;
>>>> - spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP],
>>>> - SPINOR_OP_PP, SNOR_PROTO_1_1_1);
>>>> -
>>>> if (info->flags & SPI_NOR_OCTAL_DTR_PP) {
>>>> params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
>>>> /*
>>>> @@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor
>>>> *nor)
>>>> if (!nor->params)
>>>> return -ENOMEM;
>>>>
>>>> + spi_nor_init_default_params(nor);
>>>> +
>>>> spi_nor_info_init_params(nor);
>>>>
>>>> spi_nor_manufacturer_init_params(nor);
>>>
>>> I am neutral towards this patch. I don't think it improves much, but
>>> at
>>> the same time it doesn't make anything worse either.
>>
>> I think it helps readability. It splits spi_nor_init_params() into
>> smaller logical chunks,
>> based on the type of initialization. We should usually avoid long
>> methods where we can split
>> them in logical chunks, it makes the code pleasant to read.
>
> I'm fine with that. Just want to say, that then we have to think
> about what goes where and where we draw the line.
>
Okay, I'll propose some guidance in the commit message.
More information about the linux-mtd
mailing list