Re: [RESEND PATCH v3 11/11] mtd: spi-nor: spansion: switch cypress_nor_get_page_size() to use vreg_offset

From: Takahiro Kuwano
Date: Wed Jul 26 2023 - 00:23:50 EST

Hi Tudor,

On 7/24/2023 5:12 PM, Tudor Ambarus wrote:
> All users of cypress_nor_get_page_size() retrieve n_dice and vreg_offset
> from SFDP. Switch cypress_nor_get_page_size() to always use vreg_offset
> so that we use the same code base for both single and multi chip package
> flashes. cypress_nor_get_page_size() is now called in the post_sfdp()
> hook instead of post_bfpt(), as vreg_offset and n_dice are parsed after
> BFPT and we now use them to get the page size in the post_sfdp hook.
> Consequently the null checks on n_dice and vreg_offset are moved to
> the post_sfdp() hook.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> ---
> drivers/mtd/spi-nor/spansion.c | 105 +++++++++++++--------------------
> 1 file changed, 42 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 4027f0038ce5..0b01af33aa57 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -471,28 +471,17 @@ static int cypress_nor_set_addr_mode_nbytes(struct spi_nor *nor)
> return 0;
> }
>
> -static int cypress_nor_get_page_size_single_chip(struct spi_nor *nor)
> -{
> - struct spi_mem_op op =
> - CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_mode_nbytes,
> - SPINOR_REG_CYPRESS_CFR3V, 0,
> - nor->bouncebuf);
> - int ret;
> -
> - ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
> - if (ret)
> - return ret;
> -
> - if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3_PGSZ)
> - nor->params->page_size = 512;
> - else
> - nor->params->page_size = 256;
> -
> - return 0;
> -}
> -
> -
> -static int cypress_nor_get_page_size_mcp(struct spi_nor *nor)
> +/**
> + * cypress_nor_get_page_size() - Get flash page size configuration.
> + * @nor: pointer to a 'struct spi_nor'
> + *
> + * The BFPT table advertises a 512B or 256B page size depending on part but the
> + * page size is actually configurable (with the default being 256B). Read from
> + * CFR3V[4] and set the correct size.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int cypress_nor_get_page_size(struct spi_nor *nor)
> {
> struct spi_mem_op op =
> CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_mode_nbytes,
> @@ -522,23 +511,6 @@ static int cypress_nor_get_page_size_mcp(struct spi_nor *nor)
> return 0;
> }
>
> -/**
> - * cypress_nor_get_page_size() - Get flash page size configuration.
> - * @nor: pointer to a 'struct spi_nor'
> - *
> - * The BFPT table advertises a 512B or 256B page size depending on part but the
> - * page size is actually configurable (with the default being 256B). Read from
> - * CFR3V[4] and set the correct size.
> - *
> - * Return: 0 on success, -errno otherwise.
> - */
> -static int cypress_nor_get_page_size(struct spi_nor *nor)
> -{
> - if (nor->params->n_dice)
> - return cypress_nor_get_page_size_mcp(nor);
> - return cypress_nor_get_page_size_single_chip(nor);
> -}
> -
> static void cypress_nor_ecc_init(struct spi_nor *nor)
> {
> /*
> @@ -575,20 +547,26 @@ s25fs256t_post_bfpt_fixup(struct spi_nor *nor,
> if (nor->bouncebuf[0])
> return -ENODEV;
>
> - return cypress_nor_get_page_size(nor);
> + return 0;
> }
>
> static int s25fs256t_post_sfdp_fixup(struct spi_nor *nor)
> {
> struct spi_nor_flash_parameter *params = nor->params;
>
> + if (!params->n_dice || !params->vreg_offset) {
> + dev_err(nor->dev, "%s failed. The volatile register offset could not be retrieved from SFDP.\n",
> + __func__);
> + return -EOPNOTSUPP;
> + }
> +
S25FS256T failed here. I realized that S25FS256T doesn't have SCCR map...
With fix like below, it works. Make sense?

params->vreg_offset = devm_kmalloc(nor->dev, sizeof(u32), GFP_KERNEL);
if (!params->vreg_offset)
return -ENOMEM;

params->vreg_offset[0] = SPINOR_REG_CYPRESS_VREG;
params->n_dice = 1;


> /* PP_1_1_4_4B is supported but missing in 4BAIT. */
> params->hwcaps.mask |= SNOR_HWCAPS_PP_1_1_4;
> spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_1_1_4],
> SPINOR_OP_PP_1_1_4_4B,
> SNOR_PROTO_1_1_4);
>
> - return 0;
> + return cypress_nor_get_page_size(nor);
> }
>
> static int s25fs256t_late_init(struct spi_nor *nor)
> @@ -623,10 +601,20 @@ s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>
> static int s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
> {
> - struct spi_nor_erase_type *erase_type =
> - nor->params->erase_map.erase_type;
> + struct spi_nor_flash_parameter *params = nor->params;
> + struct spi_nor_erase_type *erase_type = params->erase_map.erase_type;
> unsigned int i;
>
> + if (!params->n_dice || !params->vreg_offset) {
> + dev_err(nor->dev, "%s failed. The volatile register offset could not be retrieved from SFDP.\n",
> + __func__);
> + return -EOPNOTSUPP;
> + }
> +
> + /* The 2 Gb parts duplicate info and advertise 4 dice instead of 2. */
> + if (params->size == SZ_256M)
> + params->n_dice = 2;
> +
> /*
> * In some parts, 3byte erase opcodes are advertised by 4BAIT.
> * Convert them to 4byte erase opcodes.
> @@ -644,10 +632,6 @@ static int s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
> }
> }
>
> - /* The 2 Gb parts duplicate info and advertise 4 dice instead of 2. */
> - if (nor->params->size == SZ_256M)
> - nor->params->n_dice = 2;
> -
> return cypress_nor_get_page_size(nor);
> }
>
> @@ -655,12 +639,6 @@ static int s25hx_t_late_init(struct spi_nor *nor)
> {
> struct spi_nor_flash_parameter *params = nor->params;
>
> - if (!params->n_dice || !params->vreg_offset) {
> - dev_err(nor->dev, "%s failed. The volatile register offset could not be retrieved from SFDP.\n",
> - __func__);
> - return -EOPNOTSUPP;
> - }
> -
> /* Fast Read 4B requires mode cycles */
> params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> params->ready = cypress_nor_sr_ready_and_clear;
> @@ -694,6 +672,17 @@ static int cypress_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
> static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)
> {
> struct spi_nor_flash_parameter *params = nor->params;
> +
> + if (!params->n_dice || !params->vreg_offset) {
> + dev_err(nor->dev, "%s failed. The volatile register offset could not be retrieved from SFDP.\n",
> + __func__);
> + return -EOPNOTSUPP;
> + }
> +
> + /* The 2 Gb parts duplicate info and advertise 4 dice instead of 2. */
> + if (params->size == SZ_256M)
> + params->n_dice = 2;
> +
> /*
> * On older versions of the flash the xSPI Profile 1.0 table has the
> * 8D-8D-8D Fast Read opcode as 0x00. But it actually should be 0xEE.
> @@ -719,10 +708,6 @@ static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)
> */
> params->rdsr_addr_nbytes = 4;
>
> - /* The 2 Gb parts duplicate info and advertise 4 dice instead of 2. */
> - if (params->size == SZ_256M)
> - params->n_dice = 2;
> -
> return cypress_nor_get_page_size(nor);
> }
>
> @@ -737,12 +722,6 @@ static int s28hx_t_late_init(struct spi_nor *nor)
> {
> struct spi_nor_flash_parameter *params = nor->params;
>
> - if (!params->n_dice || !params->vreg_offset) {
> - dev_err(nor->dev, "%s failed. The volatile register offset could not be retrieved from SFDP.\n",
> - __func__);
> - return -EOPNOTSUPP;
> - }
> -
> params->set_octal_dtr = cypress_nor_set_octal_dtr;
> params->ready = cypress_nor_sr_ready_and_clear;
> cypress_nor_ecc_init(nor);

All other S25Hx and S28Hx devices work fine.

Thanks,
Takahiro