[PATCH 0/5] mtd: spi-nor: spansion: Add support for Infineon S28HS02GT

[PATCH 0/5] mtd: spi-nor: spansion: Add support for Infineon S28HS02GT

[tkuw584924]

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

Infineon S28HS02GT is 2Gbit, multi-chip package, Octal SPI NOR flash. This
series reworks octal_dtr_enable part to support MCP and then adds ID and
fixups for S28HS02GT.

Tested on Zynq-7000 platform with Infineon SPI controller

ID, SFDP, Test logs:
---
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
s28hs02gt
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
345b1c
zynq> cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
spansion
zynq> xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450080106fe00000114000100ff84000102500100ff050001055801
00ff8700011c6c0100ff88000106dc0100ff81000118040200ff0a000104
f40100ffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
fffffffffffffffffffffffffffffffff7218affffffff7f000000000000
0000eeffffffffff0000ffff00000c2100ff00ff12dc23faff8b82e7ffec
ec2319497ab07ab0f766805c000000fff910c0a0000000000000bc020000
0000ffff7e7e41120ffe21ffffdc00ee800b7171656500b0ff9600000000
0c551ca20000800000000000c0ccfffb88fbfffb00650090066500b10065
009600650095716503d0716503d0a46bfb0290a579a20040288e0000ff00
0000ff0071650690716506900000000000000000716506d1716506d17165
0691716506910000ff000000ff00716505d5716505d50000a01500008008
000000080000801000000010000080180000001800000601000000008000
710600030600fc65ff0804008000fc65ff0402008000fc65ff0804008008
fd65ff0402008008fe0202fff1ff0100f8ff0100f8fffb0ffe0902fff8ff
fb0ff8ff0100f1ff0100fe0104fff1ff0100f8ff0100f8fff70ff8ff0100
f1ff0100ff0a00fff8ffff0f                                    
zynq> md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
6193b9729008b80b9a2b4bb3ce06a91d  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
zynq> test_qspi.sh
6+0 records in
6+0 records out
6291456 bytes (6.0MB) copied, 0.234679 seconds, 25.6MB/s
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Erased 6291456 bytes from address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0600000
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
3020d73d4d0e570449faacfbadf134e074ced30e  qspi_test
3020d73d4d0e570449faacfbadf134e074ced30e  qspi_read
---

Takahiro Kuwano (5):
  mtd: spi-nor: spansion: Preserve CFR2V[7:4] at latency update
  mtd: spi-nor: spansion: Rework cypress_nor_octal_dtr_enable()
  mtd: spi-nor: spansion: Rework octal_dtr_enable() for MCP
  mtd: spi-nor: spansion: Octal DTR support in RD_ANY_REG_OP
  mtd: spi-nor: spansion: Add S28HS02GT ID and fixups

 drivers/mtd/spi-nor/spansion.c | 187 +++++++++++++++++++++++----------
 1 file changed, 130 insertions(+), 57 deletions(-)

-- 
2.34.1


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

[PATCH 1/5] mtd: spi-nor: spansion: Preserve CFR2V[7] when writing MEMLAT

[tkuw584924]

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

CFR2V[7] is assigned to Flash's address mode (3- or 4-ybte) and must not
be changed when writing MEMLAT (CFR2V[3:0]). CFR2V must be read first,
modified only CFR2V[3:0], then written back.

Fixes: c3266af101f2 ("mtd: spi-nor: spansion: add support for Cypress Semper flash")
Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
Cc: stable@vger.kernel.org
---
 drivers/mtd/spi-nor/spansion.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index f2f4bc060f5e..7804be3a9f2a 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -27,6 +27,7 @@
 #define SPINOR_REG_CYPRESS_CFR2			0x3
 #define SPINOR_REG_CYPRESS_CFR2V					\
 	(SPINOR_REG_CYPRESS_VREG + SPINOR_REG_CYPRESS_CFR2)
+#define SPINOR_REG_CYPRESS_CFR2_MEMLAT_MASK	GENMASK(3, 0)
 #define SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24	0xb
 #define SPINOR_REG_CYPRESS_CFR2_ADRBYT		BIT(7)
 #define SPINOR_REG_CYPRESS_CFR3			0x4
@@ -162,8 +163,17 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
 	int ret;
 	u8 addr_mode_nbytes = nor->params->addr_mode_nbytes;
 
