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

[PATCH v13 0/4] 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, SFDP, and test script output:
------------------------------------------------------------
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> test_qspi.sh
6+0 records in
6+0 records out
6291456 bytes (6.0MB) copied, 0.223713 seconds, 26.8MB/s
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Erased 6291456 bytes from address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0600000
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
97c13104ef8e1f895d72c64b99233c57730ec773  qspi_test
97c13104ef8e1f895d72c64b99233c57730ec773  qspi_read
------------------------------------------------------------

------------------------------------------------------------
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> test_qspi.sh
6+0 records in
6+0 records out
6291456 bytes (6.0MB) copied, 0.222716 seconds, 26.9MB/s
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Erased 6291456 bytes from address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0600000
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
000b392ab4cca7e2be97c68842794a9b10f3f8a3  qspi_test
000b392ab4cca7e2be97c68842794a9b10f3f8a3  qspi_read
------------------------------------------------------------

------------------------------------------------------------
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> test_qspi.sh
6+0 records in
6+0 records out
6291456 bytes (6.0MB) copied, 0.223581 seconds, 26.8MB/s
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Erased 6291456 bytes from address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0600000
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
e5396eabcb16cbba29aa3270d7aac6c505e6d5e2  qspi_test
e5396eabcb16cbba29aa3270d7aac6c505e6d5e2  qspi_read
------------------------------------------------------------

------------------------------------------------------------
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
zynq> test_qspi.sh
6+0 records in
6+0 records out
6291456 bytes (6.0MB) copied, 0.223877 seconds, 26.8MB/s
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Erased 6291456 bytes from address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0600000
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
4c2f38951f5c524b2e269e535c7efb93ef5a80dc  qspi_test
4c2f38951f5c524b2e269e535c7efb93ef5a80dc  qspi_read
------------------------------------------------------------

---
Changes in v13:
  - Remove patch, Call set_4byte_addr_mode() before spi_nor_quad_enalbe()
  - Remove patch, Rename local macro
  - Use 3-byte address width in cypress_nor_quad_enable_volatile()
  - Add post_sfdp to fix 3 byte erase opcode in 4BAIT

Changes in v12:
  - Rebase on top of Tudor's series
    https://patchwork.ozlabs.org/project/linux-mtd/list/?series=295933
    https://patchwork.ozlabs.org/project/linux-mtd/list/?series=294533
  - New patch: Retain nor->addr_width at 4BAIT parse
  - New patch: Call set_4byte_addr_mode() before spi_nor_quad_enalbe()
  - New patch: Rename local macro
  
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 (4):
  mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
  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/core.c     |   7 +-
 drivers/mtd/spi-nor/sfdp.c     |   1 -
 drivers/mtd/spi-nor/spansion.c | 179 +++++++++++++++++++++++++++++----
 3 files changed, 163 insertions(+), 24 deletions(-)

-- 
2.25.1


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

[PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[tkuw584924]

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

In 4BAIT parse, keep nor->addr_width because it may be used as
current address mode in SMPT parse later on.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi-nor/core.c | 7 ++++++-
 drivers/mtd/spi-nor/sfdp.c | 1 -
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 40ba45328975..87603a99938f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct spi_nor *nor,
 static int spi_nor_set_addr_width(struct spi_nor *nor)
 {
 	if (nor->addr_width) {
-		/* already configured from SFDP */
+		/*
+		 * Already configured from SFDP. Use an address width of 4 in
+		 * case the device has 4byte opcodes.
+		 */
+		if (nor->addr_width == 3 && nor->flags & SNOR_F_HAS_4BAIT)
+			nor->addr_width = 4;
 	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
 		/*
 		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index c5dd79ef75c8..9aeefd070475 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -1211,7 +1211,6 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
 	 * Spansion memory. However this quirk is no longer needed with new
 	 * SFDP compliant memories.
 	 */
-	nor->addr_width = 4;
 	nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
 
 	/* fall through */
-- 
2.25.1


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

[PATCH v13 2/4] 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>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
Changes in v13:
  - Use 3-byte address width

Changes in v12:
  - Rebase on top of Tudor's series
    https://patchwork.ozlabs.org/project/linux-mtd/list/?series=295933
  - Use macro directly instead of local variable
  - Use nor->reg_proto instead of SNOR_PROTO_1_1_1
    
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 | 59 ++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 43cd6cd92537..65ae3ade0b95 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -14,6 +14,8 @@
 #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
 #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
 #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
+#define SPINOR_REG_CYPRESS_CFR1V		0x00800002
+#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
 #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
 #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24	0xb
 #define SPINOR_REG_CYPRESS_CFR3V		0x00800004
@@ -113,6 +115,63 @@ static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
 	return 0;
 }
 
+/**
+ * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
+ *                                      register.
+ * @nor:	pointer to a 'struct spi_nor'
+ *
+ * It is recommended to update volatile registers in the field application due
+ * to a risk of the non-volatile registers corruption by power interrupt. This
+ * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
+ * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
+ * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
+ * also set during Flash power-up.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
+{
+	struct spi_mem_op op;
+	u8 cfr1v_written;
+	int ret;
+
+	op = (struct spi_mem_op)
+		CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
+					  nor->bouncebuf);
+	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
+
+	if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
+		return 0;
+
+	/* Update the Quad Enable bit. */
+	nor->bouncebuf[0] |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
+	op = (struct spi_mem_op)
+		CYPRESS_NOR_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V, 1,
+					  nor->bouncebuf);
+	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
+
+	cfr1v_written = nor->bouncebuf[0];
+
+	/* Read back and check it. */
+	op = (struct spi_mem_op)
+		CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
+					  nor->bouncebuf);
+	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
+
+	if (nor->bouncebuf[0] != cfr1v_written) {
+		dev_err(nor->dev, "CFR1: Read back test failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 /**
  * cypress_nor_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 v13 3/4] 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>
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
Changes in v13:
  - (no change)

Changes in v12:
  - Rebase on top of Tudor's series
    https://patchwork.ozlabs.org/project/linux-mtd/list/?series=295933
  - Remove addr_width param. Use nor->addr_width instead.

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 65ae3ade0b95..952d4383f9da 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -172,6 +172,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'
+ *
+ * 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)
+{
+	struct spi_mem_op op =
+		CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width,
+					  SPINOR_REG_CYPRESS_CFR3V,
+					  nor->bouncebuf);
+	int ret;
+
+	ret = spi_nor_read_any_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'
@@ -226,28 +257,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);
 }
 
 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 v13 4/4] 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 v13:
  - Remove part specific set_4byte_addr_mode()
  - Revert SNOR_F_4B_OPCODES
  - Add post_sfdp to fix 3 byte erase opcode in 4BAIT 

Changes in v12:
  - Cleanup fixups based on other patches in this series
  - Add part specific set_4byte_addr_mode()
  - Unset SNOR_F_4B_OPCODES flag to let core to call set_4byte_addr_mode
  
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 | 66 ++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 952d4383f9da..e56c48c3280b 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -203,6 +203,56 @@ static int cypress_nor_set_page_size(struct spi_nor *nor)
 	return 0;
 }
 
