Re: [PATCH v3 08/13] misc: Add support for nvmem cells

[Simon Glass <sjg@chromium.org>]

Hi Sean,

On Mon, 18 Apr 2022 at 13:37, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This adds support for "nvmem cells" as seen in Linux. The nvmem device
> class in Linux is used for various assorted ROMs and EEPROMs. In this
> sense, it is similar to UCLASS_MISC, but also includes
> UCLASS_I2C_EEPROM, UCLASS_RTC, and UCLASS_MTD. New drivers corresponding
> to a Linux-style nvmem device should be implemented as one of the
> previously-mentioned uclasses. The nvmem API acts as a compatibility
> layer to adapt the (slightly different) APIs of these uclasses. It also
> handles the lookup of nvmem cells.
>
> While nvmem devices can be accessed directly, they are most often used
> by reading/writing contiguous values called "cells". Cells typically
> hold information like calibration, versions, or configuration (such as
> mac addresses).
>
> nvmem devices can specify "cells" in their device tree:
>
>         qfprom: eeprom@700000 {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 reg = <0x00700000 0x100000>;
>
>                 /* ... */
>
>                 tsens_calibration: calib@404 {
>                         reg = <0x404 0x10>;
>                 };
>         };
>
> which can then be referenced like:
>
>         tsens {
>                 /* ... */
>                 nvmem-cells = <&tsens_calibration>;
>                 nvmem-cell-names = "calibration";
>         };
>
> The tsens driver could then read the calibration value like:
>
>         struct nvmem_cell cal_cell;
>         u8 cal[16];
>         nvmem_cell_get_by_name(dev, "calibration", &cal_cell);
>         nvmem_cell_read(&cal_cell, cal, sizeof(cal));
>
> Because nvmem devices are not all of the same uclass, supported uclasses
> must register a nvmem_interface struct. This allows CONFIG_NVMEM to be
> enabled without depending on specific uclasses. At the moment,
> nvmem_interface is very bare-bones, and assumes that no initialization
> is necessary. However, this could be amended in the future.
>
> Although I2C_EEPROM and MISC are quite similar (and could likely be
> unified), they present different read/write function signatures. To
> abstract over this, NVMEM uses the same read/write signature as Linux.
> In particular, short read/writes are not allowed, which is allowed by
> MISC.
>
> The functionality implemented by nvmem cells is very similar to that
> provided by i2c_eeprom_partition. "fixed-partition"s for eeproms does
> not seem to have made its way into Linux or into any device tree other
> than sandbox. It is possible that with the introduction of this API it
> would be possible to remove it.

I still think this would be better as a separate uclass, with child
devices created at bind time in each of the respective uclasses, like
mmc_bind() does. Then you will see the nvmem devices in the DM tree.
Wouldn't we want to add a command to access the nvmem devices? This
patch feels like a shortcut to me and I'm not sure of the benefit of
that shortcut.

