[PATCH v2 0/4] Add support for SPI-NOR Macronix OTP

[PATCH v2 0/4] Add support for SPI-NOR Macronix OTP

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Add support for SPI-NOR Macronix OTP.
And add mx25l12833f with OTP.

v2:
Improve description of mx25l12833f.
Add note about mx25l12833f using the same JEDEC ID as mx25l12805d.

Erez Geva (4):
  Add generic functions for accessing the SPI-NOR chip.
  Add support for SPI-NOR Macronix OTP.
  dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip
  Add Macronix SPI-NOR mx25l12833f with OTP.

 .../bindings/mtd/jedec,spi-nor.yaml           |   2 +-
 drivers/mtd/spi-nor/core.c                    | 131 ++++++++----
 drivers/mtd/spi-nor/core.h                    |  27 +--
 drivers/mtd/spi-nor/macronix.c                | 190 ++++++++++++++++++
 include/linux/mtd/spi-nor.h                   |  10 +
 5 files changed, 301 insertions(+), 59 deletions(-)

-- 
2.39.2

[PATCH v2 1/4] Add generic functions for accessing the SPI-NOR chip.

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Functions:

 - Send an opcode

 - Read a register

 - Write a register

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 drivers/mtd/spi-nor/core.c | 130 +++++++++++++++++++++++++++----------
 drivers/mtd/spi-nor/core.h |  27 +-------
 2 files changed, 99 insertions(+), 58 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 028514c6996f..0f267da339a4 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -354,53 +354,134 @@ int spi_nor_write_any_volatile_reg(struct spi_nor *nor, struct spi_mem_op *op,
 }
 
 /**
- * spi_nor_write_enable() - Set write enable latch with Write Enable command.
+ * _nor_send_cmd() - Send instruction without address or data to the chip.
  * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_write_enable(struct spi_nor *nor)
+static inline int _nor_send_cmd(struct spi_nor *nor, u8 opcode)
 {
 	int ret;
 
 	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_WREN_OP;
+		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+					   SPI_MEM_OP_NO_ADDR,
+					   SPI_MEM_OP_NO_DUMMY,
+					   SPI_MEM_OP_NO_DATA);
 
 		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_WREN,
-						       NULL, 0);
+		ret = spi_nor_controller_ops_write_reg(nor, opcode, NULL, 0);
 	}
 
-	if (ret)
-		dev_dbg(nor->dev, "error %d on Write Enable\n", ret);
+	return ret;
+}
+
+/**
+ * spi_nor_send_cmd() - Send instruction without address or data to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_send_cmd(struct spi_nor *nor, u8 opcode)
+{
+	int ret;
+
+	ret = _nor_send_cmd(nor, opcode);
 
 	return ret;
 }
 
 /**
- * spi_nor_write_disable() - Send Write Disable instruction to the chip.
+ * spi_nor_read_reg() - Send instruction without address or data to the chip.
  * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
+ * @len:	register value length
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_write_disable(struct spi_nor *nor)
+int spi_nor_read_reg(struct spi_nor *nor, u8 opcode, size_t len)
 {
 	int ret;
 
 	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_WRDI_OP;
+		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+					   SPI_MEM_OP_NO_ADDR,
+					   SPI_MEM_OP_NO_DUMMY,
+					   SPI_MEM_OP_DATA_IN(len, nor->bouncebuf, 0));
 
 		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_WRDI,
-						       NULL, 0);
+		ret = spi_nor_controller_ops_read_reg(nor, opcode, nor->bouncebuf, len);
 	}
 
+	return ret;
+}
+
+/*
+ * spi_nor_write_reg() - Send instruction without address or data to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
+ * @len:	register value length
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, size_t len)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+					   SPI_MEM_OP_NO_ADDR,
+					   SPI_MEM_OP_NO_DUMMY,
+					   SPI_MEM_OP_DATA_OUT(len, nor->bouncebuf, 0));
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, opcode, nor->bouncebuf, len);
+	}
+
+	return ret;
+}
+
+/**
+ * spi_nor_write_enable() - Set write enable latch with Write Enable command.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_enable(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = _nor_send_cmd(nor, SPINOR_OP_WREN);
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d on Write Enable\n", ret);
+
+	return ret;
+}
+
+/**
+ * spi_nor_write_disable() - Send Write Disable instruction to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_disable(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = _nor_send_cmd(nor, SPINOR_OP_WRDI);
+
 	if (ret)
 		dev_dbg(nor->dev, "error %d on Write Disable\n", ret);
 
@@ -521,18 +602,8 @@ int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
-	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_EN4B_EX4B_OP(enable);
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor,
-						       enable ? SPINOR_OP_EN4B :
-								SPINOR_OP_EX4B,
-						       NULL, 0);
-	}
+	ret = _nor_send_cmd(nor, enable ? SPINOR_OP_EN4B :
+						SPINOR_OP_EX4B);
 
 	if (ret)
 		dev_dbg(nor->dev, "error %d setting 4-byte mode\n", ret);
@@ -765,16 +836,7 @@ int spi_nor_global_block_unlock(struct spi_nor *nor)
 	if (ret)
 		return ret;
 
-	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_GBULK_OP;
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_GBULK,
-						       NULL, 0);
-	}
+	ret = _nor_send_cmd(nor, SPINOR_OP_GBULK);
 
 	if (ret) {
 		dev_dbg(nor->dev, "error %d on Global Block Unlock\n", ret);
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 442786685515..df456a713d92 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -25,18 +25,6 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 0),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 0))
 
-#define SPI_NOR_WREN_OP							\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WREN, 0),			\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
-#define SPI_NOR_WRDI_OP							\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRDI, 0),			\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
 #define SPI_NOR_RDSR_OP(buf)						\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDSR, 0),			\
 		   SPI_MEM_OP_NO_ADDR,					\
@@ -67,24 +55,12 @@
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_DATA_IN(1, buf, 0))
 
-#define SPI_NOR_EN4B_EX4B_OP(enable)					\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B, 0),	\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
 #define SPI_NOR_BRWR_OP(buf)						\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_BRWR, 0),			\
 		   SPI_MEM_OP_NO_ADDR,					\
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
 
-#define SPI_NOR_GBULK_OP						\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_GBULK, 0),			\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
 #define SPI_NOR_DIE_ERASE_OP(opcode, addr_nbytes, addr, dice)		\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),				\
 		   SPI_MEM_OP_ADDR(dice ? addr_nbytes : 0, addr, 0),	\
@@ -611,6 +587,9 @@ extern const struct attribute_group *spi_nor_sysfs_groups[];
 void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 			     struct spi_mem_op *op,
 			     const enum spi_nor_protocol proto);
+int spi_nor_send_cmd(struct spi_nor *nor, u8 opcode);
+int spi_nor_read_reg(struct spi_nor *nor, u8 opcode, size_t len);
+int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, size_t len);
 int spi_nor_write_enable(struct spi_nor *nor);
 int spi_nor_write_disable(struct spi_nor *nor);
 int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable);
-- 
2.39.2

Re: [PATCH v2 1/4] Add generic functions for accessing the SPI-NOR chip.

[Tudor Ambarus]

On 6/29/24 11:39 AM, Erez Geva wrote:
> From: Erez Geva <ErezGeva2@gmail.com>
> 
> Functions:
> 
>  - Send an opcode
> 
>  - Read a register
> 
>  - Write a register


Don't use the commit message as a continuation of the subject. Describe
your changes. Convince the reviewer why you need to change core methods
and that it makes sense for them to read past the first paragraph.

You ignored the feedback from v1, I'm marking this as changes requested
and ignore the rest.

[PATCH v2 2/4] Add support for SPI-NOR Macronix OTP.

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Macronix SPI-NOR support OTP.
Add callbacks to read, write and lock the OTP.

Notice Macronix OTP do not support erase.
Every bit written with '0', can not be changed further.

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 drivers/mtd/spi-nor/macronix.c | 185 +++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  10 ++
 2 files changed, 195 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index ea6be95e75a5..f210231468a6 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,180 @@
 
 #include "core.h"
 
+/**
+ * macronix_nor_otp_enter() - Send Enter Secured OTP instruction to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_nor_otp_enter(struct spi_nor *nor)
+{
+	int error;
+
+	error = spi_nor_send_cmd(nor, SPINOR_OP_ENSO);
+
+	if (error)
+		dev_dbg(nor->dev, "error %d on Macronix Enter Secured OTP\n", error);
+
+	return error;
+}
+
+/**
+ * macronix_nor_otp_exit() - Send Exit Secured OTP instruction to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_nor_otp_exit(struct spi_nor *nor)
+{
+	int error;
+
+	error = spi_nor_send_cmd(nor, SPINOR_OP_EXSO);
+
+	if (error)
+		dev_dbg(nor->dev, "error %d on Macronix Enter Secured OTP\n", error);
+
+	return error;
+}
+
+/**
+ * macronix_nor_otp_read() - read security register
+ * @nor:	pointer to 'struct spi_nor'
+ * @addr:       offset to read from
+ * @len:        number of bytes to read
+ * @buf:        pointer to dst buffer
+ *
+ * Return: number of bytes read successfully, -errno otherwise
+ */
+static int macronix_nor_otp_read(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
+{
+	int ret, error;
+
+	error = macronix_nor_otp_enter(nor);
+	if (error)
+		return error;
+
+	ret = spi_nor_read_data(nor, addr, len, buf);
+
+	error = macronix_nor_otp_exit(nor);
+
+	if (ret < 0)
+		dev_dbg(nor->dev, "error %d on Macronix read OTP data\n", ret);
+	else if (error)
+		return error;
+
+	return ret;
+}
+
+/**
+ * macronix_nor_otp_write() - write security register
+ * @nor:        pointer to 'struct spi_nor'
+ * @addr:       offset to write to
+ * @len:        number of bytes to write
+ * @buf:        pointer to src buffer
+ *
+ * Return: number of bytes written successfully, -errno otherwise
+ */
+static int macronix_nor_otp_write(struct spi_nor *nor, loff_t addr, size_t len, const u8 *buf)
+{
+	int error, ret = 0;
+
+	error = macronix_nor_otp_enter(nor);
+	if (error)
+		return error;
+
+	error = spi_nor_write_enable(nor);
+	if (error)
+		goto otp_write_err;
+
+	ret = spi_nor_write_data(nor, addr, len, buf);
+	if (ret < 0) {
+		dev_dbg(nor->dev, "error %d on Macronix write OTP data\n", ret);
+		goto otp_write_err;
+	}
+
+	error = spi_nor_wait_till_ready(nor);
+	if (error)
+		dev_dbg(nor->dev, "error %d on Macronix waiting write OTP finish\n", error);
+
+otp_write_err:
+
+	error = macronix_nor_otp_exit(nor);
+
+	return ret;
+}
+
+/**
+ * macronix_nor_otp_lock() - lock the OTP region
+ * @nor:        pointer to 'struct spi_nor'
+ * @region:     OTP region
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_nor_otp_lock(struct spi_nor *nor, unsigned int region)
+{
+	int error;
+	u8 *rdscur = nor->bouncebuf;
+
+	error = spi_nor_read_reg(nor, SPINOR_OP_RDSCUR, 1);
+	if (error) {
+		dev_dbg(nor->dev, "error %d on read security register\n", error);
+		return error;
+	}
+
+	switch (region) {
+	case 0: /* Lock 1st 4K-bit region */
+		if (rdscur[0] & SEC_REG_LDSO)
+			return 0; /* Already locked */
+		rdscur[0] |= SEC_REG_LDSO;
+		break;
+	case 1: /* Lock 2nd 4K-bit region */
+		if (rdscur[0] & SEC_REG_LDS1)
+			return 0; /* Already locked */
+		rdscur[0] |= SEC_REG_LDS1;
+		break;
+	default:
+		return 0; /* Unknown region */
+	}
+
+	error = spi_nor_write_reg(nor, SPINOR_OP_WRSCUR, 1);
+	if (error)
+		dev_dbg(nor->dev, "error %d on update security register\n", error);
+
+	return error;
+}
+
+/**
+ * macronix_nor_otp_is_locked() - get the OTP region lock status
+ * @nor:        pointer to 'struct spi_nor'
+ * @region:     OTP region
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_nor_otp_is_locked(struct spi_nor *nor, unsigned int region)
+{
+	int ret;
+	u8 *rdscur = nor->bouncebuf;
+
+	ret = spi_nor_read_reg(nor, SPINOR_OP_RDSCUR, 1);
+	if (ret) {
+		dev_dbg(nor->dev, "error %d on read security register\n", ret);
+		return ret;
+	}
+
+	switch (region) {
+	case 0: /* 1st 4K-bit region */
+		ret = (rdscur[0] & SEC_REG_LDSO) > 0;
+		break;
+	case 1: /* 2nd 4K-bit region */
+		ret = (rdscur[0] & SEC_REG_LDS1) > 0;
+		break;
+	default: /* Unknown region */
+		break;
+	}
+	return ret;
+}
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -190,8 +364,19 @@ static void macronix_nor_default_init(struct spi_nor *nor)
 	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
 }
 
