[PATCH v4 0/4] mtd: spi-nor: OTP support

[PATCH v4 0/4] mtd: spi-nor: OTP support

[Michael Walle]

This patchset implements the MTD OTP functions to allow access to the SPI
OTP data. Specific support is added for Winbond flash chips.

In the past there was already an attempt by Rahul Bedarkar to add this, but
there was no response. These patches are slightly based on his work.

https://lore.kernel.org/linux-mtd/1489754636-21461-1-git-send-email-rahul.bedarkar@imgtec.com/

Changes since v3:
 - remapped the OTP regions to a contiguous area starting at 0. The
   chips/cfi_cmdset_000[12].c remap the regions, too.
 - with that in place, read/write/lock/erase spanning multiple OTP
   regions are possible
 - picked up Tudors review remarks
 - added new erase support as RFC because MTD API/ABI is still missing.
   Feel free to review, but don't apply it.

Changes since v2:
 - improved commit messages
 - add buffer size check in spi_nor_mtd_otp_info(). just to be sure, the
   buffer is hardcoded to 4k by the mtd subsys
 - moved all code to otp.c
 - dropped the patches introduced in v2

Changes since v1:
 - added methods for Macronix and similar flashes
 - added patch to cleanup/consolidate code in core.c

Michael Walle (4):
  mtd: spi-nor: add OTP support
  mtd: spi-nor: implement OTP support for Winbond and similar flashes
  mtd: spi-nor: winbond: add OTP support to w25q32fw/jw
  mtd: spi-nor: implement OTP erase for Winbond and similar flashes

 drivers/mtd/spi-nor/Makefile  |   1 +
 drivers/mtd/spi-nor/core.c    |  11 +-
 drivers/mtd/spi-nor/core.h    |  64 +++++
 drivers/mtd/spi-nor/otp.c     | 453 ++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/winbond.c |  18 +-
 include/linux/mtd/spi-nor.h   |   9 +
 6 files changed, 551 insertions(+), 5 deletions(-)
 create mode 100644 drivers/mtd/spi-nor/otp.c

-- 
2.20.1

[PATCH v4 1/4] mtd: spi-nor: add OTP support

[Michael Walle]

SPI flashes sometimes have a special OTP area, which can (and is) used to
store immutable properties like board serial number or vendor assigned
network hardware addresses.

The MTD subsystem already supports accessing such areas and some (non
SPI NOR) flashes already implement support for it. It differentiates
between user and factory areas. User areas can be written by the user and
factory ones are pre-programmed and locked down by the vendor, usually
containing an "electrical serial number". This patch will only add support
for the user areas.

Lay the foundation and implement the MTD callbacks for the SPI NOR and add
necessary parameters to the flash_info structure. If a flash supports OTP
it can be added by the convenience macro OTP_INFO(). Sometimes there are
individual regions, which might have individual offsets. Therefore, it is
possible to specify the starting address of the first regions as well as
the distance between two regions (e.g. Winbond devices uses this method).

Additionally, the regions might be locked down. Once locked, no further
write access is possible.

For SPI NOR flashes the OTP area is accessed like the normal memory, e.g.
by offset addressing; except that you either have to use special read/write
commands (Winbond) or you have to enter (and exit) a specific OTP mode
(Macronix, Micron).

Thus we introduce four operations to which the MTD callbacks will be
mapped: .read(), .write(), .lock() and .is_locked(). The read and the write
ops will be given an address offset to operate on while the locking ops use
regions because locking always affects a whole region. It is up to the
flash driver to implement these ops.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/Makefile |   1 +
 drivers/mtd/spi-nor/core.c   |   5 +
 drivers/mtd/spi-nor/core.h   |  54 +++++++++
 drivers/mtd/spi-nor/otp.c    | 218 +++++++++++++++++++++++++++++++++++
 4 files changed, 278 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/otp.c

diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 653923896205..2ed2e76ce4f1 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -12,6 +12,7 @@ spi-nor-objs			+= intel.o
 spi-nor-objs			+= issi.o
 spi-nor-objs			+= macronix.o
 spi-nor-objs			+= micron-st.o
+spi-nor-objs			+= otp.o
 spi-nor-objs			+= spansion.o
 spi-nor-objs			+= sst.o
 spi-nor-objs			+= winbond.o
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 4a315cb1c4db..0c5c757fa95b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3009,6 +3009,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
 	spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
 			       SPINOR_OP_SE);
 	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
+
+	nor->params->otp.org = &info->otp_org;
 }
 
 /**
@@ -3550,6 +3552,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (ret)
 		return ret;
 
+	/* Configure OTP parameters and ops */
+	spi_nor_otp_init(nor);
+
 	/* Send all the required SPI flash commands to initialize device */
 	ret = spi_nor_init(nor);
 	if (ret)
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 4a3f7f150b5d..ec8da1243846 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -187,6 +187,45 @@ struct spi_nor_locking_ops {
 	int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 };
 
+/**
+ * struct spi_nor_otp_organization - Structure to describe the SPI NOR OTP regions
+ * @len:	size of one OTP region in bytes.
+ * @base:	start address of the OTP area.
+ * @offset:	offset between consecutive OTP regions if there are more
+ *              than one.
+ * @n_regions:	number of individual OTP regions.
+ */
+struct spi_nor_otp_organization {
+	size_t len;
+	loff_t base;
+	loff_t offset;
+	unsigned int n_regions;
+};
+
+/**
+ * struct spi_nor_otp_ops - SPI NOR OTP methods
+ * @read:	read from the SPI NOR OTP area.
+ * @write:	write to the SPI NOR OTP area.
+ * @lock:	lock an OTP region.
+ * @is_locked:	check if an OTP region of the SPI NOR is locked.
+ */
+struct spi_nor_otp_ops {
+	int (*read)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
+	int (*write)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
+	int (*lock)(struct spi_nor *nor, unsigned int region);
+	int (*is_locked)(struct spi_nor *nor, unsigned int region);
+};
+
+/**
+ * struct spi_nor_otp - SPI NOR OTP grouping structure
+ * @org:	OTP region organization
+ * @ops:	OTP access ops
+ */
+struct spi_nor_otp {
+	const struct spi_nor_otp_organization *org;
+	const struct spi_nor_otp_ops *ops;
+};
+
 /**
  * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings.
  * Includes legacy flash parameters and settings that can be overwritten
@@ -208,6 +247,7 @@ struct spi_nor_locking_ops {
  *                      higher index in the array, the higher priority.
  * @erase_map:		the erase map parsed from the SFDP Sector Map Parameter
  *                      Table.
+ * @otp_info:		describes the OTP regions.
  * @octal_dtr_enable:	enables SPI NOR octal DTR mode.
  * @quad_enable:	enables SPI NOR quad mode.
  * @set_4byte_addr_mode: puts the SPI NOR in 4 byte addressing mode.
@@ -219,6 +259,7 @@ struct spi_nor_locking_ops {
  *                      e.g. different opcodes, specific address calculation,
  *                      page size, etc.
  * @locking_ops:	SPI NOR locking methods.
+ * @otp:		SPI NOR OTP methods.
  */
 struct spi_nor_flash_parameter {
 	u64				size;
@@ -232,6 +273,7 @@ struct spi_nor_flash_parameter {
 	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
 
 	struct spi_nor_erase_map        erase_map;
+	struct spi_nor_otp		otp;
 
 	int (*octal_dtr_enable)(struct spi_nor *nor, bool enable);
 	int (*quad_enable)(struct spi_nor *nor);
@@ -341,6 +383,8 @@ struct flash_info {
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
+
+	const struct spi_nor_otp_organization otp_org;
 };
 
 /* Used when the "_ext_id" is two bytes at most */
@@ -393,6 +437,14 @@ struct flash_info {
 		.addr_width = 3,					\
 		.flags = SPI_NOR_NO_FR | SPI_NOR_XSR_RDY,
 
+#define OTP_INFO(_len, _n_regions, _base, _offset)			\
+		.otp_org = {						\
+			.len = (_len),					\
+			.base = (_base),				\
+			.offset = (_offset),				\
+			.n_regions = (_n_regions),			\
+		},
+
 /**
  * struct spi_nor_manufacturer - SPI NOR manufacturer object
  * @name: manufacturer name
@@ -473,6 +525,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
 			     const struct sfdp_bfpt *bfpt,
 			     struct spi_nor_flash_parameter *params);
 
+void spi_nor_otp_init(struct spi_nor *nor);
+
 static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
 {
 	return mtd->priv;
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
new file mode 100644
index 000000000000..4e301fd5156b
--- /dev/null
+++ b/drivers/mtd/spi-nor/otp.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * OTP support for SPI NOR flashes
+ *
+ * Copyright (C) 2021 Michael Walle <michael@walle.cc>
+ */
+
+#include <linux/log2.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spi-nor.h>
+
+#include "core.h"
+
+#define spi_nor_otp_ops(nor) ((nor)->params->otp.ops)
+#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
+#define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
+
+static loff_t spi_nor_otp_region_start(const struct spi_nor *nor, int region)
+{
+	const struct spi_nor_otp_organization *org = nor->params->otp.org;
+
+	return org->base + region * org->offset;
+}
+
+static size_t spi_nor_otp_size(struct spi_nor *nor)
+{
+	return spi_nor_otp_n_regions(nor) * spi_nor_otp_region_len(nor);
+}
+
+/*
+ * Translate the file offsets from and to OTP regions. See also
+ * spi_nor_mtd_otp_do_op().
+ */
+static loff_t spi_nor_otp_region_to_offset(struct spi_nor *nor, unsigned int region)
+{
+	return region * spi_nor_otp_region_len(nor);
+}
+
+static unsigned int spi_nor_otp_offset_to_region(struct spi_nor *nor, loff_t ofs)
+{
+	return ofs / spi_nor_otp_region_len(nor);
+}
+
+static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
+				size_t *retlen, struct otp_info *buf)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
+	unsigned int n_regions = spi_nor_otp_n_regions(nor);
+	unsigned int region;
+	int ret, locked;
+
+	if (len < n_regions * sizeof(*buf))
+		return -ENOSPC;
+
+	ret = spi_nor_lock_and_prep(nor);
+	if (ret)
+		return ret;
+
+	for (region = 0; region < spi_nor_otp_n_regions(nor); region++) {
+		buf->start = spi_nor_otp_region_to_offset(nor, region);
+		buf->length = spi_nor_otp_region_len(nor);
+
+		locked = ops->is_locked(nor, region);
+		if (locked < 0) {
+			ret = locked;
+			goto out;
+		}
+
+		buf->locked = !!locked;
+		buf++;
+	}
+
+	*retlen = n_regions * sizeof(*buf);
+
+out:
+	spi_nor_unlock_and_unprep(nor);
+
+	return ret;
+}
+
+static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
+				      size_t total_len, size_t *retlen,
+				      u_char *buf, bool is_write)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
+	const size_t rlen = spi_nor_otp_region_len(nor);
+	loff_t rstart, rofs;
+	unsigned int region;
+	size_t len;
+	int ret;
+
+	if (ofs < 0 || ofs >= spi_nor_otp_size(nor))
+		return 0;
+
+	ret = spi_nor_lock_and_prep(nor);
+	if (ret)
+		return ret;
+
+	/* don't access beyond the end */
+	total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
+
+	*retlen = 0;
+	while (total_len) {
+		/*
+		 * The OTP regions are mapped into a contiguous area starting
+		 * at 0 as expected by the MTD layer. This will map the MTD
+		 * file offsets to the address of an OTP region as used in the
+		 * actual SPI commands.
+		 */
+		region = spi_nor_otp_offset_to_region(nor, ofs);
+		rstart = spi_nor_otp_region_start(nor, region);
+
+		/*
+		 * The size of a OTP region is expected to be a power of two,
+		 * thus we can just mask the lower bits and get the offset into
+		 * a region.
+		 */
+		rofs = ofs & (rlen - 1);
+
+		/* don't access beyond one OTP region */
+		len = min_t(size_t, total_len, rlen - rofs);
+
+		if (is_write)
+			ret = ops->write(nor, rstart + rofs, len, buf);
+		else
+			ret = ops->read(nor, rstart + rofs, len, buf);
+		if (ret == 0)
+			ret = -EIO;
+		if (ret < 0)
+			goto out;
+
+		*retlen += ret;
+		ofs += ret;
+		buf += ret;
+		total_len -= ret;
+	}
+	ret = 0;
+
+out:
+	spi_nor_unlock_and_unprep(nor);
+	return ret;
+}
+
+static int spi_nor_mtd_otp_read(struct mtd_info *mtd, loff_t from, size_t len,
+				size_t *retlen, u_char *buf)
+{
+	return spi_nor_mtd_otp_read_write(mtd, from, len, retlen, buf, false);
+}
+
+static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
+				 size_t *retlen, u_char *buf)
+{
+	return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
+}
+
+static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
+	unsigned int region;
+	int ret;
+
+	if (from < 0 || (from + len) > spi_nor_otp_size(nor))
+		return -EINVAL;
+
+	/* the user has to explicitly ask for whole regions */
+	if (len % spi_nor_otp_region_len(nor))
+		return -EINVAL;
+
+	if (from % spi_nor_otp_region_len(nor))
+		return -EINVAL;
+
+	ret = spi_nor_lock_and_prep(nor);
+	if (ret)
+		return ret;
+
+	while (len) {
+		region = spi_nor_otp_offset_to_region(nor, from);
+		ret = ops->lock(nor, region);
+		if (ret)
+			goto out;
+
+		len -= spi_nor_otp_region_len(nor);
+		from += spi_nor_otp_region_len(nor);
+	}
+
+out:
+	spi_nor_unlock_and_unprep(nor);
+
+	return ret;
+}
+
+void spi_nor_otp_init(struct spi_nor *nor)
+{
+	struct mtd_info *mtd = &nor->mtd;
+
+	if (!nor->params->otp.ops)
+		return;
+
+	if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor))))
+		return;
+
+	/*
+	 * We only support user_prot callbacks (yet).
+	 *
+	 * Some SPI NOR flashes like Macronix ones can be ordered in two
+	 * different variants. One with a factory locked OTP area and one where
+	 * it is left to the user to write to it. The factory locked OTP is
+	 * usually preprogrammed with an "electrical serial number". We don't
+	 * support these for now.
+	 */
+	mtd->_get_user_prot_info = spi_nor_mtd_otp_info;
+	mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
+	mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
+	mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
+}
-- 
2.20.1