+	op = (struct spi_mem_op)
+		CYPRESS_NOR_RD_ANY_REG_OP(addr_mode_nbytes,
+					  SPINOR_REG_CYPRESS_CFR2V, 0, buf);
+
+	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
+
 	/* Use 24 dummy cycles for memory array reads. */
-	*buf = SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24;
+	*buf &= ~SPINOR_REG_CYPRESS_CFR2_MEMLAT_MASK;
+	*buf |= SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24;
 	op = (struct spi_mem_op)
 		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
 					  SPINOR_REG_CYPRESS_CFR2V, 1, buf);
-- 
2.34.1


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

Re: [PATCH 1/5] mtd: spi-nor: spansion: Preserve CFR2V[7] when writing MEMLAT

[Tudor Ambarus]

On 6/12/23 11:04, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> CFR2V[7] is assigned to Flash's address mode (3- or 4-ybte) and must not
> be changed when writing MEMLAT (CFR2V[3:0]). CFR2V must be read first,
> modified only CFR2V[3:0], then written back.

Last sentence could be reworded to "CFR2V shall be used in a read,
update, write back fashion."

Please specify in the commit message if this fixes a present bug or it's
just a prerequisite for the support that comes. The change is good but
we should be aware if you hit bugs or not.
> 
> Fixes: c3266af101f2 ("mtd: spi-nor: spansion: add support for Cypress Semper flash")
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/mtd/spi-nor/spansion.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index f2f4bc060f5e..7804be3a9f2a 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -27,6 +27,7 @@
>  #define SPINOR_REG_CYPRESS_CFR2			0x3
>  #define SPINOR_REG_CYPRESS_CFR2V					\
>  	(SPINOR_REG_CYPRESS_VREG + SPINOR_REG_CYPRESS_CFR2)
> +#define SPINOR_REG_CYPRESS_CFR2_MEMLAT_MASK	GENMASK(3, 0)
>  #define SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24	0xb
>  #define SPINOR_REG_CYPRESS_CFR2_ADRBYT		BIT(7)
>  #define SPINOR_REG_CYPRESS_CFR3			0x4
> @@ -162,8 +163,17 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
>  	int ret;
>  	u8 addr_mode_nbytes = nor->params->addr_mode_nbytes;
>  
> +	op = (struct spi_mem_op)
> +		CYPRESS_NOR_RD_ANY_REG_OP(addr_mode_nbytes,
> +					  SPINOR_REG_CYPRESS_CFR2V, 0, buf);
> +
> +	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
> +	if (ret)
> +		return ret;
> +
>  	/* Use 24 dummy cycles for memory array reads. */
> -	*buf = SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24;
> +	*buf &= ~SPINOR_REG_CYPRESS_CFR2_MEMLAT_MASK;
> +	*buf |= SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24;

Use FIELD_PREP please.

>  	op = (struct spi_mem_op)
>  		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
>  					  SPINOR_REG_CYPRESS_CFR2V, 1, buf);

Shall you zeroize the struct before using it again?

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

[PATCH 2/5] mtd: spi-nor: spansion: Rework octal_dtr_enable()

[tkuw584924]

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

S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
configuraion for each die. As preparation for MCP support, this patch
replaces cypress_nor_octal_dtr_en/dis() with cypress_nor_setup_memlat()
and cypress_nor_setup_opiddr(). And the ID check part is moved to
cypress_nor_octal_dtr_enable().

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

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 7804be3a9f2a..0daa3a357ae8 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -156,7 +156,7 @@ static int cypress_nor_sr_ready_and_clear(struct spi_nor *nor)
 	return 1;
 }
 
-static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
+static int cypress_nor_setup_memlat(struct spi_nor *nor)
 {
 	struct spi_mem_op op;
 	u8 *buf = nor->bouncebuf;
@@ -178,67 +178,37 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
 		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
 					  SPINOR_REG_CYPRESS_CFR2V, 1, buf);
 
-	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
-	if (ret)
-		return ret;
-
-	nor->read_dummy = 24;
-
-	/* Set the octal and DTR enable bits. */
-	buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN;
-	op = (struct spi_mem_op)
-		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
-					  SPINOR_REG_CYPRESS_CFR5V, 1, buf);
-
-	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
-	if (ret)
-		return ret;
-
-	/* Read flash ID to make sure the switch was successful. */
-	ret = spi_nor_read_id(nor, nor->addr_nbytes, 3, buf,
-			      SNOR_PROTO_8_8_8_DTR);
-	if (ret) {
-		dev_dbg(nor->dev, "error %d reading JEDEC ID after enabling 8D-8D-8D mode\n", ret);
-		return ret;
-	}
-
-	if (memcmp(buf, nor->info->id, nor->info->id_len))
-		return -EINVAL;
-
-	return 0;
+	return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
 }
 