+static int
+s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
+			const struct sfdp_parameter_header *bfpt_header,
+			const struct sfdp_bfpt *bfpt)
+{
+	/* Replace Quad Enable with volatile version */
+	nor->params->quad_enable = cypress_nor_quad_enable_volatile;
+
+	return cypress_nor_set_page_size(nor);
+}
+
+static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
+{
+	struct spi_nor_erase_type *erase_type =
+					nor->params->erase_map.erase_type;
+	int i;
+
+	/*
+	 * In some parts, 3byte erase opcodes are advertised by 4BAIT.
+	 * Convert them to 4byte erase opcodes.
+	 */
+	for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
+		switch (erase_type[i].opcode) {
+		case SPINOR_OP_SE:
+			erase_type[i].opcode = SPINOR_OP_SE_4B;
+			break;
+		case SPINOR_OP_BE_4K:
+			erase_type[i].opcode = SPINOR_OP_BE_4K_4B;
+			break;
+		default:
+			break;
+		}
+	}
+}
+
+static void s25hx_t_late_init(struct spi_nor *nor)
+{
+	/* 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_fixup,
+	.post_sfdp = s25hx_t_post_sfdp_fixup,
+	.late_init = s25hx_t_late_init,
+};
+
 /**
  * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
@@ -379,6 +429,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 v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Tudor.Ambarus]

On 4/21/22 12:40, 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>
> 
> In 4BAIT parse, keep nor->addr_width because it may be used as
> current address mode in SMPT parse later on.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>  drivers/mtd/spi-nor/sfdp.c | 1 -
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 40ba45328975..87603a99938f 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>  {
>         if (nor->addr_width) {
> -               /* already configured from SFDP */
> +               /*
> +                * Already configured from SFDP. Use an address width of 4 in
> +                * case the device has 4byte opcodes.
> +                */
> +               if (nor->addr_width == 3 && nor->flags & SNOR_F_HAS_4BAIT)
> +                       nor->addr_width = 4;
>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {

Can we have this instead?

commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
Author: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Date:   Thu Apr 21 18:40:21 2022 +0900

    mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
    
    In 4BAIT parse, keep nor->addr_width because it may be used as
    current address mode in SMPT parse later on.
    
    Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
    Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 2bfa84100d38..7095bf897318 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2272,6 +2272,9 @@ static int spi_nor_default_setup(struct spi_nor *nor,
 
 static int spi_nor_set_addr_width(struct spi_nor *nor)
 {
+       if (nor->flags & SNOR_F_HAS_4BAIT)
+               nor->addr_width = 4;
+
        if (nor->addr_width) {
                /* already configured from SFDP */
        } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index a5211543d30d..b2cc3bfe0d5c 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -1098,7 +1098,6 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
         * Spansion memory. However this quirk is no longer needed with new
         * SFDP compliant memories.
         */
-       nor->addr_width = 4;
        nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
 
        /* fall through */

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

Re: [PATCH v13 2/4] mtd: spi-nor: spansion: Add support for volatile QE bit

[Tudor.Ambarus]

On 4/21/22 12:40, 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>
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> Changes in v13:
>   - Use 3-byte address width
> 
> Changes in v12:
>   - Rebase on top of Tudor's series
>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=295933
>   - Use macro directly instead of local variable
>   - Use nor->reg_proto instead of SNOR_PROTO_1_1_1
> 
> 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 | 59 ++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 43cd6cd92537..65ae3ade0b95 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -14,6 +14,8 @@
>  #define SPINOR_OP_CLSR         0x30    /* Clear status register 1 */
>  #define SPINOR_OP_RD_ANY_REG                   0x65    /* Read any register */
>  #define SPINOR_OP_WR_ANY_REG                   0x71    /* Write any register */
> +#define SPINOR_REG_CYPRESS_CFR1V               0x00800002
> +#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN       BIT(1)  /* Quad Enable */
>  #define SPINOR_REG_CYPRESS_CFR2V               0x00800003
>  #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24  0xb
>  #define SPINOR_REG_CYPRESS_CFR3V               0x00800004
> @@ -113,6 +115,63 @@ static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
>         return 0;
>  }
> 
> +/**
> + * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
> + *                                      register.
> + * @nor:       pointer to a 'struct spi_nor'
> + *
> + * It is recommended to update volatile registers in the field application due
> + * to a risk of the non-volatile registers corruption by power interrupt. This
> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
> + * also set during Flash power-up.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
> +{
> +       struct spi_mem_op op;
> +       u8 cfr1v_written;
> +       int ret;
> +
> +       op = (struct spi_mem_op)
> +               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
nor->addr_width is 3, isn't it? can we use nor->addr_width instead of 3, please?

> +                                         nor->bouncebuf);
> +       ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
> +       if (ret)
> +               return ret;
> +
> +       if (nor->bouncebuf[0] & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
> +               return 0;
> +
> +       /* Update the Quad Enable bit. */
> +       nor->bouncebuf[0] |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
> +       op = (struct spi_mem_op)
> +               CYPRESS_NOR_WR_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V, 1,
same

> +                                         nor->bouncebuf);
> +       ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> +       if (ret)
> +               return ret;
> +
> +       cfr1v_written = nor->bouncebuf[0];
> +
> +       /* Read back and check it. */
> +       op = (struct spi_mem_op)
> +               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
same

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

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

[Tudor.Ambarus]

On 4/21/22 12:40, 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>
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> Changes in v13:
>   - (no change)
> 
> Changes in v12:
>   - Rebase on top of Tudor's series
>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=295933
>   - Remove addr_width param. Use nor->addr_width instead.
> 
> 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 65ae3ade0b95..952d4383f9da 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -172,6 +172,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'
> + *
> + * 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)
> +{
> +       struct spi_mem_op op =
> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width,
> +                                         SPINOR_REG_CYPRESS_CFR3V,
> +                                         nor->bouncebuf);
> +       int ret;
> +
> +       ret = spi_nor_read_any_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'
> @@ -226,28 +257,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,

here you substitute 3 with nor->addr_width behind the doors. It would have
worth to have a dedicated patch for this and then come with a patch for
the local function. Or at least add a comment in the commit's description.

> -                                         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);
>  }
> 
>  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 v13 4/4] mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups

[Tudor.Ambarus]

