[PATCH v5 00/10] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t

[PATCH v5 00/10] 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 summary 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)

The full version can be found in the following links (registration
required).
https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503

Tested on Xilinx Zynq-7000 FPGA board.

Changes since v5:
  - Removed 256Mb and 4Gb parts support
  - Fixed register offset issue in spansion_quad_enable_volatile()
  - Added spi_nor_default_ready() and moved existing code into it
  - Separated spansion_sr_read() to new patch
  - Renamed spansion_overlaid_erase() to spansion_non_uniform_erase() and
    changed the implementation to issue the proper erase command based on
    the address
  - Added s25hx_t_erase_non_uniform()
  - Changed mtd.writesize and mtd.flags in s25hx_t_setup()
  - Fixed page size and erase size issues in s25hx_t_post_bfpt_fixup()

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/

Takahiro Kuwano (10):
  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: Read status by Read Any Register
  mtd: spi-nor-core: Add non-uniform erase for Spansion/Cypress
  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 | 345 ++++++++++++++++++++++++++++++++-
 drivers/mtd/spi/spi-nor-ids.c  |  27 +++
 drivers/mtd/spi/spi-nor-tiny.c |  90 +++++++++
 include/linux/mtd/spi-nor.h    |  10 +
 4 files changed, 471 insertions(+), 1 deletion(-)
-- 
2.25.1

[PATCH v5 01/10] 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>
Reviewed-by: Pratyush Yadav <p.yadav@ti.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 8e6744ac2e..d4f105df0f 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 v5 02/10] 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.

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

The full version can be found in the following links (registration
required).
https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503

Tested on Xilinx Zynq-7000 FPGA board.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
Changes in v5:
  - Remove 256Mb and 4Gb parts

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

diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
index 5bd5dd3003..052a0b62ee 100644
--- a/drivers/mtd/spi/spi-nor-ids.c
+++ b/drivers/mtd/spi/spi-nor-ids.c
@@ -217,6 +217,33 @@ 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 user-configurable
+	 * sector architecture. By default, the 512Mb and 1Gb, single-die
+	 * package parts are configured to non-uniform that 4KB sectors overlaid
+	 * on bottom address. To support this, an erase hook makes overlaid
+	 * sectors appear as uniform sectors. The 2Gb, dual-die package parts
+	 * are configured to uniform by default.
+	 */
+	{ 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("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) },
 #endif
 #ifdef CONFIG_SPI_FLASH_SST		/* SST */
 	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
-- 
2.25.1

[PATCH v5 03/10] 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>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
Changes in v5:
  - Remove unused defines from spi-nor.h

 drivers/mtd/spi/spi-nor-core.c | 25 +++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  2 ++
 2 files changed, 27 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 d4f105df0f..04bf59f6f9 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -121,6 +121,8 @@
 #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 */
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
-- 
2.25.1

[PATCH v5 04/10] 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>
---
Changes in v5:
  - Fix register address calculation, 'base | offset' -> 'base + offset' 

 drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  1 +
 2 files changed, 54 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 2803536ed5..87c9fce408 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.
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 04bf59f6f9..d79f8b37ef 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -123,6 +123,7 @@
 #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_CFR1V	0x00800002
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
-- 
2.25.1

[PATCH v5 05/10] 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 change in spi-nor.h depends on:
https://patchwork.ozlabs.org/project/uboot/patch/20210205041119.145784-4-seanga2 at gmail.com/ 

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v5:
  - Move spansion_sr_ready() to different patch
  - Move original code in spi_nor_ready() to newly created
    spi_nor_default_ready()

 drivers/mtd/spi/spi-nor-core.c | 10 +++++++++-
 include/linux/mtd/spi-nor.h    |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 87c9fce408..821617bc0d 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -566,7 +566,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
 	return fsr & FSR_READY;
 }
 
-static int spi_nor_ready(struct spi_nor *nor)
+static int spi_nor_default_ready(struct spi_nor *nor)
 {
 	int sr, fsr;
 
@@ -579,6 +579,14 @@ static int spi_nor_ready(struct spi_nor *nor)
 	return sr && fsr;
 }
 
+static int spi_nor_ready(struct spi_nor *nor)
+{
+	if (nor->ready)
+		return nor->ready(nor);
+
+	return spi_nor_default_ready(nor);
+}
+
 /*
  * Service routine to read status register until ready, or timeout occurs.
  * Returns non-zero if error.
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index d79f8b37ef..2f2ad20e8e 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -435,6 +435,7 @@ struct flash_info;
  * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
  *			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 {
@@ -480,6 +481,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 v5 06/10] mtd: spi-nor-core: Read status by Read Any Register

[tkuw584924 at gmail.com]

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

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 to support multi-die package parts from Spansion/Cypress.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v5:
  - New in v5, separated from another patch

 drivers/mtd/spi/spi-nor-core.c | 29 +++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 821617bc0d..e5fc0e7965 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);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 2f2ad20e8e..25234177de 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -123,6 +123,7 @@
 #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. */
-- 
2.25.1

[PATCH v5 07/10] mtd: spi-nor-core: Add non-uniform erase for Spansion/Cypress

[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.

The spansion_erase_non_uniform() erases overlaid 4KB sectors,
non-overlaid portion of normal sector, and remaining normal sectors, by
selecting correct erase command and size based on the address to erase
and size of overlaid portion in parameters.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v5:
  - New in v5, introduce spansion_erase_non_uniform() as a replacement
    for spansion_overlaid_erase() in v4

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

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index e5fc0e7965..46948ed41b 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -793,6 +793,78 @@ erase_err:
 	return ret;
 }
 
+#ifdef CONFIG_SPI_FLASH_SPANSION
+/**
+ * spansion_erase_non_uniform() - erase non-uniform sectors for Spansion/Cypress
+ *                                chips
+ * @mtd:	pointer to a 'struct mtd_info'
+ * @instr:	pointer to a 'struct erase_info'
+ * @ovlsz_top:	size of overlaid portion at the top address
+ * @ovlsz_btm:	size of overlaid portion at the bottom address
+ *
+ * Erase an address range on the nor chip that can contain 4KB sectors overlaid
+ * on top and/or bottom. The appropriate erase opcode and size are chosen by
+ * address to erase and size of overlaid portion.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_erase_non_uniform(struct mtd_info *mtd,
+				      struct erase_info *instr, u32 ovlsz_top,
+				      u32 ovlsz_btm)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	struct spi_mem_op op =
+		SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
+			   SPI_MEM_OP_ADDR(nor->addr_width, instr->addr, 1),
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_NO_DATA);
+	u32 len = instr->len;
+	u32 erasesize;
+	int ret;
+
+	while (len) {
+		/* 4KB sectors */
+		if (op.addr.val < ovlsz_btm ||
+		    op.addr.val >= mtd->size - ovlsz_top) {
+			op.cmd.opcode = SPINOR_OP_BE_4K;
+			erasesize = SZ_4K;
+
+		/* Non-overlaid portion in the normal sector at the bottom */
+		} else if (op.addr.val == ovlsz_btm) {
+			op.cmd.opcode = nor->erase_opcode;
+			erasesize = mtd->erasesize - ovlsz_btm;
+
+		/* Non-overlaid portion in the normal sector at the top */
+		} else if (op.addr.val == mtd->size - mtd->erasesize) {
+			op.cmd.opcode = nor->erase_opcode;
+			erasesize = mtd->erasesize - ovlsz_top;
+
+		/* Normal sectors */
+		} else {
+			op.cmd.opcode = nor->erase_opcode;
+			erasesize = mtd->erasesize;
+		}
+
+		write_enable(nor);
+
+		ret = spi_mem_exec_op(nor->spi, &op);
+		if (ret)
+			break;
+
+		op.addr.val += erasesize;
+		len -= erasesize;
+
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			break;
+	}
+
+	write_disable(nor);
+
+	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 v5 08/10] 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 46948ed41b..8d63681cb3 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 v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[tkuw584924 at gmail.com]

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