-static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
+static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable)
 {
 	struct spi_mem_op op;
 	u8 *buf = nor->bouncebuf;
-	int ret;
-
-	/*
-	 * The register is 1-byte wide, but 1-byte transactions are not allowed
-	 * in 8D-8D-8D mode. Since there is no register at the next location,
-	 * just initialize the value to 0 and let the transaction go on.
-	 */
-	buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS;
-	buf[1] = 0;
-	op = (struct spi_mem_op)
-		CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes,
-					  SPINOR_REG_CYPRESS_CFR5V, 2, buf);
-	ret = spi_nor_write_any_volatile_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
-	if (ret)
-		return ret;
 
-	/* Read flash ID to make sure the switch was successful. */
-	ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
-	if (ret) {
-		dev_dbg(nor->dev, "error %d reading JEDEC ID after disabling 8D-8D-8D mode\n", ret);
-		return ret;
+	if (enable) {
+		/* Set the octal and DTR enable bits. */
+		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN;
+		op = (struct spi_mem_op)
+			CYPRESS_NOR_WR_ANY_REG_OP(nor->params->addr_mode_nbytes,
+						  SPINOR_REG_CYPRESS_CFR5V, 1,
+						  buf);
+	} else {
+		/*
+		 * The register is 1-byte wide, but 1-byte transactions are not
+		 * allowed in 8D-8D-8D mode. Since there is no register at the
+		 * next location, just initialize the value to 0 and let the
+		 * transaction go on.
+		 */
+		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS;
+		buf[1] = 0;
+		op = (struct spi_mem_op)
+			CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes,
+						  SPINOR_REG_CYPRESS_CFR5V, 2,
+						  buf);
 	}
 
-	if (memcmp(buf, nor->info->id, nor->info->id_len))
-		return -EINVAL;
-
-	return 0;
+	return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
 }
 
 static int cypress_nor_quad_enable_volatile_reg(struct spi_nor *nor, u64 addr)
@@ -642,8 +612,44 @@ static struct spi_nor_fixups s25hx_t_fixups = {
  */
 static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
 {
-	return enable ? cypress_nor_octal_dtr_en(nor) :
-			cypress_nor_octal_dtr_dis(nor);
+	int ret;
+	u8 naddr, ndummy;
+	enum spi_nor_protocol proto;
+
+	if (enable) {
+		ret = cypress_nor_setup_memlat(nor);
+		if (ret)
+			return ret;
+
+		nor->read_dummy = 24;
+	}
+
+	ret = cypress_nor_setup_opiddr(nor, enable);
+	if (ret)
+		return ret;
+
+	/* Read flash ID to make sure the switch was successful. */
+	if (enable) {
+		naddr = nor->addr_nbytes;
+		ndummy = 3;
+		proto = SNOR_PROTO_8_8_8_DTR;
+	} else {
+		naddr = 0;
+		ndummy = 0;
+		proto = SNOR_PROTO_1_1_1;
+	}
+
+	ret = spi_nor_read_id(nor, naddr, ndummy, nor->bouncebuf, proto);
+	if (ret) {
+		dev_dbg(nor->dev, "error %d reading JEDEC ID after %s 8D-8D-8D mode\n",
+			ret, enable ? "enabling" : "disabling");
+		return ret;
+	}
+
+	if (memcmp(nor->bouncebuf, nor->info->id, nor->info->id_len))
+		return -EINVAL;
+
+	return 0;
 }
 
 static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)
-- 
2.34.1


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

Re: [PATCH 2/5] mtd: spi-nor: spansion: Rework octal_dtr_enable()

[Tudor Ambarus]

On 6/12/23 11:04, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
> configuraion for each die. As preparation for MCP support, this patch
> replaces cypress_nor_octal_dtr_en/dis() with cypress_nor_setup_memlat()
> and cypress_nor_setup_opiddr(). And the ID check part is moved to
> cypress_nor_octal_dtr_enable().
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi-nor/spansion.c | 118 +++++++++++++++++----------------
>  1 file changed, 62 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 7804be3a9f2a..0daa3a357ae8 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -156,7 +156,7 @@ static int cypress_nor_sr_ready_and_clear(struct spi_nor *nor)
>  	return 1;
>  }
>  
> -static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
> +static int cypress_nor_setup_memlat(struct spi_nor *nor)

