[PATCH 0/2] net: of: Support minor nvmem MAC offset

[PATCH 0/2] net: of: Support minor nvmem MAC offset

[Linus Walleij]

In practice (as found in the OpenWrt project) many devices
with multiple ethernet interfaces just store a base MAC
address in NVMEM and increase the lowermost byte with one for
each interface, so as to occupy less NVMEM.

Here is an example patch from the Linksys WRT300N router that
was used before we had device tree:

f = ioremap(IXP4XX_EXP_BUS_BASE(0), 0x60000);
if (f) {
    for (i = 0; i < 6; i++) {
        wrt300nv2_plat_eth[0].hwaddr[i] = readb(f + 0x5FFA0 + i);
        if (i == 5)
            offset = 1;
        wrt300nv2_plat_eth[1].hwaddr[i] =
	    (wrt300nv2_plat_eth[0].hwaddr[i] + offset);
    }
}
iounmap(f);

In order to support this scheme directly from device tree
we need some way to encode the same into device tree, this
patchset provides that.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Linus Walleij (2):
      dt-bindings: net: ethernet-controller: Add mac offset option
      net: of: Support adding offset to nvmem MAC addresses

 .../devicetree/bindings/net/ethernet-controller.yaml         | 12 ++++++++++++
 net/core/of_net.c                                            | 10 ++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241219-net-mac-nvmem-offset-22b6218a4b0f

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH 1/2] dt-bindings: net: ethernet-controller: Add mac offset option

[Linus Walleij]

In practice (as found in the OpenWrt project) many devices
with multiple ethernet interfaces just store a base MAC
address in NVMEM and increase the lowermost byte with one for
each interface, so as to occupy less NVMEM.

Support this with a per-interface offset so we can encode
this in a predictable way for each interface sharing the
same NVMEM cell.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/net/ethernet-controller.yaml         | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 45819b2358002bc75e876eddb4b2ca18017c04bd..608f89359ca844e5325e3cc81bd2677b0eccb08a 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -53,6 +53,18 @@ properties:
   nvmem-cell-names:
     const: mac-address
 
+  nvmem-mac-minor-offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 255
+    description:
+      When a MAC address for a device with multiple ethernet interface
+      is stored in non-volatile memory, the address is often offsetted
+      for different interfaces by increasing the lowermost byte for each
+      interface subsequent to the first in order to save space in NVMEM.
+      This property can be used to add that offset when several
+      interfaces refer to the same NVMEM cell.
+
   phy-connection-type:
     description:
       Specifies interface type between the Ethernet device and a physical

-- 
2.47.1

Re: [PATCH 1/2] dt-bindings: net: ethernet-controller: Add mac offset option

[Rob Herring]

On Fri, Dec 20, 2024 at 08:17:06PM +0100, Linus Walleij wrote:
> In practice (as found in the OpenWrt project) many devices
> with multiple ethernet interfaces just store a base MAC
> address in NVMEM and increase the lowermost byte with one for
> each interface, so as to occupy less NVMEM.
> 
> Support this with a per-interface offset so we can encode
> this in a predictable way for each interface sharing the
> same NVMEM cell.

This has come up several times before[1][2][3]. Based on those I know 
this is not sufficient with the different variations of how MAC 
addresses are shared. OTOH, I don't think a bunch of properties to deal 
with all the possible transforms works either. It will be one of those 
cases of properties added one-by-one where we end up with something 
poorly designed. I think probably we want to just enumerate different 
schemes and leave it to code to deal with each scheme.

Or we could just say it is the bootloader's problem to figure this out 
and populate the DT using the existing properties for MAC addresses. 
Though bootloaders want to use DT too...

Rob

[1] https://lore.kernel.org/linux-devicetree/20230509143504.30382-4-fr0st61te@gmail.com/
[2] https://lore.kernel.org/linux-devicetree/20211123134425.3875656-1-michael@walle.cc/
[3] https://lore.kernel.org/linux-devicetree/20200919214941.8038-5-ansuelsmth@gmail.com/

Re: [PATCH 1/2] dt-bindings: net: ethernet-controller: Add mac offset option

[Linus Walleij]

On Tue, Dec 31, 2024 at 2:35 PM Rob Herring <robh@kernel.org> wrote:
> On Fri, Dec 20, 2024 at 08:17:06PM +0100, Linus Walleij wrote:

> > In practice (as found in the OpenWrt project) many devices
> > with multiple ethernet interfaces just store a base MAC
> > address in NVMEM and increase the lowermost byte with one for
> > each interface, so as to occupy less NVMEM.
> >
> > Support this with a per-interface offset so we can encode
> > this in a predictable way for each interface sharing the
> > same NVMEM cell.
>
> This has come up several times before[1][2][3]. Based on those I know
> this is not sufficient with the different variations of how MAC
> addresses are shared. OTOH, I don't think a bunch of properties to deal
> with all the possible transforms works either. It will be one of those
> cases of properties added one-by-one where we end up with something
> poorly designed. I think probably we want to just enumerate different
> schemes and leave it to code to deal with each scheme.

