[PATCH v4 1/4] mtd: spi-nor: add OTP support
Michael Walle
michael at walle.cc
Mon Mar 15 09:23:24 GMT 2021
Am 2021-03-15 08:28, schrieb Tudor.Ambarus at 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 at 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 at 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
More information about the linux-mtd
mailing list