[PATCH v2 4/8] mtd: spi-nor: spansion: Rework cypress_nor_set_page_size() for multi-chip device support

[Tudor Ambarus]

On 02.03.2023 08:09, Tudor Ambarus wrote:
> 
> 
> On 08.02.2023 07:53, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>
>> For multi-chip devices, we can use 512B page only when the all dice are
>> configured as 512B page size. Simple for-loop with params->num_of_dice
>> can
>> check the CFR3V in each die. The volatile register address is calculated
>> inside the loop by using die number and volatile register offset.
>>
>> The location of cypress_nor_set_page_size() call is moved from
>> post_bfpt_fixup() to post_sfdp_fixup(), because the number of dice and
>> volatile register offset are parsed after BFPT. The post_sfdp_fixup()
> 
> No, this looks wrong.
> 
> BFPT is the only mandatory table and should be sufficient to make any
> flash work (in the multi die case, just for the first die). All the
> other tables are considered optional. If we get an error when parsing
> BFPT we return an error, if we get an error in any optional table, we
> just print a warning and continue. So you should make sure that the
> flash works fine even if sccr_map_mc fails.
> 
>> does not return error code so cypress_nor_set_page_size() prints error
>> message instead of returning error code.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>>   drivers/mtd/spi-nor/spansion.c | 47 ++++++++++++++++++----------------
>>   1 file changed, 25 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c
>> b/drivers/mtd/spi-nor/spansion.c
>> index 12a256c0ef4c..7c533737d7c3 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -18,7 +18,7 @@
>>   #define SPINOR_REG_CYPRESS_CFR1_QUAD_EN        BIT(1)    /* Quad
>> Enable */
>>   #define SPINOR_REG_CYPRESS_CFR2V        0x00800003
>>   #define SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24    0xb
>> -#define SPINOR_REG_CYPRESS_CFR3V        0x00800004
>> +#define SPINOR_REG_CYPRESS_CFR3            0x4
>>   #define SPINOR_REG_CYPRESS_CFR3_PGSZ        BIT(4) /* Page size. */
>>   #define SPINOR_REG_CYPRESS_CFR5V        0x00800006
>>   #define SPINOR_REG_CYPRESS_CFR5_BIT6        BIT(6)
>> @@ -195,27 +195,34 @@ static int
>> cypress_nor_quad_enable_volatile(struct spi_nor *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)
>> +static void cypress_nor_set_page_size(struct spi_nor *nor)
>>   {
>> +    struct spi_nor_flash_parameter *params = nor->params;
>>       struct spi_mem_op op =
>> -        CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_mode_nbytes,
>> -                      SPINOR_REG_CYPRESS_CFR3V,
>> +        CYPRESS_NOR_RD_ANY_REG_OP(params->addr_mode_nbytes, 0,
>>                         nor->bouncebuf);
>>       int ret;
>> +    u8 i;
>>   -    ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
>> -    if (ret)
>> -        return ret;
>> +    /* Set to default page size */
>> +    params->page_size = 256;
>>   -    if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3_PGSZ)
>> -        nor->params->page_size = 512;
>> -    else
>> -        nor->params->page_size = 256;
>> +    for (i = 0; i < params->num_of_dice; i++) {
>> +        op.addr.val = params->vreg_offset[i] + SPINOR_REG_CYPRESS_CFR3;
> 
> in case sccr_map fails vreg_offset is zero here, you'll read garbage
>>   -    return 0;
>> +        ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
>> +        if (ret) {
>> +            dev_err(nor->dev, "Failed to read page size
>> configuration. Use default 256B.\n");
>> +            return;
> 
> why did we consider a no-go error and exited in the past and now we just
> use a default config?
> 

we should instead allow parse_sfdp to return errors. I'm sending a patch
shortly.