This patch adds Flash specific fixups and hooks for Cypress
S25HL-T/S25HS-T family, based on the features introduced in [0][1][2].

The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this
series are used for multi-die package parts.

The nor->quad_enable() sets the volatile QE bit on each die.

The mtd._erase() is hooked if the device is single-die package and not
configured to uniform sectors, assuming it is in factory default
configuration that has 32 x 4KB sectors overlaid on bottom address.
Other configurations, top and split, are not supported at this point.
Will submit additional patches to support it as needed.

The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP.

[0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/
[1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/
[2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v5:
  - Add s25hx_t_erase_non_uniform()
  - Change mtd.writesize and mtd.flags in s25hx_t_setup()
  - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup()

 drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |   3 +
 2 files changed, 158 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 8d63681cb3..315e26ab27 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -2677,8 +2677,163 @@ 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_erase_non_uniform(struct mtd_info *mtd,
+				     struct erase_info *instr)
+{
+	/* Support factory default configuration (32 x 4KB sectors at bottom) */
+	return spansion_erase_non_uniform(mtd, instr, 0, SZ_128K);
+}
+
+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.
+	 */
+	nor->mtd.writesize = 16;
+
+	if (nor->mtd.size > SZ_128M) {
+		/*
+		 * For the multi-die package parts, the ready() hook is needed
+		 * to check all dies' status via read any register.
+		 */
+		nor->ready = s25hx_t_mdp_ready;
+	} else {
+		int ret;
+		u8 cfr3v;
+
+		/*
+		 * Read CFR3V to check if uniform sector is selected. If not,
+		 * assign an erase hook that supports non-uniform erase,
+		 * assuming the part has factory default configuration,
+		 * 32 x 4KB sectors at bottom.
+		 */
+		ret = spansion_read_any_reg(nor, SPINOR_REG_ADDR_CFR3V, 0,
+					    &cfr3v);
+		if (ret)
+			return ret;
+
+		if (!(cfr3v & CFR3V_UNHYSA))
+			nor->mtd._erase = s25hx_t_erase_non_uniform;
+	}
+
+	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)
+{
+	int ret;
+	u32 addr;
+	u8 cfr3v;
+
+	/* erase size in case it is set to 4K from BFPT */
+	nor->erase_opcode = SPINOR_OP_SE_4B;
+	nor->mtd.erasesize = nor->info->sector_size;
+
+	/* Enter 4-byte addressing mode for Read Any Register */
+	ret = set_4byte(nor, nor->info, 1);
+	if (ret)
+		return ret;
+	nor->addr_width = 4;
+
+	/*
+	 * The page_size is set to 512B from BFPT, but it actually depends on
+	 * the configuration register. Look up the CFR3V and determine the
+	 * page_size. For multi-die package parts, use 512B only when the all
+	 * dies are configured to 512B buffer.
+	 */
+	for (addr = 0; addr < params->size; addr += SZ_128M) {
+		ret = spansion_read_any_reg(nor, addr + SPINOR_REG_ADDR_CFR3V,
+					    0, &cfr3v);
+		if (ret)
+			return ret;
+
+		if (!(cfr3v & CFR3V_PGMBUF)) {
+			params->page_size = 256;
+			return 0;
+		}
+	}
+	params->page_size = 512;
+
+	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)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 25234177de..cfb2104fee 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -125,6 +125,9 @@
 #define SPINOR_OP_WRAR		0x71	/* Write any register */
 #define SPINOR_REG_ADDR_STR1V	0x00800000
 #define SPINOR_REG_ADDR_CFR1V	0x00800002
+#define SPINOR_REG_ADDR_CFR3V	0x00800004
+#define CFR3V_UNHYSA		BIT(3)	/* Uniform sectors or not */
+#define CFR3V_PGMBUF		BIT(4)	/* Program buffer size */
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
-- 
2.25.1

[PATCH v5 10/10] 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>
---
Changes in v5:
  - Add a comment about Flash models and respective IDs

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

diff --git a/drivers/mtd/spi/spi-nor-tiny.c b/drivers/mtd/spi/spi-nor-tiny.c
index 5cc2b7d996..66e4c99cc6 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,12 @@ 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))
+			/* 0x2a: S25HL (QSPI, 3.3V), 0x2b: S25HS (QSPI, 1.8V) */
+			params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+#endif
 	}
 
 	if (info->flags & SPI_NOR_QUAD_READ) {
@@ -659,6 +744,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 v5 02/10] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 19/02/21 10:55AM, 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.
> 
> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
> 
> The full version can be found in the following links (registration
> required).
> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503

I created an account to view these. I get "Access denied" on both.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v5 05/10] mtd: spi-nor-core: Add the ->ready() hook

[Pratyush Yadav]

On 19/02/21 10:55AM, 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 change in spi-nor.h depends on:
> https://patchwork.ozlabs.org/project/uboot/patch/20210205041119.145784-4-seanga2 at gmail.com/ 

This line should not be a part of the commit message. This is "metadata" 
that says how the patch should be applied. One it is applied this has no 
use in the Git history. So it should go below the 3 dashed lines, like 
the changelog.

Other than this,

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v5 06/10] mtd: spi-nor-core: Read status by Read Any Register

