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

[PATCH 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.

Erez Geva (4):
  Add generic functions for accessing the SPI 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 1/4] Add generic functions for accessing the SPI-NOR chip.

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Functions:

 - Send a 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 1/4] Add generic functions for accessing the SPI-NOR chip.

[Tudor Ambarus]

Hi, Erez,

On 6/28/24 3:03 PM, Erez Geva wrote:
> From: Erez Geva <ErezGeva2@gmail.com>
> 
> Functions:
> 
>  - Send a opcode
> 
>  - Read a register
> 
>  - Write a register
> 

Please describe your changes. You may want to re-read:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#submitting-patches-the-essential-guide-to-getting-your-code-into-the-kernel

> 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)

and my review stops here. Why did you think it is good to introduce a
_nor* method and not an spi_nor* one?

I'm not going to review the rest of the patches. Please send a v2 that
convinces me to spend more than 2 minutes on it.

Cheers,
ta

[PATCH 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 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Add Macronix SPI-NOR mx25l12833f.

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 3/4] dt-bindings: mtd: macronix,mx25l12833f: add SPI-NOR chip

[Conor Dooley]

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

On Fri, Jun 28, 2024 at 04:03:27PM +0200, Erez Geva wrote:
> From: Erez Geva <ErezGeva2@gmail.com>
> 
> Add Macronix SPI-NOR mx25l12833f.
> 
> Signed-off-by: Erez Geva <ErezGeva2@gmail.com>

Should the email in here and in the From: field be your nwtime one?
Otherwise
Acked-by: Conor Dooley <conor.dooley@microchip.com>


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

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

[Michael Walle]

Hi,

On Fri Jun 28, 2024 at 5:57 PM CEST, Conor Dooley wrote:
> On Fri, Jun 28, 2024 at 04:03:27PM +0200, Erez Geva wrote:
> > From: Erez Geva <ErezGeva2@gmail.com>
> > 
> > Add Macronix SPI-NOR mx25l12833f.
> > 
> > Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
>
> Should the email in here and in the From: field be your nwtime one?
> Otherwise
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

Actually, you're not supposed to add any compatibles to this list.

From the binding:
    description:
      SPI NOR flashes compatible with the JEDEC SFDP standard or which may be
      identified with the READ ID opcode (0x9F) do not deserve a specific
      compatible. They should instead only be matched against the generic
      "jedec,spi-nor" compatible.

I presume the Macronix flash does support the read id opcode.

-michael

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

[Erez]

On Fri, 28 Jun 2024 at 18:30, Erez <erezgeva2@gmail.com> wrote:
>
>
>
> On Fri, 28 Jun 2024 at 18:04, Michael Walle <mwalle@kernel.org> wrote:
>>
>> Hi,
>>
>> On Fri Jun 28, 2024 at 5:57 PM CEST, Conor Dooley wrote:
>> > On Fri, Jun 28, 2024 at 04:03:27PM +0200, Erez Geva wrote:
>> > > From: Erez Geva <ErezGeva2@gmail.com>
>> > >
>> > > Add Macronix SPI-NOR mx25l12833f.
>> > >
>> > > Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
>> >
>> > Should the email in here and in the From: field be your nwtime one?
>> > Otherwise
>> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
>>
>> Actually, you're not supposed to add any compatibles to this list.
>>
>> From the binding:
>>     description:
>>       SPI NOR flashes compatible with the JEDEC SFDP standard or which may be
>>       identified with the READ ID opcode (0x9F) do not deserve a specific
>>       compatible. They should instead only be matched against the generic
>>       "jedec,spi-nor" compatible.
>>
>> I presume the Macronix flash does support the read id opcode.
>
>
> Yes, they do support.
> The new mx25l12833f also supports SFDP.
>
> I do not know why they decided to use the same JEDEC ID for two chips.
> Your guess is as good as mine.

https://www.macronix.com/en-us/products/NOR-Flash/Serial-NOR-Flash/Pages/spec.aspx?p=MX25L12805D&m=Serial%20NOR%20Flash&n=PM1310

End of Life (EOL)
https://www.macronix.com/Lists/TechDoc/Attachments/9861/PCN31_2009%20PCN_MX25L6405D%20and%20MX25L12805D.pdf
Macronix will continue the support of the existing MX25L6405D and
MX25L12805D as the following schedule:
The Last Time Order Date: 31st Aug., 2010
The Last Time Shipment Date: 30th Nov., 2010

Erez

>
> I know the two chips have the same flash size.
> Though the new mx25l12833f chip has a bigger OTP.
> Secondly, the old mx25l12805d has an asymmetric OTP, the 2 regions are of different size.
>
> As I see it, the first 2 patches are the real contribution.
> As I remember, the kernel maintainers usually like to see something using the code.
> I don't care if it was another Macronix chip. I simply tested it with the mx25l12833f chip.
> "[PATCH 1/4] Add generic functions for accessing the SPI-NOR chip."
> "[PATCH 2/4] Add support for SPI-NOR Macronix OTP."
>
> I did not think it is important to have a kernel configuration for choosing mx25l12833f over mx25l12805d.
>
> Erez
>
>
>
>
>
>>
>>
>> -michael

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

