[PATCH v10 0/6] Add octal DTR support for Macronix flash

[PATCH v10 0/6] Add octal DTR support for Macronix flash

[AlvinZhou]

From: AlvinZhou <alvinzhou@mxic.com.tw>

Add method for Macronix Octal DTR Enable/Disable.
Merge Tudor's patch "Allow specifying the byte order in DTR mode"

v10:
* Further explanation on adding Macronix manufacturer ID in ID table.
* Correct some typos.

v9:
* Change the name of the configuration register 2 for Macronix Octal 
  flash.
* Fix the bit value in __pad of struct spi_mem_op.
* Use the local variable proto instead of nor->read_proto.

v8:
* Supplement missing S-o-b
* Remove function spi_nor_is_octal_dtr_swab16
* Split IDs by MX25 & MX66
* Add dump of capability in debugfs
* Add dump of parameters in debugfs
* Add dump of result for mtd-utils tests
* Add SNOR_ID(0xC2) in last of Macronix ID table

v7:
* Add dtr_swab16 judgement to enable/disable Macronix xSPI host
  controller swap byte feature.

v6:
* Add byte swap support for spi-mxic.c
* Remove flash name in ID table.

v5:
* Remove manufacturer read id function.
* For increased readability, separate Flash IDs based on whether
  it supports RWW feature.

v4:
* Add patch for adding manufacturer read id function.
  remove patch "hook manufacturer by checking first byte id" 

v3:
* Add patch for hook manufacturer by comparing ID 1st byte.
* Add patches for specifying the byte order in DTR mode by merging
  Tudor's patch.

v2:
* Following exsiting rules to re-create Macronix specify Octal DTR 
  method.
* change signature to jaimeliao@mxic.com.tw
* Clear sector size information in flash INFO.

AlvinZhou (6):
  mtd: spi-nor: add Octal DTR support for Macronix flash
  spi: spi-mem: Allow specifying the byte order in Octal DTR mode
  mtd: spi-nor: core: Allow specifying the byte order in Octal DTR mode
  mtd: spi-nor: sfdp: Get the 8D-8D-8D byte order from BFPT
  spi: mxic: Add support for swapping byte
  mtd: spi-nor: add support for Macronix Octal flash

 drivers/mtd/spi-nor/core.c     |  4 ++
 drivers/mtd/spi-nor/core.h     |  1 +
 drivers/mtd/spi-nor/macronix.c | 95 +++++++++++++++++++++++++++++++++-
 drivers/mtd/spi-nor/sfdp.c     |  4 ++
 drivers/mtd/spi-nor/sfdp.h     |  1 +
 drivers/spi/spi-mem.c          |  3 ++
 drivers/spi/spi-mxic.c         | 17 ++++--
 include/linux/spi/spi-mem.h    |  8 ++-
 8 files changed, 127 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v10 1/6] mtd: spi-nor: add Octal DTR support for Macronix flash

[AlvinZhou]

From: AlvinZhou <alvinzhou@mxic.com.tw>

Create Macronix specify method for enable Octal DTR mode and
set 20 dummy cycles to allow running at the maximum supported
frequency for Macronix Octal flash.

Use number of dummy cycles which is parse by SFDP then convert
it to bit pattern and set in CR2 register.
Set CR2 register for enable octal DTR mode.

Use Read ID to confirm that enabling/disabling octal DTR mode
was successful.

Macronix ID format is A-A-B-B-C-C in octal DTR mode.
To ensure the successful enablement of octal DTR mode, confirm
that the 6-byte data is entirely correct.

Co-developed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
---
 drivers/mtd/spi-nor/macronix.c | 91 ++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index ea6be95e75a5..f039819a5252 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,24 @@
 
 #include "core.h"
 
+#define MXIC_NOR_OP_RD_CR2	0x71		/* Read configuration register 2 opcode */
+#define MXIC_NOR_OP_WR_CR2	0x72		/* Write configuration register 2 opcode */
+#define MXIC_NOR_ADDR_CR2_MODE	0x00000000	/* CR2 address for setting spi/sopi/dopi mode */
+#define MXIC_NOR_ADDR_CR2_DC	0x00000300	/* CR2 address for setting dummy cycles */
+#define MXIC_NOR_REG_DOPI_EN	0x2		/* Enable Octal DTR */
+#define MXIC_NOR_REG_SPI_EN	0x0		/* Enable SPI */
+
+/* Convert dummy cycles to bit pattern */
+#define MXIC_NOR_REG_DC(p) \
+	((20 - (p)) >> 1)
+
+/* Macronix write CR2 operations */
+#define MXIC_NOR_WR_CR2(addr, ndata, buf)			\
+	SPI_MEM_OP(SPI_MEM_OP_CMD(MXIC_NOR_OP_WR_CR2, 0),	\
+		   SPI_MEM_OP_ADDR(4, addr, 0),			\
+		   SPI_MEM_OP_NO_DUMMY,				\
+		   SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -185,6 +203,78 @@ static const struct flash_info macronix_nor_parts[] = {
 	}
 };
 
