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 16:00, Tudor.Ambarus@microchip.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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;

here should have been unsigned 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

maybe SFDP_INFO and SFDP_INFO6 instead of SNOR_ID3?

> 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/


-- 
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/