[Michael Walle]

On Fri Jun 28, 2024 at 6:30 PM CEST, Erez wrote:
> I do not know why they decided to use the same JEDEC ID for two chips.
> Your guess is as good as mine.

That's a common pattern and we try hard to figure that out during
probe time instead of hardcoding it. E.g. by looking at the SFDP
data. Have a look at various fixups in drivers/mtd/spi-nor/.

compatibles are really the last resort to distinguish flash devices.

Next time, please mention such information in the commit message,
please.

Also please have a look at
https://docs.kernel.org/driver-api/mtd/spi-nor.html

-michael

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

[Erez]

On Fri, 28 Jun 2024 at 18:51, Michael Walle <mwalle@kernel.org> wrote:
>
> On Fri Jun 28, 2024 at 6:30 PM CEST, Erez wrote:
> > I do not know why they decided to use the same JEDEC ID for two chips.
> > Your guess is as good as mine.
>
> That's a common pattern and we try hard to figure that out during
> probe time instead of hardcoding it. E.g. by looking at the SFDP
> data. Have a look at various fixups in drivers/mtd/spi-nor/.

That's a good approach.
The obsolete mx25l12805d does not support the SFDP table.
The new mx25l12833f does.

What is the kernel policy regarding obsolete flash chips?
Macronix annonce on end of life of mx25l12805d in 2010.
Perhaps we should remove mx25l12805d,
 and leave the mx25l12833f configuration in mtd/spi-nor/macronix.c?

>
> compatibles are really the last resort to distinguish flash devices.

I totally agree. Use JEDEC ID a lot better.
In this case Macronix decided to reuse an obsolete chip ID.

>
> Next time, please mention such information in the commit message,
> please.

Thanks for the tip.
I did write it in the fourth patch, but I can add it in part 3 as well.

> Also please have a look at
> https://docs.kernel.org/driver-api/mtd/spi-nor.html

The new mx25l12833f supports SFDP, the obsolete mx25l12805d does not.
I did manage to read the SFDP, though I do not have a copy of it (I do
not have the hardware any more).
To my best knowledge SFDP table do not contain information on OTP,

Thanks
  Erez Geva

>
> -michael

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

[Michael Walle]

> What is the kernel policy regarding obsolete flash chips?
> Macronix annonce on end of life of mx25l12805d in 2010.

Which doesn't mean there are no boards using it. But EOL in 2010
might be a strong argument to remove it. But I don't see the need
for it, yet.

> > Also please have a look at
> > https://docs.kernel.org/driver-api/mtd/spi-nor.html
>
> The new mx25l12833f supports SFDP, the obsolete mx25l12805d does not.
> I did manage to read the SFDP, though I do not have a copy of it (I do
> not have the hardware any more).

So how would you test newer versions of this series?

> To my best knowledge SFDP table do not contain information on OTP,

That's true, but we need it to detect flashes during runtime in the
future. Imagine sometime later, there's a third flash with this
exact ID, then we need to have the SFDP to figure out if there are
any differences between yours and the new one.

-michael

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

[Erez]

On Fri, 28 Jun 2024 at 19:45, Michael Walle <mwalle@kernel.org> wrote:
>
> > What is the kernel policy regarding obsolete flash chips?
> > Macronix annonce on end of life of mx25l12805d in 2010.
>
> Which doesn't mean there are no boards using it. But EOL in 2010
> might be a strong argument to remove it. But I don't see the need
> for it, yet.

Fair enough. That means I was in the right direction.
Just need a better description for the patch :-)
I'll update for version 2.

>
> > > Also please have a look at
> > > https://docs.kernel.org/driver-api/mtd/spi-nor.html
> >
> > The new mx25l12833f supports SFDP, the obsolete mx25l12805d does not.
> > I did manage to read the SFDP, though I do not have a copy of it (I do
> > not have the hardware any more).
>
> So how would you test newer versions of this series?

I develop the OTP callbacks with a mx25l12805d for a company using it.
I am no longer in contact with them.

The testing can be done with any Macronix SPI-NOR with OTP.
I did not check, but I guess most of their chips have an OTP using the
same opcodes/methods.

At least mx25l12805d and mx25l12833f use the same.
I did not add mx25l12833f as I did not use it and as it uses
asymmetric OTP (the MTD supports symmetric OTP).

>
> > To my best knowledge SFDP table do not contain information on OTP,
>
> That's true, but we need it to detect flashes during runtime in the
> future. Imagine sometime later, there's a third flash with this
> exact ID, then we need to have the SFDP to figure out if there are
> any differences between yours and the new one.

Sure, as I said the mx25l12833f does provide SFDP.
In patch 4, I removed the size property to force the driver to read SFDP.
I was not sure about the flags though.
I did not see any difference compared to using the setting of
mx25l12805d without SFDP.
Yet my focus of work was the OTP.

Erez

>
> -michael

[PATCH 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.
mx25l12833f support SFDP and
 have a bigger symmetric OTP.

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..f2a46add2695 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..fba3fc8e0d49 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