Hi Dragan, On 2024-02-03 14:19, Dragan Simic wrote: > Hello Quentin, > > Please see my comments below. > > On 2024-01-23 15:49, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.sch...@theobroma-systems.com> >> >> Only setup_iodomain() differs from the original misc_init_r from >> Rockchip mach code, so let's use rockchip_early_misc_init_r instead of >> reimplementing the whole misc_init_r from Rockchip. > > Your patches from this series are fine, but the trouble is that > we end up executing rockchip_setup_macaddr() for the devices > that don't use the RK3339's built-in Gigabit Ethernet interface, > which includes the Pinebook Pro and the Pinephone Pro.
The rockchip_setup_macaddr() is not just for integrated ethernet interface. The main purpose is to have a reliably mac address assigned to any device assigned to the ethernet0/ethernet1 alias in the device tree, see fdt_fixup_ethernet(). > > We should add more ifdef guards to rockchip_setup_macaddr(), > to prevent the execution of its body for devices such as the > ones listed above, which eliminates the unneeded code from the > resulting U-Boot images, making them a bit smaller, and saves > some CPU cycles and a bit of time on boot. It also prevents > the unneeded "ethaddr" and "eth1addr" variables from being > added to the environment. Adding the ethernet addresses only adds a few ms to boot, if you care about boot speed, please look into if we can disable CONFIG_USE_PREBOOT for these boards, running "usb start" as preboot adds several seconds to the boot. > > The patch below should do the trick, which also performs a few > small code cleanups for additional file-level consistency: > > diff --git a/arch/arm/mach-rockchip/misc.c > b/arch/arm/mach-rockchip/misc.c > index 7d03f0c2b679..ed5bdab5a648 100644 > --- a/arch/arm/mach-rockchip/misc.c > +++ b/arch/arm/mach-rockchip/misc.c > @@ -23,7 +23,8 @@ > > int rockchip_setup_macaddr(void) > { > -#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256) > +#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256) && \ > + CONFIG_IS_ENABLED(GMAC_ROCKCHIP) This would exclude any board not enabling GMAC_ROCKCHIP in U-Boot but want a mac-address being set in DT fixup. And for newer RK35xx SoCs the GMAC_ROCKCHIP driver is not used. A new Kconfig option should be introduced if there is a need for some boards to disable this. > int ret; > const char *cpuid = env_get("cpuid#"); > u8 hash[SHA256_SUM_LEN]; > @@ -64,15 +65,15 @@ int rockchip_cpuid_from_efuse(const u32 > cpuid_offset, > const u32 cpuid_length, > u8 *cpuid) > { > -#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE) || > IS_ENABLED(CONFIG_ROCKCHIP_OTP) > +#if CONFIG_IS_ENABLED(ROCKCHIP_EFUSE) || This changes behavior and is wrong. IS_ENABLED check for specific config flag, CONFIG_IS_ENABLED would check for e.g. CONFIG_SPL_ROCKCHIP_EFUSE or similar. > CONFIG_IS_ENABLED(ROCKCHIP_OTP) > struct udevice *dev; > int ret; > > /* retrieve the device */ > -#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE) > +#if CONFIG_IS_ENABLED(ROCKCHIP_EFUSE) Same as above. > ret = uclass_get_device_by_driver(UCLASS_MISC, > DM_DRIVER_GET(rockchip_efuse), &dev); > -#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP) > +#elif CONFIG_IS_ENABLED(ROCKCHIP_OTP) Same as above. Regards, Jonas > ret = uclass_get_device_by_driver(UCLASS_MISC, > DM_DRIVER_GET(rockchip_otp), &dev); > #endif > >> Cc: Quentin Schulz <foss+ub...@0leil.net> >> Signed-off-by: Quentin Schulz <quentin.sch...@theobroma-systems.com> >> --- >> board/pine64/rockpro64_rk3399/rockpro64-rk3399.c | 20 >> ++------------------ >> 1 file changed, 2 insertions(+), 18 deletions(-) >> >> diff --git a/board/pine64/rockpro64_rk3399/rockpro64-rk3399.c >> b/board/pine64/rockpro64_rk3399/rockpro64-rk3399.c >> index d79084614f1..d0a694ead1d 100644 >> --- a/board/pine64/rockpro64_rk3399/rockpro64-rk3399.c >> +++ b/board/pine64/rockpro64_rk3399/rockpro64-rk3399.c >> @@ -11,7 +11,6 @@ >> #include <asm/arch-rockchip/clock.h> >> #include <asm/arch-rockchip/grf_rk3399.h> >> #include <asm/arch-rockchip/hardware.h> >> -#include <asm/arch-rockchip/misc.h> >> >> #define GRF_IO_VSEL_BT565_SHIFT 0 >> #define PMUGRF_CON0_VSEL_SHIFT 8 >> @@ -31,26 +30,11 @@ static void setup_iodomain(void) >> rk_setreg(&pmugrf->soc_con0, 1 << PMUGRF_CON0_VSEL_SHIFT); >> } >> >> -int misc_init_r(void) >> +int rockchip_early_misc_init_r(void) >> { >> - const u32 cpuid_offset = 0x7; >> - const u32 cpuid_length = 0x10; >> - u8 cpuid[cpuid_length]; >> - int ret; >> - >> setup_iodomain(); >> >> - ret = rockchip_cpuid_from_efuse(cpuid_offset, cpuid_length, cpuid); >> - if (ret) >> - return ret; >> - >> - ret = rockchip_cpuid_set(cpuid, cpuid_length); >> - if (ret) >> - return ret; >> - >> - ret = rockchip_setup_macaddr(); >> - >> - return ret; >> + return 0; >> } >> >> #endif