[Pratyush Yadav]

On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> 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 to support multi-die package parts from Spansion/Cypress.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v5 07/10] mtd: spi-nor-core: Add non-uniform erase for Spansion/Cypress

[Pratyush Yadav]

On 19/02/21 10:56AM, 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.
> 
> The spansion_erase_non_uniform() erases overlaid 4KB sectors,
> non-overlaid portion of normal sector, and remaining normal sectors, by
> selecting correct erase command and size based on the address to erase
> and size of overlaid portion in parameters.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v5:
>   - New in v5, introduce spansion_erase_non_uniform() as a replacement
>     for spansion_overlaid_erase() in v4
> 
>  drivers/mtd/spi/spi-nor-core.c | 72 ++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index e5fc0e7965..46948ed41b 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -793,6 +793,78 @@ erase_err:
>  	return ret;
>  }
>  
> +#ifdef CONFIG_SPI_FLASH_SPANSION
> +/**
> + * spansion_erase_non_uniform() - erase non-uniform sectors for Spansion/Cypress
> + *                                chips
> + * @mtd:	pointer to a 'struct mtd_info'
> + * @instr:	pointer to a 'struct erase_info'
> + * @ovlsz_top:	size of overlaid portion at the top address
> + * @ovlsz_btm:	size of overlaid portion at the bottom address
> + *
> + * Erase an address range on the nor chip that can contain 4KB sectors overlaid
> + * on top and/or bottom. The appropriate erase opcode and size are chosen by
> + * address to erase and size of overlaid portion.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_erase_non_uniform(struct mtd_info *mtd,
> +				      struct erase_info *instr, u32 ovlsz_top,
> +				      u32 ovlsz_btm)

Is there any reason why you are not using the nor->erase() hook? As far 
as I can see it should also be able to perform the same erase while 
avoiding all the boilerplate needed for keeping track of address, 
remaining erase, write enable, error handling, etc.

One problem I can see is that you don't always increment address by 
nor->erasesize. That can change depending on which sector got erased. 
spi_nor_erase() can't currently handle that. But IMO a reasonable fix 
for this would be to return the size actually erased in nor->erase(), 
like how it is done for the unix read() and write() system calls. A 
negative value would still mean an error but now a positive return value 
will tell the caller how much data was actually erased.

I think it is a relatively easy refactor and worth doing.

> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	struct spi_mem_op op =
> +		SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
> +			   SPI_MEM_OP_ADDR(nor->addr_width, instr->addr, 1),
> +			   SPI_MEM_OP_NO_DUMMY,
> +			   SPI_MEM_OP_NO_DATA);
> +	u32 len = instr->len;
> +	u32 erasesize;
> +	int ret;

spi_nor_erase() does some sanity checking for instr->len. Even more 
reason to use nor->erase() so we don't have to duplicate that code.

> +
> +	while (len) {
> +		/* 4KB sectors */
> +		if (op.addr.val < ovlsz_btm ||
> +		    op.addr.val >= mtd->size - ovlsz_top) {
> +			op.cmd.opcode = SPINOR_OP_BE_4K;
> +			erasesize = SZ_4K;

Ok.

> +
> +		/* Non-overlaid portion in the normal sector at the bottom */
> +		} else if (op.addr.val == ovlsz_btm) {
> +			op.cmd.opcode = nor->erase_opcode;
> +			erasesize = mtd->erasesize - ovlsz_btm;
> +
> +		/* Non-overlaid portion in the normal sector at the top */
> +		} else if (op.addr.val == mtd->size - mtd->erasesize) {
> +			op.cmd.opcode = nor->erase_opcode;
> +			erasesize = mtd->erasesize - ovlsz_top;

Patch 9/10 does not check for uniform sector size configuration. But if 
the check is to be added later, passing 0 to ovlsz_top and ovlsz_btm 
will do the right thing because erasesize will end up equal to 
mtd->erasesize in both cases. Neat!

> +
> +		/* Normal sectors */
> +		} else {
> +			op.cmd.opcode = nor->erase_opcode;
> +			erasesize = mtd->erasesize;
> +		}

Ok.

> +
> +		write_enable(nor);
> +
> +		ret = spi_mem_exec_op(nor->spi, &op);
> +		if (ret)
> +			break;
> +
> +		op.addr.val += erasesize;
> +		len -= erasesize;

I recall a patch for Linux by Tudor recently that moved some code like 
this to after spi_nor_wait_till_ready(). Let's do so here as well. Of 
course, this will not matter if you are using nor->erase() instead. The 
problem will still be there since spi_nor_erase() also does this but 
that is a separate fix.

> +
> +		ret = spi_nor_wait_till_ready(nor);
> +		if (ret)
> +			break;
> +	}
> +
> +	write_disable(nor);
> +
> +	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 v5 08/10] mtd: spi-nor-core: Add Cypress manufacturer ID in set_4byte

[Pratyush Yadav]

On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Cypress chips support SPINOR_OP_EN4B(B7h)/SPINOR_OP_EX4B(E9h) to

The datasheet says the EN4B command is indeed B7h but EX4B is listed as 
B8h. The command E9h is for "Password Unlock". So exiting 4 byte mode 
will do something completely different.

> 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 46948ed41b..8d63681cb3 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
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> This patch adds Flash specific fixups and hooks for Cypress
> S25HL-T/S25HS-T family, based on the features introduced in [0][1][2].

Instead of linking the patches like this, it would be a better idea to 
simply include them in your series. This will make your series 
independent from mine and will make it easier for the maintainer to 
apply it.

> 
> The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this
> series are used for multi-die package parts.

Nitpick: Once this patch makes it into the Git history there is no patch 
#5 or #6. Probably a better idea to just say "introduced earlier" or 
something similar. 

> 
> The nor->quad_enable() sets the volatile QE bit on each die.
> 
> The mtd._erase() is hooked if the device is single-die package and not
> configured to uniform sectors, assuming it is in factory default
> configuration that has 32 x 4KB sectors overlaid on bottom address.
> Other configurations, top and split, are not supported at this point.
> Will submit additional patches to support it as needed.

Ah, this patch does check for uniform sector map. Nice. I assume you 
have tested with both configurations.

> 
> The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP.
> 
> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/
> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/
> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v5:
>   - Add s25hx_t_erase_non_uniform()
>   - Change mtd.writesize and mtd.flags in s25hx_t_setup()
>   - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup()
> 
>  drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |   3 +
>  2 files changed, 158 insertions(+)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 8d63681cb3..315e26ab27 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -2677,8 +2677,163 @@ 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)

Nitpick: Use if (!ret) since spansion_sr_ready() returns a boolean 
value.