+static int macronix_nor_octal_dtr_en(struct spi_nor *nor)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf, i;
+	int ret;
+
+	/* Use dummy cycles which is parse by SFDP and convert to bit pattern. */
+	buf[0] = MXIC_NOR_REG_DC(nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].num_wait_states);
+	op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_DC, 1, buf);
+	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
+	if (ret)
+		return ret;
+
+	/* Set the octal and DTR enable bits. */
+	buf[0] = MXIC_NOR_REG_DOPI_EN;
+	op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_MODE, 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, 4, 4, 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;
+	}
+
+	/* Macronix SPI-NOR flash 8D-8D-8D read ID would get 6 bytes data A-A-B-B-C-C */
+	for (i = 0; i < nor->info->id->len; i++)
+		if (buf[i * 2] != buf[(i * 2) + 1] || buf[i * 2] != nor->info->id->bytes[i])
+			return -EINVAL;
+
+	return 0;
+}
+
+static int macronix_nor_octal_dtr_dis(struct spi_nor *nor)
+{
+	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] = MXIC_NOR_REG_SPI_EN;
+	buf[1] = 0x0;
+	op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_MODE, 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 (memcmp(buf, nor->info->id->bytes, nor->info->id->len))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int macronix_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
+{
+	return enable ? macronix_nor_octal_dtr_en(nor) : macronix_nor_octal_dtr_dis(nor);
+}
+
 static void macronix_nor_default_init(struct spi_nor *nor)
 {
 	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
@@ -194,6 +284,7 @@ static int macronix_nor_late_init(struct spi_nor *nor)
 {
 	if (!nor->params->set_4byte_addr_mode)
 		nor->params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b;
+	nor->params->set_octal_dtr = macronix_nor_set_octal_dtr;
 
 	return 0;
 }
-- 
2.25.1

[PATCH v10 2/6] spi: spi-mem: Allow specifying the byte order in Octal DTR mode

[AlvinZhou]

From: AlvinZhou <alvinzhou@mxic.com.tw>

From: Tudor Ambarus <tudor.ambarus@linaro.org>

There are NOR flashes (Macronix) that swap the bytes on a 16-bit
boundary when configured in Octal DTR mode. The byte order of
16-bit words is swapped when read or written in Octal Double
Transfer Rate (DTR) mode compared to Single Transfer Rate (STR)
modes. If one writes D0 D1 D2 D3 bytes using 1-1-1 mode, and uses
8D-8D-8D SPI mode for reading, it will read back D1 D0 D3 D2.
Swapping the bytes may introduce some endianness problems. It can
affect the boot sequence if the entire boot sequence is not handled
in either 8D-8D-8D mode or 1-1-1 mode. Therefore, it is necessary
to swap the bytes back to ensure the same byte order as in STR modes.
Fortunately there are controllers that could swap the bytes back at
runtime, addressing the flash's endianness requirements. Provide a
way for the upper layers to specify the byte order in Octal DTR mode.

Merge Tudor's patch and add modifications for suiting newer version
of Linux kernel.

Suggested-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
Acked-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mem.c       | 3 +++
 include/linux/spi/spi-mem.h | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 17b8baf749e6..abc6792e738c 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -172,6 +172,9 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
 		if (!spi_mem_controller_is_capable(ctlr, dtr))
 			return false;
 
+		if (op->data.swap16 && !spi_mem_controller_is_capable(ctlr, swap16))
+			return false;
+
 		if (op->cmd.nbytes != 2)
 			return false;
 	} else {
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index f866d5c8ed32..c46d2b8029be 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -90,6 +90,8 @@ enum spi_mem_data_dir {
  * @data.buswidth: number of IO lanes used to send/receive the data
  * @data.dtr: whether the data should be sent in DTR mode or not
  * @data.ecc: whether error correction is required or not
+ * @data.swap16: whether the byte order of 16-bit words is swapped when read
+ *		 or written in Octal DTR mode compared to STR mode.
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
  *		 operation does not involve transferring data
@@ -124,7 +126,8 @@ struct spi_mem_op {
 		u8 buswidth;
 		u8 dtr : 1;
 		u8 ecc : 1;
-		u8 __pad : 6;
+		u8 swap16 : 1;
+		u8 __pad : 5;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
 		union {
@@ -297,10 +300,13 @@ struct spi_controller_mem_ops {
  * struct spi_controller_mem_caps - SPI memory controller capabilities
  * @dtr: Supports DTR operations
  * @ecc: Supports operations with error correction
+ * @swap16: Supports swapping bytes on a 16 bit boundary when configured in
+ *	    Octal DTR
  */
 struct spi_controller_mem_caps {
 	bool dtr;
 	bool ecc;
+	bool swap16;
 };
 
 #define spi_mem_controller_is_capable(ctlr, cap)	\
-- 
2.25.1

[PATCH v10 3/6] mtd: spi-nor: core: Allow specifying the byte order in Octal DTR mode

[AlvinZhou]

From: AlvinZhou <alvinzhou@mxic.com.tw>

From: Tudor Ambarus <tudor.ambarus@linaro.org>

Macronix swaps bytes on a 16-bit boundary when configured in Octal DTR.
The byte order of 16-bit words is swapped when read or written in 8D-8D-8D
mode compared to STR modes. Allow operations to specify the byte order in
DTR mode, so that controllers can swap the bytes back at run-time to
address the flash's endianness requirements, if they are capable. If the
controller is not capable of swapping the bytes, the protocol is downgrade
via spi_nor_spimem_adjust_hwcaps(). When available, the swapping of the
bytes is always done regardless if it's a data or register access, so that
it comply with the JESD216 requirements: "Byte order of 16-bit words is
swapped when read in 8D-8D-8D mode compared to 1-1-1".

Merge Tudor's patch and add modifications for suiting newer version
of Linux kernel.

Suggested-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
---
 drivers/mtd/spi-nor/core.c | 4 ++++
 drivers/mtd/spi-nor/core.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 028514c6996f..31f57b17023f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -113,6 +113,10 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 		op->cmd.opcode = (op->cmd.opcode << 8) | ext;
 		op->cmd.nbytes = 2;
 	}
+
+	/* SWAP16 is only applicable when Octal DTR mode */
+	if (proto == SNOR_PROTO_8_8_8_DTR && nor->flags & SNOR_F_SWAP16)
+		op->data.swap16 = true;
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 442786685515..baf6c4b5912b 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -140,6 +140,7 @@ enum spi_nor_option_flags {
 	SNOR_F_RWW		= BIT(14),
 	SNOR_F_ECC		= BIT(15),
 	SNOR_F_NO_WP		= BIT(16),
+	SNOR_F_SWAP16		= BIT(17),
 };
 
 struct spi_nor_read_command {
-- 
2.25.1

[PATCH v10 4/6] mtd: spi-nor: sfdp: Get the 8D-8D-8D byte order from BFPT

[AlvinZhou]

From: AlvinZhou <alvinzhou@mxic.com.tw>

From: Tudor Ambarus <tudor.ambarus@linaro.org>

Parse BFPT in order to retrieve the byte order in 8D-8D-8D mode.

This info flag will be used as a basis to determine whether
there is byte swapping of data for SPI NOR flash in octal
DTR mode.

The controller driver will check whether byte swapping is supported
to determine whether the corresponding operation are supported,
thus avoiding the generation of unexpected data order.

Merge Tudor's patch and add modifications for suiting newer version
of Linux kernel.

Reviewed-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
---
 drivers/mtd/spi-nor/sfdp.c | 4 ++++
 drivers/mtd/spi-nor/sfdp.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 5b1117265bd2..21727f9a4ac6 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -671,6 +671,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		return -EOPNOTSUPP;
 	}
 
+	/* Byte order in 8D-8D-8D mode */
+	if (bfpt.dwords[SFDP_DWORD(18)] & BFPT_DWORD18_BYTE_ORDER_SWAPPED)
+		nor->flags |= SNOR_F_SWAP16;
+
 	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
 }
 
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index da0fe5aa9bb0..d90cbd7331f7 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -130,6 +130,7 @@ struct sfdp_bfpt {
 #define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
 #define BFPT_DWORD18_CMD_EXT_RES		(0x2UL << 29) /* Reserved */
 #define BFPT_DWORD18_CMD_EXT_16B		(0x3UL << 29) /* 16-bit opcode */
+#define BFPT_DWORD18_BYTE_ORDER_SWAPPED		BIT(31)	/* Byte sawp of 16-bit in 8D-8D-8D mode */
 
 struct sfdp_parameter_header {
 	u8		id_lsb;
-- 
2.25.1

[PATCH v10 5/6] spi: mxic: Add support for swapping byte

[AlvinZhou]

From: AlvinZhou <alvinzhou@mxic.com.tw>

Some SPI-NOR flash swap the bytes on a 16-bit boundary when
configured in Octal DTR mode. It means data format D0 D1 D2 D3
would be swapped to D1 D0 D3 D2. So that whether controller
support swapping bytes should be checked before enable Octal
DTR mode. Add swap byte support on a 16-bit boundary when
configured in Octal DTR mode for Macronix xSPI host controller
driver.

According dtr_swab in operation to enable/disable Macronix
xSPI host controller swap byte feature.

To make sure swap byte feature is working well, program data in
1S-1S-1S mode then read back and compare read data in 8D-8D-8D
mode.

This feature have been validated on byte-swap flash and
non-byte-swap flash.

Macronix xSPI host controller bit "HC_CFG_DATA_PASS" determine
the byte swap feature disabled/enabled and swap byte feature is
working on 8D-8D-8D mode only.

Suggested-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
Acked-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mxic.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 6156d691630a..a669ffa27b65 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -294,7 +294,8 @@ static void mxic_spi_hw_init(struct mxic_spi *mxic)
 	       mxic->regs + HC_CFG);
 }
 
-static u32 mxic_spi_prep_hc_cfg(struct spi_device *spi, u32 flags)
+static u32 mxic_spi_prep_hc_cfg(struct spi_device *spi, u32 flags,
+				bool swap16)
 {
 	int nio = 1;
 
@@ -305,6 +306,11 @@ static u32 mxic_spi_prep_hc_cfg(struct spi_device *spi, u32 flags)
 	else if (spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
 		nio = 2;
 
+	if (swap16)
+		flags &= ~HC_CFG_DATA_PASS;
+	else
+		flags |= HC_CFG_DATA_PASS;
+
 	return flags | HC_CFG_NIO(nio) |
 	       HC_CFG_TYPE(spi_get_chipselect(spi, 0), HC_CFG_TYPE_SPI_NOR) |
 	       HC_CFG_SLV_ACT(spi_get_chipselect(spi, 0)) | HC_CFG_IDLE_SIO_LVL(1);
@@ -397,7 +403,8 @@ static ssize_t mxic_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
 	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
 		return -EINVAL;
 
-	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0), mxic->regs + HC_CFG);
+	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0, desc->info.op_tmpl.data.swap16),
+	       mxic->regs + HC_CFG);
 
 	writel(mxic_spi_mem_prep_op_cfg(&desc->info.op_tmpl, len),
 	       mxic->regs + LRD_CFG);
@@ -441,7 +448,8 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
 	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
 		return -EINVAL;
 
-	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0), mxic->regs + HC_CFG);
+	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0, desc->info.op_tmpl.data.swap16),
+	       mxic->regs + HC_CFG);
 
 	writel(mxic_spi_mem_prep_op_cfg(&desc->info.op_tmpl, len),
 	       mxic->regs + LWR_CFG);
