On 2024-02-04 05:21, Dragan Simic wrote: > On 2024-02-03 16:18, Dragan Simic wrote: >> On 2024-02-03 15:18, Jonas Karlman wrote: >>> On 2024-02-03 14:19, Dragan Simic wrote: >>>> 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(). >> >> Sure, I had already checked fdt_fixup_ethernet() before commenting. >> >> As a note, there are no ethernetX aliases in the Pinebook Pro and >> Pinephone Pro dts files, or at least there will be none eventually, >> as the redundant aliases have already been removed in the Linux >> kernel. [1] No such aliases means that fdt_fixup_ethernet() should >> end up doing nothing. >> >>>> 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. >> >> I see, but I personally don't care that much about how long the >> U-Boot takes to execute; a couple of seconds more don't bother me >> much. I care more about excluding the unneded code. >> >>>> 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. >> >> Thanks for pointing that out. Not good. >> >>> A new Kconfig option should be introduced if there is a need for some >>> boards to disable this. >> >> Is there any simpler way to achieve that? If there isn't, perhaps >> we could leave rockchip_setup_macaddr() generate the MAC address >> and rely on fdt_fixup_ethernet() ending up doing nothing when no >> ethernetX aliases exist.
As Chen-Yu Tsai pointed out in one of my prior patches [2]: The user might be loading a custom FDT for the kernel, or have DT overlays stacked on, either could have the "ethernet1" alias while the U-boot DT doesn't. So the common rockchip_setup_macaddr() cannot rely on checking for ethernetX alias, because the fixup may not run against the bundled DT. [2] https://lore.kernel.org/u-boot/CAGb2v66hR5e3nBPZ0C3=h29fs4um7whfbu7xtai1srbzxra...@mail.gmail.com/ > > After going through the source code once again, I think that adding > new configuration option would be warranted, because it would exclude > two sizable chunks of code from the resulting U-Boot images, and > because it would avoid polluting the environment with a couple of > unneeded variables. Yes a new Kconfig option would be preferred. > > I'll go ahead and implement this, and I hope that the patch will be > received well. Great :-) Regards, Jonas > >>>> 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. >> >> Sorry, my bad, somehow I managed to forget that. Thanks for >> pointing that out. >> >>>> 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 >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d90cb1edcf7c1854e4cecb52871421f29d3d849