> +			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_erase_non_uniform(struct mtd_info *mtd,
> +				     struct erase_info *instr)
> +{
> +	/* Support factory default configuration (32 x 4KB sectors at bottom) */
> +	return spansion_erase_non_uniform(mtd, instr, 0, SZ_128K);
> +}
> +
> +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.
> +	 */
> +	nor->mtd.writesize = 16;

Sorry, I forgot what your end goal was with setting the writesize. Which 
layer is doing multi-pass writes? You would need to check if those 
layers actually respect this value. In Linux some consumers of the MTD 
layer simply assumed the writesize for a NOR flash is 1 (like UBI for 
example). I had to patch them before everything was back to normal.

> +
> +	if (nor->mtd.size > SZ_128M) {
> +		/*
> +		 * For the multi-die package parts, the ready() hook is needed
> +		 * to check all dies' status via read any register.
> +		 */
> +		nor->ready = s25hx_t_mdp_ready;
> +	} else {
> +		int ret;
> +		u8 cfr3v;
> +
> +		/*
> +		 * Read CFR3V to check if uniform sector is selected. If not,
> +		 * assign an erase hook that supports non-uniform erase,
> +		 * assuming the part has factory default configuration,
> +		 * 32 x 4KB sectors at bottom.
> +		 */
> +		ret = spansion_read_any_reg(nor, SPINOR_REG_ADDR_CFR3V, 0,
> +					    &cfr3v);
> +		if (ret)
> +			return ret;
> +
> +		if (!(cfr3v & CFR3V_UNHYSA))
> +			nor->mtd._erase = s25hx_t_erase_non_uniform;

Multi-die parts can also have uniform sector sizes. Why not do this 
check for them as well?

> +	}
> +
> +	return spi_nor_default_setup(nor, info, params, hwcaps);
> +}
> +
> +static void s25hx_t_default_init(struct spi_nor *nor)
> +{
> +	nor->setup = s25hx_t_setup;
> +}

Ok.

> +
> +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)
> +{
> +	int ret;
> +	u32 addr;
> +	u8 cfr3v;
> +
> +	/* erase size in case it is set to 4K from BFPT */
> +	nor->erase_opcode = SPINOR_OP_SE_4B;
> +	nor->mtd.erasesize = nor->info->sector_size;
> +
> +	/* Enter 4-byte addressing mode for Read Any Register */
> +	ret = set_4byte(nor, nor->info, 1);
> +	if (ret)
> +		return ret;

You enter 4byte addressing but don't exit it before returning. Is this 
intentional?

> +	nor->addr_width = 4;
> +
> +	/*
> +	 * The page_size is set to 512B from BFPT, but it actually depends on
> +	 * the configuration register. Look up the CFR3V and determine the
> +	 * page_size. For multi-die package parts, use 512B only when the all
> +	 * dies are configured to 512B buffer.
> +	 */
> +	for (addr = 0; addr < params->size; addr += SZ_128M) {
> +		ret = spansion_read_any_reg(nor, addr + SPINOR_REG_ADDR_CFR3V,
> +					    0, &cfr3v);
> +		if (ret)
> +			return ret;
> +
> +		if (!(cfr3v & CFR3V_PGMBUF)) {
> +			params->page_size = 256;
> +			return 0;
> +		}
> +	}
> +	params->page_size = 512;

Ok.

> +
> +	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;

Ok.

> +
> +		default:
> +			break;
> +		}
> +	}
> +#endif
>  }
>  
>  int spi_nor_scan(struct spi_nor *nor)
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 25234177de..cfb2104fee 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -125,6 +125,9 @@
>  #define SPINOR_OP_WRAR		0x71	/* Write any register */
>  #define SPINOR_REG_ADDR_STR1V	0x00800000
>  #define SPINOR_REG_ADDR_CFR1V	0x00800002
> +#define SPINOR_REG_ADDR_CFR3V	0x00800004
> +#define CFR3V_UNHYSA		BIT(3)	/* Uniform sectors or not */
> +#define CFR3V_PGMBUF		BIT(4)	/* Program buffer size */

Ok.

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Pratyush Yadav]

On 19/02/21 10:56AM, 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.

As explained in v4 [0], 

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

[0] https://patchwork.ozlabs.org/project/uboot/patch/a5c3cf1353d9a621379e2fcfefc51fb44c9680c5.1611729896.git.Takahiro.Kuwano at infineon.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v5 00/10] mtd: spi-nor: Add support for Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

Hi all,

On 19/02/21 10:55AM, 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 summary 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)
> 
> The full version can be found in the following links (registration
> required).
> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503
> 
> Tested on Xilinx Zynq-7000 FPGA board.

I think this series is in very good shape apart from a few minor issues. 
Thanks for all the work :-)

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v5 02/10] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t

[Jagan Teki]

On Fri, Feb 19, 2021 at 7:26 AM <tkuw584924@gmail.com> wrote:
>
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
>
> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
>
> The full version can be found in the following links (registration
> required).
> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503
>
> Tested on Xilinx Zynq-7000 FPGA board.
>
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
> Changes in v5:
>   - Remove 256Mb and 4Gb parts
>
>  drivers/mtd/spi/spi-nor-ids.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
> index 5bd5dd3003..052a0b62ee 100644
> --- a/drivers/mtd/spi/spi-nor-ids.c
> +++ b/drivers/mtd/spi/spi-nor-ids.c
> @@ -217,6 +217,33 @@ 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 user-configurable
> +        * sector architecture. By default, the 512Mb and 1Gb, single-die
> +        * package parts are configured to non-uniform that 4KB sectors overlaid
> +        * on bottom address. To support this, an erase hook makes overlaid
> +        * sectors appear as uniform sectors. The 2Gb, dual-die package parts
> +        * are configured to uniform by default.
> +        */

Nice to have these comments in commit instead of id's sections.

[PATCH v5 04/10] mtd: spi-nor-core: Add support for volatile QE bit

[Jagan Teki]

On Fri, Feb 19, 2021 at 7:26 AM <tkuw584924@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>
> ---
> Changes in v5:
>   - Fix register address calculation, 'base | offset' -> 'base + offset'
>
>  drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  1 +
>  2 files changed, 54 insertions(+)
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 2803536ed5..87c9fce408 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);

What if we can use the exiting quad_enable hook by identifying
volatile QE at the function beginning instead of having a separate
call?

Jagan.

[PATCH v5 05/10] mtd: spi-nor-core: Add the ->ready() hook

[Takahiro Kuwano]