+static const struct spi_nor_otp_ops macronix_nor_otp_ops = {
+	.read = macronix_nor_otp_read,
+	.write = macronix_nor_otp_write,
+	/* .erase = Macronix OTP do not support erase, */
+	.lock = macronix_nor_otp_lock,
+	.is_locked = macronix_nor_otp_is_locked,
+};
+
 static int macronix_nor_late_init(struct spi_nor *nor)
 {
+	if (nor->params->otp.org->n_regions)
+		nor->params->otp.ops = &macronix_nor_otp_ops;
+
 	if (!nor->params->set_4byte_addr_mode)
 		nor->params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b;
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index cdcfe0fd2e7d..f5965f90f51e 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -81,6 +81,16 @@
 #define SPINOR_OP_BP		0x02	/* Byte program */
 #define SPINOR_OP_AAI_WP	0xad	/* Auto address increment word program */
 
+/* Used by Macronix OTP. */
+#define SPINOR_OP_RDSCUR	0x2b	/* read security register */
+#define SPINOR_OP_WRSCUR	0x2f	/* write security register */
+#define SPINOR_OP_ENSO		0xb1	/* enter secured OTP */
+#define SPINOR_OP_EXSO		0xc1	/* exit secured OTP */
+
+/* Security register */
+#define SEC_REG_LDSO		BIT(1)  /* lock-down bit 1st 4K-bit */
+#define SEC_REG_LDS1		BIT(0)  /* lock-down bit 2nd 4K-bit */
+
 /* Used for Macronix and Winbond flashes. */
 #define SPINOR_OP_EN4B		0xb7	/* Enter 4-byte mode */
 #define SPINOR_OP_EX4B		0xe9	/* Exit 4-byte mode */
-- 
2.39.2

[PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Add Macronix SPI-NOR mx25l12833f.

mx25l12833f uses the same JEDEC ID as mx25l12805d.

Although mx25l12833f is a new chip that support read ID and SFDP,
 users might want to specify it in the device tree,
 to differ it from the old mx25l12805d chip.

Macronix annonce the end of life of mx25l12805d in 2010.

See:
  "https://www.macronix.com/Lists/TechDoc/Attachments/9861/PCN31_2009 PCN_MX25L6405D and MX25L12805D.pdf"

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
index 6e3afb42926e..625a618a7992 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
@@ -22,7 +22,7 @@ properties:
               n25q(32b|064|128a11|128a13|256a|512a|164k)))|\
               atmel,at25df(321a|641|081a)|\
               everspin,mr25h(10|40|128|256)|\
-              (mxicy|macronix),mx25l(4005a|1606e|6405d|8005|12805d|25635e)|\
+              (mxicy|macronix),mx25l(4005a|1606e|6405d|8005|12805d|12833f|25635e)|\
               (mxicy|macronix),mx25u(4033|4035)|\
               (spansion,)?s25fl(128s|256s1|512s|008k|064k|164k)|\
               (sst|microchip),sst25vf(016b|032b|040b)|\
-- 
2.39.2

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Tudor Ambarus]

Hi,

On 6/29/24 11:39 AM, Erez Geva wrote:
> From: Erez Geva <ErezGeva2@gmail.com>
> 
> Add Macronix SPI-NOR mx25l12833f.
> 
> mx25l12833f uses the same JEDEC ID as mx25l12805d.
> 
> Although mx25l12833f is a new chip that support read ID and SFDP,
>  users might want to specify it in the device tree,
>  to differ it from the old mx25l12805d chip.

As Michael said in a reply in v1, we try really hard to don't introduce
new compatibles and instead rely on the generic "jedec,spi-nor". In your
case you can differentiate between the two flavors of flashes based on
the SFDP presence. We won't apply this.

Cheers,
ta

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

Sorry, I resend the same mail, as I send it again as HTML.

How?

When using mx25l12805d, we do not read SFDP.
As it uses the no-SFDP flags.
When using mx25l12833f hardware with mx25l12805d driver, it did not
try to read the SFDP.
Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
driver fetch the SFDP.

Secondly SFDP does not contain OTP information.

mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
While mx25l12833f has two OTP regions of 512 KiB.

How do we handle it?
I would gladly remove the obsolete mx25l12805d.
And skp compatibles all together.

Thanks
    Erez Geva




On Mon, 1 Jul 2024 at 11:40, Erez <erezgeva2@gmail.com> wrote:
>
> How?
>
> When using mx25l12805d, we do not read SFDP.
> As it uses the no-SFDP flags.
> When using mx25l12833f hardware with mx25l12805d driver, it did not try to read the SFDP.
> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the driver fetch the SFDP.
>
> Secondly SFDP does not contain OTP information.
>
> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> While mx25l12833f has two OTP regions of 512 KiB.
>
> How do we handle it?
> I would gladly remove the obsolete mx25l12805d.
> And skp compatibles all together.
>
> Thanks
>     Erez Geva
>
>
>
>
>
> On Mon, 1 Jul 2024 at 07:23, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Hi,
>>
>> On 6/29/24 11:39 AM, Erez Geva wrote:
>> > From: Erez Geva <ErezGeva2@gmail.com>
>> >
>> > Add Macronix SPI-NOR mx25l12833f.
>> >
>> > mx25l12833f uses the same JEDEC ID as mx25l12805d.
>> >
>> > Although mx25l12833f is a new chip that support read ID and SFDP,
>> >  users might want to specify it in the device tree,
>> >  to differ it from the old mx25l12805d chip.
>>
>> As Michael said in a reply in v1, we try really hard to don't introduce
>> new compatibles and instead rely on the generic "jedec,spi-nor". In your
>> case you can differentiate between the two flavors of flashes based on
>> the SFDP presence. We won't apply this.
>>
>> Cheers,
>> ta

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Tudor Ambarus]