@@ -518,7 +526,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	if (ret)
 		return ret;
 
-	writel(mxic_spi_prep_hc_cfg(mem->spi, HC_CFG_MAN_CS_EN),
+	writel(mxic_spi_prep_hc_cfg(mem->spi, HC_CFG_MAN_CS_EN, op->data.swap16),
 	       mxic->regs + HC_CFG);
 
 	writel(HC_EN_BIT, mxic->regs + HC_EN);
@@ -573,6 +581,7 @@ static const struct spi_controller_mem_ops mxic_spi_mem_ops = {
 static const struct spi_controller_mem_caps mxic_spi_mem_caps = {
 	.dtr = true,
 	.ecc = true,
+	.swap16 = true,
 };
 
 static void mxic_spi_set_cs(struct spi_device *spi, bool lvl)
-- 
2.25.1

[PATCH v10 6/6] mtd: spi-nor: add support for Macronix Octal flash

[AlvinZhou]

From: AlvinZhou <alvinzhou@mxic.com.tw>

Adding manufacturer ID 0xC2 at the end of ID table
to allow manufacturer fixup to be applied for any
Macronix flashes instead of needing to list each
flash ID in the ID table.

Such as macronix_nor_set_octal_dtr function in the
manufacturer fixup can be applied to any Macronix
Octal Flashes without the need to add the specific
ID in the ID table.

Suggested-by: Michael Walle <mwalle@kernel.org>
Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
---
 drivers/mtd/spi-nor/macronix.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index f039819a5252..1a8ccebdfe0e 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
 		.name = "mx25l3255e",
 		.size = SZ_4M,
 		.no_sfdp_flags = SECT_4K,
-	}
+	},
+	/* Need the manufacturer fixups, Keep this last */
+	{ .id = SNOR_ID(0xc2) }
 };
 
 static int macronix_nor_octal_dtr_en(struct spi_nor *nor)
