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

[Jonas Karlman]

Hi Dragan,

On 2024-02-03 14:19, Dragan Simic wrote:
> Hello Quentin,
> 
> 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().

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

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

A new Kconfig option should be introduced if there is a need for some
boards to disable this.

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

> 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