[PATCH v4 0/9] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t

[PATCH v4 0/9] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t

[tkuw584924 at gmail.com]

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

The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
The datasheets can be found in the following links.

https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)

Tested on Xilinx Zynq-7000 FPGA board.

Takahiro Kuwano (9):
  mtd: spi-nor: Add Cypress manufacturer ID
  mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t
  mtd: spi-nor-core: Add support for Read/Write Any Register
  mtd: spi-nor-core: Add support for volatile QE bit
  mtd: spi-nor-core: Add the ->ready() hook
  mtd: spi-nor-core: Add overlaid sector erase feature
  mtd: spi-nor-core: Add Cypress manufacturer ID in set_4byte
  mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t
  mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t

 drivers/mtd/spi/spi-nor-core.c | 267 +++++++++++++++++++++++++++++++++
 drivers/mtd/spi/spi-nor-ids.c  |  36 +++++
 drivers/mtd/spi/spi-nor-tiny.c |  89 +++++++++++
 include/linux/mtd/spi-nor.h    |   9 +-
 4 files changed, 400 insertions(+), 1 deletion(-)

---
Changes since v4:
  - Added Read/Write Any Register support
  - Added the ->ready() hook to support multi-die package parts
  - Added S25HL02GT/S25HL04GT/S25HS02GT/S25HS04GT support
  
Changes since v3:
  - Split into multiple patches

Changes since v2:
  - Fixed typo in comment for spansion_overlaid_erase()
  - Fixed expressions for addr and len check in spansion_overlaid_erase()
  - Added device ID check to make the changes effective for S25 only
  - Added nor->setup() and fixup hooks based on the following patches
    https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/
    https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/
    https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/

--
2.25.1

[PATCH v4 1/9] mtd: spi-nor: Add Cypress manufacturer ID

[tkuw584924 at gmail.com]

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

This patch adds Cypress manufacturer ID (34h) definition.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 include/linux/mtd/spi-nor.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5842e9d6ee..89e7a4fdcd 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -27,6 +27,7 @@
 #define SNOR_MFR_SPANSION	CFI_MFR_AMD
 #define SNOR_MFR_SST		CFI_MFR_SST
 #define SNOR_MFR_WINBOND	0xef /* Also used by some Spansion */
+#define SNOR_MFR_CYPRESS	0x34
 
 /*
  * Note on opcode nomenclature: some opcodes have a format like
-- 
2.25.1

[PATCH v4 2/9] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t

[tkuw584924 at gmail.com]

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

The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
The datasheets can be found in the following links.

https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)

Tested 512Mb/1Gb/2Gb parts on Xilinx Zynq-7000 FPGA board.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-ids.c | 36 +++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
index 5bd5dd3003..b78d13e980 100644
--- a/drivers/mtd/spi/spi-nor-ids.c
+++ b/drivers/mtd/spi/spi-nor-ids.c
@@ -217,6 +217,42 @@ const struct flash_info spi_nor_ids[] = {
 	{ INFO("s25fl208k",  0x014014,      0,  64 * 1024,  16, SECT_4K | SPI_NOR_DUAL_READ) },
 	{ INFO("s25fl064l",  0x016017,      0,  64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 	{ INFO("s25fl128l",  0x016018,      0,  64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+
+	/* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
+	 * sectors at top and/or bottom, depending on the device configuration.
+	 * To support this, an erase hook makes overlaid sectors appear as
+	 * uniform sectors.
+	 */
+	{ INFO6("s25hl256t",  0x342a19, 0x0f0390, 256 * 1024, 128,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hl512t",  0x342a1a, 0x0f0390, 256 * 1024, 256,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hl01gt",  0x342a1b, 0x0f0390, 256 * 1024, 512,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hl02gt",  0x342a1c, 0x0f0090, 256 * 1024, 1024,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hl04gt",  0x342a1d, 0x0f0090, 256 * 1024, 2048,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hs256t",  0x342b19, 0x0f0390, 256 * 1024, 128,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hs512t",  0x342b1a, 0x0f0390, 256 * 1024, 256,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hs01gt",  0x342b1b, 0x0f0390, 256 * 1024, 512,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hs02gt",  0x342b1c, 0x0f0090, 256 * 1024, 1024,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
+	{ INFO6("s25hs04gt",  0x342b1d, 0x0f0090, 256 * 1024, 2048,
+		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
+		USE_CLSR) },
 #endif
 #ifdef CONFIG_SPI_FLASH_SST		/* SST */
 	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
-- 
2.25.1

[PATCH v4 3/9] mtd: spi-nor-core: Add support for Read/Write Any Register

[tkuw584924 at gmail.com]

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

Some of Spansion/Cypress chips support Read/Write Any Register commands.
These commands are mainly used to write volatile registers and access to
the registers in second and subsequent die for multi-die package parts.

The Read Any Register instruction (65h) is followed by register address
and dummy cycles, then the selected register byte is returned.

The Write Any Register instruction (71h) is followed by register address
and register byte to write.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-core.c | 25 +++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  4 ++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 34c15f1561..2803536ed5 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -211,6 +211,31 @@ static int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	return spi_nor_read_write_reg(nor, &op, buf);
 }
 
+#ifdef CONFIG_SPI_FLASH_SPANSION
+static int spansion_read_any_reg(struct spi_nor *nor, u32 addr, u8 dummy,
+				 u8 *val)
+{
+	struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDAR, 1),
+				   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
+				   SPI_MEM_OP_DUMMY(dummy / 8, 1),
+				   SPI_MEM_OP_DATA_IN(1, NULL, 1));
+
+	return spi_nor_read_write_reg(nor, &op, val);
+}
+
+static int spansion_write_any_reg(struct spi_nor *nor, u32 addr, u8 val)
+{
+	struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
+				   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_OUT(1, NULL, 1));
+
+	return spi_nor_read_write_reg(nor, &op, &val);
+}
+#endif
+
 static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 				 u_char *buf)
 {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 89e7a4fdcd..e31073eb24 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -121,6 +121,10 @@
 #define SPINOR_OP_BRWR		0x17	/* Bank register write */
 #define SPINOR_OP_BRRD		0x16	/* Bank register read */
 #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
+#define SPINOR_OP_RDAR		0x65	/* Read any register */
+#define SPINOR_OP_WRAR		0x71	/* Write any register */
+#define SPINOR_REG_ADDR_STR1V	0x00800000
+#define SPINOR_REG_ADDR_CFR1V	0x00800002
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
-- 
2.25.1

[PATCH v4 4/9] mtd: spi-nor-core: Add support for volatile QE bit

[tkuw584924 at gmail.com]

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

Some of Spansion/Cypress 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. This patch adds a function to set Quad Enable bit in CFR1
volatile.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 2803536ed5..624e730524 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
 	return 0;
 }
 