On 7/1/24 10:46 AM, Erez wrote:
> When using mx25l12805d, we do not read SFDP.
> As it uses the no-SFDP flags.
> When using mx25l12833f hardware with mx25l12805d driver, it did not
> try to read the SFDP.
> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> driver fetch the SFDP.
> 
> Secondly SFDP does not contain OTP information.
> 
> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> While mx25l12833f has two OTP regions of 512 KiB.
> 
> How do we handle it?

You would first try to parse SFDP and initialize the flash based on
SFDP. If there's no SFDP then you fallback to the flags declared at
flash declaration. Esben had a try recently, see [1]. I don't know if
there's any progress in that direction.

Also, you haven't mentioned anything about the testing. Do you have the
flash?

[1]
https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Tudor Ambarus]

On 7/1/24 11:15 AM, Tudor Ambarus wrote:
> 
> 
> On 7/1/24 10:46 AM, Erez wrote:
>> When using mx25l12805d, we do not read SFDP.
>> As it uses the no-SFDP flags.
>> When using mx25l12833f hardware with mx25l12805d driver, it did not
>> try to read the SFDP.
>> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
>> driver fetch the SFDP.
>>
>> Secondly SFDP does not contain OTP information.
>>
>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
>> While mx25l12833f has two OTP regions of 512 KiB.
>>
>> How do we handle it?
> 
> You would first try to parse SFDP and initialize the flash based on
> SFDP. If there's no SFDP then you fallback to the flags declared at
> flash declaration. Esben had a try recently, see [1]. I don't know if
> there's any progress in that direction.
> 

And you can then decide which OTP org to use based on whether SFDP is
present or not.

> Also, you haven't mentioned anything about the testing. Do you have the
> flash?
> 
> [1]
> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Mon, 1 Jul 2024 at 12:23, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 7/1/24 11:15 AM, Tudor Ambarus wrote:
> >
> >
> > On 7/1/24 10:46 AM, Erez wrote:
> >> When using mx25l12805d, we do not read SFDP.
> >> As it uses the no-SFDP flags.
> >> When using mx25l12833f hardware with mx25l12805d driver, it did not
> >> try to read the SFDP.
> >> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> >> driver fetch the SFDP.
> >>
> >> Secondly SFDP does not contain OTP information.
> >>
> >> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> >> While mx25l12833f has two OTP regions of 512 KiB.
> >>
> >> How do we handle it?
> >
> > You would first try to parse SFDP and initialize the flash based on
> > SFDP. If there's no SFDP then you fallback to the flags declared at
> > flash declaration. Esben had a try recently, see [1]. I don't know if
> > there's any progress in that direction.
> >
>
> And you can then decide which OTP org to use based on whether SFDP is
> present or not.

That can work, but sound like a hack.
Is that really that important to hack?
Just for OTP, that very few use?
And if in the future Macronix adds a newer one with the same JEDEC ID,
but a different OTP size?
Macronix does not consult with the Linux Kernel on these matters.

Anyhow as I do not have the hardware anymore, I can not do more
changes and test them.

Erez


>
> > Also, you haven't mentioned anything about the testing. Do you have the
> > flash?
> >
> > [1]
> > https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Tudor Ambarus]

On 7/1/24 12:03 PM, Erez wrote:
> On Mon, 1 Jul 2024 at 12:23, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>>
>>
>> On 7/1/24 11:15 AM, Tudor Ambarus wrote:
>>>
>>>
>>> On 7/1/24 10:46 AM, Erez wrote:
>>>> When using mx25l12805d, we do not read SFDP.
>>>> As it uses the no-SFDP flags.
>>>> When using mx25l12833f hardware with mx25l12805d driver, it did not
>>>> try to read the SFDP.
>>>> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
>>>> driver fetch the SFDP.
>>>>
>>>> Secondly SFDP does not contain OTP information.
>>>>
>>>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
>>>> While mx25l12833f has two OTP regions of 512 KiB.
>>>>
>>>> How do we handle it?
>>>
>>> You would first try to parse SFDP and initialize the flash based on
>>> SFDP. If there's no SFDP then you fallback to the flags declared at
>>> flash declaration. Esben had a try recently, see [1]. I don't know if
>>> there's any progress in that direction.
>>>
>>
>> And you can then decide which OTP org to use based on whether SFDP is
>> present or not.
> 
> That can work, but sound like a hack.

It's not a hack, we're just doing our best to dynamically identify the
flash.

> Is that really that important to hack?

we push really hard against new compatibles. Users shouldn't care about
what SPI NOR flash is there.

> Just for OTP, that very few use?
> And if in the future Macronix adds a newer one with the same JEDEC ID,
> but a different OTP size?

we'll compare SFDP data and choose based on the differences. This is not
encouraged. Instead ask for unique IDs or choose other flash.

> Macronix does not consult with the Linux Kernel on these matters.
> 

I'm complaining about unique flash IDs for years now. Hopefully vendors
have learnt their lesson. I didn't see new flash designs reusing old IDs.

> Anyhow as I do not have the hardware anymore, I can not do more
> changes and test them.
> 

Be aware that we're not queuing patches without some minimal tests. If
you don't have the hardware, contact mchp and see if they care,
otherwise we're wasting our time. Here are the minimum testing requirements:
https://docs.kernel.org/driver-api/mtd/spi-nor.html#minimum-testing-requirements
For OTP we'll also need some OTP tests.

Cheers,
ta

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Mon, 1 Jul 2024 at 14:53, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 7/1/24 12:03 PM, Erez wrote:
> > On Mon, 1 Jul 2024 at 12:23, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >>
> >>
> >> On 7/1/24 11:15 AM, Tudor Ambarus wrote:
> >>>
> >>>
> >>> On 7/1/24 10:46 AM, Erez wrote:
> >>>> When using mx25l12805d, we do not read SFDP.
> >>>> As it uses the no-SFDP flags.
> >>>> When using mx25l12833f hardware with mx25l12805d driver, it did not
> >>>> try to read the SFDP.
> >>>> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> >>>> driver fetch the SFDP.
> >>>>
> >>>> Secondly SFDP does not contain OTP information.
> >>>>
> >>>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> >>>> While mx25l12833f has two OTP regions of 512 KiB.
> >>>>
> >>>> How do we handle it?
> >>>
> >>> You would first try to parse SFDP and initialize the flash based on
> >>> SFDP. If there's no SFDP then you fallback to the flags declared at
> >>> flash declaration. Esben had a try recently, see [1]. I don't know if
> >>> there's any progress in that direction.
> >>>
> >>
> >> And you can then decide which OTP org to use based on whether SFDP is
> >> present or not.
> >
> > That can work, but sound like a hack.
>
> It's not a hack, we're just doing our best to dynamically identify the
> flash.

Call it whatever you want.

>
> > Is that really that important to hack?
>
> we push really hard against new compatibles. Users shouldn't care about
> what SPI NOR flash is there.

I am in. I do not like compatibility.
I do not like to see them in the device tree.
I did not create the mess.
It seems like Macronix likes to reuse JEDEC IDs.
As most new chips have SPDF, the mess is with OTP mainly.
We can think about a different model.
Perhaps allow the user to define the OTP size and number of regions
using a 'flash_otp_set' tool?
I am open to ideas.

>
> > Just for OTP, that very few use?
> > And if in the future Macronix adds a newer one with the same JEDEC ID,
> > but a different OTP size?
>
> we'll compare SFDP data and choose based on the differences. This is not
> encouraged. Instead ask for unique IDs or choose other flash.

That requires additional callback in the SFDP, not a big challenge.
Yet most SFDP tables are not sufficient to determine OTP.
Found only one table with OTP support flag.
No size, offset  and number of regions to work with.
The only hint is do we have SFDP or not.

>
> > Macronix does not consult with the Linux Kernel on these matters.
> >
>
> I'm complaining about unique flash IDs for years now. Hopefully vendors
> have learnt their lesson. I didn't see new flash designs reusing old IDs.

I can not speak in Macronix's name.
I know they did that, not sure if they stop.

>
> > Anyhow as I do not have the hardware anymore, I can not do more
> > changes and test them.
> >
>
> Be aware that we're not queuing patches without some minimal tests. If
> you don't have the hardware, contact mchp and see if they care,
> otherwise we're wasting our time. Here are the minimum testing requirements:
> https://docs.kernel.org/driver-api/mtd/spi-nor.html#minimum-testing-requirements
> For OTP we'll also need some OTP tests.

OTP as its name can be written once.
So a simple OTP test would be nice.

Anyhow I ordered 3 Macronix's MX25L3233F that have OTP.
The OTP works the same as the other Macronix chips.
The MX25L3233F ID is 0xc22016 same as "mx25l3205d".
Seems Macronix are persistent on reusing IDs.
First revision of MX25L3233F was in 2015,

I will connect to my old BeagleBone-black.
I think I can perform the test with it.

As I say, I am more interested in testing the code.
Less on which Macronix chip we use.

Thanks for your time
 Erez

