[PATCH v2 0/5] mtd: spinand: add OTP support

[PATCH v2 0/5] mtd: spinand: add OTP support

[Martin Kurbanov]

This patchset implements the SPI-NAND OTP functions to allow access to
the SPI-NAND OTP data.
Specific support is added for Micron MT29F2G01ABAGD and ESMT F50L1G41LB/
F50D1G41LB flash chips.

Changelog:
  v2 since v1 at [1]:
    - Make cosmetic changes (Miquel Raynal)

Links:
  [1] https://lore.kernel.org/all/20240617133504.179705-1-mmkurbanov@salutedevices.com/

Martin Kurbanov (5):
  mtd: spinand: make spinand_{read,write}_page global
  mtd: spinand: add OTP support
  mtd: spinand: make spinand_wait() global
  mtd: spinand: micron: OTP access for MT29F2G01ABAGD
  mtd: spinand: esmt: OTP access for F50{L,D}1G41LB

 drivers/mtd/nand/spi/Makefile |   3 +-
 drivers/mtd/nand/spi/core.c   |  45 +++++--
 drivers/mtd/nand/spi/esmt.c   |  69 +++++++++-
 drivers/mtd/nand/spi/micron.c | 117 ++++++++++++++++-
 drivers/mtd/nand/spi/otp.c    | 232 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h   |  65 ++++++++++
 6 files changed, 519 insertions(+), 12 deletions(-)
 create mode 100644 drivers/mtd/nand/spi/otp.c

-- 
2.43.2

[PATCH v2 1/5] mtd: spinand: make spinand_{read,write}_page global

[Martin Kurbanov]

Change these functions from static to global so that to use them later
in OTP operations. Since reading OTP pages is no different from reading
pages from the main area.

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 drivers/mtd/nand/spi/core.c | 24 ++++++++++++++++++++----
 include/linux/mtd/spinand.h |  6 ++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index e0b6715e5dfed..807c24b0c7c4f 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -566,8 +566,16 @@ static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
 	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
 }
 
