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

Sun Feb 26 11:34:28 PST 2023

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.

Cheers,
ta