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

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

[tkuw584924]

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

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

Takahiro Kuwano (6):
  mtd: spi-nor: core: Add the ->ready() hook
  mtd: spi-nor: core: Expose spi_nor_clear_sr() to manufacturer drivers
  mtd: spi-nor: spansion: Add support for Read/Write Any  Register
  mtd: spi-nor: spansion: Add support for volatile QE bit
  mtd: spi-nor: spansion: Add status check for multi-die parts
  mtd: spi-nor: spansion: Add s25hl-t/s25hs-t IDs and fixups

 drivers/mtd/spi-nor/core.c     |   5 +-
 drivers/mtd/spi-nor/core.h     |   3 +
 drivers/mtd/spi-nor/spansion.c | 325 +++++++++++++++++++++++++++++++++
 3 files changed, 331 insertions(+), 2 deletions(-)

-- 
2.25.1


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

[PATCH v3 1/6] mtd: spi-nor: core: Add the ->ready() hook

[tkuw584924]

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

This hook can be used for SPI NOR flashes that do not support legacy
status read method.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v3:
  - New in v3
    The purpose is same as the patch introduced by Yaliang Wang.
    https://patchwork.ozlabs.org/project/linux-mtd/patch/20210301142844.1089385-1-yaliang.wang@windriver.com/  

 drivers/mtd/spi-nor/core.c | 3 ++-
 drivers/mtd/spi-nor/core.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0522304f52fa..c8a7f246ab7d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -826,7 +826,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
 		if (time_after_eq(jiffies, deadline))
 			timeout = 1;
 
-		ret = spi_nor_ready(nor);
+		ret = nor->params->ready(nor);
 		if (ret < 0)
 			return ret;
 		if (ret)
@@ -2920,6 +2920,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
 	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
 	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
 	params->setup = spi_nor_default_setup;