+/**
+ * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
+ * @nor:	pointer to a 'struct spi_nor'
+ * @addr_base:	base address of register (can be >0 in multi-die parts)
+ * @dummy:	number of dummy cycles for register read
+ *
+ * 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.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base,
+					 u8 dummy)
+{
+	u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;
+
+	u8 cr;
+	int ret;
+
+	/* Check current Quad Enable bit value. */
+	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
+	if (ret < 0) {
+		dev_dbg(nor->dev,
+			"error while reading configuration register\n");
+		return -EINVAL;
+	}
+
+	if (cr & CR_QUAD_EN_SPAN)
+		return 0;
+
+	cr |= CR_QUAD_EN_SPAN;
+
+	write_enable(nor);
+
+	ret = spansion_write_any_reg(nor, addr, cr);
+
+	if (ret < 0) {
+		dev_dbg(nor->dev,
+			"error while writing configuration register\n");
+		return -EINVAL;
+	}
+
+	/* Read back and check it. */
+	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
+	if (ret || !(cr & CR_QUAD_EN_SPAN)) {
+		dev_dbg(nor->dev, "Spansion Quad bit not set\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
 /**
  * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
-- 
2.25.1

[PATCH v4 5/9] mtd: spi-nor-core: Add the ->ready() hook

[tkuw584924 at gmail.com]

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

For dual/quad die package devices from Spansion/Cypress, the device's
status needs to be checked by reading status registers in all dies, by
using Read Any Register command. To support this, a Flash specific hook
that can overwrite the legacy status check is needed.

The spansion_sr_ready() reads status register 1 by Read Any Register
commnad. This function is called from Flash specific hook with die address
and dummy cycles.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-core.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  4 +++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 624e730524..1c0ba5abf9 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 	}
 }
 
+#ifdef CONFIG_SPI_FLASH_SPANSION
+/*
+ * Read status register 1 by using Read Any Register command to support multi
+ * die package parts.
+ */
+static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy)
+{
+	u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V;
+	u8 sr;
+	int ret;
+
+	ret = spansion_read_any_reg(nor, reg_addr, dummy, &sr);
+	if (ret < 0)
+		return ret;
+
+	if (sr & (SR_E_ERR | SR_P_ERR)) {
+		if (sr & SR_E_ERR)
+			dev_dbg(nor->dev, "Erase Error occurred\n");
+		else
+			dev_dbg(nor->dev, "Programming Error occurred\n");
+
+		nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0);
+		return -EIO;
+	}
+
+	return !(sr & SR_WIP);
+}
+#endif
+
 static int spi_nor_sr_ready(struct spi_nor *nor)
 {
 	int sr = read_sr(nor);
@@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor)
 {
 	int sr, fsr;
 
+	if (nor->ready)
+		return nor->ready(nor);
+
 	sr = spi_nor_sr_ready(nor);
 	if (sr < 0)
 		return sr;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index e31073eb24..25234177de 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -434,8 +434,9 @@ struct flash_info;
  * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
  * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
  * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
- * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
  *			completely locked
+ * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
+ * @ready:		[FLASH-SPECIFIC] check if the flash is ready
  * @priv:		the private data
  */
 struct spi_nor {
@@ -481,6 +482,7 @@ struct spi_nor {
 	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 	int (*quad_enable)(struct spi_nor *nor);
+	int (*ready)(struct spi_nor *nor);
 
 	void *priv;
 /* Compatibility for spi_flash, remove once sf layer is merged with mtd */
-- 
2.25.1

[PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature

[tkuw584924 at gmail.com]

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

Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
bottom, depending on the device configuration, while U-Boot supports
uniform sector layout only. This patch adds an erase hook that emulates
uniform sector layout.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-core.c | 48 ++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 1c0ba5abf9..70da0081b6 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -788,6 +788,54 @@ erase_err:
 	return ret;
 }
 
+#ifdef CONFIG_SPI_FLASH_SPANSION
+/*
+ * Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at
+ * the top and/or bottom.
+ */
+static int spansion_overlaid_erase(struct mtd_info *mtd,
+				   struct erase_info *instr)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	struct erase_info instr_4k;
+	u8 opcode;
+	u32 erasesize;
+	int ret;
+
+	/* Perform default erase operation (non-overlaid portion is erased) */
+	ret = spi_nor_erase(mtd, instr);
+	if (ret)
+		return ret;
+
+	/* Backup default erase opcode and size */
+	opcode = nor->erase_opcode;
+	erasesize = mtd->erasesize;
+
+	/*
+	 * Erase 4KB sectors. Use the possible max length of 4KB sector region.
+	 * The Flash just ignores the command if the address is not configured
+	 * as 4KB sector and reports ready status immediately.
+	 */
+	instr_4k.len = SZ_128K;
+	nor->erase_opcode = SPINOR_OP_BE_4K_4B;
+	mtd->erasesize = SZ_4K;
+	if (instr->addr == 0) {
+		instr_4k.addr = 0;
+		ret = spi_nor_erase(mtd, &instr_4k);
+	}
+	if (!ret && instr->addr + instr->len == mtd->size) {
+		instr_4k.addr = mtd->size - instr_4k.len;
+		ret = spi_nor_erase(mtd, &instr_4k);
+	}
+
+	/* Restore erase opcode and size */
+	nor->erase_opcode = opcode;
+	mtd->erasesize = erasesize;
+
+	return ret;
+}
+#endif
+
 #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
 /* Write status register and ensure bits in mask match written values */
 static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
-- 
2.25.1

[PATCH v4 7/9] mtd: spi-nor-core: Add Cypress manufacturer ID in set_4byte

[tkuw584924 at gmail.com]

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

Cypress chips support SPINOR_OP_EN4B(B7h)/SPINOR_OP_EX4B(E9h) to
enable/disable 4-byte addressing mode.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 70da0081b6..ef49328a28 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -492,6 +492,7 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 	case SNOR_MFR_ISSI:
 	case SNOR_MFR_MACRONIX:
 	case SNOR_MFR_WINBOND:
+	case SNOR_MFR_CYPRESS:
 		if (need_wren)
 			write_enable(nor);
 
-- 
2.25.1

[PATCH v4 8/9] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[tkuw584924 at gmail.com]

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

Add nor->setup() and fixup hooks to overwrite:
  - volatile QE bit
  - the ->ready() hook for dual/quad die package parts
  - overlaid erase
  - spi_nor_flash_parameter
  - mtd_info

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index ef49328a28..3d8cb9c333 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor)
 	return 0;
 }
 
+#ifdef CONFIG_SPI_FLASH_SPANSION
+static int s25hx_t_mdp_ready(struct spi_nor *nor)
+{
+	u32 addr;
+	int ret;
+
+	for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
+		ret = spansion_sr_ready(nor, addr, 0);
+		if (ret != 1)
+			return ret;
+	}
+
+	return 1;
+}
+
+static int s25hx_t_quad_enable(struct spi_nor *nor)
+{
+	u32 addr;
+	int ret;
+
+	for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
+		ret = spansion_quad_enable_volatile(nor, addr, 0);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info,
+			 const struct spi_nor_flash_parameter *params,
+			 const struct spi_nor_hwcaps *hwcaps)
+{
+#ifdef CONFIG_SPI_FLASH_BAR
+	return -ENOTSUPP; /* Bank Address Register is not supported */
+#endif
+	/*
+	 * The Cypress Semper family has transparent ECC. To preserve
+	 * ECC enabled, multi-pass programming within the same 16-byte
+	 * ECC data unit needs to be avoided. Set writesize to the page
+	 * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to
+	 * prevent multi-pass programming.
+	 */
+	nor->mtd.writesize = params->page_size;
+	nor->mtd.flags &= ~MTD_BIT_WRITEABLE;
+
+	/* Emulate uniform sector architecure by this erase hook*/
+	nor->mtd._erase = spansion_overlaid_erase;
+
+	/* For 2Gb (dual die) and 4Gb (quad die) parts */
+	if (nor->mtd.size > SZ_128M)
+		nor->ready = s25hx_t_mdp_ready;
+
+	/* Enter 4-byte addressing mode for WRAR used in quad_enable */
+	set_4byte(nor, info, true);
+
+	return spi_nor_default_setup(nor, info, params, hwcaps);
+}
+
+static void s25hx_t_default_init(struct spi_nor *nor)
+{
+	nor->setup = s25hx_t_setup;
+}
+
+static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
+				   const struct sfdp_parameter_header *header,
+				   const struct sfdp_bfpt *bfpt,
+				   struct spi_nor_flash_parameter *params)
+{
+	/* Default page size is 256-byte, but BFPT reports 512-byte */
+	params->page_size = 256;
+	/* Reset erase size in case it is set to 4K from BFPT */
+	nor->mtd.erasesize = 0;
+
+	return 0;
+}
+
+static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor,
+				    struct spi_nor_flash_parameter *params)
+{
+	/* READ_FAST_4B (0Ch) requires mode cycles*/
+	params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+	/* PP_1_1_4 is not supported */
+	params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
+	/* Use volatile register to enable quad */
+	params->quad_enable = s25hx_t_quad_enable;
+}
+
+static struct spi_nor_fixups s25hx_t_fixups = {
+	.default_init = s25hx_t_default_init,
+	.post_bfpt = s25hx_t_post_bfpt_fixup,
+	.post_sfdp = s25hx_t_post_sfdp_fixup,
+};
+#endif
+
 static void spi_nor_set_fixups(struct spi_nor *nor)
 {
+#ifdef CONFIG_SPI_FLASH_SPANSION
+	if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
+		switch (nor->info->id[1]) {
+		case 0x2a: /* S25HL (QSPI, 3.3V) */
+		case 0x2b: /* S25HS (QSPI, 1.8V) */
+			nor->fixups = &s25hx_t_fixups;
+			break;
+
+		default:
+			break;
+		}
+	}
+#endif
 }
 
 int spi_nor_scan(struct spi_nor *nor)
-- 
2.25.1

[PATCH v4 9/9] mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t

[tkuw584924 at gmail.com]

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

Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny.
The volatile QE bit function, spansion_quad_enable_volatile() supports
dual/quad die package parts, by taking 'die_size' parameter that is used
to iterate register update for all dies in the device.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
 drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
