[PATCH v3 00/13] Add support for NVMEM API

[PATCH v3 00/13] 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").

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-7:   These are general cleanups, and may be applied independently of
       the rest of the series.
8-9:   Add NVMEM support
10-13: Support reading ethernet addresses using the NVMEM API and add
       some tests.

Changes in v3:
- Add aliases for ethernet devices
- Add mac address for eth8 to environment
- Move patch adding test earlier in the series
- Add test for eth8 as well

Changes in v2:
- Call the appropriate API functions directly from
  nvmem_cell_(read|write). This means we can drop the nvmem_interface
  machinery.

Sean Anderson (13):
  sandbox: net: Add aliases for ethernet devices
  sandbox: net: Add mac address for eth8 to environment
  test: eth: Add test for ethernet addresses
  sandbox: net: Remove fake-host-hwaddr
  sandbox: Remove eth2addr from environment
  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
  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          |  34 +++++--
 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          |   3 +-
 drivers/misc/i2c_eeprom_emul.c     |   4 +
 drivers/misc/misc_sandbox.c        |   3 +
 drivers/misc/nvmem.c               | 142 +++++++++++++++++++++++++++
 drivers/net/sandbox.c              |  10 +-
 drivers/rtc/i2c_rtc_emul.c         |  10 ++
 include/i2c_eeprom.h               |   3 +-
 include/nvmem.h                    | 151 +++++++++++++++++++++++++++++
 net/eth-uclass.c                   |  13 ++-
 test/dm/eth.c                      |  29 ++++++
 24 files changed, 419 insertions(+), 26 deletions(-)
 create mode 100644 doc/api/nvmem.rst
 create mode 100644 drivers/misc/nvmem.c
 create mode 100644 include/nvmem.h

-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 01/13] sandbox: net: Add aliases for ethernet devices

[Sean Anderson]

Commit f3dd213e15 ("net: introduce helpers to get PHY ofnode from MAC")
changed the ethernet sequence assignment from

uclass 36: ethernet
0   * eth@10002000 @ 05813460, seq 0
1   * eth@10003000 @ 05813550, seq 5
2   * sbe5 @ 05813640, seq 3
3   * eth@10004000 @ 05813730, seq 6
4   * dsa-test-eth @ 05813820, seq 4
5   * lan0 @ 05813a30, seq 2
6   * lan1 @ 05813b50, seq 7

to

uclass 36: ethernet
0   * eth@10002000 @ 03813630, seq 0
1   * eth@10003000 @ 03813720, seq 5
2   * sbe5 @ 03813810, seq 3
3   * eth@10004000 @ 03813900, seq 6
4     phy-test-eth @ 038139f0, seq 7
5   * dsa-test-eth @ 03813ae0, seq 4
6   * lan0 @ 03813cf0, seq 2
7   * lan1 @ 03813e10, seq 8

This caused the mac address assignment to switch around. Avoid this in
the future by assigning aliases for all ethernet devices. This reverts
the sequence to what it was before the aformentioned commit (with
phy-test-eth as seq 8). There is no ethernet1 for whatever reason.

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

Changes in v3:
- New

 arch/sandbox/dts/test.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 5b38ee4a5f..e480f6aff9 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -28,6 +28,9 @@
 		ethernet3 = &eth_3;
 		ethernet4 = &dsa_eth0;
 		ethernet5 = &eth_5;
+		ethernet6 = "/eth@10004000";
+		ethernet7 = &swp_1;
+		ethernet8 = &phy_eth0;
 		gpio1 = &gpio_a;
 		gpio2 = &gpio_b;
 		gpio3 = &gpio_c;
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH v3 01/13] sandbox: net: Add aliases for ethernet devices

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

On Mon, Apr 18, 2022 at 03:36:47PM -0400, Sean Anderson wrote:

> Commit f3dd213e15 ("net: introduce helpers to get PHY ofnode from MAC")
> changed the ethernet sequence assignment from
> 
> uclass 36: ethernet
> 0   * eth@10002000 @ 05813460, seq 0
> 1   * eth@10003000 @ 05813550, seq 5
> 2   * sbe5 @ 05813640, seq 3
> 3   * eth@10004000 @ 05813730, seq 6
> 4   * dsa-test-eth @ 05813820, seq 4
> 5   * lan0 @ 05813a30, seq 2
> 6   * lan1 @ 05813b50, seq 7
> 
> to
> 
> uclass 36: ethernet
> 0   * eth@10002000 @ 03813630, seq 0
> 1   * eth@10003000 @ 03813720, seq 5
> 2   * sbe5 @ 03813810, seq 3
> 3   * eth@10004000 @ 03813900, seq 6
> 4     phy-test-eth @ 038139f0, seq 7
> 5   * dsa-test-eth @ 03813ae0, seq 4
> 6   * lan0 @ 03813cf0, seq 2
> 7   * lan1 @ 03813e10, seq 8
> 
> This caused the mac address assignment to switch around. Avoid this in
> the future by assigning aliases for all ethernet devices. This reverts
> the sequence to what it was before the aformentioned commit (with
> phy-test-eth as seq 8). There is no ethernet1 for whatever reason.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> Changes in v3:
> - New
> 
>  arch/sandbox/dts/test.dts | 3 +++
>  1 file changed, 3 insertions(+)

This needs to update test/dm/test-fdt.c::dm_test_alias_highest_id() to
know 8 is the highest now, not 5.  I'm noting this as there's other less
easy to correct problems with the series.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v3 02/13] sandbox: net: Add mac address for eth8 to environment

[Sean Anderson]

The phy_eth0 interface introduced in f3dd213e15 ("net: introduce helpers to
get PHY ofnode from MAC") uses a globally-administered address. Switch to
using a locally-administered address, and add it to the sandbox
environment, like the others.

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

Changes in v3:
- New

 board/sandbox/sandbox.env | 1 +
 1 file changed, 1 insertion(+)

diff --git a/board/sandbox/sandbox.env b/board/sandbox/sandbox.env
index b4c04635a4..6dedc755fa 100644
--- a/board/sandbox/sandbox.env
+++ b/board/sandbox/sandbox.env
@@ -11,6 +11,7 @@ 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
+eth8addr=02:00:11:22:33:49
 ipaddr=192.0.2.1
 
 /*
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 03/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

Changes in v3:
- Move patch adding test earlier in the series
- Add test for eth8 as well

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

diff --git a/test/dm/eth.c b/test/dm/eth.c
index e4ee695610..5437f9ea4a 100644
--- a/test/dm/eth.c
+++ b/test/dm/eth.c
@@ -147,6 +147,35 @@ 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)
+{
+	static const char *const 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 */
+		"02:00:11:22:33:49",
+	};
+	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.35.1.1320.gc452695387.dirty

[PATCH v3 04/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
Acked-by: Ramon Fried <rfried.dev@gmail.com>
---

(no changes since v1)

 arch/sandbox/dts/sandbox.dts   |  1 -
 arch/sandbox/dts/sandbox64.dts |  1 -
 arch/sandbox/dts/test.dts      |  6 ------
 drivers/net/sandbox.c          | 10 ++--------
 4 files changed, 2 insertions(+), 16 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 e480f6aff9..9c3a19f576 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -512,31 +512,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];
 	};
 
 	phy_eth0: phy-test-eth {
 		compatible = "sandbox,eth";
 		reg = <0x10007000 0x1000>;
-		fake-host-hwaddr = [00 00 66 44 22 77];
 		phy-handle = <&ethphy1>;
 		phy-mode = "2500base-x";
 	};
