Re: [PATCH v13 3/4] mtd: spi-nor: spansion: Add local function to discover page size

Takahiro Kuwano
2022-04-22T09:14:54Z

On 4/21/2022 7:43 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/21/22 12:40, tkuw584924@gmail.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> The page size check in s28hs512t fixup can be used for s25hs/hl-t as well.
>> Move that to a newly created local function.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> Changes in v13:
>> - (no change)
>>
>> Changes in v12:
>> - Rebase on top of Tudor's series
>> https://patchwork.ozlabs.org/project/linux-mtd/list/?series=295933
>> - Remove addr_width param. Use nor->addr_width instead.
>>
>> Changes in v11:
>> - Rebase on top of Tudor's series
>> https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
>> - Add addr_width param
>>
>> drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++--------------
>> 1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 65ae3ade0b95..952d4383f9da 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -172,6 +172,37 @@ static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>> return 0;
>> }
>>
>> +/**
>> + * cypress_nor_set_page_size() - Set page size which corresponds to the flash
>> + * 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_set_page_size(struct spi_nor *nor)
>> +{
>> + struct spi_mem_op op =
>> + CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width,
>> + SPINOR_REG_CYPRESS_CFR3V,
>> + 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_CFR3V_PGSZ)
>> + nor->params->page_size = 512;
>> + else
>> + nor->params->page_size = 256;
>> +
>> + return 0;
>> +}
>> +
>> /**
>> * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>> * @nor: pointer to a 'struct spi_nor'
>> @@ -226,28 +257,7 @@ static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
>> const struct sfdp_parameter_header *bfpt_header,
>> const struct sfdp_bfpt *bfpt)
>> {
>> - /*
>> - * The BFPT table advertises a 512B page size but the page size is
>> - * actually configurable (with the default being 256B). Read from
>> - * CFR3V[4] and set the correct size.
>> - */
>> - struct spi_mem_op op =
>> - CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR3V,
>
> here you substitute 3 with nor->addr_width behind the doors. It would have
> worth to have a dedicated patch for this and then come with a patch for
> the local function. Or at least add a comment in the commit's description.
>
I will update commit description.

>> - nor->bouncebuf);
>> - int ret;
>> -
>> - spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
>> -
>> - ret = spi_mem_exec_op(nor->spimem, &op);
These are replaced with spi_nor_read_any_reg().
I will also add a comment about this.

Thanks,
Takahiro