[PATCH v11 0/3] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t

[PATCH v11 0/3] mtd: spi-nor: Add support for Infineon s25hl-t/s25hs-t

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.

The datasheets can be found in the following link.
https://www.infineon.com/dgdl/Infineon-S25HS256T_S25HS512T_S25HS01GT_S25HL256T_S25HL512T_S25HL01GT_256-Mb_(32-MB)_512-Mb_(64-MB)_1-Gb_(128-MB)_HS-T_(1.8-V)_HL-T_(3.0-V)_Semper_Flash_with_Quad_SPI-DataSheet-v02_00-EN.pdf?fileId=8ac78c8c7d0d8da4017d0ee674b86ee3&da=t

Device ID and SFDP dumps:
------------------------------------------------------------
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
s25hl512t
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
342a1a0f0390
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
spansion
zynq> xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450080103ff00000114000100ff84000102500100ff81000116c801
00ff8700011c580100ffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffe720faffffffff1f48eb086b00ff
88bbfeffffffffff00ffffff48eb0c2000ff00ff12d823faff8b82e7ffe3
ec031c608a857a75f766805c8cd6ddfff938f8a1000000000000bc000000
0000f7f5ffff7b920ffe21ffffdc0000800000000000c0ffc3ebc8ffe3eb
00650090060500a10065009600650095716503d0716503d000000000b02e
000088a489aa716503967165039600000000000000000000000000000000
000000000000000000000000000000000000000000000000716505d57165
05d50000a015fc65ff0804008000fc65ff4002008000fd65ff0402008000
fe0002fff1ff0100f8ff0100f8fffb03fe0302fff8fffb03f8ff0100f1ff
0100fe0104fff1ff0000f8ff0200f8fff703f8ff0200f1ff0000ff0400ff
f8ffff03
zynq> md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
8a0aa90112e154ae3a797df2c211ef61  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
------------------------------------------------------------

------------------------------------------------------------
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
s25hl01gt
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
342a1b0f0390
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
spansion
zynq> xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450080103ff00000114000100ff84000102500100ff81000116c801
00ff8700011c580100ffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffe720faffffffff3f48eb086b00ff
88bbfeffffffffff00ffffff48eb0c2000ff00ff12d823faff8b82e7ffe6
ec031c608a857a75f766805c8cd6ddfff938f8a1000000000000bc000000
0000f7f5ffff7b920ffe21ffffdc0000800000000000c0ffc3ebc8ffe3eb
00650090060500a10065009600650095716503d0716503d000000000b02e
000088a489aa716503967165039600000000000000000000000000000000
000000000000000000000000000000000000000000000000716505d57165
05d50000a015fc65ff0804008000fc65ff4002008000fd65ff0402008000
fe0002fff1ff0100f8ff0100f8fffb07fe0302fff8fffb07f8ff0100f1ff
0100fe0104fff1ff0000f8ff0200f8fff707f8ff0200f1ff0000ff0400ff
f8ffff07
zynq> md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
1ad5a0d7d7e0e656986c1e678c416a7e  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
------------------------------------------------------------

------------------------------------------------------------
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
s25hs512t
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
342b1a0f0390
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
spansion
zynq> xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450080103ff00000114000100ff84000102500100ff81000116c801
00ff8700011c580100ffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffe720faffffffff1f48eb086b00ff
88bbfeffffffffff00ffffff48eb0c2000ff00ff12d823faff8b91e8ffe3
ec031c608a857a75f766805c84d6ddfff938f8a100000000000080000000
0000f7f5ffff7b920ffe20ffffd80000800000000000c0ffc3ebc8ffe3eb
00650090060500a10065009600650095716503d0716503d000000000b02e
000088a489aa716503967165039600000000000000000000000000000000
000000000000000000000000000000000000000000000000716505d57165
05d50000ee72fc65ff0804008000fc65ff4002008000fd65ff0402008000
fe0002fff1ff0100f8ff0100f8fffb03fe0302fff8fffb03f8ff0100f1ff
0100fe0104fff1ff0100f8ff0200f8fff703f8ff0200f1ff0100ff0400ff
f8ffff03
zynq> md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
f17d9e784602187a0933edec3688e30f  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
------------------------------------------------------------

------------------------------------------------------------
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
s25hs01gt
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
342b1b0f0390
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
spansion
zynq> xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450080103ff00000114000100ff84000102500100ff81000116c801
00ff8700011c580100ffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffe720faffffffff3f48eb086b00ff
88bbfeffffffffff00ffffff48eb0c2000ff00ff12d823faff8b82e7ffe6
ec031c608a857a75f766805c8cd6ddfff938f8a1000000000000bc000000
0000f7f5ffff7b920ffe21ffffdc0000800000000000c0ffc3ebc8ffe3eb
00650090060500a10065009600650095716503d0716503d000000000b02e
000088a489aa716503967165039600000000000000000000000000000000
000000000000000000000000000000000000000000000000716505d57165
05d50000a015fc65ff0804008000fc65ff4002008000fd65ff0402008000
fe0002fff1ff0100f8ff0100f8fffb07fe0302fff8fffb07f8ff0100f1ff
0100fe0104fff1ff0000f8ff0200f8fff707f8ff0200f1ff0000ff0400ff
f8ffff07
zynq> md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
1ad5a0d7d7e0e656986c1e678c416a7e  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
------------------------------------------------------------


---
Changes in v11:
  - Rebase on top of Tudor's series
    https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
  - Remove 'nor->info->addr_width for SMPT parse' patch
  
Changes in v10:
  - Rebase to v5.18-rc1
  - Remove dependencies on other series
  - Use nor->info->addr_width for SMPT parse
  - Add a local function for page size discovery
  - Clean up volatile QE function
  
Changes in v9:
  - Rebase to v5.17-rc6
  - Rename function and macro per mwalle's series
  - Fix some issues in ID table and fixup hook

Changes in v8:
  - Rebase to v5.17-rc4
  - Use spi_nor_read_reg and spi_nor_write_reg()
  
Changes in v7:
  - Some changes were missing in v6 patch. Fix it

Changes in v6:
  - Remove 2Gb dual die package parts and related changes to split mulit
    die package support into another series of patches  

Changes in v5:
  - Fix 'if (ret == 1)' to 'if (ret < 0)' in spansion_read_any_reg()
  - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT

Changes in v4:
  - Reword 'legacy' to 'default'
  - Rename spi_nor_read() to spi_nor_default_ready()
  - Fix dummy cycle calculation in spansion_read_any_reg()
  - Modify comment for spansion_write_any_reg()
  - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
  - Remove USE_CLSR flags from S25HL02GT and S25HS02GT

Changes in v3:
  - Split into multiple patches
  - Remove S25HL256T and S25HS256T
  - Add S25HL02GT and S25HS02GT 
  - Add support for multi-die package parts support
  - Cleanup Read/Write Any Register implementation
  - Remove erase_map fix for top/split sector layout
  - Set ECC data unit size (16B) to writesize 

Changes in v2:
  - Remove SPI_NOR_SKIP_SFDP flag and clean up related fixups
  - Check CFR3V[4] to determine page_size instead of force 512B
  - Depend on the patchset below to support non-uniform sector layout
    https://lore.kernel.org/linux-mtd/cover.1601612872.git.Takahiro.Kuwano@infineon.com/

Takahiro Kuwano (3):
  mtd: spi-nor: spansion: Add support for volatile QE  bit
  mtd: spi-nor: spansion: Add local function to  discover page size
  mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and  fixups

 drivers/mtd/spi-nor/spansion.c | 168 ++++++++++++++++++++++++++++-----
 1 file changed, 146 insertions(+), 22 deletions(-)

-- 
2.25.1


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

[PATCH v11 1/3] mtd: spi-nor: spansion: Add support for volatile QE bit

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

Some of 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.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v11:
  - Rebase on top of Tudor's series
    https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
  
Changes in v10:
  - Remove dependencies on other series
  
Changes in v9:
  - Rename function per mwalle's series
  
Changes in v8:
  - Use spi_nor_read/write_reg() functions
  - Use nor->bouncebuf instead of a variable on stack
  
Changes in v7:
  - Add missing macro definitions in v6
  
Changes in v6:
  - Remove multi die package support

Changes in v5:
  - No change
  
Changes in v4:
  - No change
  
Changes in v3:
  - Add multi-die package parts support

 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 952db7af6932..6bcd25180af4 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
@@ -117,6 +119,64 @@ 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;
+	u32 reg_addr = SPINOR_REG_CYPRESS_CFR1V;
+	u8 cfr1v_written;
+	int ret;
+
+	op = (struct spi_mem_op)
+		CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width, reg_addr,
+					  nor->bouncebuf);
+	ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
+	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(nor->addr_width, reg_addr, 1,
+					  nor->bouncebuf);
+	ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_1_1_1);
+	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(nor->addr_width, reg_addr,
+					  nor->bouncebuf);
+	ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
+	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_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
-- 
2.25.1


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

[PATCH v11 2/3] mtd: spi-nor: spansion: Add local function to discover page size

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

The page size check in s28hs512t fixup can be used for s25hs/hl-t as well.
Move that to a newly created local function.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v11:
  - Rebase on top of Tudor's series
    https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
  - Add addr_width param

 drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 6bcd25180af4..493240ebfd70 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -177,6 +177,37 @@ static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
 	return 0;
 }
 
