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

Jonas Karlman jonas at kwiboo.se
Sun Feb 4 10:46:36 CET 2024 

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:
>>>> Please see my comments below.
>>>>
>>>> On 2024-01-23 15:49, Quentin Schulz wrote:
>>>>> From: Quentin Schulz <quentin.schulz at theobroma-systems.com>
>>>>>
>>>>> Only setup_iodomain() differs from the original misc_init_r from
>>>>> Rockchip mach code, so let's use rockchip_early_misc_init_r instead 
>>>>> of
>>>>> reimplementing the whole misc_init_r from Rockchip.
>>>>
>>>> Your patches from this series are fine, but the trouble is that
>>>> we end up executing rockchip_setup_macaddr() for the devices
>>>> that don't use the RK3339's built-in Gigabit Ethernet interface,
>>>> which includes the Pinebook Pro and the Pinephone Pro.
>>>
>>> The rockchip_setup_macaddr() is not just for integrated ethernet
>>> interface. The main purpose is to have a reliably mac address assigned
>>> to any device assigned to the ethernet0/ethernet1 alias in the device
>>> tree, see fdt_fixup_ethernet().
>>
>> Sure, I had already checked fdt_fixup_ethernet() before commenting.
>>
>> As a note, there are no ethernetX aliases in the Pinebook Pro and
>> Pinephone Pro dts files, or at least there will be none eventually,
>> as the redundant aliases have already been removed in the Linux
>> kernel. [1]  No such aliases means that fdt_fixup_ethernet() should
>> end up doing nothing.
>>
>>>> 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/

> 
> 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 :-)

Regards,
Jonas

> 
>>>>   	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