[PATCH v4 2/4] mtd: spi-nor: implement OTP support for Winbond and similar flashes

[Michael Walle]

Use the new OTP ops to implement OTP access on Winbond flashes. Most
Winbond flashes provides up to four different OTP regions ("Security
Registers").

Winbond devices use a special opcode to read and write to the OTP
regions, just like the RDSFDP opcode. In fact, it seems that the
(undocumented) first OTP area of the newer flashes is the actual SFDP
table.

On a side note, Winbond devices also allow erasing the OTP regions as
long as the area isn't locked down.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c  |   2 +-
 drivers/mtd/spi-nor/core.h  |   6 ++
 drivers/mtd/spi-nor/otp.c   | 164 ++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h |   9 ++
 4 files changed, 180 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0c5c757fa95b..ef7df26896f1 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1034,7 +1034,7 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
  *
  * Return: 0 on success, -errno otherwise.
  */
-static int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
+int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
 {
 	int ret;
 	u8 *sr_cr = nor->bouncebuf;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index ec8da1243846..dfbf6ba42b57 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -496,6 +496,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
 int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
 int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
 int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
+int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);
 
 int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
 ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
@@ -503,6 +504,11 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 			   const u8 *buf);
 
+int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
+int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
+int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
+int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
+
 int spi_nor_hwcaps_read2cmd(u32 hwcaps);
 u8 spi_nor_convert_3to4_read(u8 opcode);
 void spi_nor_set_read_settings(struct spi_nor_read_command *read,
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 4e301fd5156b..4e8da9108c77 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -15,6 +15,170 @@
 #define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
 #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
 
+/**
+ * spi_nor_otp_read_secr() - read OTP data
+ * @nor:	pointer to 'struct spi_nor'
+ * @from:       offset to read from
+ * @len:        number of bytes to read
+ * @buf:        pointer to dst buffer
+ *
+ * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
+ * method is used on GigaDevice and Winbond flashes.
+ *
+ * Return: number of bytes read successfully, -errno otherwise
+ */
+int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
+{
+	u8 addr_width, read_opcode, read_dummy;
+	struct spi_mem_dirmap_desc *rdesc;
+	enum spi_nor_protocol read_proto;
+	int ret;
+
+	read_opcode = nor->read_opcode;
+	addr_width = nor->addr_width;
+	read_dummy = nor->read_dummy;
+	read_proto = nor->read_proto;
+	rdesc = nor->dirmap.rdesc;
+
+	nor->read_opcode = SPINOR_OP_RSECR;
+	nor->addr_width = 3;
+	nor->read_dummy = 8;
+	nor->read_proto = SNOR_PROTO_1_1_1;
+	nor->dirmap.rdesc = NULL;
+
+	ret = spi_nor_read_data(nor, addr, len, buf);
+
+	nor->read_opcode = read_opcode;
+	nor->addr_width = addr_width;
+	nor->read_dummy = read_dummy;
+	nor->read_proto = read_proto;
+	nor->dirmap.rdesc = rdesc;
+
+	return ret;
+}
+
+/**
+ * spi_nor_otp_write_secr() - write OTP data
+ * @nor:        pointer to 'struct spi_nor'
+ * @to:         offset to write to
+ * @len:        number of bytes to write
+ * @buf:        pointer to src buffer
+ *
+ * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
+ * method is used on GigaDevice and Winbond flashes.
+ *
+ * Please note, the write must not span multiple OTP regions.
+ *
+ * Return: number of bytes written successfully, -errno otherwise
+ */
+int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
+{
+	enum spi_nor_protocol write_proto;
+	struct spi_mem_dirmap_desc *wdesc;
+	u8 addr_width, program_opcode;
+	int ret, written;
+
+	program_opcode = nor->program_opcode;
+	addr_width = nor->addr_width;
+	write_proto = nor->write_proto;
+	wdesc = nor->dirmap.wdesc;
+
+	nor->program_opcode = SPINOR_OP_PSECR;
+	nor->addr_width = 3;
+	nor->write_proto = SNOR_PROTO_1_1_1;
+	nor->dirmap.wdesc = NULL;
+
+	/*
+	 * We only support a write to one single page. For now all winbond
+	 * flashes only have one page per OTP region.
+	 */
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		goto out;
+
+	written = spi_nor_write_data(nor, addr, len, buf);
+	if (written < 0)
+		goto out;
+
+	ret = spi_nor_wait_till_ready(nor);
+
+out:
+	nor->program_opcode = program_opcode;
+	nor->addr_width = addr_width;
+	nor->write_proto = write_proto;
+	nor->dirmap.wdesc = wdesc;
+
+	return ret ?: written;
+}
+
+static int spi_nor_otp_lock_bit_cr(unsigned int region)
+{
+	static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
+
+	if (region >= ARRAY_SIZE(lock_bits))
+		return -EINVAL;
+
+	return lock_bits[region];
+}
+
+/**
+ * spi_nor_otp_lock_sr2() - lock the OTP region
+ * @nor:        pointer to 'struct spi_nor'
+ * @region:     OTP region
+ *
+ * Lock the OTP region by writing the status register-2. This method is used on
+ * GigaDevice and Winbond flashes.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region)
+{
+	u8 *cr = nor->bouncebuf;
+	int ret, lock_bit;
+
+	lock_bit = spi_nor_otp_lock_bit_cr(region);
+	if (lock_bit < 0)
+		return lock_bit;
+
+	ret = spi_nor_read_cr(nor, cr);
+	if (ret)
+		return ret;
+
+	/* no need to write the register if region is already locked */
+	if (cr[0] & lock_bit)
+		return 0;
+
+	cr[0] |= lock_bit;
+
+	return spi_nor_write_16bit_cr_and_check(nor, cr[0]);
+}
+
+/**
+ * spi_nor_otp_is_locked_sr2() - get the OTP region lock status
+ * @nor:        pointer to 'struct spi_nor'
+ * @region:     OTP region
+ *
+ * Retrieve the OTP region lock bit by reading the status register-2. This
+ * method is used on GigaDevice and Winbond flashes.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region)
+{
+	u8 *cr = nor->bouncebuf;
+	int ret, lock_bit;
+
+	lock_bit = spi_nor_otp_lock_bit_cr(region);
+	if (lock_bit < 0)
+		return lock_bit;
+
+	ret = spi_nor_read_cr(nor, cr);
+	if (ret)
+		return ret;
+
+	return cr[0] & lock_bit;
+}
+
 static loff_t spi_nor_otp_region_start(const struct spi_nor *nor, int region)
 {
 	const struct spi_nor_otp_organization *org = nor->params->otp.org;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index a0d572855444..6d1956049e90 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -107,6 +107,11 @@
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
 #define SPINOR_OP_WD_EVCR      0x61    /* Write EVCR register */
 
+/* Used for GigaDevices and Winbond flashes. */
+#define SPINOR_OP_ESECR		0x44	/* Erase Security registers */
+#define SPINOR_OP_PSECR		0x42	/* Program Security registers */
+#define SPINOR_OP_RSECR		0x48	/* Read Security registers */
+
 /* Status Register bits. */
 #define SR_WIP			BIT(0)	/* Write in progress */
 #define SR_WEL			BIT(1)	/* Write enable latch */
@@ -138,8 +143,12 @@
 
 /* Status Register 2 bits. */
 #define SR2_QUAD_EN_BIT1	BIT(1)
+#define SR2_LB1			BIT(3)	/* Security Register Lock Bit 1 */
+#define SR2_LB2			BIT(4)	/* Security Register Lock Bit 2 */
+#define SR2_LB3			BIT(5)	/* Security Register Lock Bit 3 */
 #define SR2_QUAD_EN_BIT7	BIT(7)
 
+
 /* Supported SPI protocols */
 #define SNOR_PROTO_INST_MASK	GENMASK(23, 16)
 #define SNOR_PROTO_INST_SHIFT	16
-- 
2.20.1

[PATCH v4 3/4] mtd: spi-nor: winbond: add OTP support to w25q32fw/jw

[Michael Walle]

With all the helper functions in place, add OTP support for the Winbond
W25Q32JW and W25Q32FW.