-static int spinand_read_page(struct spinand_device *spinand,
-			     const struct nand_page_io_req *req)
+/**
+ * spinand_read_page() - Read the page
+ * @spinand: the spinand device
+ * @req: the I/O request
+ *
+ * Return: 0 or a positive number of bitflips corrected on success.
+ * A negative error code otherwise.
+ */
+int spinand_read_page(struct spinand_device *spinand,
+		      const struct nand_page_io_req *req)
 {
 	struct nand_device *nand = spinand_to_nand(spinand);
 	u8 status;
@@ -597,8 +605,16 @@ static int spinand_read_page(struct spinand_device *spinand,
 	return nand_ecc_finish_io_req(nand, (struct nand_page_io_req *)req);
 }
 
-static int spinand_write_page(struct spinand_device *spinand,
-			      const struct nand_page_io_req *req)
+/**
+ * spinand_write_page() - Write the page
+ * @spinand: the spinand device
+ * @req: the I/O request
+ *
+ * Return: 0 or a positive number of bitflips corrected on success.
+ * A negative error code otherwise.
+ */
+int spinand_write_page(struct spinand_device *spinand,
+		       const struct nand_page_io_req *req)
 {
 	struct nand_device *nand = spinand_to_nand(spinand);
 	u8 status;
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 5c19ead604996..555846517faf6 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -519,4 +519,10 @@ int spinand_match_and_init(struct spinand_device *spinand,
 int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
 int spinand_select_target(struct spinand_device *spinand, unsigned int target);
 
+int spinand_read_page(struct spinand_device *spinand,
+		      const struct nand_page_io_req *req);
+
+int spinand_write_page(struct spinand_device *spinand,
+		       const struct nand_page_io_req *req);
+
 #endif /* __LINUX_MTD_SPINAND_H */
-- 
2.43.2

Re: [PATCH v2 1/5] mtd: spinand: make spinand_{read,write}_page global

[Miquel Raynal]

Hi Martin,

mmkurbanov@salutedevices.com wrote on Tue, 27 Aug 2024 20:48:59 +0300:

> Change these functions from static to global so that to use them later
> in OTP operations. Since reading OTP pages is no different from reading
> pages from the main area.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
> ---
>  drivers/mtd/nand/spi/core.c | 24 ++++++++++++++++++++----
>  include/linux/mtd/spinand.h |  6 ++++++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index e0b6715e5dfed..807c24b0c7c4f 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -566,8 +566,16 @@ static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
>  	return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
>  }
>  
> -static int spinand_read_page(struct spinand_device *spinand,
> -			     const struct nand_page_io_req *req)
> +/**
> + * spinand_read_page() - Read the page

				 a page? (same below)

> + * @spinand: the spinand device
> + * @req: the I/O request
> + *
> + * Return: 0 or a positive number of bitflips corrected on success.
> + * A negative error code otherwise.
> + */
> +int spinand_read_page(struct spinand_device *spinand,
> +		      const struct nand_page_io_req *req)
>  {
>  	struct nand_device *nand = spinand_to_nand(spinand);
>  	u8 status;
> @@ -597,8 +605,16 @@ static int spinand_read_page(struct spinand_device *spinand,
>  	return nand_ecc_finish_io_req(nand, (struct nand_page_io_req *)req);
>  }
>  
> -static int spinand_write_page(struct spinand_device *spinand,
> -			      const struct nand_page_io_req *req)
> +/**
> + * spinand_write_page() - Write the page
> + * @spinand: the spinand device
> + * @req: the I/O request
> + *
> + * Return: 0 or a positive number of bitflips corrected on success.
> + * A negative error code otherwise.
> + */
> +int spinand_write_page(struct spinand_device *spinand,
> +		       const struct nand_page_io_req *req)
>  {
>  	struct nand_device *nand = spinand_to_nand(spinand);
>  	u8 status;
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 5c19ead604996..555846517faf6 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -519,4 +519,10 @@ int spinand_match_and_init(struct spinand_device *spinand,
>  int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
>  int spinand_select_target(struct spinand_device *spinand, unsigned int target);
>  
> +int spinand_read_page(struct spinand_device *spinand,
> +		      const struct nand_page_io_req *req);
> +
> +int spinand_write_page(struct spinand_device *spinand,
> +		       const struct nand_page_io_req *req);
> +
>  #endif /* __LINUX_MTD_SPINAND_H */


Thanks,
Miquèl

[PATCH v2 2/5] mtd: spinand: add OTP support

[Martin Kurbanov]

The MTD subsystem already supports accessing two OTP areas: user and
factory. User areas can be written by the user. This patch only adds
support for the user areas.

In this patch the OTP_INFO macro is provided to add parameters to
spinand_info.
To implement OTP operations, the client (flash driver) is provided with
5 callbacks: .read(), .write(), .info(), .lock(), .erase().

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 drivers/mtd/nand/spi/Makefile |   3 +-
 drivers/mtd/nand/spi/core.c   |   3 +
 drivers/mtd/nand/spi/otp.c    | 232 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h   |  56 ++++++++
 4 files changed, 293 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mtd/nand/spi/otp.c

diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
index 19cc77288ebbc..60d2e830ffc6b 100644
--- a/drivers/mtd/nand/spi/Makefile
+++ b/drivers/mtd/nand/spi/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
+spinand-objs := core.o otp.o
+spinand-objs += alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
 spinand-objs += micron.o paragon.o toshiba.o winbond.o xtx.o
 obj-$(CONFIG_MTD_SPI_NAND) += spinand.o
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 807c24b0c7c4f..2cb825edd49d0 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1111,6 +1111,7 @@ int spinand_match_and_init(struct spinand_device *spinand,
 		spinand->flags = table[i].flags;
 		spinand->id.len = 1 + table[i].devid.len;
 		spinand->select_target = table[i].select_target;
+		spinand->otp = &table[i].otp;
 
 		op = spinand_select_op_variant(spinand,
 					       info->op_variants.read_cache);
@@ -1292,6 +1293,8 @@ static int spinand_init(struct spinand_device *spinand)
 	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
 	mtd->_resume = spinand_mtd_resume;
 
+	spinand_set_mtd_otp_ops(spinand);
+
 	if (nand->ecc.engine) {
 		ret = mtd_ooblayout_count_freebytes(mtd);
 		if (ret < 0)
diff --git a/drivers/mtd/nand/spi/otp.c b/drivers/mtd/nand/spi/otp.c
new file mode 100644
index 0000000000000..d459f811f9c04
--- /dev/null
+++ b/drivers/mtd/nand/spi/otp.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ *
+ * Author: Martin Kurbanov <mmkurbanov@salutedevices.com>
+ */
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spinand.h>
+
+static unsigned int spinand_otp_npages(const struct spinand_device *spinand)
+{
+	return spinand->otp->layout.npages;
+}
+
+static size_t spinand_otp_size(struct spinand_device *spinand)
+{
+	struct nand_device *nand = spinand_to_nand(spinand);
+	size_t otp_pagesize = nanddev_page_size(nand) +
+			      nanddev_per_page_oobsize(nand);
+
+	return spinand_otp_npages(spinand) * otp_pagesize;
+}
+
+static int spinand_otp_rw(struct spinand_device *spinand, loff_t ofs,
+			  size_t len, u8 *buf, size_t *retlen, bool is_write)
+{
+	struct nand_device *nand = spinand_to_nand(spinand);
+	struct nand_page_io_req req = { 0 };
+	unsigned long long page;
+	size_t copied = 0;
+	size_t otp_pagesize = nanddev_page_size(nand) +
+			      nanddev_per_page_oobsize(nand);
+	int ret = 0;
+
+	page = ofs;
+	req.dataoffs = do_div(page, otp_pagesize);
+	req.pos.page = page;
+	req.type = is_write ? NAND_PAGE_WRITE : NAND_PAGE_READ;
+	req.mode = MTD_OPS_RAW;
+	req.databuf.in = buf;
+
+	while (copied < len && req.pos.page < spinand_otp_npages(spinand)) {
+		req.datalen = min_t(unsigned int,
+				    otp_pagesize - req.dataoffs,
+				    len - copied);
+
+		if (is_write)
+			ret = spinand_write_page(spinand, &req);
+		else
+			ret = spinand_read_page(spinand, &req);
+
+		if (ret < 0)
+			break;
+
+		req.dataoffs = 0;
+		copied += req.datalen;
+		req.pos.page++;
+	}
+
+	*retlen = copied;
+
+	return ret;
+}
+
+/**
+ * spinand_otp_read() - Read from OTP area
+ * @spinand: the spinand device
+ * @ofs: the offset to read
+ * @len: the number of data bytes to read
+ * @buf: the buffer to store the read data
+ * @retlen: the pointer to variable to store the number of read bytes
+ *
+ * Return: 0 on success, an error code otherwise.
+ */
+int spinand_otp_read(struct spinand_device *spinand, loff_t ofs, size_t len,
+		     u8 *buf, size_t *retlen)
+{
+	return spinand_otp_rw(spinand, ofs, len, buf, retlen, false);
+}
+
+/**
+ * spinand_otp_write() - Write to OTP area
+ * @spinand:  the spinand device
+ * @ofs: the offset to write to
+ * @len: the number of bytes to write
+ * @buf: the buffer with data to write
+ * @retlen: the pointer to variable to store the number of written bytes
+ *
+ * Return: 0 on success, an error code otherwise.
+ */
+int spinand_otp_write(struct spinand_device *spinand, loff_t ofs, size_t len,
+		      const u8 *buf, size_t *retlen)
+{
+	return spinand_otp_rw(spinand, ofs, len, (u8 *)buf, retlen, true);
+}
+
+static int spinand_otp_check_bounds(struct spinand_device *spinand, loff_t ofs,
+				    size_t len)
+{
+	if (ofs < 0 || ofs + len > spinand_otp_size(spinand))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int spinand_mtd_otp_info(struct mtd_info *mtd, size_t len,
+				size_t *retlen, struct otp_info *buf)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	const struct spinand_otp_ops *ops = spinand->otp->ops;
+	int ret;
+
+	mutex_lock(&spinand->lock);
+	ret = ops->info(spinand, len, buf, retlen);
+	mutex_unlock(&spinand->lock);
+
+	return ret;
+}
+
+static int spinand_mtd_otp_rw(struct mtd_info *mtd, loff_t ofs, size_t len,
+			      size_t *retlen, u8 *buf, bool is_write)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	const struct spinand_otp_ops *ops = spinand->otp->ops;
+	int ret;
+
+	if (!len)
+		return 0;
+
+	ret = spinand_otp_check_bounds(spinand, ofs, len);
+	if (ret)
+		return ret;
+
+	mutex_lock(&spinand->lock);
+
+	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, CFG_OTP_ENABLE);
+	if (ret)
+		goto out_unlock;
+
+	if (is_write)
+		ret = ops->write(spinand, ofs, len, buf, retlen);
+	else
+		ret = ops->read(spinand, ofs, len, buf, retlen);
+
+	if (spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0)) {
+		pr_warn(1, "Can not disable OTP mode\n");
+		ret = -EIO;
+	}
+
+out_unlock:
+	mutex_unlock(&spinand->lock);
+	return ret;
+}
+
+static int spinand_mtd_otp_read(struct mtd_info *mtd, loff_t ofs, size_t len,
+				size_t *retlen, u8 *buf)
+{
+	return spinand_mtd_otp_rw(mtd, ofs, len, retlen, buf, false);
+}
+
+static int spinand_mtd_otp_write(struct mtd_info *mtd, loff_t ofs, size_t len,
+				 size_t *retlen, const u8 *buf)
+{
+	return spinand_mtd_otp_rw(mtd, ofs, len, retlen, (u8 *)buf, true);
+}
+
+static int spinand_mtd_otp_erase(struct mtd_info *mtd, loff_t ofs, size_t len)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	const struct spinand_otp_ops *ops = spinand->otp->ops;
+	int ret;
+
+	if (!ops->erase)
+		return -EOPNOTSUPP;
+
+	if (!len)
+		return 0;
+
+	ret = spinand_otp_check_bounds(spinand, ofs, len);
+	if (ret)
+		return ret;
+
+	mutex_lock(&spinand->lock);
+	ret = ops->erase(spinand, ofs, len);
+	mutex_unlock(&spinand->lock);
+
+	return ret;
+}
+
+static int spinand_mtd_otp_lock(struct mtd_info *mtd, loff_t ofs, size_t len)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	const struct spinand_otp_ops *ops = spinand->otp->ops;
+	int ret;
+
+	if (!ops->lock)
+		return -EOPNOTSUPP;
+
+	if (!len)
+		return 0;
+
+	ret = spinand_otp_check_bounds(spinand, ofs, len);
+	if (ret)
+		return ret;
+
+	mutex_lock(&spinand->lock);
+	ret = ops->lock(spinand, ofs, len);
+	mutex_unlock(&spinand->lock);
+
+	return ret;
+}
+
+/**
+ * spinand_set_mtd_otp_ops() - Set up OTP methods
+ * @spinand: the spinand device
+ *
+ * Set up OTP methods.
+ */
+void spinand_set_mtd_otp_ops(struct spinand_device *spinand)
+{
+	struct mtd_info *mtd = spinand_to_mtd(spinand);
+
+	if (!spinand->otp->ops)
+		return;
+
+	mtd->_get_user_prot_info = spinand_mtd_otp_info;
+	mtd->_read_user_prot_reg = spinand_mtd_otp_read;
+	mtd->_write_user_prot_reg = spinand_mtd_otp_write;
+	mtd->_lock_user_prot_reg = spinand_mtd_otp_lock;
+	mtd->_erase_user_prot_reg = spinand_mtd_otp_erase;
+}
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 555846517faf6..8099f35f0e051 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -322,6 +322,43 @@ struct spinand_ondie_ecc_conf {
 	u8 status;
 };
 
+/**
+ * struct spinand_otp_layout - structure to describe the SPI NAND OTP area
+ * @npages: number of pages in the OTP
+ */
+struct spinand_otp_layout {
+	unsigned int npages;
+};
+
+/**
+ * struct spinand_otp_ops - SPI NAND OTP methods
+ * @info: Get the OTP area information
+ * @lock: lock an OTP region
+ * @erase: erase an OTP region
+ * @read: read from the SPI NAND OTP area
+ * @write: write to the SPI NAND OTP area
+ */
+struct spinand_otp_ops {
+	int (*info)(struct spinand_device *spinand, size_t len,
+		    struct otp_info *buf, size_t *retlen);
+	int (*lock)(struct spinand_device *spinand, loff_t from, size_t len);
+	int (*erase)(struct spinand_device *spinand, loff_t from, size_t len);
+	int (*read)(struct spinand_device *spinand, loff_t from, size_t len,
+		    u8 *buf, size_t *retlen);
+	int (*write)(struct spinand_device *spinand, loff_t from, size_t len,
+		     const u8 *buf, size_t *retlen);
+};
+
+/**
+ * struct spinand_otp - SPI NAND OTP grouping structure
+ * @layout: OTP region layout
+ * @ops: OTP access ops
+ */
+struct spinand_otp {
+	const struct spinand_otp_layout layout;
+	const struct spinand_otp_ops *ops;
+};
+
 /**
  * struct spinand_info - Structure used to describe SPI NAND chips
  * @model: model name
@@ -354,6 +391,7 @@ struct spinand_info {
 	} op_variants;
 	int (*select_target)(struct spinand_device *spinand,
 			     unsigned int target);
+	struct spinand_otp otp;
 };
 
 #define SPINAND_ID(__method, ...)					\
@@ -379,6 +417,14 @@ struct spinand_info {
 #define SPINAND_SELECT_TARGET(__func)					\
 	.select_target = __func,
 
+#define SPINAND_OTP_INFO(__npages, __ops)				\
+	.otp = {							\
+		.layout = {						\
+			.npages = __npages,				\
+		},							\
+		.ops = __ops,						\
+	}
+
 #define SPINAND_INFO(__model, __id, __memorg, __eccreq, __op_variants,	\
 		     __flags, ...)					\
 	{								\
@@ -422,6 +468,7 @@ struct spinand_dirmap {
  *		passed in spi_mem_op be DMA-able, so we can't based the bufs on
  *		the stack
  * @manufacturer: SPI NAND manufacturer information
+ * @otp: SPI NAND OTP info.
  * @priv: manufacturer private data
  */
 struct spinand_device {
@@ -450,6 +497,7 @@ struct spinand_device {
 	u8 *oobbuf;
 	u8 *scratchbuf;
 	const struct spinand_manufacturer *manufacturer;
+	const struct spinand_otp *otp;
 	void *priv;
 };
 
@@ -525,4 +573,12 @@ int spinand_read_page(struct spinand_device *spinand,
 int spinand_write_page(struct spinand_device *spinand,
 		       const struct nand_page_io_req *req);
 
+void spinand_set_mtd_otp_ops(struct spinand_device *spinand);
+
+int spinand_otp_read(struct spinand_device *spinand, loff_t ofs, size_t len,
+		     u8 *buf, size_t *retlen);
+
+int spinand_otp_write(struct spinand_device *spinand, loff_t ofs, size_t len,
+		      const u8 *buf, size_t *retlen);
+
 #endif /* __LINUX_MTD_SPINAND_H */
-- 
2.43.2

Re: [PATCH v2 2/5] mtd: spinand: add OTP support

[Martin Kurbanov]

Hello, Miquel. Thank you for the review.
Regarding your question ( https://lore.kernel.org/all/20240717103623.6d6b63be@xps-13/ ):

>> +int spinand_otp_read(struct spinand_device *spinand, loff_t from, size_t len,
>> +		     u8 *buf, size_t *retlen);
>> +
>> +int spinand_otp_write(struct spinand_device *spinand, loff_t from, size_t len,
>> +		      const u8 *buf, size_t *retlen);
>> +
> 
> Why exposing spinand_otp_read and spinand_otp_write ?

For the SPI-NAND chips we have (Micron, ESMT, FORESEE), the command
sequence for reading/writing OTP is the same. I decided to make these
functions global because other chips probably have similar read/write
OTP operations as well.

-- 
Best Regards,
Martin Kurbanov

Re: [PATCH v2 2/5] mtd: spinand: add OTP support

[Miquel Raynal]

Hi Martin,

mmkurbanov@salutedevices.com wrote on Tue, 27 Aug 2024 20:57:04 +0300:

> Hello, Miquel. Thank you for the review.
> Regarding your question ( https://lore.kernel.org/all/20240717103623.6d6b63be@xps-13/ ):
> 
> >> +int spinand_otp_read(struct spinand_device *spinand, loff_t from, size_t len,
> >> +		     u8 *buf, size_t *retlen);
> >> +
> >> +int spinand_otp_write(struct spinand_device *spinand, loff_t from, size_t len,
> >> +		      const u8 *buf, size_t *retlen);
> >> +  
> > 
> > Why exposing spinand_otp_read and spinand_otp_write ?  
> 
> For the SPI-NAND chips we have (Micron, ESMT, FORESEE), the command
> sequence for reading/writing OTP is the same. I decided to make these
> functions global because other chips probably have similar read/write
> OTP operations as well.

Of course, I understand you might need them, but then the change does
not belong to this patch. Actually you've done that correctly for the
spinand_wait() helper.

You can do all the "make foo() global" operations in a single patch if
you want.

I will soon review more in deep the content of this patchset again.

Thanks,
Miquèl

Re: [PATCH v2 2/5] mtd: spinand: add OTP support

[Miquel Raynal]

Hi Martin,

mmkurbanov@salutedevices.com wrote on Tue, 27 Aug 2024 20:49:00 +0300:

> The MTD subsystem already supports accessing two OTP areas: user and
> factory. User areas can be written by the user. This patch only adds
> support for the user areas.
> 
> In this patch the OTP_INFO macro is provided to add parameters to
> spinand_info.
> To implement OTP operations, the client (flash driver) is provided with
> 5 callbacks: .read(), .write(), .info(), .lock(), .erase().

Overall this looks good, I have minor changes to request. I'm not yet
done down to the last patch, but I think the implementation is neat.

> Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
> ---
>  drivers/mtd/nand/spi/Makefile |   3 +-
>  drivers/mtd/nand/spi/core.c   |   3 +
>  drivers/mtd/nand/spi/otp.c    | 232 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h   |  56 ++++++++
>  4 files changed, 293 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/nand/spi/otp.c
> 
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> index 19cc77288ebbc..60d2e830ffc6b 100644
> --- a/drivers/mtd/nand/spi/Makefile
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
> -spinand-objs := core.o alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
> +spinand-objs := core.o otp.o
> +spinand-objs += alliancememory.o ato.o esmt.o foresee.o gigadevice.o macronix.o
>  spinand-objs += micron.o paragon.o toshiba.o winbond.o xtx.o
>  obj-$(CONFIG_MTD_SPI_NAND) += spinand.o
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 807c24b0c7c4f..2cb825edd49d0 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1111,6 +1111,7 @@ int spinand_match_and_init(struct spinand_device *spinand,
>  		spinand->flags = table[i].flags;
>  		spinand->id.len = 1 + table[i].devid.len;
>  		spinand->select_target = table[i].select_target;
> +		spinand->otp = &table[i].otp;
>  
>  		op = spinand_select_op_variant(spinand,
>  					       info->op_variants.read_cache);
> @@ -1292,6 +1293,8 @@ static int spinand_init(struct spinand_device *spinand)
>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
>  	mtd->_resume = spinand_mtd_resume;
>  
> +	spinand_set_mtd_otp_ops(spinand);
> +
>  	if (nand->ecc.engine) {
>  		ret = mtd_ooblayout_count_freebytes(mtd);
>  		if (ret < 0)
> diff --git a/drivers/mtd/nand/spi/otp.c b/drivers/mtd/nand/spi/otp.c
> new file mode 100644
> index 0000000000000..d459f811f9c04
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/otp.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> + *
> + * Author: Martin Kurbanov <mmkurbanov@salutedevices.com>
> + */
> +
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/spinand.h>
> +
> +static unsigned int spinand_otp_npages(const struct spinand_device *spinand)
> +{
> +	return spinand->otp->layout.npages;
> +}
> +
> +static size_t spinand_otp_size(struct spinand_device *spinand)
> +{
> +	struct nand_device *nand = spinand_to_nand(spinand);
> +	size_t otp_pagesize = nanddev_page_size(nand) +
> +			      nanddev_per_page_oobsize(nand);
> +
> +	return spinand_otp_npages(spinand) * otp_pagesize;
> +}
> +
> +static int spinand_otp_rw(struct spinand_device *spinand, loff_t ofs,
> +			  size_t len, u8 *buf, size_t *retlen, bool is_write)
> +{
> +	struct nand_device *nand = spinand_to_nand(spinand);
> +	struct nand_page_io_req req = { 0 };

					= {}; is enough

> +	unsigned long long page;
> +	size_t copied = 0;
> +	size_t otp_pagesize = nanddev_page_size(nand) +
> +			      nanddev_per_page_oobsize(nand);
> +	int ret = 0;
> +
> +	page = ofs;
> +	req.dataoffs = do_div(page, otp_pagesize);
> +	req.pos.page = page;
> +	req.type = is_write ? NAND_PAGE_WRITE : NAND_PAGE_READ;
> +	req.mode = MTD_OPS_RAW;
> +	req.databuf.in = buf;
> +
> +	while (copied < len && req.pos.page < spinand_otp_npages(spinand)) {
> +		req.datalen = min_t(unsigned int,
> +				    otp_pagesize - req.dataoffs,
> +				    len - copied);
> +
> +		if (is_write)
> +			ret = spinand_write_page(spinand, &req);
> +		else
> +			ret = spinand_read_page(spinand, &req);
> +
> +		if (ret < 0)
> +			break;
> +
> +		req.dataoffs = 0;
> +		copied += req.datalen;
> +		req.pos.page++;
> +	}
> +
> +	*retlen = copied;
> +
> +	return ret;
> +}
> +
> +/**
> + * spinand_otp_read() - Read from OTP area
> + * @spinand: the spinand device
> + * @ofs: the offset to read
> + * @len: the number of data bytes to read
> + * @buf: the buffer to store the read data
> + * @retlen: the pointer to variable to store the number of read bytes
> + *
> + * Return: 0 on success, an error code otherwise.
> + */
> +int spinand_otp_read(struct spinand_device *spinand, loff_t ofs, size_t len,
> +		     u8 *buf, size_t *retlen)
> +{
> +	return spinand_otp_rw(spinand, ofs, len, buf, retlen, false);
> +}
> +
> +/**
> + * spinand_otp_write() - Write to OTP area
> + * @spinand:  the spinand device
> + * @ofs: the offset to write to
> + * @len: the number of bytes to write
> + * @buf: the buffer with data to write
> + * @retlen: the pointer to variable to store the number of written bytes
> + *
> + * Return: 0 on success, an error code otherwise.
> + */
> +int spinand_otp_write(struct spinand_device *spinand, loff_t ofs, size_t len,
> +		      const u8 *buf, size_t *retlen)
> +{
> +	return spinand_otp_rw(spinand, ofs, len, (u8 *)buf, retlen, true);
> +}

These spinand_otp_xxx() helpers are not yet used, and thus should be
introduced later, when they are useful.

> +
> +static int spinand_otp_check_bounds(struct spinand_device *spinand, loff_t ofs,
> +				    size_t len)
> +{
> +	if (ofs < 0 || ofs + len > spinand_otp_size(spinand))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int spinand_mtd_otp_info(struct mtd_info *mtd, size_t len,
> +				size_t *retlen, struct otp_info *buf)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	mutex_lock(&spinand->lock);
> +	ret = ops->info(spinand, len, buf, retlen);
> +	mutex_unlock(&spinand->lock);
> +
> +	return ret;
> +}
> +
> +static int spinand_mtd_otp_rw(struct mtd_info *mtd, loff_t ofs, size_t len,
> +			      size_t *retlen, u8 *buf, bool is_write)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	if (!len)
> +		return 0;
> +
> +	ret = spinand_otp_check_bounds(spinand, ofs, len);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&spinand->lock);
> +
> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, CFG_OTP_ENABLE);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (is_write)
> +		ret = ops->write(spinand, ofs, len, buf, retlen);
> +	else
> +		ret = ops->read(spinand, ofs, len, buf, retlen);
> +
> +	if (spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0)) {
> +		pr_warn(1, "Can not disable OTP mode\n");

dev_warn?

> +		ret = -EIO;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&spinand->lock);
> +	return ret;
> +}
> +
> +static int spinand_mtd_otp_read(struct mtd_info *mtd, loff_t ofs, size_t len,
> +				size_t *retlen, u8 *buf)
> +{
> +	return spinand_mtd_otp_rw(mtd, ofs, len, retlen, buf, false);
> +}
> +
> +static int spinand_mtd_otp_write(struct mtd_info *mtd, loff_t ofs, size_t len,
> +				 size_t *retlen, const u8 *buf)
> +{
> +	return spinand_mtd_otp_rw(mtd, ofs, len, retlen, (u8 *)buf, true);
> +}
> +
> +static int spinand_mtd_otp_erase(struct mtd_info *mtd, loff_t ofs, size_t len)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	if (!ops->erase)
> +		return -EOPNOTSUPP;
> +
> +	if (!len)
> +		return 0;
> +
> +	ret = spinand_otp_check_bounds(spinand, ofs, len);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&spinand->lock);
> +	ret = ops->erase(spinand, ofs, len);
> +	mutex_unlock(&spinand->lock);
> +
> +	return ret;
> +}
> +
> +static int spinand_mtd_otp_lock(struct mtd_info *mtd, loff_t ofs, size_t len)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	const struct spinand_otp_ops *ops = spinand->otp->ops;
> +	int ret;
> +
> +	if (!ops->lock)
> +		return -EOPNOTSUPP;
> +
> +	if (!len)
> +		return 0;
> +
> +	ret = spinand_otp_check_bounds(spinand, ofs, len);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&spinand->lock);
> +	ret = ops->lock(spinand, ofs, len);
> +	mutex_unlock(&spinand->lock);
> +
> +	return ret;
> +}
> +
> +/**
> + * spinand_set_mtd_otp_ops() - Set up OTP methods
> + * @spinand: the spinand device
> + *
> + * Set up OTP methods.
> + */
> +void spinand_set_mtd_otp_ops(struct spinand_device *spinand)
> +{
> +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> +
> +	if (!spinand->otp->ops)

Could we use something else as check? It feels odd to check for otp ops
and then just ignore the fact that they are here. Maybe check npages or
otp_size() ?

> +		return;
> +
> +	mtd->_get_user_prot_info = spinand_mtd_otp_info;
> +	mtd->_read_user_prot_reg = spinand_mtd_otp_read;
> +	mtd->_write_user_prot_reg = spinand_mtd_otp_write;
> +	mtd->_lock_user_prot_reg = spinand_mtd_otp_lock;
> +	mtd->_erase_user_prot_reg = spinand_mtd_otp_erase;
> +}
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 555846517faf6..8099f35f0e051 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -322,6 +322,43 @@ struct spinand_ondie_ecc_conf {
>  	u8 status;
>  };

