[PATCH 08/18] rockchip: pine64: rockpro64: migrate to rockchip_early_misc_init_r

[Quentin Schulz]

Hi Dragan, Jonas,

On 2/4/24 11:39, Dragan Simic wrote:
> [Some people who received this message don't often get email from 
> dsimic at manjaro.org. Learn why this is important at 
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Hello Jonas,
> 
> 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?

Cheers,
Quentin