Both were tested on a LS1028A SoC with a NXP FSPI controller.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/winbond.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index e5dfa786f190..9a3f8ff007fd 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -55,14 +55,18 @@ static const struct flash_info winbond_parts[] = {
 	{ "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
 	{ "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64,
 			   SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-			   SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
+			   SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+			   OTP_INFO(256, 3, 0x1000, 0x1000)
+	},
+
 	{ "w25q32jv", INFO(0xef7016, 0, 64 * 1024,  64,
 			   SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			   SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
 	},
 	{ "w25q32jwm", INFO(0xef8016, 0, 64 * 1024,  64,
 			    SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-			    SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
+			    SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
+			    OTP_INFO(256, 3, 0x1000, 0x1000) },
 	{ "w25q64jwm", INFO(0xef8017, 0, 64 * 1024, 128,
 			    SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			    SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
@@ -131,9 +135,18 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 	return spi_nor_write_disable(nor);
 }
 
+static const struct spi_nor_otp_ops winbond_otp_ops = {
+	.read = spi_nor_otp_read_secr,
+	.write = spi_nor_otp_write_secr,
+	.lock = spi_nor_otp_lock_sr2,
+	.is_locked = spi_nor_otp_is_locked_sr2,
+};
+
 static void winbond_default_init(struct spi_nor *nor)
 {
 	nor->params->set_4byte_addr_mode = winbond_set_4byte_addr_mode;
+	if (nor->params->otp.org->n_regions)
+		nor->params->otp.ops = &winbond_otp_ops;
 }
 
 static const struct spi_nor_fixups winbond_fixups = {
-- 
2.20.1

[PATCH v4 4/4] mtd: spi-nor: implement OTP erase for Winbond and similar flashes

[Michael Walle]

Winbond flashes with OTP support provide a command to erase the OTP
data. This might come in handy during development.

This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
NXP FSPI controller.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c    |  4 +-
 drivers/mtd/spi-nor/core.h    |  4 ++
 drivers/mtd/spi-nor/otp.c     | 74 ++++++++++++++++++++++++++++++++++-
 drivers/mtd/spi-nor/winbond.c |  1 +
 4 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index ef7df26896f1..21a737804576 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -166,8 +166,8 @@ static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
 	return nor->controller_ops->read_reg(nor, opcode, buf, len);
 }
 
-static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
-					    const u8 *buf, size_t len)
+int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
+				     const u8 *buf, size_t len)
 {
 	if (spi_nor_protocol_is_dtr(nor->reg_proto))
 		return -EOPNOTSUPP;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index dfbf6ba42b57..ef62ec4625a1 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -213,6 +213,7 @@ struct spi_nor_otp_ops {
 	int (*read)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
 	int (*write)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
 	int (*lock)(struct spi_nor *nor, unsigned int region);
+	int (*erase)(struct spi_nor *nor, loff_t addr);
 	int (*is_locked)(struct spi_nor *nor, unsigned int region);
 };
 
@@ -481,6 +482,8 @@ extern const struct spi_nor_manufacturer spi_nor_xmc;
 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_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
+				     const u8 *buf, 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(struct spi_nor *nor, bool enable);
@@ -506,6 +509,7 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 
 int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
 int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
 int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
 int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
 
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 4e8da9108c77..78ec79368c29 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -8,6 +8,7 @@
 #include <linux/log2.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/spi-nor.h>
+#include <linux/spi/spi-mem.h>
 
 #include "core.h"
 
@@ -111,6 +112,50 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf
 	return ret ?: written;
 }
 
+/**
+ * spi_nor_otp_erase_secr() - erase one OTP region
+ * @nor:        pointer to 'struct spi_nor'
+ * @to:         offset to write to
+ * @len:        number of bytes to write
+ * @buf:        pointer to src buffer
+ *
+ * Erase one OTP region by using the SPINOR_OP_ESECR commands. This method is
+ * used on GigaDevice and Winbond flashes.
+ *
+ * Return: 0 on success, -errno otherwise
+ */
+int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
+{
+	int ret;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_ESECR, 0),
+				   SPI_MEM_OP_ADDR(3, addr, 0),
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_NO_DATA);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		nor->bouncebuf[2] = addr & 0xff;
+		nor->bouncebuf[1] = (addr >> 8) & 0xff;
+		nor->bouncebuf[0] = (addr >> 16) & 0xff;
+
+		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_ESECR,
+						       nor->bouncebuf, 3);
+	}
+	if (ret)
+		return ret;
+
+	return spi_nor_wait_till_ready(nor);
+}
+
 static int spi_nor_otp_lock_bit_cr(unsigned int region)
 {
 	static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
@@ -319,11 +364,13 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
 	return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
 }
 
-static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
+static int spi_nor_mtd_otp_lock_erase(struct mtd_info *mtd, loff_t from,
+				      size_t len, bool is_erase)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
 	const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
 	unsigned int region;
+	loff_t start;
 	int ret;
 
 	if (from < 0 || (from + len) > spi_nor_otp_size(nor))
@@ -342,7 +389,12 @@ static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
 
 	while (len) {
 		region = spi_nor_otp_offset_to_region(nor, from);
-		ret = ops->lock(nor, region);
+		start = spi_nor_otp_region_start(nor, region);
+
+		if (is_erase)
+			ret = ops->erase(nor, start);
+		else
+			ret = ops->lock(nor, region);
 		if (ret)
 			goto out;
 
@@ -356,6 +408,23 @@ static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
 	return ret;
 }
 
+static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
+{
+	return spi_nor_mtd_otp_lock_erase(mtd, from, len, false);
+}
+
+static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
+
+	/* OTP erase is optional */
+	if (!ops->erase)
+		return -EOPNOTSUPP;
+
+	return spi_nor_mtd_otp_lock_erase(mtd, from, len, true);
+}
+
 void spi_nor_otp_init(struct spi_nor *nor)
 {
 	struct mtd_info *mtd = &nor->mtd;
@@ -379,4 +448,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
 	mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
 	mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
 	mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
+	mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
 }
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 9a3f8ff007fd..e04219ac11fd 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -138,6 +138,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 static const struct spi_nor_otp_ops winbond_otp_ops = {
 	.read = spi_nor_otp_read_secr,
 	.write = spi_nor_otp_write_secr,
+	.erase = spi_nor_otp_erase_secr,
 	.lock = spi_nor_otp_lock_sr2,
 	.is_locked = spi_nor_otp_is_locked_sr2,
 };
-- 
2.20.1

Re: [PATCH v4 4/4] mtd: spi-nor: implement OTP erase for Winbond and similar flashes

[Michael Walle]

Am 2021-03-06 01:05, schrieb Michael Walle:
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

This should have had [RFC PATCH] in the subject. That got missing.

-michael

Re: [PATCH v4 4/4] mtd: spi-nor: implement OTP erase for Winbond and similar flashes

[kernel test robot]

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

Hi Michael,

I love your patch! Yet something to improve:

[auto build test ERROR on mtd/spi-nor/next]
[also build test ERROR on linux/master linus/master v5.12-rc2 next-20210305]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/mtd-spi-nor-OTP-support/20210307-110709
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/61574179875574d957f00e40fa3e9fe9671c0f6e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Walle/mtd-spi-nor-OTP-support/20210307-110709
        git checkout 61574179875574d957f00e40fa3e9fe9671c0f6e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/mtd/spi-nor/otp.c: In function 'spi_nor_otp_init':
>> drivers/mtd/spi-nor/otp.c:451:7: error: 'struct mtd_info' has no member named '_erase_user_prot_reg'; did you mean '_read_user_prot_reg'?
     451 |  mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
         |       ^~~~~~~~~~~~~~~~~~~~
         |       _read_user_prot_reg


vim +451 drivers/mtd/spi-nor/otp.c

   427	
   428	void spi_nor_otp_init(struct spi_nor *nor)
   429	{
   430		struct mtd_info *mtd = &nor->mtd;
   431	
   432		if (!nor->params->otp.ops)
   433			return;
   434	
   435		if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor))))
   436			return;
   437	
   438		/*
   439		 * We only support user_prot callbacks (yet).
   440		 *
   441		 * Some SPI NOR flashes like Macronix ones can be ordered in two
   442		 * different variants. One with a factory locked OTP area and one where
   443		 * it is left to the user to write to it. The factory locked OTP is
   444		 * usually preprogrammed with an "electrical serial number". We don't
   445		 * support these for now.
   446		 */
   447		mtd->_get_user_prot_info = spi_nor_mtd_otp_info;
   448		mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
   449		mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
   450		mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
 > 451		mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68891 bytes --]

Re: [PATCH v4 1/4] mtd: spi-nor: add OTP support

[Tudor.Ambarus]

Michael,

Just cosmetic suggestions this time.

On 3/6/21 2:05 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> SPI flashes sometimes have a special OTP area, which can (and is) used to
> store immutable properties like board serial number or vendor assigned
> network hardware addresses.
> 
> The MTD subsystem already supports accessing such areas and some (non
> SPI NOR) flashes already implement support for it. It differentiates
> between user and factory areas. User areas can be written by the user and
> factory ones are pre-programmed and locked down by the vendor, usually
> containing an "electrical serial number". This patch will only add support
> for the user areas.
> 
> Lay the foundation and implement the MTD callbacks for the SPI NOR and add
> necessary parameters to the flash_info structure. If a flash supports OTP
> it can be added by the convenience macro OTP_INFO(). Sometimes there are
> individual regions, which might have individual offsets. Therefore, it is
> possible to specify the starting address of the first regions as well as
> the distance between two regions (e.g. Winbond devices uses this method).
> 
> Additionally, the regions might be locked down. Once locked, no further
> write access is possible.
> 
> For SPI NOR flashes the OTP area is accessed like the normal memory, e.g.
> by offset addressing; except that you either have to use special read/write
> commands (Winbond) or you have to enter (and exit) a specific OTP mode
> (Macronix, Micron).
> 
> Thus we introduce four operations to which the MTD callbacks will be
> mapped: .read(), .write(), .lock() and .is_locked(). The read and the write
> ops will be given an address offset to operate on while the locking ops use
> regions because locking always affects a whole region. It is up to the
> flash driver to implement these ops.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/Makefile |   1 +
>  drivers/mtd/spi-nor/core.c   |   5 +
>  drivers/mtd/spi-nor/core.h   |  54 +++++++++
>  drivers/mtd/spi-nor/otp.c    | 218 +++++++++++++++++++++++++++++++++++
>  4 files changed, 278 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/otp.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 653923896205..2ed2e76ce4f1 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -12,6 +12,7 @@ spi-nor-objs                  += intel.o
>  spi-nor-objs                   += issi.o
>  spi-nor-objs                   += macronix.o
>  spi-nor-objs                   += micron-st.o
> +spi-nor-objs                   += otp.o

spi-nor-objs                    := core.o sfdp.o otp.o

This better indicates that otp is part of the "core" driver, while
the rest are manufacturer drivers (that are too part of the core).

>  spi-nor-objs                   += spansion.o
>  spi-nor-objs                   += sst.o
>  spi-nor-objs                   += winbond.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4a315cb1c4db..0c5c757fa95b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3009,6 +3009,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>         spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
>                                SPINOR_OP_SE);
>         spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
> +
> +       nor->params->otp.org = &info->otp_org;

Init this immediately after the setup init, something like:
	params->setup = spi_nor_default_setup;
	params->otp.org = &info->otp_org;

>  }
> 
>  /**
> @@ -3550,6 +3552,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>         if (ret)
>                 return ret;
> 
> +       /* Configure OTP parameters and ops */
> +       spi_nor_otp_init(nor);

Please move this as the last init thing in spi_nor_scan, immediately after
spi_nor_init(nor);

MTD OTP ops are not used accross spi_nor_scan(), there's no need to init
these earlier.
 