On 2/19/2021 6:57 PM, Pratyush Yadav wrote:
> On 19/02/21 10:55AM, 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 change in spi-nor.h depends on:
>> https://patchwork.ozlabs.org/project/uboot/patch/20210205041119.145784-4-seanga2 at gmail.com/ 
> 
> This line should not be a part of the commit message. This is "metadata" 
> that says how the patch should be applied. One it is applied this has no 
> use in the Git history. So it should go below the 3 dashed lines, like 
> the changelog.
> 
> Other than this,
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 
I will make it goes below the "---" line. Thank you.

[PATCH v5 07/10] mtd: spi-nor-core: Add non-uniform erase for Spansion/Cypress

[Takahiro Kuwano]

Hi Pratyush,

On 2/24/2021 9:05 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, 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.
>>
>> The spansion_erase_non_uniform() erases overlaid 4KB sectors,
>> non-overlaid portion of normal sector, and remaining normal sectors, by
>> selecting correct erase command and size based on the address to erase
>> and size of overlaid portion in parameters.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v5:
>>   - New in v5, introduce spansion_erase_non_uniform() as a replacement
>>     for spansion_overlaid_erase() in v4
>>
>>  drivers/mtd/spi/spi-nor-core.c | 72 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 72 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index e5fc0e7965..46948ed41b 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -793,6 +793,78 @@ erase_err:
>>  	return ret;
>>  }
>>  
>> +#ifdef CONFIG_SPI_FLASH_SPANSION
>> +/**
>> + * spansion_erase_non_uniform() - erase non-uniform sectors for Spansion/Cypress
>> + *                                chips
>> + * @mtd:	pointer to a 'struct mtd_info'
>> + * @instr:	pointer to a 'struct erase_info'
>> + * @ovlsz_top:	size of overlaid portion at the top address
>> + * @ovlsz_btm:	size of overlaid portion at the bottom address
>> + *
>> + * Erase an address range on the nor chip that can contain 4KB sectors overlaid
>> + * on top and/or bottom. The appropriate erase opcode and size are chosen by
>> + * address to erase and size of overlaid portion.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_erase_non_uniform(struct mtd_info *mtd,
>> +				      struct erase_info *instr, u32 ovlsz_top,
>> +				      u32 ovlsz_btm)
> 
> Is there any reason why you are not using the nor->erase() hook? As far 
> as I can see it should also be able to perform the same erase while 
> avoiding all the boilerplate needed for keeping track of address, 
> remaining erase, write enable, error handling, etc.
> 
I tried to use the nor-erase() hook, but I saw the erasesize problem
you mentioned below and didn't think about changing the return value of
existing function.

> One problem I can see is that you don't always increment address by 
> nor->erasesize. That can change depending on which sector got erased. 
> spi_nor_erase() can't currently handle that. But IMO a reasonable fix 
> for this would be to return the size actually erased in nor->erase(), 
> like how it is done for the unix read() and write() system calls. A 
> negative value would still mean an error but now a positive return value 
> will tell the caller how much data was actually erased.
> 
> I think it is a relatively easy refactor and worth doing.
> 
I agree. Let me introduce it in v6.

>> +{
>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +	struct spi_mem_op op =
>> +		SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
>> +			   SPI_MEM_OP_ADDR(nor->addr_width, instr->addr, 1),
>> +			   SPI_MEM_OP_NO_DUMMY,
>> +			   SPI_MEM_OP_NO_DATA);
>> +	u32 len = instr->len;
>> +	u32 erasesize;
>> +	int ret;
> 
> spi_nor_erase() does some sanity checking for instr->len. Even more 
> reason to use nor->erase() so we don't have to duplicate that code.
> 
>> +
>> +	while (len) {
>> +		/* 4KB sectors */
>> +		if (op.addr.val < ovlsz_btm ||
>> +		    op.addr.val >= mtd->size - ovlsz_top) {
>> +			op.cmd.opcode = SPINOR_OP_BE_4K;
>> +			erasesize = SZ_4K;
> 
> Ok.
> 
>> +
>> +		/* Non-overlaid portion in the normal sector at the bottom */
>> +		} else if (op.addr.val == ovlsz_btm) {
>> +			op.cmd.opcode = nor->erase_opcode;
>> +			erasesize = mtd->erasesize - ovlsz_btm;
>> +
>> +		/* Non-overlaid portion in the normal sector at the top */
>> +		} else if (op.addr.val == mtd->size - mtd->erasesize) {
>> +			op.cmd.opcode = nor->erase_opcode;
>> +			erasesize = mtd->erasesize - ovlsz_top;
> 
> Patch 9/10 does not check for uniform sector size configuration. But if 
> the check is to be added later, passing 0 to ovlsz_top and ovlsz_btm 
> will do the right thing because erasesize will end up equal to 
> mtd->erasesize in both cases. Neat!
> 
>> +
>> +		/* Normal sectors */
>> +		} else {
>> +			op.cmd.opcode = nor->erase_opcode;
>> +			erasesize = mtd->erasesize;
>> +		}
> 
> Ok.
> 
>> +
>> +		write_enable(nor);
>> +
>> +		ret = spi_mem_exec_op(nor->spi, &op);
>> +		if (ret)
>> +			break;
>> +
>> +		op.addr.val += erasesize;
>> +		len -= erasesize;
> 
> I recall a patch for Linux by Tudor recently that moved some code like 
> this to after spi_nor_wait_till_ready(). Let's do so here as well. Of 
> course, this will not matter if you are using nor->erase() instead. The 
> problem will still be there since spi_nor_erase() also does this but 
> that is a separate fix.
> 
>> +
>> +		ret = spi_nor_wait_till_ready(nor);
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	write_disable(nor);
>> +
>> +	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 v5 08/10] mtd: spi-nor-core: Add Cypress manufacturer ID in set_4byte

[Takahiro Kuwano]

On 2/24/2021 9:11 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> Cypress chips support SPINOR_OP_EN4B(B7h)/SPINOR_OP_EX4B(E9h) to
> 
> The datasheet says the EN4B command is indeed B7h but EX4B is listed as 
> B8h. The command E9h is for "Password Unlock". So exiting 4 byte mode 
> will do something completely different.
> 
How stupid of me!
Thank you for thorough review.

[PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Takahiro Kuwano]

On 2/24/2021 9:40 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> This patch adds Flash specific fixups and hooks for Cypress
>> S25HL-T/S25HS-T family, based on the features introduced in [0][1][2].
> 
> Instead of linking the patches like this, it would be a better idea to 
> simply include them in your series. This will make your series 
> independent from mine and will make it easier for the maintainer to 
> apply it.
> 
Noted with thanks.

