linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Add octal DTR support for Macronix flash
@ 2021-04-20  6:29 Zhengxun Li
  2021-04-20  6:29 ` [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation Zhengxun Li
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Zhengxun Li @ 2021-04-20  6:29 UTC (permalink / raw)
  To: linux-mtd, linux-spi
  Cc: tudor.ambarus, miquel.raynal, broonie, jaimeliao, Zhengxun Li

This series adds support for Octal DTR for Macronix flashes. The
first set of patches is add Macronix octal dtr mode support. The
second set of patches add Macronix octaflash series support. The
last add the Octal DTR mode support for host driver.

Changes in v3:
- Add support for Macronix octaflash series.

Changes in v2:
- Define with a generic name to describe the maximum dummy cycles.
- Compare the ID directly in the loop, no longer copy and execute
  memcmp().
- Add spi_mem_dtr_supports_op() to support dtr operation.

Zhengxun Li (3):
  mtd: spi-nor: macronix: add support for Macronix octal dtr operation
  mtd: spi-nor: macronix: add support for Macronix octaflash series
  spi: mxic: patch for octal DTR mode support

 drivers/mtd/spi-nor/macronix.c | 217 +++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi-mxic.c         |  41 +++++---
 2 files changed, 247 insertions(+), 11 deletions(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation
  2021-04-20  6:29 [PATCH v3 0/3] Add octal DTR support for Macronix flash Zhengxun Li
@ 2021-04-20  6:29 ` Zhengxun Li
  2021-04-27  2:36   ` Pratyush Yadav
  2021-04-20  6:29 ` [PATCH v3 2/3] mtd: spi-nor: macronix: add support for Macronix octaflash series Zhengxun Li
  2021-04-20  6:29 ` [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support Zhengxun Li
  2 siblings, 1 reply; 12+ messages in thread
From: Zhengxun Li @ 2021-04-20  6:29 UTC (permalink / raw)
  To: linux-mtd, linux-spi
  Cc: tudor.ambarus, miquel.raynal, broonie, jaimeliao, Zhengxun Li

The ocatflash is an xSPI compliant octal DTR flash. Add support
for using it in octal DTR mode.

Enable Octal DTR mode with 20 dummy cycles to allow running at the
maximum supported frequency of 200Mhz.

Try to verify the flash ID to check whether the flash memory in octal
DTR mode is correct. When reading ID in OCTAL DTR mode, ID will appear
in a repeated manner. ex: ID[0] = 0xc2, ID[1] = 0xc2, ID[2] = 0x94,
ID[3] = 0x94... Rearrange the order so that the ID can pass.

Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
---
 drivers/mtd/spi-nor/macronix.c | 117 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 42c2cf3..881eaf8 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,16 @@
 
 #include "core.h"
 
+#define SPINOR_OP_RD_CR2		0x71		/* Read configuration register 2 */
+#define SPINOR_OP_WR_CR2		0x72		/* Write configuration register 2 */
+#define SPINOR_OP_MXIC_DTR_RD		0xee		/* Fast Read opcode in DTR mode */
+#define SPINOR_REG_MXIC_CR2_MODE	0x00000000	/* For setting octal DTR mode */
+#define SPINOR_REG_MXIC_OPI_DTR_EN	0x2		/* Enable Octal DTR */
+#define SPINOR_REG_MXIC_OPI_DTR_DIS	0x1		/* Disable Octal DTR */
+#define SPINOR_REG_MXIC_CR2_DC		0x00000300	/* For setting dummy cycles */
+#define SPINOR_REG_MXIC_DC_20		0x0		/* Setting dummy cycles to 20 */
+#define MXIC_MAX_DC			20		/* Maximum value of dummy cycles */
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -32,6 +42,113 @@
 	.post_bfpt = mx25l25635_post_bfpt_fixups,
 };
 
+/**
+ * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix flashes.
+ * @nor:		pointer to a 'struct spi_nor'
+ * @enable:		whether to enable or disable Octal DTR
+ *
+ * This also sets the memory access dummy cycles to 20 to allow the flash to
+ * run at up to 200MHz.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf, i;
+	int ret;
+
+	if (enable) {
+		/* Use 20 dummy cycles for memory array reads. */
+		ret = spi_nor_write_enable(nor);
+		if (ret)
+			return ret;
+
+		*buf = SPINOR_REG_MXIC_DC_20;
+		op = (struct spi_mem_op)
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
+				   SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_DC, 1),
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+		if (ret)
+			return ret;
+
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			return ret;
+
+		nor->read_dummy = MXIC_MAX_DC;
+	}
+
+	/* Set/unset the octal and DTR enable bits. */
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	if (enable)
+		*buf = SPINOR_REG_MXIC_OPI_DTR_EN;
+	else
+		*buf = SPINOR_REG_MXIC_OPI_DTR_DIS;
+
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
+			   SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1),
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_DATA_OUT(1, buf, 1));
+
+	if (!enable)
+		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+
+	ret = spi_mem_exec_op(nor->spimem, &op);
+	if (ret)
+		return ret;
+
+	/* Read flash ID to make sure the switch was successful. */
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
+			   SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1),
+			   SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1),
+			   SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1));
+
+	if (enable)
+		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+
+	ret = spi_mem_exec_op(nor->spimem, &op);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nor->info->id_len; i++)
+		if (buf[i * 2] != nor->info->id[i])
+			return -EINVAL;
+
+	return 0;
+}
+
+static void octaflash_default_init(struct spi_nor *nor)
+{
+	nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable;
+}
+
+static void octaflash_post_sfdp_fixup(struct spi_nor *nor)
+{
+	/* Set the Fast Read settings. */
+	nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
+	spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_8_8_8_DTR],
+				  0, MXIC_MAX_DC, SPINOR_OP_MXIC_DTR_RD,
+				  SNOR_PROTO_8_8_8_DTR);
+
+	nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
+	nor->params->rdsr_dummy = 4;
+	nor->params->rdsr_addr_nbytes = 4;
+}
+
+static struct spi_nor_fixups octaflash_fixups = {
+	.default_init = octaflash_default_init,
+	.post_sfdp = octaflash_post_sfdp_fixup,
+};
+
 static const struct flash_info macronix_parts[] = {
 	/* Macronix */
 	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 2/3] mtd: spi-nor: macronix: add support for Macronix octaflash series
  2021-04-20  6:29 [PATCH v3 0/3] Add octal DTR support for Macronix flash Zhengxun Li
  2021-04-20  6:29 ` [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation Zhengxun Li
@ 2021-04-20  6:29 ` Zhengxun Li
  2021-04-27  2:47   ` Pratyush Yadav
  2021-04-20  6:29 ` [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support Zhengxun Li
  2 siblings, 1 reply; 12+ messages in thread
From: Zhengxun Li @ 2021-04-20  6:29 UTC (permalink / raw)
  To: linux-mtd, linux-spi
  Cc: tudor.ambarus, miquel.raynal, broonie, jaimeliao, Zhengxun Li

Add 1.8V and 3V Octal NOR Flash IDs.
MX25 series : Serial NOR Flash.
MX66 series : Serial NOR Flash with stacked die.
LM/UM series : Up to 250MHz clock frequency with both DTR/STR operation.
LW/UW series : Support simultaneous Read-while-Write operation in multiple
	       bank architecture. Read-while-write feature which means read
	       data one bank while another bank is programing or erasing.

MX25LM : 3.0V Octal I/O
 -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7841/MX25LM51245G,%203V,%20512Mb,%20v1.1.pdf

MX25UM : 1.8V Octal I/O
 -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7525/MX25UM51245G%20Extreme%20Speed,%201.8V,%20512Mb,%20v1.0.pdf

MX66LM : 3.0V Octal I/O with stacked die
 -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7929/MX66LM1G45G,%203V,%201Gb,%20v1.1.pdf

MX66UM : 1.8V Octal I/O with stacked die
 -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7721/MX66UM1G45G,%201.8V,%201Gb,%20v1.1.pdf

MX25LW : 3.0V Octal I/O with Read-while-Write
MX25UW : 1.8V Octal I/O with Read-while-Write
MX66LW : 3.0V Octal I/O with Read-while-Write and stack die
MX66UW : 1.8V Octal I/O with Read-while-Write and stack die

About LW/UW series, please contact us freely if you have any
questions. For adding Octal NOR Flash IDs, we have validated
each Flash on plateform zynq-picozed.

Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
---
 drivers/mtd/spi-nor/macronix.c | 100 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 881eaf8..8c1cf1b 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -203,6 +203,106 @@ static void octaflash_post_sfdp_fixup(struct spi_nor *nor)
 	{ "mx66u2g45g",	 INFO(0xc2253c, 0, 64 * 1024, 4096,
 			      SECT_4K | SPI_NOR_DUAL_READ |
 			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+	{ "mx66lm2g45g", INFO(0xc2853c, 0, 64 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66lm1g45g", INFO(0xc2853b, 0, 32 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66lw1g45g", INFO(0xc2863b, 0, 32 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25lm51245g", INFO(0xc2853a, 0, 16 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25lw51245g", INFO(0xc2863a, 0, 16 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25lm25645g", INFO(0xc28539, 0, 8 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25lw25645g", INFO(0xc28639, 0, 8 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66um2g45g", INFO(0xc2803c, 0, 64 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66uw2g345g", INFO(0xc2843c, 0, 64 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66uw2g345gx0", INFO(0xc2943c, 0, 64 * 1024, 4096,
+				 SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+				 SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66um1g45g", INFO(0xc2803b, 0, 32 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66um1g45g40", INFO(0xc2808b, 0, 32 * 1024, 4096,
+				SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+				SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx66uw1g45g", INFO(0xc2813b, 0, 32 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25um51245g", INFO(0xc2803a, 0, 16 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw51245g", INFO(0xc2813a, 0, 16 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw51345g", INFO(0xc2843a, 0, 16 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25um25645g", INFO(0xc28039, 0, 8 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw25645g", INFO(0xc28139, 0, 8 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25um25345g", INFO(0xc28339, 0, 8 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw25345g", INFO(0xc28439, 0, 8 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw12845g", INFO(0xc28138, 0, 4 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw12a45g", INFO(0xc28938, 0, 4 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw12345g", INFO(0xc28438, 0, 4 * 1024, 4096,
+			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw6445g", INFO(0xc28137, 0, 2 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
+	{ "mx25uw6345g", INFO(0xc28437, 0, 2 * 1024, 4096,
+			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
+			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
+		.fixups = &octaflash_fixups },
 };
 
 static void macronix_default_init(struct spi_nor *nor)
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support
  2021-04-20  6:29 [PATCH v3 0/3] Add octal DTR support for Macronix flash Zhengxun Li
  2021-04-20  6:29 ` [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation Zhengxun Li
  2021-04-20  6:29 ` [PATCH v3 2/3] mtd: spi-nor: macronix: add support for Macronix octaflash series Zhengxun Li
@ 2021-04-20  6:29 ` Zhengxun Li
  2021-04-26 16:53   ` Mark Brown
  2 siblings, 1 reply; 12+ messages in thread
From: Zhengxun Li @ 2021-04-20  6:29 UTC (permalink / raw)
  To: linux-mtd, linux-spi
  Cc: tudor.ambarus, miquel.raynal, broonie, jaimeliao, Zhengxun Li

Driver patch for octal DTR mode support.

Owing to the spi_mem_default_supports_op() is not support dtr
operation. Based on Pratyush patch "spi: spi-mem: add spi_mem_dtr
_supports_op()" add spi_mem_dtr_supports_op() to support dtr and
keep checking the buswidth and command bytes.

Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
---
 drivers/spi/spi-mxic.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 96b4182..32e757a 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -335,8 +335,10 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
 static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 				     const struct spi_mem_op *op)
 {
-	if (op->data.buswidth > 4 || op->addr.buswidth > 4 ||
-	    op->dummy.buswidth > 4 || op->cmd.buswidth > 4)
+	bool all_false;
+
+	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
+	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
 		return false;
 
 	if (op->data.nbytes && op->dummy.nbytes &&
@@ -346,7 +348,13 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 	if (op->addr.nbytes > 7)
 		return false;
 
-	return spi_mem_default_supports_op(mem, op);
+	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
+		    !op->data.dtr;
+
+	if (all_false)
+		return spi_mem_default_supports_op(mem, op);
+	else
+		return spi_mem_dtr_supports_op(mem, op);
 }
 
 static int mxic_spi_mem_exec_op(struct spi_mem *mem,
@@ -355,14 +363,15 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	struct mxic_spi *mxic = spi_master_get_devdata(mem->spi->master);
 	int nio = 1, i, ret;
 	u32 ss_ctrl;
-	u8 addr[8];
-	u8 opcode = op->cmd.opcode;
+	u8 addr[8], cmd[2];
 
 	ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
 	if (ret)
 		return ret;
 
-	if (mem->spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
+	if (mem->spi->mode & (SPI_TX_OCTAL | SPI_RX_OCTAL))
+		nio = 8;
+	else if (mem->spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
 		nio = 4;
 	else if (mem->spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
 		nio = 2;
@@ -374,19 +383,25 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	       mxic->regs + HC_CFG);
 	writel(HC_EN_BIT, mxic->regs + HC_EN);
 
-	ss_ctrl = OP_CMD_BYTES(1) | OP_CMD_BUSW(fls(op->cmd.buswidth) - 1);
+	ss_ctrl = OP_CMD_BYTES(op->cmd.nbytes) |
+		  OP_CMD_BUSW(fls(op->cmd.buswidth) - 1) |
+		  (op->cmd.dtr ? OP_CMD_DDR : 0);
 
 	if (op->addr.nbytes)
 		ss_ctrl |= OP_ADDR_BYTES(op->addr.nbytes) |
-			   OP_ADDR_BUSW(fls(op->addr.buswidth) - 1);
+			   OP_ADDR_BUSW(fls(op->addr.buswidth) - 1) |
+			   (op->addr.dtr ? OP_ADDR_DDR : 0);
 
 	if (op->dummy.nbytes)
 		ss_ctrl |= OP_DUMMY_CYC(op->dummy.nbytes);
 
 	if (op->data.nbytes) {
-		ss_ctrl |= OP_DATA_BUSW(fls(op->data.buswidth) - 1);
+		ss_ctrl |= OP_DATA_BUSW(fls(op->data.buswidth) - 1) |
+			   (op->data.dtr ? OP_DATA_DDR : 0);
 		if (op->data.dir == SPI_MEM_DATA_IN)
 			ss_ctrl |= OP_READ;
+			if (op->data.dtr)
+				ss_ctrl |= OP_DQS_EN;
 	}
 
 	writel(ss_ctrl, mxic->regs + SS_CTRL(mem->spi->chip_select));
@@ -394,7 +409,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
 	       mxic->regs + HC_CFG);
 
-	ret = mxic_spi_data_xfer(mxic, &opcode, NULL, 1);
+	for (i = 0; i < op->cmd.nbytes; i++)
+		cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 1));
+
+	ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
 	if (ret)
 		goto out;
 
@@ -567,7 +585,8 @@ static int mxic_spi_probe(struct platform_device *pdev)
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->mode_bits = SPI_CPOL | SPI_CPHA |
 			SPI_RX_DUAL | SPI_TX_DUAL |
-			SPI_RX_QUAD | SPI_TX_QUAD;
+			SPI_RX_QUAD | SPI_TX_QUAD |
+			SPI_RX_OCTAL | SPI_TX_OCTAL;
 
 	mxic_spi_hw_init(mxic);
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support
  2021-04-20  6:29 ` [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support Zhengxun Li
@ 2021-04-26 16:53   ` Mark Brown
  2021-04-27  1:48     ` zhengxunli
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2021-04-26 16:53 UTC (permalink / raw)
  To: Zhengxun Li; +Cc: linux-mtd, linux-spi, tudor.ambarus, miquel.raynal, jaimeliao

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

On Tue, Apr 20, 2021 at 02:29:39PM +0800, Zhengxun Li wrote:

> -	return spi_mem_default_supports_op(mem, op);
> +	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> +		    !op->data.dtr;
> +
> +	if (all_false)

This feels like we might want a spi_mem_op_is_dtr() helper?  I can see
other controllers wanting a similar check.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support
  2021-04-26 16:53   ` Mark Brown
@ 2021-04-27  1:48     ` zhengxunli
  0 siblings, 0 replies; 12+ messages in thread
From: zhengxunli @ 2021-04-27  1:48 UTC (permalink / raw)
  To: Mark Brown; +Cc: jaimeliao, linux-mtd, linux-spi, miquel.raynal, tudor.ambarus

Hi Mark,

Thanks for your reply.

"Mark Brown" <broonie@kernel.org> wrote on 2021/04/27 上午 12:53:58:

> "Mark Brown" <broonie@kernel.org> 
> 2021/04/27 上午 12:54
> 
> To
> 
> "Zhengxun Li" <zhengxunli@mxic.com.tw>, 
> 
> cc
> 
> linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org, 
> tudor.ambarus@microchip.com, miquel.raynal@bootlin.com, 
jaimeliao@mxic.com.tw
> 
> Subject
> 
> Re: [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support
> 
> On Tue, Apr 20, 2021 at 02:29:39PM +0800, Zhengxun Li wrote:
> 
> > -   return spi_mem_default_supports_op(mem, op);
> > +   all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > +          !op->data.dtr;
> > +
> > +   if (all_false)
> 
> This feels like we might want a spi_mem_op_is_dtr() helper?  I can see
> other controllers wanting a similar check.

Yes, since spi_mem_default_supports_op does not support any dtr 
operations,
we need spi_mem_op_is dtr() to help us check dtr operations.

Thanks,
Zhengxun Li


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation
  2021-04-20  6:29 ` [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation Zhengxun Li
@ 2021-04-27  2:36   ` Pratyush Yadav
  2021-05-04  5:31     ` zhengxunli
  0 siblings, 1 reply; 12+ messages in thread
From: Pratyush Yadav @ 2021-04-27  2:36 UTC (permalink / raw)
  To: Zhengxun Li
  Cc: linux-mtd, linux-spi, tudor.ambarus, miquel.raynal, broonie, jaimeliao

Hi,

On 20/04/21 02:29PM, Zhengxun Li wrote:
> The ocatflash is an xSPI compliant octal DTR flash. Add support

Typo. s/ocatflash/octaflash/

> for using it in octal DTR mode.
> 
> Enable Octal DTR mode with 20 dummy cycles to allow running at the
> maximum supported frequency of 200Mhz.

Which octaflash is that? The flash datasheets you have linked in patch 2 
either have a max supported frequency of 133 MHz or 250 MHz.

> 
> Try to verify the flash ID to check whether the flash memory in octal
> DTR mode is correct. When reading ID in OCTAL DTR mode, ID will appear
> in a repeated manner. ex: ID[0] = 0xc2, ID[1] = 0xc2, ID[2] = 0x94,
> ID[3] = 0x94... Rearrange the order so that the ID can pass.

Ok. I don't see this mentioned in the datasheet but the timing diagram 
seems to imply this.

> 
> Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/macronix.c | 117 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 42c2cf3..881eaf8 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,16 @@
>  
>  #include "core.h"
>  
> +#define SPINOR_OP_RD_CR2		0x71		/* Read configuration register 2 */
> +#define SPINOR_OP_WR_CR2		0x72		/* Write configuration register 2 */
> +#define SPINOR_OP_MXIC_DTR_RD		0xee		/* Fast Read opcode in DTR mode */
> +#define SPINOR_REG_MXIC_CR2_MODE	0x00000000	/* For setting octal DTR mode */
> +#define SPINOR_REG_MXIC_OPI_DTR_EN	0x2		/* Enable Octal DTR */
> +#define SPINOR_REG_MXIC_OPI_DTR_DIS	0x1		/* Disable Octal DTR */

This would switch the flash to 8S-8S-8S mode, which isn't all that much 
better than 8D-8D-8D (it is a stateful mode, you need to know beforehand 
that the flash is in this mode before you can use it properly). I think 
"disabling" octal DTR should mean switching back to the default 
(1S-1S-1S) mode.

> +#define SPINOR_REG_MXIC_CR2_DC		0x00000300	/* For setting dummy cycles */
> +#define SPINOR_REG_MXIC_DC_20		0x0		/* Setting dummy cycles to 20 */
> +#define MXIC_MAX_DC			20		/* Maximum value of dummy cycles */
> +
>  static int
>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>  			    const struct sfdp_parameter_header *bfpt_header,
> @@ -32,6 +42,113 @@
>  	.post_bfpt = mx25l25635_post_bfpt_fixups,
>  };
>  
> +/**
> + * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix flashes.
> + * @nor:		pointer to a 'struct spi_nor'
> + * @enable:		whether to enable or disable Octal DTR
> + *
> + * This also sets the memory access dummy cycles to 20 to allow the flash to
> + * run at up to 200MHz.

For some flashes it is 250 MHz and for some it is 133 MHz. More on this 
below...

> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +{
> +	struct spi_mem_op op;
> +	u8 *buf = nor->bouncebuf, i;
> +	int ret;
> +
> +	if (enable) {
> +		/* Use 20 dummy cycles for memory array reads. */
> +		ret = spi_nor_write_enable(nor);
> +		if (ret)
> +			return ret;
> +
> +		*buf = SPINOR_REG_MXIC_DC_20;

I've looked at both the 133 and 250 MHz parts. In both, the default 
dummy cycles are already 20. This can be skipped. This way you also 
don't have to say in comments whether this enables 133 MHz or 250 MHz 
operation (again, I don't get where the 200 MHz comes from...).

The chip manufacturer chose sane defaults for the dummy cycle value. 
Let's reap the benefits and reduce the code we have to maintain.

> +		op = (struct spi_mem_op)
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
> +				   SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_DC, 1),
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +		if (ret)
> +			return ret;
> +
> +		ret = spi_nor_wait_till_ready(nor);
> +		if (ret)
> +			return ret;
> +
> +		nor->read_dummy = MXIC_MAX_DC;

I don't see SFDP values listed in the datasheet. Since the flash is 
supposed to be xSPI compliant, it should have a Profile 1.0 table. Does 
the Profile 1.0 parser correctly select 20 dummy cycles for this flash? 
If yes, there is no need for this.

> +	}
> +
> +	/* Set/unset the octal and DTR enable bits. */
> +	ret = spi_nor_write_enable(nor);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		*buf = SPINOR_REG_MXIC_OPI_DTR_EN;
> +	else
> +		*buf = SPINOR_REG_MXIC_OPI_DTR_DIS;
> +
> +	op = (struct spi_mem_op)
> +		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),

I see that the flash uses inverted 2nd byte for opcode in 8D mode. I 
assume this is discovered correctly via SFDP.

> +			   SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1),

4 byte addressing is used for this command even in 1S-1S-1S mode. Ok.

> +			   SPI_MEM_OP_NO_DUMMY,
> +			   SPI_MEM_OP_DATA_OUT(1, buf, 1));
> +
> +	if (!enable)
> +		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);

When disabling, the op would be in 8D-8D-8D mode so having a data length 
of 1 would be invalid. This is currently the case even in the patches 
that I sent for Micron and Cypress.

I am not sure what the correct fix for this is though. One option is to 
send the same byte twice, but I remember that on the Cypress flash the 
second byte over-writes the register at the next address. I'm not sure 
how Macronix flashes handle the second byte. Can you check what the 
behavior for your flash is when you write 2 bytes to the register?

> +
> +	ret = spi_mem_exec_op(nor->spimem, &op);
> +	if (ret)
> +		return ret;
> +
> +	/* Read flash ID to make sure the switch was successful. */
> +	op = (struct spi_mem_op)
> +		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> +			   SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1),
> +			   SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1),
> +			   SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1));
> +
> +	if (enable)
> +		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +
> +	ret = spi_mem_exec_op(nor->spimem, &op);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nor->info->id_len; i++)
> +		if (buf[i * 2] != nor->info->id[i])
> +			return -EINVAL;

Ok.

> +
> +	return 0;
> +}
> +
> +static void octaflash_default_init(struct spi_nor *nor)
> +{
> +	nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable;
> +}
> +
> +static void octaflash_post_sfdp_fixup(struct spi_nor *nor)
> +{
> +	/* Set the Fast Read settings. */
> +	nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> +	spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_8_8_8_DTR],
> +				  0, MXIC_MAX_DC, SPINOR_OP_MXIC_DTR_RD,
> +				  SNOR_PROTO_8_8_8_DTR);
> +
> +	nor->cmd_ext_type = SPI_NOR_EXT_INVERT;

Shouldn't this be discovered via BFPT DWORD 18?

> +	nor->params->rdsr_dummy = 4;
> +	nor->params->rdsr_addr_nbytes = 4;

Shouldn't these two be discovered via the Profile 1.0 table?

In general, avoid hard-coding values that can be discovered through 
SFDP. The device usually knows more about itself than we do.

> +}
> +
> +static struct spi_nor_fixups octaflash_fixups = {

Hm, I don't like this unreferenced variable here. In fact, you should
merge patch 1 and 2 into a single patch. This would combine things in a 
single neat commit, including the links to datasheets. This will make it 
easier for future archaeologists digging around with git blame to find 
some more info about the flash.

> +	.default_init = octaflash_default_init,
> +	.post_sfdp = octaflash_post_sfdp_fixup,
> +};
> +
>  static const struct flash_info macronix_parts[] = {
>  	/* Macronix */
>  	{ "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
> -- 
> 1.9.1

That's an ancient version of Git you're using there ;-)

Overall, I like the direction this patch is taking, but lots of minor 
things to polish out.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/3] mtd: spi-nor: macronix: add support for Macronix octaflash series
  2021-04-20  6:29 ` [PATCH v3 2/3] mtd: spi-nor: macronix: add support for Macronix octaflash series Zhengxun Li
@ 2021-04-27  2:47   ` Pratyush Yadav
  2021-05-04  5:38     ` zhengxunli
  0 siblings, 1 reply; 12+ messages in thread
From: Pratyush Yadav @ 2021-04-27  2:47 UTC (permalink / raw)
  To: Zhengxun Li
  Cc: linux-mtd, linux-spi, tudor.ambarus, miquel.raynal, broonie, jaimeliao

Hi,

On 20/04/21 02:29PM, Zhengxun Li wrote:
> Add 1.8V and 3V Octal NOR Flash IDs.
> MX25 series : Serial NOR Flash.
> MX66 series : Serial NOR Flash with stacked die.

I only looked briefly at the datasheet, but I don't see any mention of 
stacked dies. I assume the stacked die part is transparent from software 
point of view, and is only a hardware implementation detail. Correct?

> LM/UM series : Up to 250MHz clock frequency with both DTR/STR operation.
> LW/UW series : Support simultaneous Read-while-Write operation in multiple
> 	       bank architecture. Read-while-write feature which means read
> 	       data one bank while another bank is programing or erasing.

AFAIK, this feature is not currently supported in SPI NOR.

> 
> MX25LM : 3.0V Octal I/O
>  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7841/MX25LM51245G,%203V,%20512Mb,%20v1.1.pdf
> 
> MX25UM : 1.8V Octal I/O
>  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7525/MX25UM51245G%20Extreme%20Speed,%201.8V,%20512Mb,%20v1.0.pdf
> 
> MX66LM : 3.0V Octal I/O with stacked die
>  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7929/MX66LM1G45G,%203V,%201Gb,%20v1.1.pdf
> 
> MX66UM : 1.8V Octal I/O with stacked die
>  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7721/MX66UM1G45G,%201.8V,%201Gb,%20v1.1.pdf
> 
> MX25LW : 3.0V Octal I/O with Read-while-Write
> MX25UW : 1.8V Octal I/O with Read-while-Write
> MX66LW : 3.0V Octal I/O with Read-while-Write and stack die
> MX66UW : 1.8V Octal I/O with Read-while-Write and stack die
> 
> About LW/UW series, please contact us freely if you have any
> questions. For adding Octal NOR Flash IDs, we have validated
> each Flash on plateform zynq-picozed.
> 
> Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/macronix.c | 100 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 881eaf8..8c1cf1b 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -203,6 +203,106 @@ static void octaflash_post_sfdp_fixup(struct spi_nor *nor)
>  	{ "mx66u2g45g",	 INFO(0xc2253c, 0, 64 * 1024, 4096,
>  			      SECT_4K | SPI_NOR_DUAL_READ |
>  			      SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> +	{ "mx66lm2g45g", INFO(0xc2853c, 0, 64 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66lm1g45g", INFO(0xc2853b, 0, 32 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66lw1g45g", INFO(0xc2863b, 0, 32 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25lm51245g", INFO(0xc2853a, 0, 16 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25lw51245g", INFO(0xc2863a, 0, 16 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25lm25645g", INFO(0xc28539, 0, 8 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25lw25645g", INFO(0xc28639, 0, 8 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66um2g45g", INFO(0xc2803c, 0, 64 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66uw2g345g", INFO(0xc2843c, 0, 64 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66uw2g345gx0", INFO(0xc2943c, 0, 64 * 1024, 4096,
> +				 SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +				 SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66um1g45g", INFO(0xc2803b, 0, 32 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66um1g45g40", INFO(0xc2808b, 0, 32 * 1024, 4096,
> +				SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +				SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx66uw1g45g", INFO(0xc2813b, 0, 32 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25um51245g", INFO(0xc2803a, 0, 16 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw51245g", INFO(0xc2813a, 0, 16 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw51345g", INFO(0xc2843a, 0, 16 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25um25645g", INFO(0xc28039, 0, 8 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw25645g", INFO(0xc28139, 0, 8 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25um25345g", INFO(0xc28339, 0, 8 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw25345g", INFO(0xc28439, 0, 8 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw12845g", INFO(0xc28138, 0, 4 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw12a45g", INFO(0xc28938, 0, 4 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw12345g", INFO(0xc28438, 0, 4 * 1024, 4096,
> +			       SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			       SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw6445g", INFO(0xc28137, 0, 2 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },
> +	{ "mx25uw6345g", INFO(0xc28437, 0, 2 * 1024, 4096,
> +			      SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> +			      SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> +		.fixups = &octaflash_fixups },

Reminder to self: Check if there are any ID collisions here. I have seen 
a couple of them show up recently on Macronix flashes.

Anyway, not much to see here. I suggest you merge this with patch 1.

>  };
>  
>  static void macronix_default_init(struct spi_nor *nor)

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation
  2021-04-27  2:36   ` Pratyush Yadav
@ 2021-05-04  5:31     ` zhengxunli
  2021-05-04  7:42       ` Pratyush Yadav
  0 siblings, 1 reply; 12+ messages in thread
From: zhengxunli @ 2021-05-04  5:31 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: broonie, jaimeliao, linux-mtd, linux-spi, miquel.raynal, tudor.ambarus

Hi Pratyush,

Thanks for your comment on this patch.

"Pratyush Yadav" <p.yadav@ti.com> wrote on 2021/04/27 上午 10:36:06:

> "Pratyush Yadav" <p.yadav@ti.com> 
> 2021/04/27 上午 10:36
> 
> To
> 
> "Zhengxun Li" <zhengxunli@mxic.com.tw>, 
> 
> cc
> 
> <linux-mtd@lists.infradead.org>, <linux-spi@vger.kernel.org>, 
> <tudor.ambarus@microchip.com>, <miquel.raynal@bootlin.com>, 
> <broonie@kernel.org>, <jaimeliao@mxic.com.tw>
> 
> Subject
> 
> Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix 
> octal dtr operation
> 
> Hi,
> 
> On 20/04/21 02:29PM, Zhengxun Li wrote:
> > The ocatflash is an xSPI compliant octal DTR flash. Add support
> 
> Typo. s/ocatflash/octaflash/

Okay, it will be fixed in the next version.

> 
> > for using it in octal DTR mode.
> > 
> > Enable Octal DTR mode with 20 dummy cycles to allow running at the
> > maximum supported frequency of 200Mhz.
> 
> Which octaflash is that? The flash datasheets you have linked in patch 2 

> either have a max supported frequency of 133 MHz or 250 MHz.

Okay, it will be fixed in the next version.

> > 
> > Try to verify the flash ID to check whether the flash memory in octal
> > DTR mode is correct. When reading ID in OCTAL DTR mode, ID will appear
> > in a repeated manner. ex: ID[0] = 0xc2, ID[1] = 0xc2, ID[2] = 0x94,
> > ID[3] = 0x94... Rearrange the order so that the ID can pass.
> 
> Ok. I don't see this mentioned in the datasheet but the timing diagram 
> seems to imply this.
> 
> > 
> > Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 117 ++++++++++++++++++++++++++++
> +++++++++++++
> >  1 file changed, 117 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/macronix.c 
b/drivers/mtd/spi-nor/macronix.c
> > index 42c2cf3..881eaf8 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -8,6 +8,16 @@
> > 
> >  #include "core.h"
> > 
> > +#define SPINOR_OP_RD_CR2      0x71      /* Read configuration 
register 2 */
> > +#define SPINOR_OP_WR_CR2      0x72      /* Write configuration 
> register 2 */
> > +#define SPINOR_OP_MXIC_DTR_RD      0xee      /* Fast Read opcode 
> in DTR mode */
> > +#define SPINOR_REG_MXIC_CR2_MODE   0x00000000   /* For setting 
> octal DTR mode */
> > +#define SPINOR_REG_MXIC_OPI_DTR_EN   0x2      /* Enable Octal DTR */
> > +#define SPINOR_REG_MXIC_OPI_DTR_DIS   0x1      /* Disable Octal DTR 
*/
> 
> This would switch the flash to 8S-8S-8S mode, which isn't all that much 
> better than 8D-8D-8D (it is a stateful mode, you need to know beforehand 

> that the flash is in this mode before you can use it properly). I think 
> "disabling" octal DTR should mean switching back to the default 
> (1S-1S-1S) mode.
> 

Yes, should be switching back to default(1s-1s-1s) mode.

> > +#define SPINOR_REG_MXIC_CR2_DC      0x00000300   /* For setting 
> dummy cycles */
> > +#define SPINOR_REG_MXIC_DC_20      0x0      /* Setting dummy 
> cycles to 20 */
> > +#define MXIC_MAX_DC         20      /* Maximum value of dummy cycles 
*/
> > +
> >  static int
> >  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
> >               const struct sfdp_parameter_header *bfpt_header,
> > @@ -32,6 +42,113 @@
> >     .post_bfpt = mx25l25635_post_bfpt_fixups,
> >  };
> > 
> > +/**
> > + * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on 
> Macronix flashes.
> > + * @nor:      pointer to a 'struct spi_nor'
> > + * @enable:      whether to enable or disable Octal DTR
> > + *
> > + * This also sets the memory access dummy cycles to 20 to allow 
> the flash to
> > + * run at up to 200MHz.
> 
> For some flashes it is 250 MHz and for some it is 133 MHz. More on this 
> below...
> 

Okay, it will be fixed in the next version.

> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int spi_nor_macronix_octal_dtr_enable(struct spi_nor *nor,
> bool enable)
> > +{
> > +   struct spi_mem_op op;
> > +   u8 *buf = nor->bouncebuf, i;
> > +   int ret;
> > +
> > +   if (enable) {
> > +      /* Use 20 dummy cycles for memory array reads. */
> > +      ret = spi_nor_write_enable(nor);
> > +      if (ret)
> > +         return ret;
> > +
> > +      *buf = SPINOR_REG_MXIC_DC_20;
> 
> I've looked at both the 133 and 250 MHz parts. In both, the default 
> dummy cycles are already 20. This can be skipped. This way you also 
> don't have to say in comments whether this enables 133 MHz or 250 MHz 
> operation (again, I don't get where the 200 MHz comes from...).
> 
> The chip manufacturer chose sane defaults for the dummy cycle value. 
> Let's reap the benefits and reduce the code we have to maintain.

Okay, it will be fixed in the next version.

> > +      op = (struct spi_mem_op)
> > +         SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
> > +               SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_DC, 1),
> > +               SPI_MEM_OP_NO_DUMMY,
> > +               SPI_MEM_OP_DATA_OUT(1, buf, 1));
> > +
> > +      ret = spi_mem_exec_op(nor->spimem, &op);
> > +      if (ret)
> > +         return ret;
> > +
> > +      ret = spi_nor_wait_till_ready(nor);
> > +      if (ret)
> > +         return ret;
> > +
> > +      nor->read_dummy = MXIC_MAX_DC;
> 
> I don't see SFDP values listed in the datasheet. Since the flash is 
> supposed to be xSPI compliant, it should have a Profile 1.0 table. Does 
> the Profile 1.0 parser correctly select 20 dummy cycles for this flash? 
> If yes, there is no need for this.

yes, should be removed in the next version.

> > +   }
> > +
> > +   /* Set/unset the octal and DTR enable bits. */
> > +   ret = spi_nor_write_enable(nor);
> > +   if (ret)
> > +      return ret;
> > +
> > +   if (enable)
> > +      *buf = SPINOR_REG_MXIC_OPI_DTR_EN;
> > +   else
> > +      *buf = SPINOR_REG_MXIC_OPI_DTR_DIS;
> > +
> > +   op = (struct spi_mem_op)
> > +      SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_CR2, 1),
> 
> I see that the flash uses inverted 2nd byte for opcode in 8D mode. I 
> assume this is discovered correctly via SFDP.
> 
> > +            SPI_MEM_OP_ADDR(4, SPINOR_REG_MXIC_CR2_MODE, 1),
> 
> 4 byte addressing is used for this command even in 1S-1S-1S mode. Ok.
> 
> > +            SPI_MEM_OP_NO_DUMMY,
> > +            SPI_MEM_OP_DATA_OUT(1, buf, 1));
> > +
> > +   if (!enable)
> > +      spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> 
> When disabling, the op would be in 8D-8D-8D mode so having a data length 

> of 1 would be invalid. This is currently the case even in the patches 
> that I sent for Micron and Cypress.
> 
> I am not sure what the correct fix for this is though. One option is to 
> send the same byte twice, but I remember that on the Cypress flash the 
> second byte over-writes the register at the next address. I'm not sure 
> how Macronix flashes handle the second byte. Can you check what the 
> behavior for your flash is when you write 2 bytes to the register?

I checked the behavior of Macronix and the second byte will overwrites the 
register.
Do we need to send the same bytes to resolve this error?

> > +
> > +   ret = spi_mem_exec_op(nor->spimem, &op);
> > +   if (ret)
> > +      return ret;
> > +
> > +   /* Read flash ID to make sure the switch was successful. */
> > +   op = (struct spi_mem_op)
> > +      SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 1),
> > +            SPI_MEM_OP_ADDR(enable ? 4 : 0, 0, 1),
> > +            SPI_MEM_OP_DUMMY(enable ? 4 : 0, 1),
> > +            SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, buf, 1));
> > +
> > +   if (enable)
> > +      spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> > +
> > +   ret = spi_mem_exec_op(nor->spimem, &op);
> > +   if (ret)
> > +      return ret;
> > +
> > +   for (i = 0; i < nor->info->id_len; i++)
> > +      if (buf[i * 2] != nor->info->id[i])
> > +         return -EINVAL;
> 
> Ok.
> 
> > +
> > +   return 0;
> > +}
> > +
> > +static void octaflash_default_init(struct spi_nor *nor)
> > +{
> > +   nor->params->octal_dtr_enable = spi_nor_macronix_octal_dtr_enable;
> > +}
> > +
> > +static void octaflash_post_sfdp_fixup(struct spi_nor *nor)
> > +{
> > +   /* Set the Fast Read settings. */
> > +   nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> > + 
spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > +              0, MXIC_MAX_DC, SPINOR_OP_MXIC_DTR_RD,
> > +              SNOR_PROTO_8_8_8_DTR);
> > +
> > +   nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> 
> Shouldn't this be discovered via BFPT DWORD 18?

Okay, it will be removed in the next version.

> 
> > +   nor->params->rdsr_dummy = 4;
> > +   nor->params->rdsr_addr_nbytes = 4;
> 
> Shouldn't these two be discovered via the Profile 1.0 table?
> 
> In general, avoid hard-coding values that can be discovered through 
> SFDP. The device usually knows more about itself than we do.
> 

Okay, it will be removed in the next version.

> > +}
> > +
> > +static struct spi_nor_fixups octaflash_fixups = {
> 
> Hm, I don't like this unreferenced variable here. In fact, you should
> merge patch 1 and 2 into a single patch. This would combine things in a 
> single neat commit, including the links to datasheets. This will make it 

> easier for future archaeologists digging around with git blame to find 
> some more info about the flash.

It will be merged in the next version.

> > +   .default_init = octaflash_default_init,
> > +   .post_sfdp = octaflash_post_sfdp_fixup,
> > +};
> > +
> >  static const struct flash_info macronix_parts[] = {
> >     /* Macronix */
> >     { "mx25l512e",   INFO(0xc22010, 0, 64 * 1024,   1, SECT_4K) },
> > -- 
> > 1.9.1
> 
> That's an ancient version of Git you're using there ;-)
> 
> Overall, I like the direction this patch is taking, but lots of minor 
> things to polish out.

Thanks,
Zhengxun


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 2/3] mtd: spi-nor: macronix: add support for Macronix octaflash series
  2021-04-27  2:47   ` Pratyush Yadav
@ 2021-05-04  5:38     ` zhengxunli
  0 siblings, 0 replies; 12+ messages in thread
From: zhengxunli @ 2021-05-04  5:38 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: broonie, jaimeliao, linux-mtd, linux-spi, miquel.raynal, tudor.ambarus


Hi,

> 
> On 20/04/21 02:29PM, Zhengxun Li wrote:
> > Add 1.8V and 3V Octal NOR Flash IDs.
> > MX25 series : Serial NOR Flash.
> > MX66 series : Serial NOR Flash with stacked die.
> 
> I only looked briefly at the datasheet, but I don't see any mention of 
> stacked dies. I assume the stacked die part is transparent from software 

> point of view, and is only a hardware implementation detail. Correct?
> 
> > LM/UM series : Up to 250MHz clock frequency with both DTR/STR 
operation.
> > LW/UW series : Support simultaneous Read-while-Write operation in 
multiple
> >           bank architecture. Read-while-write feature which means read
> >           data one bank while another bank is programing or erasing.
> 
> AFAIK, this feature is not currently supported in SPI NOR.
>
> > 
> > MX25LM : 3.0V Octal I/O
> >  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7841/
> MX25LM51245G,%203V,%20512Mb,%20v1.1.pdf
> > 
> > MX25UM : 1.8V Octal I/O
> >  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7525/
> MX25UM51245G%20Extreme%20Speed,%201.8V,%20512Mb,%20v1.0.pdf
> > 
> > MX66LM : 3.0V Octal I/O with stacked die
> >  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7929/
> MX66LM1G45G,%203V,%201Gb,%20v1.1.pdf
> > 
> > MX66UM : 1.8V Octal I/O with stacked die
> >  -https://www.mxic.com.tw/Lists/Datasheet/Attachments/7721/
> MX66UM1G45G,%201.8V,%201Gb,%20v1.1.pdf
> > 
> > MX25LW : 3.0V Octal I/O with Read-while-Write
> > MX25UW : 1.8V Octal I/O with Read-while-Write
> > MX66LW : 3.0V Octal I/O with Read-while-Write and stack die
> > MX66UW : 1.8V Octal I/O with Read-while-Write and stack die
> > 
> > About LW/UW series, please contact us freely if you have any
> > questions. For adding Octal NOR Flash IDs, we have validated
> > each Flash on plateform zynq-picozed.
> > 
> > Signed-off-by: Zhengxun Li <zhengxunli@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 100 ++++++++++++++++++++++++++++
> +++++++++++++
> >  1 file changed, 100 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/macronix.c 
b/drivers/mtd/spi-nor/macronix.c
> > index 881eaf8..8c1cf1b 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -203,6 +203,106 @@ static void octaflash_post_sfdp_fixup(struct
> spi_nor *nor)
> >     { "mx66u2g45g",    INFO(0xc2253c, 0, 64 * 1024, 4096,
> >                 SECT_4K | SPI_NOR_DUAL_READ |
> >                 SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> > +   { "mx66lm2g45g", INFO(0xc2853c, 0, 64 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66lm1g45g", INFO(0xc2853b, 0, 32 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66lw1g45g", INFO(0xc2863b, 0, 32 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25lm51245g", INFO(0xc2853a, 0, 16 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25lw51245g", INFO(0xc2863a, 0, 16 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25lm25645g", INFO(0xc28539, 0, 8 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25lw25645g", INFO(0xc28639, 0, 8 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66um2g45g", INFO(0xc2803c, 0, 64 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66uw2g345g", INFO(0xc2843c, 0, 64 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66uw2g345gx0", INFO(0xc2943c, 0, 64 * 1024, 4096,
> > +             SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +             SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66um1g45g", INFO(0xc2803b, 0, 32 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66um1g45g40", INFO(0xc2808b, 0, 32 * 1024, 4096,
> > +            SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +            SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx66uw1g45g", INFO(0xc2813b, 0, 32 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25um51245g", INFO(0xc2803a, 0, 16 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw51245g", INFO(0xc2813a, 0, 16 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw51345g", INFO(0xc2843a, 0, 16 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25um25645g", INFO(0xc28039, 0, 8 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw25645g", INFO(0xc28139, 0, 8 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25um25345g", INFO(0xc28339, 0, 8 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw25345g", INFO(0xc28439, 0, 8 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw12845g", INFO(0xc28138, 0, 4 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw12a45g", INFO(0xc28938, 0, 4 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw12345g", INFO(0xc28438, 0, 4 * 1024, 4096,
> > +                SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +                SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw6445g", INFO(0xc28137, 0, 2 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> > +   { "mx25uw6345g", INFO(0xc28437, 0, 2 * 1024, 4096,
> > +               SECT_4K | SPI_NOR_OCTAL_DTR_READ |
> > +               SPI_NOR_OCTAL_DTR_PP | SPI_NOR_4B_OPCODES)
> > +      .fixups = &octaflash_fixups },
> 
> Reminder to self: Check if there are any ID collisions here. I have seen 

> a couple of them show up recently on Macronix flashes.

Yes, we have checked for ID conflicts before sending the patch.

> Anyway, not much to see here. I suggest you merge this with patch 1.

I will merge this to patch 1 in the next version.

> >  };
> > 
> >  static void macronix_default_init(struct spi_nor *nor)
> 

Thanks,
Zhengxun


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation
  2021-05-04  5:31     ` zhengxunli
@ 2021-05-04  7:42       ` Pratyush Yadav
  2021-05-05  8:29         ` zhengxunli
  0 siblings, 1 reply; 12+ messages in thread
From: Pratyush Yadav @ 2021-05-04  7:42 UTC (permalink / raw)
  To: zhengxunli
  Cc: broonie, jaimeliao, linux-mtd, linux-spi, miquel.raynal, tudor.ambarus

On 04/05/21 01:31PM, zhengxunli@mxic.com.tw wrote:
> Hi Pratyush,
> 
> Thanks for your comment on this patch.
> 
> "Pratyush Yadav" <p.yadav@ti.com> wrote on 2021/04/27 上午 10:36:06:
> 
[...]
> > > +   if (!enable)
> > > +      spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> > 
> > When disabling, the op would be in 8D-8D-8D mode so having a data length 
> 
> > of 1 would be invalid. This is currently the case even in the patches 
> > that I sent for Micron and Cypress.
> > 
> > I am not sure what the correct fix for this is though. One option is to 
> > send the same byte twice, but I remember that on the Cypress flash the 
> > second byte over-writes the register at the next address. I'm not sure 
> > how Macronix flashes handle the second byte. Can you check what the 
> > behavior for your flash is when you write 2 bytes to the register?
> 
> I checked the behavior of Macronix and the second byte will overwrites the 
> register.

Yes, I see the same behaviour on Micron and Cypress flashes. Can your 
controller send a 1-byte write in 8D mode? I am curious if this is 
possible and how flashes react to it.

My theory is that even if you ask the controller to send 1 byte in 8D 
mode, it won't deassert the CS till the end of the cycle. This would 
result the flash in taking the default value of the lines as the second 
byte.

> Do we need to send the same bytes to resolve this error?

I think this is a design oversight by flash manufacturers. Having two 
registers at consecutive addresses is problematic in 8D-8D-8D mode. The 
only solution I see is to read the register at the next address, and set 
its value as the second byte in the transaction. This way its value will 
stay the same at the end of the transaction.

PS: If possible, please try to relay this issue to your hardware design 
team. Hopefully they can come up with a clever solution for future 
devices and make our lives easier ;-)

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation
  2021-05-04  7:42       ` Pratyush Yadav
@ 2021-05-05  8:29         ` zhengxunli
  0 siblings, 0 replies; 12+ messages in thread
From: zhengxunli @ 2021-05-05  8:29 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: broonie, jaimeliao, linux-mtd, linux-spi, miquel.raynal, tudor.ambarus

Hi,

"Pratyush Yadav" <p.yadav@ti.com> wrote on 2021/05/04 下午 03:42:20:

> "Pratyush Yadav" <p.yadav@ti.com> 
> 2021/05/04 下午 03:42
> 
> To
> 
> <zhengxunli@mxic.com.tw>, 
> 
> cc
> 
> <broonie@kernel.org>, <jaimeliao@mxic.com.tw>, <linux-
> mtd@lists.infradead.org>, <linux-spi@vger.kernel.org>, 
> <miquel.raynal@bootlin.com>, <tudor.ambarus@microchip.com>
> 
> Subject
> 
> Re: [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix 
> octal dtr operation
> 
> On 04/05/21 01:31PM, zhengxunli@mxic.com.tw wrote:
> > Hi Pratyush,
> > 
> > Thanks for your comment on this patch.
> > 
> > "Pratyush Yadav" <p.yadav@ti.com> wrote on 2021/04/27 上午 10:36:06:
> > 
> [...]
> > > > +   if (!enable)
> > > > +      spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> > > 
> > > When disabling, the op would be in 8D-8D-8D mode so having a data 
length 
> > 
> > > of 1 would be invalid. This is currently the case even in the 
patches 
> > > that I sent for Micron and Cypress.
> > > 
> > > I am not sure what the correct fix for this is though. One option is 
to 
> > > send the same byte twice, but I remember that on the Cypress flash 
the 
> > > second byte over-writes the register at the next address. I'm not 
sure 
> > > how Macronix flashes handle the second byte. Can you check what the 
> > > behavior for your flash is when you write 2 bytes to the register?
> > 
> > I checked the behavior of Macronix and the second byte will overwrites 
the 
> > register.
> 
> Yes, I see the same behaviour on Micron and Cypress flashes. Can your 
> controller send a 1-byte write in 8D mode? I am curious if this is 
> possible and how flashes react to it.

Our SPI controller can not send 1 byte correctly in 8D mode. However, if 
we
send 2 bytes in 8D mode, the second byte will overwrite the first byte.

> My theory is that even if you ask the controller to send 1 byte in 8D 
> mode, it won't deassert the CS till the end of the cycle. This would 
> result the flash in taking the default value of the lines as the second 
> byte.
> 
> > Do we need to send the same bytes to resolve this error?
> 
> I think this is a design oversight by flash manufacturers. Having two 
> registers at consecutive addresses is problematic in 8D-8D-8D mode. The 
> only solution I see is to read the register at the next address, and set 

> its value as the second byte in the transaction. This way its value will 

> stay the same at the end of the transaction.
> 
> PS: If possible, please try to relay this issue to your hardware design 
> team. Hopefully they can come up with a clever solution for future 
> devices and make our lives easier ;-)

I will try to advise our design team. :=)

Thanks,
Zhengxun



CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-05-05  8:31 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-20  6:29 [PATCH v3 0/3] Add octal DTR support for Macronix flash Zhengxun Li
2021-04-20  6:29 ` [PATCH v3 1/3] mtd: spi-nor: macronix: add support for Macronix octal dtr operation Zhengxun Li
2021-04-27  2:36   ` Pratyush Yadav
2021-05-04  5:31     ` zhengxunli
2021-05-04  7:42       ` Pratyush Yadav
2021-05-05  8:29         ` zhengxunli
2021-04-20  6:29 ` [PATCH v3 2/3] mtd: spi-nor: macronix: add support for Macronix octaflash series Zhengxun Li
2021-04-27  2:47   ` Pratyush Yadav
2021-05-04  5:38     ` zhengxunli
2021-04-20  6:29 ` [PATCH v3 3/3] spi: mxic: patch for octal DTR mode support Zhengxun Li
2021-04-26 16:53   ` Mark Brown
2021-04-27  1:48     ` zhengxunli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).