[PATCH 08/18] rockchip: pine64: rockpro64: migrate to rockchip_early_misc_init_r
Dragan Simic
dsimic at manjaro.org
Sun Feb 4 11:39:11 CET 2024
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.
>>>>> 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+uboot at 0leil.net>
>>>>>> Signed-off-by: Quentin Schulz
>>>>>> <quentin.schulz at 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
More information about the U-Boot
mailing list