[PATCH 00/14] Add support for NVMEM API

[PATCH 00/14] Add support for NVMEM API

[Sean Anderson]

This adds support for the nvmem-cells properties cropping up in manyb
device trees. This is an easy way to load configuration, version
information, or calibration data from a non-volatile memory source. For
more information, refer to patch 6 ("misc: Add support for nvmem
cells").

Should this be implemented on top of/as part of syscon/regmap? In Linux,
all nvmem devices must also implement regmap support, making nvmem a
subset of regmap drivers. There are certainly similarities, but I don't
know if we want to make this depend on regmap.

For the moment I have only added some integration tests using the
ethernet addresses. This hits the main code paths (looking up nvmem
cells) but doesn't test writing. I can add a few stand-alone tests if
desired.

The patches are structured in the following manner:
1-5:   These are general cleanups, and may be applied independently of
       the rest of the series.
6-10:  Add NVMEM support
11-14: Support reading ethernet addresses using the NVMEM API and add
       some tests.


Sean Anderson (14):
  sandbox: net: Remove fake-host-hwaddr
  sandbox: Remove eth2addr from environment
  test: eth: Add test for ethernet addresses
  sandbox: Move some mac addresses to device tree
  misc: i2c_eeprom: Make i2c_eeprom_write use a const buf
  misc: Add support for nvmem cells
  sandbox: Enable NVMEM
  rtc: Implement nvmem interface
  misc: i2c_eeprom: Implement nvmem interface
  misc: Implement nvmem interface
  net: Add support for reading mac addresses from nvmem cells
  test: Load mac address with i2c eeprom
  test: Load mac address using RTC
  test: Load mac address using misc device

 MAINTAINERS                        |   7 ++
 arch/sandbox/dts/sandbox.dts       |   1 -
 arch/sandbox/dts/sandbox64.dts     |   1 -
 arch/sandbox/dts/test.dts          |  29 ++++-
 board/sandbox/sandbox.env          |   4 -
 configs/sandbox64_defconfig        |   1 +
 configs/sandbox_defconfig          |   1 +
 configs/sandbox_flattree_defconfig |   1 +
 configs/sandbox_noinst_defconfig   |   1 +
 configs/sandbox_spl_defconfig      |   1 +
 doc/api/index.rst                  |   1 +
 doc/api/nvmem.rst                  |   7 ++
 drivers/misc/Kconfig               |  16 +++
 drivers/misc/Makefile              |   1 +
 drivers/misc/i2c_eeprom.c          |  22 +++-
 drivers/misc/i2c_eeprom_emul.c     |   4 +
 drivers/misc/misc-uclass.c         |  31 +++++
 drivers/misc/misc_sandbox.c        |   3 +
 drivers/misc/nvmem.c               | 109 +++++++++++++++++
 drivers/net/sandbox.c              |  10 +-
 drivers/rtc/i2c_rtc_emul.c         |  10 ++
 drivers/rtc/rtc-uclass.c           |  19 +++
 include/i2c_eeprom.h               |   3 +-
 include/nvmem.h                    | 185 +++++++++++++++++++++++++++++
 net/eth-uclass.c                   |  13 +-
 test/dm/eth.c                      |  28 +++++
 26 files changed, 484 insertions(+), 25 deletions(-)
 create mode 100644 doc/api/nvmem.rst
 create mode 100644 drivers/misc/nvmem.c
 create mode 100644 include/nvmem.h

-- 
2.25.1

[PATCH 01/14] sandbox: net: Remove fake-host-hwaddr

[Sean Anderson]

Instead of reading a pseudo-rom mac address from the device tree, just use
whatever we get from write_hwaddr. This has the effect of using the mac
address from the environment (or from the device tree, if it is
specified).

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 arch/sandbox/dts/sandbox.dts   |  1 -
 arch/sandbox/dts/sandbox64.dts |  1 -
 arch/sandbox/dts/test.dts      |  5 -----
 drivers/net/sandbox.c          | 10 ++--------
 4 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
index 127f168f02..840b8b503f 100644
--- a/arch/sandbox/dts/sandbox.dts
+++ b/arch/sandbox/dts/sandbox.dts
@@ -63,7 +63,6 @@
 	eth@10002000 {
 		compatible = "sandbox,eth";
 		reg = <0x10002000 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 00];
 	};
 
 	i2c_0: i2c@0 {
diff --git a/arch/sandbox/dts/sandbox64.dts b/arch/sandbox/dts/sandbox64.dts
index ec53106af9..3eb0457089 100644
--- a/arch/sandbox/dts/sandbox64.dts
+++ b/arch/sandbox/dts/sandbox64.dts
@@ -58,7 +58,6 @@
 	eth@10002000 {
 		compatible = "sandbox,eth";
 		reg = <0x0 0x10002000 0x0 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 00];
 	};
 
 	i2c_0: i2c@0 {
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 48ca3e1e47..5f3332a417 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -506,31 +506,26 @@
 	eth@10002000 {
 		compatible = "sandbox,eth";
 		reg = <0x10002000 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 00];
 	};
 
 	eth_5: eth@10003000 {
 		compatible = "sandbox,eth";
 		reg = <0x10003000 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 11];
 	};
 
 	eth_3: sbe5 {
 		compatible = "sandbox,eth";
 		reg = <0x10005000 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 33];
 	};
 
 	eth@10004000 {
 		compatible = "sandbox,eth";
 		reg = <0x10004000 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 22];
 	};
 
 	dsa_eth0: dsa-test-eth {
 		compatible = "sandbox,eth";
 		reg = <0x10006000 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 66];
 	};
 
 	dsa-test {
diff --git a/drivers/net/sandbox.c b/drivers/net/sandbox.c
index 37459dfa0a..13022addb6 100644
--- a/drivers/net/sandbox.c
+++ b/drivers/net/sandbox.c
@@ -395,9 +395,11 @@ static void sb_eth_stop(struct udevice *dev)
 static int sb_eth_write_hwaddr(struct udevice *dev)
 {
 	struct eth_pdata *pdata = dev_get_plat(dev);
+	struct eth_sandbox_priv *priv = dev_get_priv(dev);
 
 	debug("eth_sandbox %s: Write HW ADDR - %pM\n", dev->name,
 	      pdata->enetaddr);
+	memcpy(priv->fake_host_hwaddr, pdata->enetaddr, ARP_HLEN);
 	return 0;
 }
 
@@ -419,16 +421,8 @@ static int sb_eth_of_to_plat(struct udevice *dev)
 {
 	struct eth_pdata *pdata = dev_get_plat(dev);
 	struct eth_sandbox_priv *priv = dev_get_priv(dev);
-	const u8 *mac;
 
 	pdata->iobase = dev_read_addr(dev);
-
-	mac = dev_read_u8_array_ptr(dev, "fake-host-hwaddr", ARP_HLEN);
-	if (!mac) {
-		printf("'fake-host-hwaddr' is missing from the DT\n");
-		return -EINVAL;
-	}
-	memcpy(priv->fake_host_hwaddr, mac, ARP_HLEN);
 	priv->disabled = false;
 	priv->tx_handler = sb_default_handler;
 
-- 
2.25.1

[PATCH 02/14] sandbox: Remove eth2addr from environment

[Sean Anderson]

DSA interfaces use the same mac address for each interface, unless
instructed otherwise. Just set eth4addr and let eth2addr and eth7addr be
set automatically.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 board/sandbox/sandbox.env | 1 -
 1 file changed, 1 deletion(-)

diff --git a/board/sandbox/sandbox.env b/board/sandbox/sandbox.env
index b4c04635a4..3ff42a9cc1 100644
--- a/board/sandbox/sandbox.env
+++ b/board/sandbox/sandbox.env
@@ -6,7 +6,6 @@ stdout=serial,vidconsole
 stderr=serial,vidconsole
 
 ethaddr=02:00:11:22:33:44
-eth2addr=02:00:11:22:33:48
 eth3addr=02:00:11:22:33:45
 eth4addr=02:00:11:22:33:48
 eth5addr=02:00:11:22:33:46
-- 
2.25.1

[PATCH 03/14] test: eth: Add test for ethernet addresses

[Sean Anderson]

This adds a test to make sure that all the ethernet interfaces have
their addresses read properly. At the moment everything is read from the
environment, but the next few commits will add additional sources.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 test/dm/eth.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/test/dm/eth.c b/test/dm/eth.c
index e4ee695610..0c786c0f9d 100644
--- a/test/dm/eth.c
+++ b/test/dm/eth.c
@@ -147,6 +147,34 @@ static int dm_test_eth_act(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_eth_act, UT_TESTF_SCAN_FDT);
 
+/* Ensure that all addresses are loaded properly */
+static int dm_test_ethaddr(struct unit_test_state *uts)
+{
+	const char *addr[] = {
+		"02:00:11:22:33:44",
+		"02:00:11:22:33:48", /* dsa slave */
+		"02:00:11:22:33:45",
+		"02:00:11:22:33:48", /* dsa master */
+		"02:00:11:22:33:46",
+		"02:00:11:22:33:47",
+		"02:00:11:22:33:48", /* dsa slave */
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(addr); i++) {
+		char addrname[10];
+
+		if (i)
+			snprintf(addrname, sizeof(addrname), "eth%daddr", i + 1);
+		else
+			strcpy(addrname, "ethaddr");
+		ut_asserteq_str(addr[i], env_get(addrname));
+	}
+
+	return 0;
+}
+DM_TEST(dm_test_ethaddr, UT_TESTF_SCAN_FDT);
+
 /* The asserts include a return on fail; cleanup in the caller */
 static int _dm_test_eth_rotate1(struct unit_test_state *uts)
 {
-- 
2.25.1

[PATCH 04/14] sandbox: Move some mac addresses to device tree

[Sean Anderson]

This prevents some conflicts when running sandbox with -D, since the
"rom" mac address will be random and won't match the environment. We
still need to keep addresses for eth1 and eth6 in the environment,
because dm_test_eth_rotate expects to be able to disable them by
removing their envaddr variables. This can likely be fixed in a future
series by adding a function to cause sandbox eth_opts callback for a
particular mac to fail immediately.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 arch/sandbox/dts/test.dts | 3 +++
 board/sandbox/sandbox.env | 3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 5f3332a417..e111a1f050 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -511,11 +511,13 @@
 	eth_5: eth@10003000 {
 		compatible = "sandbox,eth";
 		reg = <0x10003000 0x1000>;
+		mac-address = [ 02 00 11 22 33 46 ];
 	};
 
 	eth_3: sbe5 {
 		compatible = "sandbox,eth";
 		reg = <0x10005000 0x1000>;
+		mac-address = [ 02 00 11 22 33 45 ];
 	};
 
 	eth@10004000 {
@@ -526,6 +528,7 @@
 	dsa_eth0: dsa-test-eth {
 		compatible = "sandbox,eth";
 		reg = <0x10006000 0x1000>;
+		mac-address = [ 02 00 11 22 33 48 ];
 	};
 
 	dsa-test {
diff --git a/board/sandbox/sandbox.env b/board/sandbox/sandbox.env
index 3ff42a9cc1..a2c19702d6 100644
--- a/board/sandbox/sandbox.env
+++ b/board/sandbox/sandbox.env
@@ -6,9 +6,6 @@ stdout=serial,vidconsole
 stderr=serial,vidconsole
 
 ethaddr=02:00:11:22:33:44
-eth3addr=02:00:11:22:33:45
-eth4addr=02:00:11:22:33:48
-eth5addr=02:00:11:22:33:46
 eth6addr=02:00:11:22:33:47
 ipaddr=192.0.2.1
 
-- 
2.25.1

[PATCH 05/14] misc: i2c_eeprom: Make i2c_eeprom_write use a const buf

[Sean Anderson]

i2c_eeprom_ops->write uses a const buf, so use one for the wrapper
function as well.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/misc/i2c_eeprom.c | 3 ++-
 include/i2c_eeprom.h      | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
index 89a450d0f8..4302e180ac 100644
--- a/drivers/misc/i2c_eeprom.c
+++ b/drivers/misc/i2c_eeprom.c
@@ -33,7 +33,8 @@ int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf, int size)
 	return ops->read(dev, offset, buf, size);
 }
 
-int i2c_eeprom_write(struct udevice *dev, int offset, uint8_t *buf, int size)
+int i2c_eeprom_write(struct udevice *dev, int offset, const uint8_t *buf,
+		     int size)
 {
 	const struct i2c_eeprom_ops *ops = device_get_ops(dev);
 
diff --git a/include/i2c_eeprom.h b/include/i2c_eeprom.h
index 3ad565684f..90fdb25232 100644
--- a/include/i2c_eeprom.h
+++ b/include/i2c_eeprom.h
@@ -42,7 +42,8 @@ int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf, int size);
  *
  * Return: 0 on success, -ve on failure
  */
-int i2c_eeprom_write(struct udevice *dev, int offset, uint8_t *buf, int size);
+int i2c_eeprom_write(struct udevice *dev, int offset, const uint8_t *buf,
+		     int size);
 
 /*
  * i2c_eeprom_size() - get size of I2C EEPROM chip
-- 
2.25.1

[PATCH 06/14] misc: Add support for nvmem cells

[Sean Anderson]

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

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 MAINTAINERS           |   7 ++
 doc/api/index.rst     |   1 +
 doc/api/nvmem.rst     |   7 ++
 drivers/misc/Kconfig  |  16 ++++
 drivers/misc/Makefile |   1 +
 drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
 include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 326 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 dcdd99e368..69de781cdc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1007,6 +1007,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 3f36174167..b92de3cf46 100644
--- a/doc/api/index.rst
+++ b/doc/api/index.rst
@@ -13,6 +13,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 a8baaeaf5c..a89e457223 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 f9826d2462..fd88ce4181 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..e08ae00a87
--- /dev/null
+++ b/drivers/misc/nvmem.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com>
+ */
+
+#include <common.h>
+#include <linker_lists.h>
+#include <nvmem.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;
+
+	return cell->read(cell->nvmem, cell->offset, buf, size);
+}
+
+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;
+
+	return cell->write(cell->nvmem, cell->offset, buf, size);
+}
+
+/**
+ * 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 ret;
+	struct nvmem_interface *iface;
+
+	for (iface = ll_entry_start(struct nvmem_interface, nvmem_interface);
+	     iface != ll_entry_end(struct nvmem_interface, nvmem_interface);
+	     iface++) {
+		ret = uclass_get_device_by_ofnode(iface->id, node,
+						  &cell->nvmem);
+		if (!ret) {
+			cell->read = iface->read;
+			cell->write = iface->write;
+			return 0;
+		} else 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..d9416979b4
--- /dev/null
+++ b/include/nvmem.h
@@ -0,0 +1,185 @@
+/* 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
+ * @read: The function used to read data
+ * @write: The function used to write data
+ * @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;
+	nvmem_reg_read_t read;
+	nvmem_reg_write_t write;
+	unsigned int offset;
+	size_t size;
+};
+
+struct udevice;
+
+#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 */
+
+/**
+ * struct nvmem_interface - nvmem interface for a uclass
+ * @id: The uclass id for this interface
+ * @read: The function to use to read devices of uclass @id
+ * @write: The function to use to write devices of uclass @id
+ *
+ * This structure specifies the read and write nvmem functions for a given
+ * uclass. Drivers of uclasses implementing this interface will be elligible to
+ * have nvmem cell child nodes. Those nodes will specify the offset and size
+ * which will be passed to @read and @write.
+ *
+ * @read and @write *must* be implemented. If all devices in the uclass are
+ * read-only, then a dummy @write should be provided which just returns
+ * -ENOSYS.
+ */
+struct nvmem_interface {
+	enum uclass_id id;
+	nvmem_reg_read_t read;
+	nvmem_reg_write_t write;
+};
+
+#if CONFIG_IS_ENABLED(NVMEM)
+/* Declare a new nvmem_interface */
+#define NVMEM_INTERFACE(__name) \
+	ll_entry_declare(struct nvmem_interface, __name, nvmem_interface)
+#else
+#define NVMEM_INTERFACE(__name) \
+	static __always_unused struct nvmem_interface nvmem_interface_##__name
+#endif
+
+#endif /* NVMEM_H */
-- 
2.25.1

[PATCH 07/14] sandbox: Enable NVMEM

[Sean Anderson]

This enables NVMEM for all sandbox defconfigs, enabling it to be used in
unit tests in the next few commits.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 configs/sandbox64_defconfig        | 1 +
 configs/sandbox_defconfig          | 1 +
 configs/sandbox_flattree_defconfig | 1 +
 configs/sandbox_noinst_defconfig   | 1 +
 configs/sandbox_spl_defconfig      | 1 +
 5 files changed, 5 insertions(+)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index c9afe4c840..3b2d2eeac8 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -138,6 +138,7 @@ CONFIG_LED_GPIO=y
 CONFIG_DM_MAILBOX=y
 CONFIG_SANDBOX_MBOX=y
 CONFIG_MISC=y
+CONFIG_NVMEM=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_I2C=y
 CONFIG_CROS_EC_LPC=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index f85274353d..e71ea1d0a3 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -176,6 +176,7 @@ CONFIG_LED_GPIO=y
 CONFIG_DM_MAILBOX=y
 CONFIG_SANDBOX_MBOX=y
 CONFIG_MISC=y
+CONFIG_NVMEM=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_I2C=y
 CONFIG_CROS_EC_LPC=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index 7bd5d01ace..c2f5a8fc14 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -115,6 +115,7 @@ CONFIG_LED_GPIO=y
 CONFIG_DM_MAILBOX=y
 CONFIG_SANDBOX_MBOX=y
 CONFIG_MISC=y
+CONFIG_NVMEM=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_I2C=y
 CONFIG_CROS_EC_LPC=y
diff --git a/configs/sandbox_noinst_defconfig b/configs/sandbox_noinst_defconfig
index 7d872ada15..acd153d9dc 100644
--- a/configs/sandbox_noinst_defconfig
+++ b/configs/sandbox_noinst_defconfig
@@ -136,6 +136,7 @@ CONFIG_LED_GPIO=y
 CONFIG_DM_MAILBOX=y
 CONFIG_SANDBOX_MBOX=y
 CONFIG_MISC=y
+CONFIG_NVMEM=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_I2C=y
 CONFIG_CROS_EC_LPC=y
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index 29a89171bc..19a420ec65 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -138,6 +138,7 @@ CONFIG_LED_GPIO=y
 CONFIG_DM_MAILBOX=y
 CONFIG_SANDBOX_MBOX=y
 CONFIG_MISC=y
+CONFIG_SPL_NVMEM=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_I2C=y
 CONFIG_CROS_EC_LPC=y
-- 
2.25.1

[PATCH 08/14] rtc: Implement nvmem interface

[Sean Anderson]

This adds nvmem support for RTCs with nvmem registers.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/rtc/rtc-uclass.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index e5ae6ea4d5..345185d2b8 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -11,6 +11,7 @@
 #include <errno.h>
 #include <log.h>
 #include <rtc.h>
+#include <nvmem.h>
 
 int dm_rtc_get(struct udevice *dev, struct rtc_time *time)
 {
@@ -181,3 +182,21 @@ UCLASS_DRIVER(rtc) = {
 	.post_bind	= dm_scan_fdt_dev,
 #endif
 };
+
+static int rtc_nvmem_read(struct udevice *dev, unsigned int offset, void *buf,
+			  size_t size)
+{
+	return dm_rtc_read(dev, offset, buf, size);
+}
+
+static int rtc_nvmem_write(struct udevice *dev, unsigned int offset,
+			   const void *buf, size_t size)
+{
+	return dm_rtc_write(dev, offset, buf, size);
+}
+
+NVMEM_INTERFACE(rtc) = {
+	.id = UCLASS_RTC,
+	.read = rtc_nvmem_read,
+	.write = rtc_nvmem_write,
+};
-- 
2.25.1

[PATCH 09/14] misc: i2c_eeprom: Implement nvmem interface

[Sean Anderson]

This adds nvmem support for i2c eeproms.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/misc/i2c_eeprom.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
index 4302e180ac..ae18dca1bf 100644
--- a/drivers/misc/i2c_eeprom.c
+++ b/drivers/misc/i2c_eeprom.c
@@ -14,6 +14,7 @@
 #include <dm/device-internal.h>
 #include <i2c.h>
 #include <i2c_eeprom.h>
+#include <nvmem.h>
 
 struct i2c_eeprom_drv_data {
 	u32 size; /* size in bytes */
@@ -378,3 +379,21 @@ UCLASS_DRIVER(i2c_eeprom) = {
 	.id		= UCLASS_I2C_EEPROM,
 	.name		= "i2c_eeprom",
 };
+
+static int i2c_eeprom_nvmem_read(struct udevice *dev, unsigned int offset,
+				 void *buf, size_t size)
+{
+	return i2c_eeprom_read(dev, offset, buf, size);
+}
+
+static int i2c_eeprom_nvmem_write(struct udevice *dev, unsigned int offset,
+				  const void *buf, size_t size)
+{
+	return i2c_eeprom_write(dev, offset, buf, size);
+}
+
+NVMEM_INTERFACE(i2c_eeprom) = {
+	.id = UCLASS_I2C_EEPROM,
+	.read = i2c_eeprom_nvmem_read,
+	.write = i2c_eeprom_nvmem_write,
+};
-- 
2.25.1

[PATCH 10/14] misc: Implement nvmem interface

[Sean Anderson]

This adds nvmem support for misc devices. Short reads/writes are not
allowed, so we need to translate the return code a bit. This breaks the API
slightly, because the original buffer shouldn't be touched if we fail while
writing. If this becomes an issue, we can add a bounce buffer here.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 drivers/misc/misc-uclass.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/misc/misc-uclass.c b/drivers/misc/misc-uclass.c
index cfe9d562fa..6bbc78c38a 100644
--- a/drivers/misc/misc-uclass.c
+++ b/drivers/misc/misc-uclass.c
@@ -9,6 +9,7 @@
 #include <dm.h>
 #include <errno.h>
 #include <misc.h>
+#include <nvmem.h>
 
 /*
  * Implement a  miscellaneous uclass for those do not fit other more
@@ -74,3 +75,33 @@ UCLASS_DRIVER(misc) = {
 	.post_bind	= dm_scan_fdt_dev,
 #endif
 };
+
+static int misc_nvmem_read(struct udevice *dev, unsigned int offset, void *buf,
+			   size_t size)
+{
+	int ret = misc_read(dev, offset, buf, size);
+
+	if (ret < 0)
+		return ret;
+	if (ret != size)
+		return -EIO;
+	return 0;
+}
+
+static int misc_nvmem_write(struct udevice *dev, unsigned int offset,
+			    const void *buf, size_t size)
+{
+	int ret = misc_write(dev, offset, buf, size);
+
+	if (ret < 0)
+		return ret;
+	if (ret != size)
+		return -EIO;
+	return 0;
+}
+
+NVMEM_INTERFACE(misc) = {
+	.id = UCLASS_MISC,
+	.read = misc_nvmem_read,
+	.write = misc_nvmem_write,
+};
-- 
2.25.1

[PATCH 11/14] net: Add support for reading mac addresses from nvmem cells

[Sean Anderson]

This adds support for reading mac addresses from the "mac-address" nvmem
cell. If there is no (local-)mac-address property, then we will try
reading from an nvmem cell.

For some existing examples of this property, refer to imx8mn.dtsi and
imx8mp.dtsi. Unfortunately, fuse drivers have not yet been converted
to DM.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 net/eth-uclass.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 58c308f332..211e88fbbe 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -13,6 +13,7 @@
 #include <env.h>
 #include <log.h>
 #include <net.h>
+#include <nvmem.h>
 #include <asm/global_data.h>
 #include <dm/device-internal.h>
 #include <dm/uclass-internal.h>
@@ -499,17 +500,21 @@ static bool eth_dev_get_mac_address(struct udevice *dev, u8 mac[ARP_HLEN])
 {
 #if CONFIG_IS_ENABLED(OF_CONTROL)
 	const uint8_t *p;
+	struct nvmem_cell mac_cell;
 
 	p = dev_read_u8_array_ptr(dev, "mac-address", ARP_HLEN);
 	if (!p)
 		p = dev_read_u8_array_ptr(dev, "local-mac-address", ARP_HLEN);
 
-	if (!p)
+	if (p) {
+		memcpy(mac, p, ARP_HLEN);
+		return true;
+	}
+
+	if (nvmem_cell_get_by_name(dev, "mac-address", &mac_cell))
 		return false;
 
-	memcpy(mac, p, ARP_HLEN);
-
-	return true;
+	return !nvmem_cell_read(&mac_cell, mac, ARP_HLEN);
 #else
 	return false;
 #endif
-- 
2.25.1

[PATCH 12/14] test: Load mac address with i2c eeprom

[Sean Anderson]

This uses an i2c eeprom to load a mac address using the nvmem interface.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 arch/sandbox/dts/test.dts      | 9 ++++++++-
 drivers/misc/i2c_eeprom_emul.c | 4 ++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index e111a1f050..031fe2bf6d 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -517,7 +517,8 @@
 	eth_3: sbe5 {
 		compatible = "sandbox,eth";
 		reg = <0x10005000 0x1000>;
-		mac-address = [ 02 00 11 22 33 45 ];
+		nvmem-cells = <&eth3_addr>;
+		nvmem-cell-names = "mac-address";
 	};
 
 	eth@10004000 {
@@ -683,6 +684,8 @@
 		pinctrl-0 = <&pinmux_i2c0_pins>;
 
 		eeprom@2c {
+			#address-cells = <1>;
+			#size-cells = <1>;
 			reg = <0x2c>;
 			compatible = "i2c-eeprom";
 			sandbox,emul = <&emul_eeprom>;
@@ -694,6 +697,10 @@
 					reg = <10 2>;
 				};
 			};
+
+			eth3_addr: mac-address@24 {
+				reg = <24 6>;
+			};
 		};
 
 		rtc_0: rtc@43 {
diff --git a/drivers/misc/i2c_eeprom_emul.c b/drivers/misc/i2c_eeprom_emul.c
index 85b127c406..6f32087ede 100644
--- a/drivers/misc/i2c_eeprom_emul.c
+++ b/drivers/misc/i2c_eeprom_emul.c
@@ -171,11 +171,15 @@ static int sandbox_i2c_eeprom_probe(struct udevice *dev)
 {
 	struct sandbox_i2c_flash_plat_data *plat = dev_get_plat(dev);
 	struct sandbox_i2c_flash *priv = dev_get_priv(dev);
+	/* For eth3 */
+	const u8 mac[] = { 0x02, 0x00, 0x11, 0x22, 0x33, 0x45 };
 
 	priv->data = calloc(1, plat->size);
 	if (!priv->data)
 		return -ENOMEM;
 
+	memcpy(&priv->data[24], mac, sizeof(mac));
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH 13/14] test: Load mac address using RTC

[Sean Anderson]

This uses the nvmem API to load a mac address from an RTC.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 arch/sandbox/dts/test.dts  |  9 ++++++++-
 drivers/rtc/i2c_rtc_emul.c | 10 ++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 031fe2bf6d..0dfde3c122 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -529,7 +529,8 @@
 	dsa_eth0: dsa-test-eth {
 		compatible = "sandbox,eth";
 		reg = <0x10006000 0x1000>;
-		mac-address = [ 02 00 11 22 33 48 ];
+		nvmem-cells = <&eth4_addr>;
+		nvmem-cell-names = "mac-address";
 	};
 
 	dsa-test {
@@ -704,9 +705,15 @@
 		};
 
 		rtc_0: rtc@43 {
+			#address-cells = <1>;
+			#size-cells = <1>;
 			reg = <0x43>;
 			compatible = "sandbox-rtc";
 			sandbox,emul = <&emul0>;
+
+			eth4_addr: mac-address@40 {
+				reg = <0x40 6>;
+			};
 		};
 
 		rtc_1: rtc@61 {
diff --git a/drivers/rtc/i2c_rtc_emul.c b/drivers/rtc/i2c_rtc_emul.c
index ba418c25da..c307d6036d 100644
--- a/drivers/rtc/i2c_rtc_emul.c
+++ b/drivers/rtc/i2c_rtc_emul.c
@@ -203,6 +203,15 @@ static int sandbox_i2c_rtc_bind(struct udevice *dev)
 	return 0;
 }
 
+static int sandbox_i2c_rtc_probe(struct udevice *dev)
+{
+	const u8 mac[] = { 0x02, 0x00, 0x11, 0x22, 0x33, 0x48 };
+	struct sandbox_i2c_rtc_plat_data *plat = dev_get_plat(dev);
+
+	memcpy(&plat->reg[0x40], mac, sizeof(mac));
+	return 0;
+}
+
 static const struct udevice_id sandbox_i2c_rtc_ids[] = {
 	{ .compatible = "sandbox,i2c-rtc-emul" },
 	{ }
@@ -213,6 +222,7 @@ U_BOOT_DRIVER(sandbox_i2c_rtc_emul) = {
 	.id		= UCLASS_I2C_EMUL,
 	.of_match	= sandbox_i2c_rtc_ids,
 	.bind		= sandbox_i2c_rtc_bind,
+	.probe		= sandbox_i2c_rtc_probe,
 	.priv_auto	= sizeof(struct sandbox_i2c_rtc),
 	.plat_auto	= sizeof(struct sandbox_i2c_rtc_plat_data),
 	.ops		= &sandbox_i2c_rtc_emul_ops,
-- 
2.25.1

[PATCH 14/14] test: Load mac address using misc device

[Sean Anderson]

This loads a mac address using a misc device using the nvmem interface.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 arch/sandbox/dts/test.dts   | 9 ++++++++-
 drivers/misc/misc_sandbox.c | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 0dfde3c122..1c5663c8e7 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -511,7 +511,8 @@
 	eth_5: eth@10003000 {
 		compatible = "sandbox,eth";
 		reg = <0x10003000 0x1000>;
-		mac-address = [ 02 00 11 22 33 46 ];
+		nvmem-cells = <&eth5_addr>;
+		nvmem-cell-names = "mac-address";
 	};
 
 	eth_3: sbe5 {
@@ -895,7 +896,13 @@
 	};
 
 	misc-test {
+		#address-cells = <1>;
+		#size-cells = <1>;
 		compatible = "sandbox,misc_sandbox";
+
+		eth5_addr: mac-address@10 {
+			reg = <0x10 6>;
+		};
 	};
 
 	mmc2 {
diff --git a/drivers/misc/misc_sandbox.c b/drivers/misc/misc_sandbox.c
index 0e4292fd0a..31cde2dbac 100644
--- a/drivers/misc/misc_sandbox.c
+++ b/drivers/misc/misc_sandbox.c
@@ -112,8 +112,11 @@ static const struct misc_ops misc_sandbox_ops = {
 int misc_sandbox_probe(struct udevice *dev)
 {
 	struct misc_sandbox_priv *priv = dev_get_priv(dev);
+	/* For eth5 */
+	const u8 mac[] = { 0x02, 0x00, 0x11, 0x22, 0x33, 0x46 };
 
 	priv->enabled = true;
+	memcpy(&priv->mem[16], mac, sizeof(mac));
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 00/14] Add support for NVMEM API

[Michael Walle]

Hi Sean,

> Should this be implemented on top of/as part of syscon/regmap? In Linux,
> all nvmem devices must also implement regmap support, making nvmem a
> subset of regmap drivers. There are certainly similarities, but I don't
> know if we want to make this depend on regmap.

What do you mean by that? I assume with "nvmem devices" you mean the
nvmem providers. I've implemented the nvmem provider for MTD OTP devices
in linux and there is no regmap support.

-michael

Re: [PATCH 00/14] Add support for NVMEM API

[Sean Anderson]

Hi Michael,

On 2/8/22 10:00 AM, Michael Walle wrote:
> Hi Sean,
> 
>> Should this be implemented on top of/as part of syscon/regmap? In Linux,
>> all nvmem devices must also implement regmap support, making nvmem a
>> subset of regmap drivers. There are certainly similarities, but I don't
>> know if we want to make this depend on regmap.
> 
> What do you mean by that? I assume with "nvmem devices" you mean the
> nvmem providers. I've implemented the nvmem provider for MTD OTP devices
> in linux and there is no regmap support.

According to [1],

> [The NVMEM] framework is based on regmap, so that most of the
> abstraction available in regmap can be reused, across multiple types
> of buses.

and

> It is mandatory that the NVMEM provider has a regmap associated with
> its struct device. Failure to do would return error code from
> nvmem_register().

which seems to imply that NVMEM devices should also be regmaps.

--Sean

[1] https://www.kernel.org/doc/html/latest/driver-api/nvmem.html

Re: [PATCH 00/14] Add support for NVMEM API

[Michael Walle]

Hi Sean,

Am 2022-02-08 17:02, schrieb Sean Anderson:
>>> Should this be implemented on top of/as part of syscon/regmap? In 
>>> Linux,
>>> all nvmem devices must also implement regmap support, making nvmem a
>>> subset of regmap drivers. There are certainly similarities, but I 
>>> don't
>>> know if we want to make this depend on regmap.
>> 
>> What do you mean by that? I assume with "nvmem devices" you mean the
>> nvmem providers. I've implemented the nvmem provider for MTD OTP 
>> devices
>> in linux and there is no regmap support.
> 
> According to [1],
> 
>> [The NVMEM] framework is based on regmap, so that most of the
>> abstraction available in regmap can be reused, across multiple types
>> of buses.
> 
> and
> 
>> It is mandatory that the NVMEM provider has a regmap associated with
>> its struct device. Failure to do would return error code from
>> nvmem_register().
> 
> which seems to imply that NVMEM devices should also be regmaps.

Ahh I see. That seems to be outdated, see commit
795ddd18d38f9762fbfefceab9aa16caef0cf431 ("nvmem: core: remove
regmap dependency").

-michael

Re: [PATCH 01/14] sandbox: net: Remove fake-host-hwaddr

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> Instead of reading a pseudo-rom mac address from the device tree, just use
> whatever we get from write_hwaddr. This has the effect of using the mac
> address from the environment (or from the device tree, if it is
> specified).
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  arch/sandbox/dts/sandbox.dts   |  1 -
>  arch/sandbox/dts/sandbox64.dts |  1 -
>  arch/sandbox/dts/test.dts      |  5 -----
>  drivers/net/sandbox.c          | 10 ++--------
>  4 files changed, 2 insertions(+), 15 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 01/14] sandbox: net: Remove fake-host-hwaddr

[Ramon Fried]

On Sat, Feb 19, 2022 at 2:21 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
> >
> > Instead of reading a pseudo-rom mac address from the device tree, just use
> > whatever we get from write_hwaddr. This has the effect of using the mac
> > address from the environment (or from the device tree, if it is
> > specified).
> >
> > Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> > ---
> >
> >  arch/sandbox/dts/sandbox.dts   |  1 -
> >  arch/sandbox/dts/sandbox64.dts |  1 -
> >  arch/sandbox/dts/test.dts      |  5 -----
> >  drivers/net/sandbox.c          | 10 ++--------
> >  4 files changed, 2 insertions(+), 15 deletions(-)
> >
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Acked-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH 02/14] sandbox: Remove eth2addr from environment

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> DSA interfaces use the same mac address for each interface, unless
> instructed otherwise. Just set eth4addr and let eth2addr and eth7addr be
> set automatically.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  board/sandbox/sandbox.env | 1 -
>  1 file changed, 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 03/14] test: eth: Add test for ethernet addresses

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This adds a test to make sure that all the ethernet interfaces have
> their addresses read properly. At the moment everything is read from the
> environment, but the next few commits will add additional sources.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  test/dm/eth.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 04/14] sandbox: Move some mac addresses to device tree

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This prevents some conflicts when running sandbox with -D, since the
> "rom" mac address will be random and won't match the environment. We
> still need to keep addresses for eth1 and eth6 in the environment,
> because dm_test_eth_rotate expects to be able to disable them by
> removing their envaddr variables. This can likely be fixed in a future
> series by adding a function to cause sandbox eth_opts callback for a
> particular mac to fail immediately.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  arch/sandbox/dts/test.dts | 3 +++
>  board/sandbox/sandbox.env | 3 ---
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 05/14] misc: i2c_eeprom: Make i2c_eeprom_write use a const buf

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> i2c_eeprom_ops->write uses a const buf, so use one for the wrapper
> function as well.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  drivers/misc/i2c_eeprom.c | 3 ++-
>  include/i2c_eeprom.h      | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 06/14] misc: Add support for nvmem cells

[Simon Glass]

Hi Sean,

On Mon, 7 Feb 2022 at 16:42, 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. 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.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  MAINTAINERS           |   7 ++
>  doc/api/index.rst     |   1 +
>  doc/api/nvmem.rst     |   7 ++
>  drivers/misc/Kconfig  |  16 ++++
>  drivers/misc/Makefile |   1 +
>  drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
>  include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 326 insertions(+)
>  create mode 100644 doc/api/nvmem.rst
>  create mode 100644 drivers/misc/nvmem.c
>  create mode 100644 include/nvmem.h

Here I think it would be better to add a new uclass so that drivers
which support it can add a child device in that uclass. This is done
in lots of places in U-Boot.

Regards,
Simon

Re: [PATCH 07/14] sandbox: Enable NVMEM

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This enables NVMEM for all sandbox defconfigs, enabling it to be used in
> unit tests in the next few commits.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  configs/sandbox64_defconfig        | 1 +
>  configs/sandbox_defconfig          | 1 +
>  configs/sandbox_flattree_defconfig | 1 +
>  configs/sandbox_noinst_defconfig   | 1 +
>  configs/sandbox_spl_defconfig      | 1 +
>  5 files changed, 5 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 11/14] net: Add support for reading mac addresses from nvmem cells

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This adds support for reading mac addresses from the "mac-address" nvmem
> cell. If there is no (local-)mac-address property, then we will try
> reading from an nvmem cell.
>
> For some existing examples of this property, refer to imx8mn.dtsi and
> imx8mp.dtsi. Unfortunately, fuse drivers have not yet been converted
> to DM.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  net/eth-uclass.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 12/14] test: Load mac address with i2c eeprom

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This uses an i2c eeprom to load a mac address using the nvmem interface.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  arch/sandbox/dts/test.dts      | 9 ++++++++-
>  drivers/misc/i2c_eeprom_emul.c | 4 ++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 13/14] test: Load mac address using RTC

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This uses the nvmem API to load a mac address from an RTC.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  arch/sandbox/dts/test.dts  |  9 ++++++++-
>  drivers/rtc/i2c_rtc_emul.c | 10 ++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 14/14] test: Load mac address using misc device

