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

[Tudor Ambarus]

On 26.02.2023 21:34, Tudor Ambarus wrote:
> Hi, Takahiro,
> 
> On 03.02.2023 06:48, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano at 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
>>    - OP_READ_FAST_4B(0Ch) is not supported
>>    - 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.
>>
>> Link: 
>> https://www.infineon.com/dgdlac/Infineon-S25FS256T_256Mb_SEMPER_Nano_Flash_Quad_SPI_1.8V-DataSheet-v12_00-EN.pdf?fileId=8ac78c8c80027ecd0180740c5a46707a
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>> ---
>> Changes in v2:
>>    - s/EOPNOTSUPP/ENODEV
>>    - Drop READ_FAST support
>>    - Amend comment in late_init()
>>    - sector size and page size as 0 in sizeINFO6()
>>
>> ---
>> 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.025928 seconds, 3.0MB/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
>> 09ed7922931fe10795adc0ba3aff4efd2127be02  qspi_test
>> 09ed7922931fe10795adc0ba3aff4efd2127be02  qspi_read
>> ---
>>   drivers/mtd/spi-nor/spansion.c | 60 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c 
>> b/drivers/mtd/spi-nor/spansion.c
>> index 12a256c0ef4c..a499ce393454 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -29,6 +29,7 @@
>>        SPINOR_REG_CYPRESS_CFR5_OPI)
>>   #define SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS    
>> SPINOR_REG_CYPRESS_CFR5_BIT6
>>   #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)        \
>> @@ -218,6 +219,62 @@ 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;
> 
> you'll need to set params->addr_nbytes = 4; as well, even if it is set
> in parse_4bait(). From post_bfpt() to parse_4bait() you're uncovered.
> Also, 4bait is optional and if it fails, we just continue, so you'll end
> up with a wrong configuration. Please update.
> 
>> +
>> +    /* 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;
> 
>  From the datasheet I see that there are two flavors of the read any
> register command, one for volatile registers which does not require
> latency cycles and one for non volatile registers which needs latency
> cycles. Please rename CYPRESS_NOR_RD_ANY_REG_OP to
> CYPRESS_NOR_RD_ANY_VREG_OP and introduce a CYPRESS_NOR_RD_ANY_NVREG_OP
> which will handle the latency cycles.
> 
> Also you should go with the maximum number of latency cycles if you want
> to be able to operate this flash at its maximum frequency, see Table 43
> from the datasheet.
> 
> BTW, I hate the dummy.nbytes nonsense, there's no such thing as
> dummy/latency number of bytes. This should have been defined as latency
> number of cycles. Sergiu Moga had a patch addressing this in MTD and SPI
> but I don't think he's going to send a new version. If there are no
> other volunteers I'll respin it myself.
> 
>> +    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 -ENODEV;
>> +
>> +    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;
>> +
>> +    /* PP_1_1_4_4B is supported but missing in SFDP. */
> 
> missing in the 4-Byte Address Table you mean, right?
> 
> Overall the patch is good. I'd like to address the non volatile byte
> address mode if possible, but it's not a strong requirement. I'll read
> again the datasheet tomorrow. The problem is that even if the flash has
> a default 4 byte address mode (factory default), users can change the
> address mode to 3 by writing to the non-volatile configuration register
> and we'll end up with an unusable flash because as of now we assume the
> flash is in the 4 byte address mode state. I'd like to determine the
> address mode at runtime if possible. For this flash we can't use your 
> CRC idea to determine the mode, as the flash does not have such support.
> I see in the datasheet that "RDARG_C_0 transaction can be used during 
> embedded operations to read Status Register 1 (STR1V)." RDSR1_0_0 does
> not need any address, so an idea is to use RDARG_3_0, RDARG_4_0 and
> RDSR1_0_0, compare what we receive and determine the mode. If we're
> devilish, we can also do a WREN, change a bit in SR and reread all to 
> double verify the result, then a WRDIS.
> Other idea is to force 4 byte address mode by default using EN4BA_0_0,
> this command does not need address bytes. Not the most elegant method
> though, but it should work.
> Again, I'd like to re-read the datasheet. If you have other idea, shoot.
> 

I've re-read the datasheet I couldn't find a better way to determine the
address mode. In these conditions, I would do a hybrid approach of the
above two methods:
I would compare SR1 data by issuing RDSR1, RDARG_3_0, RDARG_4_0. If
there's no match or all have the same value issue EN4BA_0_0 and return.
If there's a match, do WREN and read all again and compare, then WRDIS. 
If there's no match or all are the same issue EN4BA_0_0 and return.
Compare the address mode determined at the first iteration with the
address mode determined at the second (with WREN). If there's no match,
issue EN4BA_0_0 and return. If there is a match, save the determined 
address mode and return.

No need to implement this right now, we can work in an incremental
fashion. Please send a v3 addressing the other comments and then please
try to solve this as well. With this you'll have a working flash even if
the address mode was changed to 3, and we'll stop making assumptions on
the address mode.

Cheers,
ta