+/**
+ * cypress_nor_set_page_size() - Set page size which corresponds to the flash
+ *                               configuration.
+ * @nor:	pointer to a 'struct spi_nor'
+ * @addr_width:	address width used in Read Any Register op
+ *
+ * The BFPT table advertises a 512B or 256B page size depending on part but the
+ * page size is actually configurable (with the default being 256B). Read from
+ * CFR3V[4] and set the correct size.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
+{
+	struct spi_mem_op op =
+		CYPRESS_NOR_RD_ANY_REG_OP(addr_width, SPINOR_REG_CYPRESS_CFR3V,
+					  nor->bouncebuf);
+	int ret;
+
+	ret = spi_nor_read_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
+
+	if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
+		nor->params->page_size = 512;
+	else
+		nor->params->page_size = 256;
+
+	return 0;
+}
+
 /**
  * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
@@ -231,28 +262,7 @@ static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
 				     const struct sfdp_parameter_header *bfpt_header,
 				     const struct sfdp_bfpt *bfpt)
 {
-	/*
-	 * The BFPT table advertises a 512B page size but the page size is
-	 * actually configurable (with the default being 256B). Read from
-	 * CFR3V[4] and set the correct size.
-	 */
-	struct spi_mem_op op =
-		CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR3V,
-					  nor->bouncebuf);
-	int ret;
-
-	spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-	ret = spi_mem_exec_op(nor->spimem, &op);
-	if (ret)
-		return ret;
-
-	if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
-		nor->params->page_size = 512;
-	else
-		nor->params->page_size = 256;
-
-	return 0;
+	return cypress_nor_set_page_size(nor, 3);
 }
 
 static const struct spi_nor_fixups s28hs512t_fixups = {
-- 
2.25.1


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

[PATCH v11 3/3] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups

[tkuw584924]

From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.

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>
---
Changes in v11:
  - Cleanup fixups based on other patches in this series
  
Changes in v10:
  - Cleanup fixups and ID table based on other patches in this series

Changes in v9:
  - Use late_init() hook to fix mode clocks and writesize
  - Use PARSE_SFDP instead of NO_SFDP_FLAGS
  - Use MFR_FLAGS for USE_CLSR
  - Add comment block to explain about addr mode in post_bfpt_fixups()

Changes in v8:
  - Call write_disable in error case only
  - Use spi_nor_read_reg() helper
  - Use nor->bouncebuf instead of variable on stack
  - Update ID table to use FLAGS macro
  
Changes in v7:
  - Add missing device info table in v6
  
Changes in v6:
  - Remove 2Gb multi die pacakge support

Changes in v5:
  - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT

Changes in v4:
  - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
  - Remove USE_CLSR flags from S25HL02GT and S25HS02GT

Changes in v3:
  - Remove S25HL256T and S25HS256T
  - Add S25HL02GT and S25HS02GT 
  - Add support for multi-die package parts support
  - Remove erase_map fix for top/split sector layout
  - Set ECC data unit size (16B) to writesize

 drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 493240ebfd70..dd37b829efbc 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
 	return 0;
 }
 
+static int
+s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
+			 const struct sfdp_parameter_header *bfpt_header,
+			 const struct sfdp_bfpt *bfpt)
+{
+	int ret;
+
+	/*
+	 * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
+	 * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
+	 * Read Any Reg op is called in this hook and SMPT parse, we would sync
+	 * Flash's addr mode and nor->addr_width here.
+	 */
+	ret = spi_nor_set_4byte_addr_mode(nor, true);
+	if (ret)
+		return ret;
+	nor->addr_width = 4;
+
+	/* Replace Quad Enable with volatile version */
+	nor->params->quad_enable = cypress_nor_quad_enable_volatile;
+
+	return cypress_nor_set_page_size(nor, nor->addr_width);
+}
+
+void s25hx_t_late_init(struct spi_nor *nor)
+{
+	/* Fast Read 4B requires mode cycles */
+	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+
+	/* The writesize should be ECC data unit size */
+	nor->params->writesize = 16;
+}
+
+static struct spi_nor_fixups s25hx_t_fixups = {
+	.post_bfpt = s25hx_t_post_bfpt_fixups,
+	.late_init = s25hx_t_late_init,
+};
+
 /**
  * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
@@ -384,6 +422,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)
+		PARSE_SFDP
+		MFR_FLAGS(USE_CLSR)
+		.fixups = &s25hx_t_fixups },
+	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512)
+		PARSE_SFDP
+		MFR_FLAGS(USE_CLSR)
+		.fixups = &s25hx_t_fixups },
+	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256)
+		PARSE_SFDP
+		MFR_FLAGS(USE_CLSR)
+		.fixups = &s25hx_t_fixups },
+	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512)
+		PARSE_SFDP
+		MFR_FLAGS(USE_CLSR)
+		.fixups = &s25hx_t_fixups },
 	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1)
 		FLAGS(SPI_NOR_NO_ERASE) },
 	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256)
-- 
2.25.1


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

Re: [PATCH v11 1/3] mtd: spi-nor: spansion: Add support for volatile QE bit

[Tudor.Ambarus]

On 4/18/22 08:41, tkuw584924@gmail.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Some of 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.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v11:
>   - Rebase on top of Tudor's series
>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
> 
> Changes in v10:
>   - Remove dependencies on other series
> 
> Changes in v9:
>   - Rename function per mwalle's series
> 
> Changes in v8:
>   - Use spi_nor_read/write_reg() functions
>   - Use nor->bouncebuf instead of a variable on stack
> 
> Changes in v7:
>   - Add missing macro definitions in v6
> 
> Changes in v6:
>   - Remove multi die package support
> 
> Changes in v5:
>   - No change
> 
> Changes in v4:
>   - No change
> 
> Changes in v3:
>   - Add multi-die package parts support
> 
>  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 952db7af6932..6bcd25180af4 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
> @@ -117,6 +119,64 @@ 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.
> + *

cool

> + * Return: 0 on success, -errno otherwise.
> + */
> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
> +{
> +       struct spi_mem_op op;
> +       u32 reg_addr = SPINOR_REG_CYPRESS_CFR1V;

no need for this local variable, use the define directly

> +       u8 cfr1v_written;
> +       int ret;
> +
> +       op = (struct spi_mem_op)
> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width, reg_addr,
> +                                         nor->bouncebuf);
> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);

can we use nor->reg_proto instead of SNOR_PROTO_1_1_1?

> +       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(nor->addr_width, reg_addr, 1,
> +                                         nor->bouncebuf);
> +       ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_1_1_1);

same

With these addressed:
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> +       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(nor->addr_width, reg_addr,
> +                                         nor->bouncebuf);
> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
> +       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_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:               pointer to a 'struct spi_nor'
> --
> 2.25.1
> 

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

Re: [PATCH v11 2/3] mtd: spi-nor: spansion: Add local function to discover page size

[Tudor.Ambarus]

On 4/18/22 08:41, tkuw584924@gmail.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> The page size check in s28hs512t fixup can be used for s25hs/hl-t as well.
> Move that to a newly created local function.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v11:
>   - Rebase on top of Tudor's series
>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
>   - Add addr_width param
> 
>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 6bcd25180af4..493240ebfd70 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -177,6 +177,37 @@ static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>         return 0;
>  }
> 
> +/**
> + * cypress_nor_set_page_size() - Set page size which corresponds to the flash
> + *                               configuration.
> + * @nor:       pointer to a 'struct spi_nor'
> + * @addr_width:        address width used in Read Any Register op
> + *
> + * The BFPT table advertises a 512B or 256B page size depending on part but the
> + * page size is actually configurable (with the default being 256B). Read from
> + * CFR3V[4] and set the correct size.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
> +{
> +       struct spi_mem_op op =
> +               CYPRESS_NOR_RD_ANY_REG_OP(addr_width, SPINOR_REG_CYPRESS_CFR3V,
> +                                         nor->bouncebuf);
> +       int ret;
> +
> +       ret = spi_nor_read_reg(nor, &op, nor->reg_proto);
> +       if (ret)
> +               return ret;
> +
> +       if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
> +               nor->params->page_size = 512;
> +       else
> +               nor->params->page_size = 256;
> +
> +       return 0;
> +}
> +
>  /**
>   * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:               pointer to a 'struct spi_nor'
> @@ -231,28 +262,7 @@ static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
>                                      const struct sfdp_parameter_header *bfpt_header,
>                                      const struct sfdp_bfpt *bfpt)
>  {
> -       /*
> -        * The BFPT table advertises a 512B page size but the page size is
> -        * actually configurable (with the default being 256B). Read from
> -        * CFR3V[4] and set the correct size.
> -        */
> -       struct spi_mem_op op =
> -               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR3V,
> -                                         nor->bouncebuf);
> -       int ret;
> -
> -       spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> -
> -       ret = spi_mem_exec_op(nor->spimem, &op);
> -       if (ret)
> -               return ret;
> -
> -       if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
> -               nor->params->page_size = 512;
> -       else
> -               nor->params->page_size = 256;
> -
> -       return 0;
> +       return cypress_nor_set_page_size(nor, 3);

not related to this patch, but can we use nor->add_width instead of 3?

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