[Simon Glass]

On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@seco.com> wrote:
>
> This loads a mac address using a misc device using the nvmem interface.
>
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
>
>  arch/sandbox/dts/test.dts   | 9 ++++++++-
>  drivers/misc/misc_sandbox.c | 3 +++
>  2 files changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 06/14] misc: Add support for nvmem cells

[Sean Anderson]

On 2/26/22 1:36 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Mon, 7 Feb 2022 at 16:42, 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. 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.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>>  MAINTAINERS           |   7 ++
>>  doc/api/index.rst     |   1 +
>>  doc/api/nvmem.rst     |   7 ++
>>  drivers/misc/Kconfig  |  16 ++++
>>  drivers/misc/Makefile |   1 +
>>  drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
>>  include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 326 insertions(+)
>>  create mode 100644 doc/api/nvmem.rst
>>  create mode 100644 drivers/misc/nvmem.c
>>  create mode 100644 include/nvmem.h
> 
> Here I think it would be better to add a new uclass so that drivers
> which support it can add a child device in that uclass. This is done
> in lots of places in U-Boot.

I'm not sure exactly what you have in mind. The issue is that there are at
least 6 uclasses which I would like to support:

- UCLASS_MISC
- UCLASS_I2C_EEPROM
- UCLASS_RTC
- UCLASS_MTD
- UCLASS_FUSE (doesn't exist yet, but probably should)
- Possibly UCLASS_PMIC

Most of these uclasses have existing interfaces which expose an NVMEM-like API,
in addition to other uclass-specific functionality. Instead of having an
additional API which drivers must implement, I would like to leverage these
existing APIs to make adding NVMEM support as painless as possible. NVMEM is
more of a "meta-uclass" which allows us to leverage existing read/write
functions in uclasses. If any additional devices are to be created, they need
to be created by the nvmem subsystem, or by the supported uclasses, rather than
in drivers.

Now, there are some instances where creating a new child device might be the
best approach. For example, you might have some OTP memory in an unrelated
device. In that case, creating a child device (of UCLASS_MISC, or UCLASS_FUSE
if that gets created) is the right course of action.

--Sean

Re: [PATCH 06/14] misc: Add support for nvmem cells

[Simon Glass]

Hi Sean,

On Mon, 28 Feb 2022 at 09:43, Sean Anderson <sean.anderson@seco.com> wrote:
>
>
>
> On 2/26/22 1:36 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Mon, 7 Feb 2022 at 16:42, 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. 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.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> ---
> >>
> >>  MAINTAINERS           |   7 ++
> >>  doc/api/index.rst     |   1 +
> >>  doc/api/nvmem.rst     |   7 ++
> >>  drivers/misc/Kconfig  |  16 ++++
> >>  drivers/misc/Makefile |   1 +
> >>  drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
> >>  include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
> >>  7 files changed, 326 insertions(+)
> >>  create mode 100644 doc/api/nvmem.rst
> >>  create mode 100644 drivers/misc/nvmem.c
> >>  create mode 100644 include/nvmem.h
> >
> > Here I think it would be better to add a new uclass so that drivers
> > which support it can add a child device in that uclass. This is done
> > in lots of places in U-Boot.
>
> I'm not sure exactly what you have in mind. The issue is that there are at
> least 6 uclasses which I would like to support:
>
> - UCLASS_MISC
> - UCLASS_I2C_EEPROM
> - UCLASS_RTC
> - UCLASS_MTD
> - UCLASS_FUSE (doesn't exist yet, but probably should)
> - Possibly UCLASS_PMIC
>
> Most of these uclasses have existing interfaces which expose an NVMEM-like API,
> in addition to other uclass-specific functionality. Instead of having an
> additional API which drivers must implement, I would like to leverage these
> existing APIs to make adding NVMEM support as painless as possible. NVMEM is
> more of a "meta-uclass" which allows us to leverage existing read/write
> functions in uclasses. If any additional devices are to be created, they need
> to be created by the nvmem subsystem, or by the supported uclasses, rather than
> in drivers.

I may be missing something as I have not looked in detail at your API changes.

But the way to have a consistent API is to use a uclass. We do this
with BLK. When a PMIC have GPIOs, RTC and regulators, we add them as
child devices. We also have it with bootstd, where a bootdev is
created as a child device of a storage device. We can put the required
stuff in a helper function. We can even avoid any new code in the
drivers by using the pending event system.

Can you first help me understand what is wrong with using a new uclass?

>
> Now, there are some instances where creating a new child device might be the
> best approach. For example, you might have some OTP memory in an unrelated
> device. In that case, creating a child device (of UCLASS_MISC, or UCLASS_FUSE
> if that gets created) is the right course of action.

Regards,
SImon

Re: [PATCH 06/14] misc: Add support for nvmem cells

[Sean Anderson]

Hi Simon,

On 3/1/22 9:58 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Mon, 28 Feb 2022 at 09:43, Sean Anderson <sean.anderson@seco.com> wrote:
>>
>>
>>
>> On 2/26/22 1:36 PM, Simon Glass wrote:
>> > Hi Sean,
>> >
>> > On Mon, 7 Feb 2022 at 16:42, 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. 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.
>> >>
>> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> >> ---
>> >>
>> >>  MAINTAINERS           |   7 ++
>> >>  doc/api/index.rst     |   1 +
>> >>  doc/api/nvmem.rst     |   7 ++
>> >>  drivers/misc/Kconfig  |  16 ++++
>> >>  drivers/misc/Makefile |   1 +
>> >>  drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
>> >>  include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
>> >>  7 files changed, 326 insertions(+)
>> >>  create mode 100644 doc/api/nvmem.rst
>> >>  create mode 100644 drivers/misc/nvmem.c
>> >>  create mode 100644 include/nvmem.h
>> >
>> > Here I think it would be better to add a new uclass so that drivers
>> > which support it can add a child device in that uclass. This is done
>> > in lots of places in U-Boot.
>>
>> I'm not sure exactly what you have in mind. The issue is that there are at
>> least 6 uclasses which I would like to support:
>>
>> - UCLASS_MISC
>> - UCLASS_I2C_EEPROM
>> - UCLASS_RTC
>> - UCLASS_MTD
>> - UCLASS_FUSE (doesn't exist yet, but probably should)
>> - Possibly UCLASS_PMIC
>>
>> Most of these uclasses have existing interfaces which expose an NVMEM-like API,
>> in addition to other uclass-specific functionality. Instead of having an
>> additional API which drivers must implement, I would like to leverage these
>> existing APIs to make adding NVMEM support as painless as possible. NVMEM is
>> more of a "meta-uclass" which allows us to leverage existing read/write
>> functions in uclasses. If any additional devices are to be created, they need
>> to be created by the nvmem subsystem, or by the supported uclasses, rather than
>> in drivers.
> 
> I may be missing something as I have not looked in detail at your API changes.
> 
> But the way to have a consistent API is to use a uclass. We do this
> with BLK. When a PMIC have GPIOs, RTC and regulators, we add them as
> child devices. We also have it with bootstd, where a bootdev is
> created as a child device of a storage device. We can put the required
> stuff in a helper function. We can even avoid any new code in the
> drivers by using the pending event system.
> 
> Can you first help me understand what is wrong with using a new uclass?

I suppose it could be done this way.

Effectively, we are "picking" out two functions from the existing API.
NVMEM is a proper sub-uclass of every uclass added in this series except
UCLASS_MISC (which just needs some API adjustment). In essence, we could
actually implement something like nvmem_cell_read as

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(dev, offset, buf, size)
	case UCLASS_RTC:
		return dm_rtc_read(cell->nvmem, offset, buf, size);
	case UCLASS_MISC: {
		int ret = misc_read(cell->nvmem, offset, buf, size);

		if (ret < 0)
			return ret;
		if (ret != size)
			return -EIO;
		return 0;
	/* etc */
	}
	}

	return -ENOSYS;
}

Actually, that is probably cleaner than my current approach.

--Sean

Re: [PATCH 06/14] misc: Add support for nvmem cells

[Simon Glass]

Hi Sean,

On Thu, 3 Mar 2022 at 10:45, Sean Anderson <sean.anderson@seco.com> wrote:
>
> Hi Simon,
>
> On 3/1/22 9:58 AM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Mon, 28 Feb 2022 at 09:43, Sean Anderson <sean.anderson@seco.com> wrote:
> >>
> >>
> >>
> >> On 2/26/22 1:36 PM, Simon Glass wrote:
> >> > Hi Sean,
> >> >
> >> > On Mon, 7 Feb 2022 at 16:42, 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. 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.
> >> >>
> >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> >> ---
> >> >>
> >> >>  MAINTAINERS           |   7 ++
> >> >>  doc/api/index.rst     |   1 +
> >> >>  doc/api/nvmem.rst     |   7 ++
> >> >>  drivers/misc/Kconfig  |  16 ++++
> >> >>  drivers/misc/Makefile |   1 +
> >> >>  drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
> >> >>  include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
> >> >>  7 files changed, 326 insertions(+)
> >> >>  create mode 100644 doc/api/nvmem.rst
> >> >>  create mode 100644 drivers/misc/nvmem.c
> >> >>  create mode 100644 include/nvmem.h
> >> >
> >> > Here I think it would be better to add a new uclass so that drivers
> >> > which support it can add a child device in that uclass. This is done
> >> > in lots of places in U-Boot.
> >>
> >> I'm not sure exactly what you have in mind. The issue is that there are at
> >> least 6 uclasses which I would like to support:
> >>
> >> - UCLASS_MISC
> >> - UCLASS_I2C_EEPROM
> >> - UCLASS_RTC
> >> - UCLASS_MTD
> >> - UCLASS_FUSE (doesn't exist yet, but probably should)
> >> - Possibly UCLASS_PMIC
> >>
> >> Most of these uclasses have existing interfaces which expose an NVMEM-like API,
> >> in addition to other uclass-specific functionality. Instead of having an
> >> additional API which drivers must implement, I would like to leverage these
> >> existing APIs to make adding NVMEM support as painless as possible. NVMEM is
> >> more of a "meta-uclass" which allows us to leverage existing read/write
> >> functions in uclasses. If any additional devices are to be created, they need
> >> to be created by the nvmem subsystem, or by the supported uclasses, rather than
> >> in drivers.
> >
> > I may be missing something as I have not looked in detail at your API changes.
> >
> > But the way to have a consistent API is to use a uclass. We do this
> > with BLK. When a PMIC have GPIOs, RTC and regulators, we add them as
> > child devices. We also have it with bootstd, where a bootdev is
> > created as a child device of a storage device. We can put the required
> > stuff in a helper function. We can even avoid any new code in the
> > drivers by using the pending event system.
> >
> > Can you first help me understand what is wrong with using a new uclass?
>
> I suppose it could be done this way.
>
> Effectively, we are "picking" out two functions from the existing API.
> NVMEM is a proper sub-uclass of every uclass added in this series except
> UCLASS_MISC (which just needs some API adjustment). In essence, we could
> actually implement something like nvmem_cell_read as
>
> 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(dev, offset, buf, size)
>         case UCLASS_RTC:
>                 return dm_rtc_read(cell->nvmem, offset, buf, size);
>         case UCLASS_MISC: {
>                 int ret = misc_read(cell->nvmem, offset, buf, size);
>
>                 if (ret < 0)
>                         return ret;
>                 if (ret != size)
>                         return -EIO;
>                 return 0;
>         /* etc */
>         }
>         }
>
>         return -ENOSYS;
> }
>
> Actually, that is probably cleaner than my current approach.