> +
>         /* Send all the required SPI flash commands to initialize device */
>         ret = spi_nor_init(nor);
>         if (ret)
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 4a3f7f150b5d..ec8da1243846 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -187,6 +187,45 @@ struct spi_nor_locking_ops {
>         int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>  };
> 
> +/**
> + * struct spi_nor_otp_organization - Structure to describe the SPI NOR OTP regions
> + * @len:       size of one OTP region in bytes.
> + * @base:      start address of the OTP area.
> + * @offset:    offset between consecutive OTP regions if there are more
> + *              than one.
> + * @n_regions: number of individual OTP regions.
> + */
> +struct spi_nor_otp_organization {
> +       size_t len;
> +       loff_t base;
> +       loff_t offset;
> +       unsigned int n_regions;
> +};
> +
> +/**
> + * struct spi_nor_otp_ops - SPI NOR OTP methods
> + * @read:      read from the SPI NOR OTP area.
> + * @write:     write to the SPI NOR OTP area.
> + * @lock:      lock an OTP region.
> + * @is_locked: check if an OTP region of the SPI NOR is locked.
> + */
> +struct spi_nor_otp_ops {
> +       int (*read)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
> +       int (*write)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
> +       int (*lock)(struct spi_nor *nor, unsigned int region);
> +       int (*is_locked)(struct spi_nor *nor, unsigned int region);
> +};
> +
> +/**
> + * struct spi_nor_otp - SPI NOR OTP grouping structure
> + * @org:       OTP region organization
> + * @ops:       OTP access ops
> + */
> +struct spi_nor_otp {
> +       const struct spi_nor_otp_organization *org;
> +       const struct spi_nor_otp_ops *ops;
> +};
> +
>  /**
>   * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings.
>   * Includes legacy flash parameters and settings that can be overwritten
> @@ -208,6 +247,7 @@ struct spi_nor_locking_ops {
>   *                      higher index in the array, the higher priority.
>   * @erase_map:         the erase map parsed from the SFDP Sector Map Parameter
>   *                      Table.
> + * @otp_info:          describes the OTP regions.
>   * @octal_dtr_enable:  enables SPI NOR octal DTR mode.
>   * @quad_enable:       enables SPI NOR quad mode.
>   * @set_4byte_addr_mode: puts the SPI NOR in 4 byte addressing mode.
> @@ -219,6 +259,7 @@ struct spi_nor_locking_ops {
>   *                      e.g. different opcodes, specific address calculation,
>   *                      page size, etc.
>   * @locking_ops:       SPI NOR locking methods.
> + * @otp:               SPI NOR OTP methods.
>   */
>  struct spi_nor_flash_parameter {
>         u64                             size;
> @@ -232,6 +273,7 @@ struct spi_nor_flash_parameter {
>         struct spi_nor_pp_command       page_programs[SNOR_CMD_PP_MAX];
> 
>         struct spi_nor_erase_map        erase_map;
> +       struct spi_nor_otp              otp;
> 
>         int (*octal_dtr_enable)(struct spi_nor *nor, bool enable);
>         int (*quad_enable)(struct spi_nor *nor);
> @@ -341,6 +383,8 @@ struct flash_info {
> 
>         /* Part specific fixup hooks. */
>         const struct spi_nor_fixups *fixups;
> +
> +       const struct spi_nor_otp_organization otp_org;

Can we move otp_org just before fixups? Fixups are usually the last thing
that we want to specify in a flash info.

>  };
> 
>  /* Used when the "_ext_id" is two bytes at most */
> @@ -393,6 +437,14 @@ struct flash_info {
>                 .addr_width = 3,                                        \
>                 .flags = SPI_NOR_NO_FR | SPI_NOR_XSR_RDY,
> 
> +#define OTP_INFO(_len, _n_regions, _base, _offset)                     \
> +               .otp_org = {                                            \
> +                       .len = (_len),                                  \
> +                       .base = (_base),                                \
> +                       .offset = (_offset),                            \
> +                       .n_regions = (_n_regions),                      \
> +               },
> +
>  /**
>   * struct spi_nor_manufacturer - SPI NOR manufacturer object
>   * @name: manufacturer name
> @@ -473,6 +525,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>                              const struct sfdp_bfpt *bfpt,
>                              struct spi_nor_flash_parameter *params);
> 
> +void spi_nor_otp_init(struct spi_nor *nor);
> +
>  static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
>         return mtd->priv;
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> new file mode 100644
> index 000000000000..4e301fd5156b
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -0,0 +1,218 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * OTP support for SPI NOR flashes
> + *
> + * Copyright (C) 2021 Michael Walle <michael@walle.cc>> + */
> +
> +#include <linux/log2.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/spi-nor.h>
> +
> +#include "core.h"
> +
> +#define spi_nor_otp_ops(nor) ((nor)->params->otp.ops)
> +#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
> +#define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)

I don't like these wrappers because they gratuiously hide what's really there.
I find the code more easier to read without these wrappers, because I don't 
have to memorize what these wrappers are doing, and I better see how the code
is organized.

> +
> +static loff_t spi_nor_otp_region_start(const struct spi_nor *nor, int region)
> +{
> +       const struct spi_nor_otp_organization *org = nor->params->otp.org;

how about s/org/otp_org?

> +
> +       return org->base + region * org->offset;
> +}
> +
> +static size_t spi_nor_otp_size(struct spi_nor *nor)
> +{
> +       return spi_nor_otp_n_regions(nor) * spi_nor_otp_region_len(nor);
> +}
> +
> +/*
> + * Translate the file offsets from and to OTP regions. See also
> + * spi_nor_mtd_otp_do_op().
> + */
> +static loff_t spi_nor_otp_region_to_offset(struct spi_nor *nor, unsigned int region)
> +{
> +       return region * spi_nor_otp_region_len(nor);
> +}
> +
> +static unsigned int spi_nor_otp_offset_to_region(struct spi_nor *nor, loff_t ofs)
> +{
> +       return ofs / spi_nor_otp_region_len(nor);
> +}
> +
> +static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
> +                               size_t *retlen, struct otp_info *buf)
> +{
> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +       const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
> +       unsigned int n_regions = spi_nor_otp_n_regions(nor);
> +       unsigned int region;
> +       int ret, locked;
> +
> +       if (len < n_regions * sizeof(*buf))
> +               return -ENOSPC;
> +
> +       ret = spi_nor_lock_and_prep(nor);
> +       if (ret)
> +               return ret;
> +
> +       for (region = 0; region < spi_nor_otp_n_regions(nor); region++) {

for (i = 0; i <  n_regions; i++)

already indicates that i is the index of a region, no need to have an explicit
name. Also, if you want to introduce a local variable, n_regions, use it here
too.

> +               buf->start = spi_nor_otp_region_to_offset(nor, region);
> +               buf->length = spi_nor_otp_region_len(nor);
> +
> +               locked = ops->is_locked(nor, region);
> +               if (locked < 0) {
> +                       ret = locked;
> +                       goto out;
> +               }
> +
> +               buf->locked = !!locked;
> +               buf++;
> +       }
> +
> +       *retlen = n_regions * sizeof(*buf);
> +
> +out:
> +       spi_nor_unlock_and_unprep(nor);
> +
> +       return ret;
> +}
> +
> +static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t ofs,
> +                                     size_t total_len, size_t *retlen,
> +                                     u_char *buf, bool is_write)

not related to this patch, but the list of arguments is quite big, maybe
we can update mtd to pass a pointer to a struct.

> +{
> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +       const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
> +       const size_t rlen = spi_nor_otp_region_len(nor);
> +       loff_t rstart, rofs;
> +       unsigned int region;
> +       size_t len;
> +       int ret;
> +
> +       if (ofs < 0 || ofs >= spi_nor_otp_size(nor))
> +               return 0;
> +
> +       ret = spi_nor_lock_and_prep(nor);
> +       if (ret)
> +               return ret;
> +
> +       /* don't access beyond the end */
> +       total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - ofs);
> +
> +       *retlen = 0;
> +       while (total_len) {
> +               /*
> +                * The OTP regions are mapped into a contiguous area starting
> +                * at 0 as expected by the MTD layer. This will map the MTD
> +                * file offsets to the address of an OTP region as used in the
> +                * actual SPI commands.
> +                */
> +               region = spi_nor_otp_offset_to_region(nor, ofs);
> +               rstart = spi_nor_otp_region_start(nor, region);

Maybe it's just me, but I don't like the helpers :).
Especially spi_nor_otp_offset_to_region. And spi_nor_otp_region_start()
is used just here. Maybe discard them and s/region/i?

> +
> +               /*
> +                * The size of a OTP region is expected to be a power of two,
> +                * thus we can just mask the lower bits and get the offset into
> +                * a region.
> +                */
> +               rofs = ofs & (rlen - 1);
> +
> +               /* don't access beyond one OTP region */
> +               len = min_t(size_t, total_len, rlen - rofs);
> +
> +               if (is_write)
> +                       ret = ops->write(nor, rstart + rofs, len, buf);
> +               else
> +                       ret = ops->read(nor, rstart + rofs, len, buf);
> +               if (ret == 0)
> +                       ret = -EIO;
> +               if (ret < 0)
> +                       goto out;
> +
> +               *retlen += ret;
> +               ofs += ret;
> +               buf += ret;
> +               total_len -= ret;
> +       }
> +       ret = 0;
> +
> +out:
> +       spi_nor_unlock_and_unprep(nor);
> +       return ret;
> +}
> +
> +static int spi_nor_mtd_otp_read(struct mtd_info *mtd, loff_t from, size_t len,
> +                               size_t *retlen, u_char *buf)
> +{
> +       return spi_nor_mtd_otp_read_write(mtd, from, len, retlen, buf, false);
> +}
> +
> +static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
> +                                size_t *retlen, u_char *buf)
> +{
> +       return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
> +}
> +
> +static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +       const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
> +       unsigned int region;
> +       int ret;
> +
> +       if (from < 0 || (from + len) > spi_nor_otp_size(nor))
> +               return -EINVAL;
> +
> +       /* the user has to explicitly ask for whole regions */
> +       if (len % spi_nor_otp_region_len(nor))
> +               return -EINVAL;
> +
> +       if (from % spi_nor_otp_region_len(nor))
> +               return -EINVAL;
> +
> +       ret = spi_nor_lock_and_prep(nor);
> +       if (ret)
> +               return ret;
> +
> +       while (len) {
> +               region = spi_nor_otp_offset_to_region(nor, from);
> +               ret = ops->lock(nor, region);
> +               if (ret)
> +                       goto out;
> +
> +               len -= spi_nor_otp_region_len(nor);
> +               from += spi_nor_otp_region_len(nor);
> +       }
> +
> +out:
> +       spi_nor_unlock_and_unprep(nor);
> +
> +       return ret;
> +}
> +
> +void spi_nor_otp_init(struct spi_nor *nor)
> +{
> +       struct mtd_info *mtd = &nor->mtd;
> +
> +       if (!nor->params->otp.ops)
> +               return;
> +
> +       if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor))))

Why WARN_ON and not just a debug message?

Cheers,
ta

> +               return;
> +
> +       /*
> +        * We only support user_prot callbacks (yet).
> +        *
> +        * Some SPI NOR flashes like Macronix ones can be ordered in two
> +        * different variants. One with a factory locked OTP area and one where
> +        * it is left to the user to write to it. The factory locked OTP is
> +        * usually preprogrammed with an "electrical serial number". We don't
> +        * support these for now.
> +        */
> +       mtd->_get_user_prot_info = spi_nor_mtd_otp_info;
> +       mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
> +       mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
> +       mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
> +}
> --
> 2.20.1
> 

Re: [PATCH v4 2/4] mtd: spi-nor: implement OTP support for Winbond and similar flashes

[Tudor.Ambarus]