>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Call the appropriate API functions directly from
>   nvmem_cell_(read|write). This means we can drop the nvmem_interface
>   machinery.
>
>  MAINTAINERS           |   7 ++
>  doc/api/index.rst     |   1 +
>  doc/api/nvmem.rst     |   7 ++
>  drivers/misc/Kconfig  |  16 +++++
>  drivers/misc/Makefile |   1 +
>  drivers/misc/nvmem.c  | 142 +++++++++++++++++++++++++++++++++++++++
>  include/nvmem.h       | 151 ++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 325 insertions(+)
>  create mode 100644 doc/api/nvmem.rst
>  create mode 100644 drivers/misc/nvmem.c
>  create mode 100644 include/nvmem.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 34446127d4..5175607de2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1064,6 +1064,13 @@ F:       cmd/nvme.c
>  F:     include/nvme.h
>  F:     doc/develop/driver-model/nvme.rst
>
> +NVMEM
> +M:     Sean Anderson <seanga2@gmail.com>
> +S:     Maintained
> +F:     doc/api/nvmem.rst
> +F:     drivers/misc/nvmem.c
> +F:     include/nvmem.h
> +
>  NXP C45 TJA11XX PHY DRIVER
>  M:     Radu Pirea <radu-nicolae.pirea@oss.nxp.com>
>  S:     Maintained
> diff --git a/doc/api/index.rst b/doc/api/index.rst
> index 72fea981b7..a9338cfef9 100644
> --- a/doc/api/index.rst
> +++ b/doc/api/index.rst
> @@ -14,6 +14,7 @@ U-Boot API documentation
>     linker_lists
>     lmb
>     logging
> +   nvmem
>     pinctrl
>     rng
>     sandbox
> diff --git a/doc/api/nvmem.rst b/doc/api/nvmem.rst
> new file mode 100644
> index 0000000000..15c9b5b839
> --- /dev/null
> +++ b/doc/api/nvmem.rst
> @@ -0,0 +1,7 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +NVMEM API
> +=========
> +
> +.. kernel-doc:: include/nvmem.h
> +   :internal:
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 10fd601278..218dc18a1d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -31,6 +31,22 @@ config TPL_MISC
>           set of generic read, write and ioctl methods may be used to
>           access the device.
>
> +config NVMEM
> +       bool "NVMEM support"
> +       help
> +         This adds support for a common interface to different types of
> +         non-volatile memory. Consumers can use nvmem-cells properties to look
> +         up hardware configuration data such as MAC addresses and calibration
> +         settings.
> +
> +config SPL_NVMEM
> +       bool "NVMEM support in SPL"
> +       help
> +         This adds support for a common interface to different types of
> +         non-volatile memory. Consumers can use nvmem-cells properties to look
> +         up hardware configuration data such as MAC addresses and calibration
> +         settings.
> +
>  config ALTERA_SYSID
>         bool "Altera Sysid support"
>         depends on MISC
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 6150d01e88..03fad7a23f 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -4,6 +4,7 @@
>  # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
>
>  obj-$(CONFIG_MISC) += misc-uclass.o
> +obj-$(CONFIG_$(SPL_TPL_)NVMEM) += nvmem.o
>
>  obj-$(CONFIG_$(SPL_TPL_)CROS_EC) += cros_ec.o
>  obj-$(CONFIG_$(SPL_TPL_)CROS_EC_SANDBOX) += cros_ec_sandbox.o
> diff --git a/drivers/misc/nvmem.c b/drivers/misc/nvmem.c
> new file mode 100644
> index 0000000000..fd80a72394
> --- /dev/null
> +++ b/drivers/misc/nvmem.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
> + */
> +
> +#include <common.h>
> +#include <i2c_eeprom.h>
> +#include <linker_lists.h>
> +#include <misc.h>
> +#include <nvmem.h>
> +#include <rtc.h>
> +#include <dm/device_compat.h>
> +#include <dm/ofnode.h>
> +#include <dm/read.h>
> +#include <dm/uclass.h>
> +
> +int nvmem_cell_read(struct nvmem_cell *cell, void *buf, size_t size)
> +{
> +       dev_dbg(cell->nvmem, "%s: off=%u size=%zu\n", __func__, cell->offset, size);
> +       if (size != cell->size)
> +               return -EINVAL;
> +
> +       switch (cell->nvmem->driver->id) {
> +       case UCLASS_I2C_EEPROM:
> +               return i2c_eeprom_read(cell->nvmem, cell->offset, buf, size);
> +       case UCLASS_MISC: {
> +               int ret = misc_read(cell->nvmem, cell->offset, buf, size);
> +
> +               if (ret < 0)
> +                       return ret;
> +               if (ret != size)
> +                       return -EIO;
> +               return 0;
> +       }
> +       case UCLASS_RTC:
> +               return dm_rtc_read(cell->nvmem, cell->offset, buf, size);
> +       default:
> +               return -ENOSYS;
> +       }
> +}
> +
> +int nvmem_cell_write(struct nvmem_cell *cell, const void *buf, size_t size)
> +{
> +       dev_dbg(cell->nvmem, "%s: off=%u size=%zu\n", __func__, cell->offset, size);
> +       if (size != cell->size)
> +               return -EINVAL;
> +
> +       switch (cell->nvmem->driver->id) {
> +       case UCLASS_I2C_EEPROM:
> +               return i2c_eeprom_write(cell->nvmem, cell->offset, buf, size);
> +       case UCLASS_MISC: {
> +               int ret = misc_write(cell->nvmem, cell->offset, buf, size);
> +
> +               if (ret < 0)
> +                       return ret;
> +               if (ret != size)
> +                       return -EIO;
> +               return 0;
> +       }
> +       case UCLASS_RTC:
> +               return dm_rtc_write(cell->nvmem, cell->offset, buf, size);
> +       default:
> +               return -ENOSYS;
> +       }
> +}
> +
> +/**
> + * nvmem_get_device() - Get an nvmem device for a cell
> + * @node: ofnode of the nvmem device
> + * @cell: Cell to look up
> + *
> + * Try to find a nvmem-compatible device by going through the nvmem interfaces.
> + *
> + * Return:
> + * * 0 on success
> + * * -ENODEV if we didn't find anything
> + * * A negative error if there was a problem looking up the device
> + */
> +static int nvmem_get_device(ofnode node, struct nvmem_cell *cell)
> +{
> +       int i, ret;
> +       enum uclass_id ids[] = {
> +               UCLASS_I2C_EEPROM,
> +               UCLASS_MISC,
> +               UCLASS_RTC,
> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(ids); i++) {
> +               ret = uclass_get_device_by_ofnode(ids[i], node, &cell->nvmem);
> +               if (!ret)
> +                       return 0;
> +               if (ret != -ENODEV)
> +                       return ret;
> +       }
> +
> +       return -ENODEV;
> +}
> +
> +int nvmem_cell_get_by_index(struct udevice *dev, int index,
> +                           struct nvmem_cell *cell)
> +{
> +       fdt_addr_t offset;
> +       fdt_size_t size = FDT_SIZE_T_NONE;
> +       int ret;
> +       struct ofnode_phandle_args args;
> +
> +       dev_dbg(dev, "%s: index=%d\n", __func__, index);
> +
> +       ret = dev_read_phandle_with_args(dev, "nvmem-cells", NULL, 0, index,
> +                                        &args);
> +       if (ret)
> +               return ret;
> +
> +       ret = nvmem_get_device(ofnode_get_parent(args.node), cell);
> +       if (ret)
> +               return ret;
> +
> +       offset = ofnode_get_addr_size_index_notrans(args.node, 0, &size);
> +       if (offset == FDT_ADDR_T_NONE || size == FDT_SIZE_T_NONE) {
> +               dev_dbg(cell->nvmem, "missing address or size for %s\n",
> +                       ofnode_get_name(args.node));
> +               return -EINVAL;
> +       }
> +
> +       cell->offset = offset;
> +       cell->size = size;
> +       return 0;
> +}
> +
> +int nvmem_cell_get_by_name(struct udevice *dev, const char *name,
> +                          struct nvmem_cell *cell)
> +{
> +       int index;
> +
> +       dev_dbg(dev, "%s, name=%s\n", __func__, name);
> +
> +       index = dev_read_stringlist_search(dev, "nvmem-cell-names", name);
> +       if (index < 0)
> +               return index;
> +
> +       return nvmem_cell_get_by_index(dev, index, cell);
> +}
> diff --git a/include/nvmem.h b/include/nvmem.h
> new file mode 100644
> index 0000000000..2751713a68
> --- /dev/null
> +++ b/include/nvmem.h
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2022 Sean Anderson <sean.anderson@seco.com>
> + */
> +
> +#ifndef NVMEM_H
> +#define NVMEM_H
> +
> +/**
> + * typedef nvmem_reg_read_t - Read a register from an nvmem device
> + *
> + * @dev: The device to read from
> + * @offset: The offset of the register from the beginning of @dev
> + * @buf: The buffer to read into
> + * @size: The size of @buf, in bytes
> + *
> + * Return:
> + * * 0 on success
> + * * A negative error on failure
> + */
> +typedef int (*nvmem_reg_read_t)(struct udevice *dev, unsigned int offset,
> +                               void *buf, size_t size);
> +
> +/**
> + * typedef nvmem_reg_write_t - Write a register to an nvmem device
> + * @dev: The device to write
> + * @offset: The offset of the register from the beginning of @dev
> + * @buf: The buffer to write
> + * @size: The size of @buf, in bytes
> + *
> + * Return:
> + * * 0 on success
> + * * -ENOSYS if the device is read-only
> + * * A negative error on other failures
> + */
> +typedef int (*nvmem_reg_write_t)(struct udevice *dev, unsigned int offset,
> +                                const void *buf, size_t size);
> +
> +/**
> + * struct nvmem_cell - One datum within non-volatile memory
> + * @nvmem: The backing storage device
> + * @offset: The offset of the cell from the start of @nvmem
> + * @size: The size of the cell, in bytes
> + */
> +struct nvmem_cell {
> +       struct udevice *nvmem;
> +       unsigned int offset;
> +       size_t size;
> +};
> +
> +struct udevice;