>>
>> The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this
>> series are used for multi-die package parts.
> 
> Nitpick: Once this patch makes it into the Git history there is no patch 
> #5 or #6. Probably a better idea to just say "introduced earlier" or 
> something similar. 
> 
OK.

>>
>> The nor->quad_enable() sets the volatile QE bit on each die.
>>
>> The mtd._erase() is hooked if the device is single-die package and not
>> configured to uniform sectors, assuming it is in factory default
>> configuration that has 32 x 4KB sectors overlaid on bottom address.
>> Other configurations, top and split, are not supported at this point.
>> Will submit additional patches to support it as needed.
> 
> Ah, this patch does check for uniform sector map. Nice. I assume you 
> have tested with both configurations.
> 
Yes. I tested both.

>>
>> The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP.
>>
>> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/
>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/
>> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v5:
>>   - Add s25hx_t_erase_non_uniform()
>>   - Change mtd.writesize and mtd.flags in s25hx_t_setup()
>>   - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup()
>>
>>  drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h    |   3 +
>>  2 files changed, 158 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 8d63681cb3..315e26ab27 100644
>> --- a/drivers/mtd/spi/spi-nor-core.c
>> +++ b/drivers/mtd/spi/spi-nor-core.c
>> @@ -2677,8 +2677,163 @@ 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)
> 
> Nitpick: Use if (!ret) since spansion_sr_ready() returns a boolean 
> value.
> 
Will fix.

>> +			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_erase_non_uniform(struct mtd_info *mtd,
>> +				     struct erase_info *instr)
>> +{
>> +	/* Support factory default configuration (32 x 4KB sectors at bottom) */
>> +	return spansion_erase_non_uniform(mtd, instr, 0, SZ_128K);
>> +}
>> +
>> +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.
>> +	 */
>> +	nor->mtd.writesize = 16;
> 
> Sorry, I forgot what your end goal was with setting the writesize. Which 
> layer is doing multi-pass writes? You would need to check if those 
> layers actually respect this value. In Linux some consumers of the MTD 
> layer simply assumed the writesize for a NOR flash is 1 (like UBI for 
> example). I had to patch them before everything was back to normal.
> 
My goal so far is enabling access this Flash via cmd/sf that does not
respect the writesize. I think I can remove this.

>> +
>> +	if (nor->mtd.size > SZ_128M) {
>> +		/*
>> +		 * For the multi-die package parts, the ready() hook is needed
>> +		 * to check all dies' status via read any register.
>> +		 */
>> +		nor->ready = s25hx_t_mdp_ready;
>> +	} else {
>> +		int ret;
>> +		u8 cfr3v;
>> +
>> +		/*
>> +		 * Read CFR3V to check if uniform sector is selected. If not,
>> +		 * assign an erase hook that supports non-uniform erase,
>> +		 * assuming the part has factory default configuration,
>> +		 * 32 x 4KB sectors at bottom.
>> +		 */
>> +		ret = spansion_read_any_reg(nor, SPINOR_REG_ADDR_CFR3V, 0,
>> +					    &cfr3v);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (!(cfr3v & CFR3V_UNHYSA))
>> +			nor->mtd._erase = s25hx_t_erase_non_uniform;
> 
> Multi-die parts can also have uniform sector sizes. Why not do this 
> check for them as well?
> 
Multi-die parts are configured to uniform by default and this patch assumes
they are in default sector size setting. But I have realized I can easily
add support for bottom in multi-die parts. Will do it in v6.

>> +	}
>> +
>> +	return spi_nor_default_setup(nor, info, params, hwcaps);
>> +}
>> +
>> +static void s25hx_t_default_init(struct spi_nor *nor)
>> +{
>> +	nor->setup = s25hx_t_setup;
>> +}
> 
> Ok.
> 
>> +
>> +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)
>> +{
>> +	int ret;
>> +	u32 addr;
>> +	u8 cfr3v;
>> +
>> +	/* erase size in case it is set to 4K from BFPT */
>> +	nor->erase_opcode = SPINOR_OP_SE_4B;
>> +	nor->mtd.erasesize = nor->info->sector_size;
>> +
>> +	/* Enter 4-byte addressing mode for Read Any Register */
>> +	ret = set_4byte(nor, nor->info, 1);
>> +	if (ret)
>> +		return ret;
> 
> You enter 4byte addressing but don't exit it before returning. Is this 
> intentional?
> 
Yes. The nor->addr_width must be 4 for read/write/erase to work correctly.
I think device's addressing mode should be in sync with nor->addr_width.

>> +	nor->addr_width = 4;
>> +
>> +	/*
>> +	 * The page_size is set to 512B from BFPT, but it actually depends on
>> +	 * the configuration register. Look up the CFR3V and determine the
>> +	 * page_size. For multi-die package parts, use 512B only when the all
>> +	 * dies are configured to 512B buffer.
>> +	 */
>> +	for (addr = 0; addr < params->size; addr += SZ_128M) {
>> +		ret = spansion_read_any_reg(nor, addr + SPINOR_REG_ADDR_CFR3V,
>> +					    0, &cfr3v);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (!(cfr3v & CFR3V_PGMBUF)) {
>> +			params->page_size = 256;
>> +			return 0;
>> +		}
>> +	}
>> +	params->page_size = 512;
> 
> Ok.
> 
>> +
>> +	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;
> 
> Ok.
> 
>> +
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +#endif
>>  }
>>  
>>  int spi_nor_scan(struct spi_nor *nor)
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index 25234177de..cfb2104fee 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -125,6 +125,9 @@
>>  #define SPINOR_OP_WRAR		0x71	/* Write any register */
>>  #define SPINOR_REG_ADDR_STR1V	0x00800000
>>  #define SPINOR_REG_ADDR_CFR1V	0x00800002
>> +#define SPINOR_REG_ADDR_CFR3V	0x00800004
>> +#define CFR3V_UNHYSA		BIT(3)	/* Uniform sectors or not */
>> +#define CFR3V_PGMBUF		BIT(4)	/* Program buffer size */
> 
> Ok.
> 
>>  
>>  /* Used for Micron flashes only. */
>>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
>> -- 
>> 2.25.1
>>
> 

[PATCH v5 02/10] mtd: spi-nor-ids: Add Cypress s25hl-t/s25hs-t

[Takahiro Kuwano]

Hi Jagan,

