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

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

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