Hi Dragan,
On 2/6/24 13:33, Dragan Simic wrote:
[Some people who received this message don't often get email from
dsi...@manjaro.org. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification ]
Hello Quentin,
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=h29fs4um7whfbu7xtai1srbzxra...@mail.gmail.com/
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?
Cheers,
Quentin