On 2/26/2021 7:35 PM, Jagan Teki wrote:
> On Fri, Feb 19, 2021 at 7:26 AM <tkuw584924@gmail.com> wrote:
>>
>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>
>> The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.
>>
>> https://www.cypress.com/file/424146/download (256Mb/512Mb/1Gb, single die)
>> https://www.cypress.com/file/499246/download (2Gb/4Gb, dual/quad die)
>>
>> The full version can be found in the following links (registration
>> required).
>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-Semper-Flash-with-Quad-SPI/ta-p/260789?attachment-id=19522
>> https://community.cypress.com/t5/Semper-Flash-Access-Program/Datasheet-2Gb-MCP-Semper-Flash-with-Quad-SPI/ta-p/260823?attachment-id=29503
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>> Changes in v5:
>>   - Remove 256Mb and 4Gb parts
>>
>>  drivers/mtd/spi/spi-nor-ids.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index 5bd5dd3003..052a0b62ee 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -217,6 +217,33 @@ 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 user-configurable
>> +        * sector architecture. By default, the 512Mb and 1Gb, single-die
>> +        * package parts are configured to non-uniform that 4KB sectors overlaid
>> +        * on bottom address. To support this, an erase hook makes overlaid
>> +        * sectors appear as uniform sectors. The 2Gb, dual-die package parts
>> +        * are configured to uniform by default.
>> +        */
> 
> Nice to have these comments in commit instead of id's sections.
> 
I will move it to commit in v6.

Thanks,
Takahiro

[PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 08/03/21 05:47PM, Takahiro Kuwano wrote:
> On 2/24/2021 9:40 PM, Pratyush Yadav wrote:
> > On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
> >> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> >>
> >> This patch adds Flash specific fixups and hooks for Cypress
> >> S25HL-T/S25HS-T family, based on the features introduced in [0][1][2].
> > 
> > Instead of linking the patches like this, it would be a better idea to 
> > simply include them in your series. This will make your series 
> > independent from mine and will make it easier for the maintainer to 
> > apply it.
> > 
> Noted with thanks.
> 
> >>
> >> The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this
> >> series are used for multi-die package parts.
> > 
> > Nitpick: Once this patch makes it into the Git history there is no patch 
> > #5 or #6. Probably a better idea to just say "introduced earlier" or 
> > something similar. 
> > 
> OK.
> 
> >>
> >> The nor->quad_enable() sets the volatile QE bit on each die.
> >>
> >> The mtd._erase() is hooked if the device is single-die package and not
> >> configured to uniform sectors, assuming it is in factory default
> >> configuration that has 32 x 4KB sectors overlaid on bottom address.
> >> Other configurations, top and split, are not supported at this point.
> >> Will submit additional patches to support it as needed.
> > 
> > Ah, this patch does check for uniform sector map. Nice. I assume you 
> > have tested with both configurations.
> > 
> Yes. I tested both.
> 
> >>
> >> The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP.
> >>
> >> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/
> >> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/
> >> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/
> >>
> >> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> >> ---
> >> Changes in v5:
> >>   - Add s25hx_t_erase_non_uniform()
> >>   - Change mtd.writesize and mtd.flags in s25hx_t_setup()
> >>   - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup()
> >>
> >>  drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++
> >>  include/linux/mtd/spi-nor.h    |   3 +
> >>  2 files changed, 158 insertions(+)
> >>
> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> >> index 8d63681cb3..315e26ab27 100644
> >> --- a/drivers/mtd/spi/spi-nor-core.c
> >> +++ b/drivers/mtd/spi/spi-nor-core.c
[...]
> >> +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)
> >> +{
> >> +	int ret;
> >> +	u32 addr;
> >> +	u8 cfr3v;
> >> +
> >> +	/* erase size in case it is set to 4K from BFPT */
> >> +	nor->erase_opcode = SPINOR_OP_SE_4B;
> >> +	nor->mtd.erasesize = nor->info->sector_size;
> >> +
> >> +	/* Enter 4-byte addressing mode for Read Any Register */
> >> +	ret = set_4byte(nor, nor->info, 1);
> >> +	if (ret)
> >> +		return ret;
> > 
> > You enter 4byte addressing but don't exit it before returning. Is this 
> > intentional?
> > 
> Yes. The nor->addr_width must be 4 for read/write/erase to work correctly.
> I think device's addressing mode should be in sync with nor->addr_width.

Right. But the comment above implies that 4-byte mode is only enabled 
for the "Read Any Register" command. Either remove the comment 
completely or make it clear that you are changing to 4-byte for all 
further operations, not just the Read Any Register command.

> 
> >> +	nor->addr_width = 4;
> >> +
> >> +	/*
> >> +	 * The page_size is set to 512B from BFPT, but it actually depends on
> >> +	 * the configuration register. Look up the CFR3V and determine the
> >> +	 * page_size. For multi-die package parts, use 512B only when the all
> >> +	 * dies are configured to 512B buffer.
> >> +	 */
> >> +	for (addr = 0; addr < params->size; addr += SZ_128M) {
> >> +		ret = spansion_read_any_reg(nor, addr + SPINOR_REG_ADDR_CFR3V,
> >> +					    0, &cfr3v);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		if (!(cfr3v & CFR3V_PGMBUF)) {
> >> +			params->page_size = 256;
> >> +			return 0;
> >> +		}
> >> +	}
> >> +	params->page_size = 512;
> > 
> > Ok.
> > 
> >> +
> >> +	return 0;
> >> +}
> >> +

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Takahiro Kuwano]