Yes but it is still not using a new uclass, right?

Regards,
Simon

Re: [PATCH 06/14] misc: Add support for nvmem cells

[Sean Anderson]

On 3/11/22 9:25 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Thu, 3 Mar 2022 at 10:45, Sean Anderson <sean.anderson@seco.com> wrote:
>>
>> Hi Simon,
>>
>> On 3/1/22 9:58 AM, Simon Glass wrote:
>> > Hi Sean,
>> >
>> > On Mon, 28 Feb 2022 at 09:43, Sean Anderson <sean.anderson@seco.com> wrote:
>> >>
>> >>
>> >>
>> >> On 2/26/22 1:36 PM, Simon Glass wrote:
>> >> > Hi Sean,
>> >> >
>> >> > On Mon, 7 Feb 2022 at 16:42, 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. 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.
>> >> >>
>> >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> >> >> ---
>> >> >>
>> >> >>  MAINTAINERS           |   7 ++
>> >> >>  doc/api/index.rst     |   1 +
>> >> >>  doc/api/nvmem.rst     |   7 ++
>> >> >>  drivers/misc/Kconfig  |  16 ++++
>> >> >>  drivers/misc/Makefile |   1 +
>> >> >>  drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
>> >> >>  include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
>> >> >>  7 files changed, 326 insertions(+)
>> >> >>  create mode 100644 doc/api/nvmem.rst
>> >> >>  create mode 100644 drivers/misc/nvmem.c
>> >> >>  create mode 100644 include/nvmem.h
>> >> >
>> >> > Here I think it would be better to add a new uclass so that drivers
>> >> > which support it can add a child device in that uclass. This is done
>> >> > in lots of places in U-Boot.
>> >>
>> >> I'm not sure exactly what you have in mind. The issue is that there are at
>> >> least 6 uclasses which I would like to support:
>> >>
>> >> - UCLASS_MISC
>> >> - UCLASS_I2C_EEPROM
>> >> - UCLASS_RTC
>> >> - UCLASS_MTD
>> >> - UCLASS_FUSE (doesn't exist yet, but probably should)
>> >> - Possibly UCLASS_PMIC
>> >>
>> >> Most of these uclasses have existing interfaces which expose an NVMEM-like API,
>> >> in addition to other uclass-specific functionality. Instead of having an
>> >> additional API which drivers must implement, I would like to leverage these
>> >> existing APIs to make adding NVMEM support as painless as possible. NVMEM is
>> >> more of a "meta-uclass" which allows us to leverage existing read/write
>> >> functions in uclasses. If any additional devices are to be created, they need
>> >> to be created by the nvmem subsystem, or by the supported uclasses, rather than
>> >> in drivers.
>> >
>> > I may be missing something as I have not looked in detail at your API changes.
>> >
>> > But the way to have a consistent API is to use a uclass. We do this
>> > with BLK. When a PMIC have GPIOs, RTC and regulators, we add them as
>> > child devices. We also have it with bootstd, where a bootdev is
>> > created as a child device of a storage device. We can put the required
>> > stuff in a helper function. We can even avoid any new code in the
>> > drivers by using the pending event system.
>> >
>> > Can you first help me understand what is wrong with using a new uclass?
>>
>> I suppose it could be done this way.
>>
>> Effectively, we are "picking" out two functions from the existing API.
>> NVMEM is a proper sub-uclass of every uclass added in this series except
>> UCLASS_MISC (which just needs some API adjustment). In essence, we could
>> actually implement something like nvmem_cell_read as
>>
>> 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(dev, offset, buf, size)
>>         case UCLASS_RTC:
>>                 return dm_rtc_read(cell->nvmem, offset, buf, size);
>>         case UCLASS_MISC: {
>>                 int ret = misc_read(cell->nvmem, offset, buf, size);
>>
>>                 if (ret < 0)
>>                         return ret;
>>                 if (ret != size)
>>                         return -EIO;
>>                 return 0;
>>         /* etc */
>>         }
>>         }
>>
>>         return -ENOSYS;
>> }
>>
>> Actually, that is probably cleaner than my current approach.
> 
> Yes but it is still not using a new uclass, right?

Right. But the idea is to use existing uclasses, because several of
them effectively already implement the interface. See e.g. I2C and
RTC which almost don't need a wrapper (except that they use different
types for the size field).

--Sean