>  }
> 
>  static const struct spi_nor_fixups s28hs512t_fixups = {
> --
> 2.25.1
> 

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

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

[Tudor.Ambarus]

On 4/18/22 08:41, tkuw584924@gmail.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
> 
> 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>
> ---
> Changes in v11:
>   - Cleanup fixups based on other patches in this series
> 
> Changes in v10:
>   - Cleanup fixups and ID table based on other patches in this series
> 
> Changes in v9:
>   - Use late_init() hook to fix mode clocks and writesize
>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>   - Use MFR_FLAGS for USE_CLSR
>   - Add comment block to explain about addr mode in post_bfpt_fixups()
> 
> Changes in v8:
>   - Call write_disable in error case only
>   - Use spi_nor_read_reg() helper
>   - Use nor->bouncebuf instead of variable on stack
>   - Update ID table to use FLAGS macro
> 
> Changes in v7:
>   - Add missing device info table in v6
> 
> Changes in v6:
>   - Remove 2Gb multi die pacakge support
> 
> Changes in v5:
>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
> 
> Changes in v4:
>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
> 
> Changes in v3:
>   - Remove S25HL256T and S25HS256T
>   - Add S25HL02GT and S25HS02GT
>   - Add support for multi-die package parts support
>   - Remove erase_map fix for top/split sector layout
>   - Set ECC data unit size (16B) to writesize
> 
>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 493240ebfd70..dd37b829efbc 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>         return 0;
>  }
> 
> +static int
> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
> +                        const struct sfdp_parameter_header *bfpt_header,
> +                        const struct sfdp_bfpt *bfpt)
> +{
> +       int ret;
> +
> +       /*
> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync

Hi, Takahiro,

I would like some details, please.
1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?

2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
Reg op used. Why do you need an addr_width of value 4 in SMPT parse?


> +        * Flash's addr mode and nor->addr_width here.
> +        */
> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
> +       if (ret)
> +               return ret;
> +       nor->addr_width = 4;
> +
> +       /* Replace Quad Enable with volatile version */
> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
> +
> +       return cypress_nor_set_page_size(nor, nor->addr_width);
> +}
> +
> +void s25hx_t_late_init(struct spi_nor *nor)
> +{
> +       /* Fast Read 4B requires mode cycles */
> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;

Isn't this info already handled in BFPT? What value of num_mode_clocks
do you obtain from BFPT for the non-4B opcode?

> +
> +       /* The writesize should be ECC data unit size */
> +       nor->params->writesize = 16;
> +}
> +
> +static struct spi_nor_fixups s25hx_t_fixups = {
> +       .post_bfpt = s25hx_t_post_bfpt_fixups,
> +       .late_init = s25hx_t_late_init,
> +};
> +
>  /**
>   * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:               pointer to a 'struct spi_nor'
> @@ -384,6 +422,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)
> +               PARSE_SFDP
> +               MFR_FLAGS(USE_CLSR)
> +               .fixups = &s25hx_t_fixups },
> +       { "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512)
> +               PARSE_SFDP
> +               MFR_FLAGS(USE_CLSR)
> +               .fixups = &s25hx_t_fixups },
> +       { "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256)
> +               PARSE_SFDP
> +               MFR_FLAGS(USE_CLSR)
> +               .fixups = &s25hx_t_fixups },
> +       { "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512)
> +               PARSE_SFDP
> +               MFR_FLAGS(USE_CLSR)
> +               .fixups = &s25hx_t_fixups },

After we establish the details from above, would you please run this test_qspi.sh
script for all the flashes?

#!/bin/sh 
dd if=/dev/urandom of=./qspi_test bs=1M count=6 
mtd_debug write /dev/mtd0 0 6291456 qspi_test 
mtd_debug erase /dev/mtd0 0 6291456 
mtd_debug read /dev/mtd0 0 6291456 qspi_read 
hexdump qspi_read 
mtd_debug write /dev/mtd0 0 6291456 qspi_test 
mtd_debug read /dev/mtd0 0 6291456 qspi_read 
sha1sum qspi_test qspi_read 

The two SHA-1 sums must be the same to pass this test. Send us the output,
please.

thanks,
ta

>         { "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1)
>                 FLAGS(SPI_NOR_NO_ERASE) },
>         { "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256)
> --
> 2.25.1
> 

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

Re: [PATCH v11 1/3] mtd: spi-nor: spansion: Add support for volatile QE bit

[Tudor.Ambarus]

On 4/18/22 08:41, tkuw584924@gmail.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Some of 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.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v11:
>   - Rebase on top of Tudor's series
>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
> 
> Changes in v10:
>   - Remove dependencies on other series
> 
> Changes in v9:
>   - Rename function per mwalle's series
> 
> Changes in v8:
>   - Use spi_nor_read/write_reg() functions
>   - Use nor->bouncebuf instead of a variable on stack
> 
> Changes in v7:
>   - Add missing macro definitions in v6
> 
> Changes in v6:
>   - Remove multi die package support
> 
> Changes in v5:
>   - No change
> 
> Changes in v4:
>   - No change
> 
> Changes in v3:
>   - Add multi-die package parts support
> 
>  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 952db7af6932..6bcd25180af4 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
> @@ -117,6 +119,64 @@ 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;
> +       u32 reg_addr = SPINOR_REG_CYPRESS_CFR1V;
> +       u8 cfr1v_written;
> +       int ret;
> +
> +       op = (struct spi_mem_op)
> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width, reg_addr,
> +                                         nor->bouncebuf);
> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
> +       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(nor->addr_width, reg_addr, 1,
> +                                         nor->bouncebuf);
> +       ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_1_1_1);
> +       if (ret)
> +               return ret;

should you call spi_nor_wait_until_ready() here?

> +
> +       cfr1v_written = nor->bouncebuf[0];
> +
> +       /* Read back and check it. */
> +       op = (struct spi_mem_op)
> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width, reg_addr,
> +                                         nor->bouncebuf);
> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
> +       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_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:               pointer to a 'struct spi_nor'
> --
> 2.25.1
> 

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

Re: [PATCH v11 1/3] mtd: spi-nor: spansion: Add support for volatile QE bit

[Takahiro Kuwano]

On 4/19/2022 9:53 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of 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.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v11:
>>   - Rebase on top of Tudor's series
>>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
>>
>> Changes in v10:
>>   - Remove dependencies on other series
>>
>> Changes in v9:
>>   - Rename function per mwalle's series
>>
>> Changes in v8:
>>   - Use spi_nor_read/write_reg() functions
>>   - Use nor->bouncebuf instead of a variable on stack
>>
>> Changes in v7:
>>   - Add missing macro definitions in v6
>>
>> Changes in v6:
>>   - Remove multi die package support
>>
>> Changes in v5:
>>   - No change
>>
>> Changes in v4:
>>   - No change
>>
>> Changes in v3:
>>   - Add multi-die package parts support
>>
>>  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 952db7af6932..6bcd25180af4 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
>> @@ -117,6 +119,64 @@ 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;
>> +       u32 reg_addr = SPINOR_REG_CYPRESS_CFR1V;
>> +       u8 cfr1v_written;
>> +       int ret;
>> +
>> +       op = (struct spi_mem_op)
>> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width, reg_addr,
>> +                                         nor->bouncebuf);
>> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
>> +       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(nor->addr_width, reg_addr, 1,
>> +                                         nor->bouncebuf);
>> +       ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_1_1_1);
>> +       if (ret)
>> +               return ret;
> 
> should you call spi_nor_wait_until_ready() here?
> 
No. For volatile register write, it finishes immediately and the device
does not get in busy state. Reading back and verifying the register bit
is good enough.

>> +
>> +       cfr1v_written = nor->bouncebuf[0];
>> +
>> +       /* Read back and check it. */
>> +       op = (struct spi_mem_op)
>> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width, reg_addr,
>> +                                         nor->bouncebuf);
>> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
>> +       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_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:               pointer to a 'struct spi_nor'
>> --
>> 2.25.1
>>
> 

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

Re: [PATCH v11 1/3] mtd: spi-nor: spansion: Add support for volatile QE bit

[Takahiro Kuwano]

On 4/19/2022 5:01 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of 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.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v11:
>>   - Rebase on top of Tudor's series
>>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
>>
>> Changes in v10:
>>   - Remove dependencies on other series
>>
>> Changes in v9:
>>   - Rename function per mwalle's series
>>
>> Changes in v8:
>>   - Use spi_nor_read/write_reg() functions
>>   - Use nor->bouncebuf instead of a variable on stack
>>
>> Changes in v7:
>>   - Add missing macro definitions in v6
>>
>> Changes in v6:
>>   - Remove multi die package support
>>
>> Changes in v5:
>>   - No change
>>
>> Changes in v4:
>>   - No change
>>
>> Changes in v3:
>>   - Add multi-die package parts support
>>
>>  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 952db7af6932..6bcd25180af4 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
>> @@ -117,6 +119,64 @@ 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.
>> + *
> 
> cool
> 
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>> +{
>> +       struct spi_mem_op op;
>> +       u32 reg_addr = SPINOR_REG_CYPRESS_CFR1V;
> 
> no need for this local variable, use the define directly
> 
OK.

>> +       u8 cfr1v_written;
>> +       int ret;
>> +
>> +       op = (struct spi_mem_op)
>> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width, reg_addr,
>> +                                         nor->bouncebuf);
>> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
> 
> can we use nor->reg_proto instead of SNOR_PROTO_1_1_1?
> 
Yes, I will replace it.

>> +       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(nor->addr_width, reg_addr, 1,
>> +                                         nor->bouncebuf);
>> +       ret = spi_nor_write_reg(nor, &op, SNOR_PROTO_1_1_1);
> 
> same
> 
> With these addressed:
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
>> +       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(nor->addr_width, reg_addr,
>> +                                         nor->bouncebuf);
>> +       ret = spi_nor_read_reg(nor, &op, SNOR_PROTO_1_1_1);
>> +       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_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:               pointer to a 'struct spi_nor'
>> --
>> 2.25.1
>>
> 
Thank you!
Takahiro

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

Re: [PATCH v11 2/3] mtd: spi-nor: spansion: Add local function to discover page size

[Takahiro Kuwano]

