Re: [PATCH v4 1/4] mtd: spi-nor: add OTP support
From: Michael Walle
Date: Mon Mar 15 2021 - 05:24:18 EST
Am 2021-03-15 08:28, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
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@xxxxxxxx>
---
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@xxxxxxxx>> + */
+
+#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