[PATCH 08/18] rockchip: pine64: rockpro64: migrate to rockchip_early_misc_init_r
Dragan Simic
dsimic at manjaro.org
Tue Feb 6 13:42:38 CET 2024
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=h29fS4Um7whfBu7XTAi1sRbzXRAPxg@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?
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.
More information about the U-Boot
mailing list