cypress_nor_set_memlat?

Be kind and introduce a description for the method so that we don't
cross check the datasheet each time. I see that for memlat we use a
hardcoded value, whereas it should have been dynamically determined
based on the flash freq. Something to improve in the future if you care.


>  {
>  	struct spi_mem_op op;
>  	u8 *buf = nor->bouncebuf;
> @@ -178,67 +178,37 @@ static int cypress_nor_octal_dtr_en(struct spi_nor *nor)
>  		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
>  					  SPINOR_REG_CYPRESS_CFR2V, 1, buf);
>  
> -	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> -	if (ret)
> -		return ret;
> -
> -	nor->read_dummy = 24;
> -
> -	/* Set the octal and DTR enable bits. */
> -	buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN;
> -	op = (struct spi_mem_op)
> -		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
> -					  SPINOR_REG_CYPRESS_CFR5V, 1, buf);
> -
> -	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> -	if (ret)
> -		return ret;
> -
> -	/* Read flash ID to make sure the switch was successful. */
> -	ret = spi_nor_read_id(nor, nor->addr_nbytes, 3, buf,
> -			      SNOR_PROTO_8_8_8_DTR);
> -	if (ret) {
> -		dev_dbg(nor->dev, "error %d reading JEDEC ID after enabling 8D-8D-8D mode\n", ret);
> -		return ret;
> -	}
> -
> -	if (memcmp(buf, nor->info->id, nor->info->id_len))
> -		return -EINVAL;
> -
> -	return 0;
> +	return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
>  }
>  
> -static int cypress_nor_octal_dtr_dis(struct spi_nor *nor)
> +static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable)

what does opiddr stand for? Let's rename it to something humans can
understand.

>  {
>  	struct spi_mem_op op;
>  	u8 *buf = nor->bouncebuf;
> -	int ret;
> -
> -	/*
> -	 * The register is 1-byte wide, but 1-byte transactions are not allowed
> -	 * in 8D-8D-8D mode. Since there is no register at the next location,
> -	 * just initialize the value to 0 and let the transaction go on.
> -	 */
> -	buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS;
> -	buf[1] = 0;
> -	op = (struct spi_mem_op)
> -		CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes,
> -					  SPINOR_REG_CYPRESS_CFR5V, 2, buf);
> -	ret = spi_nor_write_any_volatile_reg(nor, &op, SNOR_PROTO_8_8_8_DTR);
> -	if (ret)
> -		return ret;
>  
> -	/* Read flash ID to make sure the switch was successful. */
> -	ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
> -	if (ret) {
> -		dev_dbg(nor->dev, "error %d reading JEDEC ID after disabling 8D-8D-8D mode\n", ret);
> -		return ret;
> +	if (enable) {
> +		/* Set the octal and DTR enable bits. */
> +		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN;
> +		op = (struct spi_mem_op)
> +			CYPRESS_NOR_WR_ANY_REG_OP(nor->params->addr_mode_nbytes,
> +						  SPINOR_REG_CYPRESS_CFR5V, 1,
> +						  buf);
> +	} else {
> +		/*
> +		 * The register is 1-byte wide, but 1-byte transactions are not
> +		 * allowed in 8D-8D-8D mode. Since there is no register at the
> +		 * next location, just initialize the value to 0 and let the
> +		 * transaction go on.
> +		 */
> +		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS;
> +		buf[1] = 0;
> +		op = (struct spi_mem_op)
> +			CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes,
> +						  SPINOR_REG_CYPRESS_CFR5V, 2,
> +						  buf);
>  	}
>  
> -	if (memcmp(buf, nor->info->id, nor->info->id_len))
> -		return -EINVAL;
> -
> -	return 0;
> +	return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
>  }
>  
>  static int cypress_nor_quad_enable_volatile_reg(struct spi_nor *nor, u64 addr)
> @@ -642,8 +612,44 @@ static struct spi_nor_fixups s25hx_t_fixups = {
>   */
>  static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)

what a terrible name