On 3/6/21 2:05 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Use the new OTP ops to implement OTP access on Winbond flashes. Most
> Winbond flashes provides up to four different OTP regions ("Security
> Registers").
> 
> Winbond devices use a special opcode to read and write to the OTP
> regions, just like the RDSFDP opcode. In fact, it seems that the
> (undocumented) first OTP area of the newer flashes is the actual SFDP
> table.
> 
> On a side note, Winbond devices also allow erasing the OTP regions as
> long as the area isn't locked down.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c  |   2 +-
>  drivers/mtd/spi-nor/core.h  |   6 ++
>  drivers/mtd/spi-nor/otp.c   | 164 ++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h |   9 ++
>  4 files changed, 180 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0c5c757fa95b..ef7df26896f1 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1034,7 +1034,7 @@ static int spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
> +int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
>  {
>         int ret;
>         u8 *sr_cr = nor->bouncebuf;
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index ec8da1243846..dfbf6ba42b57 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -496,6 +496,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
>  int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
>  int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
>  int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
> +int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);
> 
>  int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
>  ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
> @@ -503,6 +504,11 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>                            const u8 *buf);
> 
> +int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
> +int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
> +int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
> +int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
> +
>  int spi_nor_hwcaps_read2cmd(u32 hwcaps);
>  u8 spi_nor_convert_3to4_read(u8 opcode);
>  void spi_nor_set_read_settings(struct spi_nor_read_command *read,
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 4e301fd5156b..4e8da9108c77 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -15,6 +15,170 @@
>  #define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
>  #define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
> 
> +/**
> + * spi_nor_otp_read_secr() - read OTP data
> + * @nor:       pointer to 'struct spi_nor'
> + * @from:       offset to read from
> + * @len:        number of bytes to read
> + * @buf:        pointer to dst buffer

is buf DMA-able?

> + *
> + * Read OTP data from one region by using the SPINOR_OP_RSECR commands. This
> + * method is used on GigaDevice and Winbond flashes.
> + *
> + * Return: number of bytes read successfully, -errno otherwise
> + */
> +int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
> +{
> +       u8 addr_width, read_opcode, read_dummy;
> +       struct spi_mem_dirmap_desc *rdesc;
> +       enum spi_nor_protocol read_proto;
> +       int ret;
> +
> +       read_opcode = nor->read_opcode;
> +       addr_width = nor->addr_width;
> +       read_dummy = nor->read_dummy;
> +       read_proto = nor->read_proto;
> +       rdesc = nor->dirmap.rdesc;
> +
> +       nor->read_opcode = SPINOR_OP_RSECR;
> +       nor->addr_width = 3;
> +       nor->read_dummy = 8;
> +       nor->read_proto = SNOR_PROTO_1_1_1;

any winbond/gigadevice flashes with octal dtr support? Do they
provide SEC Register opcodes for octal dtr?

> +       nor->dirmap.rdesc = NULL;
> +
> +       ret = spi_nor_read_data(nor, addr, len, buf);
> +
> +       nor->read_opcode = read_opcode;
> +       nor->addr_width = addr_width;
> +       nor->read_dummy = read_dummy;
> +       nor->read_proto = read_proto;
> +       nor->dirmap.rdesc = rdesc;
> +
> +       return ret;
> +}
> +
> +/**
> + * spi_nor_otp_write_secr() - write OTP data
> + * @nor:        pointer to 'struct spi_nor'
> + * @to:         offset to write to
> + * @len:        number of bytes to write
> + * @buf:        pointer to src buffer
> + *
> + * Write OTP data to one region by using the SPINOR_OP_PSECR commands. This
> + * method is used on GigaDevice and Winbond flashes.
> + *
> + * Please note, the write must not span multiple OTP regions.
> + *
> + * Return: number of bytes written successfully, -errno otherwise
> + */
> +int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
> +{
> +       enum spi_nor_protocol write_proto;
> +       struct spi_mem_dirmap_desc *wdesc;
> +       u8 addr_width, program_opcode;
> +       int ret, written;
> +
> +       program_opcode = nor->program_opcode;
> +       addr_width = nor->addr_width;
> +       write_proto = nor->write_proto;
> +       wdesc = nor->dirmap.wdesc;
> +
> +       nor->program_opcode = SPINOR_OP_PSECR;
> +       nor->addr_width = 3;
> +       nor->write_proto = SNOR_PROTO_1_1_1;
> +       nor->dirmap.wdesc = NULL;
> +
> +       /*
> +        * We only support a write to one single page. For now all winbond
> +        * flashes only have one page per OTP region.
> +        */
> +       ret = spi_nor_write_enable(nor);
> +       if (ret)
> +               goto out;
> +
> +       written = spi_nor_write_data(nor, addr, len, buf);
> +       if (written < 0)
> +               goto out;
> +
> +       ret = spi_nor_wait_till_ready(nor);
> +
> +out:
> +       nor->program_opcode = program_opcode;
> +       nor->addr_width = addr_width;
> +       nor->write_proto = write_proto;
> +       nor->dirmap.wdesc = wdesc;
> +
> +       return ret ?: written;
> +}
> +
> +static int spi_nor_otp_lock_bit_cr(unsigned int region)
> +{
> +       static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
> +
> +       if (region >= ARRAY_SIZE(lock_bits))
> +               return -EINVAL;
> +
> +       return lock_bits[region];
> +}
> +
> +/**
> + * spi_nor_otp_lock_sr2() - lock the OTP region
> + * @nor:        pointer to 'struct spi_nor'
> + * @region:     OTP region
> + *
> + * Lock the OTP region by writing the status register-2. This method is used on
> + * GigaDevice and Winbond flashes.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region)
> +{
> +       u8 *cr = nor->bouncebuf;
> +       int ret, lock_bit;
> +
> +       lock_bit = spi_nor_otp_lock_bit_cr(region);
> +       if (lock_bit < 0)
> +               return lock_bit;
> +
> +       ret = spi_nor_read_cr(nor, cr);
> +       if (ret)
> +               return ret;
> +
> +       /* no need to write the register if region is already locked */
> +       if (cr[0] & lock_bit)
> +               return 0;
> +
> +       cr[0] |= lock_bit;
> +
> +       return spi_nor_write_16bit_cr_and_check(nor, cr[0]);
> +}
> +
> +/**
> + * spi_nor_otp_is_locked_sr2() - get the OTP region lock status
> + * @nor:        pointer to 'struct spi_nor'
> + * @region:     OTP region
> + *
> + * Retrieve the OTP region lock bit by reading the status register-2. This
> + * method is used on GigaDevice and Winbond flashes.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region)
> +{
> +       u8 *cr = nor->bouncebuf;
> +       int ret, lock_bit;
> +
> +       lock_bit = spi_nor_otp_lock_bit_cr(region);
> +       if (lock_bit < 0)
> +               return lock_bit;
> +
> +       ret = spi_nor_read_cr(nor, cr);
> +       if (ret)
> +               return ret;
> +
> +       return cr[0] & lock_bit;
> +}
> +
>  static loff_t spi_nor_otp_region_start(const struct spi_nor *nor, int region)
>  {
>         const struct spi_nor_otp_organization *org = nor->params->otp.org;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index a0d572855444..6d1956049e90 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -107,6 +107,11 @@
>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
>  #define SPINOR_OP_WD_EVCR      0x61    /* Write EVCR register */
> 
> +/* Used for GigaDevices and Winbond flashes. */
> +#define SPINOR_OP_ESECR                0x44    /* Erase Security registers */
> +#define SPINOR_OP_PSECR                0x42    /* Program Security registers */
> +#define SPINOR_OP_RSECR                0x48    /* Read Security registers */
> +
>  /* Status Register bits. */
>  #define SR_WIP                 BIT(0)  /* Write in progress */
>  #define SR_WEL                 BIT(1)  /* Write enable latch */
> @@ -138,8 +143,12 @@
> 
>  /* Status Register 2 bits. */
>  #define SR2_QUAD_EN_BIT1       BIT(1)
> +#define SR2_LB1                        BIT(3)  /* Security Register Lock Bit 1 */
> +#define SR2_LB2                        BIT(4)  /* Security Register Lock Bit 2 */
> +#define SR2_LB3                        BIT(5)  /* Security Register Lock Bit 3 */
>  #define SR2_QUAD_EN_BIT7       BIT(7)
> 
> +

not needed. You can catch this when running ./scripts/checkpatch --strict patch-name

>  /* Supported SPI protocols */
>  #define SNOR_PROTO_INST_MASK   GENMASK(23, 16)
>  #define SNOR_PROTO_INST_SHIFT  16
> --
> 2.20.1
> 

Re: [PATCH v4 3/4] mtd: spi-nor: winbond: add OTP support to w25q32fw/jw

[Tudor.Ambarus]

On 3/6/21 2:05 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> With all the helper functions in place, add OTP support for the Winbond
> W25Q32JW and W25Q32FW.
> 
> Both were tested on a LS1028A SoC with a NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/winbond.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index e5dfa786f190..9a3f8ff007fd 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -55,14 +55,18 @@ static const struct flash_info winbond_parts[] = {
>         { "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
>         { "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64,
>                            SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -                          SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> +                          SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> +                          OTP_INFO(256, 3, 0x1000, 0x1000)
> +       },
> +
>         { "w25q32jv", INFO(0xef7016, 0, 64 * 1024,  64,
>                            SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                            SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>         },
>         { "w25q32jwm", INFO(0xef8016, 0, 64 * 1024,  64,
>                             SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> -                           SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> +                           SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> +                           OTP_INFO(256, 3, 0x1000, 0x1000) },
>         { "w25q64jwm", INFO(0xef8017, 0, 64 * 1024, 128,
>                             SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                             SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> @@ -131,9 +135,18 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>         return spi_nor_write_disable(nor);
>  }
> 
> +static const struct spi_nor_otp_ops winbond_otp_ops = {
> +       .read = spi_nor_otp_read_secr,
> +       .write = spi_nor_otp_write_secr,
> +       .lock = spi_nor_otp_lock_sr2,
> +       .is_locked = spi_nor_otp_is_locked_sr2,
> +};

Should we have this in otp.c? It can be shared with gigadevice as well
as far as I understood.

Cheers,
ta

> +
>  static void winbond_default_init(struct spi_nor *nor)
>  {
>         nor->params->set_4byte_addr_mode = winbond_set_4byte_addr_mode;
> +       if (nor->params->otp.org->n_regions)
> +               nor->params->otp.ops = &winbond_otp_ops;
>  }
> 
>  static const struct spi_nor_fixups winbond_fixups = {
> --
> 2.20.1
> 

Re: [PATCH v4 2/4] mtd: spi-nor: implement OTP support for Winbond and similar flashes

[Tudor.Ambarus]

On 3/6/21 2:05 AM, Michael Walle wrote:
> +       nor->dirmap.rdesc = NULL;

why can't we use dirmap?

> +
> +       ret = spi_nor_read_data(nor, addr, len, buf);

Re: [PATCH v4 4/4] mtd: spi-nor: implement OTP erase for Winbond and similar flashes

[Tudor.Ambarus]

On 3/6/21 2:05 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Winbond flashes with OTP support provide a command to erase the OTP
> data. This might come in handy during development.
> 
> This was tested with a Winbond W25Q32JW on a LS1028A SoC with the
> NXP FSPI controller.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

I skimmed through this, seems ok. Send 4/4 with the new ioctl addition
in a dedicated patch set, ideally both will go through the spi-nor/next
branch. For the new ioctl we'll need ACKs from all the mtd maintainers
and at least a Tested-by tag. Please send the mtd-utils changes too.

Cheers,
ta
> ---
>  drivers/mtd/spi-nor/core.c    |  4 +-
>  drivers/mtd/spi-nor/core.h    |  4 ++
>  drivers/mtd/spi-nor/otp.c     | 74 ++++++++++++++++++++++++++++++++++-
>  drivers/mtd/spi-nor/winbond.c |  1 +
>  4 files changed, 79 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ef7df26896f1..21a737804576 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -166,8 +166,8 @@ static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
>         return nor->controller_ops->read_reg(nor, opcode, buf, len);
>  }
> 
> -static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> -                                           const u8 *buf, size_t len)
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +                                    const u8 *buf, size_t len)
>  {
>         if (spi_nor_protocol_is_dtr(nor->reg_proto))
>                 return -EOPNOTSUPP;
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index dfbf6ba42b57..ef62ec4625a1 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -213,6 +213,7 @@ struct spi_nor_otp_ops {
>         int (*read)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
>         int (*write)(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
>         int (*lock)(struct spi_nor *nor, unsigned int region);
> +       int (*erase)(struct spi_nor *nor, loff_t addr);
>         int (*is_locked)(struct spi_nor *nor, unsigned int region);
>  };
> 
> @@ -481,6 +482,8 @@ extern const struct spi_nor_manufacturer spi_nor_xmc;
>  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_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +                                    const u8 *buf, 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(struct spi_nor *nor, bool enable);
> @@ -506,6 +509,7 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> 
>  int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
>  int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf);
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr);
>  int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
>  int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region);
> 
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index 4e8da9108c77..78ec79368c29 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -8,6 +8,7 @@
>  #include <linux/log2.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi-mem.h>
> 
>  #include "core.h"
> 
> @@ -111,6 +112,50 @@ int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf
>         return ret ?: written;
>  }
> 
> +/**
> + * spi_nor_otp_erase_secr() - erase one OTP region
> + * @nor:        pointer to 'struct spi_nor'
> + * @to:         offset to write to
> + * @len:        number of bytes to write
> + * @buf:        pointer to src buffer
> + *
> + * Erase one OTP region by using the SPINOR_OP_ESECR commands. This method is
> + * used on GigaDevice and Winbond flashes.
> + *
> + * Return: 0 on success, -errno otherwise
> + */
> +int spi_nor_otp_erase_secr(struct spi_nor *nor, loff_t addr)
> +{
> +       int ret;
> +
> +       ret = spi_nor_write_enable(nor);
> +       if (ret)
> +               return ret;
> +
> +       if (nor->spimem) {
> +               struct spi_mem_op op =
> +                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_ESECR, 0),
> +                                  SPI_MEM_OP_ADDR(3, addr, 0),
> +                                  SPI_MEM_OP_NO_DUMMY,
> +                                  SPI_MEM_OP_NO_DATA);
> +
> +               spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> +
> +               ret = spi_mem_exec_op(nor->spimem, &op);
> +       } else {
> +               nor->bouncebuf[2] = addr & 0xff;
> +               nor->bouncebuf[1] = (addr >> 8) & 0xff;
> +               nor->bouncebuf[0] = (addr >> 16) & 0xff;
> +
> +               ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_ESECR,
> +                                                      nor->bouncebuf, 3);
> +       }
> +       if (ret)
> +               return ret;
> +
> +       return spi_nor_wait_till_ready(nor);
> +}
> +
>  static int spi_nor_otp_lock_bit_cr(unsigned int region)
>  {
>         static const int lock_bits[] = { SR2_LB1, SR2_LB2, SR2_LB3 };
> @@ -319,11 +364,13 @@ static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, size_t len,
>         return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, true);
>  }
> 
> -static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
> +static int spi_nor_mtd_otp_lock_erase(struct mtd_info *mtd, loff_t from,
> +                                     size_t len, bool is_erase)
>  {
>         struct spi_nor *nor = mtd_to_spi_nor(mtd);
>         const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
>         unsigned int region;
> +       loff_t start;
>         int ret;
> 
>         if (from < 0 || (from + len) > spi_nor_otp_size(nor))
> @@ -342,7 +389,12 @@ static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
> 
>         while (len) {
>                 region = spi_nor_otp_offset_to_region(nor, from);
> -               ret = ops->lock(nor, region);
> +               start = spi_nor_otp_region_start(nor, region);
> +
> +               if (is_erase)
> +                       ret = ops->erase(nor, start);
> +               else
> +                       ret = ops->lock(nor, region);
>                 if (ret)
>                         goto out;
> 
> @@ -356,6 +408,23 @@ static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
>         return ret;
>  }
> 
> +static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +       return spi_nor_mtd_otp_lock_erase(mtd, from, len, false);
> +}
> +
> +static int spi_nor_mtd_otp_erase(struct mtd_info *mtd, loff_t from, size_t len)
> +{
> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +       const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
> +
> +       /* OTP erase is optional */
> +       if (!ops->erase)
> +               return -EOPNOTSUPP;
> +
> +       return spi_nor_mtd_otp_lock_erase(mtd, from, len, true);
> +}
> +
>  void spi_nor_otp_init(struct spi_nor *nor)
>  {
>         struct mtd_info *mtd = &nor->mtd;
> @@ -379,4 +448,5 @@ void spi_nor_otp_init(struct spi_nor *nor)
>         mtd->_read_user_prot_reg = spi_nor_mtd_otp_read;
>         mtd->_write_user_prot_reg = spi_nor_mtd_otp_write;
>         mtd->_lock_user_prot_reg = spi_nor_mtd_otp_lock;
> +       mtd->_erase_user_prot_reg = spi_nor_mtd_otp_erase;
>  }
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 9a3f8ff007fd..e04219ac11fd 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -138,6 +138,7 @@ static int winbond_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  static const struct spi_nor_otp_ops winbond_otp_ops = {
>         .read = spi_nor_otp_read_secr,
>         .write = spi_nor_otp_write_secr,
> +       .erase = spi_nor_otp_erase_secr,
>         .lock = spi_nor_otp_lock_sr2,
>         .is_locked = spi_nor_otp_is_locked_sr2,
>  };
> --
> 2.20.1
> 

