Re: [PATCH v2 26/35] mtd: spi-nor: core: Introduce spi_nor_init_default_params()

Pratyush Yadav
2021-08-24T17:31:53Z

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@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?

> +
> + 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(&params->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(&params->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(&params->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 = &params->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(&params->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(&params->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(&params->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(&params->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.

--
Regards,
Pratyush Yadav
Texas Instruments Inc.