>
> Cheers,
> ta

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Mon, 1 Jul 2024 at 12:15, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 7/1/24 10:46 AM, Erez wrote:
> > When using mx25l12805d, we do not read SFDP.
> > As it uses the no-SFDP flags.
> > When using mx25l12833f hardware with mx25l12805d driver, it did not
> > try to read the SFDP.
> > Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> > driver fetch the SFDP.
> >
> > Secondly SFDP does not contain OTP information.
> >
> > mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> > While mx25l12833f has two OTP regions of 512 KiB.
> >
> > How do we handle it?
>
> You would first try to parse SFDP and initialize the flash based on
> SFDP. If there's no SFDP then you fallback to the flags declared at
> flash declaration. Esben had a try recently, see [1]. I don't know if
> there's any progress in that direction.

That's great. I probably haven't seen it yet, as I used kernel 6.5 based.
However the OTP is not in the SFDP.
Reading the SFDP with "mx25l12805d" does improve :-)
if a user wants to use the OTP, he must specify "mx25l12833f".
Perhaps less likely, but yet no other solution for OTP at the moment.
I would not go to a kernel config for this.

>
> Also, you haven't mentioned anything about the testing. Do you have the
> flash?

I test the code with mx25l12833f hardware, with an older version of
the kernel, 6.5 I think.
It is a kernel provided by Microchip with their setting, The SPI-NOR
driver is vanilla.

In my testing using kernel 6.5 with a Microchip based board.
Before changing, the kernel driver, was using the "mx25l12805d"
setting and did NOT read the SFDP.
Once I added the "mx25l12833f" without any no-sfdp flags, the driver
did read the SFDP.
The driver can find "mx25l12833f" with a read ID, once I remove the
"mx25l12805d" setting, I did test that too.
Adding the new 'SPI_NOR_TRY_SFDP' is great. but does not provide proper OTP!

All the testings were done using the same mx25l12833f hardware, only
the settings in the driver were changed.

I am no longer in contact with the company that provides me the hardware.
And as this is a Microchip based hardware it is difficult to use the
latest kernel.

Anyhow as mx25l12805d hardware has not been sold since 2010, it is
hard to get one.
I do not want to change any setting of mx25l12805d without testing!

Anyhow my patch is focused on the new Macronix OTP callbacks.
I do not care about the "mx25l12833f" or "mx25l12805d" compatibilities
or settings.
If I had a different Macronix chip with a unique ID, I would gladly test it.

I only add them as the kernel rule is that new functions must be used
and tested with real hardware.

Erez

>
> [1]
> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Mon, 1 Jul 2024 at 12:15, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 7/1/24 10:46 AM, Erez wrote:
> > When using mx25l12805d, we do not read SFDP.
> > As it uses the no-SFDP flags.
> > When using mx25l12833f hardware with mx25l12805d driver, it did not
> > try to read the SFDP.
> > Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> > driver fetch the SFDP.
> >
> > Secondly SFDP does not contain OTP information.
> >
> > mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> > While mx25l12833f has two OTP regions of 512 KiB.
> >
> > How do we handle it?
>
> You would first try to parse SFDP and initialize the flash based on
> SFDP. If there's no SFDP then you fallback to the flags declared at
> flash declaration. Esben had a try recently, see [1]. I don't know if
> there's any progress in that direction.
>
> Also, you haven't mentioned anything about the testing. Do you have the
> flash?
>
> [1]
> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/

Looking at "mtd: spi-nor: macronix: workaround for device id re-use"
I guess it can be applied to all Macronix devices.
Adding something like the following in macronix_nor_default_init():

if (info>no_sfdp_flags)
    info>no_sfdp_flags |= SPI_NOR_TRY_SFDP

It seems Macronix did many reuse of IDs.
I saw it with "mx25l12833f" reusing "mx25l12805d".
And Esben saw it with MX25L3233F reusing "MX25L3205D".

The only thing I notice is the flash using the same size.
A sort of "backward" compatible.

Erez

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Tudor Ambarus]

On 7/1/24 6:08 PM, Erez wrote:
> On Mon, 1 Jul 2024 at 12:15, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>>
>>
>> On 7/1/24 10:46 AM, Erez wrote:
>>> When using mx25l12805d, we do not read SFDP.
>>> As it uses the no-SFDP flags.
>>> When using mx25l12833f hardware with mx25l12805d driver, it did not
>>> try to read the SFDP.
>>> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
>>> driver fetch the SFDP.
>>>
>>> Secondly SFDP does not contain OTP information.
>>>
>>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
>>> While mx25l12833f has two OTP regions of 512 KiB.
>>>
>>> How do we handle it?
>>
>> You would first try to parse SFDP and initialize the flash based on
>> SFDP. If there's no SFDP then you fallback to the flags declared at
>> flash declaration. Esben had a try recently, see [1]. I don't know if
>> there's any progress in that direction.
>>
>> Also, you haven't mentioned anything about the testing. Do you have the
>> flash?
>>
>> [1]
>> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/
> 
> Looking at "mtd: spi-nor: macronix: workaround for device id re-use"
> I guess it can be applied to all Macronix devices.

No, no, we're going to do it individually just where it's needed.
Issuing unsupported commands is not that great.

> Adding something like the following in macronix_nor_default_init():
> 
> if (info>no_sfdp_flags)
>     info>no_sfdp_flags |= SPI_NOR_TRY_SFDP
> 
> It seems Macronix did many reuse of IDs.
> I saw it with "mx25l12833f" reusing "mx25l12805d".
> And Esben saw it with MX25L3233F reusing "MX25L3205D".
> 
> The only thing I notice is the flash using the same size.
> A sort of "backward" compatible.
> 
> Erez

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Tue, 2 Jul 2024 at 07:00, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 7/1/24 6:08 PM, Erez wrote:
> > On Mon, 1 Jul 2024 at 12:15, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >>
> >>
> >> On 7/1/24 10:46 AM, Erez wrote:
> >>> When using mx25l12805d, we do not read SFDP.
> >>> As it uses the no-SFDP flags.
> >>> When using mx25l12833f hardware with mx25l12805d driver, it did not
> >>> try to read the SFDP.
> >>> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> >>> driver fetch the SFDP.
> >>>
> >>> Secondly SFDP does not contain OTP information.
> >>>
> >>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> >>> While mx25l12833f has two OTP regions of 512 KiB.
> >>>
> >>> How do we handle it?
> >>
> >> You would first try to parse SFDP and initialize the flash based on
> >> SFDP. If there's no SFDP then you fallback to the flags declared at
> >> flash declaration. Esben had a try recently, see [1]. I don't know if
> >> there's any progress in that direction.
> >>
> >> Also, you haven't mentioned anything about the testing. Do you have the
> >> flash?
> >>
> >> [1]
> >> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/
> >
> > Looking at "mtd: spi-nor: macronix: workaround for device id re-use"
> > I guess it can be applied to all Macronix devices.
>
> No, no, we're going to do it individually just where it's needed.
> Issuing unsupported commands is not that great.

Would be nice if we could ask Macronix directly.

Looking on their web site and reading some spec. and status reports.
Using the IDs with  'no_sfdp_flags' in drivers/mtd/spi-nor/macronix.c
I did not search for new chips reusing IDs of chips at end of life.
But we found 3 already:
MX25U51245G appears in the table with the same ID as MX66U51235F.
Esben Haabendal found MX25L3233F which reuses  MX25L3205D ID.
And I found MX25L12833F reuses MX25L12805D ID.
Chips that are not in end of life do support SFDP, at least the new
versions of the chips according to their spec.
It seems quite systematic.

By the way, the chip MX25L2005A part number is 'MX25L2005' without the 'A'.

We can support Macronix chips that are not in the table, just by
reading the SFDP.
In that case we can name them like "mx-szNN".

The table below uses fixed width characters.

ID      Part.         Size              Status          SFDP status
according to spec.
                                                        New chip with
SFDP for EOL
c22012  MX25L2005(A)  SZ_256K =  2Mb    EOL             MX25L2006E
c22532  MX25U2033E    SZ_256K =  2Mb    EOL
c22013  MX25L4005A    SZ_512K =  4Mb    EOL
c22533  MX25U4035     SZ_512K =  4Mb    EOL
c22534  MX25U8035     SZ_1M   =  8Mb    EOL
c22016  MX25L3205D    SZ_4M   =  32Mb   EOL             MX25L3233F
c29e16  MX25L3255E    SZ_4M   =  32Mb   EOL
c22017  MX25L6405D    SZ_8M   =  64Mb   EOL
c22018  MX25L12805D   SZ_16M  =  128Mb  EOL             MX25L12833F
c22538  MX25U12835F   SZ_16M  =  128Mb  EOL
c2253a  MX66U51235F   SZ_64M  =  512Mb  EOL             MX25U51245G
c22010  MX25L512E     SZ_64K  =  512Kb  NO_REC          Have-SFDP!
c22015  MX25L1606E    SZ_2M   =  16Mb   NO_REC          Have-SFDP!
c22536  MX25U3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
c22816  MX25R3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
c22537  MX25U6435F    SZ_8M   =  64Mb   NO_REC          Have-SFDP!
c22019  MX25L25635E   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
c22539  MX25U25635F   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
c2201a  MX66L51235F   SZ_64M  =  512Mb  NO_REC          Have-SFDP!
c2261b  MX66L1G55G    SZ_128M =  1Gb    NO_REC          Spec. is not public
c22314  MX25V8035F    SZ_1M   =  8Mb    PROD            Have-SFDP!
c22815  MX25R1635F    SZ_2M   =  16Mb   PROD            Have-SFDP!
c2201b  MX66L1G45G    SZ_128M =  1Gb    PROD            Have-SFDP!
c2253c  MX66U2G45G    SZ_256M =  2Gb    PROD            Have-SFDP!
c2253a  MX25U51245G   SZ_64M  =  512Mb  PROD            Have-SFDP!

