| Message ID | 20240314144403.491850-3-detlev.casanova@collabora.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | net: Add a CONFIG_NET_BOARD_ETHADDR | expand |
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;
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)
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(-)