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.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.
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.
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.
I'll go ahead and implement this, and I hope that the patch will be
received well.
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+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
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5d90cb1edcf7c1854e4cecb52871421f29d3d849