The problem here is that the code needs some handle on which
ethernet instance we are dealing with so the bindings need some
way to pass that along from the consumer.

What about a third, implementation-defined nvmem cell?
#mac-index-cells = <1>; or however we best deal with
this.

If it really is per-machine then maybe this is simply one of those
cases where the kernel should:

if (IS_ENABLED(CONFIG_ARCH_FOO) &&
   of_machine_is_compatible("foo,bar-machine)) {
    // Read third cell if present
    // Add to minor mac address
}

> Or we could just say it is the bootloader's problem to figure this out
> and populate the DT using the existing properties for MAC addresses.
> Though bootloaders want to use DT too...

In my current case it's so fantastically organized that if the bootloader
goes into TFTP boot it will use *another* unique MAC address.
(Yes, it's fantastic.)

Yours,
Linus Walleij

Re: [PATCH 1/2] dt-bindings: net: ethernet-controller: Add mac offset option

[Rob Herring]

On Mon, Jan 13, 2025 at 03:59:24PM +0100, Linus Walleij wrote:
> On Tue, Dec 31, 2024 at 2:35 PM Rob Herring <robh@kernel.org> wrote:
> > On Fri, Dec 20, 2024 at 08:17:06PM +0100, Linus Walleij wrote:
> 
> > > In practice (as found in the OpenWrt project) many devices
> > > with multiple ethernet interfaces just store a base MAC
> > > address in NVMEM and increase the lowermost byte with one for
> > > each interface, so as to occupy less NVMEM.
> > >
> > > Support this with a per-interface offset so we can encode
> > > this in a predictable way for each interface sharing the
> > > same NVMEM cell.
> >
> > This has come up several times before[1][2][3]. Based on those I know
> > this is not sufficient with the different variations of how MAC
> > addresses are shared. OTOH, I don't think a bunch of properties to deal
> > with all the possible transforms works either. It will be one of those
> > cases of properties added one-by-one where we end up with something
> > poorly designed. I think probably we want to just enumerate different
> > schemes and leave it to code to deal with each scheme.
> 
> The problem here is that the code needs some handle on which
> ethernet instance we are dealing with so the bindings need some
> way to pass that along from the consumer.
> 
> What about a third, implementation-defined nvmem cell?
> #mac-index-cells = <1>; or however we best deal with
> this.

We have #nvmem-cells-cells, doesn't that work?

> If it really is per-machine then maybe this is simply one of those
> cases where the kernel should:
> 
> if (IS_ENABLED(CONFIG_ARCH_FOO) &&
>    of_machine_is_compatible("foo,bar-machine)) {
>     // Read third cell if present
>     // Add to minor mac address
> }

Where would that go? I think it needs to be in the nvmem driver because 
that is what knows the format of the data and the transform needed.

> 
> > Or we could just say it is the bootloader's problem to figure this out
> > and populate the DT using the existing properties for MAC addresses.
> > Though bootloaders want to use DT too...
> 
> In my current case it's so fantastically organized that if the bootloader
> goes into TFTP boot it will use *another* unique MAC address.
> (Yes, it's fantastic.)

Fun.

Rob

[PATCH 2/2] net: of: Support adding offset to nvmem MAC addresses

[Linus Walleij]

If a lower-byte MAC address offset is encoded into the device
tree, make sure to add this to the returned address when
looking up a MAC address from NVMEM.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 net/core/of_net.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/core/of_net.c b/net/core/of_net.c
index 93ea425b9248a23f4f95a336e9cdbf0053248e32..c98000ec13377ea4b541e182a66be8b1010edc40 100644
--- a/net/core/of_net.c
+++ b/net/core/of_net.c
@@ -64,6 +64,7 @@ int of_get_mac_address_nvmem(struct device_node *np, u8 *addr)
 	struct nvmem_cell *cell;
 	const void *mac;
 	size_t len;
+	u32 offset;
 	int ret;
 
 	/* Try lookup by device first, there might be a nvmem_cell_lookup
@@ -72,7 +73,7 @@ int of_get_mac_address_nvmem(struct device_node *np, u8 *addr)
 	if (pdev) {
 		ret = nvmem_get_mac_address(&pdev->dev, addr);
 		put_device(&pdev->dev);
-		return ret;
+		goto add_offset_exit;
 	}
 
 	cell = of_nvmem_cell_get(np, "mac-address");
@@ -92,8 +93,13 @@ int of_get_mac_address_nvmem(struct device_node *np, u8 *addr)
 
 	memcpy(addr, mac, ETH_ALEN);
 	kfree(mac);
+	ret = 0;
 
-	return 0;
+add_offset_exit:
+	if (!ret && !of_property_read_u32(np, "nvmem-mac-minor-offset", &offset))
+		addr[ETH_ALEN - 1] += offset;
+
+	return ret;
 }
 EXPORT_SYMBOL(of_get_mac_address_nvmem);
 

-- 
2.47.1