On 4/21/22 12:40, 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 v13:
>   - Remove part specific set_4byte_addr_mode()
>   - Revert SNOR_F_4B_OPCODES
>   - Add post_sfdp to fix 3 byte erase opcode in 4BAIT
> 
> Changes in v12:
>   - Cleanup fixups based on other patches in this series
>   - Add part specific set_4byte_addr_mode()
>   - Unset SNOR_F_4B_OPCODES flag to let core to call set_4byte_addr_mode
> 
> 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 | 66 ++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 952d4383f9da..e56c48c3280b 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -203,6 +203,56 @@ static int cypress_nor_set_page_size(struct spi_nor *nor)
>         return 0;
>  }
> 
> +static int
> +s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
> +                       const struct sfdp_parameter_header *bfpt_header,
> +                       const struct sfdp_bfpt *bfpt)
> +{
> +       /* Replace Quad Enable with volatile version */
> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
> +
> +       return cypress_nor_set_page_size(nor);
> +}
> +
> +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
> +{
> +       struct spi_nor_erase_type *erase_type =
> +                                       nor->params->erase_map.erase_type;
> +       int i;
> +
> +       /*
> +        * In some parts, 3byte erase opcodes are advertised by 4BAIT.
> +        * Convert them to 4byte erase opcodes.

this is a fix, those opcodes don't work with 4 byte addresses, right?
Looks good!
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> +        */
> +       for (i = 0; i < SNOR_ERASE_TYPE_MAX; i++) {
> +               switch (erase_type[i].opcode) {
> +               case SPINOR_OP_SE:
> +                       erase_type[i].opcode = SPINOR_OP_SE_4B;
> +                       break;
> +               case SPINOR_OP_BE_4K:
> +                       erase_type[i].opcode = SPINOR_OP_BE_4K_4B;
> +                       break;
> +               default:
> +                       break;
> +               }
> +       }
> +}
> +
> +static void s25hx_t_late_init(struct spi_nor *nor)
> +{
> +       /* 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_fixup,
> +       .post_sfdp = s25hx_t_post_sfdp_fixup,
> +       .late_init = s25hx_t_late_init,
> +};
> +
>  /**
>   * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:               pointer to a 'struct spi_nor'
> @@ -379,6 +429,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 v13 2/4] mtd: spi-nor: spansion: Add support for volatile QE bit

[Takahiro Kuwano]

On 4/21/2022 7:41 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/21/22 12:40, tkuw584924@gmail.com wrote:
[...]
>> +/**
>> + * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
>> + *                                      register.
>> + * @nor:       pointer to a 'struct spi_nor'
>> + *
>> + * It is recommended to update volatile registers in the field application due
>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
>> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
>> + * also set during Flash power-up.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>> +{
>> +       struct spi_mem_op op;
>> +       u8 cfr1v_written;
>> +       int ret;
>> +
>> +       op = (struct spi_mem_op)
>> +               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
> nor->addr_width is 3, isn't it? can we use nor->addr_width instead of 3, please?
>
No, at the time this method is called, nor->addr_width is set to 4 by
spi_nor_set_addr_width().

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Takahiro Kuwano]

On 4/21/2022 7:38 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/21/22 12:40, 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>
>>
>> In 4BAIT parse, keep nor->addr_width because it may be used as
>> current address mode in SMPT parse later on.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>>  drivers/mtd/spi-nor/sfdp.c | 1 -
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 40ba45328975..87603a99938f 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>>  {
>>         if (nor->addr_width) {
>> -               /* already configured from SFDP */
>> +               /*
>> +                * Already configured from SFDP. Use an address width of 4 in
>> +                * case the device has 4byte opcodes.
>> +                */
>> +               if (nor->addr_width == 3 && nor->flags & SNOR_F_HAS_4BAIT)
>> +                       nor->addr_width = 4;
>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> 
> Can we have this instead?
> 
> commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
> Author: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Date:   Thu Apr 21 18:40:21 2022 +0900
> 
>     mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
>     
>     In 4BAIT parse, keep nor->addr_width because it may be used as
>     current address mode in SMPT parse later on.
>     
>     Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>     Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 2bfa84100d38..7095bf897318 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2272,6 +2272,9 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>  
>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>  {
> +       if (nor->flags & SNOR_F_HAS_4BAIT)
> +               nor->addr_width = 4;
> +
>         if (nor->addr_width) {
>                 /* already configured from SFDP */
>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index a5211543d30d..b2cc3bfe0d5c 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -1098,7 +1098,6 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>          * Spansion memory. However this quirk is no longer needed with new
>          * SFDP compliant memories.
>          */
> -       nor->addr_width = 4;
>         nor->flags |= SNOR_F_4B_OPCODES | SNOR_F_HAS_4BAIT;
>  
>         /* fall through */
> 
Yes, thanks!

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

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

[Takahiro Kuwano]

On 4/21/2022 7:45 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/21/22 12:40, 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 v13:
>>   - Remove part specific set_4byte_addr_mode()
>>   - Revert SNOR_F_4B_OPCODES
>>   - Add post_sfdp to fix 3 byte erase opcode in 4BAIT
>>
>> Changes in v12:
>>   - Cleanup fixups based on other patches in this series
>>   - Add part specific set_4byte_addr_mode()
>>   - Unset SNOR_F_4B_OPCODES flag to let core to call set_4byte_addr_mode
>>
>> 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 | 66 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 952d4383f9da..e56c48c3280b 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -203,6 +203,56 @@ static int cypress_nor_set_page_size(struct spi_nor *nor)
>>         return 0;
>>  }
>>
>> +static int
>> +s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>> +                       const struct sfdp_parameter_header *bfpt_header,
>> +                       const struct sfdp_bfpt *bfpt)
>> +{
>> +       /* Replace Quad Enable with volatile version */
>> +       nor->params->quad_enable = cypress_nor_quad_enable_volatile;
>> +
>> +       return cypress_nor_set_page_size(nor);
>> +}
>> +
>> +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor)
>> +{
>> +       struct spi_nor_erase_type *erase_type =
>> +                                       nor->params->erase_map.erase_type;
>> +       int i;
>> +
>> +       /*
>> +        * In some parts, 3byte erase opcodes are advertised by 4BAIT.
>> +        * Convert them to 4byte erase opcodes.
> 
> this is a fix, those opcodes don't work with 4 byte addresses, right?
Right. If we want to use 3byte opcodes with 4byte address, need to call
set_4byte_addr_mode().

> Looks good!
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
Thank you!


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

Re: [PATCH v13 2/4] mtd: spi-nor: spansion: Add support for volatile QE bit

[Tudor.Ambarus]

On 4/21/22 13:47, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/21/2022 7:41 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/21/22 12:40, tkuw584924@gmail.com wrote:
> [...]
>>> +/**
>>> + * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
>>> + *                                      register.
>>> + * @nor:       pointer to a 'struct spi_nor'
>>> + *
>>> + * It is recommended to update volatile registers in the field application due
>>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>>> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
>>> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
>>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
>>> + * also set during Flash power-up.
>>> + *
>>> + * Return: 0 on success, -errno otherwise.
>>> + */
>>> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>>> +{
>>> +       struct spi_mem_op op;
>>> +       u8 cfr1v_written;
>>> +       int ret;
>>> +
>>> +       op = (struct spi_mem_op)
>>> +               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
>> nor->addr_width is 3, isn't it? can we use nor->addr_width instead of 3, please?
>>
> No, at the time this method is called, nor->addr_width is set to 4 by
> spi_nor_set_addr_width().

I see. Allow me some time to re-read this.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Michael Walle]

> On 4/21/22 12:40, 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>
>> 
>> In 4BAIT parse, keep nor->addr_width because it may be used as
>> current address mode in SMPT parse later on.
>> 
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>>  drivers/mtd/spi-nor/sfdp.c | 1 -
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 40ba45328975..87603a99938f 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>>  {
>>         if (nor->addr_width) {
>> -               /* already configured from SFDP */
>> +               /*
>> +                * Already configured from SFDP. Use an address width of 4 in
>> +                * case the device has 4byte opcodes.
>> +                */
>> +               if (nor->addr_width == 3 && nor->flags & SNOR_F_HAS_4BAIT)
>> +                       nor->addr_width = 4;
>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> 
> Can we have this instead?
> 
> commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
> Author: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Date:   Thu Apr 21 18:40:21 2022 +0900
> 
>     mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
>     
>     In 4BAIT parse, keep nor->addr_width because it may be used as
>     current address mode in SMPT parse later on.

Mh, I don't know it that is any better, there are places where
addr_width is set in parse_bfpt. Why can't we fix the real problem
here and collect any changes made by the SFDP parsing (and possible
fixups) and apply them at a common place?

-michael

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

Re: [PATCH v13 2/4] mtd: spi-nor: spansion: Add support for volatile QE bit

[Tudor.Ambarus]

On 4/21/22 13:56, Tudor.Ambarus@microchip.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/21/22 13:47, Takahiro Kuwano wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/21/2022 7:41 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/21/22 12:40, tkuw584924@gmail.com wrote:
>> [...]
>>>> +/**
>>>> + * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
>>>> + *                                      register.
>>>> + * @nor:       pointer to a 'struct spi_nor'
>>>> + *
>>>> + * It is recommended to update volatile registers in the field application due
>>>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>>>> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
>>>> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
>>>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
>>>> + * also set during Flash power-up.
>>>> + *
>>>> + * Return: 0 on success, -errno otherwise.
>>>> + */
>>>> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>>>> +{
>>>> +       struct spi_mem_op op;
>>>> +       u8 cfr1v_written;
>>>> +       int ret;
>>>> +
>>>> +       op = (struct spi_mem_op)
>>>> +               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
>>> nor->addr_width is 3, isn't it? can we use nor->addr_width instead of 3, please?
>>>
>> No, at the time this method is called, nor->addr_width is set to 4 by
>> spi_nor_set_addr_width().
> 
> I see. Allow me some time to re-read this.

Does this help you?
https://github.com/ambarus/linux-0day/commit/05f20ab7ee349628f0e2d22a4d3852038a6c8c70

Find the entire branch at:
https://github.com/ambarus/linux-0day/commits/spi-nor/next

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

Re: [PATCH v13 2/4] mtd: spi-nor: spansion: Add support for volatile QE bit

[Tudor.Ambarus]

On 4/21/22 14:36, Tudor.Ambarus@microchip.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 4/21/22 13:56, Tudor.Ambarus@microchip.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/21/22 13:47, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 4/21/2022 7:41 PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 4/21/22 12:40, tkuw584924@gmail.com wrote:
>>> [...]
>>>>> +/**
>>>>> + * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
>>>>> + *                                      register.
>>>>> + * @nor:       pointer to a 'struct spi_nor'
>>>>> + *
>>>>> + * It is recommended to update volatile registers in the field application due
>>>>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>>>>> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
>>>>> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
>>>>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
>>>>> + * also set during Flash power-up.
>>>>> + *
>>>>> + * Return: 0 on success, -errno otherwise.
>>>>> + */
>>>>> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>>>>> +{
>>>>> +       struct spi_mem_op op;
>>>>> +       u8 cfr1v_written;
>>>>> +       int ret;
>>>>> +
>>>>> +       op = (struct spi_mem_op)
>>>>> +               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
>>>> nor->addr_width is 3, isn't it? can we use nor->addr_width instead of 3, please?
>>>>
>>> No, at the time this method is called, nor->addr_width is set to 4 by
>>> spi_nor_set_addr_width().
>>
>> I see. Allow me some time to re-read this.
> 
> Does this help you?
> https://github.com/ambarus/linux-0day/commit/05f20ab7ee349628f0e2d22a4d3852038a6c8c70

commit 05f20ab7ee349628f0e2d22a4d3852038a6c8c70
Author: Tudor Ambarus <tudor.ambarus@microchip.com>
Date:   Thu Apr 21 14:15:42 2022 +0300

    mtd: spi-nor: core: Couple the number of address bytes with the address mode
    
    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.
    Such a volatile configuration register is used to enable the Quad mode.
    The register write sequence requires the number of bytes of address in
    order to be programmed. As it was before, the nor->addr_width was set to 4
    before calling the volatile Quad enable method. This was incorrect as the
    address mode was still at default (3-byte address), which resulted in
    incorrect register configuration.
    Move the setting of the number of bytes of adress after the the Quad enable
    method to allow reads or writes to registers that reguire the number of
    address bytes to work with the default address mode. Now the number of
    address bytes and the adress mode are tightly coupled, which is a natural
    change.
    Other (standard) Quad Enable methods are not affected, as they don't
    require the number of address bytes, so no functionality changes expected.
    
    Reported-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
    Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 7095bf897318..a057b177dca5 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2270,52 +2270,6 @@ static int spi_nor_default_setup(struct spi_nor *nor,
 	return 0;
 }
 
-static int spi_nor_set_addr_width(struct spi_nor *nor)
-{
-	if (nor->flags & SNOR_F_HAS_4BAIT)
-		nor->addr_width = 4;
-
-	if (nor->addr_width) {
-		/* already configured from SFDP */
-	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
-		/*
-		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
-		 * in this protocol an odd address width cannot be used because
-		 * then the address phase would only span a cycle and a half.
-		 * Half a cycle would be left over. We would then have to start
-		 * the dummy phase in the middle of a cycle and so too the data
-		 * phase, and we will end the transaction with half a cycle left
-		 * over.
-		 *
-		 * Force all 8D-8D-8D flashes to use an address width of 4 to
-		 * avoid this situation.
-		 */
-		nor->addr_width = 4;
-	} else if (nor->info->addr_width) {
-		nor->addr_width = nor->info->addr_width;
-	} else {
-		nor->addr_width = 3;
-	}
-
-	if (nor->addr_width == 3 && nor->params->size > 0x1000000) {
-		/* enable 4-byte addressing if the device exceeds 16MiB */
-		nor->addr_width = 4;
-	}
-
-	if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) {
-		dev_dbg(nor->dev, "address width is too large: %u\n",
-			nor->addr_width);
-		return -EINVAL;
-	}
-
-	/* Set 4byte opcodes when possible. */
-	if (nor->addr_width == 4 && nor->flags & SNOR_F_4B_OPCODES &&
-	    !(nor->flags & SNOR_F_HAS_4BAIT))
-		spi_nor_set_4byte_opcodes(nor);
-
-	return 0;
-}
-
 static int spi_nor_setup(struct spi_nor *nor,
 			 const struct spi_nor_hwcaps *hwcaps)
 {
@@ -2325,10 +2279,7 @@ static int spi_nor_setup(struct spi_nor *nor,
 		ret = nor->params->setup(nor, hwcaps);
 	else
 		ret = spi_nor_default_setup(nor, hwcaps);
-	if (ret)
-		return ret;
-
-	return spi_nor_set_addr_width(nor);
+	return ret;
 }
 
 /**
@@ -2711,6 +2662,78 @@ static int spi_nor_quad_enable(struct spi_nor *nor)
 	return nor->params->quad_enable(nor);
 }
 
+static int spi_nor_set_addr_width(struct spi_nor *nor)
+{
+	if (nor->flags & SNOR_F_HAS_4BAIT)
+		nor->addr_width = 4;
+
+	if (nor->addr_width) {
+		/* already configured from SFDP */
+	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
+		/*
+		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
+		 * in this protocol an odd address width cannot be used because
+		 * then the address phase would only span a cycle and a half.
+		 * Half a cycle would be left over. We would then have to start
+		 * the dummy phase in the middle of a cycle and so too the data
+		 * phase, and we will end the transaction with half a cycle left
+		 * over.
+		 *
+		 * Force all 8D-8D-8D flashes to use an address width of 4 to
+		 * avoid this situation.
+		 */
+		nor->addr_width = 4;
+	} else if (nor->info->addr_width) {
+		nor->addr_width = nor->info->addr_width;
+	} else {
+		nor->addr_width = 3;
+	}
+
+	if (nor->addr_width == 3 && nor->params->size > 0x1000000) {
+		/* enable 4-byte addressing if the device exceeds 16MiB */
+		nor->addr_width = 4;
+	}
+
+	if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) {
+		dev_dbg(nor->dev, "address width is too large: %u\n",
+			nor->addr_width);
+		return -EINVAL;
+	}
+
+	/* Set 4byte opcodes when possible. */
+	if (nor->addr_width == 4 && nor->flags & SNOR_F_4B_OPCODES &&
+	    !(nor->flags & SNOR_F_HAS_4BAIT))
+		spi_nor_set_4byte_opcodes(nor);
+
+	return 0;
+}
+
+static int spi_nor_set_addr_mode(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = spi_nor_set_addr_width(nor);
+	if (ret)
+		return ret;
+
+	if (nor->addr_width == 4 &&
+	    nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
+	    !(nor->flags & SNOR_F_4B_OPCODES)) {
+		/*
+		 * If the RESET# pin isn't hooked up properly, or the system
+		 * otherwise doesn't perform a reset command in the boot
+		 * sequence, it's impossible to 100% protect against unexpected
+		 * reboots (e.g., crashes). Warn the user (or hopefully, system
+		 * designer) that this is bad.
+		 */
+		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
+			  "enabling reset hack; may not recover from unexpected reboots\n");
+		nor->params->set_4byte_addr_mode(nor, true);
+	}
+
+	return 0;
+}
+
 static int spi_nor_init(struct spi_nor *nor)
 {
 	int err;
@@ -2742,22 +2765,7 @@ static int spi_nor_init(struct spi_nor *nor)
 	     nor->flags & SNOR_F_SWP_IS_VOLATILE))
 		spi_nor_try_unlock_all(nor);
 
