Re: [PATCH v17 7/7] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups

[<Tudor.Ambarus@microchip.com>]

On 7/27/22 14:18, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-07-25 11:25, schrieb Tudor Ambarus:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>
>> These Infineon chips support volatile version of configuration
>> registers
>> and it is recommended to update volatile registers in the field
>> application
>> due to a risk of the non-volatile registers corruption by power
>> interrupt.
>> Add support for volatile QE bit.
>>
>> For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
>> uniform sector sizes are supported. This is due to missing or incorrect
>> entries in SMPT. Fixup for other sector sizes configurations will be
>> followed up as needed.
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/spansion.c | 132 +++++++++++++++++++++++++++++++++
>>  1 file changed, 132 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c
>> b/drivers/mtd/spi-nor/spansion.c
>> index 60e41e1a9a92..0f5b9e81719f 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -14,6 +14,8 @@
>>  #define SPINOR_OP_CLSR               0x30    /* Clear status register 1 */
>>  #define SPINOR_OP_RD_ANY_REG                 0x65    /* Read any register */
>>  #define SPINOR_OP_WR_ANY_REG                 0x71    /* Write any register */
>> +#define SPINOR_REG_CYPRESS_CFR1V             0x00800002
>> +#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN     BIT(1)  /* Quad Enable */
>>  #define SPINOR_REG_CYPRESS_CFR2V             0x00800003
>>  #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24        0xb
>>  #define SPINOR_REG_CYPRESS_CFR3V             0x00800004
>> @@ -113,6 +115,68 @@ static int cypress_nor_octal_dtr_dis(struct
>> spi_nor *nor)
>>       return 0;
>>  }
>>
>> +/**
>> + * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in
>> volatile
>> + *                                      register.
>> + * @nor:     pointer to a 'struct spi_nor'
>> + *
>> + * It is recommended to update volatile registers in the field
>> application due
>> + * to a risk of the non-volatile registers corruption by power
>> interrupt. This
>> + * function sets Quad Enable bit in CFR1 volatile. If users set the
>> Quad Enable
>> + * bit in the CFR1 non-volatile in advance (typically by a Flash
>> programmer
>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1
>> volatile is
>> + * also set during Flash power-up.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>> +{
>> +     struct spi_mem_op op;
>> +     u8 addr_mode_nbytes = nor->params->addr_mode_nbytes;
>> +     u8 cfr1v_written;
>> +     int ret;
>> +
>> +     op = (struct spi_mem_op)
>> +             CYPRESS_NOR_RD_ANY_REG_OP(addr_mode_nbytes,
>> +                                       SPINOR_REG_CYPRESS_CFR1V,
>> +                                       nor->bouncebuf);
>> +
>> +     ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
>> +             return 0;
>> +
>> +     /* Update the Quad Enable bit. */
>> +     nor->bouncebuf[0] |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
>> +     op = (struct spi_mem_op)
>> +             CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
>> +                                       SPINOR_REG_CYPRESS_CFR1V, 1,
>> +                                       nor->bouncebuf);
>> +     ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
>> +     if (ret)
>> +             return ret;
>> +
>> +     cfr1v_written = nor->bouncebuf[0];
>> +
>> +     /* Read back and check it. */
>> +     op = (struct spi_mem_op)
>> +             CYPRESS_NOR_RD_ANY_REG_OP(addr_mode_nbytes,
>> +                                       SPINOR_REG_CYPRESS_CFR1V,
>> +                                       nor->bouncebuf);
>> +     ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (nor->bouncebuf[0] != cfr1v_written) {
>> +             dev_err(nor->dev, "CFR1: Read back test failed\n");
>> +             return -EIO;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  /**
>>   * cypress_nor_set_page_size() - Set page size which corresponds to
>> the flash
>>   *                               configuration.
>> @@ -143,6 +207,58 @@ static int cypress_nor_set_page_size(struct
>> spi_nor *nor)
>>       return 0;
>>  }
>>
>> +static int
>> +s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>> +                     const struct sfdp_parameter_header *bfpt_header,
>> +                     const struct sfdp_bfpt *bfpt)
>> +{
>> +     /* Replace Quad Enable with volatile version */
>> +     nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>> +
>> +     return cypress_nor_set_page_size(nor);
>> +}
>> +
>> +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
>> +{
>> +     struct spi_nor_erase_type *erase_type =
>> +                                     nor->params->erase_map.erase_type;
>> +     int i;
>> +
>> +     /*
>> +      * In some parts, 3byte erase opcodes are advertised by 4BAIT.
>> +      * Convert them to 4byte erase opcodes.
>> +      */
>> +     for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
>> +             switch (erase_type[i].opcode) {
>> +             case SPINOR_OP_SE:
>> +                     erase_type[i].opcode = SPINOR_OP_SE_4B;
>> +                     break;
>> +             case SPINOR_OP_BE_4K:
>> +                     erase_type[i].opcode = SPINOR_OP_BE_4K_4B;
>> +                     break;
>> +             default:
>> +                     break;
>> +             }
>> +     }
>> +}
>> +
>> +static void s25hx_t_late_init(struct spi_nor *nor)
>> +{
>> +     struct spi_nor_flash_parameter *params = nor->params;
>> +
>> +     /* Fast Read 4B requires mode cycles */
>> +     params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>> +
>> +     /* The writesize should be ECC data unit size */
>> +     params->writesize = 16;
>> +}
>> +
>> +static struct spi_nor_fixups s25hx_t_fixups = {
>> +     .post_bfpt = s25hx_t_post_bfpt_fixup,
>> +     .post_sfdp = s25hx_t_post_sfdp_fixup,
>> +     .late_init = s25hx_t_late_init,
>> +};
>> +
>>  /**
>>   * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress
>> flashes.
>>   * @nor:             pointer to a 'struct spi_nor'
>> @@ -319,6 +435,22 @@ static const struct flash_info
>> spansion_nor_parts[] = {
>>       { "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512)
>>               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>>               FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
>> +     { "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256)
> 
> I know I'm really late, but would this also work with
> { "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 0, 0)
>                PARSE_SFDP
> 
> It seems the former patch will figure out the page size anyway.

That's sector_size and n_sectors. Probably works, but we'd have
to change it anyway when the SNOR_ID3 patch is integrated. Plus, it may
confuse people as using zero values for these parameters will make
BP protection fail. I'd queue this as it is, and we'll ping Takahiro to
give us a Tested-by tag when converting all these to SNOR_ID3.

If it sounds fair to you, I'm happy to receive your R-b tag.

Thanks again for taking the time to review these.
-- 
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/