[PATCH 0/3] net: Add a CONFIG_NET_BOARD_ETHADDR

Basically, the MAC address generation needs to use a board function in its decision algorithm so that there is no MAC mismatch between ROM and environment depending on the order of function calling.
See the first commit message for details.
Detlev Casanova (3): net: Add a CONFIG_NET_BOARD_ETHADDR rockchip: Add a board_gen_ethaddr() function net: eth-uclass: Add driver source possibility
arch/arm/Kconfig | 1 + arch/arm/include/asm/arch-rockchip/misc.h | 1 + arch/arm/mach-rockchip/board.c | 30 ++++++++++++++++++----- arch/arm/mach-rockchip/misc.c | 22 ++++++++++++----- net/Kconfig | 7 ++++++ net/eth-uclass.c | 25 ++++++++++++++++--- 6 files changed, 70 insertions(+), 16 deletions(-)

On some boards, a MAC address is set based on the CPU ID or other information. This is usually done in the misc_init_r() function.
This becomes a problem for net devices that are probed after the call to misc_init_r(), for example, when the ethernet is on a PCI port, which needs to be enumerated.
In this case, misc_init_r() will set the ethaddr variable, then, when the ethernet device is probed, if it has a ROM address, u-boot will warn about a MAC address mismatch and use the misc_init_r() address instead of the one in ROM.
The operating system later will most likely use the ROM MAC address, which can be confusing.
To avoid that, this commit introduces a CONFIG_NET_BOARD_ETHADDR that allows board files to implement a function to set an ethaddr in the environment, that will only be called when necessary. The logic is now:
- If there is there an ethaddr env var, use it. - If not, if there is a DT MAC address, use it. - If not, if there is a ROM MAC address, use it. - If not, if CONFIG_NET_BOARD_ETHADDR, call board_gen_ethaddr() and use it. - If not, if CONFIG_NET_RANDOM_ETHADDR, generate random MAC - If not, fail with No valid MAC address found
Signed-off-by: Detlev Casanova detlev.casanova@collabora.com --- net/Kconfig | 7 +++++++ net/eth-uclass.c | 17 +++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/net/Kconfig b/net/Kconfig index 5dff6336293..6dd333ddb9e 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -54,6 +54,13 @@ config NET_RANDOM_ETHADDR generated. It will be saved to the appropriate environment variable, too.
+config NET_BOARD_ETHADDR + bool "Board specific ethaddr if unset" + help + Allow a board function to set a specific Ethernet address that can + be, e.g., based on the CPU ID. If set, this will be tried before + setting a random address (if set). + config NETCONSOLE bool "NetConsole support" help diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 3d0ec91dfa4..f194df8512a 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -56,6 +56,12 @@ __weak int board_interface_eth_init(struct udevice *dev, return 0; }
+/* board-specific MAC Address generation. */ +__weak int board_gen_ethaddr(int dev_num, u8 *mac_addr) +{ + return 0; +} + static struct eth_uclass_priv *eth_get_uclass_priv(void) { struct uclass *uc; @@ -563,13 +569,20 @@ static int eth_post_probe(struct udevice *dev) if (!eth_dev_get_mac_address(dev, pdata->enetaddr) || !is_valid_ethaddr(pdata->enetaddr)) { /* Check if the device has a MAC address in ROM */ + int ret = -1; if (eth_get_ops(dev)->read_rom_hwaddr) { - int ret; - ret = eth_get_ops(dev)->read_rom_hwaddr(dev); if (!ret) source = "ROM"; } + if (IS_ENABLED(CONFIG_NET_BOARD_ETHADDR) && ret) { + board_gen_ethaddr(dev_seq(dev), pdata->enetaddr); + + if (!is_zero_ethaddr(pdata->enetaddr) && + is_valid_ethaddr(pdata->enetaddr)) { + source = "board"; + } + } }
eth_env_get_enetaddr_by_index("eth", dev_seq(dev), env_enetaddr);

On 3/14/24 3:43 PM, Detlev Casanova wrote:
On some boards, a MAC address is set based on the CPU ID or other information. This is usually done in the misc_init_r() function.
This becomes a problem for net devices that are probed after the call to misc_init_r(), for example, when the ethernet is on a PCI port, which needs to be enumerated.
In this case, misc_init_r() will set the ethaddr variable, then, when the ethernet device is probed, if it has a ROM address, u-boot will warn about a MAC address mismatch and use the misc_init_r() address instead of the one in ROM.
The operating system later will most likely use the ROM MAC address, which can be confusing.
To avoid that, this commit introduces a CONFIG_NET_BOARD_ETHADDR that allows board files to implement a function to set an ethaddr in the environment, that will only be called when necessary. The logic is now:
- If there is there an ethaddr env var, use it.
- If not, if there is a DT MAC address, use it.
- If not, if there is a ROM MAC address, use it.
- If not, if CONFIG_NET_BOARD_ETHADDR, call board_gen_ethaddr() and use it.
- If not, if CONFIG_NET_RANDOM_ETHADDR, generate random MAC
- If not, fail with No valid MAC address found
Signed-off-by: Detlev Casanova detlev.casanova@collabora.com
net/Kconfig | 7 +++++++ net/eth-uclass.c | 17 +++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/net/Kconfig b/net/Kconfig index 5dff6336293..6dd333ddb9e 100644 --- a/net/Kconfig +++ b/net/Kconfig @@ -54,6 +54,13 @@ config NET_RANDOM_ETHADDR generated. It will be saved to the appropriate environment variable, too.
+config NET_BOARD_ETHADDR
- bool "Board specific ethaddr if unset"
- help
Allow a board function to set a specific Ethernet address that can
be, e.g., based on the CPU ID. If set, this will be tried before
setting a random address (if set).
- config NETCONSOLE bool "NetConsole support" help
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index 3d0ec91dfa4..f194df8512a 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -56,6 +56,12 @@ __weak int board_interface_eth_init(struct udevice *dev, return 0; }
+/* board-specific MAC Address generation. */ +__weak int board_gen_ethaddr(int dev_num, u8 *mac_addr) +{
- return 0;
+}
- static struct eth_uclass_priv *eth_get_uclass_priv(void) { struct uclass *uc;
@@ -563,13 +569,20 @@ static int eth_post_probe(struct udevice *dev) if (!eth_dev_get_mac_address(dev, pdata->enetaddr) || !is_valid_ethaddr(pdata->enetaddr)) { /* Check if the device has a MAC address in ROM */
if (eth_get_ops(dev)->read_rom_hwaddr) {int ret = -1;
int ret;
}ret = eth_get_ops(dev)->read_rom_hwaddr(dev); if (!ret) source = "ROM";
if (IS_ENABLED(CONFIG_NET_BOARD_ETHADDR) && ret) {
board_gen_ethaddr(dev_seq(dev), pdata->enetaddr);
You do need to pass in the udevice *dev directly, otherwise this DM uclass patch would behave like non-DM code.
I'd personally just create a __weak board_gen_ethaddr() function here and let boards override it, without new Kconfig symbol.

Set the MAC address based on the CPU ID only if the ethernet device has no ROM or DT address set.
Signed-off-by: Detlev Casanova detlev.casanova@collabora.com --- arch/arm/Kconfig | 1 + arch/arm/include/asm/arch-rockchip/misc.h | 1 + arch/arm/mach-rockchip/board.c | 30 ++++++++++++++++++----- arch/arm/mach-rockchip/misc.c | 22 ++++++++++++----- 4 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 01d6556c42b..21b41675ef6 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -2003,6 +2003,7 @@ config ARCH_ROCKCHIP select DM_SPI_FLASH select DM_USB_GADGET if USB_DWC3_GADGET select ENABLE_ARM_SOC_BOOT0_HOOK + select NET_BOARD_ETHADDR select OF_CONTROL select MTD select SPI diff --git a/arch/arm/include/asm/arch-rockchip/misc.h b/arch/arm/include/asm/arch-rockchip/misc.h index 4155af8c3b0..6e972de6279 100644 --- a/arch/arm/include/asm/arch-rockchip/misc.h +++ b/arch/arm/include/asm/arch-rockchip/misc.h @@ -10,5 +10,6 @@ int rockchip_cpuid_from_efuse(const u32 cpuid_offset, const u32 cpuid_length, u8 *cpuid); int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length); +int rockchip_gen_macaddr(int dev_num, u8 *mac_addr); int rockchip_setup_macaddr(void); void rockchip_capsule_update_board_setup(void); diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 2620530e03f..283d3b9ed3a 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -296,8 +296,8 @@ int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) } #endif
-#ifdef CONFIG_MISC_INIT_R -__weak int misc_init_r(void) +#if IS_ENABLED(CONFIG_MISC_INIT_R) || IS_ENABLED(CONFIG_NET_BOARD_ETHADDR) +static int set_cpuid(void) { const u32 cpuid_offset = CFG_CPUID_OFFSET; const u32 cpuid_length = 0x10; @@ -309,10 +309,6 @@ __weak int misc_init_r(void) return ret;
ret = rockchip_cpuid_set(cpuid, cpuid_length); - if (ret) - return ret; - - ret = rockchip_setup_macaddr();
return ret; } @@ -349,3 +345,25 @@ __weak int board_rng_seed(struct abuf *buf) return 0; } #endif + +#if IS_ENABLED(CONFIG_MISC_INIT_R) +__weak int misc_init_r(void) +{ + return set_cpuid(); +} +#endif + +int board_gen_ethaddr(int dev_num, u8 *mac_addr) +{ + if (!IS_ENABLED(CONFIG_NET_BOARD_ETHADDR)) + return 0; + + if (!env_get("cpuid#")) { + int err = set_cpuid(); + + if (err) + return err; + } + + return rockchip_gen_macaddr(dev_num, mac_addr); +} diff --git a/arch/arm/mach-rockchip/misc.c b/arch/arm/mach-rockchip/misc.c index 7d03f0c2b67..9c7b04ee5a8 100644 --- a/arch/arm/mach-rockchip/misc.c +++ b/arch/arm/mach-rockchip/misc.c @@ -21,14 +21,13 @@
#include <asm/arch-rockchip/misc.h>
-int rockchip_setup_macaddr(void) +int rockchip_gen_macaddr(int dev_num, u8 *mac_addr) { #if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256) int ret; const char *cpuid = env_get("cpuid#"); u8 hash[SHA256_SUM_LEN]; int size = sizeof(hash); - u8 mac_addr[6];
/* Only generate a MAC address, if none is set in the environment */ if (env_get("ethaddr")) @@ -51,15 +50,26 @@ int rockchip_setup_macaddr(void) /* Make this a valid MAC address and set it */ mac_addr[0] &= 0xfe; /* clear multicast bit */ mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */ - eth_env_set_enetaddr("ethaddr", mac_addr);
- /* Make a valid MAC address for ethernet1 */ - mac_addr[5] ^= 0x01; - eth_env_set_enetaddr("eth1addr", mac_addr); + /* Make a valid MAC address for the given device number */ + mac_addr[5] ^= dev_num; #endif return 0; }
+int rockchip_setup_macaddr(void) +{ + u8 mac_addr[6]; + + if (rockchip_gen_macaddr(0, mac_addr) == 0) + eth_env_set_enetaddr("ethaddr", mac_addr); + + if (rockchip_gen_macaddr(1, mac_addr) == 0) + eth_env_set_enetaddr("eth1addr", mac_addr); + + return 0; +} + int rockchip_cpuid_from_efuse(const u32 cpuid_offset, const u32 cpuid_length, u8 *cpuid)

On 3/14/24 3:43 PM, Detlev Casanova wrote:
Set the MAC address based on the CPU ID only if the ethernet device has no ROM or DT address set.
Signed-off-by: Detlev Casanova detlev.casanova@collabora.com
More of a general design question -- how much of this can be done in MAC driver specific .read_rom_hwaddr callback ?

Hi Detlev,
On 2024-03-14 15:43, Detlev Casanova wrote:
Set the MAC address based on the CPU ID only if the ethernet device has no ROM or DT address set.
This patch changes behavior and once again require CONFIG_NET to fixup the device tree with local-mac-address for the ethernet0/1 alias nodes.
I.e. reverts one of the intentions with the commit 628fb0683b65 ("rockchip: misc: Set eth1addr mac address"), fixup of ethaddr in DT should not depend on enabled and working ethernet in U-Boot.
Maybe just having a Kconfig to control if ENV ethaddr should overwrite ROM ethaddr could as easily solve your issue?
Please also note that misc.c is merging into board.c in another pending series, see custodian u-boot-rockchip/for-next branch.
Signed-off-by: Detlev Casanova detlev.casanova@collabora.com
arch/arm/Kconfig | 1 + arch/arm/include/asm/arch-rockchip/misc.h | 1 + arch/arm/mach-rockchip/board.c | 30 ++++++++++++++++++----- arch/arm/mach-rockchip/misc.c | 22 ++++++++++++----- 4 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 01d6556c42b..21b41675ef6 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -2003,6 +2003,7 @@ config ARCH_ROCKCHIP select DM_SPI_FLASH select DM_USB_GADGET if USB_DWC3_GADGET select ENABLE_ARM_SOC_BOOT0_HOOK
- select NET_BOARD_ETHADDR
You are selecting here, so why the need for IS_ENABLED checks below?
select OF_CONTROL select MTD select SPI diff --git a/arch/arm/include/asm/arch-rockchip/misc.h b/arch/arm/include/asm/arch-rockchip/misc.h index 4155af8c3b0..6e972de6279 100644 --- a/arch/arm/include/asm/arch-rockchip/misc.h +++ b/arch/arm/include/asm/arch-rockchip/misc.h @@ -10,5 +10,6 @@ int rockchip_cpuid_from_efuse(const u32 cpuid_offset, const u32 cpuid_length, u8 *cpuid); int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length); +int rockchip_gen_macaddr(int dev_num, u8 *mac_addr); int rockchip_setup_macaddr(void); void rockchip_capsule_update_board_setup(void); diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 2620530e03f..283d3b9ed3a 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -296,8 +296,8 @@ int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) } #endif
-#ifdef CONFIG_MISC_INIT_R -__weak int misc_init_r(void) +#if IS_ENABLED(CONFIG_MISC_INIT_R) || IS_ENABLED(CONFIG_NET_BOARD_ETHADDR)
NET_BOARD_ETHADDR is selected so this #if will always be true.
+static int set_cpuid(void) { const u32 cpuid_offset = CFG_CPUID_OFFSET; const u32 cpuid_length = 0x10; @@ -309,10 +309,6 @@ __weak int misc_init_r(void) return ret;
ret = rockchip_cpuid_set(cpuid, cpuid_length);
if (ret)
return ret;
ret = rockchip_setup_macaddr();
return ret;
} @@ -349,3 +345,25 @@ __weak int board_rng_seed(struct abuf *buf) return 0; } #endif
+#if IS_ENABLED(CONFIG_MISC_INIT_R) +__weak int misc_init_r(void) +{
- return set_cpuid();
+} +#endif
+int board_gen_ethaddr(int dev_num, u8 *mac_addr) +{
- if (!IS_ENABLED(CONFIG_NET_BOARD_ETHADDR))
return 0;
NET_BOARD_ETHADDR is selected so this if return 0 is dead code?
Regards, Jonas
- if (!env_get("cpuid#")) {
int err = set_cpuid();
if (err)
return err;
- }
- return rockchip_gen_macaddr(dev_num, mac_addr);
+} diff --git a/arch/arm/mach-rockchip/misc.c b/arch/arm/mach-rockchip/misc.c index 7d03f0c2b67..9c7b04ee5a8 100644 --- a/arch/arm/mach-rockchip/misc.c +++ b/arch/arm/mach-rockchip/misc.c @@ -21,14 +21,13 @@
#include <asm/arch-rockchip/misc.h>
-int rockchip_setup_macaddr(void) +int rockchip_gen_macaddr(int dev_num, u8 *mac_addr) { #if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256) int ret; const char *cpuid = env_get("cpuid#"); u8 hash[SHA256_SUM_LEN]; int size = sizeof(hash);
u8 mac_addr[6];
/* Only generate a MAC address, if none is set in the environment */ if (env_get("ethaddr"))
@@ -51,15 +50,26 @@ int rockchip_setup_macaddr(void) /* Make this a valid MAC address and set it */ mac_addr[0] &= 0xfe; /* clear multicast bit */ mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */
eth_env_set_enetaddr("ethaddr", mac_addr);
/* Make a valid MAC address for ethernet1 */
mac_addr[5] ^= 0x01;
eth_env_set_enetaddr("eth1addr", mac_addr);
- /* Make a valid MAC address for the given device number */
- mac_addr[5] ^= dev_num;
#endif return 0; }
+int rockchip_setup_macaddr(void) +{
- u8 mac_addr[6];
- if (rockchip_gen_macaddr(0, mac_addr) == 0)
eth_env_set_enetaddr("ethaddr", mac_addr);
- if (rockchip_gen_macaddr(1, mac_addr) == 0)
eth_env_set_enetaddr("eth1addr", mac_addr);
- return 0;
+}
int rockchip_cpuid_from_efuse(const u32 cpuid_offset, const u32 cpuid_length, u8 *cpuid)

Hi Jonas,
On Thursday, March 14, 2024 2:38:30 P.M. EDT Jonas Karlman wrote:
Hi Detlev,
On 2024-03-14 15:43, Detlev Casanova wrote:
Set the MAC address based on the CPU ID only if the ethernet device has no ROM or DT address set.
This patch changes behavior and once again require CONFIG_NET to fixup the device tree with local-mac-address for the ethernet0/1 alias nodes.
I.e. reverts one of the intentions with the commit 628fb0683b65 ("rockchip: misc: Set eth1addr mac address"), fixup of ethaddr in DT should not depend on enabled and working ethernet in U-Boot.
I see the intention, that makes sense.
Maybe just having a Kconfig to control if ENV ethaddr should overwrite ROM ethaddr could as easily solve your issue?
If that is ok for everybody, it would indeed be enough for this issue and keep the eth driver less complex.
Please also note that misc.c is merging into board.c in another pending series, see custodian u-boot-rockchip/for-next branch.
Signed-off-by: Detlev Casanova detlev.casanova@collabora.com
arch/arm/Kconfig | 1 + arch/arm/include/asm/arch-rockchip/misc.h | 1 + arch/arm/mach-rockchip/board.c | 30 ++++++++++++++++++----- arch/arm/mach-rockchip/misc.c | 22 ++++++++++++----- 4 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 01d6556c42b..21b41675ef6 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -2003,6 +2003,7 @@ config ARCH_ROCKCHIP
select DM_SPI_FLASH select DM_USB_GADGET if USB_DWC3_GADGET select ENABLE_ARM_SOC_BOOT0_HOOK
- select NET_BOARD_ETHADDR
You are selecting here, so why the need for IS_ENABLED checks below?
select OF_CONTROL select MTD select SPI
diff --git a/arch/arm/include/asm/arch-rockchip/misc.h b/arch/arm/include/asm/arch-rockchip/misc.h index 4155af8c3b0..6e972de6279 100644 --- a/arch/arm/include/asm/arch-rockchip/misc.h +++ b/arch/arm/include/asm/arch-rockchip/misc.h @@ -10,5 +10,6 @@ int rockchip_cpuid_from_efuse(const u32 cpuid_offset,
const u32 cpuid_length, u8 *cpuid);
int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length);
+int rockchip_gen_macaddr(int dev_num, u8 *mac_addr);
int rockchip_setup_macaddr(void); void rockchip_capsule_update_board_setup(void);
diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 2620530e03f..283d3b9ed3a 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -296,8 +296,8 @@ int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason)> } #endif
-#ifdef CONFIG_MISC_INIT_R -__weak int misc_init_r(void) +#if IS_ENABLED(CONFIG_MISC_INIT_R) || IS_ENABLED(CONFIG_NET_BOARD_ETHADDR)
NET_BOARD_ETHADDR is selected so this #if will always be true.
+static int set_cpuid(void)
{
const u32 cpuid_offset = CFG_CPUID_OFFSET; const u32 cpuid_length = 0x10;
@@ -309,10 +309,6 @@ __weak int misc_init_r(void)
return ret;
ret = rockchip_cpuid_set(cpuid, cpuid_length);
if (ret)
return ret;
ret = rockchip_setup_macaddr();
return ret;
}
@@ -349,3 +345,25 @@ __weak int board_rng_seed(struct abuf *buf)
return 0;

Some net driver, like rtl8169, can set/get the MAC address from the registers and store it in pdata->enetaddr.
When that happens, if there is a mismatch with the environment MAC address, u-boot will show that the MAC address source is DT. This patch ensures that the shown source is "driver" instead to avoid confusion.
Signed-off-by: Detlev Casanova detlev.casanova@collabora.com --- net/eth-uclass.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index f194df8512a..6e521955fa5 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -565,9 +565,13 @@ static int eth_post_probe(struct udevice *dev) priv->state = ETH_STATE_INIT; priv->running = false;
+ /* Check if the driver has already set a valid MAC address */ + if (is_valid_ethaddr(pdata->enetaddr)) { + source = "driver"; + } /* Check if the device has a valid MAC address in device tree */ - if (!eth_dev_get_mac_address(dev, pdata->enetaddr) || - !is_valid_ethaddr(pdata->enetaddr)) { + else if (!eth_dev_get_mac_address(dev, pdata->enetaddr) || + !is_valid_ethaddr(pdata->enetaddr)) { /* Check if the device has a MAC address in ROM */ int ret = -1; if (eth_get_ops(dev)->read_rom_hwaddr) {
participants (3)
-
Detlev Casanova
-
Jonas Karlman
-
Marek Vasut