EOL     End of Life
PROD    Normal Production
NO_REC  Not recommend for new design


Erez





>
> > Adding something like the following in macronix_nor_default_init():
> >
> > if (info>no_sfdp_flags)
> >     info>no_sfdp_flags |= SPI_NOR_TRY_SFDP
> >
> > It seems Macronix did many reuse of IDs.
> > I saw it with "mx25l12833f" reusing "mx25l12805d".
> > And Esben saw it with MX25L3233F reusing "MX25L3205D".
> >
> > The only thing I notice is the flash using the same size.
> > A sort of "backward" compatible.
> >
> > Erez

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Tudor Ambarus]

On 7/3/24 12:16 AM, Erez wrote:
> On Tue, 2 Jul 2024 at 07:00, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>>
>>
>> On 7/1/24 6:08 PM, Erez wrote:
>>> On Mon, 1 Jul 2024 at 12:15, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 7/1/24 10:46 AM, Erez wrote:
>>>>> When using mx25l12805d, we do not read SFDP.
>>>>> As it uses the no-SFDP flags.
>>>>> When using mx25l12833f hardware with mx25l12805d driver, it did not
>>>>> try to read the SFDP.
>>>>> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
>>>>> driver fetch the SFDP.
>>>>>
>>>>> Secondly SFDP does not contain OTP information.
>>>>>
>>>>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
>>>>> While mx25l12833f has two OTP regions of 512 KiB.
>>>>>
>>>>> How do we handle it?
>>>>
>>>> You would first try to parse SFDP and initialize the flash based on
>>>> SFDP. If there's no SFDP then you fallback to the flags declared at
>>>> flash declaration. Esben had a try recently, see [1]. I don't know if
>>>> there's any progress in that direction.
>>>>
>>>> Also, you haven't mentioned anything about the testing. Do you have the
>>>> flash?
>>>>
>>>> [1]
>>>> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/
>>>
>>> Looking at "mtd: spi-nor: macronix: workaround for device id re-use"
>>> I guess it can be applied to all Macronix devices.
>>
>> No, no, we're going to do it individually just where it's needed.
>> Issuing unsupported commands is not that great.
> 
> Would be nice if we could ask Macronix directly.

we did. They said it's not ideal, but it's okay.
> 
> Looking on their web site and reading some spec. and status reports.
> Using the IDs with  'no_sfdp_flags' in drivers/mtd/spi-nor/macronix.c
> I did not search for new chips reusing IDs of chips at end of life.
> But we found 3 already:
> MX25U51245G appears in the table with the same ID as MX66U51235F.

is there an extension ID that differentiate the two?

> Esben Haabendal found MX25L3233F which reuses  MX25L3205D ID.
> And I found MX25L12833F reuses MX25L12805D ID.

Yes. And we already have a plan for these. We need someone that cares
about them to implement it.

> Chips that are not in end of life do support SFDP, at least the new
> versions of the chips according to their spec.
> It seems quite systematic.
> 

maybe

> By the way, the chip MX25L2005A part number is 'MX25L2005' without the 'A'.

feel free to propose a patch

> 
> We can support Macronix chips that are not in the table, just by
> reading the SFDP.
> In that case we can name them like "mx-szNN".

We don't care about the flash name.

If all the flash settings that we care about can be discovered by SFDP
then one won't need to declare a flash entry at all, and instead rely on
the driver to setup the flash settings solely based on the SFDP tables.
See spi-nor-generic handling.

> 
> The table below uses fixed width characters.
> 
> ID      Part.         Size              Status          SFDP status
> according to spec.
>                                                         New chip with
> SFDP for EOL
> c22012  MX25L2005(A)  SZ_256K =  2Mb    EOL             MX25L2006E
> c22532  MX25U2033E    SZ_256K =  2Mb    EOL
> c22013  MX25L4005A    SZ_512K =  4Mb    EOL
> c22533  MX25U4035     SZ_512K =  4Mb    EOL
> c22534  MX25U8035     SZ_1M   =  8Mb    EOL
> c22016  MX25L3205D    SZ_4M   =  32Mb   EOL             MX25L3233F
> c29e16  MX25L3255E    SZ_4M   =  32Mb   EOL
> c22017  MX25L6405D    SZ_8M   =  64Mb   EOL
> c22018  MX25L12805D   SZ_16M  =  128Mb  EOL             MX25L12833F
> c22538  MX25U12835F   SZ_16M  =  128Mb  EOL
> c2253a  MX66U51235F   SZ_64M  =  512Mb  EOL             MX25U51245G
> c22010  MX25L512E     SZ_64K  =  512Kb  NO_REC          Have-SFDP!
> c22015  MX25L1606E    SZ_2M   =  16Mb   NO_REC          Have-SFDP!
> c22536  MX25U3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
> c22816  MX25R3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
> c22537  MX25U6435F    SZ_8M   =  64Mb   NO_REC          Have-SFDP!
> c22019  MX25L25635E   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
> c22539  MX25U25635F   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
> c2201a  MX66L51235F   SZ_64M  =  512Mb  NO_REC          Have-SFDP!
> c2261b  MX66L1G55G    SZ_128M =  1Gb    NO_REC          Spec. is not public
> c22314  MX25V8035F    SZ_1M   =  8Mb    PROD            Have-SFDP!
> c22815  MX25R1635F    SZ_2M   =  16Mb   PROD            Have-SFDP!
> c2201b  MX66L1G45G    SZ_128M =  1Gb    PROD            Have-SFDP!
> c2253c  MX66U2G45G    SZ_256M =  2Gb    PROD            Have-SFDP!
> c2253a  MX25U51245G   SZ_64M  =  512Mb  PROD            Have-SFDP!
> 
> EOL     End of Life
> PROD    Normal Production
> NO_REC  Not recommend for new design
> 
> 

not sure what you want me to do with these.

Cheers,
ta

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Wed, 3 Jul 2024 at 09:12, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 7/3/24 12:16 AM, Erez wrote:
> > On Tue, 2 Jul 2024 at 07:00, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >>
> >>
> >> On 7/1/24 6:08 PM, Erez wrote:
> >>> On Mon, 1 Jul 2024 at 12:15, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 7/1/24 10:46 AM, Erez wrote:
> >>>>> When using mx25l12805d, we do not read SFDP.
> >>>>> As it uses the no-SFDP flags.
> >>>>> When using mx25l12833f hardware with mx25l12805d driver, it did not
> >>>>> try to read the SFDP.
> >>>>> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> >>>>> driver fetch the SFDP.
> >>>>>
> >>>>> Secondly SFDP does not contain OTP information.
> >>>>>
> >>>>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> >>>>> While mx25l12833f has two OTP regions of 512 KiB.
> >>>>>
> >>>>> How do we handle it?
> >>>>
> >>>> You would first try to parse SFDP and initialize the flash based on
> >>>> SFDP. If there's no SFDP then you fallback to the flags declared at
> >>>> flash declaration. Esben had a try recently, see [1]. I don't know if
> >>>> there's any progress in that direction.
> >>>>
> >>>> Also, you haven't mentioned anything about the testing. Do you have the
> >>>> flash?
> >>>>
> >>>> [1]
> >>>> https://lore.kernel.org/linux-mtd/20240603-macronix-mx25l3205d-fixups-v2-0-ff98da26835c@geanix.com/
> >>>
> >>> Looking at "mtd: spi-nor: macronix: workaround for device id re-use"
> >>> I guess it can be applied to all Macronix devices.
> >>
> >> No, no, we're going to do it individually just where it's needed.
> >> Issuing unsupported commands is not that great.
> >
> > Would be nice if we could ask Macronix directly.
>
> we did. They said it's not ideal, but it's okay.
> >
> > Looking on their web site and reading some spec. and status reports.
> > Using the IDs with  'no_sfdp_flags' in drivers/mtd/spi-nor/macronix.c
> > I did not search for new chips reusing IDs of chips at end of life.
> > But we found 3 already:
> > MX25U51245G appears in the table with the same ID as MX66U51235F.
>
> is there an extension ID that differentiate the two?
>
> > Esben Haabendal found MX25L3233F which reuses  MX25L3205D ID.
> > And I found MX25L12833F reuses MX25L12805D ID.
>
> Yes. And we already have a plan for these. We need someone that cares
> about them to implement it.
>
> > Chips that are not in end of life do support SFDP, at least the new
> > versions of the chips according to their spec.
> > It seems quite systematic.
> >
>
> maybe

I check the public spec of all chips, but one, which are in the
Macronix driver table and are still in production.
They all support SFDP. I do not understand the "maybe".
As for the chips in end of life, we found 3 new chips that reuse the
ID, we can find the rest.

>
> > By the way, the chip MX25L2005A part number is 'MX25L2005' without the 'A'.
>
> feel free to propose a patch
>
> >
> > We can support Macronix chips that are not in the table, just by
> > reading the SFDP.
> > In that case we can name them like "mx-szNN".
>
> We don't care about the flash name.

Agree.

>
> If all the flash settings that we care about can be discovered by SFDP
> then one won't need to declare a flash entry at all, and instead rely on
> the driver to setup the flash settings solely based on the SFDP tables.
> See spi-nor-generic handling.

Excellent feature!