-- 
2.25.1

Re: [PATCH v10 1/6] mtd: spi-nor: add Octal DTR support for Macronix flash

[Tudor Ambarus]

On 26.09.2024 17:19, AlvinZhou wrote:
> From: AlvinZhou <alvinzhou@mxic.com.tw>
> 
> Create Macronix specify method for enable Octal DTR mode and
> set 20 dummy cycles to allow running at the maximum supported
> frequency for Macronix Octal flash.
> 
> Use number of dummy cycles which is parse by SFDP then convert
> it to bit pattern and set in CR2 register.
> Set CR2 register for enable octal DTR mode.
> 
> Use Read ID to confirm that enabling/disabling octal DTR mode
> was successful.
> 
> Macronix ID format is A-A-B-B-C-C in octal DTR mode.
> To ensure the successful enablement of octal DTR mode, confirm
> that the 6-byte data is entirely correct.
> 
> Co-developed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/macronix.c | 91 ++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index ea6be95e75a5..f039819a5252 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,24 @@
>  
>  #include "core.h"
>  
> +#define MXIC_NOR_OP_RD_CR2	0x71		/* Read configuration register 2 opcode */
> +#define MXIC_NOR_OP_WR_CR2	0x72		/* Write configuration register 2 opcode */
> +#define MXIC_NOR_ADDR_CR2_MODE	0x00000000	/* CR2 address for setting spi/sopi/dopi mode */
> +#define MXIC_NOR_ADDR_CR2_DC	0x00000300	/* CR2 address for setting dummy cycles */
> +#define MXIC_NOR_REG_DOPI_EN	0x2		/* Enable Octal DTR */
> +#define MXIC_NOR_REG_SPI_EN	0x0		/* Enable SPI */
> +
> +/* Convert dummy cycles to bit pattern */
> +#define MXIC_NOR_REG_DC(p) \
> +	((20 - (p)) >> 1)