On 3/8/2021 5:54 PM, Pratyush Yadav wrote:
> On 08/03/21 05:47PM, Takahiro Kuwano wrote:
>> On 2/24/2021 9:40 PM, Pratyush Yadav wrote:
>>> On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
>>>> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>>
>>>> This patch adds Flash specific fixups and hooks for Cypress
>>>> S25HL-T/S25HS-T family, based on the features introduced in [0][1][2].
>>>
>>> Instead of linking the patches like this, it would be a better idea to 
>>> simply include them in your series. This will make your series 
>>> independent from mine and will make it easier for the maintainer to 
>>> apply it.
>>>
>> Noted with thanks.
>>
>>>>
>>>> The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this
>>>> series are used for multi-die package parts.
>>>
>>> Nitpick: Once this patch makes it into the Git history there is no patch 
>>> #5 or #6. Probably a better idea to just say "introduced earlier" or 
>>> something similar. 
>>>
>> OK.
>>
>>>>
>>>> The nor->quad_enable() sets the volatile QE bit on each die.
>>>>
>>>> The mtd._erase() is hooked if the device is single-die package and not
>>>> configured to uniform sectors, assuming it is in factory default
>>>> configuration that has 32 x 4KB sectors overlaid on bottom address.
>>>> Other configurations, top and split, are not supported at this point.
>>>> Will submit additional patches to support it as needed.
>>>
>>> Ah, this patch does check for uniform sector map. Nice. I assume you 
>>> have tested with both configurations.
>>>
>> Yes. I tested both.
>>
>>>>
>>>> The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP.
>>>>
>>>> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav at ti.com/
>>>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav at ti.com/
>>>> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav at ti.com/
>>>>
>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>> ---
>>>> Changes in v5:
>>>>   - Add s25hx_t_erase_non_uniform()
>>>>   - Change mtd.writesize and mtd.flags in s25hx_t_setup()
>>>>   - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup()
>>>>
>>>>  drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++
>>>>  include/linux/mtd/spi-nor.h    |   3 +
>>>>  2 files changed, 158 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>>>> index 8d63681cb3..315e26ab27 100644
>>>> --- a/drivers/mtd/spi/spi-nor-core.c
>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
> [...]
>>>> +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)
>>>> +{
>>>> +	int ret;
>>>> +	u32 addr;
>>>> +	u8 cfr3v;
>>>> +
>>>> +	/* erase size in case it is set to 4K from BFPT */
>>>> +	nor->erase_opcode = SPINOR_OP_SE_4B;
>>>> +	nor->mtd.erasesize = nor->info->sector_size;
>>>> +
>>>> +	/* Enter 4-byte addressing mode for Read Any Register */
>>>> +	ret = set_4byte(nor, nor->info, 1);
>>>> +	if (ret)
>>>> +		return ret;
>>>
>>> You enter 4byte addressing but don't exit it before returning. Is this 
>>> intentional?
>>>
>> Yes. The nor->addr_width must be 4 for read/write/erase to work correctly.
>> I think device's addressing mode should be in sync with nor->addr_width.
> 
> Right. But the comment above implies that 4-byte mode is only enabled 
> for the "Read Any Register" command. Either remove the comment 
> completely or make it clear that you are changing to 4-byte for all 
> further operations, not just the Read Any Register command.
> 
Indeed. The comment is bad. Will fix.

Thanks,
Takahiro

[PATCH v5 04/10] mtd: spi-nor-core: Add support for volatile QE bit

[Takahiro Kuwano]

Hi Jagan,

On 2/26/2021 7:42 PM, Jagan Teki wrote:
> On Fri, Feb 19, 2021 at 7:26 AM <tkuw584924@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>
>> ---
>> Changes in v5:
>>   - Fix register address calculation, 'base | offset' -> 'base + offset'
>>
>>  drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h    |  1 +
>>  2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
>> index 2803536ed5..87c9fce408 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);
> 
> What if we can use the exiting quad_enable hook by identifying
> volatile QE at the function beginning instead of having a separate
> call?
> 
Do you mean something like this?

static int spansion_read_cr_quad_enable(struct spi_nor *nor)
{
	u8 sr_cr[2];
	int ret;

	if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
		u32 base;

		for (base = 0; base < nor->mtd.size; base += SZ_128M) {
			u32 addr = base + SPINOR_REG_ADDR_CFR1V;

			/* Check current Quad Enable bit value. */
			ret = spansion_read_any_reg(nor, addr, 0, &sr_cr[1]);

			[...]

			ret = spansion_write_any_reg(nor, addr, sr_cr[1]);

			[...]

			/* Read back and check it. */
			ret = spansion_read_any_reg(nor, addr, 0, &sr_cr[1]);

			[...]
		}

		return 0;
	}

	/* Check current Quad Enable bit value. */
	ret = read_cr(nor);
	if (ret < 0) {
		dev_dbg(nor->dev,
			"error while reading configuration register\n");
		return -EINVAL;
	}

	[...]
}	

Or defining a new flag like 'SNOR_F_HAS_VOLATILE_QE'?

> Jagan.
> 

Best Regards,
Takahiro

[PATCH v5 04/10] mtd: spi-nor-core: Add support for volatile QE bit

[Pratyush Yadav]

On 08/03/21 06:10PM, Takahiro Kuwano wrote:
> Hi Jagan,
> 
> On 2/26/2021 7:42 PM, Jagan Teki wrote:
> > On Fri, Feb 19, 2021 at 7:26 AM <tkuw584924@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>
> >> ---
> >> Changes in v5:
> >>   - Fix register address calculation, 'base | offset' -> 'base + offset'
> >>
> >>  drivers/mtd/spi/spi-nor-core.c | 53 ++++++++++++++++++++++++++++++++++
> >>  include/linux/mtd/spi-nor.h    |  1 +
> >>  2 files changed, 54 insertions(+)
> >>
> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> >> index 2803536ed5..87c9fce408 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);
> > 
> > What if we can use the exiting quad_enable hook by identifying
> > volatile QE at the function beginning instead of having a separate
> > call?
> > 
> Do you mean something like this?
> 
> static int spansion_read_cr_quad_enable(struct spi_nor *nor)
> {
> 	u8 sr_cr[2];
> 	int ret;
> 
> 	if (JEDEC_MFR(nor->info) == SNOR_MFR_CYPRESS) {
> 		u32 base;
> 
> 		for (base = 0; base < nor->mtd.size; base += SZ_128M) {
> 			u32 addr = base + SPINOR_REG_ADDR_CFR1V;
> 
> 			/* Check current Quad Enable bit value. */
> 			ret = spansion_read_any_reg(nor, addr, 0, &sr_cr[1]);
> 
> 			[...]
> 
> 			ret = spansion_write_any_reg(nor, addr, sr_cr[1]);
> 
> 			[...]
> 
> 			/* Read back and check it. */
> 			ret = spansion_read_any_reg(nor, addr, 0, &sr_cr[1]);
> 
> 			[...]
> 		}
> 
> 		return 0;
> 	}
> 
> 	/* Check current Quad Enable bit value. */
> 	ret = read_cr(nor);
> 	if (ret < 0) {
> 		dev_dbg(nor->dev,
> 			"error while reading configuration register\n");
> 		return -EINVAL;
> 	}
> 
> 	[...]
> }	
> 
> Or defining a new flag like 'SNOR_F_HAS_VOLATILE_QE'?

FWIW I think your current implementation is better than both these 
alternatives.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Takahiro Kuwano]

On 2/24/2021 9:43 PM, Pratyush Yadav wrote:
> On 19/02/21 10:56AM, 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.
> 
> As explained in v4 [0], 
> 
> Nacked-by: Pratyush Yadav <p.yadav@ti.com>
> 
> [0] https://patchwork.ozlabs.org/project/uboot/patch/a5c3cf1353d9a621379e2fcfefc51fb44c9680c5.1611729896.git.Takahiro.Kuwano at infineon.com/
> 
I will remove this in v6.