>
> >
> > The table below uses fixed width characters.
> >
> > ID      Part.         Size              Status          SFDP status
> > according to spec.
> >                                                         New chip with
> > SFDP for EOL
> > c22012  MX25L2005(A)  SZ_256K =  2Mb    EOL             MX25L2006E
> > c22532  MX25U2033E    SZ_256K =  2Mb    EOL
> > c22013  MX25L4005A    SZ_512K =  4Mb    EOL
> > c22533  MX25U4035     SZ_512K =  4Mb    EOL
> > c22534  MX25U8035     SZ_1M   =  8Mb    EOL
> > c22016  MX25L3205D    SZ_4M   =  32Mb   EOL             MX25L3233F
> > c29e16  MX25L3255E    SZ_4M   =  32Mb   EOL
> > c22017  MX25L6405D    SZ_8M   =  64Mb   EOL
> > c22018  MX25L12805D   SZ_16M  =  128Mb  EOL             MX25L12833F
> > c22538  MX25U12835F   SZ_16M  =  128Mb  EOL
> > c2253a  MX66U51235F   SZ_64M  =  512Mb  EOL             MX25U51245G
> > c22010  MX25L512E     SZ_64K  =  512Kb  NO_REC          Have-SFDP!
> > c22015  MX25L1606E    SZ_2M   =  16Mb   NO_REC          Have-SFDP!
> > c22536  MX25U3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
> > c22816  MX25R3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
> > c22537  MX25U6435F    SZ_8M   =  64Mb   NO_REC          Have-SFDP!
> > c22019  MX25L25635E   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
> > c22539  MX25U25635F   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
> > c2201a  MX66L51235F   SZ_64M  =  512Mb  NO_REC          Have-SFDP!
> > c2261b  MX66L1G55G    SZ_128M =  1Gb    NO_REC          Spec. is not public
> > c22314  MX25V8035F    SZ_1M   =  8Mb    PROD            Have-SFDP!
> > c22815  MX25R1635F    SZ_2M   =  16Mb   PROD            Have-SFDP!
> > c2201b  MX66L1G45G    SZ_128M =  1Gb    PROD            Have-SFDP!
> > c2253c  MX66U2G45G    SZ_256M =  2Gb    PROD            Have-SFDP!
> > c2253a  MX25U51245G   SZ_64M  =  512Mb  PROD            Have-SFDP!
> >
> > EOL     End of Life
> > PROD    Normal Production
> > NO_REC  Not recommend for new design
> >
> >
>
> not sure what you want me to do with these.

That we can read SFDP for all chips from Macronix.
Only old chips before 2010 do not have SFDP.

Erez

>
> Cheers,
> ta

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Esben Haabendal]

Erez <erezgeva2@gmail.com> writes:

> On Wed, 3 Jul 2024 at 09:12, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>> On 7/3/24 12:16 AM, Erez wrote:
>>> On Tue, 2 Jul 2024 at 07:00, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>>
>>> The table below uses fixed width characters.
>>>
>>> ID      Part.         Size              Status          SFDP status
>>> according to spec.
>>>                                                         New chip with
>>> SFDP for EOL
>>> c22012  MX25L2005(A)  SZ_256K =  2Mb    EOL             MX25L2006E
>>> c22532  MX25U2033E    SZ_256K =  2Mb    EOL
>>> c22013  MX25L4005A    SZ_512K =  4Mb    EOL
>>> c22533  MX25U4035     SZ_512K =  4Mb    EOL
>>> c22534  MX25U8035     SZ_1M   =  8Mb    EOL
>>> c22016  MX25L3205D    SZ_4M   =  32Mb   EOL             MX25L3233F
>>> c29e16  MX25L3255E    SZ_4M   =  32Mb   EOL
>>> c22017  MX25L6405D    SZ_8M   =  64Mb   EOL
>>> c22018  MX25L12805D   SZ_16M  =  128Mb  EOL             MX25L12833F
>>> c22538  MX25U12835F   SZ_16M  =  128Mb  EOL
>>> c2253a  MX66U51235F   SZ_64M  =  512Mb  EOL             MX25U51245G
>>> c22010  MX25L512E     SZ_64K  =  512Kb  NO_REC          Have-SFDP!
>>> c22015  MX25L1606E    SZ_2M   =  16Mb   NO_REC          Have-SFDP!
>>> c22536  MX25U3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
>>> c22816  MX25R3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
>>> c22537  MX25U6435F    SZ_8M   =  64Mb   NO_REC          Have-SFDP!
>>> c22019  MX25L25635E   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
>>> c22539  MX25U25635F   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
>>> c2201a  MX66L51235F   SZ_64M  =  512Mb  NO_REC          Have-SFDP!
>>> c2261b  MX66L1G55G    SZ_128M =  1Gb    NO_REC          Spec. is not public
>>> c22314  MX25V8035F    SZ_1M   =  8Mb    PROD            Have-SFDP!
>>> c22815  MX25R1635F    SZ_2M   =  16Mb   PROD            Have-SFDP!
>>> c2201b  MX66L1G45G    SZ_128M =  1Gb    PROD            Have-SFDP!
>>> c2253c  MX66U2G45G    SZ_256M =  2Gb    PROD            Have-SFDP!
>>> c2253a  MX25U51245G   SZ_64M  =  512Mb  PROD            Have-SFDP!
>>>
>>> EOL     End of Life
>>> PROD    Normal Production
>>> NO_REC  Not recommend for new design
>>>
>>>
>>
>> not sure what you want me to do with these.
>
> That we can read SFDP for all chips from Macronix.
> Only old chips before 2010 do not have SFDP.

So, should we try and identify new chips (with SFDP) that re-use the ID of all the
above mentioned EOL chips that does not have SFDP?

As I read the communication from Macronix, then we should expect new
chips re-using the ID for all of them. It is just a matter of digging.

/Esben

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

Yes, I think we should.

Reading the specification provided publicly by Macronix.
For all the JEDEC IDs with the no SFDP flag in drivers/mtd/spi-nor/macronix.c
All of them have a new version or a new chip with the same JEDEC ID
that supports SFDP.
There are 2 chips that Macronix does not provide spec. in public.
I can ask Macronix technical support on these 2 chips.

Erez

"RDID"    "Part."         "Size"              "Status"          "SFDP
status according to spec. or new chip replacing with same RDID and
SFDP supported according to spec."
c22012  MX25L2005(A)  SZ_256K =  2Mb    EOL             MX25L2006E
c22013  MX25L4005A    SZ_512K =  4Mb    EOL             MX25L4006E
c22533  MX25U4035     SZ_512K =  4Mb    EOL             MX25U4033E
c22534  MX25U8035     SZ_1M   =  8Mb    EOL             MX25U8033E
c22016  MX25L3205D    SZ_4M   =  32Mb   EOL             MX25L3233F
c22017  MX25L6405D    SZ_8M   =  64Mb   EOL             MX25L6406E / MX25L6433F
c22018  MX25L12805D   SZ_16M  =  128Mb  EOL             MX25L12833F
c22538  MX25U12835F   SZ_16M  =  128Mb  EOL             MX25U12832F
c2253a  MX66U51235F   SZ_64M  =  512Mb  EOL             MX25U51245G
c22532  MX25U2033E    SZ_256K =  2Mb    EOL             Have-SFDP!
c22010  MX25L512E     SZ_64K  =  512Kb  NO_REC          Have-SFDP!
c22015  MX25L1606E    SZ_2M   =  16Mb   NO_REC          Have-SFDP!
c22536  MX25U3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
c22816  MX25R3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
c22537  MX25U6435F    SZ_8M   =  64Mb   NO_REC          Have-SFDP!
c22019  MX25L25635E   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
c22539  MX25U25635F   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
c2201a  MX66L51235F   SZ_64M  =  512Mb  NO_REC          Have-SFDP!
c2253a  MX25U51245G   SZ_64M  =  512Mb  PROD            Have-SFDP!
c22314  MX25V8035F    SZ_1M   =  8Mb    PROD            Have-SFDP!
c22815  MX25R1635F    SZ_2M   =  16Mb   PROD            Have-SFDP!
c2201b  MX66L1G45G    SZ_128M =  1Gb    PROD            Have-SFDP!
c2253c  MX66U2G45G    SZ_256M =  2Gb    PROD            Have-SFDP!
c2261b  MX66L1G55G    SZ_128M =  1Gb    NO_REC          Spec. is not public
c29e16  MX25L3255E    SZ_4M   =  32Mb   PROD            Spec. is not public

EOL     End of Life
PROD    Production
NO_REC  Not recommend