On 4/19/2022 5:06 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> The page size check in s28hs512t fixup can be used for s25hs/hl-t as well.
>> Move that to a newly created local function.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v11:
>>   - Rebase on top of Tudor's series
>>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294490
>>   - Add addr_width param
>>
>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++--------------
>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 6bcd25180af4..493240ebfd70 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -177,6 +177,37 @@ static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>>         return 0;
>>  }
>>
>> +/**
>> + * cypress_nor_set_page_size() - Set page size which corresponds to the flash
>> + *                               configuration.
>> + * @nor:       pointer to a 'struct spi_nor'
>> + * @addr_width:        address width used in Read Any Register op
>> + *
>> + * The BFPT table advertises a 512B or 256B page size depending on part but the
>> + * page size is actually configurable (with the default being 256B). Read from
>> + * CFR3V[4] and set the correct size.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>> +{
>> +       struct spi_mem_op op =
>> +               CYPRESS_NOR_RD_ANY_REG_OP(addr_width, SPINOR_REG_CYPRESS_CFR3V,
>> +                                         nor->bouncebuf);
>> +       int ret;
>> +
>> +       ret = spi_nor_read_reg(nor, &op, nor->reg_proto);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
>> +               nor->params->page_size = 512;
>> +       else
>> +               nor->params->page_size = 256;
>> +
>> +       return 0;
>> +}
>> +
>>  /**
>>   * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:               pointer to a 'struct spi_nor'
>> @@ -231,28 +262,7 @@ static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
>>                                      const struct sfdp_parameter_header *bfpt_header,
>>                                      const struct sfdp_bfpt *bfpt)
>>  {
>> -       /*
>> -        * The BFPT table advertises a 512B page size but the page size is
>> -        * actually configurable (with the default being 256B). Read from
>> -        * CFR3V[4] and set the correct size.
>> -        */
>> -       struct spi_mem_op op =
>> -               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR3V,
>> -                                         nor->bouncebuf);
>> -       int ret;
>> -
>> -       spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
>> -
>> -       ret = spi_mem_exec_op(nor->spimem, &op);
>> -       if (ret)
>> -               return ret;
>> -
>> -       if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
>> -               nor->params->page_size = 512;
>> -       else
>> -               nor->params->page_size = 256;
>> -
>> -       return 0;
>> +       return cypress_nor_set_page_size(nor, 3);
> 
> not related to this patch, but can we use nor->add_width instead of 3?
> 
I confirmed with a s28hs512t device on hand that nor->addr_width is set
to 3 during BFPT parse. We can use nor->addr_width and eliminate addr_width
param from cypress_nor_set_page_size(). I will do it in next rev.

> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
>>  }
>>
>>  static const struct spi_nor_fixups s28hs512t_fixups = {
>> --
>> 2.25.1
>>
> 

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

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

[Takahiro Kuwano]

Hi Tudor,

Thank you for your feedback.

On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>
>> 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>
>> ---
>> Changes in v11:
>>   - Cleanup fixups based on other patches in this series
>>
>> Changes in v10:
>>   - Cleanup fixups and ID table based on other patches in this series
>>
>> Changes in v9:
>>   - Use late_init() hook to fix mode clocks and writesize
>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>   - Use MFR_FLAGS for USE_CLSR
>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>
>> Changes in v8:
>>   - Call write_disable in error case only
>>   - Use spi_nor_read_reg() helper
>>   - Use nor->bouncebuf instead of variable on stack
>>   - Update ID table to use FLAGS macro
>>
>> Changes in v7:
>>   - Add missing device info table in v6
>>
>> Changes in v6:
>>   - Remove 2Gb multi die pacakge support
>>
>> Changes in v5:
>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>
>> Changes in v4:
>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>
>> Changes in v3:
>>   - Remove S25HL256T and S25HS256T
>>   - Add S25HL02GT and S25HS02GT
>>   - Add support for multi-die package parts support
>>   - Remove erase_map fix for top/split sector layout
>>   - Set ECC data unit size (16B) to writesize
>>
>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 493240ebfd70..dd37b829efbc 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>         return 0;
>>  }
>>
>> +static int
>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>> +                        const struct sfdp_parameter_header *bfpt_header,
>> +                        const struct sfdp_bfpt *bfpt)
>> +{
>> +       int ret;
>> +
>> +       /*
>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
> 
> Hi, Takahiro,
> 
> I would like some details, please.
> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
> 
If we are sure that the Flash is in 3-byte address mode, we can use the value 3
for reading CFR3V. However, the Flash's address mode may be changed prior to
Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
address mode. We need to set the Flash's address mode in known state and update
nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
choice.

> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
> 
In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the 
spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
with 4-byte address. We need to set the Flash into 4-byte address mode in advance.

> 
>> +        * Flash's addr mode and nor->addr_width here.
>> +        */
>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>> +       if (ret)
>> +               return ret;
>> +       nor->addr_width = 4;
>> +
>> +       /* Replace Quad Enable with volatile version */
>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>> +
>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>> +}
>> +
>> +void s25hx_t_late_init(struct spi_nor *nor)
>> +{
>> +       /* Fast Read 4B requires mode cycles */
>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> 
> Isn't this info already handled in BFPT? What value of num_mode_clocks
> do you obtain from BFPT for the non-4B opcode?
> 
There is no parameter in BFPT that describes about Fast Read 1-1-1 op
(as we can see 'struct sfdp_bfpt_read' in sfdp.c).
The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
spi_nor_init_default_params().

>> +
>> +       /* The writesize should be ECC data unit size */
>> +       nor->params->writesize = 16;
>> +}
>> +
>> +static struct spi_nor_fixups s25hx_t_fixups = {
>> +       .post_bfpt = s25hx_t_post_bfpt_fixups,
>> +       .late_init = s25hx_t_late_init,
>> +};
>> +
>>  /**
>>   * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:               pointer to a 'struct spi_nor'
>> @@ -384,6 +422,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)
>> +               PARSE_SFDP
>> +               MFR_FLAGS(USE_CLSR)
>> +               .fixups = &s25hx_t_fixups },
>> +       { "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512)
>> +               PARSE_SFDP
>> +               MFR_FLAGS(USE_CLSR)
>> +               .fixups = &s25hx_t_fixups },
>> +       { "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256)
>> +               PARSE_SFDP
>> +               MFR_FLAGS(USE_CLSR)
>> +               .fixups = &s25hx_t_fixups },
>> +       { "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512)
>> +               PARSE_SFDP
>> +               MFR_FLAGS(USE_CLSR)
>> +               .fixups = &s25hx_t_fixups },
> 
> After we establish the details from above, would you please run this test_qspi.sh
> script for all the flashes?
> 
> #!/bin/sh 
> dd if=/dev/urandom of=./qspi_test bs=1M count=6 
> mtd_debug write /dev/mtd0 0 6291456 qspi_test 
> mtd_debug erase /dev/mtd0 0 6291456 
> mtd_debug read /dev/mtd0 0 6291456 qspi_read 
> hexdump qspi_read 
> mtd_debug write /dev/mtd0 0 6291456 qspi_test 
> mtd_debug read /dev/mtd0 0 6291456 qspi_read 
> sha1sum qspi_test qspi_read 
> 
> The two SHA-1 sums must be the same to pass this test. Send us the output,
> please.
> 
OK.

> thanks,
> ta
> 
>>         { "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1)
>>                 FLAGS(SPI_NOR_NO_ERASE) },
>>         { "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256)
>> --
>> 2.25.1
>>
> 
Best Regards,
Takahiro

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

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

[Tudor.Ambarus]

On 4/20/22 08:34, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> Thank you for your feedback.
> 
> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>
>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>
>>> 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>
>>> ---
>>> Changes in v11:
>>>   - Cleanup fixups based on other patches in this series
>>>
>>> Changes in v10:
>>>   - Cleanup fixups and ID table based on other patches in this series
>>>
>>> Changes in v9:
>>>   - Use late_init() hook to fix mode clocks and writesize
>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>   - Use MFR_FLAGS for USE_CLSR
>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>
>>> Changes in v8:
>>>   - Call write_disable in error case only
>>>   - Use spi_nor_read_reg() helper
>>>   - Use nor->bouncebuf instead of variable on stack
>>>   - Update ID table to use FLAGS macro
>>>
>>> Changes in v7:
>>>   - Add missing device info table in v6
>>>
>>> Changes in v6:
>>>   - Remove 2Gb multi die pacakge support
>>>
>>> Changes in v5:
>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>
>>> Changes in v4:
>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>
>>> Changes in v3:
>>>   - Remove S25HL256T and S25HS256T
>>>   - Add S25HL02GT and S25HS02GT
>>>   - Add support for multi-die package parts support
>>>   - Remove erase_map fix for top/split sector layout
>>>   - Set ECC data unit size (16B) to writesize
>>>
>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>  1 file changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>> index 493240ebfd70..dd37b829efbc 100644
>>> --- a/drivers/mtd/spi-nor/spansion.c
>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>         return 0;
>>>  }
>>>
>>> +static int
>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>> +                        const struct sfdp_bfpt *bfpt)
>>> +{
>>> +       int ret;
>>> +
>>> +       /*
>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>
>> Hi, Takahiro,
>>
>> I would like some details, please.
>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>
> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
> for reading CFR3V. However, the Flash's address mode may be changed prior to
> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
> address mode. We need to set the Flash's address mode in known state and update

addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
you'll be sure that the flash is in its default state.

> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
> choice.
> 
>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>
> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to

ok

> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg

if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
then you don't have to do anything for SMPT, right?

> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
> 
>>
>>> +        * Flash's addr mode and nor->addr_width here.
>>> +        */
>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>> +       if (ret)
>>> +               return ret;
>>> +       nor->addr_width = 4;
>>> +
>>> +       /* Replace Quad Enable with volatile version */
>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>> +
>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>> +}
>>> +
>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>> +{
>>> +       /* Fast Read 4B requires mode cycles */
>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>
>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>> do you obtain from BFPT for the non-4B opcode?
>>
> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
> spi_nor_init_default_params().
> 

I see, thanks!
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Takahiro Kuwano]