This is unfortunate as we convert dummy cycles to bytes in mtd and then
we convert back from bytes to cycles in spi. I had an attempt fixing
this in the past, but couldn't allocate more time for spinning another
version for the patch set.

I won't block the patch set for this, but if someone cares to fix it,
would be great to take over.

> +
> +/* Macronix write CR2 operations */

I'll drop this comment when applying, as I can already see what the
macro is doing from its name.

> +#define MXIC_NOR_WR_CR2(addr, ndata, buf)			\
> +	SPI_MEM_OP(SPI_MEM_OP_CMD(MXIC_NOR_OP_WR_CR2, 0),	\
> +		   SPI_MEM_OP_ADDR(4, addr, 0),			\
> +		   SPI_MEM_OP_NO_DUMMY,				\
> +		   SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
> +
>  static int
>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>  			    const struct sfdp_parameter_header *bfpt_header,
> @@ -185,6 +203,78 @@ static const struct flash_info macronix_nor_parts[] = {
>  	}
>  };
>  
> +static int macronix_nor_octal_dtr_en(struct spi_nor *nor)
> +{
> +	struct spi_mem_op op;
> +	u8 *buf = nor->bouncebuf, i;
> +	int ret;
> +
> +	/* Use dummy cycles which is parse by SFDP and convert to bit pattern. */
> +	buf[0] = MXIC_NOR_REG_DC(nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].num_wait_states);
> +	op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_DC, 1, buf);
> +	ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the octal and DTR enable bits. */
> +	buf[0] = MXIC_NOR_REG_DOPI_EN;
> +	op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_MODE, 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, 4, 4, buf, SNOR_PROTO_8_8_8_DTR);

can we use nor->addr_nbytes for the second argument? Please test and
confirm. No need to resend for this, just confirm and I can amend when
applying.

What about the third argument, the number of dummy nbytes. Can we get
the cycles needed for READID from somewhere in SFDP?

Looks good.

Re: [PATCH v10 3/6] mtd: spi-nor: core: Allow specifying the byte order in Octal DTR mode

[Tudor Ambarus]

On 26.09.2024 17:19, AlvinZhou wrote:
> +
> +	/* SWAP16 is only applicable when Octal DTR mode */

the comment is redundant, I can already see the condition in the if
below. No need to resend, I'll amend when applying. Looking good.

> +	if (proto == SNOR_PROTO_8_8_8_DTR && nor->flags & SNOR_F_SWAP16)
> +		op->data.swap16 = true;

Re: [PATCH v10 4/6] mtd: spi-nor: sfdp: Get the 8D-8D-8D byte order from BFPT

[Tudor Ambarus]

On 26.09.2024 17:19, AlvinZhou wrote:
> +#define BFPT_DWORD18_BYTE_ORDER_SWAPPED		BIT(31)	/* Byte sawp of 16-bit in 8D-8D-8D mode */
typo: sawp. I'll replace the comment with
/* Byte order swapped in 8D-8D-8D mode */