On Wed, 10 Jul 2024 at 16:34, Esben Haabendal <esben@geanix.com> wrote:
>
> Erez <erezgeva2@gmail.com> writes:
>
> > On Wed, 3 Jul 2024 at 09:12, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >> On 7/3/24 12:16 AM, Erez wrote:
> >>> On Tue, 2 Jul 2024 at 07:00, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>>
> >>> The table below uses fixed width characters.
> >>>
> >>> ID      Part.         Size              Status          SFDP status
> >>> according to spec.
> >>>                                                         New chip with
> >>> SFDP for EOL
> >>> c22012  MX25L2005(A)  SZ_256K =  2Mb    EOL             MX25L2006E
> >>> c22532  MX25U2033E    SZ_256K =  2Mb    EOL
> >>> c22013  MX25L4005A    SZ_512K =  4Mb    EOL
> >>> c22533  MX25U4035     SZ_512K =  4Mb    EOL
> >>> c22534  MX25U8035     SZ_1M   =  8Mb    EOL
> >>> c22016  MX25L3205D    SZ_4M   =  32Mb   EOL             MX25L3233F
> >>> c29e16  MX25L3255E    SZ_4M   =  32Mb   EOL
> >>> c22017  MX25L6405D    SZ_8M   =  64Mb   EOL
> >>> c22018  MX25L12805D   SZ_16M  =  128Mb  EOL             MX25L12833F
> >>> c22538  MX25U12835F   SZ_16M  =  128Mb  EOL
> >>> c2253a  MX66U51235F   SZ_64M  =  512Mb  EOL             MX25U51245G
> >>> c22010  MX25L512E     SZ_64K  =  512Kb  NO_REC          Have-SFDP!
> >>> c22015  MX25L1606E    SZ_2M   =  16Mb   NO_REC          Have-SFDP!
> >>> c22536  MX25U3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
> >>> c22816  MX25R3235F    SZ_4M   =  32Mb   NO_REC          Have-SFDP!
> >>> c22537  MX25U6435F    SZ_8M   =  64Mb   NO_REC          Have-SFDP!
> >>> c22019  MX25L25635E   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
> >>> c22539  MX25U25635F   SZ_32M  =  256Mb  NO_REC          Have-SFDP!
> >>> c2201a  MX66L51235F   SZ_64M  =  512Mb  NO_REC          Have-SFDP!
> >>> c2261b  MX66L1G55G    SZ_128M =  1Gb    NO_REC          Spec. is not public
> >>> c22314  MX25V8035F    SZ_1M   =  8Mb    PROD            Have-SFDP!
> >>> c22815  MX25R1635F    SZ_2M   =  16Mb   PROD            Have-SFDP!
> >>> c2201b  MX66L1G45G    SZ_128M =  1Gb    PROD            Have-SFDP!
> >>> c2253c  MX66U2G45G    SZ_256M =  2Gb    PROD            Have-SFDP!
> >>> c2253a  MX25U51245G   SZ_64M  =  512Mb  PROD            Have-SFDP!
> >>>
> >>> EOL     End of Life
> >>> PROD    Normal Production
> >>> NO_REC  Not recommend for new design
> >>>
> >>>
> >>
> >> not sure what you want me to do with these.
> >
> > That we can read SFDP for all chips from Macronix.
> > Only old chips before 2010 do not have SFDP.
>
> So, should we try and identify new chips (with SFDP) that re-use the ID of all the
> above mentioned EOL chips that does not have SFDP?
>
> As I read the communication from Macronix, then we should expect new
> chips re-using the ID for all of them. It is just a matter of digging.
>
> /Esben

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Michael Walle]

Hi Erez,

No top posting please, see also
https://subspace.kernel.org/etiquette.html

On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote:
> Yes, I think we should.
>
> Reading the specification provided publicly by Macronix.
> For all the JEDEC IDs with the no SFDP flag in drivers/mtd/spi-nor/macronix.c
> All of them have a new version or a new chip with the same JEDEC ID
> that supports SFDP.
> There are 2 chips that Macronix does not provide spec. in public.
> I can ask Macronix technical support on these 2 chips.

We don't add flashes we cannot test.

-michael

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Thu, 11 Jul 2024 at 21:57, Michael Walle <mwalle@kernel.org> wrote:
>
> Hi Erez,
>
> No top posting please, see also
> https://subspace.kernel.org/etiquette.html

It was a single question. Which I think can be answered in one reply.
In cases where there are different parts in the mail, it makes sense
to avoid top posting.
I do not believe we need to be pedantic.
The guidance is not holy, it is aimed to make communication more comprehensive.

>
> On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote:
> > Yes, I think we should.
> >
> > Reading the specification provided publicly by Macronix.
> > For all the JEDEC IDs with the no SFDP flag in drivers/mtd/spi-nor/macronix.c
> > All of them have a new version or a new chip with the same JEDEC ID
> > that supports SFDP.
> > There are 2 chips that Macronix does not provide spec. in public.
> > I can ask Macronix technical support on these 2 chips.
>
> We don't add flashes we cannot test.

I did not suggest adding anything new.
I refer to the list of chips we already have in drivers/mtd/spi-nor/macronix.c
I presume someone tested them before adding them to the list in the past.
And probably the old chip did not have the SFDP table back then.

What I checked with the chip specifications is that all Macronix chips
since 2010 have SFDP.

The situation today is that all Macronix chips that are NOT in the
Macronix table work based on the SFDP table.
But new chips that use a JEDEC found in the Macronix table, skip the
SFDP table and
 use the setting of the old chip.

So I suggest we read the SFDP table for all Macronix chips.
Old Macronix chips that do not have SFDP will use the setting from the
Macronix table. i.e backward compatible.
While new chips which do have an SFDP table will work with the new
setting we find in the table.

Of course, we might have issues in parsing the SFDP table itself.
So we fix them as developers report and send chip ID and part number
with the SFDP table content.
I do not see the point of "hiding" with the old setting.
Anyhow, as we do not like the IDs table and keep it for backward-compatible,
  so it only makes sense we should use the SFDP table as much as possible.

My check was to ensure Tudor that all Macronix chips have SFDP whether
they are in the IDs table or not
 and we are not wasting a no-op on a chip which can not have an SFDP table.

All I suggest is we add the new 'SPI_NOR_TRY_SFDP' flag, to all Macronix chips..
Which will try to read the SFDP to any Macronix chip.

Erez

>
> -michael

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Michael Walle]

On Fri Jul 12, 2024 at 12:09 AM CEST, Erez wrote:
> On Thu, 11 Jul 2024 at 21:57, Michael Walle <mwalle@kernel.org> wrote:
> >
> > Hi Erez,
> >
> > No top posting please, see also
> > https://subspace.kernel.org/etiquette.html
>
> It was a single question. Which I think can be answered in one reply.
> In cases where there are different parts in the mail, it makes sense
> to avoid top posting.
> I do not believe we need to be pedantic.
> The guidance is not holy, it is aimed to make communication more comprehensive.
>
> >
> > On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote:
> > > Yes, I think we should.

Regarding top posting, what was the question here?

I don't see any context. So don't expect any reply from me.

-michael

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

On Fri, 12 Jul 2024 at 00:13, Michael Walle <mwalle@kernel.org> wrote:
>
> On Fri Jul 12, 2024 at 12:09 AM CEST, Erez wrote:
> > On Thu, 11 Jul 2024 at 21:57, Michael Walle <mwalle@kernel.org> wrote:
> > >
> > > Hi Erez,
> > >
> > > No top posting please, see also
> > > https://subspace.kernel.org/etiquette.html
> >
> > It was a single question. Which I think can be answered in one reply.
> > In cases where there are different parts in the mail, it makes sense
> > to avoid top posting.
> > I do not believe we need to be pedantic.
> > The guidance is not holy, it is aimed to make communication more comprehensive.
> >
> > >
> > > On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote:
> > > > Yes, I think we should.
>
> Regarding top posting, what was the question here?
>
> I don't see any context. So don't expect any reply from me.

Fair point.
I'll try better next time and write more comprehen answers.
I do hope my reply on the second part was more informative.

Erez

>
> -michael

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Esben Haabendal]

Erez <erezgeva2@gmail.com> writes:

> On Thu, 11 Jul 2024 at 21:57, Michael Walle <mwalle@kernel.org> wrote:
>> On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote:
>> > Yes, I think we should.
>> >
>> > Reading the specification provided publicly by Macronix.
>> > For all the JEDEC IDs with the no SFDP flag in drivers/mtd/spi-nor/macronix.c
>> > All of them have a new version or a new chip with the same JEDEC ID
>> > that supports SFDP.
>> > There are 2 chips that Macronix does not provide spec. in public.
>> > I can ask Macronix technical support on these 2 chips.
>>
>> We don't add flashes we cannot test.
>
> I did not suggest adding anything new.
> I refer to the list of chips we already have in drivers/mtd/spi-nor/macronix.c
> I presume someone tested them before adding them to the list in the past.
> And probably the old chip did not have the SFDP table back then.
>
> What I checked with the chip specifications is that all Macronix chips
> since 2010 have SFDP.
>
> The situation today is that all Macronix chips that are NOT in the
> Macronix table work based on the SFDP table.
> But new chips that use a JEDEC found in the Macronix table, skip the
> SFDP table and use the setting of the old chip.

Not entirely true.

Those that entries in the Macronix table that has SPI_NOR_DUAL_READ
and/or SPI_NOR_QUAD_READ in no_sfdp_flags is caught by
the magic flags matching in spi_nor_init_params_deprecated() and will
have spi_nor_parse_sfdp() called from
spi_nor_sfdp_init_params_deprecated(). So flashes reusing ID for these
will have the SFDP tables parsed.

The rest of the entries in the Macronix table is not so lucky. When a
replacement chip (with the same ID) is used, it will not be configured
with the values found in SFDP table.

> So I suggest we read the SFDP table for all Macronix chips.

Based on their strategy of re-using flash ID, I think this is a sane
approach.

> Old Macronix chips that do not have SFDP will use the setting from the
> Macronix table. i.e backward compatible.
> While new chips which do have an SFDP table will work with the new
> setting we find in the table.

Yes, if we apply the new SPI_NOR_TRY_SFDP flag to the matching table
entries.