>  {
> -	return enable ? cypress_nor_octal_dtr_en(nor) :
> -			cypress_nor_octal_dtr_dis(nor);
> +	int ret;
> +	u8 naddr, ndummy;
> +	enum spi_nor_protocol proto;
> +
> +	if (enable) {

so we have cypress_nor_octal_dtr_enable and now we check for enable,
yuck. I know it comes from the SPI NOR core, we shall update the core, I
wouldn't continue like this.

> +		ret = cypress_nor_setup_memlat(nor);
> +		if (ret)
> +			return ret;
> +
> +		nor->read_dummy = 24;

shouldn't this be set in cypress_nor_set_memlat?
> +	}
> +
> +	ret = cypress_nor_setup_opiddr(nor, enable);
> +	if (ret)
> +		return ret;
> +
> +	/* Read flash ID to make sure the switch was successful. */
> +	if (enable) {
> +		naddr = nor->addr_nbytes;
> +		ndummy = 3;
> +		proto = SNOR_PROTO_8_8_8_DTR;
> +	} else {
> +		naddr = 0;
> +		ndummy = 0;
> +		proto = SNOR_PROTO_1_1_1;
> +	}

I don't like all the if conditions in the octal_dtr_enable methods, I
find the method hard to read and I feel we are butchering the code just
to make it work.
> +
> +	ret = spi_nor_read_id(nor, naddr, ndummy, nor->bouncebuf, proto);
> +	if (ret) {
> +		dev_dbg(nor->dev, "error %d reading JEDEC ID after %s 8D-8D-8D mode\n",
> +			ret, enable ? "enabling" : "disabling");
> +		return ret;
> +	}
> +
> +	if (memcmp(nor->bouncebuf, nor->info->id, nor->info->id_len))
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)

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

[PATCH 3/5] mtd: spi-nor: spansion: Add MCP support in octal_dtr_enable()

[tkuw584924]

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

S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
configuraion for each die. We can access to configuration registers in each
die by using params->n_dice and params->vreg_offset[] populated from SFDP.

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

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 0daa3a357ae8..eda3731c17be 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -156,7 +156,7 @@ static int cypress_nor_sr_ready_and_clear(struct spi_nor *nor)
 	return 1;
 }
 
-static int cypress_nor_setup_memlat(struct spi_nor *nor)
+static int cypress_nor_setup_memlat(struct spi_nor *nor, u64 addr)
 {
 	struct spi_mem_op op;
 	u8 *buf = nor->bouncebuf;
@@ -164,8 +164,7 @@ static int cypress_nor_setup_memlat(struct spi_nor *nor)
 	u8 addr_mode_nbytes = nor->params->addr_mode_nbytes;
 
 	op = (struct spi_mem_op)
-		CYPRESS_NOR_RD_ANY_REG_OP(addr_mode_nbytes,
-					  SPINOR_REG_CYPRESS_CFR2V, 0, buf);
+		CYPRESS_NOR_RD_ANY_REG_OP(addr_mode_nbytes, addr, 0, buf);
 
 	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
 	if (ret)
@@ -175,13 +174,12 @@ static int cypress_nor_setup_memlat(struct spi_nor *nor)
 	*buf &= ~SPINOR_REG_CYPRESS_CFR2_MEMLAT_MASK;
 	*buf |= SPINOR_REG_CYPRESS_CFR2_MEMLAT_11_24;
 	op = (struct spi_mem_op)
-		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes,
-					  SPINOR_REG_CYPRESS_CFR2V, 1, buf);
+		CYPRESS_NOR_WR_ANY_REG_OP(addr_mode_nbytes, addr, 1, buf);
 
 	return spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
 }
 