-	if (nor->addr_width == 4 &&
-	    nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
-	    !(nor->flags & SNOR_F_4B_OPCODES)) {
-		/*
-		 * If the RESET# pin isn't hooked up properly, or the system
-		 * otherwise doesn't perform a reset command in the boot
-		 * sequence, it's impossible to 100% protect against unexpected
-		 * reboots (e.g., crashes). Warn the user (or hopefully, system
-		 * designer) that this is bad.
-		 */
-		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
-			  "enabling reset hack; may not recover from unexpected reboots\n");
-		nor->params->set_4byte_addr_mode(nor, true);
-	}
-
-	return 0;
+	return spi_nor_set_addr_mode(nor);
 }


> 
> Find the entire branch at:
> https://github.com/ambarus/linux-0day/commits/spi-nor/next
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Tudor.Ambarus]

On 4/21/22 14:29, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> On 4/21/22 12:40, 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>
>>>
>>> In 4BAIT parse, keep nor->addr_width because it may be used as
>>> current address mode in SMPT parse later on.
>>>
>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>> ---
>>>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>>>  drivers/mtd/spi-nor/sfdp.c | 1 -
>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 40ba45328975..87603a99938f 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>>>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>  {
>>>         if (nor->addr_width) {
>>> -               /* already configured from SFDP */
>>> +               /*
>>> +                * Already configured from SFDP. Use an address width of 4 in
>>> +                * case the device has 4byte opcodes.
>>> +                */
>>> +               if (nor->addr_width == 3 && nor->flags & SNOR_F_HAS_4BAIT)
>>> +                       nor->addr_width = 4;
>>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>>
>> Can we have this instead?
>>
>> commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
>> Author: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> Date:   Thu Apr 21 18:40:21 2022 +0900
>>
>>     mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
>>
>>     In 4BAIT parse, keep nor->addr_width because it may be used as
>>     current address mode in SMPT parse later on.
> 
> Mh, I don't know it that is any better, there are places where
> addr_width is set in parse_bfpt. Why can't we fix the real problem

which I find it correct. The only thing that worth attention is at
BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, which we're already taken care of. We
don't change the addr mode at parse time and use SNOR_F_HAS_4BAIT to change
the number of bytes in the aforementioned case. You may check the other patch
that I've submitted as a reply to this patch set, it should show the bigger
picture.


> here and collect any changes made by the SFDP parsing (and possible
> fixups) and apply them at a common place?
> 
> -michael

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Michael Walle]

Am 2022-04-21 14:06, schrieb Tudor.Ambarus@microchip.com:
> On 4/21/22 14:29, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>>> On 4/21/22 12:40, 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>
>>>> 
>>>> In 4BAIT parse, keep nor->addr_width because it may be used as
>>>> current address mode in SMPT parse later on.
>>>> 
>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>>>>  drivers/mtd/spi-nor/sfdp.c | 1 -
>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 40ba45328975..87603a99938f 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct 
>>>> spi_nor *nor,
>>>>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>>  {
>>>>         if (nor->addr_width) {
>>>> -               /* already configured from SFDP */
>>>> +               /*
>>>> +                * Already configured from SFDP. Use an address 
>>>> width of 4 in
>>>> +                * case the device has 4byte opcodes.
>>>> +                */
>>>> +               if (nor->addr_width == 3 && nor->flags & 
>>>> SNOR_F_HAS_4BAIT)
>>>> +                       nor->addr_width = 4;
>>>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>>> 
>>> Can we have this instead?
>>> 
>>> commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
>>> Author: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>> Date:   Thu Apr 21 18:40:21 2022 +0900
>>> 
>>>     mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
>>> 
>>>     In 4BAIT parse, keep nor->addr_width because it may be used as
>>>     current address mode in SMPT parse later on.
>> 
>> Mh, I don't know it that is any better, there are places where
>> addr_width is set in parse_bfpt. Why can't we fix the real problem
> 
> which I find it correct. The only thing that worth attention is at
> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, which we're already taken care of. We
> don't change the addr mode at parse time and use SNOR_F_HAS_4BAIT to 
> change
> the number of bytes in the aforementioned case.

It's not about the correctness but that we have a clear sync point
and code flow.

Which parameters can you change in the parse_sfdp() which can't you
change, you'll have to document that and even then it is really hard
to follow. To make thing worse, it turns out, you can sometimes
change addr_width and sometimes it doesn't matter?
If the parsing wouldn't change any runtime parameters we wouldn't
have this problem at all. no?

The parse_sfdp() should only change members of struct
spi_nor_flash_parameters, the caller will then decide if they
should be used and more imporantly *when* they should be used.

Then you can do the sane thing in spi_nor_set_addr_width():
setting the addr_width.

Right now it's:

+static int spi_nor_set_addr_width(struct spi_nor *nor)
+{
+	if (nor->flags & SNOR_F_HAS_4BAIT)
+		nor->addr_width = 4;
+
+	if (nor->addr_width) {
+		/* already configured from SFDP */
..

Sometimes it will set addr_width, sometimes it will not be set
and every once in a while 4byte mode is determined by SFDP but
it is not configured (SNOR_F_HAS_4BAIT).

-michael

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Tudor.Ambarus]

On 4/21/22 16:01, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-04-21 14:06, schrieb Tudor.Ambarus@microchip.com:
>> On 4/21/22 14:29, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>>> On 4/21/22 12:40, 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>
>>>>>
>>>>> In 4BAIT parse, keep nor->addr_width because it may be used as
>>>>> current address mode in SMPT parse later on.
>>>>>
>>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/core.c | 7 ++++++-
>>>>>  drivers/mtd/spi-nor/sfdp.c | 1 -
>>>>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>> index 40ba45328975..87603a99938f 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>> @@ -2210,7 +2210,12 @@ static int spi_nor_default_setup(struct
>>>>> spi_nor *nor,
>>>>>  static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>>>  {
>>>>>         if (nor->addr_width) {
>>>>> -               /* already configured from SFDP */
>>>>> +               /*
>>>>> +                * Already configured from SFDP. Use an address
>>>>> width of 4 in
>>>>> +                * case the device has 4byte opcodes.
>>>>> +                */
>>>>> +               if (nor->addr_width == 3 && nor->flags &
>>>>> SNOR_F_HAS_4BAIT)
>>>>> +                       nor->addr_width = 4;
>>>>>         } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>>>>
>>>> Can we have this instead?
>>>>
>>>> commit 61d73dea7e63db4c7a3ffaa7f2b5068fb71c2d8b
>>>> Author: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>> Date:   Thu Apr 21 18:40:21 2022 +0900
>>>>
>>>>     mtd: spi-nor: Retain nor->addr_width at 4BAIT parse
>>>>
>>>>     In 4BAIT parse, keep nor->addr_width because it may be used as
>>>>     current address mode in SMPT parse later on.
>>>
>>> Mh, I don't know it that is any better, there are places where
>>> addr_width is set in parse_bfpt. Why can't we fix the real problem
>>
>> which I find it correct. The only thing that worth attention is at
>> BFPT_DWORD1_ADDRESS_BYTES_3_OR_4, which we're already taken care of. We
>> don't change the addr mode at parse time and use SNOR_F_HAS_4BAIT to
>> change
>> the number of bytes in the aforementioned case.
> 
> It's not about the correctness but that we have a clear sync point
> and code flow.
> 
> Which parameters can you change in the parse_sfdp() which can't you
> change, you'll have to document that and even then it is really hard
> to follow. To make thing worse, it turns out, you can sometimes
> change addr_width and sometimes it doesn't matter?

no, it matters all the time. nor->addr_width (which I'll rename to
nor->addr_nbytes btw) is a particular case and we shouldn't
generalize it, I think.

nor->addr_width is initialized only in sfdp in:
        /* Number of address bytes. */                                          
        switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {  
        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:                                  
        case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:                                  
                nor->addr_width = 3;                                            
                break;                                                          
                                                                                
        case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:                                  
                nor->addr_width = 4;                                            
                break;                                                          
                                                                                
        default:                                                                
                break;                                                          
        }

BFPT_DWORD1_ADDRESS_BYTES_3_ONLY and BFPT_DWORD1_ADDRESS_BYTES_4_ONLY
are clear and we can always set nor->addr_width with them.

For BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 we have the following note in BFPT:
"01b: 3- or 4-Byte addressing (e.g., defaults to 3-Byte mode; enters 4-Byte
mode on command)"

Entering in 4-byte mode and deciding the number of addr nbytes is done after
the SFDP parsing, which we currently done, so we're safe.

> If the parsing wouldn't change any runtime parameters we wouldn't
> have this problem at all. no?

It doesn't change, _but_ it sets the correct number of address nbytes.
> 
> The parse_sfdp() should only change members of struct
> spi_nor_flash_parameters, the caller will then decide if they
> should be used and more imporantly *when* they should be used.

this would mean introducing a nor->params->addr_nbytes, which
is redundant with SNOR_F_HAS_4BAIT.
> 
> Then you can do the sane thing in spi_nor_set_addr_width():
> setting the addr_width.
> 
> Right now it's:
> 
> +static int spi_nor_set_addr_width(struct spi_nor *nor)
> +{
> +       if (nor->flags & SNOR_F_HAS_4BAIT)
> +               nor->addr_width = 4;
> +
> +       if (nor->addr_width) {
> +               /* already configured from SFDP */
> ..
> 
> Sometimes it will set addr_width, sometimes it will not be set
> and every once in a while 4byte mode is determined by SFDP but
> it is not configured (SNOR_F_HAS_4BAIT).

which is good! we prefer using 4b opcodes than entering 4byte address
mode.

> 
> -michael

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Michael Walle]

Am 2022-04-21 15:13, schrieb Tudor.Ambarus@microchip.com:

>> If the parsing wouldn't change any runtime parameters we wouldn't
>> have this problem at all. no?
> 
> It doesn't change, _but_ it sets the correct number of address nbytes.

It changes nor->addr_width which might be used for all commands
except the read_sfdp_data(). It changes it before we are entering
the 4 byte mode. Also parse_sfdp changes the opcodes. It seems this
was the reason for the SNOR_F_HAS_4BAIT flag in the first place,
so the core doesn't convert the opcodes again.

>> The parse_sfdp() should only change members of struct
>> spi_nor_flash_parameters, the caller will then decide if they
>> should be used and more imporantly *when* they should be used.
> 
> this would mean introducing a nor->params->addr_nbytes, which
> is redundant with SNOR_F_HAS_4BAIT.

So? The SFDP table has both information, I don't see a problem
with that. And I'm not sure they are redunant, I think a flash
can have 4 byte addresses and no 4BAIT table.

And if it would be redundant why do we need that empty
case below..

>> Then you can do the sane thing in spi_nor_set_addr_width():
>> setting the addr_width.
>> 
>> Right now it's:
>> 
>> +static int spi_nor_set_addr_width(struct spi_nor *nor)
>> +{
>> +       if (nor->flags & SNOR_F_HAS_4BAIT)
>> +               nor->addr_width = 4;
>> +
>> +       if (nor->addr_width) {
>> +               /* already configured from SFDP */
>> +       }
>> ..

here.

>> 
>> Sometimes it will set addr_width, sometimes it will not be set
>> and every once in a while 4byte mode is determined by SFDP but
>> it is not configured (SNOR_F_HAS_4BAIT).
> 
> which is good! we prefer using 4b opcodes than entering 4byte address
> mode.

You didn't understand my point. All the assignments of addr_width
are clustered around in the code. Why can't we have them in a common
place. We even have this place already: spi_nor_set_addr_width().
Also you could probably get rid of that "don't change opcodes
if SNOR_F_HAS_4BAIT is set" thingy if the parsing code wouldn't
change the opcodes but returns the parsed ones for the core
to decide what to use. With the benefit of better readability
and lesser bugs.

-michael

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Tudor.Ambarus]

On 4/21/22 16:42, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-04-21 15:13, schrieb Tudor.Ambarus@microchip.com:
> 
>>> If the parsing wouldn't change any runtime parameters we wouldn't
>>> have this problem at all. no?
>>
>> It doesn't change, _but_ it sets the correct number of address nbytes.
> 
> It changes nor->addr_width which might be used for all commands

no, it _sets_ the nor->addr_width. nor->addr_width is initialized only
in BFPT, where BFPT is present. No change done.

> except the read_sfdp_data(). It changes it before we are entering
> the 4 byte mode. Also parse_sfdp changes the opcodes. It seems this
> was the reason for the SNOR_F_HAS_4BAIT flag in the first place,
> so the core doesn't convert the opcodes again.
> 
>>> The parse_sfdp() should only change members of struct
>>> spi_nor_flash_parameters, the caller will then decide if they
>>> should be used and more imporantly *when* they should be used.
>>
>> this would mean introducing a nor->params->addr_nbytes, which
>> is redundant with SNOR_F_HAS_4BAIT.
> 
> So? The SFDP table has both information, I don't see a problem
> with that. And I'm not sure they are redunant, I think a flash
> can have 4 byte addresses and no 4BAIT table.

right, but this is not something that we are addressing right now.
> 
> And if it would be redundant why do we need that empty
> case below..
> 
>>> Then you can do the sane thing in spi_nor_set_addr_width():
>>> setting the addr_width.
>>>
>>> Right now it's:
>>>
>>> +static int spi_nor_set_addr_width(struct spi_nor *nor)
>>> +{
>>> +       if (nor->flags & SNOR_F_HAS_4BAIT)
>>> +               nor->addr_width = 4;
>>> +
>>> +       if (nor->addr_width) {
>>> +               /* already configured from SFDP */
>>> +       }
>>> ..
> 
> here.
> 
>>>
>>> Sometimes it will set addr_width, sometimes it will not be set
>>> and every once in a while 4byte mode is determined by SFDP but
>>> it is not configured (SNOR_F_HAS_4BAIT).
>>
>> which is good! we prefer using 4b opcodes than entering 4byte address
>> mode.
> 
> You didn't understand my point. All the assignments of addr_width
> are clustered around in the code. Why can't we have them in a common

set in bfpt and then at updated at flash init. It's not spread
throughout the code.

> place. We even have this place already: spi_nor_set_addr_width().
> Also you could probably get rid of that "don't change opcodes
> if SNOR_F_HAS_4BAIT is set" thingy if the parsing code wouldn't
> change the opcodes but returns the parsed ones for the core
> to decide what to use. With the benefit of better readability
> and lesser bugs.
> 

I think I understood you from the beginning. Both approaches are fine
IMO, but seems that you care about yours, so let's implement your
suggestion. Takahiro, will you handle it, or do you want me to do it?

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Takahiro Kuwano]

On 4/21/2022 10:56 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/21/22 16:42, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Am 2022-04-21 15:13, schrieb Tudor.Ambarus@microchip.com:
>>
>>>> If the parsing wouldn't change any runtime parameters we wouldn't
>>>> have this problem at all. no?
>>>
>>> It doesn't change, _but_ it sets the correct number of address nbytes.
>>
>> It changes nor->addr_width which might be used for all commands
> 
> no, it _sets_ the nor->addr_width. nor->addr_width is initialized only
> in BFPT, where BFPT is present. No change done.
> 
>> except the read_sfdp_data(). It changes it before we are entering
>> the 4 byte mode. Also parse_sfdp changes the opcodes. It seems this
>> was the reason for the SNOR_F_HAS_4BAIT flag in the first place,
>> so the core doesn't convert the opcodes again.
>>
>>>> The parse_sfdp() should only change members of struct
>>>> spi_nor_flash_parameters, the caller will then decide if they
>>>> should be used and more imporantly *when* they should be used.
>>>
>>> this would mean introducing a nor->params->addr_nbytes, which
>>> is redundant with SNOR_F_HAS_4BAIT.
>>
>> So? The SFDP table has both information, I don't see a problem
>> with that. And I'm not sure they are redunant, I think a flash
>> can have 4 byte addresses and no 4BAIT table.
> 
> right, but this is not something that we are addressing right now.
>>
>> And if it would be redundant why do we need that empty
>> case below..
>>
>>>> Then you can do the sane thing in spi_nor_set_addr_width():
>>>> setting the addr_width.
>>>>
>>>> Right now it's:
>>>>
>>>> +static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>> +{
>>>> +       if (nor->flags & SNOR_F_HAS_4BAIT)
>>>> +               nor->addr_width = 4;
>>>> +
>>>> +       if (nor->addr_width) {
>>>> +               /* already configured from SFDP */
>>>> +       }
>>>> ..
>>
>> here.
>>
>>>>
>>>> Sometimes it will set addr_width, sometimes it will not be set
>>>> and every once in a while 4byte mode is determined by SFDP but
>>>> it is not configured (SNOR_F_HAS_4BAIT).
>>>
>>> which is good! we prefer using 4b opcodes than entering 4byte address
>>> mode.
>>
>> You didn't understand my point. All the assignments of addr_width
>> are clustered around in the code. Why can't we have them in a common
> 
> set in bfpt and then at updated at flash init. It's not spread
> throughout the code.
> 
>> place. We even have this place already: spi_nor_set_addr_width().
>> Also you could probably get rid of that "don't change opcodes
>> if SNOR_F_HAS_4BAIT is set" thingy if the parsing code wouldn't
>> change the opcodes but returns the parsed ones for the core
>> to decide what to use. With the benefit of better readability
>> and lesser bugs.
>>
> 
> I think I understood you from the beginning. Both approaches are fine
> IMO, but seems that you care about yours, so let's implement your
> suggestion. Takahiro, will you handle it, or do you want me to do it?
> 
I want you to do it, please.

> Thanks,
> ta

Thank you!

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

Re: [PATCH v13 2/4] mtd: spi-nor: spansion: Add support for volatile QE bit

[Takahiro Kuwano]

On 4/21/2022 8:48 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/21/22 14:36, Tudor.Ambarus@microchip.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 4/21/22 13:56, Tudor.Ambarus@microchip.com wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 4/21/22 13:47, Takahiro Kuwano wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 4/21/2022 7:41 PM, Tudor.Ambarus@microchip.com wrote:
>>>>> On 4/21/22 12:40, tkuw584924@gmail.com wrote:
>>>> [...]
>>>>>> +/**
>>>>>> + * cypress_nor_quad_enable_volatile() - enable Quad I/O mode in volatile
>>>>>> + *                                      register.
>>>>>> + * @nor:       pointer to a 'struct spi_nor'
>>>>>> + *
>>>>>> + * It is recommended to update volatile registers in the field application due
>>>>>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>>>>>> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
>>>>>> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
>>>>>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
>>>>>> + * also set during Flash power-up.
>>>>>> + *
>>>>>> + * Return: 0 on success, -errno otherwise.
>>>>>> + */
>>>>>> +static int cypress_nor_quad_enable_volatile(struct spi_nor *nor)
>>>>>> +{
>>>>>> +       struct spi_mem_op op;
>>>>>> +       u8 cfr1v_written;
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       op = (struct spi_mem_op)
>>>>>> +               CYPRESS_NOR_RD_ANY_REG_OP(3, SPINOR_REG_CYPRESS_CFR1V,
>>>>> nor->addr_width is 3, isn't it? can we use nor->addr_width instead of 3, please?
>>>>>
>>>> No, at the time this method is called, nor->addr_width is set to 4 by
>>>> spi_nor_set_addr_width().
>>>
>>> I see. Allow me some time to re-read this.
>>
>> Does this help you?
>> https://github.com/ambarus/linux-0day/commit/05f20ab7ee349628f0e2d22a4d3852038a6c8c70
> 
Yes, I have confirmed it's working on my hardware setup. Thanks a lot.

Some typos in commit description.
> commit 05f20ab7ee349628f0e2d22a4d3852038a6c8c70
> Author: Tudor Ambarus <tudor.ambarus@microchip.com>
> Date:   Thu Apr 21 14:15:42 2022 +0300
> 
>     mtd: spi-nor: core: Couple the number of address bytes with the address mode
>     
>     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.
>     Such a volatile configuration register is used to enable the Quad mode.
>     The register write sequence requires the number of bytes of address in
>     order to be programmed. As it was before, the nor->addr_width was set to 4
>     before calling the volatile Quad enable method. This was incorrect as the
>     address mode was still at default (3-byte address), which resulted in
>     incorrect register configuration.
>     Move the setting of the number of bytes of adress after the the Quad enable
s/adress/address

>     method to allow reads or writes to registers that reguire the number of
s/reguire/require

>     address bytes to work with the default address mode. Now the number of
>     address bytes and the adress mode are tightly coupled, which is a natural
s/adress/address

Thanks again,
Takahiro

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

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

[Takahiro Kuwano]

On 4/21/2022 7:43 PM, Tudor.Ambarus@microchip.com wrote:
> On 4/21/22 12:40, 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>
>> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>> Changes in v13:
>>   - (no change)
>>
>> Changes in v12:
>>   - Rebase on top of Tudor's series
>>     https://patchwork.ozlabs.org/project/linux-mtd/list/?series=295933
>>   - Remove addr_width param. Use nor->addr_width instead.
>>
>> 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 65ae3ade0b95..952d4383f9da 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -172,6 +172,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'
>> + *
>> + * 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)
>> +{
>> +       struct spi_mem_op op =
>> +               CYPRESS_NOR_RD_ANY_REG_OP(nor->addr_width,
>> +                                         SPINOR_REG_CYPRESS_CFR3V,
>> +                                         nor->bouncebuf);
>> +       int ret;
>> +
>> +       ret = spi_nor_read_any_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'
>> @@ -226,28 +257,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,
> 
> here you substitute 3 with nor->addr_width behind the doors. It would have
> worth to have a dedicated patch for this and then come with a patch for
> the local function. Or at least add a comment in the commit's description.
> 
I will update commit description.

>> -                                         nor->bouncebuf);
>> -       int ret;
>> -
>> -       spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
>> -
>> -       ret = spi_mem_exec_op(nor->spimem, &op);
These are replaced with spi_nor_read_any_reg().
I will also add a comment about this.

Thanks,
Takahiro

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Takahiro Kuwano]

Hi Tudor,

On 4/21/2022 11:26 PM, Takahiro Kuwano wrote:
> 
> 
> On 4/21/2022 10:56 PM, Tudor.Ambarus@microchip.com wrote:
>> On 4/21/22 16:42, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Am 2022-04-21 15:13, schrieb Tudor.Ambarus@microchip.com:
>>>
>>>>> If the parsing wouldn't change any runtime parameters we wouldn't
>>>>> have this problem at all. no?
>>>>
>>>> It doesn't change, _but_ it sets the correct number of address nbytes.
>>>
>>> It changes nor->addr_width which might be used for all commands
>>
>> no, it _sets_ the nor->addr_width. nor->addr_width is initialized only
>> in BFPT, where BFPT is present. No change done.
>>
>>> except the read_sfdp_data(). It changes it before we are entering
>>> the 4 byte mode. Also parse_sfdp changes the opcodes. It seems this
>>> was the reason for the SNOR_F_HAS_4BAIT flag in the first place,
>>> so the core doesn't convert the opcodes again.
>>>
>>>>> The parse_sfdp() should only change members of struct
>>>>> spi_nor_flash_parameters, the caller will then decide if they
>>>>> should be used and more imporantly *when* they should be used.
>>>>
>>>> this would mean introducing a nor->params->addr_nbytes, which
>>>> is redundant with SNOR_F_HAS_4BAIT.
>>>
>>> So? The SFDP table has both information, I don't see a problem
>>> with that. And I'm not sure they are redunant, I think a flash
>>> can have 4 byte addresses and no 4BAIT table.
>>
>> right, but this is not something that we are addressing right now.
>>>
>>> And if it would be redundant why do we need that empty
>>> case below..
>>>
>>>>> Then you can do the sane thing in spi_nor_set_addr_width():
>>>>> setting the addr_width.
>>>>>
>>>>> Right now it's:
>>>>>
>>>>> +static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>>> +{
>>>>> +       if (nor->flags & SNOR_F_HAS_4BAIT)
>>>>> +               nor->addr_width = 4;
>>>>> +
>>>>> +       if (nor->addr_width) {
>>>>> +               /* already configured from SFDP */
>>>>> +       }
>>>>> ..
>>>
>>> here.
>>>
>>>>>
>>>>> Sometimes it will set addr_width, sometimes it will not be set
>>>>> and every once in a while 4byte mode is determined by SFDP but
>>>>> it is not configured (SNOR_F_HAS_4BAIT).
>>>>
>>>> which is good! we prefer using 4b opcodes than entering 4byte address
>>>> mode.
>>>
>>> You didn't understand my point. All the assignments of addr_width
>>> are clustered around in the code. Why can't we have them in a common
>>
>> set in bfpt and then at updated at flash init. It's not spread
>> throughout the code.
>>
>>> place. We even have this place already: spi_nor_set_addr_width().
>>> Also you could probably get rid of that "don't change opcodes
>>> if SNOR_F_HAS_4BAIT is set" thingy if the parsing code wouldn't
>>> change the opcodes but returns the parsed ones for the core
>>> to decide what to use. With the benefit of better readability
>>> and lesser bugs.
>>>
>>
>> I think I understood you from the beginning. Both approaches are fine
>> IMO, but seems that you care about yours, so let's implement your
>> suggestion. Takahiro, will you handle it, or do you want me to do it?
>>
> I want you to do it, please.
> 
Do you plan to help on another implementation shortly?
Or can we go with this one for now?

Best Regards,
Takahiro

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

Re: [PATCH v13 1/4] mtd: spi-nor: Retain nor->addr_width at 4BAIT parse

[Tudor.Ambarus]

On 4/27/22 07:16, Takahiro Kuwano wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> On 4/21/2022 11:26 PM, Takahiro Kuwano wrote:
>>
>>
>> On 4/21/2022 10:56 PM, Tudor.Ambarus@microchip.com wrote:
>>> On 4/21/22 16:42, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Am 2022-04-21 15:13, schrieb Tudor.Ambarus@microchip.com:
>>>>
>>>>>> If the parsing wouldn't change any runtime parameters we wouldn't
>>>>>> have this problem at all. no?
>>>>>
>>>>> It doesn't change, _but_ it sets the correct number of address nbytes.
>>>>
>>>> It changes nor->addr_width which might be used for all commands
>>>
>>> no, it _sets_ the nor->addr_width. nor->addr_width is initialized only
>>> in BFPT, where BFPT is present. No change done.
>>>
>>>> except the read_sfdp_data(). It changes it before we are entering
>>>> the 4 byte mode. Also parse_sfdp changes the opcodes. It seems this
>>>> was the reason for the SNOR_F_HAS_4BAIT flag in the first place,
>>>> so the core doesn't convert the opcodes again.
>>>>
>>>>>> The parse_sfdp() should only change members of struct
>>>>>> spi_nor_flash_parameters, the caller will then decide if they
>>>>>> should be used and more imporantly *when* they should be used.
>>>>>
>>>>> this would mean introducing a nor->params->addr_nbytes, which
>>>>> is redundant with SNOR_F_HAS_4BAIT.
>>>>
>>>> So? The SFDP table has both information, I don't see a problem
>>>> with that. And I'm not sure they are redunant, I think a flash
>>>> can have 4 byte addresses and no 4BAIT table.
>>>
>>> right, but this is not something that we are addressing right now.
>>>>
>>>> And if it would be redundant why do we need that empty
>>>> case below..
>>>>
>>>>>> Then you can do the sane thing in spi_nor_set_addr_width():
>>>>>> setting the addr_width.
>>>>>>
>>>>>> Right now it's:
>>>>>>
>>>>>> +static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>>>> +{
>>>>>> +       if (nor->flags & SNOR_F_HAS_4BAIT)
>>>>>> +               nor->addr_width = 4;
>>>>>> +
>>>>>> +       if (nor->addr_width) {
>>>>>> +               /* already configured from SFDP */
>>>>>> +       }
>>>>>> ..
>>>>
>>>> here.
>>>>
>>>>>>
>>>>>> Sometimes it will set addr_width, sometimes it will not be set
>>>>>> and every once in a while 4byte mode is determined by SFDP but
>>>>>> it is not configured (SNOR_F_HAS_4BAIT).
>>>>>
>>>>> which is good! we prefer using 4b opcodes than entering 4byte address
>>>>> mode.
>>>>
>>>> You didn't understand my point. All the assignments of addr_width
>>>> are clustered around in the code. Why can't we have them in a common
>>>
>>> set in bfpt and then at updated at flash init. It's not spread
>>> throughout the code.
>>>
>>>> place. We even have this place already: spi_nor_set_addr_width().
>>>> Also you could probably get rid of that "don't change opcodes
>>>> if SNOR_F_HAS_4BAIT is set" thingy if the parsing code wouldn't
>>>> change the opcodes but returns the parsed ones for the core
>>>> to decide what to use. With the benefit of better readability
>>>> and lesser bugs.
>>>>
>>>
>>> I think I understood you from the beginning. Both approaches are fine
>>> IMO, but seems that you care about yours, so let's implement your
>>> suggestion. Takahiro, will you handle it, or do you want me to do it?
>>>
>> I want you to do it, please.
>>
> Do you plan to help on another implementation shortly?

yes. I think sometime this week, I'm a little busy right now.

> Or can we go with this one for now?

No, let's implement Michael's suggestion from the start.

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