> Of course, we might have issues in parsing the SFDP table itself.
> So we fix them as developers report and send chip ID and part number
> with the SFDP table content.
> I do not see the point of "hiding" with the old setting.
> Anyhow, as we do not like the IDs table and keep it for backward-compatible,
> so it only makes sense we should use the SFDP table as much as possible.
>
> My check was to ensure Tudor that all Macronix chips have SFDP whether
> they are in the IDs table or not and we are not wasting a no-op on a
> chip which can not have an SFDP table.
>
> All I suggest is we add the new 'SPI_NOR_TRY_SFDP' flag, to all
> Macronix chips.. Which will try to read the SFDP to any Macronix chip.

Makes sense. But obviously comes with a risk as we won't be able to
test all chips for regression when doing that.

/Esben

Re: [PATCH v2 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez]

Hi,

I now try to test the OTP code using a Mactronix MX25L3233F with
Beagle bone black.
I manage to connect using a SOP DIP and connect it to an spi port using SPIDEV.
And I use the flashprog tool from https://flashprog.org.

That is just the background of my environment.
Now to the point, it seems flashprog.org does follow Mactronix.
The same JEDEC "C2 20 16" used for 4 different chips:

# ./flashprog -p linux_spi:dev=/dev/spidev0.0,spispeed=512 --flash-name

flashprog v1.1-dirty on Linux 6.6.32-ti-arm32-r5 (armv7l)
flashprog is free software, get the source code at https://flashprog.org

Using clock_gettime for delay loops (clk_id: 1, resolution: 1ns).
Found Macronix flash chip "MX25L3205(A)" (4096 kB, SPI) on linux_spi.
Found Macronix flash chip "MX25L3205D/MX25L3208D" (4096 kB, SPI) on linux_spi.
Found Macronix flash chip "MX25L3206E/MX25L3208E" (4096 kB, SPI) on linux_spi.
Found Macronix flash chip "MX25L3233F/MX25L3273E" (4096 kB, SPI) on linux_spi.
Multiple flash chip definitions match the detected chip(s):
"MX25L3205(A)", "MX25L3205D/MX25L3208D", "MX25L3206E/MX25L3208E",
"MX25L3233F/MX25L3273E"
Please specify which chip definition to use with the -c <chipname> option.

Now for OTP, looking at Mactronix data sheets:
MX25L3205(A) does not have SFDP and no OTP
MX25L3205D does not have SFDP, uses 2 asymmetric OTP banks of 128-bit
and 384-bit.
MX25L3206E supports SFDP, uses 2 asymmetric OTP banks of 128-bit and 384-bit.
MX25L3233F supports SFDP, uses 1 bank of 4096-bit.

New Mactronix chips also use symmetric OTP banks:
For example, a chip I tested before with OTP:
MX25L12833F with a JEDEC "C2 20 18" supports SFDP, uses 2 symmetric
OTP banks of 4096-bit.


My conclusions are:
1. The best way to support Mactronix chips is to read the SFDP.
    The table with IDs is relevant only to the old chips that do not
support SFDP.
2. The OTP parameters need to be provided via device tree or sysfs.
     We can not find them on SFDP nor can we deduce them from JEDEC ID
with conjunction of having SFDP or not.
     Sadly, we need to enter them on a hardware basis.
     As OTP support in driver is for symmetric only.
     We are talking about 2 parameters. bank size and number of banks
as Mactronix offset is always equal to bank size.

Erez




On Fri, 12 Jul 2024 at 22:16, Erez <erezgeva2@gmail.com> wrote:
>
>
>
> On Fri, 12 Jul 2024 at 10:21, Esben Haabendal <esben@geanix.com> wrote:
>>
>> Erez <erezgeva2@gmail.com> writes:
>>
>> > On Thu, 11 Jul 2024 at 21:57, Michael Walle <mwalle@kernel.org> wrote:
>> >> On Thu Jul 11, 2024 at 8:57 PM CEST, Erez wrote:
>> >> > Yes, I think we should.
>> >> >
>> >> > Reading the specification provided publicly by Macronix.
>> >> > For all the JEDEC IDs with the no SFDP flag in drivers/mtd/spi-nor/macronix.c
>> >> > All of them have a new version or a new chip with the same JEDEC ID
>> >> > that supports SFDP.
>> >> > There are 2 chips that Macronix does not provide spec. in public.
>> >> > I can ask Macronix technical support on these 2 chips.
>> >>
>> >> We don't add flashes we cannot test.
>> >
>> > I did not suggest adding anything new.
>> > I refer to the list of chips we already have in drivers/mtd/spi-nor/macronix.c
>> > I presume someone tested them before adding them to the list in the past.
>> > And probably the old chip did not have the SFDP table back then.
>> >
>> > What I checked with the chip specifications is that all Macronix chips
>> > since 2010 have SFDP.
>> >
>> > The situation today is that all Macronix chips that are NOT in the
>> > Macronix table work based on the SFDP table.
>> > But new chips that use a JEDEC found in the Macronix table, skip the
>> > SFDP table and use the setting of the old chip.
>>
>> Not entirely true.
>>
>> Those that entries in the Macronix table that has SPI_NOR_DUAL_READ
>> and/or SPI_NOR_QUAD_READ in no_sfdp_flags is caught by
>> the magic flags matching in spi_nor_init_params_deprecated() and will
>> have spi_nor_parse_sfdp() called from
>> spi_nor_sfdp_init_params_deprecated(). So flashes reusing ID for these
>> will have the SFDP tables parsed.
>>
>> The rest of the entries in the Macronix table is not so lucky. When a
>> replacement chip (with the same ID) is used, it will not be configured
>> with the values found in SFDP table.
>>
>> > So I suggest we read the SFDP table for all Macronix chips.
>>
>> Based on their strategy of re-using flash ID, I think this is a sane
>> approach.
>>
>> > Old Macronix chips that do not have SFDP will use the setting from the
>> > Macronix table. i.e backward compatible.
>> > While new chips which do have an SFDP table will work with the new
>> > setting we find in the table.
>>
>> Yes, if we apply the new SPI_NOR_TRY_SFDP flag to the matching table
>> entries.
>>
>> > Of course, we might have issues in parsing the SFDP table itself.
>> > So we fix them as developers report and send chip ID and part number
>> > with the SFDP table content.
>> > I do not see the point of "hiding" with the old setting.
>> > Anyhow, as we do not like the IDs table and keep it for backward-compatible,
>> > so it only makes sense we should use the SFDP table as much as possible.
>> >
>> > My check was to ensure Tudor that all Macronix chips have SFDP whether
>> > they are in the IDs table or not and we are not wasting a no-op on a
>> > chip which can not have an SFDP table.
>> >
>> > All I suggest is we add the new 'SPI_NOR_TRY_SFDP' flag, to all
>> > Macronix chips.. Which will try to read the SFDP to any Macronix chip.
>>
>> Makes sense. But obviously comes with a risk as we won't be able to
>> test all chips for regression when doing that.
>
>
> Finding an old chip for testing might be a challenge.
> But I think we can simulate the behaviour with a new chip that does support RDSFDP.
>
> We can change the RDSFDP opcode of 0x5A to another value of an unrecognized opcode of the chip we use for testing.
> I assume the behaviour of receiving unrecognized opcodes remains.
>
> Erez
>
>
>
>
>>
>>
>> /Esben

[PATCH v2 4/4] Add Macronix SPI-NOR mx25l12833f with OTP.

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

mx25l12833f uses the same JEDEC ID as mx25l12805d.

The 2 chips have the same flash size.
So user can use mx25l12805d setting with mx25l12833f chip.

mx25l12833f support SFDP and have a bigger symmetric OTP.

Macronix annonce the end of life of mx25l12805d in 2010.

See:
  "https://www.macronix.com/Lists/TechDoc/Attachments/9861/PCN31_2009 PCN_MX25L6405D and MX25L12805D.pdf"

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 drivers/mtd/spi-nor/core.c     | 1 +
 drivers/mtd/spi-nor/macronix.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0f267da339a4..6a5caa199978 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3799,6 +3799,7 @@ static const struct spi_device_id spi_nor_dev_ids[] = {
 	 */
 	{"at25df321a"},	{"at25df641"},	{"at26df081a"},
 	{"mx25l4005a"},	{"mx25l1606e"},	{"mx25l6405d"},	{"mx25l12805d"},
+	{"mx25l12833f"}, /* Uses the same JEDEC ID of mx25l12805d */
 	{"mx25l25635e"},{"mx66l51235l"},
 	{"n25q064"},	{"n25q128a11"},	{"n25q128a13"},	{"n25q512a"},
 	{"s25fl256s1"},	{"s25fl512s"},	{"s25sl12801"},	{"s25fl008k"},
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index f210231468a6..28ae6041fe8c 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -247,6 +247,11 @@ static const struct flash_info macronix_nor_parts[] = {
 		.size = SZ_16M,
 		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP,
 		.no_sfdp_flags = SECT_4K,
+	}, {	/* Yes, Same JEDEC ID as mx25l12805d */
+		.id = SNOR_ID(0xc2, 0x20, 0x18),
+		.name = "mx25l12833f",
+		.flags = SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP,
+		.otp = SNOR_OTP(512, 2, 0x000, 0x200),
 	}, {
 		.id = SNOR_ID(0xc2, 0x20, 0x19),
 		.name = "mx25l25635e",
-- 
2.39.2