-static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable)
+static int cypress_nor_setup_opiddr(struct spi_nor *nor, u64 addr, bool enable)
 {
 	struct spi_mem_op op;
 	u8 *buf = nor->bouncebuf;
@@ -191,8 +189,7 @@ static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable)
 		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_EN;
 		op = (struct spi_mem_op)
 			CYPRESS_NOR_WR_ANY_REG_OP(nor->params->addr_mode_nbytes,
-						  SPINOR_REG_CYPRESS_CFR5V, 1,
-						  buf);
+						  addr, 1, buf);
 	} else {
 		/*
 		 * The register is 1-byte wide, but 1-byte transactions are not
@@ -203,8 +200,7 @@ static int cypress_nor_setup_opiddr(struct spi_nor *nor, bool enable)
 		buf[0] = SPINOR_REG_CYPRESS_CFR5_OCT_DTR_DS;
 		buf[1] = 0;
 		op = (struct spi_mem_op)
-			CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes,
-						  SPINOR_REG_CYPRESS_CFR5V, 2,
+			CYPRESS_NOR_WR_ANY_REG_OP(nor->addr_nbytes, addr, 2,
 						  buf);
 	}
 
@@ -600,6 +596,49 @@ static struct spi_nor_fixups s25hx_t_fixups = {
 	.late_init = s25hx_t_late_init,
 };
 
+static int cypress_nor_octal_dtr_enable_single_chip(struct spi_nor *nor,
+						    bool enable)
+{
+	int ret;
+
+	if (enable) {
+		ret = cypress_nor_setup_memlat(nor, SPINOR_REG_CYPRESS_CFR2V);
+		if (ret)
+			return ret;
+
+		nor->read_dummy = 24;
+	}
+
+	return cypress_nor_setup_opiddr(nor, SPINOR_REG_CYPRESS_CFR5V, enable);
+}
+
+static int cypress_nor_octal_dtr_enable_mcp(struct spi_nor *nor, bool enable)
+{
+	struct spi_nor_flash_parameter *params = nor->params;
+	u64 addr;
+	u8 i;
+	int ret;
+
+	if (enable) {
+		for (i = 0; i < params->n_dice; i++) {
+			addr = params->vreg_offset[i] + SPINOR_REG_CYPRESS_CFR2;
+			ret = cypress_nor_setup_memlat(nor, addr);
+			if (ret)
+				return ret;
+		}
+		nor->read_dummy = 24;
+	}
+
+	for (i = 0; i < params->n_dice; i++) {
+		addr = params->vreg_offset[i] + SPINOR_REG_CYPRESS_CFR5;
+		ret = cypress_nor_setup_opiddr(nor, addr, enable);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * cypress_nor_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
  * @nor:		pointer to a 'struct spi_nor'
@@ -616,15 +655,11 @@ static int cypress_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
 	u8 naddr, ndummy;
 	enum spi_nor_protocol proto;
 
-	if (enable) {
-		ret = cypress_nor_setup_memlat(nor);
-		if (ret)
-			return ret;
-
-		nor->read_dummy = 24;
-	}
+	if (nor->params->n_dice)
+		ret = cypress_nor_octal_dtr_enable_mcp(nor, enable);
+	else
+		ret = cypress_nor_octal_dtr_enable_single_chip(nor, enable);
 
-	ret = cypress_nor_setup_opiddr(nor, enable);
 	if (ret)
 		return ret;
 
-- 
2.34.1


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

Re: [PATCH 3/5] mtd: spi-nor: spansion: Add MCP support in octal_dtr_enable()

[Tudor Ambarus]

On 6/12/23 11:04, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
> configuraion for each die. We can access to configuration registers in each
> die by using params->n_dice and params->vreg_offset[] populated from SFDP.

I wonder if we really need to differentiate between the MCP and single
chip package flashes. Do the single chip flashes that are currently
supported in spansion.c and support Octal DDR define the params->n_dice
and params->vreg_offset[]? Do they have the necessary SFDP tables?

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

Re: [PATCH 3/5] mtd: spi-nor: spansion: Add MCP support in octal_dtr_enable()

[Michael Walle]

Hi Takahiro, Hi Miquel,

> S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
> configuraion for each die. We can access to configuration registers in 
> each
> die by using params->n_dice and params->vreg_offset[] populated from 
> SFDP.

I'm looking into cleaning up the flash db and come across the n_banks
and n_dice parameter. Which sounds like they seem to be similar, maybe
identical? Unfortunately, there is no public datasheet for the macronix
flash.. IIRC there is one for the Semperflashes but I'm not sure if 
there
is one for the MCP parts.
Could we somehow clarify if these are the same properties? I've looked 
at
the SFDP tables of the macronix flash and they don't have the SCCR map 
for
multi chip devices :/

-michael

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

Re: [PATCH 3/5] mtd: spi-nor: spansion: Add MCP support in octal_dtr_enable()

[Takahiro Kuwano]

Hi Michael,

On 7/18/2023 7:49 AM, Michael Walle wrote:
> Hi Takahiro, Hi Miquel,
> 
>> S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
>> configuraion for each die. We can access to configuration registers in each
>> die by using params->n_dice and params->vreg_offset[] populated from SFDP.
> 
> I'm looking into cleaning up the flash db and come across the n_banks
> and n_dice parameter. Which sounds like they seem to be similar, maybe
> identical? Unfortunately, there is no public datasheet for the macronix
> flash.. IIRC there is one for the Semperflashes but I'm not sure if there
> is one for the MCP parts.
> Could we somehow clarify if these are the same properties? I've looked at
> the SFDP tables of the macronix flash and they don't have the SCCR map for
> multi chip devices :/
> 
> -michael

The n_banks and n_dice are different properties. One die can contain multiple
banks. Infineon does not have multi-bank parts that support RWW. The SEMPER
MCP parts do not support RWW on different die.

Thanks,
Takahiro

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

Re: [PATCH 3/5] mtd: spi-nor: spansion: Add MCP support in octal_dtr_enable()

[Michael Walle]

Hi,

>>> S28HS02GT is multi-chip package (MCP) device that requires Octal DTR
>>> configuraion for each die. We can access to configuration registers 
>>> in each
>>> die by using params->n_dice and params->vreg_offset[] populated from 
>>> SFDP.
>> 
>> I'm looking into cleaning up the flash db and come across the n_banks
>> and n_dice parameter. Which sounds like they seem to be similar, maybe
>> identical? Unfortunately, there is no public datasheet for the 
>> macronix
>> flash.. IIRC there is one for the Semperflashes but I'm not sure if 
>> there
>> is one for the MCP parts.
>> Could we somehow clarify if these are the same properties? I've looked 
>> at
>> the SFDP tables of the macronix flash and they don't have the SCCR map 
>> for
>> multi chip devices :/
>> 
>> -michael
> 
> The n_banks and n_dice are different properties. One die can contain 
> multiple
> banks. Infineon does not have multi-bank parts that support RWW. The 
> SEMPER
> MCP parts do not support RWW on different die.

Ok. thanks for the quick answer!

-michael

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

[PATCH 4/5] mtd: spi-nor: spansion: Octal DTR support in RD_ANY_REG_OP

[tkuw584924]

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

S28HS02GT uses RD_ANY_REG_OP to read status of each die. In Octal DTR
mode, RD_ANY_REG_OP needs dummy cycles (same as params->rdsr_dummy) and
data length should be 2.

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

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index eda3731c17be..8b8512402639 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -103,11 +103,17 @@ static void spansion_nor_clear_sr(struct spi_nor *nor)
 
 static int cypress_nor_sr_ready_and_clear_reg(struct spi_nor *nor, u64 addr)
 {
+	struct spi_nor_flash_parameter *params = nor->params;
 	struct spi_mem_op op =
-		CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_mode_nbytes, addr,
+		CYPRESS_NOR_RD_ANY_REG_OP(params->addr_mode_nbytes, addr,
 					  0, nor->bouncebuf);
 	int ret;
 
+	if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
+		op.dummy.nbytes = params->rdsr_dummy;
+		op.data.nbytes = 2;
+	}
+
 	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
 	if (ret)
 		return ret;
-- 
2.34.1


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

Re: [PATCH 4/5] mtd: spi-nor: spansion: Octal DTR support in RD_ANY_REG_OP

[Tudor Ambarus]

On 6/12/23 11:04, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> S28HS02GT uses RD_ANY_REG_OP to read status of each die. In Octal DTR
> mode, RD_ANY_REG_OP needs dummy cycles (same as params->rdsr_dummy) and
> data length should be 2.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi-nor/spansion.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index eda3731c17be..8b8512402639 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -103,11 +103,17 @@ static void spansion_nor_clear_sr(struct spi_nor *nor)
>  
>  static int cypress_nor_sr_ready_and_clear_reg(struct spi_nor *nor, u64 addr)
>  {
> +	struct spi_nor_flash_parameter *params = nor->params;
>  	struct spi_mem_op op =
> -		CYPRESS_NOR_RD_ANY_REG_OP(nor->params->addr_mode_nbytes, addr,
> +		CYPRESS_NOR_RD_ANY_REG_OP(params->addr_mode_nbytes, addr,
>  					  0, nor->bouncebuf);
>  	int ret;
>  
> +	if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> +		op.dummy.nbytes = params->rdsr_dummy;

yet another dummy nbytes nonsense.

$ git grep rdsr_dummy drivers/mtd/spi-nor/core.h
drivers/mtd/spi-nor/core.h: * @rdsr_dummy:              dummy cycles
needed for Read Status Register command

rdsr_dummy is defined as number of cycles and here you assume it's in
bytes. Who's right?

> +		op.data.nbytes = 2;
> +	}
> +
>  	ret = spi_nor_read_any_reg(nor, &op, nor->reg_proto);
>  	if (ret)
>  		return ret;

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

[PATCH 5/5] mtd: spi-nor: spansion: Add S28HS02GT ID and fixups

[tkuw584924]

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

Infineon S28HS02GT is 2Gb, multi-chip package, Octal SPI Flash. This patch
adds ID table, fix params->n_dice value, and replace params->ready() with
MCP version.

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

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 8b8512402639..88dc849a63b3 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -720,6 +720,10 @@ static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)
 	 */
 	nor->params->rdsr_addr_nbytes = 4;
 
+	/* The 2 Gb parts duplicate info and advertise 4 dice instead of 2. */
+	if (nor->params->size == SZ_256M)
+		nor->params->n_dice = 2;
+
 	return cypress_nor_get_page_size(nor);
 }
 