No need to resend.

Re: [PATCH v10 6/6] mtd: spi-nor: add support for Macronix Octal flash

[Tudor Ambarus]

On 26.09.2024 17:19, AlvinZhou wrote:
> From: AlvinZhou <alvinzhou@mxic.com.tw>
> 
> Adding manufacturer ID 0xC2 at the end of ID table
> to allow manufacturer fixup to be applied for any
> Macronix flashes instead of needing to list each
> flash ID in the ID table.
> 
> Such as macronix_nor_set_octal_dtr function in the
> manufacturer fixup can be applied to any Macronix
> Octal Flashes without the need to add the specific
> ID in the ID table.
> 
> Suggested-by: Michael Walle <mwalle@kernel.org>
> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/macronix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index f039819a5252..1a8ccebdfe0e 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
>  		.name = "mx25l3255e",
>  		.size = SZ_4M,
>  		.no_sfdp_flags = SECT_4K,
> -	}
> +	},
> +	/* Need the manufacturer fixups, Keep this last */

you have a capital letter in the middle of the sentence.

I'll replace the comment with:

/*

 * This spares us of adding new flash entries for flashes that can be
 * initialized solely based on the SFDP data, but still need the
 * manufacturer hooks to set parameters that can't be discovered at SFDP
 * parsing time.
 */

Which brings me to why you really set this. I remember SFDP contains
tables with sequence of commands for enabling/disabling Octal DTR mode.
Would you please remember me, why you didn't use those SFDP tables and
implemented your own enable/disable methods?

> +	{ .id = SNOR_ID(0xc2) }
>  };
>  
>  static int macronix_nor_octal_dtr_en(struct spi_nor *nor)

Re: [PATCH v10 0/6] Add octal DTR support for Macronix flash

[Tudor Ambarus]

On Thu, 26 Sep 2024 22:19:50 +0800, AlvinZhou wrote:
> Add method for Macronix Octal DTR Enable/Disable.
> Merge Tudor's patch "Allow specifying the byte order in DTR mode"
> 
> v10:
> * Further explanation on adding Macronix manufacturer ID in ID table.
> * Correct some typos.
> 
> [...]

Made the changes that I specified in replies. SFDP
"Command Sequences to Change to Octal DDR (8D-8D-8D) Mode" can be parsed
later on.

Applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
spi-nor/next branch. Thanks!

[1/6] mtd: spi-nor: add Octal DTR support for Macronix flash
      https://git.kernel.org/mtd/c/ccac858d2bdb
[2/6] spi: spi-mem: Allow specifying the byte order in Octal DTR mode
      https://git.kernel.org/mtd/c/030ace430afc
[3/6] mtd: spi-nor: core: Allow specifying the byte order in Octal DTR mode
      https://git.kernel.org/mtd/c/6a42bc97ccda
[4/6] mtd: spi-nor: sfdp: Get the 8D-8D-8D byte order from BFPT
      https://git.kernel.org/mtd/c/46b6256a68b4
[5/6] spi: mxic: Add support for swapping byte
      https://git.kernel.org/mtd/c/50cb86f21ec2
[6/6] mtd: spi-nor: add support for Macronix Octal flash
      https://git.kernel.org/mtd/c/afe1ea1344bb

Cheers,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>

Re: [PATCH v10 1/6] mtd: spi-nor: add Octal DTR support for Macronix flash

[Alvin Zhou]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年10月2日 週三 下午3:16寫道：
>
>
>
> On 26.09.2024 17:19, AlvinZhou wrote:
> > From: AlvinZhou <alvinzhou@mxic.com.tw>
> >
> > Create Macronix specify method for enable Octal DTR mode and
> > set 20 dummy cycles to allow running at the maximum supported
> > frequency for Macronix Octal flash.
> >
> > Use number of dummy cycles which is parse by SFDP then convert
> > it to bit pattern and set in CR2 register.
> > Set CR2 register for enable octal DTR mode.
> >
> > Use Read ID to confirm that enabling/disabling octal DTR mode
> > was successful.
> >
> > Macronix ID format is A-A-B-B-C-C in octal DTR mode.
> > To ensure the successful enablement of octal DTR mode, confirm
> > that the 6-byte data is entirely correct.
> >
> > Co-developed-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> > Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 91 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> > index ea6be95e75a5..f039819a5252 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -8,6 +8,24 @@
> >
> >  #include "core.h"
> >
> > +#define MXIC_NOR_OP_RD_CR2   0x71            /* Read configuration register 2 opcode */
> > +#define MXIC_NOR_OP_WR_CR2   0x72            /* Write configuration register 2 opcode */
> > +#define MXIC_NOR_ADDR_CR2_MODE       0x00000000      /* CR2 address for setting spi/sopi/dopi mode */
> > +#define MXIC_NOR_ADDR_CR2_DC 0x00000300      /* CR2 address for setting dummy cycles */
> > +#define MXIC_NOR_REG_DOPI_EN 0x2             /* Enable Octal DTR */
> > +#define MXIC_NOR_REG_SPI_EN  0x0             /* Enable SPI */
> > +
> > +/* Convert dummy cycles to bit pattern */
> > +#define MXIC_NOR_REG_DC(p) \
> > +     ((20 - (p)) >> 1)
>
> This is unfortunate as we convert dummy cycles to bytes in mtd and then
> we convert back from bytes to cycles in spi. I had an attempt fixing
> this in the past, but couldn't allocate more time for spinning another
> version for the patch set.
>
> I won't block the patch set for this, but if someone cares to fix it,
> would be great to take over.
>
> > +
> > +/* Macronix write CR2 operations */
>
> I'll drop this comment when applying, as I can already see what the
> macro is doing from its name.

