Re: [PATCH] mtd: spi-nor: spansion: Add support for Infineon S25FS256T

[Takahiro Kuwano <tkuw584924@gmail.com>]

On 12/19/2022 4:29 PM, Michael Walle wrote:
> Hi,
> 
Hi Michael,

> Am 2022-12-19 02:55, schrieb tkuw584924@gmail.com:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Infineon S25FS256T is 256Mbit Quad SPI NOR flash. The key features and
>> differences comparing to other Spansion/Cypress flash familes are:
>>   - 4-byte address mode by factory default
>>   - Quad mode is enabled by factory default
>>   - Supports mixture of 128KB and 64KB sectors by OTP configuration
>>     (this patch supports uniform 128KB only due to complexity of
>>      non-uniform layout)
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Datasheet:
>> fileId=8ac78c8c80027ecd0180740c5a46707https://www.infineon.com/dgdlac/Infineon-S25FS256T_256Mb_SEMPER_Nano_Flash_Quad_SPI_1.8V-DataSheet-v12_00-EN.pdf?a
>>
>> zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
>> s25fs256t
>> zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
>> 342b190f0890
>> zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
>> spansion
>> zynq> xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
>> 53464450080101ff00000114000100ff84000102500100ffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> ffffffffffffffffffffffffffffffffe7ffe2ffffffff0f48eb086bffff
>> ffffeeffffffffff00ffffff00ff11d810d800ff00ff321cfeff71e9ffe1
>> ec031c607a757a75f766805c00d65dfff938c0a100000000000000000000
>> 0000ffffffff710600fedcdcffff
>> zynq> md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
>> 13ecce2f195c4c71648e90d4a7e4a0df  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
>> zynq> test_qspi.sh
>> random: crng init done
>> 6+0 records in
>> 6+0 records out
>> 6291456 bytes (6.0MB) copied, 2.787435 seconds, 2.2MB/s
>> Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
>> Erased 6291456 bytes from address 0x00000000 in flash
>> Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
>> 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
>> *
>> 0600000
>> Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
>> Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
>> 2c6e12c0a2346cab755ef72cdf2a72f827241d35  qspi_test
>> 2c6e12c0a2346cab755ef72cdf2a72f827241d35  qspi_read
>> ---
>>  drivers/mtd/spi-nor/spansion.c | 64 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index b621cdfd506f..7d1d2c27ad71 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -24,6 +24,7 @@
>>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN    0x3
>>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS    0
>>  #define SPINOR_OP_CYPRESS_RD_FAST        0xee
>> +#define SPINOR_REG_CYPRESS_ARCFN        0x00000006
>>
>>  /* Cypress SPI NOR flash operations. */
>>  #define CYPRESS_NOR_WR_ANY_REG_OP(naddr, addr, ndata, buf)        \
>> @@ -213,6 +214,66 @@ static int cypress_nor_set_page_size(struct spi_nor *nor)
>>      return 0;
>>  }
>>
>> +static int
>> +s25fs256t_post_bfpt_fixup(struct spi_nor *nor,
>> +              const struct sfdp_parameter_header *bfpt_header,
>> +              const struct sfdp_bfpt *bfpt)
>> +{
>> +    struct spi_mem_op op;
>> +    int ret;
>> +
>> +    /* 4-byte address mode is enabled by default */
>> +    nor->params->addr_mode_nbytes = 4;
> 
> Shouldn't this already be set in spi_nor_parse_4bait()?
> 
No, params->addr_mode_nbytes is initialized in spi_nor_parse_bfpt().
In spi_nor_parse_4bait(), params->addr_nbytes is set to 4.

>> +
>> +    /* Read Architecture Configuration Register (ARCFN) */
>> +    op = (struct spi_mem_op)
>> +        CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_mode_nbytes,
>> +                      SPINOR_REG_CYPRESS_ARCFN,
>> +                      nor->bouncebuf);
>> +    op.dummy.nbytes = 1;
>> +    ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* ARCFN value must be 0 if uniform sector is selected  */
>> +    if (nor->bouncebuf[0])
>> +        return -EOPNOTSUPP;
> 
> EOPNOTSUPP is wrong here. I'd say ENODEV.
> 
OK.

>> +
>> +    return cypress_nor_set_page_size(nor);
>> +}
>> +
>> +static void s25fs256t_post_sfdp_fixup(struct spi_nor *nor)
>> +{
>> +    struct spi_nor_flash_parameter *params = nor->params;
>> +
>> +    /*
>> +     * READ_FAST is omitted in 4BAIT parse since OP_READ_FAST_4B(0Ch) is not
>> +     * supported. Enable OP_READ_FAST(0Bh) that can work in 4-byte address
>> +     * mode.
>> +     */
>> +    params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
>> +    spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST], 0, 8,
>> +                  SPINOR_OP_READ_FAST, SNOR_PROTO_1_1_1);
> 
> Mh, this requires mode switching, the advantage of the opcodes in
> the 4bait table are that they don't require mode switching.
> OP_READ_FAST doesn't work here if the address mode is set to 3. (I
> know this flash defaults to 1, but there is also a non-volatile
> setting for this).
> 
> Regarding mode switching, I guess this is wrong for this flash, because
> it is set to spansion_set_4byte_addr_mode() by default while it should
> really be set to spi_nor_set_4byte_addr_mode().
> 
Let me just drop fast read support so far and mention this in commit
description.

> Also I'm not sure when set_4byte_addr_mode() is called during init.
> It seems slightly wrong to me because it will check wether
> SNOR_F_4B_OPCODES is set. But in the restore path, it is checked for
> !SNOR_F_4B_OPCODES before 3 byte mode is enabled again. Mhh.
> 
>> +
>> +    /* PP_1_1_4_4B is supported but missing in SFDP. */
>> +    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);
>> +}
>> +
>> +static void s25fs256t_late_init(struct spi_nor *nor)
>> +{
>> +    /* The writesize should be ECC data unit size */
>> +    nor->params->writesize = 16;
> 
> The datasheets mentions, a PP should be either 128 or 256 (for best
> performance). So why 16?
> 
writesize is used as minimum IO size in UBI.
Please see:
https://lore.kernel.org/linux-mtd/20201118182459.18197-1-p.yadav@ti.com/

>> +}
>> +
>> +static struct spi_nor_fixups s25fs256t_fixups = {
>> +    .post_bfpt = s25fs256t_post_bfpt_fixup,
>> +    .post_sfdp = s25fs256t_post_sfdp_fixup,
>> +    .late_init = s25fs256t_late_init,
>> +};
>> +
>>  static int
>>  s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>>              const struct sfdp_parameter_header *bfpt_header,
>> @@ -441,6 +502,9 @@ 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) },
>> +    { "s25fs256t",  INFO6(0x342b19, 0x0f0890, 128 * 1024, 256)
> 
> Does INFO6(0x342b19, 0x0f0890, 0, 0) work for you?
> 
Yes, it works.

>> +        PARSE_SFDP
>> +        .fixups = &s25fs256t_fixups },
>>      { "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256)
>>          PARSE_SFDP
>>          MFR_FLAGS(USE_CLSR)

Thanks,
Takahiro

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