On 2024-02-06 13:37, Quentin Schulz wrote:
On 2/6/24 13:33, Dragan Simic wrote:
On 2024-02-06 13:26, Quentin Schulz wrote:
On 2/4/24 11:39, Dragan Simic wrote:
On 2024-02-04 10:46, Jonas Karlman wrote:
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: >>> 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=h29fS4Um7whfBu7XTAi1sRbz...
I see, we unfortunately cannot know the final outcome in advance, to be able to avoid polluting the environment by adding the "eth1addr" variable if it actually isn't needed, for example.
Though, why can't the user supply an FDT that contains ethernetX aliases 0 through 2, for example, in which case we wouldn't provide a stable MAC address for ethernet2? Am I missing something, i.e. is there something preventing an ethernet2 alias from being present?
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 :-)
Thanks. :) Got it implemented already and tested a bit. I need to write the patch and series summaries, and I'll send them over.
Regarding the above-described uncertainty about what ethernetX aliases the final FDT containts, I'd say that ignoring the ethernetX aliases completely for the Pinebook Pro and the Pinephone Pro is safe and valid, because those are actual devices, instead of being development boards for which the final hardware configuration is determined by the user. I can hardly see anyone adding an Ethernet interface to them, except by plugging in a USB Ethernet dongle.
I hope you agree.
What should be done with the patches for Pinephone/Pinebook Pro then? Since I was asked to wait on your answer/patches before respinning the patch series, I would like to know what to do with them :) Drop the patches for now or keep them as is?
Your patches are fine, just please update their subjects as I already suggested. The patches I'll send a bit later will resolve the issues I raised previously for your patches, together with doing a bit more, so there's no need to change your patches further.
I would rather not break devices if I have a choice. Is merging those patches without yours going to break those devices?
Don't worry, I don't see how they could end up broken that way. There should only be some redundant code built into the resulting U-Boot images, and some redundant variables added to the environment at runtime.