Hi Sean, On Mon, 18 Apr 2022 at 13:37, Sean Anderson <[email protected]> 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 <[email protected]> > --- > > (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 <[email protected]> > +S: Maintained > +F: doc/api/nvmem.rst > +F: drivers/misc/nvmem.c > +F: include/nvmem.h > + > NXP C45 TJA11XX PHY DRIVER > M: Radu Pirea <[email protected]> > 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, [email protected]. > > 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 <[email protected]> > + */ > + > +#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 <[email protected]> > + */ > + > +#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