Got it, I will pay attention to it, thanks!

>
> > +#define MXIC_NOR_WR_CR2(addr, ndata, buf)                    \
> > +     SPI_MEM_OP(SPI_MEM_OP_CMD(MXIC_NOR_OP_WR_CR2, 0),       \
> > +                SPI_MEM_OP_ADDR(4, addr, 0),                 \
> > +                SPI_MEM_OP_NO_DUMMY,                         \
> > +                SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
> > +
> >  static int
> >  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
> >                           const struct sfdp_parameter_header *bfpt_header,
> > @@ -185,6 +203,78 @@ static const struct flash_info macronix_nor_parts[] = {
> >       }
> >  };
> >
> > +static int macronix_nor_octal_dtr_en(struct spi_nor *nor)
> > +{
> > +     struct spi_mem_op op;
> > +     u8 *buf = nor->bouncebuf, i;
> > +     int ret;
> > +
> > +     /* Use dummy cycles which is parse by SFDP and convert to bit pattern. */
> > +     buf[0] = MXIC_NOR_REG_DC(nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].num_wait_states);
> > +     op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_DC, 1, buf);
> > +     ret = spi_nor_write_any_volatile_reg(nor, &op, nor->reg_proto);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Set the octal and DTR enable bits. */
> > +     buf[0] = MXIC_NOR_REG_DOPI_EN;
> > +     op = (struct spi_mem_op)MXIC_NOR_WR_CR2(MXIC_NOR_ADDR_CR2_MODE, 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, 4, 4, buf, SNOR_PROTO_8_8_8_DTR);
>
> can we use nor->addr_nbytes for the second argument? Please test and
> confirm. No need to resend for this, just confirm and I can amend when
> applying.

The following is the process of spi_nor_scan()
int spi_nor_scan(...)
{
......
ret = spi_nor_init_params(nor);
......
ret = spi_nor_setup(nor, hwcaps);
......
}
First, within the spi_nor_parse_sfdp() function inside
spi_nor_init_params(): nor->params->addr_nbytes is set based on the
SFDP, while nor->addr_nbytes is not available. Therefore, the second
argument cannot use nor->addr_nbytes but can use
nor->params->addr_nbytes. Additionally, For Macronix Octal NOR Flash
in Octal DDR mode, both the address and dummy cycles are fixed at 4
in READID, so setting the second and third argument to 4 is also valid.
Moreover, nor->addr_nbytes is set within the spi_nor_setup() function.

>
> What about the third argument, the number of dummy nbytes. Can we get
> the cycles needed for READID from somewhere in SFDP?

Currently, the SFDP does not provide the number of dummy cycles for the
READID.

>
> Looks good.

Thanks,
Alvin

Re: [PATCH v10 1/6] mtd: spi-nor: add Octal DTR support for Macronix flash

[Tudor Ambarus]

On 10/4/24 10:05 AM, Alvin Zhou wrote:
>>> +     /* Read flash ID to make sure the switch was successful. */
>>> +     ret = spi_nor_read_id(nor, 4, 4, buf, SNOR_PROTO_8_8_8_DTR);
>> can we use nor->addr_nbytes for the second argument? Please test and
>> confirm. No need to resend for this, just confirm and I can amend when
>> applying.
> The following is the process of spi_nor_scan()
> int spi_nor_scan(...)
> {
> ......
> ret = spi_nor_init_params(nor);
> ......
> ret = spi_nor_setup(nor, hwcaps);
> ......
> }
> First, within the spi_nor_parse_sfdp() function inside
> spi_nor_init_params(): nor->params->addr_nbytes is set based on the
> SFDP, while nor->addr_nbytes is not available. Therefore, the second
> argument cannot use nor->addr_nbytes but can use
> nor->params->addr_nbytes. Additionally, For Macronix Octal NOR Flash