nit: can you put this at the top of the file, after any #define stuff
but before declarations?

> +
> +#if CONFIG_IS_ENABLED(NVMEM)
> +
> +/**
> + * nvmem_cell_read() - Read the value of an nvmem cell
> + * @cell: The nvmem cell to read
> + * @buf: The buffer to read into
> + * @size: The size of @buf
> + *
> + * Return:
> + * * 0 on success
> + * * -EINVAL if @buf is not the same size as @cell.
> + * * -ENOSYS if CONFIG_NVMEM is disabled
> + * * A negative error if there was a problem reading the underlying storage
> + */
> +int nvmem_cell_read(struct nvmem_cell *cell, void *buf, size_t size);
> +
> +/**
> + * nvmem_cell_write() - Write a value to an nvmem cell
> + * @cell: The nvmem cell to write
> + * @buf: The buffer to write from
> + * @size: The size of @buf
> + *
> + * Return:
> + * * 0 on success
> + * * -EINVAL if @buf is not the same size as @cell
> + * * -ENOSYS if @cell is read-only, or if CONFIG_NVMEM is disabled
> + * * A negative error if there was a problem writing the underlying storage
> + */
> +int nvmem_cell_write(struct nvmem_cell *cell, const void *buf, size_t size);
> +
> +/**
> + * nvmem_cell_get_by_index() - Get an nvmem cell from a given device and index
> + * @dev: The device that uses the nvmem cell
> + * @index: The index of the cell in nvmem-cells
> + * @cell: The cell to initialize
> + *
> + * Look up the nvmem cell referenced by the phandle at @index in nvmem-cells in
> + * @dev.
> + *
> + * Return:
> + * * 0 on success
> + * * -EINVAL if the regs property is missing, empty, or undersized
> + * * -ENODEV if the nvmem device is missing or unimplemented
> + * * -ENOSYS if CONFIG_NVMEM is disabled
> + * * A negative error if there was a problem reading nvmem-cells or getting the
> + *   device
> + */
> +int nvmem_cell_get_by_index(struct udevice *dev, int index,
> +                           struct nvmem_cell *cell);
> +
> +/**
> + * nvmem_cell_get_by_name() - Get an nvmem cell from a given device and name
> + * @dev: The device that uses the nvmem cell
> + * @name: The name of the nvmem cell
> + * @cell: The cell to initialize
> + *
> + * Look up the nvmem cell referenced by @name in the nvmem-cell-names property
> + * of @dev.
> + *
> + * Return:
> + * * 0 on success
> + * * -EINVAL if the regs property is missing, empty, or undersized
> + * * -ENODEV if the nvmem device is missing or unimplemented
> + * * -ENODATA if @name is not in nvmem-cell-names
> + * * -ENOSYS if CONFIG_NVMEM is disabled
> + * * A negative error if there was a problem reading nvmem-cell-names,
> + *   nvmem-cells, or getting the device
> + */
> +int nvmem_cell_get_by_name(struct udevice *dev, const char *name,
> +                          struct nvmem_cell *cell);
> +
> +#else /* CONFIG_NVMEM */
> +
> +static inline int nvmem_cell_read(struct nvmem_cell *cell, void *buf, int size)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline int nvmem_cell_write(struct nvmem_cell *cell, const void *buf,
> +                                  int size)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline int nvmem_cell_get_by_index(struct udevice *dev, int index,
> +                                         struct nvmem_cell *cell)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline int nvmem_cell_get_by_name(struct udevice *dev, const char *name,
> +                                        struct nvmem_cell *cell)
> +{
> +       return -ENOSYS;
> +}
> +
> +#endif /* CONFIG_NVMEM */
> +
> +#endif /* NVMEM_H */
> --
> 2.35.1.1320.gc452695387.dirty
>

Regards,
Simon