Thanks,
Miquèl

Re: [PATCH v2 2/5] mtd: spinand: add OTP support

[Martin Kurbanov]

Hi Miquel,

On 10/1/24 12:12, Miquel Raynal wrote:

>> +/**
>> + * spinand_set_mtd_otp_ops() - Set up OTP methods
>> + * @spinand: the spinand device
>> + *
>> + * Set up OTP methods.
>> + */
>> +void spinand_set_mtd_otp_ops(struct spinand_device *spinand)
>> +{
>> +	struct mtd_info *mtd = spinand_to_mtd(spinand);
>> +
>> +	if (!spinand->otp->ops)
> 
> Could we use something else as check? It feels odd to check for otp ops
> and then just ignore the fact that they are here. Maybe check npages or
> otp_size() ?

A developer may not specify OTP callbacks:
SPINAND_OTP_INFO(otp_pages, NULL /* OTP ops */)

Or do you mean that it is better to check in each function
spinand_mtd_otp_{info,read,write,lock}? E.g.:
static int spinand_mtd_otp_erase(struct mtd_info *mtd, loff_t ofs, size_t len)
{
	struct spinand_device *spinand = mtd_to_spinand(mtd);
	const struct spinand_otp_ops *ops = spinand->otp->ops;
	int ret;

	if (!ops || !ops->erase)
		return -EOPNOTSUPP;


-- 
Best Regards,
Martin Kurbanov

Re: [PATCH v2 2/5] mtd: spinand: add OTP support

[Miquel Raynal]

Hi Martin,

Sorry for the slow feedback.

> >> +/**
> >> + * spinand_set_mtd_otp_ops() - Set up OTP methods
> >> + * @spinand: the spinand device
> >> + *
> >> + * Set up OTP methods.
> >> + */
> >> +void spinand_set_mtd_otp_ops(struct spinand_device *spinand)
> >> +{
> >> +	struct mtd_info *mtd = spinand_to_mtd(spinand);
> >> +
> >> +	if (!spinand->otp->ops)  
> > 
> > Could we use something else as check? It feels odd to check for otp ops
> > and then just ignore the fact that they are here. Maybe check npages or
> > otp_size() ?  
> 
> A developer may not specify OTP callbacks:
> SPINAND_OTP_INFO(otp_pages, NULL /* OTP ops */)

Is this really a valid situation?

In set_mtd_otp_ops() you set spinand functions only if there are otp
operations. First, is it relevant to consider the fact that a device
would have an otp and not provide operations? Otherwise, my initial
comment was about the fact that the check seems uncorrelated with the
second part of the function.

Maybe setting these functions only if relevant is the best choice, so
you no longer have to make the checks after the init.

	if (ops && ops->erase)
		mtd->_otp_erase = spinand_otp_erase;
	...

?

Thanks,
Miquèl

[PATCH v2 3/5] mtd: spinand: make spinand_wait() global

[Martin Kurbanov]

Change the function spinand_wait() from static to global so that SPI
NAND flash drivers don't duplicate it.

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 drivers/mtd/nand/spi/core.c | 18 ++++++++++++++----
 include/linux/mtd/spinand.h |  3 +++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 2cb825edd49d0..c3e3d1e9599f1 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -496,10 +496,20 @@ static int spinand_erase_op(struct spinand_device *spinand,
 	return spi_mem_exec_op(spinand->spimem, &op);
 }
 
-static int spinand_wait(struct spinand_device *spinand,
-			unsigned long initial_delay_us,
-			unsigned long poll_delay_us,
-			u8 *s)
+/**
+ * spinand_wait() - Poll memory device status
+ * @spinand: the spinand device
+ * @initial_delay_us: delay in us before starting to poll
+ * @poll_delay_us: time to sleep between reads in us
+ * @s: the pointer to variable to store the value of REG_STATUS
+ *
+ * This function polls a status register (REG_STATUS) and returns when
+ * the STATUS_READY bit is 0 or when the timeout has expired.
+ *
+ * Return: 0 on success, a negative error code otherwise.
+ */
+int spinand_wait(struct spinand_device *spinand, unsigned long initial_delay_us,
+		 unsigned long poll_delay_us, u8 *s)
 {
 	struct spi_mem_op op = SPINAND_GET_FEATURE_OP(REG_STATUS,
 						      spinand->scratchbuf);
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 8099f35f0e051..5768968933530 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -567,6 +567,9 @@ int spinand_match_and_init(struct spinand_device *spinand,
 int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
 int spinand_select_target(struct spinand_device *spinand, unsigned int target);
 
+int spinand_wait(struct spinand_device *spinand, unsigned long initial_delay_us,
+		 unsigned long poll_delay_us, u8 *s);
+
 int spinand_read_page(struct spinand_device *spinand,
 		      const struct nand_page_io_req *req);
 
-- 
2.43.2

[PATCH v2 4/5] mtd: spinand: micron: OTP access for MT29F2G01ABAGD

[Martin Kurbanov]

Support for OTP area access on Micron MT29F2G01ABAGD chip.

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 drivers/mtd/nand/spi/micron.c | 117 +++++++++++++++++++++++++++++++++-
 1 file changed, 116 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
index 8d741be6d5f3e..a538409db4ccd 100644
--- a/drivers/mtd/nand/spi/micron.c
+++ b/drivers/mtd/nand/spi/micron.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/mtd/spinand.h>
+#include <linux/spi/spi-mem.h>
 
 #define SPINAND_MFR_MICRON		0x2c
 
@@ -28,6 +29,16 @@
 
 #define MICRON_SELECT_DIE(x)	((x) << 6)
 
+#define MICRON_MT29F2G01ABAGD_OTP_PAGES			12
+#define MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE		2176
+#define MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES		\
+	(MICRON_MT29F2G01ABAGD_OTP_PAGES *		\
+	 MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE)
+
+#define MICRON_MT29F2G01ABAGD_CFG_OTP_STATE		BIT(7)
+#define MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK		\
+	(CFG_OTP_ENABLE | MICRON_MT29F2G01ABAGD_CFG_OTP_STATE)
+
 static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
 		//SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
 		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
@@ -182,6 +193,108 @@ static int micron_8_ecc_get_status(struct spinand_device *spinand,
 	return -EINVAL;
 }
 
+static int mt29f2g01abagd_otp_is_locked(struct spinand_device *spinand)
+{
+	size_t buf_size = MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE;
+	size_t retlen;
+	u8 *buf;
+	int ret;
+
+	buf = kmalloc(buf_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = spinand_upd_cfg(spinand,
+			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
+			      MICRON_MT29F2G01ABAGD_CFG_OTP_STATE);
+	if (ret)
+		goto out;
+
+	ret = spinand_otp_read(spinand, 0, buf_size, buf, &retlen);
+
+	if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
+			    0)) {
+		WARN(1, "Can not disable OTP mode\n");
+		ret = -EIO;
+	}
+
+	if (!ret) {
+		size_t i = 0;
+
+		/* If all zeros, then the OTP area is locked. */
+		while (i < buf_size && *(uint32_t *)(&buf[i]) == 0)
+			i += 4;
+
+		if (i == buf_size)
+			ret = 1;
+	}
+
+out:
+	kfree(buf);
+	return ret;
+}
+
+static int mt29f2g01abagd_otp_info(struct spinand_device *spinand, size_t len,
+				   struct otp_info *buf, size_t *retlen)
+{
+	int locked;
+
+	if (len < sizeof(*buf))
+		return -EINVAL;
+
+	locked = mt29f2g01abagd_otp_is_locked(spinand);
+	if (locked < 0)
+		return locked;
+
+	buf->locked = locked;
+	buf->start = 0;
+	buf->length = MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES;
+
+	*retlen = sizeof(*buf);
+	return 0;
+}
+
+static int mt29f2g01abagd_otp_lock(struct spinand_device *spinand, loff_t from,
+				   size_t len)
+{
+	struct spi_mem_op write_op = SPINAND_WR_EN_DIS_OP(true);
+	struct spi_mem_op exec_op = SPINAND_PROG_EXEC_OP(0);
+	int ret;
+
+	ret = spinand_upd_cfg(spinand,
+			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
+			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK);
+	if (!ret)
+		return ret;
+
+	ret = spi_mem_exec_op(spinand->spimem, &write_op);
+	if (!ret)
+		goto out;
+
+	ret = spi_mem_exec_op(spinand->spimem, &exec_op);
+	if (!ret)
+		goto out;
+
+	ret = spinand_wait(spinand, 10, 5, NULL);
+	if (!ret)
+		goto out;
+
+out:
+	if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK, 0)) {
+		WARN(1, "Can not disable OTP mode\n");
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+static const struct spinand_otp_ops mt29f2g01abagd_otp_ops = {
+	.info = mt29f2g01abagd_otp_info,
+	.lock = mt29f2g01abagd_otp_lock,
+	.read = spinand_otp_read,
+	.write = spinand_otp_write,
+};
+
 static const struct spinand_info micron_spinand_table[] = {
 	/* M79A 2Gb 3.3V */
 	SPINAND_INFO("MT29F2G01ABAGD",
@@ -193,7 +306,9 @@ static const struct spinand_info micron_spinand_table[] = {
 					      &x4_update_cache_variants),
 		     0,
 		     SPINAND_ECCINFO(&micron_8_ooblayout,
-				     micron_8_ecc_get_status)),
+				     micron_8_ecc_get_status),
+		     SPINAND_OTP_INFO(MICRON_MT29F2G01ABAGD_OTP_PAGES,
+				      &mt29f2g01abagd_otp_ops)),
 	/* M79A 2Gb 1.8V */
 	SPINAND_INFO("MT29F2G01ABBGD",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),
-- 
2.43.2

Re: [PATCH v2 4/5] mtd: spinand: micron: OTP access for MT29F2G01ABAGD

[Miquel Raynal]

Hi Martin,

mmkurbanov@salutedevices.com wrote on Tue, 27 Aug 2024 20:49:02 +0300:

> Support for OTP area access on Micron MT29F2G01ABAGD chip.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
> ---
>  drivers/mtd/nand/spi/micron.c | 117 +++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
> index 8d741be6d5f3e..a538409db4ccd 100644
> --- a/drivers/mtd/nand/spi/micron.c
> +++ b/drivers/mtd/nand/spi/micron.c
> @@ -9,6 +9,7 @@
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/mtd/spinand.h>
> +#include <linux/spi/spi-mem.h>
>  
>  #define SPINAND_MFR_MICRON		0x2c
>  
> @@ -28,6 +29,16 @@
>  
>  #define MICRON_SELECT_DIE(x)	((x) << 6)
>  
> +#define MICRON_MT29F2G01ABAGD_OTP_PAGES			12
> +#define MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE		2176

In the core we did add the data size and the OOB size to get the OTP
page size. I would prefer something dynamic here as well, otherwise the
implementation is very device specific for now reason?

> +#define MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES		\
> +	(MICRON_MT29F2G01ABAGD_OTP_PAGES *		\
> +	 MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE)

This is a function from the core as well once you've filled all the
information in the core structures, so why hardcoding it?

> +#define MICRON_MT29F2G01ABAGD_CFG_OTP_STATE		BIT(7)
> +#define MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK		\
> +	(CFG_OTP_ENABLE | MICRON_MT29F2G01ABAGD_CFG_OTP_STATE)
> +
>  static SPINAND_OP_VARIANTS(quadio_read_cache_variants,
>  		//SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>  		SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> @@ -182,6 +193,108 @@ static int micron_8_ecc_get_status(struct spinand_device *spinand,
>  	return -EINVAL;
>  }
>  
> +static int mt29f2g01abagd_otp_is_locked(struct spinand_device *spinand)
> +{
> +	size_t buf_size = MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE;
> +	size_t retlen;
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kmalloc(buf_size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = spinand_upd_cfg(spinand,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_STATE);
> +	if (ret)
> +		goto out;

can we name the label free_buf?

> +
> +	ret = spinand_otp_read(spinand, 0, buf_size, buf, &retlen);
> +
> +	if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> +			    0)) {
> +		WARN(1, "Can not disable OTP mode\n");

I prefer dev_warn as well here, so we know which device is concerned.

> +		ret = -EIO;
> +	}
> +
> +	if (!ret) {

if (ret)
	goto out;

> +		size_t i = 0;
> +
> +		/* If all zeros, then the OTP area is locked. */
> +		while (i < buf_size && *(uint32_t *)(&buf[i]) == 0)
> +			i += 4;

Shall we expect buf_size to always be a multiple of 4? (real question)

I am not a big fan of the casting game here. I see the optimization
you're attempting to do, but I'm a little bit skeptical. I must admit I
didn't find a helper for that, buf at least maybe you can use an
intermediate variable and loop over it.

> +
> +		if (i == buf_size)
> +			ret = 1;

If buf_size is not a multiple of 4, this is not gonna work.

> +	}
> +
> +out:
> +	kfree(buf);
> +	return ret;
> +}
> +
> +static int mt29f2g01abagd_otp_info(struct spinand_device *spinand, size_t len,
> +				   struct otp_info *buf, size_t *retlen)
> +{
> +	int locked;
> +
> +	if (len < sizeof(*buf))
> +		return -EINVAL;
> +
> +	locked = mt29f2g01abagd_otp_is_locked(spinand);
> +	if (locked < 0)
> +		return locked;
> +
> +	buf->locked = locked;
> +	buf->start = 0;
> +	buf->length = MICRON_MT29F2G01ABAGD_OTP_SIZE_BYTES;
> +
> +	*retlen = sizeof(*buf);
> +	return 0;
> +}
> +
> +static int mt29f2g01abagd_otp_lock(struct spinand_device *spinand, loff_t from,
> +				   size_t len)
> +{
> +	struct spi_mem_op write_op = SPINAND_WR_EN_DIS_OP(true);
> +	struct spi_mem_op exec_op = SPINAND_PROG_EXEC_OP(0);
> +	int ret;
> +
> +	ret = spinand_upd_cfg(spinand,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK,
> +			      MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK);
> +	if (!ret)
> +		return ret;
> +
> +	ret = spi_mem_exec_op(spinand->spimem, &write_op);
> +	if (!ret)
> +		goto out;
> +
> +	ret = spi_mem_exec_op(spinand->spimem, &exec_op);
> +	if (!ret)
> +		goto out;
> +
> +	ret = spinand_wait(spinand, 10, 5, NULL);

Usually I expect timeouts to be bigger.

> +	if (!ret)
> +		goto out;

This goto seems to have not impact :)

> +
> +out:
> +	if (spinand_upd_cfg(spinand, MICRON_MT29F2G01ABAGD_CFG_OTP_LOCK, 0)) {
> +		WARN(1, "Can not disable OTP mode\n");

dev_warn()

> +		ret = -EIO;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct spinand_otp_ops mt29f2g01abagd_otp_ops = {
> +	.info = mt29f2g01abagd_otp_info,
> +	.lock = mt29f2g01abagd_otp_lock,
> +	.read = spinand_otp_read,
> +	.write = spinand_otp_write,
> +};
> +
>  static const struct spinand_info micron_spinand_table[] = {
>  	/* M79A 2Gb 3.3V */
>  	SPINAND_INFO("MT29F2G01ABAGD",
> @@ -193,7 +306,9 @@ static const struct spinand_info micron_spinand_table[] = {
>  					      &x4_update_cache_variants),
>  		     0,
>  		     SPINAND_ECCINFO(&micron_8_ooblayout,
> -				     micron_8_ecc_get_status)),
> +				     micron_8_ecc_get_status),
> +		     SPINAND_OTP_INFO(MICRON_MT29F2G01ABAGD_OTP_PAGES,
> +				      &mt29f2g01abagd_otp_ops)),
>  	/* M79A 2Gb 1.8V */
>  	SPINAND_INFO("MT29F2G01ABBGD",
>  		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0x25),


Thanks,
Miquèl

Re: [PATCH v2 4/5] mtd: spinand: micron: OTP access for MT29F2G01ABAGD

[Martin Kurbanov]

Hi Miquel,

On 10/1/24 12:31, Miquel Raynal wrote:

>> +#define MICRON_MT29F2G01ABAGD_OTP_PAGES			12
>> +#define MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE		2176
> 
> In the core we did add the data size and the OOB size to get the OTP
> page size. I would prefer something dynamic here as well, otherwise the
> implementation is very device specific for now reason?

Do you mean:
otp_page_size = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand)

>> +		size_t i = 0;
>> +
>> +		/* If all zeros, then the OTP area is locked. */
>> +		while (i < buf_size && *(uint32_t *)(&buf[i]) == 0)
>> +			i += 4;
> 
> Shall we expect buf_size to always be a multiple of 4? (real question)

This function is only for the nand flash MT29F2G01ABAGD that has a page
size multiple of 4.

-- 
Best Regards,
Martin Kurbanov

Re: [PATCH v2 4/5] mtd: spinand: micron: OTP access for MT29F2G01ABAGD

[Miquel Raynal]

Hi Martin,

mmkurbanov@salutedevices.com wrote on Mon, 14 Oct 2024 15:48:46 +0300:

> Hi Miquel,
> 
> On 10/1/24 12:31, Miquel Raynal wrote:
> 
> >> +#define MICRON_MT29F2G01ABAGD_OTP_PAGES			12
> >> +#define MICRON_MT29F2G01ABAGD_OTP_PAGE_SIZE		2176  
> > 
> > In the core we did add the data size and the OOB size to get the OTP
> > page size. I would prefer something dynamic here as well, otherwise the
> > implementation is very device specific for now reason?  

						no

> 
> Do you mean:
> otp_page_size = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand)