Re: [PATCH v4 1/4] mtd: spi-nor: add OTP support

[Michael Walle]

Am 2021-03-15 08:28, schrieb Tudor.Ambarus@microchip.com:
> Michael,
> 
> Just cosmetic suggestions this time.
> 
> On 3/6/21 2:05 AM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> SPI flashes sometimes have a special OTP area, which can (and is) used 
>> to
>> store immutable properties like board serial number or vendor assigned
>> network hardware addresses.
>> 
>> The MTD subsystem already supports accessing such areas and some (non
>> SPI NOR) flashes already implement support for it. It differentiates
>> between user and factory areas. User areas can be written by the user 
>> and
>> factory ones are pre-programmed and locked down by the vendor, usually
>> containing an "electrical serial number". This patch will only add 
>> support
>> for the user areas.
>> 
>> Lay the foundation and implement the MTD callbacks for the SPI NOR and 
>> add
>> necessary parameters to the flash_info structure. If a flash supports 
>> OTP
>> it can be added by the convenience macro OTP_INFO(). Sometimes there 
>> are
>> individual regions, which might have individual offsets. Therefore, it 
>> is
>> possible to specify the starting address of the first regions as well 
>> as
>> the distance between two regions (e.g. Winbond devices uses this 
>> method).
>> 
>> Additionally, the regions might be locked down. Once locked, no 
>> further
>> write access is possible.
>> 
>> For SPI NOR flashes the OTP area is accessed like the normal memory, 
>> e.g.
>> by offset addressing; except that you either have to use special 
>> read/write
>> commands (Winbond) or you have to enter (and exit) a specific OTP mode
>> (Macronix, Micron).
>> 
>> Thus we introduce four operations to which the MTD callbacks will be
>> mapped: .read(), .write(), .lock() and .is_locked(). The read and the 
>> write
>> ops will be given an address offset to operate on while the locking 
>> ops use
>> regions because locking always affects a whole region. It is up to the
>> flash driver to implement these ops.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/Makefile |   1 +
>>  drivers/mtd/spi-nor/core.c   |   5 +
>>  drivers/mtd/spi-nor/core.h   |  54 +++++++++
>>  drivers/mtd/spi-nor/otp.c    | 218 
>> +++++++++++++++++++++++++++++++++++
>>  4 files changed, 278 insertions(+)
>>  create mode 100644 drivers/mtd/spi-nor/otp.c
>> 
>> diff --git a/drivers/mtd/spi-nor/Makefile 
>> b/drivers/mtd/spi-nor/Makefile
>> index 653923896205..2ed2e76ce4f1 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -12,6 +12,7 @@ spi-nor-objs                  += intel.o
>>  spi-nor-objs                   += issi.o
>>  spi-nor-objs                   += macronix.o
>>  spi-nor-objs                   += micron-st.o
>> +spi-nor-objs                   += otp.o
> 
> spi-nor-objs                    := core.o sfdp.o otp.o
> 
> This better indicates that otp is part of the "core" driver, while
> the rest are manufacturer drivers (that are too part of the core).

That is already fixed here. Didn't want to send yet another version ;)

> 
>>  spi-nor-objs                   += spansion.o
>>  spi-nor-objs                   += sst.o
>>  spi-nor-objs                   += winbond.o
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 4a315cb1c4db..0c5c757fa95b 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -3009,6 +3009,8 @@ static void spi_nor_info_init_params(struct 
>> spi_nor *nor)
>>         spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
>>                                SPINOR_OP_SE);
>>         spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
>> +
>> +       nor->params->otp.org = &info->otp_org;
> 
> Init this immediately after the setup init, something like:
> 	params->setup = spi_nor_default_setup;
> 	params->otp.org = &info->otp_org;
> 
>>  }
>> 
>>  /**
>> @@ -3550,6 +3552,9 @@ int spi_nor_scan(struct spi_nor *nor, const char 
>> *name,
>>         if (ret)
>>                 return ret;
>> 
>> +       /* Configure OTP parameters and ops */
>> +       spi_nor_otp_init(nor);
> 
> Please move this as the last init thing in spi_nor_scan, immediately 
> after
> spi_nor_init(nor);
> 
> MTD OTP ops are not used accross spi_nor_scan(), there's no need to 
> init
> these earlier.

Makes sense.