On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/20/22 08:34, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi Tudor,
>>
>> Thank you for your feedback.
>>
>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>
>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>
>>>> 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>
>>>> ---
>>>> Changes in v11:
>>>>   - Cleanup fixups based on other patches in this series
>>>>
>>>> Changes in v10:
>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>
>>>> Changes in v9:
>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>
>>>> Changes in v8:
>>>>   - Call write_disable in error case only
>>>>   - Use spi_nor_read_reg() helper
>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>   - Update ID table to use FLAGS macro
>>>>
>>>> Changes in v7:
>>>>   - Add missing device info table in v6
>>>>
>>>> Changes in v6:
>>>>   - Remove 2Gb multi die pacakge support
>>>>
>>>> Changes in v5:
>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>
>>>> Changes in v4:
>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>
>>>> Changes in v3:
>>>>   - Remove S25HL256T and S25HS256T
>>>>   - Add S25HL02GT and S25HS02GT
>>>>   - Add support for multi-die package parts support
>>>>   - Remove erase_map fix for top/split sector layout
>>>>   - Set ECC data unit size (16B) to writesize
>>>>
>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 54 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>> index 493240ebfd70..dd37b829efbc 100644
>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>         return 0;
>>>>  }
>>>>
>>>> +static int
>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>> +                        const struct sfdp_bfpt *bfpt)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       /*
>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>
>>> Hi, Takahiro,
>>>
>>> I would like some details, please.
>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>
>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>> address mode. We need to set the Flash's address mode in known state and update
> 
> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
> you'll be sure that the flash is in its default state.
> 
Resetting the Flash to revert back to default state should work for this. However,
we still have SMPT issue below.

>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>> choice.
>>
>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>
>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
> 
> ok
> 
>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
> 
> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
> then you don't have to do anything for SMPT, right?
> 
Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
address mode (default), SMPT parse does not work correctly.

Please see this about 4BAIT and SMPT issue.
https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/

I introduced another workaround for SMPT issue in my previous series.
https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
I withdrew this because this does not work if Flash address mode is not default state.
But if we can reset the Flash at probe, this should work.


>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>
>>>
>>>> +        * Flash's addr mode and nor->addr_width here.
>>>> +        */
>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +       nor->addr_width = 4;
>>>> +
>>>> +       /* Replace Quad Enable with volatile version */
>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>> +
>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>> +}
>>>> +
>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>> +{
>>>> +       /* Fast Read 4B requires mode cycles */
>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>
>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>> do you obtain from BFPT for the non-4B opcode?
>>>
>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>> spi_nor_init_default_params().
>>
> 
> I see, thanks!

Thanks,
Takahiro

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

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

[Tudor.Ambarus]

On 4/20/22 09:58, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Hi Tudor,
>>>
>>> Thank you for your feedback.
>>>
>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>
>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>
>>>>> 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>
>>>>> ---
>>>>> Changes in v11:
>>>>>   - Cleanup fixups based on other patches in this series
>>>>>
>>>>> Changes in v10:
>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>
>>>>> Changes in v9:
>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>
>>>>> Changes in v8:
>>>>>   - Call write_disable in error case only
>>>>>   - Use spi_nor_read_reg() helper
>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>   - Update ID table to use FLAGS macro
>>>>>
>>>>> Changes in v7:
>>>>>   - Add missing device info table in v6
>>>>>
>>>>> Changes in v6:
>>>>>   - Remove 2Gb multi die pacakge support
>>>>>
>>>>> Changes in v5:
>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>
>>>>> Changes in v4:
>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>
>>>>> Changes in v3:
>>>>>   - Remove S25HL256T and S25HS256T
>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>   - Add support for multi-die package parts support
>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>
>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 54 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> +static int
>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>> +{
>>>>> +       int ret;
>>>>> +
>>>>> +       /*
>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>
>>>> Hi, Takahiro,
>>>>
>>>> I would like some details, please.
>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>
>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>> address mode. We need to set the Flash's address mode in known state and update
>>
>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>> you'll be sure that the flash is in its default state.
>>
> Resetting the Flash to revert back to default state should work for this. However,

good, let's do this.

> we still have SMPT issue below.
> 
>>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>>> choice.
>>>
>>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>>
>>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
>>
>> ok
>>
>>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
>>
>> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
>> then you don't have to do anything for SMPT, right?
>>
> Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
> mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
> address mode (default), SMPT parse does not work correctly.
> 
> Please see this about 4BAIT and SMPT issue.
> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
> 
> I introduced another workaround for SMPT issue in my previous series.
> https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
> I withdrew this because this does not work if Flash address mode is not default state.
> But if we can reset the Flash at probe, this should work.

Hmm. How about issuing spi_nor_set_4byte_addr_mode after parsing 4bait table,
to solve this dependency?

> 
> 
>>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>>
>>>>
>>>>> +        * Flash's addr mode and nor->addr_width here.
>>>>> +        */
>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +       nor->addr_width = 4;
>>>>> +
>>>>> +       /* Replace Quad Enable with volatile version */
>>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>>> +
>>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>>> +}
>>>>> +
>>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>>> +{
>>>>> +       /* Fast Read 4B requires mode cycles */
>>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>>
>>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>>> do you obtain from BFPT for the non-4B opcode?
>>>>
>>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>>> spi_nor_init_default_params().
>>>
>>
>> I see, thanks!
> 
> Thanks,
> Takahiro

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

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

[Tudor.Ambarus]

On 4/20/22 10:35, Tudor Ambarus - M18064 wrote:
> On 4/20/22 09:58, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hi Tudor,
>>>>
>>>> Thank you for your feedback.
>>>>
>>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>
>>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> Changes in v11:
>>>>>>   - Cleanup fixups based on other patches in this series
>>>>>>
>>>>>> Changes in v10:
>>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>>
>>>>>> Changes in v9:
>>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>>
>>>>>> Changes in v8:
>>>>>>   - Call write_disable in error case only
>>>>>>   - Use spi_nor_read_reg() helper
>>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>>   - Update ID table to use FLAGS macro
>>>>>>
>>>>>> Changes in v7:
>>>>>>   - Add missing device info table in v6
>>>>>>
>>>>>> Changes in v6:
>>>>>>   - Remove 2Gb multi die pacakge support
>>>>>>
>>>>>> Changes in v5:
>>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>>
>>>>>> Changes in v4:
>>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>>
>>>>>> Changes in v3:
>>>>>>   - Remove S25HL256T and S25HS256T
>>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>>   - Add support for multi-die package parts support
>>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>>
>>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 54 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>>         return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int
>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>>> +{
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       /*
>>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>>
>>>>> Hi, Takahiro,
>>>>>
>>>>> I would like some details, please.
>>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>>
>>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>>> address mode. We need to set the Flash's address mode in known state and update
>>>
>>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>>> you'll be sure that the flash is in its default state.
>>>
>> Resetting the Flash to revert back to default state should work for this. However,
> 
> good, let's do this.
> 
>> we still have SMPT issue below.
>>
>>>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>>>> choice.
>>>>
>>>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>>>
>>>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>>>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
>>>
>>> ok
>>>
>>>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>>>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
>>>
>>> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
>>> then you don't have to do anything for SMPT, right?
>>>
>> Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
>> mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
>> address mode (default), SMPT parse does not work correctly.
>>
>> Please see this about 4BAIT and SMPT issue.
>> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
>>
>> I introduced another workaround for SMPT issue in my previous series.
>> https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
>> I withdrew this because this does not work if Flash address mode is not default state.
>> But if we can reset the Flash at probe, this should work.
> 
> Hmm. How about issuing spi_nor_set_4byte_addr_mode after parsing 4bait table,
> to solve this dependency?

or maybe we can add a smpt hook where you interrogate the state of addr mode
from CR2V and use the add_width from CR2V instead of relying on what we get
from SMPT_CMD_ADDRESS_LEN_USE_CURRENT. This will avoid changing the state
of the flash at parse time.
 
> 
>>
>>
>>>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>>>
>>>>>
>>>>>> +        * Flash's addr mode and nor->addr_width here.
>>>>>> +        */
>>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>>> +       if (ret)
>>>>>> +               return ret;
>>>>>> +       nor->addr_width = 4;
>>>>>> +
>>>>>> +       /* Replace Quad Enable with volatile version */
>>>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>>>> +
>>>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>>>> +}
>>>>>> +
>>>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>>>> +{
>>>>>> +       /* Fast Read 4B requires mode cycles */
>>>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>>>
>>>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>>>> do you obtain from BFPT for the non-4B opcode?
>>>>>
>>>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>>>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>>>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>>>> spi_nor_init_default_params().
>>>>
>>>
>>> I see, thanks!
>>
>> Thanks,
>> Takahiro
> 

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

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

[Takahiro Kuwano]

On 4/20/2022 4:47 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/20/22 10:35, Tudor Ambarus - M18064 wrote:
>> On 4/20/22 09:58, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> Hi Tudor,
>>>>>
>>>>> Thank you for your feedback.
>>>>>
>>>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>
>>>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>> Changes in v11:
>>>>>>>   - Cleanup fixups based on other patches in this series
>>>>>>>
>>>>>>> Changes in v10:
>>>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>>>
>>>>>>> Changes in v9:
>>>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>>>
>>>>>>> Changes in v8:
>>>>>>>   - Call write_disable in error case only
>>>>>>>   - Use spi_nor_read_reg() helper
>>>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>>>   - Update ID table to use FLAGS macro
>>>>>>>
>>>>>>> Changes in v7:
>>>>>>>   - Add missing device info table in v6
>>>>>>>
>>>>>>> Changes in v6:
>>>>>>>   - Remove 2Gb multi die pacakge support
>>>>>>>
>>>>>>> Changes in v5:
>>>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>>>
>>>>>>> Changes in v4:
>>>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>>   - Remove S25HL256T and S25HS256T
>>>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>>>   - Add support for multi-die package parts support
>>>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>>>
>>>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 54 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>>>         return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +static int
>>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>>>> +{
>>>>>>> +       int ret;
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>>>
>>>>>> Hi, Takahiro,
>>>>>>
>>>>>> I would like some details, please.
>>>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>>>
>>>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>>>> address mode. We need to set the Flash's address mode in known state and update
>>>>
>>>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>>>> you'll be sure that the flash is in its default state.
>>>>
>>> Resetting the Flash to revert back to default state should work for this. However,
>>
>> good, let's do this.
>>
>>> we still have SMPT issue below.
>>>
>>>>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>>>>> choice.
>>>>>
>>>>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>>>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>>>>
>>>>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>>>>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
>>>>
>>>> ok
>>>>
>>>>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>>>>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
>>>>
>>>> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
>>>> then you don't have to do anything for SMPT, right?
>>>>
>>> Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
>>> mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
>>> address mode (default), SMPT parse does not work correctly.
>>>
>>> Please see this about 4BAIT and SMPT issue.
>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
>>>
>>> I introduced another workaround for SMPT issue in my previous series.
>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
>>> I withdrew this because this does not work if Flash address mode is not default state.
>>> But if we can reset the Flash at probe, this should work.
>>
>> Hmm. How about issuing spi_nor_set_4byte_addr_mode after parsing 4bait table,
>> to solve this dependency?
> 
> or maybe we can add a smpt hook where you interrogate the state of addr mode
> from CR2V and use the add_width from CR2V instead of relying on what we get
> from SMPT_CMD_ADDRESS_LEN_USE_CURRENT. This will avoid changing the state
> of the flash at parse time.
> 
Reading CR2V requires Read Any Reg op with correct address width.
Chicken-and-egg...
 
>>
>>>
>>>
>>>>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>>>>
>>>>>>
>>>>>>> +        * Flash's addr mode and nor->addr_width here.
>>>>>>> +        */
>>>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>>>> +       if (ret)
>>>>>>> +               return ret;
>>>>>>> +       nor->addr_width = 4;
>>>>>>> +
>>>>>>> +       /* Replace Quad Enable with volatile version */
>>>>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>>>>> +
>>>>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>>>>> +}
>>>>>>> +
>>>>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>>>>> +{
>>>>>>> +       /* Fast Read 4B requires mode cycles */
>>>>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>>>>
>>>>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>>>>> do you obtain from BFPT for the non-4B opcode?
>>>>>>
>>>>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>>>>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>>>>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>>>>> spi_nor_init_default_params().
>>>>>
>>>>
>>>> I see, thanks!
>>>
>>> Thanks,
>>> Takahiro
>>
> 

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

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

[Tudor.Ambarus]

On 4/20/22 10:50, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/20/2022 4:47 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/20/22 10:35, Tudor Ambarus - M18064 wrote:
>>> On 4/20/22 09:58, Takahiro Kuwano wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>>>>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> Hi Tudor,
>>>>>>
>>>>>> Thank you for your feedback.
>>>>>>
>>>>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>
>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>
>>>>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>>>>
>>>>>>>> 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>
>>>>>>>> ---
>>>>>>>> Changes in v11:
>>>>>>>>   - Cleanup fixups based on other patches in this series
>>>>>>>>
>>>>>>>> Changes in v10:
>>>>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>>>>
>>>>>>>> Changes in v9:
>>>>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>>>>
>>>>>>>> Changes in v8:
>>>>>>>>   - Call write_disable in error case only
>>>>>>>>   - Use spi_nor_read_reg() helper
>>>>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>>>>   - Update ID table to use FLAGS macro
>>>>>>>>
>>>>>>>> Changes in v7:
>>>>>>>>   - Add missing device info table in v6
>>>>>>>>
>>>>>>>> Changes in v6:
>>>>>>>>   - Remove 2Gb multi die pacakge support
>>>>>>>>
>>>>>>>> Changes in v5:
>>>>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>>>>
>>>>>>>> Changes in v4:
>>>>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>>   - Remove S25HL256T and S25HS256T
>>>>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>>>>   - Add support for multi-die package parts support
>>>>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>>>>
>>>>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>>>>  1 file changed, 54 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>>>>         return 0;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +static int
>>>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>>>>> +{
>>>>>>>> +       int ret;
>>>>>>>> +
>>>>>>>> +       /*
>>>>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>>>>
>>>>>>> Hi, Takahiro,
>>>>>>>
>>>>>>> I would like some details, please.
>>>>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>>>>
>>>>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>>>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>>>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>>>>> address mode. We need to set the Flash's address mode in known state and update
>>>>>
>>>>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>>>>> you'll be sure that the flash is in its default state.
>>>>>
>>>> Resetting the Flash to revert back to default state should work for this. However,
>>>
>>> good, let's do this.
>>>
>>>> we still have SMPT issue below.
>>>>
>>>>>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>>>>>> choice.
>>>>>>
>>>>>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>>>>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>>>>>
>>>>>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>>>>>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
>>>>>
>>>>> ok
>>>>>
>>>>>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>>>>>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
>>>>>
>>>>> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
>>>>> then you don't have to do anything for SMPT, right?
>>>>>
>>>> Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
>>>> mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
>>>> address mode (default), SMPT parse does not work correctly.
>>>>
>>>> Please see this about 4BAIT and SMPT issue.
>>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
>>>>
>>>> I introduced another workaround for SMPT issue in my previous series.
>>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
>>>> I withdrew this because this does not work if Flash address mode is not default state.
>>>> But if we can reset the Flash at probe, this should work.
>>>
>>> Hmm. How about issuing spi_nor_set_4byte_addr_mode after parsing 4bait table,
>>> to solve this dependency?
>>
>> or maybe we can add a smpt hook where you interrogate the state of addr mode
>> from CR2V and use the add_width from CR2V instead of relying on what we get
>> from SMPT_CMD_ADDRESS_LEN_USE_CURRENT. This will avoid changing the state
>> of the flash at parse time.
>>
> Reading CR2V requires Read Any Reg op with correct address width.
> Chicken-and-egg...

I agree with Pratyush that we shouldn't change the state of the flash at parsing
time. So to solve this we could introduce nor->params->addr_width to set it at
parsing time, and use the default nor->addr_width for ops at parsing time for
all the parsers.


> 
>>>
>>>>
>>>>
>>>>>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>>>>>
>>>>>>>
>>>>>>>> +        * Flash's addr mode and nor->addr_width here.
>>>>>>>> +        */
>>>>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>>>>> +       if (ret)
>>>>>>>> +               return ret;
>>>>>>>> +       nor->addr_width = 4;
>>>>>>>> +
>>>>>>>> +       /* Replace Quad Enable with volatile version */
>>>>>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>>>>>> +
>>>>>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>>>>>> +{
>>>>>>>> +       /* Fast Read 4B requires mode cycles */
>>>>>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>>>>>
>>>>>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>>>>>> do you obtain from BFPT for the non-4B opcode?
>>>>>>>
>>>>>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>>>>>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>>>>>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>>>>>> spi_nor_init_default_params().
>>>>>>
>>>>>
>>>>> I see, thanks!
>>>>
>>>> Thanks,
>>>> Takahiro
>>>
>>

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

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

[Takahiro Kuwano]

On 4/20/2022 4:35 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/20/22 09:58, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hi Tudor,
>>>>
>>>> Thank you for your feedback.
>>>>
>>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>
>>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> Changes in v11:
>>>>>>   - Cleanup fixups based on other patches in this series
>>>>>>
>>>>>> Changes in v10:
>>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>>
>>>>>> Changes in v9:
>>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>>
>>>>>> Changes in v8:
>>>>>>   - Call write_disable in error case only
>>>>>>   - Use spi_nor_read_reg() helper
>>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>>   - Update ID table to use FLAGS macro
>>>>>>
>>>>>> Changes in v7:
>>>>>>   - Add missing device info table in v6
>>>>>>
>>>>>> Changes in v6:
>>>>>>   - Remove 2Gb multi die pacakge support
>>>>>>
>>>>>> Changes in v5:
>>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>>
>>>>>> Changes in v4:
>>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>>
>>>>>> Changes in v3:
>>>>>>   - Remove S25HL256T and S25HS256T
>>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>>   - Add support for multi-die package parts support
>>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>>
>>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 54 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>>         return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int
>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>>> +{
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       /*
>>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>>
>>>>> Hi, Takahiro,
>>>>>
>>>>> I would like some details, please.
>>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>>
>>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>>> address mode. We need to set the Flash's address mode in known state and update
>>>
>>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>>> you'll be sure that the flash is in its default state.
>>>
>> Resetting the Flash to revert back to default state should work for this. However,
> 
> good, let's do this.
> 
I will do this in u-boot side, like Pratyush did for resetting 8D-8D-8D mode.
https://patchwork.ozlabs.org/project/uboot/patch/20210625191729.31798-23-p.yadav@ti.com/

For Linux MTD, let's assume the address mode is 3-byte (default).

>> we still have SMPT issue below.
>>
>>>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>>>> choice.
>>>>
>>>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>>>
>>>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>>>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
>>>
>>> ok
>>>
>>>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>>>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
>>>
>>> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
>>> then you don't have to do anything for SMPT, right?
>>>
>> Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
>> mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
>> address mode (default), SMPT parse does not work correctly.
>>
>> Please see this about 4BAIT and SMPT issue.
>> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
>>
>> I introduced another workaround for SMPT issue in my previous series.
>> https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
>> I withdrew this because this does not work if Flash address mode is not default state.
>> But if we can reset the Flash at probe, this should work.
> 
> Hmm. How about issuing spi_nor_set_4byte_addr_mode after parsing 4bait table,
> to solve this dependency?
> 
>>
>>
>>>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>>>
>>>>>
>>>>>> +        * Flash's addr mode and nor->addr_width here.
>>>>>> +        */
>>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>>> +       if (ret)
>>>>>> +               return ret;
>>>>>> +       nor->addr_width = 4;
>>>>>> +
>>>>>> +       /* Replace Quad Enable with volatile version */
>>>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>>>> +
>>>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>>>> +}
>>>>>> +
>>>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>>>> +{
>>>>>> +       /* Fast Read 4B requires mode cycles */
>>>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>>>
>>>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>>>> do you obtain from BFPT for the non-4B opcode?
>>>>>
>>>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>>>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>>>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>>>> spi_nor_init_default_params().
>>>>
>>>
>>> I see, thanks!
>>
>> Thanks,
>> Takahiro
> 

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

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

[Takahiro Kuwano]

On 4/20/2022 5:11 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/20/22 10:50, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/20/2022 4:47 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/20/22 10:35, Tudor Ambarus - M18064 wrote:
>>>> On 4/20/22 09:58, Takahiro Kuwano wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> Hi Tudor,
>>>>>>>
>>>>>>> Thank you for your feedback.
>>>>>>>
>>>>>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>>
>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>
>>>>>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>>>>>
>>>>>>>>> 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>
>>>>>>>>> ---
>>>>>>>>> Changes in v11:
>>>>>>>>>   - Cleanup fixups based on other patches in this series
>>>>>>>>>
>>>>>>>>> Changes in v10:
>>>>>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>>>>>
>>>>>>>>> Changes in v9:
>>>>>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>>>>>
>>>>>>>>> Changes in v8:
>>>>>>>>>   - Call write_disable in error case only
>>>>>>>>>   - Use spi_nor_read_reg() helper
>>>>>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>>>>>   - Update ID table to use FLAGS macro
>>>>>>>>>
>>>>>>>>> Changes in v7:
>>>>>>>>>   - Add missing device info table in v6
>>>>>>>>>
>>>>>>>>> Changes in v6:
>>>>>>>>>   - Remove 2Gb multi die pacakge support
>>>>>>>>>
>>>>>>>>> Changes in v5:
>>>>>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>>>>>
>>>>>>>>> Changes in v4:
>>>>>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>>>>>
>>>>>>>>> Changes in v3:
>>>>>>>>>   - Remove S25HL256T and S25HS256T
>>>>>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>>>>>   - Add support for multi-die package parts support
>>>>>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>>>>>
>>>>>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>>>>>  1 file changed, 54 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>>>>>         return 0;
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> +static int
>>>>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>>>>>> +{
>>>>>>>>> +       int ret;
>>>>>>>>> +
>>>>>>>>> +       /*
>>>>>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>>>>>
>>>>>>>> Hi, Takahiro,
>>>>>>>>
>>>>>>>> I would like some details, please.
>>>>>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>>>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>>>>>
>>>>>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>>>>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>>>>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>>>>>> address mode. We need to set the Flash's address mode in known state and update
>>>>>>
>>>>>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>>>>>> you'll be sure that the flash is in its default state.
>>>>>>
>>>>> Resetting the Flash to revert back to default state should work for this. However,
>>>>
>>>> good, let's do this.
>>>>
>>>>> we still have SMPT issue below.
>>>>>
>>>>>>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>>>>>>> choice.
>>>>>>>
>>>>>>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>>>>>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>>>>>>
>>>>>>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>>>>>>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
>>>>>>
>>>>>> ok
>>>>>>
>>>>>>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>>>>>>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
>>>>>>
>>>>>> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
>>>>>> then you don't have to do anything for SMPT, right?
>>>>>>
>>>>> Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
>>>>> mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
>>>>> address mode (default), SMPT parse does not work correctly.
>>>>>
>>>>> Please see this about 4BAIT and SMPT issue.
>>>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
>>>>>
>>>>> I introduced another workaround for SMPT issue in my previous series.
>>>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
>>>>> I withdrew this because this does not work if Flash address mode is not default state.
>>>>> But if we can reset the Flash at probe, this should work.
>>>>
>>>> Hmm. How about issuing spi_nor_set_4byte_addr_mode after parsing 4bait table,
>>>> to solve this dependency?
>>>
>>> or maybe we can add a smpt hook where you interrogate the state of addr mode
>>> from CR2V and use the add_width from CR2V instead of relying on what we get
>>> from SMPT_CMD_ADDRESS_LEN_USE_CURRENT. This will avoid changing the state
>>> of the flash at parse time.
>>>
>> Reading CR2V requires Read Any Reg op with correct address width.
>> Chicken-and-egg...
> 
> I agree with Pratyush that we shouldn't change the state of the flash at parsing
> time. So to solve this we could introduce nor->params->addr_width to set it at
> parsing time, and use the default nor->addr_width for ops at parsing time for
> all the parsers.
> 
The default nor->addr_width comes from BFPT parse (no change in BFPT parse).
In 4bait parse, nor->params->addr_width is updated instead of nor->addr_width.
Then,

static int spi_nor_set_addr_width(struct spi_nor *nor)
{
+	if (nor->params->addr_width)
+		nor->addr_width = nor->params->addr_width;

	if (nor->addr_width) {
		/* already configured from SFDP */
	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {

Is this something we are going to do?

> 
>>
>>>>
>>>>>
>>>>>
>>>>>>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>>>>>>
>>>>>>>>
>>>>>>>>> +        * Flash's addr mode and nor->addr_width here.
>>>>>>>>> +        */
>>>>>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>>>>>> +       if (ret)
>>>>>>>>> +               return ret;
>>>>>>>>> +       nor->addr_width = 4;
>>>>>>>>> +
>>>>>>>>> +       /* Replace Quad Enable with volatile version */
>>>>>>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>>>>>>> +
>>>>>>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>>>>>>> +{
>>>>>>>>> +       /* Fast Read 4B requires mode cycles */
>>>>>>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>>>>>>
>>>>>>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>>>>>>> do you obtain from BFPT for the non-4B opcode?
>>>>>>>>
>>>>>>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>>>>>>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>>>>>>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>>>>>>> spi_nor_init_default_params().
>>>>>>>
>>>>>>
>>>>>> I see, thanks!
>>>>>
>>>>> Thanks,
>>>>> Takahiro
>>>>
>>>
> 

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

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

[Tudor.Ambarus]

On 4/20/22 11:38, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/20/2022 5:11 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/20/22 10:50, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 4/20/2022 4:47 PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 4/20/22 10:35, Tudor Ambarus - M18064 wrote:
>>>>> On 4/20/22 09:58, Takahiro Kuwano wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>
>>>>>>>> Hi Tudor,
>>>>>>>>
>>>>>>>> Thank you for your feedback.
>>>>>>>>
>>>>>>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>>>>
>>>>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>>>>
>>>>>>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>>>>>>
>>>>>>>>>> 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>
>>>>>>>>>> ---
>>>>>>>>>> Changes in v11:
>>>>>>>>>>   - Cleanup fixups based on other patches in this series
>>>>>>>>>>
>>>>>>>>>> Changes in v10:
>>>>>>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>>>>>>
>>>>>>>>>> Changes in v9:
>>>>>>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>>>>>>
>>>>>>>>>> Changes in v8:
>>>>>>>>>>   - Call write_disable in error case only
>>>>>>>>>>   - Use spi_nor_read_reg() helper
>>>>>>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>>>>>>   - Update ID table to use FLAGS macro
>>>>>>>>>>
>>>>>>>>>> Changes in v7:
>>>>>>>>>>   - Add missing device info table in v6
>>>>>>>>>>
>>>>>>>>>> Changes in v6:
>>>>>>>>>>   - Remove 2Gb multi die pacakge support
>>>>>>>>>>
>>>>>>>>>> Changes in v5:
>>>>>>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>>>>>>
>>>>>>>>>> Changes in v4:
>>>>>>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>>>>>>
>>>>>>>>>> Changes in v3:
>>>>>>>>>>   - Remove S25HL256T and S25HS256T
>>>>>>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>>>>>>   - Add support for multi-die package parts support
>>>>>>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>>>>>>
>>>>>>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>>>>>>  1 file changed, 54 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>>>>>>         return 0;
>>>>>>>>>>  }
>>>>>>>>>>
>>>>>>>>>> +static int
>>>>>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>>>>>>> +{
>>>>>>>>>> +       int ret;
>>>>>>>>>> +
>>>>>>>>>> +       /*
>>>>>>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>>>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>>>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>>>>>>
>>>>>>>>> Hi, Takahiro,
>>>>>>>>>
>>>>>>>>> I would like some details, please.
>>>>>>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>>>>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>>>>>>
>>>>>>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>>>>>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>>>>>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>>>>>>> address mode. We need to set the Flash's address mode in known state and update
>>>>>>>
>>>>>>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>>>>>>> you'll be sure that the flash is in its default state.
>>>>>>>
>>>>>> Resetting the Flash to revert back to default state should work for this. However,
>>>>>
>>>>> good, let's do this.
>>>>>
>>>>>> we still have SMPT issue below.
>>>>>>
>>>>>>>> nor->addr_width accordingly. Due to SMPT issue below, value of 4 would be better
>>>>>>>> choice.
>>>>>>>>
>>>>>>>>> 2/ Where in SMPT parse? I looked through the code and couldn't find the Read Any
>>>>>>>>> Reg op used. Why do you need an addr_width of value 4 in SMPT parse?
>>>>>>>>>
>>>>>>>> In the spi_nor_get_map_in_use(), the nor->read_opcode is set to 0x65 (=Read Any Reg).
>>>>>>>> The spi_nor_smpt_addr_width() returns the value of the nor->addr_width due to
>>>>>>>
>>>>>>> ok
>>>>>>>
>>>>>>>> SMPT_CMD_ADDRESS_LEN_USE_CURRENT. The nor->addr_width is set to 4 in the
>>>>>>>> spi_nor_parse_4bait() before SMPT parse. Therefore, the host issues Read Any Reg
>>>>>>>
>>>>>>> if 4bait is parsed before SMPT and nor->addr_width is already set to 4 in parse_4bait,
>>>>>>> then you don't have to do anything for SMPT, right?
>>>>>>>
>>>>>> Currently 4bait is parsed before SMPT. So, if we are sure the Flash is in 4-byte address
>>>>>> mode (by u-boot, for example), we don't have to do anything. If the Flash is in 3-byte
>>>>>> address mode (default), SMPT parse does not work correctly.
>>>>>>
>>>>>> Please see this about 4BAIT and SMPT issue.
>>>>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/20201212115817.5122-1-vigneshr@ti.com/
>>>>>>
>>>>>> I introduced another workaround for SMPT issue in my previous series.
>>>>>> https://patchwork.ozlabs.org/project/linux-mtd/patch/9a2d323b2c18485d13f271e3bb213b96fea0e7e1.1649641729.git.Takahiro.Kuwano@infineon.com/
>>>>>> I withdrew this because this does not work if Flash address mode is not default state.
>>>>>> But if we can reset the Flash at probe, this should work.
>>>>>
>>>>> Hmm. How about issuing spi_nor_set_4byte_addr_mode after parsing 4bait table,
>>>>> to solve this dependency?
>>>>
>>>> or maybe we can add a smpt hook where you interrogate the state of addr mode
>>>> from CR2V and use the add_width from CR2V instead of relying on what we get
>>>> from SMPT_CMD_ADDRESS_LEN_USE_CURRENT. This will avoid changing the state
>>>> of the flash at parse time.
>>>>
>>> Reading CR2V requires Read Any Reg op with correct address width.
>>> Chicken-and-egg...
>>
>> I agree with Pratyush that we shouldn't change the state of the flash at parsing
>> time. So to solve this we could introduce nor->params->addr_width to set it at
>> parsing time, and use the default nor->addr_width for ops at parsing time for
>> all the parsers.
>>
> The default nor->addr_width comes from BFPT parse (no change in BFPT parse).
> In 4bait parse, nor->params->addr_width is updated instead of nor->addr_width.
> Then,
> 
> static int spi_nor_set_addr_width(struct spi_nor *nor)
> {
> +       if (nor->params->addr_width)
> +               nor->addr_width = nor->params->addr_width;

remove the above and
> 
>         if (nor->addr_width) {
replace the if condition with:
	if (nor->params->addr_width)
		nor->addr_width = nor->params->addr_width;
	else if {

and the behavior should be kept as before

>                 /* already configured from SFDP */
>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> 
> Is this something we are going to do?

sounds good on a first read, yes
> 
>>
>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>>> with 4-byte address. We need to set the Flash into 4-byte address mode in advance.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +        * Flash's addr mode and nor->addr_width here.
>>>>>>>>>> +        */
>>>>>>>>>> +       ret = spi_nor_set_4byte_addr_mode(nor, true);
>>>>>>>>>> +       if (ret)
>>>>>>>>>> +               return ret;
>>>>>>>>>> +       nor->addr_width = 4;
>>>>>>>>>> +
>>>>>>>>>> +       /* Replace Quad Enable with volatile version */
>>>>>>>>>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>>>>>>>>>> +
>>>>>>>>>> +       return cypress_nor_set_page_size(nor, nor->addr_width);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +void s25hx_t_late_init(struct spi_nor *nor)
>>>>>>>>>> +{
>>>>>>>>>> +       /* Fast Read 4B requires mode cycles */
>>>>>>>>>> +       nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>>>>>>>>>
>>>>>>>>> Isn't this info already handled in BFPT? What value of num_mode_clocks
>>>>>>>>> do you obtain from BFPT for the non-4B opcode?
>>>>>>>>>
>>>>>>>> There is no parameter in BFPT that describes about Fast Read 1-1-1 op
>>>>>>>> (as we can see 'struct sfdp_bfpt_read' in sfdp.c).
>>>>>>>> The value of num_mode_clocks for Fast Read 1-1-1 is set to 0 in
>>>>>>>> spi_nor_init_default_params().
>>>>>>>>
>>>>>>>
>>>>>>> I see, thanks!
>>>>>>
>>>>>> Thanks,
>>>>>> Takahiro
>>>>>
>>>>
>>

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

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

[Tudor.Ambarus]

On 4/20/22 11:32, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/20/2022 4:35 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/20/22 09:58, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 4/20/2022 3:11 PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 4/20/22 08:34, Takahiro Kuwano wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> Hi Tudor,
>>>>>
>>>>> Thank you for your feedback.
>>>>>
>>>>> On 4/19/2022 6:32 PM, Tudor.Ambarus@microchip.com wrote:
>>>>>> On 4/18/22 08:41, tkuw584924@gmail.com wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>>>>
>>>>>>> The S25HL-T/S25HS-T family is the Infineon SEMPER Flash with Quad SPI.
>>>>>>>
>>>>>>> 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>
>>>>>>> ---
>>>>>>> Changes in v11:
>>>>>>>   - Cleanup fixups based on other patches in this series
>>>>>>>
>>>>>>> Changes in v10:
>>>>>>>   - Cleanup fixups and ID table based on other patches in this series
>>>>>>>
>>>>>>> Changes in v9:
>>>>>>>   - Use late_init() hook to fix mode clocks and writesize
>>>>>>>   - Use PARSE_SFDP instead of NO_SFDP_FLAGS
>>>>>>>   - Use MFR_FLAGS for USE_CLSR
>>>>>>>   - Add comment block to explain about addr mode in post_bfpt_fixups()
>>>>>>>
>>>>>>> Changes in v8:
>>>>>>>   - Call write_disable in error case only
>>>>>>>   - Use spi_nor_read_reg() helper
>>>>>>>   - Use nor->bouncebuf instead of variable on stack
>>>>>>>   - Update ID table to use FLAGS macro
>>>>>>>
>>>>>>> Changes in v7:
>>>>>>>   - Add missing device info table in v6
>>>>>>>
>>>>>>> Changes in v6:
>>>>>>>   - Remove 2Gb multi die pacakge support
>>>>>>>
>>>>>>> Changes in v5:
>>>>>>>   - Add NO_CHIP_ERASE flag to S25HL02GT and S25HS02GT
>>>>>>>
>>>>>>> Changes in v4:
>>>>>>>   - Merge block comments about SMPT in s25hx_t_post_sfdp_fixups()
>>>>>>>   - Remove USE_CLSR flags from S25HL02GT and S25HS02GT
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>>   - Remove S25HL256T and S25HS256T
>>>>>>>   - Add S25HL02GT and S25HS02GT
>>>>>>>   - Add support for multi-die package parts support
>>>>>>>   - Remove erase_map fix for top/split sector layout
>>>>>>>   - Set ECC data unit size (16B) to writesize
>>>>>>>
>>>>>>>  drivers/mtd/spi-nor/spansion.c | 54 ++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 54 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>>>>> index 493240ebfd70..dd37b829efbc 100644
>>>>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>>>>> @@ -208,6 +208,44 @@ static int cypress_nor_set_page_size(struct spi_nor *nor, u8 addr_width)
>>>>>>>         return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +static int
>>>>>>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>>>>>>> +                        const struct sfdp_parameter_header *bfpt_header,
>>>>>>> +                        const struct sfdp_bfpt *bfpt)
>>>>>>> +{
>>>>>>> +       int ret;
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * From BFPT, the nor->addr_width is set to 3. In Read Any Reg op, the
>>>>>>> +        * Flash takes 3-byte or 4-byte addr depending current addr mode. Since
>>>>>>> +        * Read Any Reg op is called in this hook and SMPT parse, we would sync
>>>>>>
>>>>>> Hi, Takahiro,
>>>>>>
>>>>>> I would like some details, please.
>>>>>> 1/ with "this hook" you refer to cypress_nor_set_page_size(). Why can't you use a
>>>>>> addr_width of value 3 when reading SPINOR_REG_CYPRESS_CFR3V?
>>>>>>
>>>>> If we are sure that the Flash is in 3-byte address mode, we can use the value 3
>>>>> for reading CFR3V. However, the Flash's address mode may be changed prior to
>>>>> Linux MTD probe in some use cases. Actually, in u-boot, it is set to 4-byte
>>>>> address mode. We need to set the Flash's address mode in known state and update
>>>>
>>>> addr_width is set via CFR2Volatile, can we reset the flash at probe instead? Then
>>>> you'll be sure that the flash is in its default state.
>>>>
>>> Resetting the Flash to revert back to default state should work for this. However,
>>
>> good, let's do this.
>>
> I will do this in u-boot side, like Pratyush did for resetting 8D-8D-8D mode.
> https://patchwork.ozlabs.org/project/uboot/patch/20210625191729.31798-23-p.yadav@ti.com/
> 
> For Linux MTD, let's assume the address mode is 3-byte (default).

would be good to have the reset in kernel as well to avoid any dependency on the
bootloaders, but do as you want/need.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Takahiro Kuwano]

On 4/20/2022 5:46 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/20/22 11:38, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/20/2022 5:11 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/20/22 10:50, Takahiro Kuwano wrote:
[...]
>>> I agree with Pratyush that we shouldn't change the state of the flash at parsing
>>> time. So to solve this we could introduce nor->params->addr_width to set it at
>>> parsing time, and use the default nor->addr_width for ops at parsing time for
>>> all the parsers.
>>>
>> The default nor->addr_width comes from BFPT parse (no change in BFPT parse).
>> In 4bait parse, nor->params->addr_width is updated instead of nor->addr_width.
>> Then,
>>
>> static int spi_nor_set_addr_width(struct spi_nor *nor)
>> {
>> +       if (nor->params->addr_width)
>> +               nor->addr_width = nor->params->addr_width;
> 
> remove the above and
>>
>>         if (nor->addr_width) {
> replace the if condition with:
> 	if (nor->params->addr_width)
> 		nor->addr_width = nor->params->addr_width;
> 	else if {
> 
> and the behavior should be kept as before
> 
>>                 /* already configured from SFDP */
>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>>
>> Is this something we are going to do?
> 
> sounds good on a first read, yes

Hi Tudor,

As Pratyush mentioned in that thread, we already have SNOR_F_HAS_4BAIT flag.
Please see new patch in revised series.
https://patchwork.ozlabs.org/project/linux-mtd/patch/d705a2c1fc8d0a59c93454aa05642aafe4fc960b.1650519576.git.Takahiro.Kuwano@infineon.com/

Best Regards,
Takahiro

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