index 5cc2b7d996..66680df5a9 100644
--- a/drivers/mtd/spi/spi-nor-tiny.c
+++ b/drivers/mtd/spi/spi-nor-tiny.c
@@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
 }
 #endif /* CONFIG_SPI_FLASH_SPANSION */
 
+#ifdef CONFIG_SPI_FLASH_SPANSION
+/**
+ * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
+ * @nor:	pointer to a 'struct spi_nor'
+ * @die_size:	maximum number of bytes per die ('mtd.size' > 'die_size' in
+ *              multi die package parts).
+ * @dummy:	number of dummy cycles for register read
+ *
+ * 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.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size,
+					 u8 dummy)
+{
+	struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),
+				   SPI_MEM_OP_ADDR(4, 0, 1),
+				   SPI_MEM_OP_DUMMY(0, 1),
+				   SPI_MEM_OP_DATA_IN(1, NULL, 1));
+	u32 addr;
+	u8 cr;
+	int ret;
+
+	/* Use 4-byte address for RDAR/WRAR */
+	ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
+	if (ret < 0) {
+		dev_dbg(nor->dev,
+			"error while enabling 4-byte address\n");
+		return ret;
+	}
+
+	for (addr = 0; addr < nor->mtd.size; addr += die_size) {
+		op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;
+
+		/* Check current Quad Enable bit value. */
+		op.cmd.opcode = SPINOR_OP_RDAR;
+		op.dummy.nbytes = dummy / 8;
+		op.data.dir = SPI_MEM_DATA_IN;
+		ret = spi_nor_read_write_reg(nor, &op, &cr);
+		if (ret < 0) {
+			dev_dbg(nor->dev,
+				"error while reading configuration register\n");
+			return -EINVAL;
+		}
+
+		if (cr & CR_QUAD_EN_SPAN)
+			return 0;
+
+		/* Write new value. */
+		cr |= CR_QUAD_EN_SPAN;
+		op.cmd.opcode = SPINOR_OP_WRAR;
+		op.dummy.nbytes = 0;
+		op.data.dir = SPI_MEM_DATA_OUT;
+		write_enable(nor);
+		ret = spi_nor_read_write_reg(nor, &op, &cr);
+		if (ret < 0) {
+			dev_dbg(nor->dev,
+				"error while writing configuration register\n");
+			return -EINVAL;
+		}
+
+		/* Read back and check it. */
+		op.cmd.opcode = SPINOR_OP_RDAR;
+		op.dummy.nbytes = dummy / 8;
+		op.data.dir = SPI_MEM_DATA_IN;
+		ret = spi_nor_read_write_reg(nor, &op, &cr);
+		if (ret || !(cr & CR_QUAD_EN_SPAN)) {
+			dev_dbg(nor->dev, "Spansion Quad bit not set\n");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+#endif
+
 static void
 spi_nor_set_read_settings(struct spi_nor_read_command *read,
 			  u8 num_mode_clocks,
@@ -583,6 +662,11 @@ static int spi_nor_init_params(struct spi_nor *nor,
 		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
 					  0, 8, SPINOR_OP_READ_FAST,
 					  SNOR_PROTO_1_1_1);
+#ifdef CONFIG_SPI_FLASH_SPANSION
+		if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS &&
+		    (info->id[1] == 0x2a || info->id[1] == 0x2b))
+			params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+#endif
 	}
 
 	if (info->flags & SPI_NOR_QUAD_READ) {
@@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
 		case SNOR_MFR_MACRONIX:
 			err = macronix_quad_enable(nor);
 			break;
+#endif
+#ifdef CONFIG_SPI_FLASH_SPANSION
+		case SNOR_MFR_CYPRESS:
+			err = spansion_quad_enable_volatile(nor, SZ_128M, 0);
+			break;
 #endif
 		case SNOR_MFR_ST:
 		case SNOR_MFR_MICRON:
-- 
2.25.1

[PATCH v4 1/9] mtd: spi-nor: Add Cypress manufacturer ID

[Pratyush Yadav]

On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> This patch adds Cypress manufacturer ID (34h) definition.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 2/9] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
> The datasheets can be found in the following links.
> 
> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
> 
> Tested 512Mb/1Gb/2Gb parts on Xilinx Zynq-7000 FPGA board.

Why not test the 256 Mb and 4 Gb parts as well? They might work 
perfectly well but adding support for untested hardware sounds wrong to 
me.
 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-ids.c | 36 +++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
> index 5bd5dd3003..b78d13e980 100644
> --- a/drivers/mtd/spi/spi-nor-ids.c
> +++ b/drivers/mtd/spi/spi-nor-ids.c
> @@ -217,6 +217,42 @@ const struct flash_info spi_nor_ids[] = {
>  	{ INFO("s25fl208k",  0x014014,      0,  64 * 1024,  16, SECT_4K | SPI_NOR_DUAL_READ) },
>  	{ INFO("s25fl064l",  0x016017,      0,  64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>  	{ INFO("s25fl128l",  0x016018,      0,  64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> +
> +	/* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB

Nitpick: Please leave first line of multi-line comments blank like so:

/*
 * S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
 * ...
 */

> +	 * sectors at top and/or bottom, depending on the device configuration.
> +	 * To support this, an erase hook makes overlaid sectors appear as
> +	 * uniform sectors.
> +	 */
> +	{ INFO6("s25hl256t",  0x342a19, 0x0f0390, 256 * 1024, 128,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hl512t",  0x342a1a, 0x0f0390, 256 * 1024, 256,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hl01gt",  0x342a1b, 0x0f0390, 256 * 1024, 512,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hl02gt",  0x342a1c, 0x0f0090, 256 * 1024, 1024,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hl04gt",  0x342a1d, 0x0f0090, 256 * 1024, 2048,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hs256t",  0x342b19, 0x0f0390, 256 * 1024, 128,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hs512t",  0x342b1a, 0x0f0390, 256 * 1024, 256,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hs01gt",  0x342b1b, 0x0f0390, 256 * 1024, 512,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hs02gt",  0x342b1c, 0x0f0090, 256 * 1024, 1024,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },
> +	{ INFO6("s25hs04gt",  0x342b1d, 0x0f0090, 256 * 1024, 2048,
> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
> +		USE_CLSR) },

The datasheets you have linked do not specify a mapping between device 
ID and part number. So I can't verify that you are using the correct IDs 
here. But I'll trust you to get them right :-)

With the above comment addressed and 256 Mb and 4 Gb parts tested, 
please add

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

>  #endif
>  #ifdef CONFIG_SPI_FLASH_SST		/* SST */
>  	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 3/9] mtd: spi-nor-core: Add support for Read/Write Any Register

[Pratyush Yadav]

On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Some of Spansion/Cypress chips support Read/Write Any Register commands.
> These commands are mainly used to write volatile registers and access to
> the registers in second and subsequent die for multi-die package parts.
> 
> The Read Any Register instruction (65h) is followed by register address
> and dummy cycles, then the selected register byte is returned.
> 
> The Write Any Register instruction (71h) is followed by register address
> and register byte to write.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 25 +++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  4 ++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 34c15f1561..2803536ed5 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -211,6 +211,31 @@ static int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>  	return spi_nor_read_write_reg(nor, &op, buf);
>  }
>  
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +static int spansion_read_any_reg(struct spi_nor *nor, u32 addr, u8 dummy,
> +				 u8 *val)
> +{
> +	struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDAR, 1),
> +				   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
> +				   SPI_MEM_OP_DUMMY(dummy / 8, 1),
> +				   SPI_MEM_OP_DATA_IN(1, NULL, 1));
> +
> +	return spi_nor_read_write_reg(nor, &op, val);
> +}
> +
> +static int spansion_write_any_reg(struct spi_nor *nor, u32 addr, u8 val)
> +{
> +	struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
> +				   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_OUT(1, NULL, 1));
> +
> +	return spi_nor_read_write_reg(nor, &op, &val);
> +}
> +#endif
> +
>  static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>  				 u_char *buf)
>  {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 89e7a4fdcd..e31073eb24 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -121,6 +121,10 @@
>  #define SPINOR_OP_BRWR		0x17	/* Bank register write */
>  #define SPINOR_OP_BRRD		0x16	/* Bank register read */
>  #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
> +#define SPINOR_OP_RDAR		0x65	/* Read any register */
> +#define SPINOR_OP_WRAR		0x71	/* Write any register */
> +#define SPINOR_REG_ADDR_STR1V	0x00800000
> +#define SPINOR_REG_ADDR_CFR1V	0x00800002

These two defines are not used by this patch. Remove them from this one 
and add them to the one that actually uses them for the first time.

With this fixed,

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

>  
>  /* Used for Micron flashes only. */
>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 4/9] mtd: spi-nor-core: Add support for volatile QE bit

[Pratyush Yadav]

Hi,

On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Some of Spansion/Cypress 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. This patch adds a function to set Quad Enable bit in CFR1
> volatile.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 2803536ed5..624e730524 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +/**
> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @addr_base:	base address of register (can be >0 in multi-die parts)
> + * @dummy:	number of dummy cycles for register read
> + *
> + * 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.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base,
> +					 u8 dummy)
> +{
> +	u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;

Why do you OR the register offset with the base? Shouldn't you be adding 
to it?

> +
> +	u8 cr;
> +	int ret;
> +
> +	/* Check current Quad Enable bit value. */
> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
> +	if (ret < 0) {
> +		dev_dbg(nor->dev,
> +			"error while reading configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	if (cr & CR_QUAD_EN_SPAN)
> +		return 0;
> +
> +	cr |= CR_QUAD_EN_SPAN;
> +
> +	write_enable(nor);
> +
> +	ret = spansion_write_any_reg(nor, addr, cr);
> +
> +	if (ret < 0) {
> +		dev_dbg(nor->dev,
> +			"error while writing configuration register\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Read back and check it. */
> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
> +	if (ret || !(cr & CR_QUAD_EN_SPAN)) {
> +		dev_dbg(nor->dev, "Spansion Quad bit not set\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

Rest of the patch LGTM.

>  #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
>  /**
>   * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 5/9] mtd: spi-nor-core: Add the ->ready() hook

[Pratyush Yadav]

Hi,

On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> For dual/quad die package devices from Spansion/Cypress, the device's
> status needs to be checked by reading status registers in all dies, by
> using Read Any Register command. To support this, a Flash specific hook
> that can overwrite the legacy status check is needed.
> 
> The spansion_sr_ready() reads status register 1 by Read Any Register
> commnad. This function is called from Flash specific hook with die address
> and dummy cycles.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  4 +++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 624e730524..1c0ba5abf9 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>  	}
>  }
>  
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +/*
> + * Read status register 1 by using Read Any Register command to support multi
> + * die package parts.
> + */
> +static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy)
> +{
> +	u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V;
> +	u8 sr;
> +	int ret;
> +
> +	ret = spansion_read_any_reg(nor, reg_addr, dummy, &sr);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (sr & (SR_E_ERR | SR_P_ERR)) {
> +		if (sr & SR_E_ERR)
> +			dev_dbg(nor->dev, "Erase Error occurred\n");
> +		else
> +			dev_dbg(nor->dev, "Programming Error occurred\n");
> +
> +		nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0);
> +		return -EIO;
> +	}
> +
> +	return !(sr & SR_WIP);
> +}
> +#endif
> +

Do not add this function in this patch. Just add the hook here. Add it 
in the patch that actually uses it.

>  static int spi_nor_sr_ready(struct spi_nor *nor)
>  {
>  	int sr = read_sr(nor);
> @@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor)
>  {
>  	int sr, fsr;
>  
> +	if (nor->ready)
> +		return nor->ready(nor);
> +

Move the code below in a separate function and call it something like 
spi_nor_default_ready(). Then call that function from here.

>  	sr = spi_nor_sr_ready(nor);
>  	if (sr < 0)
>  		return sr;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index e31073eb24..25234177de 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -434,8 +434,9 @@ struct flash_info;
>   * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
>   * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
>   * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
> - * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
>   *			completely locked
> + * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
> + * @ready:		[FLASH-SPECIFIC] check if the flash is ready
>   * @priv:		the private data
>   */
>  struct spi_nor {
> @@ -481,6 +482,7 @@ struct spi_nor {
>  	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>  	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>  	int (*quad_enable)(struct spi_nor *nor);
> +	int (*ready)(struct spi_nor *nor);
>  
>  	void *priv;
>  /* Compatibility for spi_flash, remove once sf layer is merged with mtd */
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature

[Pratyush Yadav]

Hi Takahiro,

On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
> bottom, depending on the device configuration, while U-Boot supports
> uniform sector layout only. This patch adds an erase hook that emulates
> uniform sector layout.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 48 ++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 1c0ba5abf9..70da0081b6 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -788,6 +788,54 @@ erase_err:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +/*
> + * Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at
> + * the top and/or bottom.
> + */
> +static int spansion_overlaid_erase(struct mtd_info *mtd,
> +				   struct erase_info *instr)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	struct erase_info instr_4k;
> +	u8 opcode;
> +	u32 erasesize;
> +	int ret;
> +
> +	/* Perform default erase operation (non-overlaid portion is erased) */
> +	ret = spi_nor_erase(mtd, instr);
> +	if (ret)
> +		return ret;
> +
> +	/* Backup default erase opcode and size */
> +	opcode = nor->erase_opcode;
> +	erasesize = mtd->erasesize;
> +
> +	/*
> +	 * Erase 4KB sectors. Use the possible max length of 4KB sector region.
> +	 * The Flash just ignores the command if the address is not configured
> +	 * as 4KB sector and reports ready status immediately.
> +	 */
> +	instr_4k.len = SZ_128K;
> +	nor->erase_opcode = SPINOR_OP_BE_4K_4B;
> +	mtd->erasesize = SZ_4K;
> +	if (instr->addr == 0) {
> +		instr_4k.addr = 0;
> +		ret = spi_nor_erase(mtd, &instr_4k);
> +	}
> +	if (!ret && instr->addr + instr->len == mtd->size) {
> +		instr_4k.addr = mtd->size - instr_4k.len;
> +		ret = spi_nor_erase(mtd, &instr_4k);
> +	}

This feels like a hack to me. Does the flash datasheet explicitly say 
that erasing the overlaid area with the "normal" erase opcode is a 
no-op?

I don't see a big reason to run this hack. You are already in a 
flash-specific erase hook. Why not just directly issue the correct erase 
commands to the sectors? That is, why not issue 4k erase to overlaid 
sectors and normal erase to the rest? Why do you need to emulate uniform 
erase?

> +
> +	/* Restore erase opcode and size */
> +	nor->erase_opcode = opcode;
> +	mtd->erasesize = erasesize;
> +
> +	return ret;
> +}
> +#endif
> +
>  #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>  /* Write status register and ensure bits in mask match written values */
>  static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 8/9] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 28/01/21 01:37PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Add nor->setup() and fixup hooks to overwrite:
>   - volatile QE bit
>   - the ->ready() hook for dual/quad die package parts
>   - overlaid erase
>   - spi_nor_flash_parameter
>   - mtd_info
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++
>  1 file changed, 108 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index ef49328a28..3d8cb9c333 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +static int s25hx_t_mdp_ready(struct spi_nor *nor)
> +{
> +	u32 addr;
> +	int ret;
> +
> +	for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
> +		ret = spansion_sr_ready(nor, addr, 0);
> +		if (ret != 1)
> +			return ret;
> +	}
> +
> +	return 1;
> +}
> +
> +static int s25hx_t_quad_enable(struct spi_nor *nor)
> +{
> +	u32 addr;
> +	int ret;
> +
> +	for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
> +		ret = spansion_quad_enable_volatile(nor, addr, 0);

So you need to set the QE bit on each die. Ok.

Out of curiosity, what will happen if you only set the QE bit on the 
first die? Will reads from first die work in quad mode and rest in 
single mode?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info,
> +			 const struct spi_nor_flash_parameter *params,
> +			 const struct spi_nor_hwcaps *hwcaps)
> +{
> +#ifdef CONFIG_SPI_FLASH_BAR
> +	return -ENOTSUPP; /* Bank Address Register is not supported */
> +#endif
> +	/*
> +	 * The Cypress Semper family has transparent ECC. To preserve
> +	 * ECC enabled, multi-pass programming within the same 16-byte
> +	 * ECC data unit needs to be avoided. Set writesize to the page
> +	 * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to
> +	 * prevent multi-pass programming.
> +	 */
> +	nor->mtd.writesize = params->page_size;

The writesize should be set to 16. See [0][1][2].

> +	nor->mtd.flags &= ~MTD_BIT_WRITEABLE;

Not needed. See discussions pointed to above.

> +
> +	/* Emulate uniform sector architecure by this erase hook*/
> +	nor->mtd._erase = spansion_overlaid_erase;
> +
> +	/* For 2Gb (dual die) and 4Gb (quad die) parts */
> +	if (nor->mtd.size > SZ_128M)
> +		nor->ready = s25hx_t_mdp_ready;
> +
> +	/* Enter 4-byte addressing mode for WRAR used in quad_enable */
> +	set_4byte(nor, info, true);
> +
> +	return spi_nor_default_setup(nor, info, params, hwcaps);
> +}
> +
> +static void s25hx_t_default_init(struct spi_nor *nor)
> +{
> +	nor->setup = s25hx_t_setup;
> +}
> +
> +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
> +				   const struct sfdp_parameter_header *header,
> +				   const struct sfdp_bfpt *bfpt,
> +				   struct spi_nor_flash_parameter *params)
> +{
> +	/* Default page size is 256-byte, but BFPT reports 512-byte */
> +	params->page_size = 256;

Read the page size from the register, like it is done on Linux for S28 
flash family.

> +	/* Reset erase size in case it is set to 4K from BFPT */
> +	nor->mtd.erasesize = 0;

What does erasesize of 0 mean? I would take that to mean that the flash 
does not support erases. I can't find any mention of 0 erase size in the 
documentation of struct mtd_info.

> +
> +	return 0;
> +}
> +
> +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor,
> +				    struct spi_nor_flash_parameter *params)
> +{
> +	/* READ_FAST_4B (0Ch) requires mode cycles*/
> +	params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> +	/* PP_1_1_4 is not supported */
> +	params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
> +	/* Use volatile register to enable quad */
> +	params->quad_enable = s25hx_t_quad_enable;
> +}
> +
> +static struct spi_nor_fixups s25hx_t_fixups = {
> +	.default_init = s25hx_t_default_init,
> +	.post_bfpt = s25hx_t_post_bfpt_fixup,
> +	.post_sfdp = s25hx_t_post_sfdp_fixup,

Hmm, I don't think the fixups feature was ever applied to the u-boot 
tree. I sent a patch for them a while ago [3] but they were never 
applied due to some issues. I can't find any mentions of 
"spi_nor_set_fixups" on my 4 day old checkout of Tom's master branch. 

And that reminds me, the nor->setup() hook you are using is not there 
either. Your patch series should not even build on upstream u-boot. 
Please cherry pick the required patches from my series and send them 
along with yours.

Please make sure your patch series builds and works on top of _upstream_ 
u-boot. Even if it works on your company's fork of U-Boot does not 
necessarily mean it will work on upstream.

> +};
> +#endif
> +
>  static void spi_nor_set_fixups(struct spi_nor *nor)

This function is also not present in u-boot master.

>  {
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +	if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
> +		switch (nor->info->id[1]) {
> +		case 0x2a: /* S25HL (QSPI, 3.3V) */
> +		case 0x2b: /* S25HS (QSPI, 1.8V) */
> +			nor->fixups = &s25hx_t_fixups;
> +			break;

I recall using strcmp() in my series but I guess this should also work 
just as well.

> +
> +		default:
> +			break;
> +		}
> +	}
> +#endif
>  }
>  
>  int spi_nor_scan(struct spi_nor *nor)
> -- 
> 2.25.1
> 

[0] https://lore.kernel.org/linux-mtd/4c0e3207-72a4-8c1a-5fca-e9f30cc60828 at ti.com/
[1] https://lore.kernel.org/linux-mtd/20201201102711.8727-3-p.yadav at ti.com/
[2] https://lore.kernel.org/linux-mtd/20201201102711.8727-4-p.yadav at ti.com/
[3] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 9/9] mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 28/01/21 01:37PM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny.
> The volatile QE bit function, spansion_quad_enable_volatile() supports
> dual/quad die package parts, by taking 'die_size' parameter that is used
> to iterate register update for all dies in the device.

I'm not so sure if this is a good idea. spi-nor-tiny should be the 
minimal set of functionality to get the bootloader to the next stage. 
1S-1S-1S mode is sufficient for that. Adding quad enable functions of 
all the flashes will increase the size quite a bit. I know that some 
flashes already have their quad enable hooks, and I don't think they 
should be there either.

Of course, the maintainers have the final call, but from my side,
 
Nacked-by: Pratyush Yadav <p.yadav@ti.com>

Anyway, comments below in case the maintainers do plan on picking this 
patch up.

> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
> index 5cc2b7d996..66680df5a9 100644
> --- a/drivers/mtd/spi/spi-nor-tiny.c
> +++ b/drivers/mtd/spi/spi-nor-tiny.c
> @@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
>  }
>  #endif /* CONFIG_SPI_FLASH_SPANSION */
>  
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +/**
> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @die_size:	maximum number of bytes per die ('mtd.size' > 'die_size' in
> + *              multi die package parts).
> + * @dummy:	number of dummy cycles for register read
> + *
> + * 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.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size,
> +					 u8 dummy)
> +{
> +	struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),
> +				   SPI_MEM_OP_ADDR(4, 0, 1),
> +				   SPI_MEM_OP_DUMMY(0, 1),
> +				   SPI_MEM_OP_DATA_IN(1, NULL, 1));
> +	u32 addr;
> +	u8 cr;
> +	int ret;
> +
> +	/* Use 4-byte address for RDAR/WRAR */
> +	ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
> +	if (ret < 0) {
> +		dev_dbg(nor->dev,
> +			"error while enabling 4-byte address\n");
> +		return ret;
> +	}
> +
> +	for (addr = 0; addr < nor->mtd.size; addr += die_size) {
> +		op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;

So here you add the register offset to the base address, instead of 
ORing it. Ok.

> +
> +		/* Check current Quad Enable bit value. */
> +		op.cmd.opcode = SPINOR_OP_RDAR;
> +		op.dummy.nbytes = dummy / 8;
> +		op.data.dir = SPI_MEM_DATA_IN;
> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
> +		if (ret < 0) {
> +			dev_dbg(nor->dev,
> +				"error while reading configuration register\n");
> +			return -EINVAL;
> +		}
> +
> +		if (cr & CR_QUAD_EN_SPAN)
> +			return 0;
> +
> +		/* Write new value. */
> +		cr |= CR_QUAD_EN_SPAN;
> +		op.cmd.opcode = SPINOR_OP_WRAR;
> +		op.dummy.nbytes = 0;
> +		op.data.dir = SPI_MEM_DATA_OUT;
> +		write_enable(nor);
> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
> +		if (ret < 0) {
> +			dev_dbg(nor->dev,
> +				"error while writing configuration register\n");
> +			return -EINVAL;
> +		}
> +
> +		/* Read back and check it. */
> +		op.data.dir = SPI_MEM_DATA_IN;
> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
> +		if (ret || !(cr & CR_QUAD_EN_SPAN)) {
> +			dev_dbg(nor->dev, "Spansion Quad bit not set\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +#endif
> +

LGTM.

>  static void
>  spi_nor_set_read_settings(struct spi_nor_read_command *read,
>  			  u8 num_mode_clocks,
> @@ -583,6 +662,11 @@ static int spi_nor_init_params(struct spi_nor *nor,
>  		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
>  					  0, 8, SPINOR_OP_READ_FAST,
>  					  SNOR_PROTO_1_1_1);
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +		if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS &&
> +		    (info->id[1] == 0x2a || info->id[1] == 0x2b))

Add a comment about which flash models these two are.

> +			params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> +#endif
>  	}
>  
>  	if (info->flags & SPI_NOR_QUAD_READ) {
> @@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
>  		case SNOR_MFR_MACRONIX:
>  			err = macronix_quad_enable(nor);
>  			break;
> +#endif
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +		case SNOR_MFR_CYPRESS:
> +			err = spansion_quad_enable_volatile(nor, SZ_128M, 0);
> +			break;
>  #endif
>  		case SNOR_MFR_ST:
>  		case SNOR_MFR_MICRON:
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v4 2/9] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t

[Takahiro Kuwano]

Hi Pratyush,

On 1/30/2021 3:08 AM, Pratyush Yadav wrote:
> On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
>> The datasheets can be found in the following links.
>>
>> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
>> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
>>
>> Tested 512Mb/1Gb/2Gb parts on Xilinx Zynq-7000 FPGA board.
> 
> Why not test the 256 Mb and 4 Gb parts as well? They might work 
> perfectly well but adding support for untested hardware sounds wrong to 
> me.

The 256Mb and 4Gb parts are not yet officially sampled, although they are
described in the datasheet. They should work like other tested ones, but
as you commented, only tested devices should be added. I will remove 256Mb
and 4Gb in v5.

>  
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-ids.c | 36 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index 5bd5dd3003..b78d13e980 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -217,6 +217,42 @@ const struct flash_info spi_nor_ids[] = {
>>  	{ INFO("s25fl208k",  0x014014,      0,  64 * 1024,  16, SECT_4K | SPI_NOR_DUAL_READ) },
>>  	{ INFO("s25fl064l",  0x016017,      0,  64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>  	{ INFO("s25fl128l",  0x016018,      0,  64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> +
>> +	/* S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
> 
> Nitpick: Please leave first line of multi-line comments blank like so:
> 
> /*
>  * S25HL/HS-T (Semper Flash with Quad SPI) Family has overlaid 4KB
>  * ...
>  */
> 

I will fix it.

>> +	 * sectors at top and/or bottom, depending on the device configuration.
>> +	 * To support this, an erase hook makes overlaid sectors appear as
>> +	 * uniform sectors.
>> +	 */
>> +	{ INFO6("s25hl256t",  0x342a19, 0x0f0390, 256 * 1024, 128,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hl512t",  0x342a1a, 0x0f0390, 256 * 1024, 256,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hl01gt",  0x342a1b, 0x0f0390, 256 * 1024, 512,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hl02gt",  0x342a1c, 0x0f0090, 256 * 1024, 1024,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hl04gt",  0x342a1d, 0x0f0090, 256 * 1024, 2048,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs256t",  0x342b19, 0x0f0390, 256 * 1024, 128,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs512t",  0x342b1a, 0x0f0390, 256 * 1024, 256,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs01gt",  0x342b1b, 0x0f0390, 256 * 1024, 512,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs02gt",  0x342b1c, 0x0f0090, 256 * 1024, 1024,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
>> +	{ INFO6("s25hs04gt",  0x342b1d, 0x0f0090, 256 * 1024, 2048,
>> +		SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES |
>> +		USE_CLSR) },
> 
> The datasheets you have linked do not specify a mapping between device 
> ID and part number. So I can't verify that you are using the correct IDs 
> here. But I'll trust you to get them right :-)
> 

Thank you :-)

> With the above comment addressed and 256 Mb and 4 Gb parts tested, 
> please add
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 
>>  #endif
>>  #ifdef CONFIG_SPI_FLASH_SST		/* SST */
>>  	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro

[PATCH v4 3/9] mtd: spi-nor-core: Add support for Read/Write Any Register

[Takahiro Kuwano]

On 1/30/2021 3:17 AM, Pratyush Yadav wrote:
> On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of Spansion/Cypress chips support Read/Write Any Register commands.
>> These commands are mainly used to write volatile registers and access to
>> the registers in second and subsequent die for multi-die package parts.
>>
>> The Read Any Register instruction (65h) is followed by register address
>> and dummy cycles, then the selected register byte is returned.
>>
>> The Write Any Register instruction (71h) is followed by register address
>> and register byte to write.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 25 +++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h    |  4 ++++
>>  2 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 34c15f1561..2803536ed5 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -211,6 +211,31 @@ static int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>>  	return spi_nor_read_write_reg(nor, &op, buf);
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +static int spansion_read_any_reg(struct spi_nor *nor, u32 addr, u8 dummy,
>> +				 u8 *val)
>> +{
>> +	struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDAR, 1),
>> +				   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
>> +				   SPI_MEM_OP_DUMMY(dummy / 8, 1),
>> +				   SPI_MEM_OP_DATA_IN(1, NULL, 1));
>> +
>> +	return spi_nor_read_write_reg(nor, &op, val);
>> +}
>> +
>> +static int spansion_write_any_reg(struct spi_nor *nor, u32 addr, u8 val)
>> +{
>> +	struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRAR, 1),
>> +				   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
>> +				   SPI_MEM_OP_NO_DUMMY,
>> +				   SPI_MEM_OP_DATA_OUT(1, NULL, 1));
>> +
>> +	return spi_nor_read_write_reg(nor, &op, &val);
>> +}
>> +#endif
>> +
>>  static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>>  				 u_char *buf)
>>  {
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 89e7a4fdcd..e31073eb24 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -121,6 +121,10 @@
>>  #define SPINOR_OP_BRWR		0x17	/* Bank register write */
>>  #define SPINOR_OP_BRRD		0x16	/* Bank register read */
>>  #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
>> +#define SPINOR_OP_RDAR		0x65	/* Read any register */
>> +#define SPINOR_OP_WRAR		0x71	/* Write any register */
>> +#define SPINOR_REG_ADDR_STR1V	0x00800000
>> +#define SPINOR_REG_ADDR_CFR1V	0x00800002
> 
> These two defines are not used by this patch. Remove them from this one 
> and add them to the one that actually uses them for the first time.
> 
I will fix it, thank you.

> With this fixed,
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 
>>  
>>  /* Used for Micron flashes only. */
>>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro

[PATCH v4 4/9] mtd: spi-nor-core: Add support for volatile QE bit

[Takahiro Kuwano]

Hi Pratyush,

On 1/30/2021 3:40 AM, Pratyush Yadav wrote:
> Hi,
> 
> On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of Spansion/Cypress 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. This patch adds a function to set Quad Enable bit in CFR1
>> volatile.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 2803536ed5..624e730524 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -1576,6 +1576,59 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
>>  	return 0;
>>  }
>>  
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @addr_base:	base address of register (can be >0 in multi-die parts)
>> + * @dummy:	number of dummy cycles for register read
>> + *
>> + * 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.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 addr_base,
>> +					 u8 dummy)
>> +{
>> +	u32 addr = addr_base | SPINOR_REG_ADDR_CFR1V;
> 
> Why do you OR the register offset with the base? Shouldn't you be adding 
> to it?
> 
I missed it during review... I will fix.


>> +
>> +	u8 cr;
>> +	int ret;
>> +
>> +	/* Check current Quad Enable bit value. */
>> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while reading configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cr & CR_QUAD_EN_SPAN)
>> +		return 0;
>> +
>> +	cr |= CR_QUAD_EN_SPAN;
>> +
>> +	write_enable(nor);
>> +
>> +	ret = spansion_write_any_reg(nor, addr, cr);
>> +
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while writing configuration register\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Read back and check it. */
>> +	ret = spansion_read_any_reg(nor, addr, dummy, &cr);
>> +	if (ret || !(cr & CR_QUAD_EN_SPAN)) {
>> +		dev_dbg(nor->dev, "Spansion Quad bit not set\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 
> Rest of the patch LGTM.
> 
>>  #if CONFIG_IS_ENABLED(SPI_FLASH_SFDP_SUPPORT)
>>  /**
>>   * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro

[PATCH v4 5/9] mtd: spi-nor-core: Add the ->ready() hook

[Takahiro Kuwano]

Hi Pratyush,

Thank you for the feedback. I will address this in v5.

On 1/30/2021 3:49 AM, Pratyush Yadav wrote:
> Hi,
> 
> On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> For dual/quad die package devices from Spansion/Cypress, the device's
>> status needs to be checked by reading status registers in all dies, by
>> using Read Any Register command. To support this, a Flash specific hook
>> that can overwrite the legacy status check is needed.
>>
>> The spansion_sr_ready() reads status register 1 by Read Any Register
>> commnad. This function is called from Flash specific hook with die address
>> and dummy cycles.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 32 ++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h    |  4 +++-
>>  2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 624e730524..1c0ba5abf9 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>  	}
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/*
>> + * Read status register 1 by using Read Any Register command to support multi
>> + * die package parts.
>> + */
>> +static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy)
>> +{
>> +	u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V;
>> +	u8 sr;
>> +	int ret;
>> +
>> +	ret = spansion_read_any_reg(nor, reg_addr, dummy, &sr);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (sr & (SR_E_ERR | SR_P_ERR)) {
>> +		if (sr & SR_E_ERR)
>> +			dev_dbg(nor->dev, "Erase Error occurred\n");
>> +		else
>> +			dev_dbg(nor->dev, "Programming Error occurred\n");
>> +
>> +		nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0);
>> +		return -EIO;
>> +	}
>> +
>> +	return !(sr & SR_WIP);
>> +}
>> +#endif
>> +
> 
> Do not add this function in this patch. Just add the hook here. Add it 
> in the patch that actually uses it.
> 
>>  static int spi_nor_sr_ready(struct spi_nor *nor)
>>  {
>>  	int sr = read_sr(nor);
>> @@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor)
>>  {
>>  	int sr, fsr;
>>  
>> +	if (nor->ready)
>> +		return nor->ready(nor);
>> +
> 
> Move the code below in a separate function and call it something like 
> spi_nor_default_ready(). Then call that function from here.
> 
>>  	sr = spi_nor_sr_ready(nor);
>>  	if (sr < 0)
>>  		return sr;
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index e31073eb24..25234177de 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -434,8 +434,9 @@ struct flash_info;
>>   * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
>>   * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
>>   * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
>> - * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
>>   *			completely locked
>> + * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
>> + * @ready:		[FLASH-SPECIFIC] check if the flash is ready
>>   * @priv:		the private data
>>   */
>>  struct spi_nor {
>> @@ -481,6 +482,7 @@ struct spi_nor {
>>  	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>>  	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>>  	int (*quad_enable)(struct spi_nor *nor);
>> +	int (*ready)(struct spi_nor *nor);
>>  
>>  	void *priv;
>>  /* Compatibility for spi_flash, remove once sf layer is merged with mtd */
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro

[PATCH v4 6/9] mtd: spi-nor-core: Add overlaid sector erase feature

[Takahiro Kuwano]

Hi Pratyush,

On 2/2/2021 3:56 AM, Pratyush Yadav wrote:
> Hi Takahiro,
> 
> On 28/01/21 01:36PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Some of Spansion/Cypress chips have overlaid 4KB sectors at top and/or
>> bottom, depending on the device configuration, while U-Boot supports
>> uniform sector layout only. This patch adds an erase hook that emulates
>> uniform sector layout.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 48 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 1c0ba5abf9..70da0081b6 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -788,6 +788,54 @@ erase_err:
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/*
>> + * Erase for Spansion/Cypress Flash devices that has overlaid 4KB sectors at
>> + * the top and/or bottom.
>> + */
>> +static int spansion_overlaid_erase(struct mtd_info *mtd,
>> +				   struct erase_info *instr)
>> +{
>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +	struct erase_info instr_4k;
>> +	u8 opcode;
>> +	u32 erasesize;
>> +	int ret;
>> +
>> +	/* Perform default erase operation (non-overlaid portion is erased) */
>> +	ret = spi_nor_erase(mtd, instr);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Backup default erase opcode and size */
>> +	opcode = nor->erase_opcode;
>> +	erasesize = mtd->erasesize;
>> +
>> +	/*
>> +	 * Erase 4KB sectors. Use the possible max length of 4KB sector region.
>> +	 * The Flash just ignores the command if the address is not configured
>> +	 * as 4KB sector and reports ready status immediately.
>> +	 */
>> +	instr_4k.len = SZ_128K;
>> +	nor->erase_opcode = SPINOR_OP_BE_4K_4B;
>> +	mtd->erasesize = SZ_4K;
>> +	if (instr->addr == 0) {
>> +		instr_4k.addr = 0;
>> +		ret = spi_nor_erase(mtd, &instr_4k);
>> +	}
>> +	if (!ret && instr->addr + instr->len == mtd->size) {
>> +		instr_4k.addr = mtd->size - instr_4k.len;
>> +		ret = spi_nor_erase(mtd, &instr_4k);
>> +	}
> 
> This feels like a hack to me. Does the flash datasheet explicitly say 
> that erasing the overlaid area with the "normal" erase opcode is a 
> no-op?
> 
Sorry, I have noticed the datasheet I mentioned in the cover letter is a
summary version. Here is the link to he full version, but you need
registration to access.
https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522

So, let me quote the description about erasing overlaid area:

"If a sector erase transaction is applied to a 256 KB sector that is
overlaid by 4 KB sectors, the overlaid 4 KB sectors are not affected
by the erase. Only the visible (non-overlaid) portion of the 128 KB
or 192 KB sector is erased."


And about 4 KB sector erase:

"This transaction is ignored when the device is configured for uniform
sector only (CFR3V[3] = 1). If the Erase 4K B sector transaction is issued
to a non-4 KB sector address, the device will abort the operation and will
not set the ERSERR status fail bit."


> I don't see a big reason to run this hack. You are already in a 
> flash-specific erase hook. Why not just directly issue the correct erase 
> commands to the sectors? That is, why not issue 4k erase to overlaid 
> sectors and normal erase to the rest? Why do you need to emulate uniform 
> erase?
> 
Thanks for pointing this out. I should probably hook nor->erase() instead
of mtd->_erase(), then issue 4k or normal erase depending on the address.
I will introduce that in v5. 

>> +
>> +	/* Restore erase opcode and size */
>> +	nor->erase_opcode = opcode;
>> +	mtd->erasesize = erasesize;
>> +
>> +	return ret;
>> +}
>> +#endif
>> +
>>  #if defined(CONFIG_SPI_FLASH_STMICRO) || defined(CONFIG_SPI_FLASH_SST)
>>  /* Write status register and ensure bits in mask match written values */
>>  static int write_sr_and_check(struct spi_nor *nor, u8 status_new, u8 mask)
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro

[PATCH v4 9/9] mtd: spi-nor-tiny: Add fixups for Cypress s25hl-t/s25hs-t

[Takahiro Kuwano]

Hi Pratyush,

On 2/2/2021 4:40 AM, Pratyush Yadav wrote:
> On 28/01/21 01:37PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Fixes mode clocks for SPINOR_OP_READ_FAST_4B and volatile QE bit in tiny.
>> The volatile QE bit function, spansion_quad_enable_volatile() supports
>> dual/quad die package parts, by taking 'die_size' parameter that is used
>> to iterate register update for all dies in the device.
> 
> I'm not so sure if this is a good idea. spi-nor-tiny should be the 
> minimal set of functionality to get the bootloader to the next stage. 
> 1S-1S-1S mode is sufficient for that. Adding quad enable functions of 
> all the flashes will increase the size quite a bit. I know that some 
> flashes already have their quad enable hooks, and I don't think they 
> should be there either.
> 
> Of course, the maintainers have the final call, but from my side,
>  
> Nacked-by: Pratyush Yadav <p.yadav@ti.com>
> 
I understand your point. Let's leave the decision up to the maintainers.

> Anyway, comments below in case the maintainers do plan on picking this 
> patch up.
> 
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-tiny.c | 89 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 89 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
>> index 5cc2b7d996..66680df5a9 100644
>> --- a/drivers/mtd/spi/spi-nor-tiny.c
>> +++ b/drivers/mtd/spi/spi-nor-tiny.c
>> @@ -555,6 +555,85 @@ static int spansion_read_cr_quad_enable(struct spi_nor *nor)
>>  }
>>  #endif /* CONFIG_SPI_FLASH_SPANSION */
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @die_size:	maximum number of bytes per die ('mtd.size' > 'die_size' in
>> + *              multi die package parts).
>> + * @dummy:	number of dummy cycles for register read
>> + *
>> + * 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.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u32 die_size,
>> +					 u8 dummy)
>> +{
>> +	struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(0, 1),
>> +				   SPI_MEM_OP_ADDR(4, 0, 1),
>> +				   SPI_MEM_OP_DUMMY(0, 1),
>> +				   SPI_MEM_OP_DATA_IN(1, NULL, 1));
>> +	u32 addr;
>> +	u8 cr;
>> +	int ret;
>> +
>> +	/* Use 4-byte address for RDAR/WRAR */
>> +	ret = spi_nor_write_reg(nor, SPINOR_OP_EN4B, NULL, 0);
>> +	if (ret < 0) {
>> +		dev_dbg(nor->dev,
>> +			"error while enabling 4-byte address\n");
>> +		return ret;
>> +	}
>> +
>> +	for (addr = 0; addr < nor->mtd.size; addr += die_size) {
>> +		op.addr.val = addr + SPINOR_REG_ADDR_CFR1V;
> 
> So here you add the register offset to the base address, instead of 
> ORing it. Ok.
> 
>> +
>> +		/* Check current Quad Enable bit value. */
>> +		op.cmd.opcode = SPINOR_OP_RDAR;
>> +		op.dummy.nbytes = dummy / 8;
>> +		op.data.dir = SPI_MEM_DATA_IN;
>> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +		if (ret < 0) {
>> +			dev_dbg(nor->dev,
>> +				"error while reading configuration register\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (cr & CR_QUAD_EN_SPAN)
>> +			return 0;
>> +
>> +		/* Write new value. */
>> +		cr |= CR_QUAD_EN_SPAN;
>> +		op.cmd.opcode = SPINOR_OP_WRAR;
>> +		op.dummy.nbytes = 0;
>> +		op.data.dir = SPI_MEM_DATA_OUT;
>> +		write_enable(nor);
>> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +		if (ret < 0) {
>> +			dev_dbg(nor->dev,
>> +				"error while writing configuration register\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* Read back and check it. */
>> +		op.data.dir = SPI_MEM_DATA_IN;
>> +		ret = spi_nor_read_write_reg(nor, &op, &cr);
>> +		if (ret || !(cr & CR_QUAD_EN_SPAN)) {
>> +			dev_dbg(nor->dev, "Spansion Quad bit not set\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
> 
> LGTM.
> 
>>  static void
>>  spi_nor_set_read_settings(struct spi_nor_read_command *read,
>>  			  u8 num_mode_clocks,
>> @@ -583,6 +662,11 @@ static int spi_nor_init_params(struct spi_nor *nor,
>>  		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_FAST],
>>  					  0, 8, SPINOR_OP_READ_FAST,
>>  					  SNOR_PROTO_1_1_1);
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +		if (JEDEC_MFR(info) == SNOR_MFR_CYPRESS &&
>> +		    (info->id[1] == 0x2a || info->id[1] == 0x2b))
> 
> Add a comment about which flash models these two are.
> 
Will do.

>> +			params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>> +#endif
>>  	}
>>  
>>  	if (info->flags & SPI_NOR_QUAD_READ) {
>> @@ -659,6 +743,11 @@ static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
>>  		case SNOR_MFR_MACRONIX:
>>  			err = macronix_quad_enable(nor);
>>  			break;
>> +#endif
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +		case SNOR_MFR_CYPRESS:
>> +			err = spansion_quad_enable_volatile(nor, SZ_128M, 0);
>> +			break;
>>  #endif
>>  		case SNOR_MFR_ST:
>>  		case SNOR_MFR_MICRON:
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro

[PATCH v4 8/9] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Takahiro Kuwano]

Hi Pratyush,

On 2/2/2021 4:22 AM, Pratyush Yadav wrote:
> On 28/01/21 01:37PM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Add nor->setup() and fixup hooks to overwrite:
>>   - volatile QE bit
>>   - the ->ready() hook for dual/quad die package parts
>>   - overlaid erase
>>   - spi_nor_flash_parameter
>>   - mtd_info
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>>  drivers/mtd/spi/spi-nor-core.c | 108 +++++++++++++++++++++++++++++++++
>>  1 file changed, 108 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index ef49328a28..3d8cb9c333 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -2648,8 +2648,116 @@ static int spi_nor_init(struct spi_nor *nor)
>>  	return 0;
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +static int s25hx_t_mdp_ready(struct spi_nor *nor)
>> +{
>> +	u32 addr;
>> +	int ret;
>> +
>> +	for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
>> +		ret = spansion_sr_ready(nor, addr, 0);
>> +		if (ret != 1)
>> +			return ret;
>> +	}
>> +
>> +	return 1;
>> +}
>> +
>> +static int s25hx_t_quad_enable(struct spi_nor *nor)
>> +{
>> +	u32 addr;
>> +	int ret;
>> +
>> +	for (addr = 0; addr < nor->mtd.size; addr += SZ_128M) {
>> +		ret = spansion_quad_enable_volatile(nor, addr, 0);
> 
> So you need to set the QE bit on each die. Ok.
> 
> Out of curiosity, what will happen if you only set the QE bit on the 
> first die? Will reads from first die work in quad mode and rest in 
> single mode?
> 
If the host issues quad read command, only the first die works and rest
do not respond to the quad read command.

>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int s25hx_t_setup(struct spi_nor *nor, const struct flash_info *info,
>> +			 const struct spi_nor_flash_parameter *params,
>> +			 const struct spi_nor_hwcaps *hwcaps)
>> +{
>> +#ifdef CONFIG_SPI_FLASH_BAR
>> +	return -ENOTSUPP; /* Bank Address Register is not supported */
>> +#endif
>> +	/*
>> +	 * The Cypress Semper family has transparent ECC. To preserve
>> +	 * ECC enabled, multi-pass programming within the same 16-byte
>> +	 * ECC data unit needs to be avoided. Set writesize to the page
>> +	 * size and remove the MTD_BIT_WRITEABLE flag in mtd_info to
>> +	 * prevent multi-pass programming.
>> +	 */
>> +	nor->mtd.writesize = params->page_size;
> 
> The writesize should be set to 16. See [0][1][2].
> 
>> +	nor->mtd.flags &= ~MTD_BIT_WRITEABLE;
> 
> Not needed. See discussions pointed to above.
> 
OK, thank you for the information.

>> +
>> +	/* Emulate uniform sector architecure by this erase hook*/
>> +	nor->mtd._erase = spansion_overlaid_erase;
>> +
>> +	/* For 2Gb (dual die) and 4Gb (quad die) parts */
>> +	if (nor->mtd.size > SZ_128M)
>> +		nor->ready = s25hx_t_mdp_ready;
>> +
>> +	/* Enter 4-byte addressing mode for WRAR used in quad_enable */
>> +	set_4byte(nor, info, true);
>> +
>> +	return spi_nor_default_setup(nor, info, params, hwcaps);
>> +}
>> +
>> +static void s25hx_t_default_init(struct spi_nor *nor)
>> +{
>> +	nor->setup = s25hx_t_setup;
>> +}
>> +
>> +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
>> +				   const struct sfdp_parameter_header *header,
>> +				   const struct sfdp_bfpt *bfpt,
>> +				   struct spi_nor_flash_parameter *params)
>> +{
>> +	/* Default page size is 256-byte, but BFPT reports 512-byte */
>> +	params->page_size = 256;
> 
> Read the page size from the register, like it is done on Linux for S28 
> flash family.
> 
Will fix.

>> +	/* Reset erase size in case it is set to 4K from BFPT */
>> +	nor->mtd.erasesize = 0;
> 
> What does erasesize of 0 mean? I would take that to mean that the flash 
> does not support erases. I can't find any mention of 0 erase size in the 
> documentation of struct mtd_info.
> 
In this device, the erasesize is wrongly configured to 4K through BFPT
parse. I would reset it to 0 expecting the correct value is set in
spi_nor_select_erase() afterwards. But I should simply set correct value
in this fixup hook.


>> +
>> +	return 0;
>> +}
>> +
>> +static void s25hx_t_post_sfdp_fixup(struct spi_nor *nor,
>> +				    struct spi_nor_flash_parameter *params)
>> +{
>> +	/* READ_FAST_4B (0Ch) requires mode cycles*/
>> +	params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>> +	/* PP_1_1_4 is not supported */
>> +	params->hwcaps.mask &= ~SNOR_HWCAPS_PP_1_1_4;
>> +	/* Use volatile register to enable quad */
>> +	params->quad_enable = s25hx_t_quad_enable;
>> +}
>> +
>> +static struct spi_nor_fixups s25hx_t_fixups = {
>> +	.default_init = s25hx_t_default_init,
>> +	.post_bfpt = s25hx_t_post_bfpt_fixup,
>> +	.post_sfdp = s25hx_t_post_sfdp_fixup,
> 
> Hmm, I don't think the fixups feature was ever applied to the u-boot 
> tree. I sent a patch for them a while ago [3] but they were never 
> applied due to some issues. I can't find any mentions of 
> "spi_nor_set_fixups" on my 4 day old checkout of Tom's master branch. 
> 
> And that reminds me, the nor->setup() hook you are using is not there 
> either. Your patch series should not even build on upstream u-boot. 
> Please cherry pick the required patches from my series and send them 
> along with yours.
> 
> Please make sure your patch series builds and works on top of _upstream_ 
> u-boot. Even if it works on your company's fork of U-Boot does not 
> necessarily mean it will work on upstream.
> 
This patch depends on your patch that introduces the fixups feature.
I mentioned it in the changes log section in cover letter only. I will
add it into commit description of this patch. 

>> +};
>> +#endif
>> +
>>  static void spi_nor_set_fixups(struct spi_nor *nor)
> 
> This function is also not present in u-boot master.
> 
>>  {
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +	if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
>> +		switch (nor->info->id[1]) {
>> +		case 0x2a: /* S25HL (QSPI, 3.3V) */
>> +		case 0x2b: /* S25HS (QSPI, 1.8V) */
>> +			nor->fixups = &s25hx_t_fixups;
>> +			break;
> 
> I recall using strcmp() in my series but I guess this should also work 
> just as well.
> 
>> +
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +#endif
>>  }
>>  
>>  int spi_nor_scan(struct spi_nor *nor)
>> -- 
>> 2.25.1
>>
> 
> [0] https://lore.kernel.org/linux-mtd/4c0e3207-72a4-8c1a-5fca-e9f30cc60828 at ti.com/
> [1] https://lore.kernel.org/linux-mtd/20201201102711.8727-3-p.yadav at ti.com/
> [2] https://lore.kernel.org/linux-mtd/20201201102711.8727-4-p.yadav at ti.com/
> [3] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/
> 

Best Regards,
Takahiro