> 
>> +
>>         /* Send all the required SPI flash commands to initialize 
>> device */
>>         ret = spi_nor_init(nor);
>>         if (ret)
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 4a3f7f150b5d..ec8da1243846 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -187,6 +187,45 @@ struct spi_nor_locking_ops {
>>         int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t 
>> len);
>>  };
>> 
>> +/**
>> + * struct spi_nor_otp_organization - Structure to describe the SPI 
>> NOR OTP regions
>> + * @len:       size of one OTP region in bytes.
>> + * @base:      start address of the OTP area.
>> + * @offset:    offset between consecutive OTP regions if there are 
>> more
>> + *              than one.
>> + * @n_regions: number of individual OTP regions.
>> + */
>> +struct spi_nor_otp_organization {
>> +       size_t len;
>> +       loff_t base;
>> +       loff_t offset;
>> +       unsigned int n_regions;
>> +};
>> +
>> +/**
>> + * struct spi_nor_otp_ops - SPI NOR OTP methods
>> + * @read:      read from the SPI NOR OTP area.
>> + * @write:     write to the SPI NOR OTP area.
>> + * @lock:      lock an OTP region.
>> + * @is_locked: check if an OTP region of the SPI NOR is locked.
>> + */
>> +struct spi_nor_otp_ops {
>> +       int (*read)(struct spi_nor *nor, loff_t addr, size_t len, u8 
>> *buf);
>> +       int (*write)(struct spi_nor *nor, loff_t addr, size_t len, u8 
>> *buf);
>> +       int (*lock)(struct spi_nor *nor, unsigned int region);
>> +       int (*is_locked)(struct spi_nor *nor, unsigned int region);
>> +};
>> +
>> +/**
>> + * struct spi_nor_otp - SPI NOR OTP grouping structure
>> + * @org:       OTP region organization
>> + * @ops:       OTP access ops
>> + */
>> +struct spi_nor_otp {
>> +       const struct spi_nor_otp_organization *org;
>> +       const struct spi_nor_otp_ops *ops;
>> +};
>> +
>>  /**
>>   * struct spi_nor_flash_parameter - SPI NOR flash parameters and 
>> settings.
>>   * Includes legacy flash parameters and settings that can be 
>> overwritten
>> @@ -208,6 +247,7 @@ struct spi_nor_locking_ops {
>>   *                      higher index in the array, the higher 
>> priority.
>>   * @erase_map:         the erase map parsed from the SFDP Sector Map 
>> Parameter
>>   *                      Table.
>> + * @otp_info:          describes the OTP regions.
>>   * @octal_dtr_enable:  enables SPI NOR octal DTR mode.
>>   * @quad_enable:       enables SPI NOR quad mode.
>>   * @set_4byte_addr_mode: puts the SPI NOR in 4 byte addressing mode.
>> @@ -219,6 +259,7 @@ struct spi_nor_locking_ops {
>>   *                      e.g. different opcodes, specific address 
>> calculation,
>>   *                      page size, etc.
>>   * @locking_ops:       SPI NOR locking methods.
>> + * @otp:               SPI NOR OTP methods.
>>   */
>>  struct spi_nor_flash_parameter {
>>         u64                             size;
>> @@ -232,6 +273,7 @@ struct spi_nor_flash_parameter {
>>         struct spi_nor_pp_command       
>> page_programs[SNOR_CMD_PP_MAX];
>> 
>>         struct spi_nor_erase_map        erase_map;
>> +       struct spi_nor_otp              otp;
>> 
>>         int (*octal_dtr_enable)(struct spi_nor *nor, bool enable);
>>         int (*quad_enable)(struct spi_nor *nor);
>> @@ -341,6 +383,8 @@ struct flash_info {
>> 
>>         /* Part specific fixup hooks. */
>>         const struct spi_nor_fixups *fixups;
>> +
>> +       const struct spi_nor_otp_organization otp_org;
> 
> Can we move otp_org just before fixups? Fixups are usually the last 
> thing
> that we want to specify in a flash info.

sure.

>>  };
>> 
>>  /* Used when the "_ext_id" is two bytes at most */
>> @@ -393,6 +437,14 @@ struct flash_info {
>>                 .addr_width = 3,                                       
>>  \
>>                 .flags = SPI_NOR_NO_FR | SPI_NOR_XSR_RDY,
>> 
>> +#define OTP_INFO(_len, _n_regions, _base, _offset)                    
>>  \
>> +               .otp_org = {                                           
>>  \
>> +                       .len = (_len),                                 
>>  \
>> +                       .base = (_base),                               
>>  \
>> +                       .offset = (_offset),                           
>>  \
>> +                       .n_regions = (_n_regions),                     
>>  \
>> +               },
>> +
>>  /**
>>   * struct spi_nor_manufacturer - SPI NOR manufacturer object
>>   * @name: manufacturer name
>> @@ -473,6 +525,8 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
>>                              const struct sfdp_bfpt *bfpt,
>>                              struct spi_nor_flash_parameter *params);
>> 
>> +void spi_nor_otp_init(struct spi_nor *nor);
>> +
>>  static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info 
>> *mtd)
>>  {
>>         return mtd->priv;
>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>> new file mode 100644
>> index 000000000000..4e301fd5156b
>> --- /dev/null
>> +++ b/drivers/mtd/spi-nor/otp.c
>> @@ -0,0 +1,218 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * OTP support for SPI NOR flashes
>> + *
>> + * Copyright (C) 2021 Michael Walle <michael@walle.cc>> + */
>> +
>> +#include <linux/log2.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/spi-nor.h>
>> +
>> +#include "core.h"
>> +
>> +#define spi_nor_otp_ops(nor) ((nor)->params->otp.ops)
>> +#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
>> +#define spi_nor_otp_n_regions(nor) 
>> ((nor)->params->otp.org->n_regions)
> 
> I don't like these wrappers because they gratuiously hide what's really 
> there.
> I find the code more easier to read without these wrappers, because I 
> don't
> have to memorize what these wrappers are doing, and I better see how 
> the code
> is organized.

TBH I find it easier on the eye and I've never seen so much dereferences
as in mtd/spi-nor.

>> +
>> +static loff_t spi_nor_otp_region_start(const struct spi_nor *nor, int 
>> region)
>> +{
>> +       const struct spi_nor_otp_organization *org = 
>> nor->params->otp.org;
> 
> how about s/org/otp_org?

Yeah, as it is just a one line function, I don't really care ;)

>> +
>> +       return org->base + region * org->offset;
>> +}
>> +
>> +static size_t spi_nor_otp_size(struct spi_nor *nor)
>> +{
>> +       return spi_nor_otp_n_regions(nor) * 
>> spi_nor_otp_region_len(nor);
>> +}
>> +
>> +/*
>> + * Translate the file offsets from and to OTP regions. See also
>> + * spi_nor_mtd_otp_do_op().
>> + */
>> +static loff_t spi_nor_otp_region_to_offset(struct spi_nor *nor, 
>> unsigned int region)
>> +{
>> +       return region * spi_nor_otp_region_len(nor);
>> +}
>> +
>> +static unsigned int spi_nor_otp_offset_to_region(struct spi_nor *nor, 
>> loff_t ofs)
>> +{
>> +       return ofs / spi_nor_otp_region_len(nor);
>> +}
>> +
>> +static int spi_nor_mtd_otp_info(struct mtd_info *mtd, size_t len,
>> +                               size_t *retlen, struct otp_info *buf)
>> +{
>> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +       const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
>> +       unsigned int n_regions = spi_nor_otp_n_regions(nor);
>> +       unsigned int region;
>> +       int ret, locked;
>> +
>> +       if (len < n_regions * sizeof(*buf))
>> +               return -ENOSPC;
>> +
>> +       ret = spi_nor_lock_and_prep(nor);
>> +       if (ret)
>> +               return ret;
>> +
>> +       for (region = 0; region < spi_nor_otp_n_regions(nor); 
>> region++) {
> 
> for (i = 0; i <  n_regions; i++)
> 
> already indicates that i is the index of a region, no need to have an 
> explicit
> name. Also, if you want to introduce a local variable, n_regions, use 
> it here
> too.

Mh, thas was a left over, from when I had a for_each_ helper. But the
new version just has this one occurence, so I've removed that. I
already felt there was some resistance on these helpers ;)

> 
>> +               buf->start = spi_nor_otp_region_to_offset(nor, 
>> region);
>> +               buf->length = spi_nor_otp_region_len(nor);
>> +
>> +               locked = ops->is_locked(nor, region);
>> +               if (locked < 0) {
>> +                       ret = locked;
>> +                       goto out;
>> +               }
>> +
>> +               buf->locked = !!locked;
>> +               buf++;
>> +       }
>> +
>> +       *retlen = n_regions * sizeof(*buf);
>> +
>> +out:
>> +       spi_nor_unlock_and_unprep(nor);
>> +
>> +       return ret;
>> +}
>> +
>> +static int spi_nor_mtd_otp_read_write(struct mtd_info *mtd, loff_t 
>> ofs,
>> +                                     size_t total_len, size_t 
>> *retlen,
>> +                                     u_char *buf, bool is_write)
> 
> not related to this patch, but the list of arguments is quite big, 
> maybe
> we can update mtd to pass a pointer to a struct.

For external functions, sure. But this is internal so I don't
see any advantage here. Also this is kinda special because its
the argument list of the mtd ops appended with the is_write
parameter.

>> +{
>> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +       const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
>> +       const size_t rlen = spi_nor_otp_region_len(nor);
>> +       loff_t rstart, rofs;
>> +       unsigned int region;
>> +       size_t len;
>> +       int ret;
>> +
>> +       if (ofs < 0 || ofs >= spi_nor_otp_size(nor))
>> +               return 0;
>> +
>> +       ret = spi_nor_lock_and_prep(nor);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* don't access beyond the end */
>> +       total_len = min_t(size_t, total_len, spi_nor_otp_size(nor) - 
>> ofs);
>> +
>> +       *retlen = 0;
>> +       while (total_len) {
>> +               /*
>> +                * The OTP regions are mapped into a contiguous area 
>> starting
>> +                * at 0 as expected by the MTD layer. This will map 
>> the MTD
>> +                * file offsets to the address of an OTP region as 
>> used in the
>> +                * actual SPI commands.
>> +                */
>> +               region = spi_nor_otp_offset_to_region(nor, ofs);
>> +               rstart = spi_nor_otp_region_start(nor, region);
> 
> Maybe it's just me, but I don't like the helpers :).
> Especially spi_nor_otp_offset_to_region.

Ha, I actually had this open coded. But in the end, I wanted to
keep the translation from MTD offsets to OTP offsets in one place.
That is spi_nor_otp_region_to_offset() and 
spi_nor_otp_offset_to_region().

> And spi_nor_otp_region_start()
> is used just here. Maybe discard them and s/region/i?

I'm not sure what you mean by use i? Here is no for loop.
And regarding the helper, it is used in 4/4 again. So it is
quite handy ;)

>> +
>> +               /*
>> +                * The size of a OTP region is expected to be a power 
>> of two,
>> +                * thus we can just mask the lower bits and get the 
>> offset into
>> +                * a region.
>> +                */
>> +               rofs = ofs & (rlen - 1);
>> +
>> +               /* don't access beyond one OTP region */
>> +               len = min_t(size_t, total_len, rlen - rofs);
>> +
>> +               if (is_write)
>> +                       ret = ops->write(nor, rstart + rofs, len, 
>> buf);
>> +               else
>> +                       ret = ops->read(nor, rstart + rofs, len, buf);
>> +               if (ret == 0)
>> +                       ret = -EIO;
>> +               if (ret < 0)
>> +                       goto out;
>> +
>> +               *retlen += ret;
>> +               ofs += ret;
>> +               buf += ret;
>> +               total_len -= ret;
>> +       }
>> +       ret = 0;
>> +
>> +out:
>> +       spi_nor_unlock_and_unprep(nor);
>> +       return ret;
>> +}
>> +
>> +static int spi_nor_mtd_otp_read(struct mtd_info *mtd, loff_t from, 
>> size_t len,
>> +                               size_t *retlen, u_char *buf)
>> +{
>> +       return spi_nor_mtd_otp_read_write(mtd, from, len, retlen, buf, 
>> false);
>> +}
>> +
>> +static int spi_nor_mtd_otp_write(struct mtd_info *mtd, loff_t to, 
>> size_t len,
>> +                                size_t *retlen, u_char *buf)
>> +{
>> +       return spi_nor_mtd_otp_read_write(mtd, to, len, retlen, buf, 
>> true);
>> +}
>> +
>> +static int spi_nor_mtd_otp_lock(struct mtd_info *mtd, loff_t from, 
>> size_t len)
>> +{
>> +       struct spi_nor *nor = mtd_to_spi_nor(mtd);
>> +       const struct spi_nor_otp_ops *ops = spi_nor_otp_ops(nor);
>> +       unsigned int region;
>> +       int ret;
>> +
>> +       if (from < 0 || (from + len) > spi_nor_otp_size(nor))
>> +               return -EINVAL;
>> +
>> +       /* the user has to explicitly ask for whole regions */
>> +       if (len % spi_nor_otp_region_len(nor))
>> +               return -EINVAL;
>> +
>> +       if (from % spi_nor_otp_region_len(nor))
>> +               return -EINVAL;
>> +
>> +       ret = spi_nor_lock_and_prep(nor);
>> +       if (ret)
>> +               return ret;
>> +
>> +       while (len) {
>> +               region = spi_nor_otp_offset_to_region(nor, from);
>> +               ret = ops->lock(nor, region);
>> +               if (ret)
>> +                       goto out;
>> +
>> +               len -= spi_nor_otp_region_len(nor);
>> +               from += spi_nor_otp_region_len(nor);
>> +       }
>> +
>> +out:
>> +       spi_nor_unlock_and_unprep(nor);
>> +
>> +       return ret;
>> +}
>> +
>> +void spi_nor_otp_init(struct spi_nor *nor)
>> +{
>> +       struct mtd_info *mtd = &nor->mtd;
>> +
>> +       if (!nor->params->otp.ops)
>> +               return;
>> +
>> +       if (WARN_ON(!is_power_of_2(spi_nor_otp_region_len(nor))))
> 
> Why WARN_ON and not just a debug message?

Because it really is a programming error, but I don't want to use
BUG_ON(). That is, the developer has either used a OTP_INFO() wrong
or he wants to add a flash which isn't yet supported. I don't see
a reason to "hide" that behind a debug message. It just depends
on the static OTP configuration and can't change at runtime.

-michael

Re: [PATCH v4 3/4] mtd: spi-nor: winbond: add OTP support to w25q32fw/jw

[Michael Walle]

Am 2021-03-15 09:26, schrieb Tudor.Ambarus@microchip.com:
> On 3/6/21 2:05 AM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> With all the helper functions in place, add OTP support for the 
>> Winbond
>> W25Q32JW and W25Q32FW.
>> 
>> Both were tested on a LS1028A SoC with a NXP FSPI controller.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/winbond.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/winbond.c 
>> b/drivers/mtd/spi-nor/winbond.c
>> index e5dfa786f190..9a3f8ff007fd 100644
>> --- a/drivers/mtd/spi-nor/winbond.c
>> +++ b/drivers/mtd/spi-nor/winbond.c
>> @@ -55,14 +55,18 @@ static const struct flash_info winbond_parts[] = {
>>         { "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
>>         { "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64,
>>                            SECT_4K | SPI_NOR_DUAL_READ | 
>> SPI_NOR_QUAD_READ |
>> -                          SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>> +                          SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>> +                          OTP_INFO(256, 3, 0x1000, 0x1000)
>> +       },
>> +
>>         { "w25q32jv", INFO(0xef7016, 0, 64 * 1024,  64,
>>                            SECT_4K | SPI_NOR_DUAL_READ | 
>> SPI_NOR_QUAD_READ |
>>                            SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>         },
>>         { "w25q32jwm", INFO(0xef8016, 0, 64 * 1024,  64,
>>                             SECT_4K | SPI_NOR_DUAL_READ | 
>> SPI_NOR_QUAD_READ |
>> -                           SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>> +                           SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>> +                           OTP_INFO(256, 3, 0x1000, 0x1000) },
>>         { "w25q64jwm", INFO(0xef8017, 0, 64 * 1024, 128,
>>                             SECT_4K | SPI_NOR_DUAL_READ | 
>> SPI_NOR_QUAD_READ |
>>                             SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>> @@ -131,9 +135,18 @@ static int winbond_set_4byte_addr_mode(struct 
>> spi_nor *nor, bool enable)
>>         return spi_nor_write_disable(nor);
>>  }
>> 
>> +static const struct spi_nor_otp_ops winbond_otp_ops = {
>> +       .read = spi_nor_otp_read_secr,
>> +       .write = spi_nor_otp_write_secr,
>> +       .lock = spi_nor_otp_lock_sr2,
>> +       .is_locked = spi_nor_otp_is_locked_sr2,
>> +};
> 
> Should we have this in otp.c? It can be shared with gigadevice as well
> as far as I understood.

It should work in principle for both vendors, but I couldn't test it. So
for now, I've kept it private to winbond.c. If there will be OTP support
for other flashes with the same ops, it should be moved.

-michael

Re: [PATCH v4 3/4] mtd: spi-nor: winbond: add OTP support to w25q32fw/jw

[Tudor.Ambarus]

On 3/15/21 11:26 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2021-03-15 09:26, schrieb Tudor.Ambarus@microchip.com:
>> On 3/6/21 2:05 AM, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> With all the helper functions in place, add OTP support for the
>>> Winbond
>>> W25Q32JW and W25Q32FW.
>>>
>>> Both were tested on a LS1028A SoC with a NXP FSPI controller.
>>>
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>>> ---
>>>  drivers/mtd/spi-nor/winbond.c | 17 +++++++++++++++--
>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/winbond.c
>>> b/drivers/mtd/spi-nor/winbond.c
>>> index e5dfa786f190..9a3f8ff007fd 100644
>>> --- a/drivers/mtd/spi-nor/winbond.c
>>> +++ b/drivers/mtd/spi-nor/winbond.c
>>> @@ -55,14 +55,18 @@ static const struct flash_info winbond_parts[] = {
>>>         { "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
>>>         { "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64,
>>>                            SECT_4K | SPI_NOR_DUAL_READ |
>>> SPI_NOR_QUAD_READ |
>>> -                          SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>>> +                          SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>> +                          OTP_INFO(256, 3, 0x1000, 0x1000)
>>> +       },
>>> +
>>>         { "w25q32jv", INFO(0xef7016, 0, 64 * 1024,  64,
>>>                            SECT_4K | SPI_NOR_DUAL_READ |
>>> SPI_NOR_QUAD_READ |
>>>                            SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>>         },
>>>         { "w25q32jwm", INFO(0xef8016, 0, 64 * 1024,  64,
>>>                             SECT_4K | SPI_NOR_DUAL_READ |
>>> SPI_NOR_QUAD_READ |
>>> -                           SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>>> +                           SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
>>> +                           OTP_INFO(256, 3, 0x1000, 0x1000) },
>>>         { "w25q64jwm", INFO(0xef8017, 0, 64 * 1024, 128,
>>>                             SECT_4K | SPI_NOR_DUAL_READ |
>>> SPI_NOR_QUAD_READ |
>>>                             SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>>> @@ -131,9 +135,18 @@ static int winbond_set_4byte_addr_mode(struct
>>> spi_nor *nor, bool enable)
>>>         return spi_nor_write_disable(nor);
>>>  }
>>>
>>> +static const struct spi_nor_otp_ops winbond_otp_ops = {
>>> +       .read = spi_nor_otp_read_secr,
>>> +       .write = spi_nor_otp_write_secr,
>>> +       .lock = spi_nor_otp_lock_sr2,
>>> +       .is_locked = spi_nor_otp_is_locked_sr2,
>>> +};
>>
>> Should we have this in otp.c? It can be shared with gigadevice as well
>> as far as I understood.
> 
> It should work in principle for both vendors, but I couldn't test it. So
> for now, I've kept it private to winbond.c. If there will be OTP support
> for other flashes with the same ops, it should be moved.
> 

Ok.

Re: [PATCH v4 1/4] mtd: spi-nor: add OTP support

[Tudor.Ambarus]

On 3/15/21 11:23 AM, Michael Walle wrote:

cut

>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>> new file mode 100644
>>> index 000000000000..4e301fd5156b
>>> --- /dev/null
>>> +++ b/drivers/mtd/spi-nor/otp.c
>>> @@ -0,0 +1,218 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * OTP support for SPI NOR flashes
>>> + *
>>> + * Copyright (C) 2021 Michael Walle <michael@walle.cc>> + */
>>> +
>>> +#include <linux/log2.h>
>>> +#include <linux/mtd/mtd.h>
>>> +#include <linux/mtd/spi-nor.h>
>>> +
>>> +#include "core.h"
>>> +
>>> +#define spi_nor_otp_ops(nor) ((nor)->params->otp.ops)
>>> +#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
>>> +#define spi_nor_otp_n_regions(nor)
>>> ((nor)->params->otp.org->n_regions)
>>
>> I don't like these wrappers because they gratuiously hide what's really
>> there.
>> I find the code more easier to read without these wrappers, because I
>> don't
>> have to memorize what these wrappers are doing, and I better see how
>> the code
>> is organized.
> 
> TBH I find it easier on the eye and I've never seen so much dereferences
> as in mtd/spi-nor.

It's what I prefer, but it's not a hard requirement. Would you please check
for a second opinion with Vignesh? Inquire about the helpers too. Then do as
you find best.

Cheers,
ta

Re: [PATCH v4 1/4] mtd: spi-nor: add OTP support

[Tudor.Ambarus]

On 3/15/21 11:39 AM, Tudor Ambarus - M18064 wrote:
> On 3/15/21 11:23 AM, Michael Walle wrote:
> 
> cut
> 
>>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>>> new file mode 100644
>>>> index 000000000000..4e301fd5156b
>>>> --- /dev/null
>>>> +++ b/drivers/mtd/spi-nor/otp.c
>>>> @@ -0,0 +1,218 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * OTP support for SPI NOR flashes
>>>> + *
>>>> + * Copyright (C) 2021 Michael Walle <michael@walle.cc>> + */
>>>> +
>>>> +#include <linux/log2.h>
>>>> +#include <linux/mtd/mtd.h>
>>>> +#include <linux/mtd/spi-nor.h>
>>>> +
>>>> +#include "core.h"
>>>> +
>>>> +#define spi_nor_otp_ops(nor) ((nor)->params->otp.ops)
>>>> +#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
>>>> +#define spi_nor_otp_n_regions(nor)
>>>> ((nor)->params->otp.org->n_regions)
>>>
>>> I don't like these wrappers because they gratuiously hide what's really
>>> there.
>>> I find the code more easier to read without these wrappers, because I
>>> don't
>>> have to memorize what these wrappers are doing, and I better see how
>>> the code
>>> is organized.
>>
>> TBH I find it easier on the eye and I've never seen so much dereferences
>> as in mtd/spi-nor.

the dereferences will still be there, but will be just hidden by these wrappers,
don't they? Why would one prefer the wrappers?

> 
> It's what I prefer, but it's not a hard requirement. Would you please check
> for a second opinion with Vignesh? Inquire about the helpers too. Then do as
> you find best.
> 
> Cheers,
> ta
> 

Re: [PATCH v4 1/4] mtd: spi-nor: add OTP support

[Michael Walle]

Am 2021-03-15 10:44, schrieb Tudor.Ambarus@microchip.com:
> On 3/15/21 11:39 AM, Tudor Ambarus - M18064 wrote:
>> On 3/15/21 11:23 AM, Michael Walle wrote:
>> 
>> cut
>> 
>>>>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>>>>> new file mode 100644
>>>>> index 000000000000..4e301fd5156b
>>>>> --- /dev/null
>>>>> +++ b/drivers/mtd/spi-nor/otp.c
>>>>> @@ -0,0 +1,218 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * OTP support for SPI NOR flashes
>>>>> + *
>>>>> + * Copyright (C) 2021 Michael Walle <michael@walle.cc>> + */
>>>>> +
>>>>> +#include <linux/log2.h>
>>>>> +#include <linux/mtd/mtd.h>
>>>>> +#include <linux/mtd/spi-nor.h>
>>>>> +
>>>>> +#include "core.h"
>>>>> +
>>>>> +#define spi_nor_otp_ops(nor) ((nor)->params->otp.ops)
>>>>> +#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
>>>>> +#define spi_nor_otp_n_regions(nor)
>>>>> ((nor)->params->otp.org->n_regions)
>>>> 
>>>> I don't like these wrappers because they gratuiously hide what's 
>>>> really
>>>> there.
>>>> I find the code more easier to read without these wrappers, because 
>>>> I
>>>> don't
>>>> have to memorize what these wrappers are doing, and I better see how
>>>> the code
>>>> is organized.
>>> 
>>> TBH I find it easier on the eye and I've never seen so much 
>>> dereferences
>>> as in mtd/spi-nor.
> 
> the dereferences will still be there, but will be just hidden by these 
> wrappers,
> don't they? Why would one prefer the wrappers?

That's why I wrote "easier on the eye", yes that is subjective. But
there are also technical aspects, for example, the helpers all
operate on "struct nor*", so you don't have to use the
dereference chain or build up local variables. For example, see
spi_nor_otp_read_write(), it doesn't have to know
anything about the "struct spi_nor_otp_organization".

And of course you could change the actual implementation
without touching the code everywhere,.

-michael

Re: [PATCH v4 2/4] mtd: spi-nor: implement OTP support for Winbond and similar flashes

[Michael Walle]

Am 2021-03-15 09:20, schrieb Tudor.Ambarus@microchip.com:
> On 3/6/21 2:05 AM, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Use the new OTP ops to implement OTP access on Winbond flashes. Most
>> Winbond flashes provides up to four different OTP regions ("Security
>> Registers").
>> 
>> Winbond devices use a special opcode to read and write to the OTP
>> regions, just like the RDSFDP opcode. In fact, it seems that the
>> (undocumented) first OTP area of the newer flashes is the actual SFDP
>> table.
>> 
>> On a side note, Winbond devices also allow erasing the OTP regions as
>> long as the area isn't locked down.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  drivers/mtd/spi-nor/core.c  |   2 +-
>>  drivers/mtd/spi-nor/core.h  |   6 ++
>>  drivers/mtd/spi-nor/otp.c   | 164 
>> ++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spi-nor.h |   9 ++
>>  4 files changed, 180 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 0c5c757fa95b..ef7df26896f1 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -1034,7 +1034,7 @@ static int 
>> spi_nor_write_16bit_sr_and_check(struct spi_nor *nor, u8 sr1)
>>   *
>>   * Return: 0 on success, -errno otherwise.
>>   */
>> -static int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 
>> cr)
>> +int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr)
>>  {
>>         int ret;
>>         u8 *sr_cr = nor->bouncebuf;
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index ec8da1243846..dfbf6ba42b57 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -496,6 +496,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
>>  int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
>>  int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
>>  int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
>> +int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);
>> 
>>  int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
>>  ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t 
>> len,
>> @@ -503,6 +504,11 @@ ssize_t spi_nor_read_data(struct spi_nor *nor, 
>> loff_t from, size_t len,
>>  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t 
>> len,
>>                            const u8 *buf);
>> 
>> +int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t 
>> len, u8 *buf);
>> +int spi_nor_otp_write_secr(struct spi_nor *nor, loff_t addr, size_t 
>> len, u8 *buf);
>> +int spi_nor_otp_lock_sr2(struct spi_nor *nor, unsigned int region);
>> +int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int 
>> region);
>> +
>>  int spi_nor_hwcaps_read2cmd(u32 hwcaps);
>>  u8 spi_nor_convert_3to4_read(u8 opcode);
>>  void spi_nor_set_read_settings(struct spi_nor_read_command *read,
>> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
>> index 4e301fd5156b..4e8da9108c77 100644
>> --- a/drivers/mtd/spi-nor/otp.c
>> +++ b/drivers/mtd/spi-nor/otp.c
>> @@ -15,6 +15,170 @@
>>  #define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
>>  #define spi_nor_otp_n_regions(nor) 
>> ((nor)->params->otp.org->n_regions)
>> 
>> +/**
>> + * spi_nor_otp_read_secr() - read OTP data
>> + * @nor:       pointer to 'struct spi_nor'
>> + * @from:       offset to read from
>> + * @len:        number of bytes to read
>> + * @buf:        pointer to dst buffer
> 
> is buf DMA-able?

That's actually the same description as spi_nor_read_data().
Looks like the spimem will provide a DMA-able buffer on
the fly if necessary. I'm not sure about the the
spi_nor_controller_ops.

>> + *
>> + * Read OTP data from one region by using the SPINOR_OP_RSECR 
>> commands. This
>> + * method is used on GigaDevice and Winbond flashes.
>> + *
>> + * Return: number of bytes read successfully, -errno otherwise
>> + */
>> +int spi_nor_otp_read_secr(struct spi_nor *nor, loff_t addr, size_t 
>> len, u8 *buf)
>> +{
>> +       u8 addr_width, read_opcode, read_dummy;
>> +       struct spi_mem_dirmap_desc *rdesc;
>> +       enum spi_nor_protocol read_proto;
>> +       int ret;
>> +
>> +       read_opcode = nor->read_opcode;
>> +       addr_width = nor->addr_width;
>> +       read_dummy = nor->read_dummy;
>> +       read_proto = nor->read_proto;
>> +       rdesc = nor->dirmap.rdesc;
>> +
>> +       nor->read_opcode = SPINOR_OP_RSECR;
>> +       nor->addr_width = 3;
>> +       nor->read_dummy = 8;
>> +       nor->read_proto = SNOR_PROTO_1_1_1;
> 
> any winbond/gigadevice flashes with octal dtr support? Do they
> provide SEC Register opcodes for octal dtr?

AFAIK there are no winbond flashes with 8 bit I/O. There are
4bit/DTR modes, but not for the security registers.

I don't know what you had in mind. But I don't think it is
worth to read the 3x 256byte data faster than single bit I/O.

-michael

Re: [PATCH v4 2/4] mtd: spi-nor: implement OTP support for Winbond and similar flashes

[Michael Walle]

Am 2021-03-15 09:31, schrieb Tudor.Ambarus@microchip.com:
> On 3/6/21 2:05 AM, Michael Walle wrote:
>> +       nor->dirmap.rdesc = NULL;
> 
> why can't we use dirmap?

Dirmap is used if the controller supports (transparent)
memory mapped access, right?

As you see I'm not familiar with that, nor does my
controller has support for it, well at least the driver,
the controller supports it actually.

But it also seems like how the flash is accessed is
set up in

   spi_nor_probe()
     spi_nor_create_read_dirmap()

And because the read opcode has to be changed, that isn't
possible.

Plese correct me if I'm wrong.

-michael