@@ -738,8 +742,15 @@ static int s28hx_t_post_bfpt_fixup(struct spi_nor *nor,
 
 static void s28hx_t_late_init(struct spi_nor *nor)
 {
-	nor->params->octal_dtr_enable = cypress_nor_octal_dtr_enable;
+	struct spi_nor_flash_parameter *params = nor->params;
+
+	params->octal_dtr_enable = cypress_nor_octal_dtr_enable;
+
 	cypress_nor_ecc_init(nor);
+
+	/* Replace ready() with multi die version */
+	if (params->n_dice)
+		params->ready = cypress_nor_sr_ready_and_clear;
 }
 
 static const struct spi_nor_fixups s28hx_t_fixups = {
@@ -913,6 +924,11 @@ static const struct flash_info spansion_nor_parts[] = {
 		MFR_FLAGS(USE_CLPEF)
 		.fixups = &s28hx_t_fixups,
 	},
+	{ "s28hs02gt",   INFO(0x345b1c,      0, 256 * 1024, 1024)
+		PARSE_SFDP
+		MFR_FLAGS(USE_CLPEF)
+		.fixups = &s28hx_t_fixups,
+	},
 };
 
 /**
-- 
2.34.1


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

Re: [PATCH 5/5] mtd: spi-nor: spansion: Add S28HS02GT ID and fixups

[Tudor Ambarus]

On 6/12/23 11:04, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> 
> Infineon S28HS02GT is 2Gb, multi-chip package, Octal SPI Flash. This patch
> adds ID table, fix params->n_dice value, and replace params->ready() with
> MCP version.
> 
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
>  drivers/mtd/spi-nor/spansion.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 8b8512402639..88dc849a63b3 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -720,6 +720,10 @@ static int s28hx_t_post_sfdp_fixup(struct spi_nor *nor)

you are modifying all the s28hx_t flashes, yet I don't see anything
mentioned about them in the commit message, why they are still working
and what tests have been done.

>  	 */
>  	nor->params->rdsr_addr_nbytes = 4;
>  
> +	/* The 2 Gb parts duplicate info and advertise 4 dice instead of 2. */
> +	if (nor->params->size == SZ_256M)
> +		nor->params->n_dice = 2;
> +
>  	return cypress_nor_get_page_size(nor);
>  }
>  
> @@ -738,8 +742,15 @@ static int s28hx_t_post_bfpt_fixup(struct spi_nor *nor,
>  
>  static void s28hx_t_late_init(struct spi_nor *nor)
>  {
> -	nor->params->octal_dtr_enable = cypress_nor_octal_dtr_enable;
> +	struct spi_nor_flash_parameter *params = nor->params;
> +
> +	params->octal_dtr_enable = cypress_nor_octal_dtr_enable;
> +
>  	cypress_nor_ecc_init(nor);
> +
> +	/* Replace ready() with multi die version */
> +	if (params->n_dice)
> +		params->ready = cypress_nor_sr_ready_and_clear;
>  }
>  
>  static const struct spi_nor_fixups s28hx_t_fixups = {
> @@ -913,6 +924,11 @@ static const struct flash_info spansion_nor_parts[] = {
>  		MFR_FLAGS(USE_CLPEF)
>  		.fixups = &s28hx_t_fixups,
>  	},
> +	{ "s28hs02gt",   INFO(0x345b1c,      0, 256 * 1024, 1024)
> +		PARSE_SFDP
> +		MFR_FLAGS(USE_CLPEF)
> +		.fixups = &s28hx_t_fixups,
> +	},
>  };
>  
>  /**

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