+	params->ready = spi_nor_ready;
 	/* Default to 16-bit Write Status (01h) Command */
 	nor->flags |= SNOR_F_HAS_16BIT_SR;
 
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 4a3f7f150b5d..4d06c27630fe 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -218,6 +218,7 @@ struct spi_nor_locking_ops {
  *                      flashes that have peculiarities to the SPI NOR standard
  *                      e.g. different opcodes, specific address calculation,
  *                      page size, etc.
+ * @ready:		checks if the SPI NOR flash is ready.
  * @locking_ops:	SPI NOR locking methods.
  */
 struct spi_nor_flash_parameter {
@@ -238,6 +239,7 @@ struct spi_nor_flash_parameter {
 	int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
 	u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
 	int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
+	int (*ready)(struct spi_nor *nor);
 
 	const struct spi_nor_locking_ops *locking_ops;
 };
-- 
2.25.1


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

Re: [PATCH v3 1/6] mtd: spi-nor: core: Add the ->ready() hook

[Pratyush Yadav]

Hi,

On 12/03/21 06:41PM, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> This hook can be used for SPI NOR flashes that do not support legacy
> status read method.

Nitpick: I wouldn't call it "legacy". There is no newer standard status 
read method. Something like "default" or "standard" would be better.

> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v3:
>   - New in v3
>     The purpose is same as the patch introduced by Yaliang Wang.
>     https://patchwork.ozlabs.org/project/linux-mtd/patch/20210301142844.1089385-1-yaliang.wang@windriver.com/  
> 
>  drivers/mtd/spi-nor/core.c | 3 ++-
>  drivers/mtd/spi-nor/core.h | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304f52fa..c8a7f246ab7d 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -826,7 +826,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
>  		if (time_after_eq(jiffies, deadline))
>  			timeout = 1;
>  
> -		ret = spi_nor_ready(nor);
> +		ret = nor->params->ready(nor);
>  		if (ret < 0)
>  			return ret;
>  		if (ret)
> @@ -2920,6 +2920,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>  	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
>  	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
>  	params->setup = spi_nor_default_setup;
> +	params->ready = spi_nor_ready;

Nitpick: Rename it to spi_nor_default_ready or something similar.

>  	/* Default to 16-bit Write Status (01h) Command */
>  	nor->flags |= SNOR_F_HAS_16BIT_SR;
>  
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 4a3f7f150b5d..4d06c27630fe 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -218,6 +218,7 @@ struct spi_nor_locking_ops {
>   *                      flashes that have peculiarities to the SPI NOR standard
>   *                      e.g. different opcodes, specific address calculation,
>   *                      page size, etc.
> + * @ready:		checks if the SPI NOR flash is ready.
>   * @locking_ops:	SPI NOR locking methods.
>   */
>  struct spi_nor_flash_parameter {
> @@ -238,6 +239,7 @@ struct spi_nor_flash_parameter {
>  	int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
>  	u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
>  	int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
> +	int (*ready)(struct spi_nor *nor);
>  
>  	const struct spi_nor_locking_ops *locking_ops;
>  };

Other than the above comments,

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[PATCH v3 2/6] mtd: spi-nor: core: Expose spi_nor_clear_sr() to manufacturer drivers

[tkuw584924]

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

spi_nor_clear_sr() needs to be called from manufacturer drivers that
implement the ->ready() hook.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v3:
  - New in v3
  
 drivers/mtd/spi-nor/core.c | 2 +-
 drivers/mtd/spi-nor/core.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index c8a7f246ab7d..3ca199bb83e3 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -653,7 +653,7 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
  * spi_nor_clear_sr() - Clear the Status Register.
  * @nor:	pointer to 'struct spi_nor'.
  */
-static void spi_nor_clear_sr(struct spi_nor *nor)
+void spi_nor_clear_sr(struct spi_nor *nor)
 {
 	int ret;
 
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 4d06c27630fe..596480aef924 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -447,6 +447,7 @@ int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
 int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
 int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
 
+void spi_nor_clear_sr(struct spi_nor *nor);
 int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
 ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 			  u8 *buf);
-- 
2.25.1


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

Re: [PATCH v3 2/6] mtd: spi-nor: core: Expose spi_nor_clear_sr() to manufacturer drivers

[Pratyush Yadav]

On 12/03/21 06:42PM, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> spi_nor_clear_sr() needs to be called from manufacturer drivers that
> implement the ->ready() hook.

I would expect flashes that need a custom method to read sr would also 
need a custom one to clear it. But maybe that is not the case.

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

> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v3:
>   - New in v3
>   
>  drivers/mtd/spi-nor/core.c | 2 +-
>  drivers/mtd/spi-nor/core.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c8a7f246ab7d..3ca199bb83e3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -653,7 +653,7 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
>   * spi_nor_clear_sr() - Clear the Status Register.
>   * @nor:	pointer to 'struct spi_nor'.
>   */
> -static void spi_nor_clear_sr(struct spi_nor *nor)
> +void spi_nor_clear_sr(struct spi_nor *nor)
>  {
>  	int ret;
>  
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 4d06c27630fe..596480aef924 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -447,6 +447,7 @@ int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
>  int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
>  int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
>  
> +void spi_nor_clear_sr(struct spi_nor *nor);
>  int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
>  ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>  			  u8 *buf);
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[PATCH v3 3/6] mtd: spi-nor: spansion: Add support for Read/Write Any Register

[tkuw584924]

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>
---
Changes in v3:
  - Cleanup implementation
  
 drivers/mtd/spi-nor/spansion.c | 102 +++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index b0c5521c1e27..1bce95cb7896 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -19,6 +19,108 @@
 #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
 #define SPINOR_OP_CYPRESS_RD_FAST		0xee
 
+/**
+ * spansion_read_any_reg() - Read Any Register.
+ * @nor:	pointer to a 'struct spi_nor'
+ * @reg_addr:	register address
+ * @reg_dummy:	number of dummy cycles for register read
+ * @reg_val:	pointer to a buffer where the register value is copied into
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_read_any_reg(struct spi_nor *nor, u32 reg_addr,
+				 u8 reg_dummy, u8 *reg_val)
+{
+	ssize_t ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 0),
+				   SPI_MEM_OP_ADDR(nor->addr_width, reg_addr, 0),
+				   SPI_MEM_OP_DUMMY((reg_dummy/8), 0),
+				   SPI_MEM_OP_DATA_IN(1, reg_val, 0));
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		enum spi_nor_protocol proto = nor->read_proto;
+		u8 opcode = nor->read_opcode;
+		u8 dummy = nor->read_dummy;
+
+		nor->read_opcode = SPINOR_OP_RD_ANY_REG;
+		nor->read_dummy = reg_dummy;
+		nor->read_proto = nor->reg_proto;
+
+		ret = nor->controller_ops->read(nor, reg_addr, 1, reg_val);
+
+		nor->read_opcode = opcode;
+		nor->read_dummy = dummy;
+		nor->read_proto = proto;
+
+		if (ret == 1)
+			return ret;
+		if (ret != 1)
+			return -EIO;
+
+		ret = 0;
+	}
+
+	return ret;
+}
+
+/**
+ * spansion_write_any_reg() - Write Any Register.
+ * @nor:	pointer to a 'struct spi_nor'
+ * @reg_addr:	register address
+ * @reg_val:	register value to be written
+ *
+ * Volatile register write will be effective immediately after the operation so
+ * this function does not poll the status.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
+{
+	ssize_t ret;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 0),
+				   SPI_MEM_OP_ADDR(nor->addr_width, reg_addr, 0),
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_OUT(1, &reg_val, 0));
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		enum spi_nor_protocol proto = nor->write_proto;
+		u8 opcode = nor->program_opcode;
+
+		nor->program_opcode = SPINOR_OP_WR_ANY_REG;
+		nor->write_proto = nor->reg_proto;
+
+		ret = nor->controller_ops->write(nor, reg_addr, 1, &reg_val);
+
+		nor->program_opcode = opcode;
+		nor->write_proto = proto;
+
+		if (ret < 0)
+			return ret;
+		if (ret != 1)
+			return -EIO;
+
+		ret = 0;
+	}
+
+	return ret;
+}
+
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
-- 
2.25.1


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

Re: [PATCH v3 3/6] mtd: spi-nor: spansion: Add support for Read/Write Any Register

[Pratyush Yadav]

On 12/03/21 06:44PM, tkuw584924@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>
> ---
> Changes in v3:
>   - Cleanup implementation
>   
>  drivers/mtd/spi-nor/spansion.c | 102 +++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index b0c5521c1e27..1bce95cb7896 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -19,6 +19,108 @@
>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
>  #define SPINOR_OP_CYPRESS_RD_FAST		0xee
>  
> +/**
> + * spansion_read_any_reg() - Read Any Register.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @reg_addr:	register address
> + * @reg_dummy:	number of dummy cycles for register read
> + * @reg_val:	pointer to a buffer where the register value is copied into
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_read_any_reg(struct spi_nor *nor, u32 reg_addr,
> +				 u8 reg_dummy, u8 *reg_val)
> +{
> +	ssize_t ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 0),
> +				   SPI_MEM_OP_ADDR(nor->addr_width, reg_addr, 0),
> +				   SPI_MEM_OP_DUMMY((reg_dummy/8), 0),

This is only correct when dummy buswidth is 1. It will lead to some very 
nasty and hard-to-catch bugs when someone uses it for a flash that has a 
different dummy buswidth. See how dummy nbytes are calculated in 
spi_nor_spimem_read_data().

> +				   SPI_MEM_OP_DATA_IN(1, reg_val, 0));
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		enum spi_nor_protocol proto = nor->read_proto;
> +		u8 opcode = nor->read_opcode;
> +		u8 dummy = nor->read_dummy;
> +
> +		nor->read_opcode = SPINOR_OP_RD_ANY_REG;
> +		nor->read_dummy = reg_dummy;
> +		nor->read_proto = nor->reg_proto;
> +
> +		ret = nor->controller_ops->read(nor, reg_addr, 1, reg_val);
> +
> +		nor->read_opcode = opcode;
> +		nor->read_dummy = dummy;
> +		nor->read_proto = proto;
> +
> +		if (ret == 1)
> +			return ret;
> +		if (ret != 1)
> +			return -EIO;
> +
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * spansion_write_any_reg() - Write Any Register.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @reg_addr:	register address
> + * @reg_val:	register value to be written
> + *
> + * Volatile register write will be effective immediately after the operation so
> + * this function does not poll the status.

The same opcode can be used to write to non-volatile registers as well, 
you just need to change the address passed. It is not recommended to 
write to non-volatile registers in SPI NOR though. Maybe add "(should be 
a volatile register)" after "register address" above?

As for not polling the status, can a volatile register write cause a 
program error? If so, you should check the status to know if any error 
bits were set.

> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
> +{
> +	ssize_t ret;
> +
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 0),
> +				   SPI_MEM_OP_ADDR(nor->addr_width, reg_addr, 0),
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_OUT(1, &reg_val, 0));
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		enum spi_nor_protocol proto = nor->write_proto;
> +		u8 opcode = nor->program_opcode;
> +
> +		nor->program_opcode = SPINOR_OP_WR_ANY_REG;
> +		nor->write_proto = nor->reg_proto;
> +
> +		ret = nor->controller_ops->write(nor, reg_addr, 1, &reg_val);
> +
> +		nor->program_opcode = opcode;
> +		nor->write_proto = proto;
> +
> +		if (ret < 0)
> +			return ret;
> +		if (ret != 1)
> +			return -EIO;
> +
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:		pointer to a 'struct spi_nor'
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH v3 3/6] mtd: spi-nor: spansion: Add support for Read/Write Any Register

[Takahiro Kuwano]

On 3/15/2021 8:27 PM, Pratyush Yadav wrote:
> On 12/03/21 06:44PM, tkuw584924@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>
>> ---
>> Changes in v3:
>>   - Cleanup implementation
>>   
>>  drivers/mtd/spi-nor/spansion.c | 102 +++++++++++++++++++++++++++++++++
>>  1 file changed, 102 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index b0c5521c1e27..1bce95cb7896 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -19,6 +19,108 @@
>>  #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
>>  #define SPINOR_OP_CYPRESS_RD_FAST		0xee
>>  
>> +/**
>> + * spansion_read_any_reg() - Read Any Register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @reg_addr:	register address
>> + * @reg_dummy:	number of dummy cycles for register read
>> + * @reg_val:	pointer to a buffer where the register value is copied into
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_read_any_reg(struct spi_nor *nor, u32 reg_addr,
>> +				 u8 reg_dummy, u8 *reg_val)
>> +{
>> +	ssize_t ret;
>> +
>> +	if (nor->spimem) {
>> +		struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 0),
>> +				   SPI_MEM_OP_ADDR(nor->addr_width, reg_addr, 0),
>> +				   SPI_MEM_OP_DUMMY((reg_dummy/8), 0),
> 
> This is only correct when dummy buswidth is 1. It will lead to some very 
> nasty and hard-to-catch bugs when someone uses it for a flash that has a 
> different dummy buswidth. See how dummy nbytes are calculated in 
> spi_nor_spimem_read_data().
> 
Yes, I will fix in v4.

>> +				   SPI_MEM_OP_DATA_IN(1, reg_val, 0));
>> +
>> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
>> +
>> +		ret = spi_mem_exec_op(nor->spimem, &op);
>> +	} else {
>> +		enum spi_nor_protocol proto = nor->read_proto;
>> +		u8 opcode = nor->read_opcode;
>> +		u8 dummy = nor->read_dummy;
>> +
>> +		nor->read_opcode = SPINOR_OP_RD_ANY_REG;
>> +		nor->read_dummy = reg_dummy;
>> +		nor->read_proto = nor->reg_proto;
>> +
>> +		ret = nor->controller_ops->read(nor, reg_addr, 1, reg_val);
>> +
>> +		nor->read_opcode = opcode;
>> +		nor->read_dummy = dummy;
>> +		nor->read_proto = proto;
>> +
>> +		if (ret == 1)
>> +			return ret;
>> +		if (ret != 1)
>> +			return -EIO;
>> +
>> +		ret = 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * spansion_write_any_reg() - Write Any Register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @reg_addr:	register address
>> + * @reg_val:	register value to be written
>> + *
>> + * Volatile register write will be effective immediately after the operation so
>> + * this function does not poll the status.
> 
> The same opcode can be used to write to non-volatile registers as well, 
> you just need to change the address passed. It is not recommended to 
> write to non-volatile registers in SPI NOR though. Maybe add "(should be 
> a volatile register)" after "register address" above?
> 
Yes, it is better to add such comment.

> As for not polling the status, can a volatile register write cause a 
> program error? If so, you should check the status to know if any error 
> bits were set.
> 
No, writing to volatile registers does not cause a program error.

Best Regards,
Takahiro


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

[PATCH v3 4/6] mtd: spi-nor: spansion: Add support for volatile QE bit

[tkuw584924]

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. The function supports multi-die package parts that require to
set the Quad Enable bit in each die.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v3:
  - Add multi-die package parts support

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

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 1bce95cb7896..b5b5df4836c6 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -10,6 +10,8 @@
 
 #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
 #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
+#define SPINOR_REG_CYPRESS_CFR1V		0x00800002
+#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
 #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
 #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24	0xb
 #define SPINOR_REG_CYPRESS_CFR3V		0x00800004
@@ -121,6 +123,62 @@ static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
 	return ret;
 }
 
+/**
+ * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
+ * @nor:	pointer to a 'struct spi_nor'
+ * @reg_dummy:	number of dummy cycles for register read
+ * @die_size:	size of each die to determine the number of dies
+ *
+ * It is recommended to update volatile registers in the field application due
+ * to a risk of the non-volatile registers corruption by power interrupt. This
+ * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
+ * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
+ * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
+ * also set during Flash power-up. This function supports multi-die package
+ * parts that require to set the Quad Enable bit in each die.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy,
+					 u32 die_size)
+{
+	int ret;
+	u32 base, reg_addr;
+	u8 cfr1v, cfr1v_written;
+
+	for (base = 0; base < nor->params->size; base += die_size) {
+		reg_addr = base + SPINOR_REG_CYPRESS_CFR1V;
+
+		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
+		if (ret)
+			return ret;
+
+		if (cfr1v & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
+			continue;
+
+		/* Update the Quad Enable bit. */
+		cfr1v |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
+
+		ret = spansion_write_any_reg(nor, reg_addr, cfr1v);
+		if (ret)
+			return ret;
+
+		cfr1v_written = cfr1v;
+
+		/* Read back and check it. */
+		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
+		if (ret)
+			return ret;
+
+		if (cfr1v != cfr1v_written) {
+			dev_err(nor->dev, "CFR1: Read back test failed\n");
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
-- 
2.25.1


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

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

[Pratyush Yadav]

On 12/03/21 06:44PM, 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. The function supports multi-die package parts that require to
> set the Quad Enable bit in each die.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v3:
>   - Add multi-die package parts support
> 
>  drivers/mtd/spi-nor/spansion.c | 58 ++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 1bce95cb7896..b5b5df4836c6 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -10,6 +10,8 @@
>  
>  #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
>  #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
> +#define SPINOR_REG_CYPRESS_CFR1V		0x00800002
> +#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
>  #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
>  #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24	0xb
>  #define SPINOR_REG_CYPRESS_CFR3V		0x00800004
> @@ -121,6 +123,62 @@ static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
>  	return ret;
>  }
>  
> +/**
> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
> + * @nor:	pointer to a 'struct spi_nor'
> + * @reg_dummy:	number of dummy cycles for register read
> + * @die_size:	size of each die to determine the number of dies
> + *
> + * It is recommended to update volatile registers in the field application due
> + * to a risk of the non-volatile registers corruption by power interrupt. This
> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
> + * also set during Flash power-up. This function supports multi-die package
> + * parts that require to set the Quad Enable bit in each die.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy,
> +					 u32 die_size)
> +{
> +	int ret;
> +	u32 base, reg_addr;
> +	u8 cfr1v, cfr1v_written;
> +
> +	for (base = 0; base < nor->params->size; base += die_size) {
> +		reg_addr = base + SPINOR_REG_CYPRESS_CFR1V;
> +
> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);

I didn't notice it when reviewing the U-Boot series. How does register 
read work here? This will be issued in 1-1-4 mode since the 
nor->read_proto should be set to that protocol. But the flash is still 
in 1-1-1 mode. So the flash will output data on 1 line and the 
controller will read it on 4 lines, giving us a bogus register value. In 
fact I see this with pretty much every quad_enable() hook. What am I 
missing?

> +		if (ret)
> +			return ret;
> +
> +		if (cfr1v & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
> +			continue;
> +
> +		/* Update the Quad Enable bit. */
> +		cfr1v |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
> +
> +		ret = spansion_write_any_reg(nor, reg_addr, cfr1v);
> +		if (ret)
> +			return ret;
> +
> +		cfr1v_written = cfr1v;
> +
> +		/* Read back and check it. */
> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
> +		if (ret)
> +			return ret;
> +
> +		if (cfr1v != cfr1v_written) {
> +			dev_err(nor->dev, "CFR1: Read back test failed\n");
> +			return -EIO;
> +		}
> +	}
> +
> +	return 0;
> +}
> +

Rest of the patch looks good.

>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:		pointer to a 'struct spi_nor'
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

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

[Takahiro Kuwano]

On 3/15/2021 8:47 PM, Pratyush Yadav wrote:
> On 12/03/21 06:44PM, 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. The function supports multi-die package parts that require to
>> set the Quad Enable bit in each die.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v3:
>>   - Add multi-die package parts support
>>
>>  drivers/mtd/spi-nor/spansion.c | 58 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 58 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 1bce95cb7896..b5b5df4836c6 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -10,6 +10,8 @@
>>  
>>  #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
>>  #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
>> +#define SPINOR_REG_CYPRESS_CFR1V		0x00800002
>> +#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
>>  #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
>>  #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24	0xb
>>  #define SPINOR_REG_CYPRESS_CFR3V		0x00800004
>> @@ -121,6 +123,62 @@ static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
>>  	return ret;
>>  }
>>  
>> +/**
>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
>> + * @nor:	pointer to a 'struct spi_nor'
>> + * @reg_dummy:	number of dummy cycles for register read
>> + * @die_size:	size of each die to determine the number of dies
>> + *
>> + * It is recommended to update volatile registers in the field application due
>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
>> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
>> + * also set during Flash power-up. This function supports multi-die package
>> + * parts that require to set the Quad Enable bit in each die.
>> + *
>> + * Return: 0 on success, -errno otherwise.
>> + */
>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy,
>> +					 u32 die_size)
>> +{
>> +	int ret;
>> +	u32 base, reg_addr;
>> +	u8 cfr1v, cfr1v_written;
>> +
>> +	for (base = 0; base < nor->params->size; base += die_size) {
>> +		reg_addr = base + SPINOR_REG_CYPRESS_CFR1V;
>> +
>> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
> 
> I didn't notice it when reviewing the U-Boot series. How does register 
> read work here? This will be issued in 1-1-4 mode since the 
> nor->read_proto should be set to that protocol. But the flash is still 
> in 1-1-1 mode. So the flash will output data on 1 line and the 
> controller will read it on 4 lines, giving us a bogus register value. In 
> fact I see this with pretty much every quad_enable() hook. What am I 
> missing?
> 
The nor->reg_proto is used for register access and it is 1-1-1 at this
point.

>> +		if (ret)
>> +			return ret;
>> +
>> +		if (cfr1v & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
>> +			continue;
>> +
>> +		/* Update the Quad Enable bit. */
>> +		cfr1v |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
>> +
>> +		ret = spansion_write_any_reg(nor, reg_addr, cfr1v);
>> +		if (ret)
>> +			return ret;
>> +
>> +		cfr1v_written = cfr1v;
>> +
>> +		/* Read back and check it. */
>> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (cfr1v != cfr1v_written) {
>> +			dev_err(nor->dev, "CFR1: Read back test failed\n");
>> +			return -EIO;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> 
> Rest of the patch looks good.
> 
>>  /**
>>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:		pointer to a 'struct spi_nor'
>> -- 
>> 2.25.1
>>
> 

Best Regards,
Takahiro

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

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

[Pratyush Yadav]

On 18/03/21 05:00PM, Takahiro Kuwano wrote:
> On 3/15/2021 8:47 PM, Pratyush Yadav wrote:
> > On 12/03/21 06:44PM, 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. The function supports multi-die package parts that require to
> >> set the Quad Enable bit in each die.
> >>
> >> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> >> ---
> >> Changes in v3:
> >>   - Add multi-die package parts support
> >>
> >>  drivers/mtd/spi-nor/spansion.c | 58 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 58 insertions(+)
> >>
> >> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> >> index 1bce95cb7896..b5b5df4836c6 100644
> >> --- a/drivers/mtd/spi-nor/spansion.c
> >> +++ b/drivers/mtd/spi-nor/spansion.c
> >> @@ -10,6 +10,8 @@
> >>  
> >>  #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
> >>  #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
> >> +#define SPINOR_REG_CYPRESS_CFR1V		0x00800002
> >> +#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
> >>  #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
> >>  #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24	0xb
> >>  #define SPINOR_REG_CYPRESS_CFR3V		0x00800004
> >> @@ -121,6 +123,62 @@ static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
> >>  	return ret;
> >>  }
> >>  
> >> +/**
> >> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
> >> + * @nor:	pointer to a 'struct spi_nor'
> >> + * @reg_dummy:	number of dummy cycles for register read
> >> + * @die_size:	size of each die to determine the number of dies
> >> + *
> >> + * It is recommended to update volatile registers in the field application due
> >> + * to a risk of the non-volatile registers corruption by power interrupt. This
> >> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
> >> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
> >> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
> >> + * also set during Flash power-up. This function supports multi-die package
> >> + * parts that require to set the Quad Enable bit in each die.
> >> + *
> >> + * Return: 0 on success, -errno otherwise.
> >> + */
> >> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy,
> >> +					 u32 die_size)
> >> +{
> >> +	int ret;
> >> +	u32 base, reg_addr;
> >> +	u8 cfr1v, cfr1v_written;
> >> +
> >> +	for (base = 0; base < nor->params->size; base += die_size) {
> >> +		reg_addr = base + SPINOR_REG_CYPRESS_CFR1V;
> >> +
> >> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
> > 
> > I didn't notice it when reviewing the U-Boot series. How does register 
> > read work here? This will be issued in 1-1-4 mode since the 
> > nor->read_proto should be set to that protocol. But the flash is still 
> > in 1-1-1 mode. So the flash will output data on 1 line and the 
> > controller will read it on 4 lines, giving us a bogus register value. In 
> > fact I see this with pretty much every quad_enable() hook. What am I 
> > missing?
> > 
> The nor->reg_proto is used for register access and it is 1-1-1 at this
> point.

Ah, right. Also...

> 
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		if (cfr1v & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
> >> +			continue;
> >> +
> >> +		/* Update the Quad Enable bit. */
> >> +		cfr1v |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
> >> +
> >> +		ret = spansion_write_any_reg(nor, reg_addr, cfr1v);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		cfr1v_written = cfr1v;
> >> +
> >> +		/* Read back and check it. */
> >> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
> >> +		if (ret)
> >> +			return ret;

... this would still send data in 1-1-1 even after quad mode being 
enabled, right? If so,

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

> >> +
> >> +		if (cfr1v != cfr1v_written) {
> >> +			dev_err(nor->dev, "CFR1: Read back test failed\n");
> >> +			return -EIO;
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> > 
> > Rest of the patch looks good.
> > 
> >>  /**
> >>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
> >>   * @nor:		pointer to a 'struct spi_nor'
> >> -- 
> >> 2.25.1
> >>
> > 
> 
> Best Regards,
> Takahiro

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

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

[Takahiro Kuwano]

On 3/18/2021 5:19 PM, Pratyush Yadav wrote:
> On 18/03/21 05:00PM, Takahiro Kuwano wrote:
>> On 3/15/2021 8:47 PM, Pratyush Yadav wrote:
>>> On 12/03/21 06:44PM, 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. The function supports multi-die package parts that require to
>>>> set the Quad Enable bit in each die.
>>>>
>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>>>> ---
>>>> Changes in v3:
>>>>   - Add multi-die package parts support
>>>>
>>>>  drivers/mtd/spi-nor/spansion.c | 58 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 58 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>>>> index 1bce95cb7896..b5b5df4836c6 100644
>>>> --- a/drivers/mtd/spi-nor/spansion.c
>>>> +++ b/drivers/mtd/spi-nor/spansion.c
>>>> @@ -10,6 +10,8 @@
>>>>  
>>>>  #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
>>>>  #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
>>>> +#define SPINOR_REG_CYPRESS_CFR1V		0x00800002
>>>> +#define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
>>>>  #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
>>>>  #define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24	0xb
>>>>  #define SPINOR_REG_CYPRESS_CFR3V		0x00800004
>>>> @@ -121,6 +123,62 @@ static int spansion_write_any_reg(struct spi_nor *nor, u32 reg_addr, u8 reg_val)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +/**
>>>> + * spansion_quad_enable_volatile() - enable Quad I/O mode in volatile register.
>>>> + * @nor:	pointer to a 'struct spi_nor'
>>>> + * @reg_dummy:	number of dummy cycles for register read
>>>> + * @die_size:	size of each die to determine the number of dies
>>>> + *
>>>> + * It is recommended to update volatile registers in the field application due
>>>> + * to a risk of the non-volatile registers corruption by power interrupt. This
>>>> + * function sets Quad Enable bit in CFR1 volatile. If users set the Quad Enable
>>>> + * bit in the CFR1 non-volatile in advance (typically by a Flash programmer
>>>> + * before mounting Flash on PCB), the Quad Enable bit in the CFR1 volatile is
>>>> + * also set during Flash power-up. This function supports multi-die package
>>>> + * parts that require to set the Quad Enable bit in each die.
>>>> + *
>>>> + * Return: 0 on success, -errno otherwise.
>>>> + */
>>>> +static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy,
>>>> +					 u32 die_size)
>>>> +{
>>>> +	int ret;
>>>> +	u32 base, reg_addr;
>>>> +	u8 cfr1v, cfr1v_written;
>>>> +
>>>> +	for (base = 0; base < nor->params->size; base += die_size) {
>>>> +		reg_addr = base + SPINOR_REG_CYPRESS_CFR1V;
>>>> +
>>>> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
>>>
>>> I didn't notice it when reviewing the U-Boot series. How does register 
>>> read work here? This will be issued in 1-1-4 mode since the 
>>> nor->read_proto should be set to that protocol. But the flash is still 
>>> in 1-1-1 mode. So the flash will output data on 1 line and the 
>>> controller will read it on 4 lines, giving us a bogus register value. In 
>>> fact I see this with pretty much every quad_enable() hook. What am I 
>>> missing?
>>>
>> The nor->reg_proto is used for register access and it is 1-1-1 at this
>> point.
> 
> Ah, right. Also...
> 
>>
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		if (cfr1v & SPINOR_REG_CYPRESS_CFR1V_QUAD_EN)
>>>> +			continue;
>>>> +
>>>> +		/* Update the Quad Enable bit. */
>>>> +		cfr1v |= SPINOR_REG_CYPRESS_CFR1V_QUAD_EN;
>>>> +
>>>> +		ret = spansion_write_any_reg(nor, reg_addr, cfr1v);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		cfr1v_written = cfr1v;
>>>> +
>>>> +		/* Read back and check it. */
>>>> +		ret = spansion_read_any_reg(nor, reg_addr, reg_dummy, &cfr1v);
>>>> +		if (ret)
>>>> +			return ret;
> 
> ... this would still send data in 1-1-1 even after quad mode being 
> enabled, right? If so,
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 
Yes, the Flash works in 1-1-1 for register access even if quad mode
is enabled.

Best Regards,
Takahiro

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

[PATCH v3 5/6] mtd: spi-nor: spansion: Add status check for multi-die parts

[tkuw584924]

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

The multi-die package parts from Spansion/Cypress require to check
the status register value in each die to detemine the device is
ready. This patch adds a helper that queries status of each die by
Read Any Register command. The device specific ->ready() hook will
call this helper with the number of dummy cycles and die size.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v3:
  - New in v3

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

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index b5b5df4836c6..5742212fde2e 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -10,6 +10,7 @@
 
 #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
 #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
+#define SPINOR_REG_CYPRESS_STR1V		0x00800000
 #define SPINOR_REG_CYPRESS_CFR1V		0x00800002
 #define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
 #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
@@ -179,6 +180,51 @@ static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy,
 	return 0;
 }
 
+/**
+ * spansion_mdp_ready() - Query the Status Register via Read Any Register
+ *                        command for multi-die package parts that do not
+ *                        support legacy RDSR(05h)
+ * @nor:	pointer to 'struct spi_nor'.
+ * @reg_dummy:	number of dummy cycles for register read
+ * @die_size:	size of each die to determine the number of dies
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spansion_mdp_ready(struct spi_nor *nor, u8 reg_dummy, u32 die_size)
+{
+	int ret;
+	u32 base;
+	u8 sr;
+
+	for (base = 0; base < nor->params->size; base += die_size) {
+		ret = spansion_read_any_reg(nor,
+					    base + SPINOR_REG_CYPRESS_STR1V,
+					    reg_dummy, &sr);
+		if (ret)
+			return ret;
+
+		if (sr & (SR_E_ERR | SR_P_ERR)) {
+			if (sr & SR_E_ERR)
+				dev_err(nor->dev, "Erase Error occurred\n");
+			else
+				dev_err(nor->dev, "Programming Error occurred\n");
+
+			spi_nor_clear_sr(nor);
+
+			ret = spi_nor_write_disable(nor);
+			if (ret)
+				return ret;
+
+			return -EIO;
+		}
+
+		if (sr & SR_WIP)
+			return 0;
+	}
+
+	return 1;
+}
+
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
-- 
2.25.1


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

Re: [PATCH v3 5/6] mtd: spi-nor: spansion: Add status check for multi-die parts

[Pratyush Yadav]

On 12/03/21 06:44PM, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> The multi-die package parts from Spansion/Cypress require to check
> the status register value in each die to detemine the device is
> ready. This patch adds a helper that queries status of each die by
> Read Any Register command. The device specific ->ready() hook will
> call this helper with the number of dummy cycles and die size.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v3:
>   - New in v3
> 
>  drivers/mtd/spi-nor/spansion.c | 46 ++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index b5b5df4836c6..5742212fde2e 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -10,6 +10,7 @@
>  
>  #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
>  #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
> +#define SPINOR_REG_CYPRESS_STR1V		0x00800000
>  #define SPINOR_REG_CYPRESS_CFR1V		0x00800002
>  #define SPINOR_REG_CYPRESS_CFR1V_QUAD_EN	BIT(1)	/* Quad Enable */
>  #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
> @@ -179,6 +180,51 @@ static int spansion_quad_enable_volatile(struct spi_nor *nor, u8 reg_dummy,
>  	return 0;
>  }
>  
> +/**
> + * spansion_mdp_ready() - Query the Status Register via Read Any Register
> + *                        command for multi-die package parts that do not
> + *                        support legacy RDSR(05h)

Same comment about the use of "legacy".

> + * @nor:	pointer to 'struct spi_nor'.
> + * @reg_dummy:	number of dummy cycles for register read
> + * @die_size:	size of each die to determine the number of dies
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spansion_mdp_ready(struct spi_nor *nor, u8 reg_dummy, u32 die_size)
> +{
> +	int ret;
> +	u32 base;
> +	u8 sr;
> +
> +	for (base = 0; base < nor->params->size; base += die_size) {
> +		ret = spansion_read_any_reg(nor,
> +					    base + SPINOR_REG_CYPRESS_STR1V,
> +					    reg_dummy, &sr);
> +		if (ret)
> +			return ret;
> +
> +		if (sr & (SR_E_ERR | SR_P_ERR)) {
> +			if (sr & SR_E_ERR)
> +				dev_err(nor->dev, "Erase Error occurred\n");
> +			else
> +				dev_err(nor->dev, "Programming Error occurred\n");
> +
> +			spi_nor_clear_sr(nor);

This will clear the error flags on all dies. Ok.

> +
> +			ret = spi_nor_write_disable(nor);
> +			if (ret)
> +				return ret;
> +
> +			return -EIO;
> +		}
> +
> +		if (sr & SR_WIP)
> +			return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:		pointer to a 'struct spi_nor'

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

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

[tkuw584924]

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

The S25HL-T/S25HS-T family is the Cypress Semper Flash with Quad SPI.

For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
uniform sector sizes are supported. For the multi-die package parts (2Gb),
only uniform sector sizes is supprted. This is due to missing or incorrect
entries in SMPT. Fixup for other sector sizes configurations will be
followed up as needed.

Tested on Xilinx Zynq-7000 FPGA board.

Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
---
Changes in v3:
  - Remove S25HL256T and S25HS256T
  - Add S25HL02GT and S25HS02GT 
  - Add support for multi-die package parts support
  - Remove erase_map fix for top/split sector layout
  - Set ECC data unit size (16B) to writesize 

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

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 5742212fde2e..05a4ea00acfc 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -225,6 +225,107 @@ static int spansion_mdp_ready(struct spi_nor *nor, u8 reg_dummy, u32 die_size)
 	return 1;
 }
 
+static int s25hx_t_quad_enable(struct spi_nor *nor)
+{
+	return spansion_quad_enable_volatile(nor, 0, SZ_128M);
+}
+
+static int s25hx_t_mdp_ready(struct spi_nor *nor)
+{
+	return spansion_mdp_ready(nor, 0, SZ_128M);
+}
+
+static int
+s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
+			 const struct sfdp_parameter_header *bfpt_header,
+			 const struct sfdp_bfpt *bfpt,
+			 struct spi_nor_flash_parameter *params)
+{
+	int ret;
+	u32 addr;
+	u8 cfr3v;
+
+	ret = spi_nor_set_4byte_addr_mode(nor, true);
+	if (ret)
+		return ret;
+	nor->addr_width = 4;
+
+	/* Replace Quad Enable with volatile version */
+	params->quad_enable = s25hx_t_quad_enable;
+
+	/*
+	 * 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_CYPRESS_CFR3V, 0,
+					    &cfr3v);
+		if (ret)
+			return ret;
+
+		if (!(cfr3v & SPINOR_REG_CYPRESS_CFR3V_PGSZ)) {
+			params->page_size = 256;
+			return 0;
+		}
+	}
+	params->page_size = 512;
+
+	return 0;
+}
+
+void s25hx_t_post_sfdp_fixups(struct spi_nor *nor)
+{
+	/*
+	 * For the signle-die package parts (512Mb and 1Gb), bottom 4KB and
+	 * uniform sector maps are correctly populated in the erase_map
+	 * structure. The table below shows all possible combinations of related
+	 * register bits and its availability in SMPT.
+	 *
+	 *   CFR3[3] | CFR1[6] | CFR1[2] | Sector Map | Available in SMPT?
+	 *  -------------------------------------------------------------------
+	 *      0    |    0    |    0    | Bottom     | YES
+	 *      0    |    0    |    1    | Top        | NO (decoded as Split)
+	 *      0    |    1    |    0    | Split      | NO
+	 *      0    |    1    |    1    | Split      | NO (decoded as Top)
+	 *      1    |    0    |    0    | Uniform    | YES
+	 *      1    |    0    |    1    | Uniform    | NO
+	 *      1    |    1    |    0    | Uniform    | NO
+	 *      1    |    1    |    1    | Uniform    | NO
+	 *  -------------------------------------------------------------------
+	 */
+
+	/* Fast Read 4B requires mode cycles */
+	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
+
+	/* The writesize should be ECC data unit size */
+	nor->params->writesize = 16;
+
+	/* Fixup for multi-die package parts */
+	if (nor->params->size > SZ_128M) {
+		/*
+		 * For the dual-die package parts (2Gb), SMPT parse fails due to
+		 * incorrect SMPT entries and the erase map is populated as 4K
+		 * uniform that does not supported the parts. So it needs to be
+		 * rolled back to 256K uniform that is the factory default of
+		 * multi-die package parts.
+		 */
+		spi_nor_init_uniform_erase_map(&nor->params->erase_map,
+					       BIT(SNOR_ERASE_TYPE_MAX - 1),
+					       nor->params->size);
+
+		/* Need to check status of each die via RDAR command */
+		nor->params->ready = s25hx_t_mdp_ready;
+	}
+}
+
+static struct spi_nor_fixups s25hx_t_fixups = {
+	.post_bfpt = s25hx_t_post_bfpt_fixups,
+	.post_sfdp = s25hx_t_post_sfdp_fixups
+};
+
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
@@ -475,6 +576,24 @@ static const struct flash_info spansion_parts[] = {
 	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			     SPI_NOR_4B_OPCODES) },
+	{ "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hl02gt",  INFO6(0x342a1c, 0x0f0090, 256 * 1024, 1024,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
+	{ "s25hs02gt",  INFO6(0x342b1c, 0x0f0090, 256 * 1024, 1024,
+			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
+	  .fixups = &s25hx_t_fixups },
 	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
 			      SPI_NOR_NO_ERASE) },
 	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,
-- 
2.25.1


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

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

[Pratyush Yadav]

On 12/03/21 06:45PM, 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.
> 
> For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
> uniform sector sizes are supported. For the multi-die package parts (2Gb),
> only uniform sector sizes is supprted. This is due to missing or incorrect
> entries in SMPT. Fixup for other sector sizes configurations will be
> followed up as needed.
> 
> Tested on Xilinx Zynq-7000 FPGA board.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v3:
>   - Remove S25HL256T and S25HS256T
>   - Add S25HL02GT and S25HS02GT 
>   - Add support for multi-die package parts support
>   - Remove erase_map fix for top/split sector layout
>   - Set ECC data unit size (16B) to writesize 
> 
>  drivers/mtd/spi-nor/spansion.c | 119 +++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 5742212fde2e..05a4ea00acfc 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -225,6 +225,107 @@ static int spansion_mdp_ready(struct spi_nor *nor, u8 reg_dummy, u32 die_size)
>  	return 1;
>  }
>  
> +static int s25hx_t_quad_enable(struct spi_nor *nor)
> +{
> +	return spansion_quad_enable_volatile(nor, 0, SZ_128M);
> +}
> +
> +static int s25hx_t_mdp_ready(struct spi_nor *nor)
> +{
> +	return spansion_mdp_ready(nor, 0, SZ_128M);
> +}
> +
> +static int
> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
> +			 const struct sfdp_parameter_header *bfpt_header,
> +			 const struct sfdp_bfpt *bfpt,
> +			 struct spi_nor_flash_parameter *params)
> +{
> +	int ret;
> +	u32 addr;
> +	u8 cfr3v;
> +
> +	ret = spi_nor_set_4byte_addr_mode(nor, true);
> +	if (ret)
> +		return ret;
> +	nor->addr_width = 4;
> +
> +	/* Replace Quad Enable with volatile version */
> +	params->quad_enable = s25hx_t_quad_enable;
> +
> +	/*
> +	 * 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_CYPRESS_CFR3V, 0,
> +					    &cfr3v);
> +		if (ret)
> +			return ret;
> +
> +		if (!(cfr3v & SPINOR_REG_CYPRESS_CFR3V_PGSZ)) {
> +			params->page_size = 256;
> +			return 0;
> +		}
> +	}
> +	params->page_size = 512;
> +
> +	return 0;
> +}
> +
> +void s25hx_t_post_sfdp_fixups(struct spi_nor *nor)
> +{
> +	/*
> +	 * For the signle-die package parts (512Mb and 1Gb), bottom 4KB and

Typo. s/signle/single/

> +	 * uniform sector maps are correctly populated in the erase_map
> +	 * structure. The table below shows all possible combinations of related
> +	 * register bits and its availability in SMPT.
> +	 *
> +	 *   CFR3[3] | CFR1[6] | CFR1[2] | Sector Map | Available in SMPT?
> +	 *  -------------------------------------------------------------------
> +	 *      0    |    0    |    0    | Bottom     | YES
> +	 *      0    |    0    |    1    | Top        | NO (decoded as Split)
> +	 *      0    |    1    |    0    | Split      | NO
> +	 *      0    |    1    |    1    | Split      | NO (decoded as Top)
> +	 *      1    |    0    |    0    | Uniform    | YES
> +	 *      1    |    0    |    1    | Uniform    | NO
> +	 *      1    |    1    |    0    | Uniform    | NO
> +	 *      1    |    1    |    1    | Uniform    | NO
> +	 *  -------------------------------------------------------------------

Neat! This would be very helpful for anyone looking at this flash's SMPT 
table in the future.

> +	 */

Nitpick: It would be good to merge this

> +
> +	/* Fast Read 4B requires mode cycles */
> +	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
> +
> +	/* The writesize should be ECC data unit size */
> +	nor->params->writesize = 16;
> +
> +	/* Fixup for multi-die package parts */
> +	if (nor->params->size > SZ_128M) {
> +		/*
> +		 * For the dual-die package parts (2Gb), SMPT parse fails due to
> +		 * incorrect SMPT entries and the erase map is populated as 4K
> +		 * uniform that does not supported the parts. So it needs to be
> +		 * rolled back to 256K uniform that is the factory default of
> +		 * multi-die package parts.
> +		 */

... and this comment. If the table can't fit due to indentation, at 
least move it to above the "if" so it is at least somewhat close to 
where the SMPT is actually being fixed up.

> +		spi_nor_init_uniform_erase_map(&nor->params->erase_map,
> +					       BIT(SNOR_ERASE_TYPE_MAX - 1),
> +					       nor->params->size);
> +
> +		/* Need to check status of each die via RDAR command */
> +		nor->params->ready = s25hx_t_mdp_ready;
> +	}
> +}
> +
> +static struct spi_nor_fixups s25hx_t_fixups = {
> +	.post_bfpt = s25hx_t_post_bfpt_fixups,
> +	.post_sfdp = s25hx_t_post_sfdp_fixups
> +};
> +
>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:		pointer to a 'struct spi_nor'
> @@ -475,6 +576,24 @@ static const struct flash_info spansion_parts[] = {
>  	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>  			     SPI_NOR_4B_OPCODES) },
> +	{ "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hl02gt",  INFO6(0x342a1c, 0x0f0090, 256 * 1024, 1024,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hs02gt",  INFO6(0x342b1c, 0x0f0090, 256 * 1024, 1024,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
>  	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
>  			      SPI_NOR_NO_ERASE) },
>  	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,

Other than the comments above,

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

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

[Takahiro Kuwano]

Hi Pratyush,

I will fix the typo and merge the comment blocks.

On 3/15/2021 9:15 PM, Pratyush Yadav wrote:
> On 12/03/21 06:45PM, 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.
>>
>> For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
>> uniform sector sizes are supported. For the multi-die package parts (2Gb),
>> only uniform sector sizes is supprted. This is due to missing or incorrect
>> entries in SMPT. Fixup for other sector sizes configurations will be
>> followed up as needed.
>>
>> Tested on Xilinx Zynq-7000 FPGA board.
>>
>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>> ---
>> Changes in v3:
>>   - Remove S25HL256T and S25HS256T
>>   - Add S25HL02GT and S25HS02GT 
>>   - Add support for multi-die package parts support
>>   - Remove erase_map fix for top/split sector layout
>>   - Set ECC data unit size (16B) to writesize 
>>
>>  drivers/mtd/spi-nor/spansion.c | 119 +++++++++++++++++++++++++++++++++
>>  1 file changed, 119 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
>> index 5742212fde2e..05a4ea00acfc 100644
>> --- a/drivers/mtd/spi-nor/spansion.c
>> +++ b/drivers/mtd/spi-nor/spansion.c
>> @@ -225,6 +225,107 @@ static int spansion_mdp_ready(struct spi_nor *nor, u8 reg_dummy, u32 die_size)
>>  	return 1;
>>  }
>>  
>> +static int s25hx_t_quad_enable(struct spi_nor *nor)
>> +{
>> +	return spansion_quad_enable_volatile(nor, 0, SZ_128M);
>> +}
>> +
>> +static int s25hx_t_mdp_ready(struct spi_nor *nor)
>> +{
>> +	return spansion_mdp_ready(nor, 0, SZ_128M);
>> +}
>> +
>> +static int
>> +s25hx_t_post_bfpt_fixups(struct spi_nor *nor,
>> +			 const struct sfdp_parameter_header *bfpt_header,
>> +			 const struct sfdp_bfpt *bfpt,
>> +			 struct spi_nor_flash_parameter *params)
>> +{
>> +	int ret;
>> +	u32 addr;
>> +	u8 cfr3v;
>> +
>> +	ret = spi_nor_set_4byte_addr_mode(nor, true);
>> +	if (ret)
>> +		return ret;
>> +	nor->addr_width = 4;
>> +
>> +	/* Replace Quad Enable with volatile version */
>> +	params->quad_enable = s25hx_t_quad_enable;
>> +
>> +	/*
>> +	 * 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_CYPRESS_CFR3V, 0,
>> +					    &cfr3v);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (!(cfr3v & SPINOR_REG_CYPRESS_CFR3V_PGSZ)) {
>> +			params->page_size = 256;
>> +			return 0;
>> +		}
>> +	}
>> +	params->page_size = 512;
>> +
>> +	return 0;
>> +}
>> +
>> +void s25hx_t_post_sfdp_fixups(struct spi_nor *nor)
>> +{
>> +	/*
>> +	 * For the signle-die package parts (512Mb and 1Gb), bottom 4KB and
> 
> Typo. s/signle/single/
> 
>> +	 * uniform sector maps are correctly populated in the erase_map
>> +	 * structure. The table below shows all possible combinations of related
>> +	 * register bits and its availability in SMPT.
>> +	 *
>> +	 *   CFR3[3] | CFR1[6] | CFR1[2] | Sector Map | Available in SMPT?
>> +	 *  -------------------------------------------------------------------
>> +	 *      0    |    0    |    0    | Bottom     | YES
>> +	 *      0    |    0    |    1    | Top        | NO (decoded as Split)
>> +	 *      0    |    1    |    0    | Split      | NO
>> +	 *      0    |    1    |    1    | Split      | NO (decoded as Top)
>> +	 *      1    |    0    |    0    | Uniform    | YES
>> +	 *      1    |    0    |    1    | Uniform    | NO
>> +	 *      1    |    1    |    0    | Uniform    | NO
>> +	 *      1    |    1    |    1    | Uniform    | NO
>> +	 *  -------------------------------------------------------------------
> 
> Neat! This would be very helpful for anyone looking at this flash's SMPT 
> table in the future.
> 
>> +	 */
> 
> Nitpick: It would be good to merge this
> 
>> +
>> +	/* Fast Read 4B requires mode cycles */
>> +	nor->params->reads[SNOR_CMD_READ_FAST].num_mode_clocks = 8;
>> +
>> +	/* The writesize should be ECC data unit size */
>> +	nor->params->writesize = 16;
>> +
>> +	/* Fixup for multi-die package parts */
>> +	if (nor->params->size > SZ_128M) {
>> +		/*
>> +		 * For the dual-die package parts (2Gb), SMPT parse fails due to
>> +		 * incorrect SMPT entries and the erase map is populated as 4K
>> +		 * uniform that does not supported the parts. So it needs to be
>> +		 * rolled back to 256K uniform that is the factory default of
>> +		 * multi-die package parts.
>> +		 */
> 
> ... and this comment. If the table can't fit due to indentation, at 
> least move it to above the "if" so it is at least somewhat close to 
> where the SMPT is actually being fixed up.
> 
>> +		spi_nor_init_uniform_erase_map(&nor->params->erase_map,
>> +					       BIT(SNOR_ERASE_TYPE_MAX - 1),
>> +					       nor->params->size);
>> +
>> +		/* Need to check status of each die via RDAR command */
>> +		nor->params->ready = s25hx_t_mdp_ready;
>> +	}
>> +}
>> +
>> +static struct spi_nor_fixups s25hx_t_fixups = {
>> +	.post_bfpt = s25hx_t_post_bfpt_fixups,
>> +	.post_sfdp = s25hx_t_post_sfdp_fixups
>> +};
>> +
>>  /**
>>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>>   * @nor:		pointer to a 'struct spi_nor'
>> @@ -475,6 +576,24 @@ static const struct flash_info spansion_parts[] = {
>>  	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
>>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>  			     SPI_NOR_4B_OPCODES) },
>> +	{ "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hl02gt",  INFO6(0x342a1c, 0x0f0090, 256 * 1024, 1024,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>> +	{ "s25hs02gt",  INFO6(0x342b1c, 0x0f0090, 256 * 1024, 1024,
>> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
>> +	  .fixups = &s25hx_t_fixups },
>>  	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
>>  			      SPI_NOR_NO_ERASE) },
>>  	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,
> 
> Other than the comments above,
> 
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 

Best Regards,
Takahiro

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

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

[Takahiro Kuwano]

Hi,

On 3/12/2021 6:45 PM, 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.
> 
> For the single-die package parts (512Mb and 1Gb), only bottom 4KB and
> uniform sector sizes are supported. For the multi-die package parts (2Gb),
> only uniform sector sizes is supprted. This is due to missing or incorrect
> entries in SMPT. Fixup for other sector sizes configurations will be
> followed up as needed.
> 
> Tested on Xilinx Zynq-7000 FPGA board.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> Changes in v3:
>   - Remove S25HL256T and S25HS256T
>   - Add S25HL02GT and S25HS02GT 
>   - Add support for multi-die package parts support
>   - Remove erase_map fix for top/split sector layout
>   - Set ECC data unit size (16B) to writesize 
> 
>  drivers/mtd/spi-nor/spansion.c | 119 +++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 5742212fde2e..05a4ea00acfc 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c

[...]  

>  /**
>   * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
>   * @nor:		pointer to a 'struct spi_nor'
> @@ -475,6 +576,24 @@ static const struct flash_info spansion_parts[] = {
>  	{ "s25fl256l",  INFO(0x016019,      0,  64 * 1024, 512,
>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>  			     SPI_NOR_4B_OPCODES) },
> +	{ "s25hl512t",  INFO6(0x342a1a, 0x0f0390, 256 * 1024, 256,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hl01gt",  INFO6(0x342a1b, 0x0f0390, 256 * 1024, 512,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hl02gt",  INFO6(0x342a1c, 0x0f0090, 256 * 1024, 1024,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hs512t",  INFO6(0x342b1a, 0x0f0390, 256 * 1024, 256,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hs01gt",  INFO6(0x342b1b, 0x0f0390, 256 * 1024, 512,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },
> +	{ "s25hs02gt",  INFO6(0x342b1c, 0x0f0090, 256 * 1024, 1024,
> +			     SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
> +	  .fixups = &s25hx_t_fixups },

I noticed USE_CLSR is not needed for 2Gb parts since the ->ready() hook
for multi-die package parts knows about that.

Thanks,
Takahiro

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