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

[Dragan Simic]

Hello Quentin,

Please see my comments below.
On 2024-01-23 15:49, Quentin Schulz wrote:
From: Quentin Schulz <quentin.sch...@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.

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.

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)
        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) || 
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)
        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)
        ret = uclass_get_device_by_driver(UCLASS_MISC,
                                          DM_DRIVER_GET(rockchip_otp), &dev);
 #endif

Cc: Quentin Schulz <foss+ub...@0leil.net>
Signed-off-by: Quentin Schulz <quentin.sch...@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