nor->addr_nbytes is set in spi_nor_setup().
spi_nor_set_octal_dtr() is called after spi_nor_setup(), thus you can
use nor->addr_nbytes.

> in Octal DDR mode, both the address and dummy cycles are fixed at 4
> in READID, so setting the second and third argument to 4 is also valid.

but we don't want magic numbers or states that are not tracked, so use
the parameters set

> Moreover, nor->addr_nbytes is set within the spi_nor_setup() function.

yep, use it then.

Re: [PATCH v10 1/6] mtd: spi-nor: add Octal DTR support for Macronix flash

[Alvin Zhou]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年10月4日 週五 下午5:58寫道：
>
>
>
> On 10/4/24 10:05 AM, Alvin Zhou wrote:
> >>> +     /* Read flash ID to make sure the switch was successful. */
> >>> +     ret = spi_nor_read_id(nor, 4, 4, buf, SNOR_PROTO_8_8_8_DTR);
> >> can we use nor->addr_nbytes for the second argument? Please test and
> >> confirm. No need to resend for this, just confirm and I can amend when
> >> applying.
> > The following is the process of spi_nor_scan()
> > int spi_nor_scan(...)
> > {
> > ......
> > ret = spi_nor_init_params(nor);
> > ......
> > ret = spi_nor_setup(nor, hwcaps);
> > ......
> > }
> > First, within the spi_nor_parse_sfdp() function inside
> > spi_nor_init_params(): nor->params->addr_nbytes is set based on the
> > SFDP, while nor->addr_nbytes is not available. Therefore, the second
> > argument cannot use nor->addr_nbytes but can use
> > nor->params->addr_nbytes. Additionally, For Macronix Octal NOR Flash
>
>
> nor->addr_nbytes is set in spi_nor_setup().
> spi_nor_set_octal_dtr() is called after spi_nor_setup(), thus you can
> use nor->addr_nbytes.

I apologize for the misunderstanding. Thanks for your clarification. So
using nor->addr_nbytes as the second argument is not a problem. I
have verified, thanks!
>
> > in Octal DDR mode, both the address and dummy cycles are fixed at 4
> > in READID, so setting the second and third argument to 4 is also valid.
>
> but we don't want magic numbers or states that are not tracked, so use
> the parameters set
>
> > Moreover, nor->addr_nbytes is set within the spi_nor_setup() function.
>
> yep, use it then.

Thanks,
Alvin

Re: [PATCH v10 6/6] mtd: spi-nor: add support for Macronix Octal flash

[Alvin Zhou]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@linaro.org> 於 2024年10月2日 週三 下午3:45寫道：
>
>
>
> On 26.09.2024 17:19, AlvinZhou wrote:
> > From: AlvinZhou <alvinzhou@mxic.com.tw>
> >
> > Adding manufacturer ID 0xC2 at the end of ID table
> > to allow manufacturer fixup to be applied for any
> > Macronix flashes instead of needing to list each
> > flash ID in the ID table.
> >
> > Such as macronix_nor_set_octal_dtr function in the
> > manufacturer fixup can be applied to any Macronix
> > Octal Flashes without the need to add the specific
> > ID in the ID table.
> >
> > Suggested-by: Michael Walle <mwalle@kernel.org>
> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw>
> > Signed-off-by: AlvinZhou <alvinzhou@mxic.com.tw>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> > index f039819a5252..1a8ccebdfe0e 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
> >               .name = "mx25l3255e",
> >               .size = SZ_4M,
> >               .no_sfdp_flags = SECT_4K,
> > -     }
> > +     },
> > +     /* Need the manufacturer fixups, Keep this last */
>
> you have a capital letter in the middle of the sentence.
>
> I'll replace the comment with:
>
> /*
>
>  * This spares us of adding new flash entries for flashes that can be
>  * initialized solely based on the SFDP data, but still need the
>  * manufacturer hooks to set parameters that can't be discovered at SFDP
>  * parsing time.
>  */
>
> Which brings me to why you really set this. I remember SFDP contains
> tables with sequence of commands for enabling/disabling Octal DTR mode.
> Would you please remember me, why you didn't use those SFDP tables and
> implemented your own enable/disable methods?

While the SFDP does provide a sequence of commands to enable Octal
DDR mode, following this sequence forces the I/O driver strength to 50 ohms,
which causes I/O driver strength to be weak and and leads to read/write
issues, so we chose to use a fixup approach to enable/disable Octal DDR
mode.

Thanks,
Alvin