@@ -544,7 +539,6 @@
 	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.35.1.1320.gc452695387.dirty

[PATCH v3 05/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

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

diff --git a/board/sandbox/sandbox.env b/board/sandbox/sandbox.env
index 6dedc755fa..88ed7a9606 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.35.1.1320.gc452695387.dirty

[PATCH v3 06/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

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

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 9c3a19f576..23a4e1f078 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -517,11 +517,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 {
@@ -532,6 +534,7 @@
 	phy_eth0: phy-test-eth {
 		compatible = "sandbox,eth";
 		reg = <0x10007000 0x1000>;
+		mac-address = [ 02 00 11 22 33 49 ];
 		phy-handle = <&ethphy1>;
 		phy-mode = "2500base-x";
 	};
@@ -539,6 +542,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 88ed7a9606..a2c19702d6 100644
--- a/board/sandbox/sandbox.env
+++ b/board/sandbox/sandbox.env
@@ -6,11 +6,7 @@ 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
-eth8addr=02:00:11:22:33:49
 ipaddr=192.0.2.1
 
 /*
-- 
2.35.1.1320.gc452695387.dirty

[PATCH v3 07/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 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.35.1.1320.gc452695387.dirty

[PATCH v3 08/13] 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. 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.

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

(no changes since v2)

Changes in v2:
- Call the appropriate API functions directly from
  nvmem_cell_(read|write). This means we can drop the nvmem_interface
  machinery.

 MAINTAINERS           |   7 ++
 doc/api/index.rst     |   1 +
 doc/api/nvmem.rst     |   7 ++
 drivers/misc/Kconfig  |  16 +++++
 drivers/misc/Makefile |   1 +
 drivers/misc/nvmem.c  | 142 +++++++++++++++++++++++++++++++++++++++
 include/nvmem.h       | 151 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 325 insertions(+)
 create mode 100644 doc/api/nvmem.rst
 create mode 100644 drivers/misc/nvmem.c
 create mode 100644 include/nvmem.h

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

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

[Simon Glass]

Hi Sean,

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

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

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

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

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

Regards,
Simon

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

[Sean Anderson]

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

We already do. E.g. the misc/rtc/eeprom commands. The problem is that
for software to access them, they would have to use misc_read/dm_rtc_read/
i2c_eeprom_read.

> This patch feels like a shortcut to me and I'm not sure of the
> benefit of that shortcut.
Well, I suppose it's because "nvmem" devices are strict subsets of
existing devices. There is no new functionality here (except adapting
between semantics like for misc). We should always be able to use the
existing API to implement support for a new underlying uclass. There
should never be device-specific read/write methods, because we can
use the existing read/write uclass methods.

What I'm trying to get at is that we sort of already have an nvmem
uclass with nvmem devices, they're just not accessible in a uniform
way. This series is trying to address the uniformity aspect. But I
don't think we need new devices for each nvmem interface, because
all they would do would take up ram/rom.

--Sean

PS. In an ideal world we'd have something like

struct nvmem_ops {
	read();
	write();
};

struct dm_rtc_ops {
	nvmem_ops nvmem;
	/* the other ops minus read/write */
};

int nvmem_read (...) {
	struct nvmem_ops *ops = cell->nvmem->ops;
	/* ... */

	return ops->read(...);
}

but unfortunately, we already have fragmented implementations.

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

[Sean Anderson]

Hi Simon,

On 4/25/22 11:24 AM, Sean Anderson wrote:
> 
> 
> On 4/25/22 1:48 AM, Simon Glass wrote:
>> Hi Sean,
>> 
>> On Mon, 18 Apr 2022 at 13:37, Sean Anderson <sean.anderson@seco.com> wrote:
>>>
>>> This adds support for "nvmem cells" as seen in Linux. The nvmem device
>>> class in Linux is used for various assorted ROMs and EEPROMs. In this
>>> sense, it is similar to UCLASS_MISC, but also includes
>>> UCLASS_I2C_EEPROM, UCLASS_RTC, and UCLASS_MTD. New drivers corresponding
>>> to a Linux-style nvmem device should be implemented as one of the
>>> previously-mentioned uclasses. The nvmem API acts as a compatibility
>>> layer to adapt the (slightly different) APIs of these uclasses. It also
>>> handles the lookup of nvmem cells.
>>>
>>> While nvmem devices can be accessed directly, they are most often used
>>> by reading/writing contiguous values called "cells". Cells typically
>>> hold information like calibration, versions, or configuration (such as
>>> mac addresses).
>>>
>>> nvmem devices can specify "cells" in their device tree:
>>>
>>>         qfprom: eeprom@700000 {
>>>                 #address-cells = <1>;
>>>                 #size-cells = <1>;
>>>                 reg = <0x00700000 0x100000>;
>>>
>>>                 /* ... */
>>>
>>>                 tsens_calibration: calib@404 {
>>>                         reg = <0x404 0x10>;
>>>                 };
>>>         };
>>>
>>> which can then be referenced like:
>>>
>>>         tsens {
>>>                 /* ... */
>>>                 nvmem-cells = <&tsens_calibration>;
>>>                 nvmem-cell-names = "calibration";
>>>         };
>>>
>>> The tsens driver could then read the calibration value like:
>>>
>>>         struct nvmem_cell cal_cell;
>>>         u8 cal[16];
>>>         nvmem_cell_get_by_name(dev, "calibration", &cal_cell);
>>>         nvmem_cell_read(&cal_cell, cal, sizeof(cal));
>>>
>>> Because nvmem devices are not all of the same uclass, supported uclasses
>>> must register a nvmem_interface struct. This allows CONFIG_NVMEM to be
>>> enabled without depending on specific uclasses. At the moment,
>>> nvmem_interface is very bare-bones, and assumes that no initialization
>>> is necessary. However, this could be amended in the future.
>>>
>>> Although I2C_EEPROM and MISC are quite similar (and could likely be
>>> unified), they present different read/write function signatures. To
>>> abstract over this, NVMEM uses the same read/write signature as Linux.
>>> In particular, short read/writes are not allowed, which is allowed by
>>> MISC.
>>>
>>> The functionality implemented by nvmem cells is very similar to that
>>> provided by i2c_eeprom_partition. "fixed-partition"s for eeproms does
>>> not seem to have made its way into Linux or into any device tree other
>>> than sandbox. It is possible that with the introduction of this API it
>>> would be possible to remove it.
>> 
>> I still think this would be better as a separate uclass, with child
>> devices created at bind time in each of the respective uclasses, like
>> mmc_bind() does. Then you will see the nvmem devices in the DM tree.
>> Wouldn't we want to add a command to access the nvmem devices?
> 
> We already do. E.g. the misc/rtc/eeprom commands. The problem is that
> for software to access them, they would have to use misc_read/dm_rtc_read/
> i2c_eeprom_read.
> 
>> This patch feels like a shortcut to me and I'm not sure of the
>> benefit of that shortcut.
> Well, I suppose it's because "nvmem" devices are strict subsets of
> existing devices. There is no new functionality here (except adapting
> between semantics like for misc). We should always be able to use the
> existing API to implement support for a new underlying uclass. There
> should never be device-specific read/write methods, because we can
> use the existing read/write uclass methods.
> 
> What I'm trying to get at is that we sort of already have an nvmem
> uclass with nvmem devices, they're just not accessible in a uniform
> way. This series is trying to address the uniformity aspect. But I
> don't think we need new devices for each nvmem interface, because
> all they would do would take up ram/rom.
> 
> --Sean
> 
> PS. In an ideal world we'd have something like
> 
> struct nvmem_ops {
> 	read();
> 	write();
> };
> 
> struct dm_rtc_ops {
> 	nvmem_ops nvmem;
> 	/* the other ops minus read/write */
> };
> 
> int nvmem_read (...) {
> 	struct nvmem_ops *ops = cell->nvmem->ops;
> 	/* ... */
> 
> 	return ops->read(...);
> }
> 
> but unfortunately, we already have fragmented implementations.
> 

To follow up on this, I've conducted some size experiments. The
following is the bloat caused by applying the current series on
sandbox64_defconfig:

add/remove: 8/0 grow/shrink: 7/2 up/down: 1069/-170 (899)
Function                                     old     new   delta
nvmem_cell_get_by_index                        -     216    +216
dm_test_ethaddr                                -     192    +192
nvmem_cell_write                               -     125    +125
nvmem_cell_read                                -     125    +125
nvmem_cell_get_by_name                         -      65     +65
addr                                           -      64     +64
sandbox_i2c_rtc_probe                          -      54     +54
sb_eth_write_hwaddr                           14      57     +43
sandbox_i2c_eeprom_probe                      70     112     +42
misc_sandbox_probe                            21      61     +40
eth_post_probe                               444     484     +40
_u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
__func__                                   15147   15163     +16
data_gz                                    18327   18338     +11
dsa_pre_probe                                181     185      +4
sb_eth_of_to_plat                            126      64     -62
default_environment                          553     445    -108
Total: Before=1765267, After=1766166, chg +0.05%

And here is the difference (from baseline) when using your
suggested approach:

add/remove: 26/0 grow/shrink: 8/2 up/down: 2030/-170 (1860)
Function                                     old     new   delta
dm_test_ethaddr                                -     192    +192
nvmem_cell_get_by_index                        -     152    +152
nvmem_register                                 -     137    +137
_u_boot_list_2_driver_2_rtc_nvmem              -     128    +128
_u_boot_list_2_driver_2_misc_nvmem             -     128    +128
_u_boot_list_2_driver_2_i2c_eeprom_nvmem       -     128    +128
_u_boot_list_2_uclass_driver_2_nvmem           -     120    +120
misc_nvmem_write                               -      68     +68
misc_nvmem_read                                -      68     +68
nvmem_cell_write                               -      66     +66
nvmem_cell_read                                -      65     +65
nvmem_cell_get_by_name                         -      65     +65
addr                                           -      64     +64
sandbox_i2c_rtc_probe                          -      54     +54
rtc_post_bind                                  -      48     +48
nvmem_rtc_write                                -      48     +48
nvmem_rtc_read                                 -      48     +48
misc_post_bind                                 -      48     +48
i2c_eeprom_nvmem_write                         -      48     +48
i2c_eeprom_nvmem_read                          -      48     +48
sb_eth_write_hwaddr                           14      57     +43
sandbox_i2c_eeprom_probe                      70     112     +42
misc_sandbox_probe                            21      61     +40
eth_post_probe                               444     484     +40
_u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
rtc_nvmem_ops                                  -      16     +16
misc_nvmem_ops                                 -      16     +16
i2c_eeprom_post_bind                           -      16     +16
i2c_eeprom_nvmem_ops                           -      16     +16
__func__                                   15147   15163     +16
data_gz                                    18327   18338     +11
fmt                                            -       9      +9
version_string                                68      74      +6
dsa_pre_probe                                181     185      +4
sb_eth_of_to_plat                            126      64     -62
default_environment                          553     445    -108
Total: Before=1765267, After=1767127, chg +0.11%

As you can see, adding a second driver for each nvmem device
doubles the size of this feature. The patch I used for this follows
(it does not apply cleanly to v3 because the base contains some
changes fixing bugs pointed out by Tom).

From 958bc25e3bbe78b0a861d1f54b277f63e826b830 Mon Sep 17 00:00:00 2001
From: Sean Anderson <sean.anderson@seco.com>
Date: Fri, 29 Apr 2022 15:33:38 -0400
Subject: [PATCH] misc: nvmem: Convert to using udevices

Instead of calling uclass methods directly, instead create some nvmem
devices solely for the purpose of holding nvmem ops. This is primarily
to illustrate the size difference between these approaches.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---
 drivers/misc/i2c_eeprom.c  |  37 ++++++++++++++
 drivers/misc/misc-uclass.c |  58 +++++++++++++++++++--
 drivers/misc/nvmem.c       | 100 ++++++++++++-------------------------
 drivers/rtc/rtc-uclass.c   |  46 +++++++++++++++--
 include/dm/uclass-id.h     |   1 +
 include/nvmem.h            |  96 +++++++++++++++++++++++------------
 6 files changed, 233 insertions(+), 105 deletions(-)

diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c
index 4302e180ac..3f6c7ebf4a 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 */
@@ -374,7 +375,43 @@ U_BOOT_DRIVER(i2c_eeprom_partition) = {
 	.ops			= &i2c_eeprom_partition_ops,
 };
 
+static int i2c_eeprom_nvmem_read(struct udevice *dev, unsigned int offset,
+				 void *buf, size_t size)
+{
+	return i2c_eeprom_read(dev_get_parent(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_get_parent(dev), offset, buf, size);
+}
+
+static struct __maybe_unused nvmem_ops i2c_eeprom_nvmem_ops = {
+	.read = i2c_eeprom_nvmem_read,
+	.write = i2c_eeprom_nvmem_write,
+};
+
+#if CONFIG_IS_ENABLED(NVMEM)
+U_BOOT_DRIVER(i2c_eeprom_nvmem) = {
+	.name	= "i2c_eeprom_nvmem",
+	.id	= UCLASS_NVMEM,
+	.ops	= &i2c_eeprom_nvmem_ops,
+	.flags	= DM_FLAG_NAME_ALLOCED,
+};
+#endif
+
+#if CONFIG_IS_ENABLED(NVMEM)
+static int i2c_eeprom_post_bind(struct udevice *dev)
+{
+	return nvmem_register(dev, DM_DRIVER_GET(misc_nvmem));
+}
+#endif
+
 UCLASS_DRIVER(i2c_eeprom) = {
 	.id		= UCLASS_I2C_EEPROM,
 	.name		= "i2c_eeprom",
+#if CONFIG_IS_ENABLED(NVMEM)
+	.post_bind	= i2c_eeprom_post_bind,
+#endif
 };
diff --git a/drivers/misc/misc-uclass.c b/drivers/misc/misc-uclass.c
index cfe9d562fa..e9b58d3abe 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
@@ -67,10 +68,61 @@ int misc_set_enabled(struct udevice *dev, bool val)
 	return ops->set_enabled(dev, val);
 }
 
+static int misc_nvmem_read(struct udevice *dev, unsigned int offset, void *buf,
+			   size_t size)
+{
+	int ret = misc_read(dev_get_parent(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_get_parent(dev), offset, buf, size);
+
+	if (ret < 0)
+		return ret;
+	if (ret != size)
+		return -EIO;
+	return 0;
+}
+
+static struct __maybe_unused nvmem_ops misc_nvmem_ops = {
+	.read = misc_nvmem_read,
+	.write = misc_nvmem_write,
+};
+
+#if CONFIG_IS_ENABLED(NVMEM)
+U_BOOT_DRIVER(misc_nvmem) = {
+	.name	= "misc_nvmem",
+	.id	= UCLASS_NVMEM,
+	.ops	= &misc_nvmem_ops,
+	.flags	= DM_FLAG_NAME_ALLOCED,
+};
+#endif
+
+static int misc_post_bind(struct udevice *dev)
+{
+#if CONFIG_IS_ENABLED(OF_REAL)
+	int ret = dm_scan_fdt_dev(dev);
+
+	if (ret)
+		return ret;
+#endif
+#if CONFIG_IS_ENABLED(NVMEM)
+	return nvmem_register(dev, DM_DRIVER_GET(misc_nvmem));
+#else
+	return 0;
+#endif
+}
+
 UCLASS_DRIVER(misc) = {
 	.id		= UCLASS_MISC,
 	.name		= "misc",
-#if CONFIG_IS_ENABLED(OF_REAL)
-	.post_bind	= dm_scan_fdt_dev,
-#endif
+	.post_bind	= misc_post_bind,
 };
diff --git a/drivers/misc/nvmem.c b/drivers/misc/nvmem.c
index 5a2bd1f9f7..afd8c7b5ab 100644
--- a/drivers/misc/nvmem.c
+++ b/drivers/misc/nvmem.c
@@ -10,90 +10,48 @@
 #include <nvmem.h>
 #include <rtc.h>
 #include <dm/device_compat.h>
+#include <dm/device-internal.h>
 #include <dm/ofnode.h>
 #include <dm/read.h>
 #include <dm/uclass.h>
 
+int nvmem_register(struct udevice *dev, const struct driver* drv)
+{
+	char *name;
+	int name_size;
+	static const char fmt[] = "%s.nvmem";
+
+	assert(drv->flags & DM_FLAG_NAME_ALLOCED);
+
+	name_size = snprintf(NULL, 0, fmt, dev->name) + 1;
+	name = malloc(name_size);
+	if (!name)
+		return -ENOMEM;
+	snprintf(name, name_size, fmt, dev->name);
+
+	return device_bind(dev, drv, name, NULL, dev_ofnode(dev), NULL);
+}
+
 int nvmem_cell_read(struct nvmem_cell *cell, void *buf, size_t size)
-{
+{	
+	const struct nvmem_ops *ops = dev_get_driver_ops(cell->nvmem);
+
 	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;
-	}
+	return ops->read(cell->nvmem, cell->offset, buf, size);
 }
 
 int nvmem_cell_write(struct nvmem_cell *cell, const void *buf, size_t size)
 {
+	const struct nvmem_ops *ops = dev_get_driver_ops(cell->nvmem);
+
 	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 && ret != -EPFNOSUPPORT)
-			return ret;
-	}
-
-	return -ENODEV;
+	return ops->write(cell->nvmem, cell->offset, buf, size);
 }
 
 int nvmem_cell_get_by_index(struct udevice *dev, int index,
@@ -111,7 +69,8 @@ int nvmem_cell_get_by_index(struct udevice *dev, int index,
 	if (ret)
 		return ret;
 
-	ret = nvmem_get_device(ofnode_get_parent(args.node), cell);
+	ret = uclass_get_device_by_ofnode(UCLASS_NVMEM, ofnode_get_parent(args.node),
+					  &cell->nvmem);
 	if (ret)
 		return ret;
 
@@ -140,3 +99,8 @@ int nvmem_cell_get_by_name(struct udevice *dev, const char *name,
 
 	return nvmem_cell_get_by_index(dev, index, cell);
 }
+
+UCLASS_DRIVER(nvmem) = {
+	.id		= UCLASS_NVMEM,
+	.name		= "nvmem",
+};
diff --git a/drivers/rtc/rtc-uclass.c b/drivers/rtc/rtc-uclass.c
index e5ae6ea4d5..bef2eefecf 100644
--- a/drivers/rtc/rtc-uclass.c
+++ b/drivers/rtc/rtc-uclass.c
@@ -10,6 +10,7 @@
 #include <dm.h>
 #include <errno.h>
 #include <log.h>
+#include <nvmem.h>
 #include <rtc.h>
 
 int dm_rtc_get(struct udevice *dev, struct rtc_time *time)
@@ -173,11 +174,50 @@ int rtc_write32(struct udevice *dev, unsigned int reg, u32 value)
 	return 0;
 }
 
+static int nvmem_rtc_read(struct udevice *dev, unsigned int offset, void *buf,
+			  size_t size)
+{
+	return dm_rtc_read(dev_get_parent(dev), offset, buf, size);
+}
+
+static int nvmem_rtc_write(struct udevice *dev, unsigned int offset,
+			   const void *buf, size_t size)
+{
+	return dm_rtc_write(dev_get_parent(dev), offset, buf, size);
+}
+
+static struct __maybe_unused nvmem_ops rtc_nvmem_ops = {
+	.read = nvmem_rtc_read,
+	.write = nvmem_rtc_write,
+};
+
+#if CONFIG_IS_ENABLED(NVMEM)
+U_BOOT_DRIVER(rtc_nvmem) = {
+	.name	= "rtc_nvmem",
+	.id	= UCLASS_NVMEM,
+	.ops	= &rtc_nvmem_ops,
+	.flags	= DM_FLAG_NAME_ALLOCED,
+};
+#endif
+
+static int rtc_post_bind(struct udevice *dev)
+{
+#if CONFIG_IS_ENABLED(OF_REAL)
+	int ret = dm_scan_fdt_dev(dev);
+
+	if (ret)
+		return ret;
+#endif
+#if CONFIG_IS_ENABLED(NVMEM)
+	return nvmem_register(dev, DM_DRIVER_GET(rtc_nvmem));
+#else
+	return 0;
+#endif
+}
+
 UCLASS_DRIVER(rtc) = {
 	.name		= "rtc",
 	.id		= UCLASS_RTC,
 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
-#if CONFIG_IS_ENABLED(OF_REAL)
-	.post_bind	= dm_scan_fdt_dev,
-#endif
+	.post_bind	= rtc_post_bind,
 };
diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
index 3ba69ad9a0..50877890b3 100644
--- a/include/dm/uclass-id.h
+++ b/include/dm/uclass-id.h
@@ -83,6 +83,7 @@ enum uclass_id {
 	UCLASS_NOP,		/* No-op devices */
 	UCLASS_NORTHBRIDGE,	/* Intel Northbridge / SDRAM controller */
 	UCLASS_NVME,		/* NVM Express device */
+	UCLASS_NVMEM,		/* Non-volatile memory devices */
 	UCLASS_P2SB,		/* (x86) Primary-to-Sideband Bus */
 	UCLASS_PANEL,		/* Display panel, such as an LCD */
 	UCLASS_PANEL_BACKLIGHT,	/* Backlight controller for panel */
diff --git a/include/nvmem.h b/include/nvmem.h
index 2751713a68..230841499d 100644
--- a/include/nvmem.h
+++ b/include/nvmem.h
@@ -6,35 +6,8 @@
 #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 driver;
+struct udevice;
 
 /**
  * struct nvmem_cell - One datum within non-volatile memory
@@ -48,8 +21,6 @@ struct nvmem_cell {
 	size_t size;
 };
 
-struct udevice;
-
 #if CONFIG_IS_ENABLED(NVMEM)
 
 /**
@@ -121,6 +92,22 @@ int nvmem_cell_get_by_index(struct udevice *dev, int index,
 int nvmem_cell_get_by_name(struct udevice *dev, const char *name,
 			   struct nvmem_cell *cell);
 
+/**
+ * nvmem_register() - Register a new nvmem device
+ * @dev: The nvmem device proper
+ * @drv: The nvmem adapter driver
+ *
+ * This registers a child device of @dev as an nvmem device using @drv. The
+ * child device's name will be @dev's name with ".nvmem" appended. @drv should
+ * contain %DM_FLAG_NAME_ALLOCED in its flags.
+ *
+ * Return:
+ * * 0 on success
+ * * -ENOMEM if no memory could be allocated for the name
+ * * A negative error if device_bind() failed
+ */
+int nvmem_register(struct udevice *dev, const struct driver* drv);
+
 #else /* CONFIG_NVMEM */
 
 static inline int nvmem_cell_read(struct nvmem_cell *cell, void *buf, int size)
@@ -146,6 +133,53 @@ static inline int nvmem_cell_get_by_name(struct udevice *dev, const char *name,
 	return -ENOSYS;
 }
 
+static inline int nvmem_register(struct udevice *dev, const struct driver* drv)
+{
+	return 0;
+}
 #endif /* CONFIG_NVMEM */
 
+/**
+ * struct nvmem_ops - Ops implemented by nvmem devices
+ * read: Read a register
+ * write: Write a register
+ */
+struct nvmem_ops {
+	int (*read)(struct udevice *dev, unsigned int offset, void *buf,
+		    size_t size);
+	int (*write)(struct udevice *dev, unsigned int offset, const void *buf,
+		     size_t size);
+};
+
+#if 0 /* for docs only */
+/**
+ * read() - 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
+ */
+int read(struct udevice *dev, unsigned int offset, void *buf, size_t size);
+
+/**
+ * write() - 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
+ */
+int write(struct udevice *dev, unsigned int offset, const void *buf,
+	  size_t size);
+#endif
+
 #endif /* NVMEM_H */
-- 
2.35.1.1320.gc452695387.dirty

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

[Simon Glass]

Hi Sean,

On Fri, 29 Apr 2022 at 13:40, Sean Anderson <sean.anderson@seco.com> wrote:
>
> Hi Simon,
>
> On 4/25/22 11:24 AM, Sean Anderson wrote:
> >
> >
> > On 4/25/22 1:48 AM, Simon Glass wrote:
> >> Hi Sean,
> >>
> >> On Mon, 18 Apr 2022 at 13:37, Sean Anderson <sean.anderson@seco.com> wrote:
> >>>
> >>> This adds support for "nvmem cells" as seen in Linux. The nvmem device
> >>> class in Linux is used for various assorted ROMs and EEPROMs. In this
> >>> sense, it is similar to UCLASS_MISC, but also includes
> >>> UCLASS_I2C_EEPROM, UCLASS_RTC, and UCLASS_MTD. New drivers corresponding
> >>> to a Linux-style nvmem device should be implemented as one of the
> >>> previously-mentioned uclasses. The nvmem API acts as a compatibility
> >>> layer to adapt the (slightly different) APIs of these uclasses. It also
> >>> handles the lookup of nvmem cells.
> >>>
> >>> While nvmem devices can be accessed directly, they are most often used
> >>> by reading/writing contiguous values called "cells". Cells typically
> >>> hold information like calibration, versions, or configuration (such as
> >>> mac addresses).
> >>>
> >>> nvmem devices can specify "cells" in their device tree:
> >>>
> >>>         qfprom: eeprom@700000 {
> >>>                 #address-cells = <1>;
> >>>                 #size-cells = <1>;
> >>>                 reg = <0x00700000 0x100000>;
> >>>
> >>>                 /* ... */
> >>>
> >>>                 tsens_calibration: calib@404 {
> >>>                         reg = <0x404 0x10>;
> >>>                 };
> >>>         };
> >>>
> >>> which can then be referenced like:
> >>>
> >>>         tsens {
> >>>                 /* ... */
> >>>                 nvmem-cells = <&tsens_calibration>;
> >>>                 nvmem-cell-names = "calibration";
> >>>         };
> >>>
> >>> The tsens driver could then read the calibration value like:
> >>>
> >>>         struct nvmem_cell cal_cell;
> >>>         u8 cal[16];
> >>>         nvmem_cell_get_by_name(dev, "calibration", &cal_cell);
> >>>         nvmem_cell_read(&cal_cell, cal, sizeof(cal));
> >>>
> >>> Because nvmem devices are not all of the same uclass, supported uclasses
> >>> must register a nvmem_interface struct. This allows CONFIG_NVMEM to be
> >>> enabled without depending on specific uclasses. At the moment,
> >>> nvmem_interface is very bare-bones, and assumes that no initialization
> >>> is necessary. However, this could be amended in the future.
> >>>
> >>> Although I2C_EEPROM and MISC are quite similar (and could likely be
> >>> unified), they present different read/write function signatures. To
> >>> abstract over this, NVMEM uses the same read/write signature as Linux.
> >>> In particular, short read/writes are not allowed, which is allowed by
> >>> MISC.
> >>>
> >>> The functionality implemented by nvmem cells is very similar to that
> >>> provided by i2c_eeprom_partition. "fixed-partition"s for eeproms does
> >>> not seem to have made its way into Linux or into any device tree other
> >>> than sandbox. It is possible that with the introduction of this API it
> >>> would be possible to remove it.
> >>
> >> I still think this would be better as a separate uclass, with child
> >> devices created at bind time in each of the respective uclasses, like
> >> mmc_bind() does. Then you will see the nvmem devices in the DM tree.
> >> Wouldn't we want to add a command to access the nvmem devices?
> >
> > We already do. E.g. the misc/rtc/eeprom commands. The problem is that
> > for software to access them, they would have to use misc_read/dm_rtc_read/
> > i2c_eeprom_read.
> >
> >> This patch feels like a shortcut to me and I'm not sure of the
> >> benefit of that shortcut.
> > Well, I suppose it's because "nvmem" devices are strict subsets of
> > existing devices. There is no new functionality here (except adapting
> > between semantics like for misc). We should always be able to use the
> > existing API to implement support for a new underlying uclass. There
> > should never be device-specific read/write methods, because we can
> > use the existing read/write uclass methods.
> >
> > What I'm trying to get at is that we sort of already have an nvmem
> > uclass with nvmem devices, they're just not accessible in a uniform
> > way. This series is trying to address the uniformity aspect. But I
> > don't think we need new devices for each nvmem interface, because
> > all they would do would take up ram/rom.
> >
> > --Sean
> >
> > PS. In an ideal world we'd have something like
> >
> > struct nvmem_ops {
> >       read();
> >       write();
> > };
> >
> > struct dm_rtc_ops {
> >       nvmem_ops nvmem;
> >       /* the other ops minus read/write */
> > };
> >
> > int nvmem_read (...) {
> >       struct nvmem_ops *ops = cell->nvmem->ops;
> >       /* ... */
> >
> >       return ops->read(...);
> > }
> >
> > but unfortunately, we already have fragmented implementations.

I don't see that as ideal as it involves a 'nested' API, something we
have avoided so far.

> >
>
> To follow up on this, I've conducted some size experiments. The
> following is the bloat caused by applying the current series on
> sandbox64_defconfig:
>
> add/remove: 8/0 grow/shrink: 7/2 up/down: 1069/-170 (899)
> Function                                     old     new   delta
> nvmem_cell_get_by_index                        -     216    +216
> dm_test_ethaddr                                -     192    +192
> nvmem_cell_write                               -     125    +125
> nvmem_cell_read                                -     125    +125
> nvmem_cell_get_by_name                         -      65     +65
> addr                                           -      64     +64
> sandbox_i2c_rtc_probe                          -      54     +54
> sb_eth_write_hwaddr                           14      57     +43
> sandbox_i2c_eeprom_probe                      70     112     +42
> misc_sandbox_probe                            21      61     +40
> eth_post_probe                               444     484     +40
> _u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
> __func__                                   15147   15163     +16
> data_gz                                    18327   18338     +11
> dsa_pre_probe                                181     185      +4
> sb_eth_of_to_plat                            126      64     -62
> default_environment                          553     445    -108
> Total: Before=1765267, After=1766166, chg +0.05%
>
> And here is the difference (from baseline) when using your
> suggested approach:
>
> add/remove: 26/0 grow/shrink: 8/2 up/down: 2030/-170 (1860)
> Function                                     old     new   delta
> dm_test_ethaddr                                -     192    +192
> nvmem_cell_get_by_index                        -     152    +152
> nvmem_register                                 -     137    +137
> _u_boot_list_2_driver_2_rtc_nvmem              -     128    +128
> _u_boot_list_2_driver_2_misc_nvmem             -     128    +128
> _u_boot_list_2_driver_2_i2c_eeprom_nvmem       -     128    +128
> _u_boot_list_2_uclass_driver_2_nvmem           -     120    +120
> misc_nvmem_write                               -      68     +68
> misc_nvmem_read                                -      68     +68
> nvmem_cell_write                               -      66     +66
> nvmem_cell_read                                -      65     +65
> nvmem_cell_get_by_name                         -      65     +65
> addr                                           -      64     +64
> sandbox_i2c_rtc_probe                          -      54     +54
> rtc_post_bind                                  -      48     +48
> nvmem_rtc_write                                -      48     +48
> nvmem_rtc_read                                 -      48     +48
> misc_post_bind                                 -      48     +48
> i2c_eeprom_nvmem_write                         -      48     +48
> i2c_eeprom_nvmem_read                          -      48     +48
> sb_eth_write_hwaddr                           14      57     +43
> sandbox_i2c_eeprom_probe                      70     112     +42
> misc_sandbox_probe                            21      61     +40
> eth_post_probe                               444     484     +40
> _u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
> rtc_nvmem_ops                                  -      16     +16
> misc_nvmem_ops                                 -      16     +16
> i2c_eeprom_post_bind                           -      16     +16
> i2c_eeprom_nvmem_ops                           -      16     +16
> __func__                                   15147   15163     +16
> data_gz                                    18327   18338     +11
> fmt                                            -       9      +9
> version_string                                68      74      +6
> dsa_pre_probe                                181     185      +4
> sb_eth_of_to_plat                            126      64     -62
> default_environment                          553     445    -108
> Total: Before=1765267, After=1767127, chg +0.11%
>
> As you can see, adding a second driver for each nvmem device
> doubles the size of this feature. The patch I used for this follows
> (it does not apply cleanly to v3 because the base contains some
> changes fixing bugs pointed out by Tom).

Thanks for the analysis and patch. This is what buildman reports for me:

01: test: Load mac address using misc device
   sandbox:  w+   sandbox
02: misc: nvmem: Convert to using udevices
   sandbox: (for 1/1 boards) all +1584.0 data +576.0 rodata +32.0 text +976.0
[..]

So we have text growth of about 1KB on 64-bit x86. The data size is
not that important IMO since this is most likely to be used in U-Boot
proper which doesn't have tight constraints. For that we should
instead focus on reducing the cost of driver model overall, e.g. with
the ideas mentioned at [1].

I didn't try on Thumb2 but I suppose it would be about 0.5KB.

It seems OK to pay this cost to keep things clean.

If we do go ahead with this series (i.e. without the last patch), I
don't think it should be a model for how to add new APIs in future.
E.g. we could save space by making a special case for PMICs which
support GPIOs, so that GPIO operations could accept a PMIC device or a
GPIO device, but it would be quite confusing, We could make the
random-number device disappear and just 'know' that a TPM and a MISC
device can produce random numbers, but I have the same feeling about
that.

Given that you have done the analysis and you are still pretty keen on
this, I am OK with it going in. I don't like it very much. but we can
always review things later. Perhaps add a comment in the nvmem header
files about how it works and why it doesn't use driver model in the
normal way?

Regards,
Simon

[1] https://lore.kernel.org/all/20220327202622.3438333-1-sjg@chromium.org/

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

[Sean Anderson]

Hi Simon,

On 5/3/22 4:50 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Fri, 29 Apr 2022 at 13:40, Sean Anderson <sean.anderson@seco.com> wrote:
>>
>> Hi Simon,
>>
>> On 4/25/22 11:24 AM, Sean Anderson wrote:
>> >
>> >
>> > On 4/25/22 1:48 AM, Simon Glass wrote:
>> >> Hi Sean,
>> >>
>> >> On Mon, 18 Apr 2022 at 13:37, Sean Anderson <sean.anderson@seco.com> wrote:
>> >>>
>> >>> This adds support for "nvmem cells" as seen in Linux. The nvmem device
>> >>> class in Linux is used for various assorted ROMs and EEPROMs. In this
>> >>> sense, it is similar to UCLASS_MISC, but also includes
>> >>> UCLASS_I2C_EEPROM, UCLASS_RTC, and UCLASS_MTD. New drivers corresponding
>> >>> to a Linux-style nvmem device should be implemented as one of the
>> >>> previously-mentioned uclasses. The nvmem API acts as a compatibility
>> >>> layer to adapt the (slightly different) APIs of these uclasses. It also
>> >>> handles the lookup of nvmem cells.
>> >>>
>> >>> While nvmem devices can be accessed directly, they are most often used
>> >>> by reading/writing contiguous values called "cells". Cells typically
>> >>> hold information like calibration, versions, or configuration (such as
>> >>> mac addresses).
>> >>>
>> >>> nvmem devices can specify "cells" in their device tree:
>> >>>
>> >>>         qfprom: eeprom@700000 {
>> >>>                 #address-cells = <1>;
>> >>>                 #size-cells = <1>;
>> >>>                 reg = <0x00700000 0x100000>;
>> >>>
>> >>>                 /* ... */
>> >>>
>> >>>                 tsens_calibration: calib@404 {
>> >>>                         reg = <0x404 0x10>;
>> >>>                 };
>> >>>         };
>> >>>
>> >>> which can then be referenced like:
>> >>>
>> >>>         tsens {
>> >>>                 /* ... */
>> >>>                 nvmem-cells = <&tsens_calibration>;
>> >>>                 nvmem-cell-names = "calibration";
>> >>>         };
>> >>>
>> >>> The tsens driver could then read the calibration value like:
>> >>>
>> >>>         struct nvmem_cell cal_cell;
>> >>>         u8 cal[16];
>> >>>         nvmem_cell_get_by_name(dev, "calibration", &cal_cell);
>> >>>         nvmem_cell_read(&cal_cell, cal, sizeof(cal));
>> >>>
>> >>> Because nvmem devices are not all of the same uclass, supported uclasses
>> >>> must register a nvmem_interface struct. This allows CONFIG_NVMEM to be
>> >>> enabled without depending on specific uclasses. At the moment,
>> >>> nvmem_interface is very bare-bones, and assumes that no initialization
>> >>> is necessary. However, this could be amended in the future.
>> >>>
>> >>> Although I2C_EEPROM and MISC are quite similar (and could likely be
>> >>> unified), they present different read/write function signatures. To
>> >>> abstract over this, NVMEM uses the same read/write signature as Linux.
>> >>> In particular, short read/writes are not allowed, which is allowed by
>> >>> MISC.
>> >>>
>> >>> The functionality implemented by nvmem cells is very similar to that
>> >>> provided by i2c_eeprom_partition. "fixed-partition"s for eeproms does
>> >>> not seem to have made its way into Linux or into any device tree other
>> >>> than sandbox. It is possible that with the introduction of this API it
>> >>> would be possible to remove it.
>> >>
>> >> I still think this would be better as a separate uclass, with child
>> >> devices created at bind time in each of the respective uclasses, like
>> >> mmc_bind() does. Then you will see the nvmem devices in the DM tree.
>> >> Wouldn't we want to add a command to access the nvmem devices?
>> >
>> > We already do. E.g. the misc/rtc/eeprom commands. The problem is that
>> > for software to access them, they would have to use misc_read/dm_rtc_read/
>> > i2c_eeprom_read.
>> >
>> >> This patch feels like a shortcut to me and I'm not sure of the
>> >> benefit of that shortcut.
>> > Well, I suppose it's because "nvmem" devices are strict subsets of
>> > existing devices. There is no new functionality here (except adapting
>> > between semantics like for misc). We should always be able to use the
>> > existing API to implement support for a new underlying uclass. There
>> > should never be device-specific read/write methods, because we can
>> > use the existing read/write uclass methods.
>> >
>> > What I'm trying to get at is that we sort of already have an nvmem
>> > uclass with nvmem devices, they're just not accessible in a uniform
>> > way. This series is trying to address the uniformity aspect. But I
>> > don't think we need new devices for each nvmem interface, because
>> > all they would do would take up ram/rom.
>> >
>> > --Sean
>> >
>> > PS. In an ideal world we'd have something like
>> >
>> > struct nvmem_ops {
>> >       read();
>> >       write();
>> > };
>> >
>> > struct dm_rtc_ops {
>> >       nvmem_ops nvmem;
>> >       /* the other ops minus read/write */
>> > };
>> >
>> > int nvmem_read (...) {
>> >       struct nvmem_ops *ops = cell->nvmem->ops;
>> >       /* ... */
>> >
>> >       return ops->read(...);
>> > }
>> >
>> > but unfortunately, we already have fragmented implementations.
> 
> I don't see that as ideal as it involves a 'nested' API, something we
> have avoided so far.
> 
>> >
>>
>> To follow up on this, I've conducted some size experiments. The
>> following is the bloat caused by applying the current series on
>> sandbox64_defconfig:
>>
>> add/remove: 8/0 grow/shrink: 7/2 up/down: 1069/-170 (899)
>> Function                                     old     new   delta
>> nvmem_cell_get_by_index                        -     216    +216
>> dm_test_ethaddr                                -     192    +192
>> nvmem_cell_write                               -     125    +125
>> nvmem_cell_read                                -     125    +125
>> nvmem_cell_get_by_name                         -      65     +65
>> addr                                           -      64     +64
>> sandbox_i2c_rtc_probe                          -      54     +54
>> sb_eth_write_hwaddr                           14      57     +43
>> sandbox_i2c_eeprom_probe                      70     112     +42
>> misc_sandbox_probe                            21      61     +40
>> eth_post_probe                               444     484     +40
>> _u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
>> __func__                                   15147   15163     +16
>> data_gz                                    18327   18338     +11
>> dsa_pre_probe                                181     185      +4
>> sb_eth_of_to_plat                            126      64     -62
>> default_environment                          553     445    -108
>> Total: Before=1765267, After=1766166, chg +0.05%
>>
>> And here is the difference (from baseline) when using your
>> suggested approach:
>>
>> add/remove: 26/0 grow/shrink: 8/2 up/down: 2030/-170 (1860)
>> Function                                     old     new   delta
>> dm_test_ethaddr                                -     192    +192
>> nvmem_cell_get_by_index                        -     152    +152
>> nvmem_register                                 -     137    +137
>> _u_boot_list_2_driver_2_rtc_nvmem              -     128    +128
>> _u_boot_list_2_driver_2_misc_nvmem             -     128    +128
>> _u_boot_list_2_driver_2_i2c_eeprom_nvmem       -     128    +128
>> _u_boot_list_2_uclass_driver_2_nvmem           -     120    +120
>> misc_nvmem_write                               -      68     +68
>> misc_nvmem_read                                -      68     +68
>> nvmem_cell_write                               -      66     +66
>> nvmem_cell_read                                -      65     +65
>> nvmem_cell_get_by_name                         -      65     +65
>> addr                                           -      64     +64
>> sandbox_i2c_rtc_probe                          -      54     +54
>> rtc_post_bind                                  -      48     +48
>> nvmem_rtc_write                                -      48     +48
>> nvmem_rtc_read                                 -      48     +48
>> misc_post_bind                                 -      48     +48
>> i2c_eeprom_nvmem_write                         -      48     +48
>> i2c_eeprom_nvmem_read                          -      48     +48
>> sb_eth_write_hwaddr                           14      57     +43
>> sandbox_i2c_eeprom_probe                      70     112     +42
>> misc_sandbox_probe                            21      61     +40
>> eth_post_probe                               444     484     +40
>> _u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
>> rtc_nvmem_ops                                  -      16     +16
>> misc_nvmem_ops                                 -      16     +16
>> i2c_eeprom_post_bind                           -      16     +16
>> i2c_eeprom_nvmem_ops                           -      16     +16
>> __func__                                   15147   15163     +16
>> data_gz                                    18327   18338     +11
>> fmt                                            -       9      +9
>> version_string                                68      74      +6
>> dsa_pre_probe                                181     185      +4
>> sb_eth_of_to_plat                            126      64     -62
>> default_environment                          553     445    -108
>> Total: Before=1765267, After=1767127, chg +0.11%
>>
>> As you can see, adding a second driver for each nvmem device
>> doubles the size of this feature. The patch I used for this follows
>> (it does not apply cleanly to v3 because the base contains some
>> changes fixing bugs pointed out by Tom).
> 
> Thanks for the analysis and patch. This is what buildman reports for me:
> 
> 01: test: Load mac address using misc device
>    sandbox:  w+   sandbox
> 02: misc: nvmem: Convert to using udevices
>    sandbox: (for 1/1 boards) all +1584.0 data +576.0 rodata +32.0 text +976.0
> [..]
> 
> So we have text growth of about 1KB on 64-bit x86. The data size is
> not that important IMO since this is most likely to be used in U-Boot
> proper which doesn't have tight constraints. For that we should
> instead focus on reducing the cost of driver model overall, e.g. with
> the ideas mentioned at [1].

Yes, I agree. One of the big problems is that each driver struct takes
128 bytes each. With 3 drivers added (and a uclass driver), that's 512
bytes right from the start. I don't think this is covered by your series.
The current design is very ergonomic, but I don't know if it's the best
space-wise.

Another problem is that each function has to move registers around to set
up the parameters for its two calls:

00000000000a56c4 <i2c_eeprom_nvmem_write>:
   a56c4:	f3 0f 1e fa          	endbr64 
   a56c8:	53                   	push   %rbx
   a56c9:	48 89 cb             	mov    %rcx,%rbx
   a56cc:	48 83 ec 10          	sub    $0x10,%rsp
   a56d0:	89 74 24 0c          	mov    %esi,0xc(%rsp)
   a56d4:	48 89 14 24          	mov    %rdx,(%rsp)
   a56d8:	e8 49 00 fd ff       	callq  75726 <dev_get_parent>
   a56dd:	48 8b 14 24          	mov    (%rsp),%rdx
   a56e1:	8b 74 24 0c          	mov    0xc(%rsp),%esi
   a56e5:	89 d9                	mov    %ebx,%ecx
   a56e7:	48 83 c4 10          	add    $0x10,%rsp
   a56eb:	48 89 c7             	mov    %rax,%rdi
   a56ee:	5b                   	pop    %rbx
   a56ef:	e9 4c ff ff ff       	jmpq   a5640 <i2c_eeprom_write>


I suppose we could shave off ~120 bytes by moving the dev_get_parent
calls to nvmem_cell_read.

> I didn't try on Thumb2 but I suppose it would be about 0.5KB.

add/remove: 20/0 grow/shrink: 0/3 up/down: 731/-100 (631)
Function                                     old     new   delta
dm_rtc_write                                   -      78     +78
nvmem_register                                 -      72     +72
_u_boot_list_2_uclass_driver_2_nvmem           -      72     +72
_u_boot_list_2_driver_2_rtc_nvmem              -      68     +68
_u_boot_list_2_driver_2_misc_nvmem             -      68     +68
_u_boot_list_2_driver_2_i2c_eeprom_nvmem       -      68     +68
misc_nvmem_write                               -      38     +38
misc_nvmem_read                                -      38     +38
rtc_post_bind                                  -      28     +28
misc_post_bind                                 -      28     +28
nvmem_rtc_write                                -      26     +26
nvmem_rtc_read                                 -      26     +26
i2c_eeprom_nvmem_write                         -      26     +26
i2c_eeprom_nvmem_read                          -      26     +26
misc_write                                     -      24     +24
i2c_eeprom_post_bind                           -      12     +12
fmt                                            -       9      +9
rtc_nvmem_ops                                  -       8      +8
misc_nvmem_ops                                 -       8      +8
i2c_eeprom_nvmem_ops                           -       8      +8
version_string                                74      68      -6
nvmem_cell_read                               96      50     -46
nvmem_cell_get_by_index                      144      96     -48
Total: Before=362129, After=362760, chg +0.17%

> It seems OK to pay this cost to keep things clean.

It's not terribly large, but I would like to try and keep the size of
new features down. I'd like to make it easy to choose to use NVMEM
instead of e.g. mac_read_from_eeprom

> If we do go ahead with this series (i.e. without the last patch), I
> don't think it should be a model for how to add new APIs in future.
> E.g. we could save space by making a special case for PMICs which
> support GPIOs, so that GPIO operations could accept a PMIC device or a
> GPIO device, but it would be quite confusing, We could make the
> random-number device disappear and just 'know' that a TPM and a MISC
> device can produce random numbers, but I have the same feeling about
> that.

I agree. I think this is a bit of a special case because of how we
already have several APIs all implementing the same idea.

> Given that you have done the analysis and you are still pretty keen on
> this, I am OK with it going in. I don't like it very much. but we can
> always review things later. Perhaps add a comment in the nvmem header
> files about how it works and why it doesn't use driver model in the
> normal way?

Sure. I will also include the patch from before as an RFC on the end of the series.

--Sean

[PATCH v3 09/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 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 d7f22b39ae..fc7e100eaa 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -144,6 +144,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 06dbf38dcc..03fe6b7b85 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -185,6 +185,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 2e530ac887..5130aa4a30 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 c9430da0f0..e9cb299538 100644
--- a/configs/sandbox_noinst_defconfig
+++ b/configs/sandbox_noinst_defconfig
@@ -143,6 +143,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 13a76e89ea..1a1bc4058f 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -144,6 +144,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.35.1.1320.gc452695387.dirty

[PATCH v3 10/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 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.35.1.1320.gc452695387.dirty

Re: [PATCH v3 10/13] net: Add support for reading mac addresses from nvmem cells

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

On Mon, Apr 18, 2022 at 03:36:56PM -0400, Sean Anderson 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>

So, this breaks sandbox_noinst as drivers/misc/i2c_eeprom_emul.c does
not provide i2c_eeprom_read(...) and we fail to link.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v3 11/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 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 23a4e1f078..8e16d3cf79 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -523,7 +523,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 {
@@ -686,6 +687,8 @@
 		pinctrl-0 = <&pinmux_i2c0_pins>;
 
 		eeprom@2c {
+			#address-cells = <1>;
+			#size-cells = <1>;
 			reg = <0x2c>;
 			compatible = "i2c-eeprom";
 			sandbox,emul = <&emul_eeprom>;
@@ -697,6 +700,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.35.1.1320.gc452695387.dirty

[PATCH v3 12/13] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---

(no changes since v1)

 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 8e16d3cf79..64f8edca7b 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -543,7 +543,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 {
@@ -707,9 +708,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.35.1.1320.gc452695387.dirty

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

On Mon, Apr 18, 2022 at 03:36:58PM -0400, Sean Anderson wrote:

> This uses the nvmem API to load a mac address from an RTC.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  arch/sandbox/dts/test.dts  |  9 ++++++++-
>  drivers/rtc/i2c_rtc_emul.c | 10 ++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)

This breaks sandbox_spl testing, and most easily seen with the "handoff"
test:
------------------------------------ Captured stdout setup ------------------------------------
/spl/u-boot-spl
Creating new bloblist size 400 at c000

U-Boot SPL 2022.07-rc1-00103-g5e422746a1d3 (Apr 29 2022 - 10:45:23 -0400)
Finished bloblist size 400 at c000
Finished bloblist size 400 at c000
Found existing bloblist size 400 at c000


U-Boot 2022.07-rc1-00103-g5e422746a1d3 (Apr 29 2022 - 10:45:23 -0400)

Model: sandbox
DRAM:  128 MiB
Core:  214 devices, 79 uclasses, devicetree: board
MMC:   Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'
Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'
mmc1 - probe failed: -1
mmc2: 2 (SD)Can't map file 'mmc1.img': Invalid argument
mmc1: Unable to map file 'mmc1.img'

Loading Environment from nowhere... OK
In:    cros-ec-keyb
Out:   vidconsole
Err:   vidconsole
Model: sandbox
Net:   eth0: eth@10002000, eth5: eth@10003000
Error: sbe5 address not set.
, eth6: eth@10004000, eth8: phy-test-eth
Error: dsa-test-eth address not set.
=================================== short test summary info ===================================

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v3 13/13] 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>
---

(no changes since v1)

 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 64f8edca7b..941c79c6d8 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -517,7 +517,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 {
@@ -898,7 +899,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.35.1.1320.gc452695387.dirty