Yes!

> >> +		size_t i = 0;
> >> +
> >> +		/* If all zeros, then the OTP area is locked. */
> >> +		while (i < buf_size && *(uint32_t *)(&buf[i]) == 0)
> >> +			i += 4;  
> > 
> > Shall we expect buf_size to always be a multiple of 4? (real question)  
> 
> This function is only for the nand flash MT29F2G01ABAGD that has a page
> size multiple of 4.

Ok.

Thanks,
Miquèl

[PATCH v2 5/5] mtd: spinand: esmt: OTP access for F50{L,D}1G41LB

[Martin Kurbanov]

Support for OTP area access on ESMT F50L1G41LB and F50D1G41LB chips.

Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>
---
 drivers/mtd/nand/spi/esmt.c | 69 +++++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/spi/esmt.c b/drivers/mtd/nand/spi/esmt.c
index 4597a82de23a4..1806e9d48c176 100644
--- a/drivers/mtd/nand/spi/esmt.c
+++ b/drivers/mtd/nand/spi/esmt.c
@@ -12,6 +12,13 @@
 /* ESMT uses GigaDevice 0xc8 JECDEC ID on some SPI NANDs */
 #define SPINAND_MFR_ESMT_C8			0xc8
 
+#define ESMT_F50L1G41LB_CFG_OTP_PROTECT		BIT(7)
+#define ESMT_F50L1G41LB_CFG_OTP_LOCK		\
+	(CFG_OTP_ENABLE | ESMT_F50L1G41LB_CFG_OTP_PROTECT)
+
+#define ESMT_F50L1G41LB_PAGE_SIZE		2112
+#define ESMT_F50L1G41LB_OTP_PAGES		28
+
 static SPINAND_OP_VARIANTS(read_cache_variants,
 			   SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
 			   SPINAND_PAGE_READ_FROM_CACHE_X2_OP(0, 1, NULL, 0),
@@ -102,6 +109,60 @@ static const struct mtd_ooblayout_ops f50l1g41lb_ooblayout = {
 	.free = f50l1g41lb_ooblayout_free,
 };
 
+static int f50l1g41lb_otp_info(struct spinand_device *spinand, size_t len,
+			       struct otp_info *buf, size_t *retlen)
+{
+	if (len < sizeof(*buf))
+		return -EINVAL;
+
+	buf->locked = 0;
+	buf->start = 0;
+	buf->length = ESMT_F50L1G41LB_PAGE_SIZE * ESMT_F50L1G41LB_OTP_PAGES;
+
+	*retlen = sizeof(*buf);
+	return 0;
+}
+
+static int f50l1g41lb_otp_lock(struct spinand_device *spinand, loff_t from,
+			       size_t len)
+{
+	struct spi_mem_op write_op = SPINAND_WR_EN_DIS_OP(true);
+	struct spi_mem_op exec_op = SPINAND_PROG_EXEC_OP(0);
+	int ret;
+
+	ret = spinand_upd_cfg(spinand, ESMT_F50L1G41LB_CFG_OTP_LOCK,
+			      ESMT_F50L1G41LB_CFG_OTP_LOCK);
+	if (!ret)
+		return ret;
+
+	ret = spi_mem_exec_op(spinand->spimem, &write_op);
+	if (!ret)
+		goto out;
+
+	ret = spi_mem_exec_op(spinand->spimem, &exec_op);
+	if (!ret)
+		goto out;
+
+	ret = spinand_wait(spinand, 10, 5, NULL);
+	if (!ret)
+		goto out;
+
+out:
+	if (spinand_upd_cfg(spinand, ESMT_F50L1G41LB_CFG_OTP_LOCK, 0)) {
+		WARN(1, "Can not disable OTP mode\n");
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
+static const struct spinand_otp_ops f50l1g41lb_otp_ops = {
+	.info = f50l1g41lb_otp_info,
+	.lock = f50l1g41lb_otp_lock,
+	.read = spinand_otp_read,
+	.write = spinand_otp_write,
+};
+
 static const struct spinand_info esmt_c8_spinand_table[] = {
 	SPINAND_INFO("F50L1G41LB",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x01, 0x7f,
@@ -112,7 +173,9 @@ static const struct spinand_info esmt_c8_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&f50l1g41lb_ooblayout, NULL)),
+		     SPINAND_ECCINFO(&f50l1g41lb_ooblayout, NULL),
+		     SPINAND_OTP_INFO(ESMT_F50L1G41LB_OTP_PAGES,
+				      &f50l1g41lb_otp_ops)),
 	SPINAND_INFO("F50D1G41LB",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x11, 0x7f,
 				0x7f, 0x7f),
@@ -122,7 +185,9 @@ static const struct spinand_info esmt_c8_spinand_table[] = {
 					      &write_cache_variants,
 					      &update_cache_variants),
 		     0,
-		     SPINAND_ECCINFO(&f50l1g41lb_ooblayout, NULL)),
+		     SPINAND_ECCINFO(&f50l1g41lb_ooblayout, NULL),
+		     SPINAND_OTP_INFO(ESMT_F50L1G41LB_OTP_PAGES,
+				      &f50l1g41lb_otp_ops)),
 	SPINAND_INFO("F50D2G41KA",
 		     SPINAND_ID(SPINAND_READID_METHOD_OPCODE_ADDR, 0x51, 0x7f,
 				0x7f, 0x7f),
-- 
2.43.2

Re: [PATCH v2 5/5] mtd: spinand: esmt: OTP access for F50{L,D}1G41LB

[Miquel Raynal]

Hi Martin,

mmkurbanov@salutedevices.com wrote on Tue, 27 Aug 2024 20:49:03 +0300:

> Support for OTP area access on ESMT F50L1G41LB and F50D1G41LB chips.
> 
> Signed-off-by: Martin Kurbanov <mmkurbanov@salutedevices.com>

All my comments in 4/5 apply here as well.

Thanks,
Miquèl