On Fri, 9 Feb 2024 at 09:50, Quentin Schulz foss+uboot@0leil.net wrote:

From: Quentin Schulz quentin.schulz@theobroma-systems.com

Compared to the original misc_init_r from Rockchip mach code, setup_iodomain() is added and rockchip_setup_macaddr() is not called.

It is assumed adding rockchip_setup_macaddr() back is fine. Let's use rockchip_early_misc_init_r instead of reimplementing the whole misc_init_r from Rockchip (the side effect being that rockchip_setup_macaddr() is back).

The Pinebook Pro doesn't have onboard ethernet, nor an alias defined, I wonder if rockchip_setup_macaddr should be gated on GMAC_ROCKCHIP or something similar. Otherwise: Reviewed-by: Peter Robinson pbrobinson@gmail.com

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

---

board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c b/board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c index 4ad780767ea..2408a367305 100644 --- a/board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c +++ b/board/pine64/pinebook-pro-rk3399/pinebook-pro-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> #include <linux/printk.h> #include <power/regulator.h>

@@ -54,23 +53,10 @@ 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;
  

•   return ret;
  

•   return 0;
  

} #endif

-- 2.43.0

Hello Peter,

On 2024-02-12 14:41, Peter Robinson wrote:

On Fri, 9 Feb 2024 at 09:50, Quentin Schulz foss+uboot@0leil.net wrote:

...

From: Quentin Schulz quentin.schulz@theobroma-systems.com

Compared to the original misc_init_r from Rockchip mach code, setup_iodomain() is added and rockchip_setup_macaddr() is not called.

It is assumed adding rockchip_setup_macaddr() back is fine. Let's use rockchip_early_misc_init_r instead of reimplementing the whole misc_init_r from Rockchip (the side effect being that rockchip_setup_macaddr() is back).

The Pinebook Pro doesn't have onboard ethernet, nor an alias defined, I wonder if rockchip_setup_macaddr should be gated on GMAC_ROCKCHIP or something similar. Otherwise:

It's the same with the Pinephone Pro, plus we've got the Pinephone, the PineTab, and the "OG" Pinebook with no Ethernet interfaces. I've already prepares patches that gate out the redundant code, and I'll send them over after a bit more testing.

Reviewed-by: Peter Robinson pbrobinson@gmail.com

...

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

---

board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c b/board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c index 4ad780767ea..2408a367305 100644 --- a/board/pine64/pinebook-pro-rk3399/pinebook-pro-rk3399.c +++ b/board/pine64/pinebook-pro-rk3399/pinebook-pro-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> #include <linux/printk.h> #include <power/regulator.h>

@@ -54,23 +53,10 @@ 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;
  

•   return ret;
  

•   return 0;
  

} #endif

-- 2.43.0

On 2024-02-09 10:50, Quentin Schulz wrote:

From: Quentin Schulz quentin.schulz@theobroma-systems.com

Compared to the original misc_init_r from Rockchip mach code, setup_iodomain() is added and rockchip_setup_macaddr() is not called.

It is assumed adding rockchip_setup_macaddr() back is fine. Let's use rockchip_early_misc_init_r instead of reimplementing the whole misc_init_r from Rockchip (the side effect being that rockchip_setup_macaddr() is back).

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

Looking good to me. Thank you for including the changes suggested in my earlier review.

Reviewed-by: Dragan Simic dsimic@manjaro.org

---

.../pine64/pinephone-pro-rk3399/pinephone-pro-rk3399.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/board/pine64/pinephone-pro-rk3399/pinephone-pro-rk3399.c b/board/pine64/pinephone-pro-rk3399/pinephone-pro-rk3399.c index b6ccbb9c1c4..de75ee329d8 100644 --- a/board/pine64/pinephone-pro-rk3399/pinephone-pro-rk3399.c +++ b/board/pine64/pinephone-pro-rk3399/pinephone-pro-rk3399.c @@ -12,7 +12,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> #include <power/regulator.h>

#define GRF_IO_VSEL_BT565_GPIO2AB 1 @@ -56,23 +55,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;
  

• return 0;

• return ret;

} #endif

On Fri, 9 Feb 2024 at 09:50, Quentin Schulz foss+uboot@0leil.net wrote:

From: Quentin Schulz quentin.schulz@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.

Reviewed-by: Peter Robinson pbrobinson@gmail.com

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@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

-- 2.43.0

Hello Quentin,

Please, see my comments below.

On 2024-02-09 10:50, Quentin Schulz wrote:

From: Quentin Schulz quentin.schulz@theobroma-systems.com

The functions aren't used anywhere else than in board.c, therefore, let's not expose them anymore at all.

This merges misc.c and board.c together and removes the functions from the misc.h header file.

The patches I'm going to send, which exclude the unneeded code from the U-Boot images for the Pinebook Pro and the Pinebook Pro, change the code that this patch moves around.

Perhaps it would be better to have the move of the code and the subsequent changes to it performed in a separate series, to make checking and accepting the patches simpler as a group, and to perhaps make it a bit more clear what actually happened to anyone going through the repository history later.

Thus, I was wondering would you, please, be fine with excluding this patch from this series and letting me submit it, with proper attribution tags, of course, as part of the series I'll submit in the next couple of days?

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

---

arch/arm/include/asm/arch-rockchip/misc.h | 5 -- arch/arm/mach-rockchip/Makefile | 1 - arch/arm/mach-rockchip/board.c | 125 +++++++++++++++++++++++++++ arch/arm/mach-rockchip/misc.c | 135

---

4 files changed, 125 insertions(+), 141 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/misc.h b/arch/arm/include/asm/arch-rockchip/misc.h index 4155af8c3b0..ef37ff1661a 100644 --- a/arch/arm/include/asm/arch-rockchip/misc.h +++ b/arch/arm/include/asm/arch-rockchip/misc.h @@ -6,9 +6,4 @@

•  Rohan Garg <rohan.garg@collabora.com>
  

*/

-int rockchip_cpuid_from_efuse(const u32 cpuid_offset,

• 	      const u32 cpuid_length,
  

• 	      u8 *cpuid);
  

-int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length); -int rockchip_setup_macaddr(void); void rockchip_capsule_update_board_setup(void); diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bbf..c07bdaee4c3 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -23,7 +23,6 @@ ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) # meaning "turn it off". obj-y += boot_mode.o obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o -obj-$(CONFIG_MISC_INIT_R) += misc.o endif

ifeq ($(CONFIG_TPL_BUILD),) diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index d5cb59c10fa..80b4514852f 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -1,20 +1,32 @@ // SPDX-License-Identifier: GPL-2.0+ /*

• (C) Copyright 2019 Rockchip Electronics Co., Ltd.

• • Copyright (C) 2019 Collabora Inc - https://www.collabora.com/

• •  Rohan Garg <rohan.garg@collabora.com>
    

• • Based on puma-rk3399.c:

• •  (C) Copyright 2017 Theobroma Systems Design und Consulting 
    

GmbH */ #include <common.h> #include <clk.h> #include <cpu_func.h> +#include <env.h> #include <dm.h> #include <efi_loader.h> #include <fastboot.h> +#include <hash.h> #include <init.h> #include <log.h> #include <mmc.h> +#include <dm/uclass-internal.h> +#include <misc.h> #include <part.h> #include <ram.h> #include <syscon.h> #include <uuid.h> +#include <u-boot/crc.h> +#include <u-boot/sha256.h> #include <asm/cache.h> #include <asm/io.h> #include <asm/arch-rockchip/boot_mode.h> @@ -297,6 +309,119 @@ int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) #endif

#ifdef CONFIG_MISC_INIT_R +int rockchip_setup_macaddr(void) +{ +#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256)

• int ret;
• const char *cpuid = env_get("cpuid#");
• u8 hash[SHA256_SUM_LEN];
• int size = sizeof(hash);
• u8 mac_addr[6];
• /* Only generate a MAC address, if none is set in the environment */
• if (env_get("ethaddr"))

• return 0;
  

• if (!cpuid) {

• debug("%s: could not retrieve 'cpuid#'\n", __func__);
  

• return -1;
  

• }
• ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash,

&size);

• if (ret) {

• debug("%s: failed to calculate SHA256\n", __func__);
  

• return -1;
  

• }
• /* Copy 6 bytes of the hash to base the MAC address on */
• memcpy(mac_addr, hash, 6);
• /* Make this a valid MAC address and set it */
• mac_addr[0] &= 0xfe; /* clear multicast bit */
• mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */
• eth_env_set_enetaddr("ethaddr", mac_addr);
• /* Make a valid MAC address for ethernet1 */
• mac_addr[5] ^= 0x01;
• eth_env_set_enetaddr("eth1addr", mac_addr);

+#endif

• return 0;

+}

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

• struct udevice *dev;
• int ret;
• /* retrieve the device */

+#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_efuse), &dev);
  

+#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_otp), &dev);
  

+#endif

• if (ret) {

• debug("%s: could not find efuse device\n", __func__);
  

• return -1;
  

• }
• /* read the cpu_id range from the efuses */
• ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length);
• if (ret < 0) {

• debug("%s: reading cpuid from the efuses failed\n",
  

•       __func__);
  

• return -1;
  

• }

+#endif

• return 0;

+}

+int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) +{

• u8 low[cpuid_length / 2], high[cpuid_length / 2];
• char cpuid_str[cpuid_length * 2 + 1];
• u64 serialno;
• char serialno_str[17];
• const char *oldid;
• int i;
• memset(cpuid_str, 0, sizeof(cpuid_str));
• for (i = 0; i < cpuid_length; i++)

• sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]);
  

• debug("cpuid: %s\n", cpuid_str);
• /*

• * Mix the cpuid bytes using the same rules as in
  

• *   ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c
  

• */
  

• for (i = 0; i < cpuid_length / 2; i++) {

• low[i] = cpuid[1 + (i << 1)];
  

• high[i] = cpuid[i << 1];
  

• }
• serialno = crc32_no_comp(0, low, cpuid_length / 2);
• serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) <<

32;

• snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno);
• oldid = env_get("cpuid#");
• if (oldid && strcmp(oldid, cpuid_str) != 0)

• printf("cpuid: value %s present in env does not match hardware 
  

%s\n",

•        oldid, cpuid_str);
  

• env_set("cpuid#", cpuid_str);
• /* Only generate serial# when none is set yet */
• if (!env_get("serial#"))

• env_set("serial#", serialno_str);
  

• return 0;

+}

__weak int rockchip_early_misc_init_r(void) { return 0; diff --git a/arch/arm/mach-rockchip/misc.c b/arch/arm/mach-rockchip/misc.c deleted file mode 100644 index 15397cff009..00000000000 --- a/arch/arm/mach-rockchip/misc.c +++ /dev/null @@ -1,135 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/*

• • RK3399: Architecture common definitions
• • Copyright (C) 2019 Collabora Inc - https://www.collabora.com/

• •  Rohan Garg <rohan.garg@collabora.com>
    

• • Based on puma-rk3399.c:

• •  (C) Copyright 2017 Theobroma Systems Design und Consulting 
    

GmbH

• */

-#include <common.h> -#include <env.h> -#include <dm.h> -#include <hash.h> -#include <log.h> -#include <dm/uclass-internal.h> -#include <misc.h> -#include <u-boot/crc.h> -#include <u-boot/sha256.h>

-#include <asm/arch-rockchip/misc.h>

-int rockchip_setup_macaddr(void) -{ -#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256)

• int ret;
• const char *cpuid = env_get("cpuid#");
• u8 hash[SHA256_SUM_LEN];
• int size = sizeof(hash);
• u8 mac_addr[6];
• /* Only generate a MAC address, if none is set in the environment */
• if (env_get("ethaddr"))

• return 0;
  

• if (!cpuid) {

• debug("%s: could not retrieve 'cpuid#'\n", __func__);
  

• return -1;
  

• }
• ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash,

&size);

• if (ret) {

• debug("%s: failed to calculate SHA256\n", __func__);
  

• return -1;
  

• }
• /* Copy 6 bytes of the hash to base the MAC address on */
• memcpy(mac_addr, hash, 6);
• /* Make this a valid MAC address and set it */
• mac_addr[0] &= 0xfe; /* clear multicast bit */
• mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */
• eth_env_set_enetaddr("ethaddr", mac_addr);
• /* Make a valid MAC address for ethernet1 */
• mac_addr[5] ^= 0x01;
• eth_env_set_enetaddr("eth1addr", mac_addr);

-#endif

• return 0;

-}

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

• struct udevice *dev;
• int ret;
• /* retrieve the device */

-#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_efuse), &dev);
  

-#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_otp), &dev);
  

-#endif

• if (ret) {

• debug("%s: could not find efuse device\n", __func__);
  

• return -1;
  

• }
• /* read the cpu_id range from the efuses */
• ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length);
• if (ret < 0) {

• debug("%s: reading cpuid from the efuses failed\n",
  

•       __func__);
  

• return -1;
  

• }

-#endif

• return 0;

-}

-int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) -{

• u8 low[cpuid_length / 2], high[cpuid_length / 2];
• char cpuid_str[cpuid_length * 2 + 1];
• u64 serialno;
• char serialno_str[17];
• const char *oldid;
• int i;
• memset(cpuid_str, 0, sizeof(cpuid_str));
• for (i = 0; i < cpuid_length; i++)

• sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]);
  

• debug("cpuid: %s\n", cpuid_str);
• /*

• * Mix the cpuid bytes using the same rules as in
  

• *   ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c
  

• */
  

• for (i = 0; i < cpuid_length / 2; i++) {

• low[i] = cpuid[1 + (i << 1)];
  

• high[i] = cpuid[i << 1];
  

• }
• serialno = crc32_no_comp(0, low, cpuid_length / 2);
• serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) <<

32;

• snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno);
• oldid = env_get("cpuid#");
• if (oldid && strcmp(oldid, cpuid_str) != 0)

• printf("cpuid: value %s present in env does not match hardware 
  

%s\n",

•        oldid, cpuid_str);
  

• env_set("cpuid#", cpuid_str);
• /* Only generate serial# when none is set yet */
• if (!env_get("serial#"))

• env_set("serial#", serialno_str);
  

• return 0;

-}

Hello Mark,

On 2024-02-09 19:36, Mark Kettenis wrote:

...

Date: Fri, 09 Feb 2024 18:58:01 +0100 From: Dragan Simic dsimic@manjaro.org Please, see my comments below.

On 2024-02-09 10:50, Quentin Schulz wrote:

...

From: Quentin Schulz quentin.schulz@theobroma-systems.com

The functions aren't used anywhere else than in board.c, therefore, let's not expose them anymore at all.

This merges misc.c and board.c together and removes the functions from the misc.h header file.

The patches I'm going to send, which exclude the unneeded code from the U-Boot images for the Pinebook Pro and the Pinebook Pro, change the code that this patch moves around.

Perhaps it would be better to have the move of the code and the subsequent changes to it performed in a separate series, to make checking and accepting the patches simpler as a group, and to perhaps make it a bit more clear what actually happened to anyone going through the repository history later.

Thus, I was wondering would you, please, be fine with excluding this patch from this series and letting me submit it, with proper attribution tags, of course, as part of the series I'll submit in the next couple of days?

I'm not covinced your patches are a good idea. Unless we're running into space constraints, or if there are noticable slowdowns in the boot process because of the extra code, I really don't see what the benefit of further differentiation between rk3399 boards would be. It just makes testing things harder.

Oh, I fully understand your position, but please let's also keep in mind that the subject of those patches are actual devices with pretty much fixed hardware configurations, instead of targeting development boards whose actual hardware configurations, at least in theory, depend on the user.

In addition to preventing the unneeded code from entering the U-Boot images, my patches would also prevent useless "ethaddr" and "eth1addr" from appearing in the saved environments on those devices. I think that's useful and may actually prevent some confusion when the saved environment is checked by the users on those devices.

The smaller the number of special snow flakes, the better!

I'd agree that having fewer special cases helps, but again, handling the specifics of some devices also has its benefits. Additionally, perhaps simply the fact that those are actual devices makes them some kind of special snowflakes. :)

...

...

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

---

arch/arm/include/asm/arch-rockchip/misc.h | 5 -- arch/arm/mach-rockchip/Makefile | 1 - arch/arm/mach-rockchip/board.c | 125 +++++++++++++++++++++++++++ arch/arm/mach-rockchip/misc.c | 135

---

4 files changed, 125 insertions(+), 141 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/misc.h b/arch/arm/include/asm/arch-rockchip/misc.h index 4155af8c3b0..ef37ff1661a 100644 --- a/arch/arm/include/asm/arch-rockchip/misc.h +++ b/arch/arm/include/asm/arch-rockchip/misc.h @@ -6,9 +6,4 @@

•  Rohan Garg <rohan.garg@collabora.com>
  

*/

-int rockchip_cpuid_from_efuse(const u32 cpuid_offset,

• 	      const u32 cpuid_length,
  

• 	      u8 *cpuid);
  

-int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length); -int rockchip_setup_macaddr(void); void rockchip_capsule_update_board_setup(void); diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bbf..c07bdaee4c3 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -23,7 +23,6 @@ ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) # meaning "turn it off". obj-y += boot_mode.o obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o -obj-$(CONFIG_MISC_INIT_R) += misc.o endif

ifeq ($(CONFIG_TPL_BUILD),) diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index d5cb59c10fa..80b4514852f 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -1,20 +1,32 @@ // SPDX-License-Identifier: GPL-2.0+ /*

• (C) Copyright 2019 Rockchip Electronics Co., Ltd.

• • Copyright (C) 2019 Collabora Inc - https://www.collabora.com/

• •  Rohan Garg <rohan.garg@collabora.com>
    

• • Based on puma-rk3399.c:

• •  (C) Copyright 2017 Theobroma Systems Design und Consulting
    

GmbH */ #include <common.h> #include <clk.h> #include <cpu_func.h> +#include <env.h> #include <dm.h> #include <efi_loader.h> #include <fastboot.h> +#include <hash.h> #include <init.h> #include <log.h> #include <mmc.h> +#include <dm/uclass-internal.h> +#include <misc.h> #include <part.h> #include <ram.h> #include <syscon.h> #include <uuid.h> +#include <u-boot/crc.h> +#include <u-boot/sha256.h> #include <asm/cache.h> #include <asm/io.h> #include <asm/arch-rockchip/boot_mode.h> @@ -297,6 +309,119 @@ int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) #endif

#ifdef CONFIG_MISC_INIT_R +int rockchip_setup_macaddr(void) +{ +#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256)

• int ret;
• const char *cpuid = env_get("cpuid#");
• u8 hash[SHA256_SUM_LEN];
• int size = sizeof(hash);
• u8 mac_addr[6];
• /* Only generate a MAC address, if none is set in the environment */
• if (env_get("ethaddr"))

• return 0;
  

• if (!cpuid) {

• debug("%s: could not retrieve 'cpuid#'\n", __func__);
  

• return -1;
  

• }
• ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash,

&size);

• if (ret) {

• debug("%s: failed to calculate SHA256\n", __func__);
  

• return -1;
  

• }
• /* Copy 6 bytes of the hash to base the MAC address on */
• memcpy(mac_addr, hash, 6);
• /* Make this a valid MAC address and set it */
• mac_addr[0] &= 0xfe; /* clear multicast bit */
• mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */
• eth_env_set_enetaddr("ethaddr", mac_addr);
• /* Make a valid MAC address for ethernet1 */
• mac_addr[5] ^= 0x01;
• eth_env_set_enetaddr("eth1addr", mac_addr);

+#endif

• return 0;

+}

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

• struct udevice *dev;
• int ret;
• /* retrieve the device */

+#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_efuse), &dev);
  

+#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_otp), &dev);
  

+#endif

• if (ret) {

• debug("%s: could not find efuse device\n", __func__);
  

• return -1;
  

• }
• /* read the cpu_id range from the efuses */
• ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length);
• if (ret < 0) {

• debug("%s: reading cpuid from the efuses failed\n",
  

•       __func__);
  

• return -1;
  

• }

+#endif

• return 0;

+}

+int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) +{

• u8 low[cpuid_length / 2], high[cpuid_length / 2];
• char cpuid_str[cpuid_length * 2 + 1];
• u64 serialno;
• char serialno_str[17];
• const char *oldid;
• int i;
• memset(cpuid_str, 0, sizeof(cpuid_str));
• for (i = 0; i < cpuid_length; i++)

• sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]);
  

• debug("cpuid: %s\n", cpuid_str);
• /*

• * Mix the cpuid bytes using the same rules as in
  

• *   ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c
  

• */
  

• for (i = 0; i < cpuid_length / 2; i++) {

• low[i] = cpuid[1 + (i << 1)];
  

• high[i] = cpuid[i << 1];
  

• }
• serialno = crc32_no_comp(0, low, cpuid_length / 2);
• serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) <<

32;

• snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno);
• oldid = env_get("cpuid#");
• if (oldid && strcmp(oldid, cpuid_str) != 0)

• printf("cpuid: value %s present in env does not match hardware
  

%s\n",

•        oldid, cpuid_str);
  

• env_set("cpuid#", cpuid_str);
• /* Only generate serial# when none is set yet */
• if (!env_get("serial#"))

• env_set("serial#", serialno_str);
  

• return 0;

+}

__weak int rockchip_early_misc_init_r(void) { return 0; diff --git a/arch/arm/mach-rockchip/misc.c b/arch/arm/mach-rockchip/misc.c deleted file mode 100644 index 15397cff009..00000000000 --- a/arch/arm/mach-rockchip/misc.c +++ /dev/null @@ -1,135 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/*

• • RK3399: Architecture common definitions
• • Copyright (C) 2019 Collabora Inc - https://www.collabora.com/

• •  Rohan Garg <rohan.garg@collabora.com>
    

• • Based on puma-rk3399.c:

• •  (C) Copyright 2017 Theobroma Systems Design und Consulting
    

GmbH

• */

-#include <common.h> -#include <env.h> -#include <dm.h> -#include <hash.h> -#include <log.h> -#include <dm/uclass-internal.h> -#include <misc.h> -#include <u-boot/crc.h> -#include <u-boot/sha256.h>

-#include <asm/arch-rockchip/misc.h>

-int rockchip_setup_macaddr(void) -{ -#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256)

• int ret;
• const char *cpuid = env_get("cpuid#");
• u8 hash[SHA256_SUM_LEN];
• int size = sizeof(hash);
• u8 mac_addr[6];
• /* Only generate a MAC address, if none is set in the environment */
• if (env_get("ethaddr"))

• return 0;
  

• if (!cpuid) {

• debug("%s: could not retrieve 'cpuid#'\n", __func__);
  

• return -1;
  

• }
• ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash,

&size);

• if (ret) {

• debug("%s: failed to calculate SHA256\n", __func__);
  

• return -1;
  

• }
• /* Copy 6 bytes of the hash to base the MAC address on */
• memcpy(mac_addr, hash, 6);
• /* Make this a valid MAC address and set it */
• mac_addr[0] &= 0xfe; /* clear multicast bit */
• mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */
• eth_env_set_enetaddr("ethaddr", mac_addr);
• /* Make a valid MAC address for ethernet1 */
• mac_addr[5] ^= 0x01;
• eth_env_set_enetaddr("eth1addr", mac_addr);

-#endif

• return 0;

-}

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

• struct udevice *dev;
• int ret;
• /* retrieve the device */

-#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_efuse), &dev);
  

-#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

• 			  DM_DRIVER_GET(rockchip_otp), &dev);
  

-#endif

• if (ret) {

• debug("%s: could not find efuse device\n", __func__);
  

• return -1;
  

• }
• /* read the cpu_id range from the efuses */
• ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length);
• if (ret < 0) {

• debug("%s: reading cpuid from the efuses failed\n",
  

•       __func__);
  

• return -1;
  

• }

-#endif

• return 0;

-}

-int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) -{

• u8 low[cpuid_length / 2], high[cpuid_length / 2];
• char cpuid_str[cpuid_length * 2 + 1];
• u64 serialno;
• char serialno_str[17];
• const char *oldid;
• int i;
• memset(cpuid_str, 0, sizeof(cpuid_str));
• for (i = 0; i < cpuid_length; i++)

• sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]);
  

• debug("cpuid: %s\n", cpuid_str);
• /*

• * Mix the cpuid bytes using the same rules as in
  

• *   ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c
  

• */
  

• for (i = 0; i < cpuid_length / 2; i++) {

• low[i] = cpuid[1 + (i << 1)];
  

• high[i] = cpuid[i << 1];
  

• }
• serialno = crc32_no_comp(0, low, cpuid_length / 2);
• serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) <<

32;

• snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno);
• oldid = env_get("cpuid#");
• if (oldid && strcmp(oldid, cpuid_str) != 0)

• printf("cpuid: value %s present in env does not match hardware
  

%s\n",

•        oldid, cpuid_str);
  

• env_set("cpuid#", cpuid_str);
• /* Only generate serial# when none is set yet */
• if (!env_get("serial#"))

• env_set("serial#", serialno_str);
  

• return 0;

-}

Hello Peter,

On 2024-02-12 14:56, Peter Robinson wrote:

On Fri, 9 Feb 2024 at 18:57, Dragan Simic dsimic@manjaro.org wrote:

...

On 2024-02-09 19:36, Mark Kettenis wrote:

...

...

Date: Fri, 09 Feb 2024 18:58:01 +0100 From: Dragan Simic dsimic@manjaro.org Please, see my comments below.

On 2024-02-09 10:50, Quentin Schulz wrote:

...

From: Quentin Schulz quentin.schulz@theobroma-systems.com

The functions aren't used anywhere else than in board.c, therefore, let's not expose them anymore at all.

This merges misc.c and board.c together and removes the functions from the misc.h header file.

The patches I'm going to send, which exclude the unneeded code from the U-Boot images for the Pinebook Pro and the Pinebook Pro, change the code that this patch moves around.

Perhaps it would be better to have the move of the code and the subsequent changes to it performed in a separate series, to make checking and accepting the patches simpler as a group, and to perhaps make it a bit more clear what actually happened to anyone going through the repository history later.

Thus, I was wondering would you, please, be fine with excluding this patch from this series and letting me submit it, with proper attribution tags, of course, as part of the series I'll submit in the next couple of days?

I'm not covinced your patches are a good idea. Unless we're running into space constraints, or if there are noticable slowdowns in the boot process because of the extra code, I really don't see what the benefit of further differentiation between rk3399 boards would be. It just makes testing things harder.

Oh, I fully understand your position, but please let's also keep in mind that the subject of those patches are actual devices with pretty much fixed hardware configurations, instead of targeting development boards whose actual hardware configurations, at least in theory, depend on the user.

In addition to preventing the unneeded code from entering the U-Boot images, my patches would also prevent useless "ethaddr" and "eth1addr" from appearing in the saved environments on those devices. I think that's useful and may actually prevent some confusion when the saved environment is checked by the users on those devices.

Maybe that code should be guarded on if the wired ethernet is enabled or not. Would allow one code but the ability to disable it when the ethernet isn't used.

Basically, we can't know for sure are the users going to supply some DT overlays that add another Ethernet interface to an SBC, for example a Fast Ethernet interface on RK3328-based SBCs, which is the reason why we can't rely on the built-in DT to decide whether to provide the stable MAC addresses or not.

However, it's highly unlikely that an Ethernet interface is going to be added to a fixed-configuration device such as the Pinebook Pro or the Pinephone. Other than plugging in a USB Ethernet dongle, that is, which already come with fixed MACA addresses. Thus, not providing stable MAC addresses should be just fine for such fixed- configuration devices.

...

...

The smaller the number of special snow flakes, the better!

I'd agree that having fewer special cases helps, but again, handling the specifics of some devices also has its benefits. Additionally, perhaps simply the fact that those are actual devices makes them some kind of special snowflakes. :)

...

...

...

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

---

arch/arm/include/asm/arch-rockchip/misc.h | 5 -- arch/arm/mach-rockchip/Makefile | 1 - arch/arm/mach-rockchip/board.c | 125 +++++++++++++++++++++++++++ arch/arm/mach-rockchip/misc.c | 135

---

4 files changed, 125 insertions(+), 141 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/misc.h b/arch/arm/include/asm/arch-rockchip/misc.h index 4155af8c3b0..ef37ff1661a 100644 --- a/arch/arm/include/asm/arch-rockchip/misc.h +++ b/arch/arm/include/asm/arch-rockchip/misc.h @@ -6,9 +6,4 @@

•  Rohan Garg <rohan.garg@collabora.com>
  

*/

-int rockchip_cpuid_from_efuse(const u32 cpuid_offset,

•                    const u32 cpuid_length,
  

•                    u8 *cpuid);
  

-int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length); -int rockchip_setup_macaddr(void); void rockchip_capsule_update_board_setup(void); diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index 1dc92066bbf..c07bdaee4c3 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -23,7 +23,6 @@ ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) # meaning "turn it off". obj-y += boot_mode.o obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o -obj-$(CONFIG_MISC_INIT_R) += misc.o endif

ifeq ($(CONFIG_TPL_BUILD),) diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index d5cb59c10fa..80b4514852f 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -1,20 +1,32 @@ // SPDX-License-Identifier: GPL-2.0+ /*

• (C) Copyright 2019 Rockchip Electronics Co., Ltd.

• • Copyright (C) 2019 Collabora Inc - https://www.collabora.com/

• •  Rohan Garg <rohan.garg@collabora.com>
    

• • Based on puma-rk3399.c:

• •  (C) Copyright 2017 Theobroma Systems Design und Consulting
    

GmbH */ #include <common.h> #include <clk.h> #include <cpu_func.h> +#include <env.h> #include <dm.h> #include <efi_loader.h> #include <fastboot.h> +#include <hash.h> #include <init.h> #include <log.h> #include <mmc.h> +#include <dm/uclass-internal.h> +#include <misc.h> #include <part.h> #include <ram.h> #include <syscon.h> #include <uuid.h> +#include <u-boot/crc.h> +#include <u-boot/sha256.h> #include <asm/cache.h> #include <asm/io.h> #include <asm/arch-rockchip/boot_mode.h> @@ -297,6 +309,119 @@ int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) #endif

#ifdef CONFIG_MISC_INIT_R +int rockchip_setup_macaddr(void) +{ +#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256)

• int ret;
• const char *cpuid = env_get("cpuid#");
• u8 hash[SHA256_SUM_LEN];
• int size = sizeof(hash);
• u8 mac_addr[6];
• /* Only generate a MAC address, if none is set in the environment */
• if (env_get("ethaddr"))

•      return 0;
  

• if (!cpuid) {

•      debug("%s: could not retrieve 'cpuid#'\n", __func__);
  

•      return -1;
  

• }
• ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash,

&size);

• if (ret) {

•      debug("%s: failed to calculate SHA256\n", __func__);
  

•      return -1;
  

• }
• /* Copy 6 bytes of the hash to base the MAC address on */
• memcpy(mac_addr, hash, 6);
• /* Make this a valid MAC address and set it */
• mac_addr[0] &= 0xfe; /* clear multicast bit */
• mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */
• eth_env_set_enetaddr("ethaddr", mac_addr);
• /* Make a valid MAC address for ethernet1 */
• mac_addr[5] ^= 0x01;
• eth_env_set_enetaddr("eth1addr", mac_addr);

+#endif

• return 0;

+}

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

• struct udevice *dev;
• int ret;
• /* retrieve the device */

+#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

•                                DM_DRIVER_GET(rockchip_efuse), &dev);
  

+#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

•                                DM_DRIVER_GET(rockchip_otp), &dev);
  

+#endif

• if (ret) {

•      debug("%s: could not find efuse device\n", __func__);
  

•      return -1;
  

• }
• /* read the cpu_id range from the efuses */
• ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length);
• if (ret < 0) {

•      debug("%s: reading cpuid from the efuses failed\n",
  

•            __func__);
  

•      return -1;
  

• }

+#endif

• return 0;

+}

+int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) +{

• u8 low[cpuid_length / 2], high[cpuid_length / 2];
• char cpuid_str[cpuid_length * 2 + 1];
• u64 serialno;
• char serialno_str[17];
• const char *oldid;
• int i;
• memset(cpuid_str, 0, sizeof(cpuid_str));
• for (i = 0; i < cpuid_length; i++)

•      sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]);
  

• debug("cpuid: %s\n", cpuid_str);
• /*
• • Mix the cpuid bytes using the same rules as in
• • ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c
• */
• for (i = 0; i < cpuid_length / 2; i++) {

•      low[i] = cpuid[1 + (i << 1)];
  

•      high[i] = cpuid[i << 1];
  

• }
• serialno = crc32_no_comp(0, low, cpuid_length / 2);
• serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) <<

32;

• snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno);
• oldid = env_get("cpuid#");
• if (oldid && strcmp(oldid, cpuid_str) != 0)

•      printf("cpuid: value %s present in env does not match hardware
  

%s\n",

•             oldid, cpuid_str);
  

• env_set("cpuid#", cpuid_str);
• /* Only generate serial# when none is set yet */
• if (!env_get("serial#"))

•      env_set("serial#", serialno_str);
  

• return 0;

+}

__weak int rockchip_early_misc_init_r(void) { return 0; diff --git a/arch/arm/mach-rockchip/misc.c b/arch/arm/mach-rockchip/misc.c deleted file mode 100644 index 15397cff009..00000000000 --- a/arch/arm/mach-rockchip/misc.c +++ /dev/null @@ -1,135 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/*

• • RK3399: Architecture common definitions
• • Copyright (C) 2019 Collabora Inc - https://www.collabora.com/

• •  Rohan Garg <rohan.garg@collabora.com>
    

• • Based on puma-rk3399.c:

• •  (C) Copyright 2017 Theobroma Systems Design und Consulting
    

GmbH

• */

-#include <common.h> -#include <env.h> -#include <dm.h> -#include <hash.h> -#include <log.h> -#include <dm/uclass-internal.h> -#include <misc.h> -#include <u-boot/crc.h> -#include <u-boot/sha256.h>

-#include <asm/arch-rockchip/misc.h>

-int rockchip_setup_macaddr(void) -{ -#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256)

• int ret;
• const char *cpuid = env_get("cpuid#");
• u8 hash[SHA256_SUM_LEN];
• int size = sizeof(hash);
• u8 mac_addr[6];
• /* Only generate a MAC address, if none is set in the environment */
• if (env_get("ethaddr"))

•      return 0;
  

• if (!cpuid) {

•      debug("%s: could not retrieve 'cpuid#'\n", __func__);
  

•      return -1;
  

• }
• ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash,

&size);

• if (ret) {

•      debug("%s: failed to calculate SHA256\n", __func__);
  

•      return -1;
  

• }
• /* Copy 6 bytes of the hash to base the MAC address on */
• memcpy(mac_addr, hash, 6);
• /* Make this a valid MAC address and set it */
• mac_addr[0] &= 0xfe; /* clear multicast bit */
• mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */
• eth_env_set_enetaddr("ethaddr", mac_addr);
• /* Make a valid MAC address for ethernet1 */
• mac_addr[5] ^= 0x01;
• eth_env_set_enetaddr("eth1addr", mac_addr);

-#endif

• return 0;

-}

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

• struct udevice *dev;
• int ret;
• /* retrieve the device */

-#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

•                                DM_DRIVER_GET(rockchip_efuse), &dev);
  

-#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP)

• ret = uclass_get_device_by_driver(UCLASS_MISC,

•                                DM_DRIVER_GET(rockchip_otp), &dev);
  

-#endif

• if (ret) {

•      debug("%s: could not find efuse device\n", __func__);
  

•      return -1;
  

• }
• /* read the cpu_id range from the efuses */
• ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length);
• if (ret < 0) {

•      debug("%s: reading cpuid from the efuses failed\n",
  

•            __func__);
  

•      return -1;
  

• }

-#endif

• return 0;

-}

-int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) -{

• u8 low[cpuid_length / 2], high[cpuid_length / 2];
• char cpuid_str[cpuid_length * 2 + 1];
• u64 serialno;
• char serialno_str[17];
• const char *oldid;
• int i;
• memset(cpuid_str, 0, sizeof(cpuid_str));
• for (i = 0; i < cpuid_length; i++)

•      sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]);
  

• debug("cpuid: %s\n", cpuid_str);
• /*
• • Mix the cpuid bytes using the same rules as in
• • ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c
• */
• for (i = 0; i < cpuid_length / 2; i++) {

•      low[i] = cpuid[1 + (i << 1)];
  

•      high[i] = cpuid[i << 1];
  

• }
• serialno = crc32_no_comp(0, low, cpuid_length / 2);
• serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) <<

32;

• snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno);
• oldid = env_get("cpuid#");
• if (oldid && strcmp(oldid, cpuid_str) != 0)

•      printf("cpuid: value %s present in env does not match hardware
  

%s\n",

•             oldid, cpuid_str);
  

• env_set("cpuid#", cpuid_str);
• /* Only generate serial# when none is set yet */
• if (!env_get("serial#"))

•      env_set("serial#", serialno_str);
  

• return 0;

-}

On 2024-02-12 15:21, Peter Robinson wrote:

On Mon, 12 Feb 2024 at 14:16, Dragan Simic dsimic@manjaro.org wrote:

...

On 2024-02-12 14:56, Peter Robinson wrote:

...

On Fri, 9 Feb 2024 at 18:57, Dragan Simic dsimic@manjaro.org wrote:

...

On 2024-02-09 19:36, Mark Kettenis wrote:

...

...

Date: Fri, 09 Feb 2024 18:58:01 +0100 From: Dragan Simic dsimic@manjaro.org Please, see my comments below.

On 2024-02-09 10:50, Quentin Schulz wrote: > From: Quentin Schulz quentin.schulz@theobroma-systems.com > > The functions aren't used anywhere else than in board.c, therefore, > let's not expose them anymore at all. > > This merges misc.c and board.c together and removes the functions from > the misc.h header file.

The patches I'm going to send, which exclude the unneeded code from the U-Boot images for the Pinebook Pro and the Pinebook Pro, change the code that this patch moves around.

Perhaps it would be better to have the move of the code and the subsequent changes to it performed in a separate series, to make checking and accepting the patches simpler as a group, and to perhaps make it a bit more clear what actually happened to anyone going through the repository history later.

Thus, I was wondering would you, please, be fine with excluding this patch from this series and letting me submit it, with proper attribution tags, of course, as part of the series I'll submit in the next couple of days?

I'm not covinced your patches are a good idea. Unless we're running into space constraints, or if there are noticable slowdowns in the boot process because of the extra code, I really don't see what the benefit of further differentiation between rk3399 boards would be. It just makes testing things harder.

Oh, I fully understand your position, but please let's also keep in mind that the subject of those patches are actual devices with pretty much fixed hardware configurations, instead of targeting development boards whose actual hardware configurations, at least in theory, depend on the user.

In addition to preventing the unneeded code from entering the U-Boot images, my patches would also prevent useless "ethaddr" and "eth1addr" from appearing in the saved environments on those devices. I think that's useful and may actually prevent some confusion when the saved environment is checked by the users on those devices.

Maybe that code should be guarded on if the wired ethernet is enabled or not. Would allow one code but the ability to disable it when the ethernet isn't used.

Basically, we can't know for sure are the users going to supply some DT overlays that add another Ethernet interface to an SBC, for example a Fast Ethernet interface on RK3328-based SBCs, which is the reason why we can't rely on the built-in DT to decide whether to provide the stable MAC addresses or not.

That will likely not be solvable with purely an overlay, they may need to enable drivers that aren't by default.

Sure, but from the Linux kernel's perspective, if the additional Ethernet interface isn't going to be used in U-Boot, all the kernel wants from U-Boot is the fixed MAC address in the DT.

By the way, I've got a hopefully neat Linux kernel patch in the works, which will make the testing of fixed MAC addresses being passed around correctly a bit easier.

...

However, it's highly unlikely that an Ethernet interface is going to be added to a fixed-configuration device such as the Pinebook Pro or the Pinephone. Other than plugging in a USB Ethernet dongle, that is, which already come with fixed MACA addresses. Thus, not providing stable MAC addresses should be just fine for such fixed- configuration devices.

Don't disagree with that, which is why I did those configs like that.

Great, thanks.

...

...

...

...

The smaller the number of special snow flakes, the better!

I'd agree that having fewer special cases helps, but again, handling the specifics of some devices also has its benefits. Additionally, perhaps simply the fact that those are actual devices makes them some kind of special snowflakes. :)

...

...

> Cc: Quentin Schulz foss+uboot@0leil.net > Reviewed-by: Kever Yang kever.yang@rock-chips.com > Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com > --- > arch/arm/include/asm/arch-rockchip/misc.h | 5 -- > arch/arm/mach-rockchip/Makefile | 1 - > arch/arm/mach-rockchip/board.c | 125 > +++++++++++++++++++++++++++ > arch/arm/mach-rockchip/misc.c | 135 > ------------------------------ > 4 files changed, 125 insertions(+), 141 deletions(-) > > diff --git a/arch/arm/include/asm/arch-rockchip/misc.h > b/arch/arm/include/asm/arch-rockchip/misc.h > index 4155af8c3b0..ef37ff1661a 100644 > --- a/arch/arm/include/asm/arch-rockchip/misc.h > +++ b/arch/arm/include/asm/arch-rockchip/misc.h > @@ -6,9 +6,4 @@ > * Rohan Garg rohan.garg@collabora.com > */ > > -int rockchip_cpuid_from_efuse(const u32 cpuid_offset, > - const u32 cpuid_length, > - u8 *cpuid); > -int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length); > -int rockchip_setup_macaddr(void); > void rockchip_capsule_update_board_setup(void); > diff --git a/arch/arm/mach-rockchip/Makefile > b/arch/arm/mach-rockchip/Makefile > index 1dc92066bbf..c07bdaee4c3 100644 > --- a/arch/arm/mach-rockchip/Makefile > +++ b/arch/arm/mach-rockchip/Makefile > @@ -23,7 +23,6 @@ ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) > # meaning "turn it off". > obj-y += boot_mode.o > obj-$(CONFIG_ROCKCHIP_COMMON_BOARD) += board.o > -obj-$(CONFIG_MISC_INIT_R) += misc.o > endif > > ifeq ($(CONFIG_TPL_BUILD),) > diff --git a/arch/arm/mach-rockchip/board.c > b/arch/arm/mach-rockchip/board.c > index d5cb59c10fa..80b4514852f 100644 > --- a/arch/arm/mach-rockchip/board.c > +++ b/arch/arm/mach-rockchip/board.c > @@ -1,20 +1,32 @@ > // SPDX-License-Identifier: GPL-2.0+ > /* > * (C) Copyright 2019 Rockchip Electronics Co., Ltd. > + * > + * Copyright (C) 2019 Collabora Inc - https://www.collabora.com/ > + * Rohan Garg rohan.garg@collabora.com > + * > + * Based on puma-rk3399.c: > + * (C) Copyright 2017 Theobroma Systems Design und Consulting > GmbH > */ > #include <common.h> > #include <clk.h> > #include <cpu_func.h> > +#include <env.h> > #include <dm.h> > #include <efi_loader.h> > #include <fastboot.h> > +#include <hash.h> > #include <init.h> > #include <log.h> > #include <mmc.h> > +#include <dm/uclass-internal.h> > +#include <misc.h> > #include <part.h> > #include <ram.h> > #include <syscon.h> > #include <uuid.h> > +#include <u-boot/crc.h> > +#include <u-boot/sha256.h> > #include <asm/cache.h> > #include <asm/io.h> > #include <asm/arch-rockchip/boot_mode.h> > @@ -297,6 +309,119 @@ int fastboot_set_reboot_flag(enum > fastboot_reboot_reason reason) > #endif > > #ifdef CONFIG_MISC_INIT_R > +int rockchip_setup_macaddr(void) > +{ > +#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256) > + int ret; > + const char *cpuid = env_get("cpuid#"); > + u8 hash[SHA256_SUM_LEN]; > + int size = sizeof(hash); > + u8 mac_addr[6]; > + > + /* Only generate a MAC address, if none is set in the environment */ > + if (env_get("ethaddr")) > + return 0; > + > + if (!cpuid) { > + debug("%s: could not retrieve 'cpuid#'\n", __func__); > + return -1; > + } > + > + ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash, > &size); > + if (ret) { > + debug("%s: failed to calculate SHA256\n", __func__); > + return -1; > + } > + > + /* Copy 6 bytes of the hash to base the MAC address on */ > + memcpy(mac_addr, hash, 6); > + > + /* Make this a valid MAC address and set it */ > + mac_addr[0] &= 0xfe; /* clear multicast bit */ > + mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */ > + eth_env_set_enetaddr("ethaddr", mac_addr); > + > + /* Make a valid MAC address for ethernet1 */ > + mac_addr[5] ^= 0x01; > + eth_env_set_enetaddr("eth1addr", mac_addr); > +#endif > + return 0; > +} > + > +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) > + struct udevice *dev; > + int ret; > + > + /* retrieve the device */ > +#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE) > + ret = uclass_get_device_by_driver(UCLASS_MISC, > + DM_DRIVER_GET(rockchip_efuse), &dev); > +#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP) > + ret = uclass_get_device_by_driver(UCLASS_MISC, > + DM_DRIVER_GET(rockchip_otp), &dev); > +#endif > + if (ret) { > + debug("%s: could not find efuse device\n", __func__); > + return -1; > + } > + > + /* read the cpu_id range from the efuses */ > + ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length); > + if (ret < 0) { > + debug("%s: reading cpuid from the efuses failed\n", > + __func__); > + return -1; > + } > +#endif > + return 0; > +} > + > +int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) > +{ > + u8 low[cpuid_length / 2], high[cpuid_length / 2]; > + char cpuid_str[cpuid_length * 2 + 1]; > + u64 serialno; > + char serialno_str[17]; > + const char *oldid; > + int i; > + > + memset(cpuid_str, 0, sizeof(cpuid_str)); > + for (i = 0; i < cpuid_length; i++) > + sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]); > + > + debug("cpuid: %s\n", cpuid_str); > + > + /* > + * Mix the cpuid bytes using the same rules as in > + * ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c > + */ > + for (i = 0; i < cpuid_length / 2; i++) { > + low[i] = cpuid[1 + (i << 1)]; > + high[i] = cpuid[i << 1]; > + } > + > + serialno = crc32_no_comp(0, low, cpuid_length / 2); > + serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) << > 32; > + snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno); > + > + oldid = env_get("cpuid#"); > + if (oldid && strcmp(oldid, cpuid_str) != 0) > + printf("cpuid: value %s present in env does not match hardware > %s\n", > + oldid, cpuid_str); > + > + env_set("cpuid#", cpuid_str); > + > + /* Only generate serial# when none is set yet */ > + if (!env_get("serial#")) > + env_set("serial#", serialno_str); > + > + return 0; > +} > + > __weak int rockchip_early_misc_init_r(void) > { > return 0; > diff --git a/arch/arm/mach-rockchip/misc.c > b/arch/arm/mach-rockchip/misc.c > deleted file mode 100644 > index 15397cff009..00000000000 > --- a/arch/arm/mach-rockchip/misc.c > +++ /dev/null > @@ -1,135 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0+ */ > -/* > - * RK3399: Architecture common definitions > - * > - * Copyright (C) 2019 Collabora Inc - https://www.collabora.com/ > - * Rohan Garg rohan.garg@collabora.com > - * > - * Based on puma-rk3399.c: > - * (C) Copyright 2017 Theobroma Systems Design und Consulting > GmbH > - */ > - > -#include <common.h> > -#include <env.h> > -#include <dm.h> > -#include <hash.h> > -#include <log.h> > -#include <dm/uclass-internal.h> > -#include <misc.h> > -#include <u-boot/crc.h> > -#include <u-boot/sha256.h> > - > -#include <asm/arch-rockchip/misc.h> > - > -int rockchip_setup_macaddr(void) > -{ > -#if CONFIG_IS_ENABLED(HASH) && CONFIG_IS_ENABLED(SHA256) > - int ret; > - const char *cpuid = env_get("cpuid#"); > - u8 hash[SHA256_SUM_LEN]; > - int size = sizeof(hash); > - u8 mac_addr[6]; > - > - /* Only generate a MAC address, if none is set in the environment */ > - if (env_get("ethaddr")) > - return 0; > - > - if (!cpuid) { > - debug("%s: could not retrieve 'cpuid#'\n", __func__); > - return -1; > - } > - > - ret = hash_block("sha256", (void *)cpuid, strlen(cpuid), hash, > &size); > - if (ret) { > - debug("%s: failed to calculate SHA256\n", __func__); > - return -1; > - } > - > - /* Copy 6 bytes of the hash to base the MAC address on */ > - memcpy(mac_addr, hash, 6); > - > - /* Make this a valid MAC address and set it */ > - mac_addr[0] &= 0xfe; /* clear multicast bit */ > - mac_addr[0] |= 0x02; /* set local assignment bit (IEEE802) */ > - eth_env_set_enetaddr("ethaddr", mac_addr); > - > - /* Make a valid MAC address for ethernet1 */ > - mac_addr[5] ^= 0x01; > - eth_env_set_enetaddr("eth1addr", mac_addr); > -#endif > - return 0; > -} > - > -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) > - struct udevice *dev; > - int ret; > - > - /* retrieve the device */ > -#if IS_ENABLED(CONFIG_ROCKCHIP_EFUSE) > - ret = uclass_get_device_by_driver(UCLASS_MISC, > - DM_DRIVER_GET(rockchip_efuse), &dev); > -#elif IS_ENABLED(CONFIG_ROCKCHIP_OTP) > - ret = uclass_get_device_by_driver(UCLASS_MISC, > - DM_DRIVER_GET(rockchip_otp), &dev); > -#endif > - if (ret) { > - debug("%s: could not find efuse device\n", __func__); > - return -1; > - } > - > - /* read the cpu_id range from the efuses */ > - ret = misc_read(dev, cpuid_offset, cpuid, cpuid_length); > - if (ret < 0) { > - debug("%s: reading cpuid from the efuses failed\n", > - __func__); > - return -1; > - } > -#endif > - return 0; > -} > - > -int rockchip_cpuid_set(const u8 *cpuid, const u32 cpuid_length) > -{ > - u8 low[cpuid_length / 2], high[cpuid_length / 2]; > - char cpuid_str[cpuid_length * 2 + 1]; > - u64 serialno; > - char serialno_str[17]; > - const char *oldid; > - int i; > - > - memset(cpuid_str, 0, sizeof(cpuid_str)); > - for (i = 0; i < cpuid_length; i++) > - sprintf(&cpuid_str[i * 2], "%02x", cpuid[i]); > - > - debug("cpuid: %s\n", cpuid_str); > - > - /* > - * Mix the cpuid bytes using the same rules as in > - * ${linux}/drivers/soc/rockchip/rockchip-cpuinfo.c > - */ > - for (i = 0; i < cpuid_length / 2; i++) { > - low[i] = cpuid[1 + (i << 1)]; > - high[i] = cpuid[i << 1]; > - } > - > - serialno = crc32_no_comp(0, low, cpuid_length / 2); > - serialno |= (u64)crc32_no_comp(serialno, high, cpuid_length / 2) << > 32; > - snprintf(serialno_str, sizeof(serialno_str), "%016llx", serialno); > - > - oldid = env_get("cpuid#"); > - if (oldid && strcmp(oldid, cpuid_str) != 0) > - printf("cpuid: value %s present in env does not match hardware > %s\n", > - oldid, cpuid_str); > - > - env_set("cpuid#", cpuid_str); > - > - /* Only generate serial# when none is set yet */ > - if (!env_get("serial#")) > - env_set("serial#", serialno_str); > - > - return 0; > -}

On 2024-02-09 10:50, Quentin Schulz wrote:

From: Quentin Schulz quentin.schulz@theobroma-systems.com

There's only one user of rockchip_capsule_update_board_setup, which is in board.c, and only one board defines it, so instead of having a header only for one function symbol, let's just use a weak symbol instead.

Cc: Quentin Schulz foss+uboot@0leil.net Reviewed-by: Kever Yang kever.yang@rock-chips.com Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

Looking good to me.

Reviewed-by: Dragan Simic dsimic@manjaro.org

---

arch/arm/include/asm/arch-rockchip/misc.h | 9 --------- arch/arm/mach-rockchip/board.c | 5 ++++- 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/arm/include/asm/arch-rockchip/misc.h b/arch/arm/include/asm/arch-rockchip/misc.h deleted file mode 100644 index ef37ff1661a..00000000000 --- a/arch/arm/include/asm/arch-rockchip/misc.h +++ /dev/null @@ -1,9 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/*

• • RK3399: Architecture common definitions
• • Copyright (C) 2019 Collabora Inc - https://www.collabora.com/

• •  Rohan Garg <rohan.garg@collabora.com>
    

• */

-void rockchip_capsule_update_board_setup(void); diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c index 80b4514852f..4f666aee706 100644 --- a/arch/arm/mach-rockchip/board.c +++ b/arch/arm/mach-rockchip/board.c @@ -32,7 +32,6 @@ #include <asm/arch-rockchip/boot_mode.h> #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/periph.h> -#include <asm/arch-rockchip/misc.h> #include <power/regulator.h>

#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION) @@ -148,6 +147,10 @@ void set_dfu_alt_info(char *interface, char *devstr) env_set("dfu_alt_info", buf); }

+__weak void rockchip_capsule_update_board_setup(void) +{ +}

static void gpt_capsule_update_setup(void) { int p, i, ret;

Hi Johan,

On 2/9/24 14:07, Johan Jonker wrote: [...]

From codingstyle.rst:

You should follow this ordering in U-Boot. The common.h header (which is going away at some point) should always be first, followed by other headers in order, then headers with directories, then local files. Within that order, sort your includes.

Good catch indeed. I'll avoid resending a 20-patch series today though.

I have the following diff that can be applied on top of that commit:

""" diff --git a/drivers/ram/rockchip/dmc-rk3368.c b/drivers/ram/rockchip/dmc-rk3368.c index 74d8aed571c..859fc47030f 100644 --- a/drivers/ram/rockchip/dmc-rk3368.c +++ b/drivers/ram/rockchip/dmc-rk3368.c @@ -17,10 +17,10 @@ #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/cru_rk3368.h> #include <asm/arch-rockchip/grf_rk3368.h> +#include <asm/arch-rockchip/hardware.h> #include <asm/arch-rockchip/ddr_rk3368.h> #include <asm/arch-rockchip/sdram.h> #include <asm/arch-rockchip/sdram_rk3288.h> -#include <asm/arch-rockchip/hardware.h> #include <linux/bitops.h> #include <linux/delay.h> #include <linux/err.h> diff --git a/drivers/ram/rockchip/sdram_rk3188.c b/drivers/ram/rockchip/sdram_rk3188.c index 16a68885f1f..d23d4231798 100644 --- a/drivers/ram/rockchip/sdram_rk3188.c +++ b/drivers/ram/rockchip/sdram_rk3188.c @@ -22,10 +22,10 @@ #include <asm/arch-rockchip/cru_rk3188.h> #include <asm/arch-rockchip/ddr_rk3188.h> #include <asm/arch-rockchip/grf_rk3188.h> +#include <asm/arch-rockchip/hardware.h> #include <asm/arch-rockchip/pmu_rk3188.h> #include <asm/arch-rockchip/sdram.h> #include <asm/arch-rockchip/sdram_rk3288.h> -#include <asm/arch-rockchip/hardware.h> #include <linux/delay.h> #include <linux/err.h>

diff --git a/drivers/ram/rockchip/sdram_rk3288.c b/drivers/ram/rockchip/sdram_rk3288.c index ec6bdcb2015..3177051dd12 100644 --- a/drivers/ram/rockchip/sdram_rk3288.c +++ b/drivers/ram/rockchip/sdram_rk3288.c @@ -22,10 +22,10 @@ #include <asm/arch-rockchip/cru.h> #include <asm/arch-rockchip/ddr_rk3288.h> #include <asm/arch-rockchip/grf_rk3288.h> +#include <asm/arch-rockchip/hardware.h> #include <asm/arch-rockchip/pmu_rk3288.h> #include <asm/arch-rockchip/sdram.h> #include <asm/arch-rockchip/sdram_rk3288.h> -#include <asm/arch-rockchip/hardware.h> #include <linux/delay.h> #include <linux/err.h> #include <power/regulator.h> """

You'll note that drivers/ram/rockchip/dmc-rk3368.c currently already doesn't follow the coding style :)

@Kever, up to you if you want me to send a v3 for that or if you can apply the diff above yourself before merging.

Cheers, Quentin

Hi Jonas,

On 2/11/24 22:34, Jonas Karlman wrote:

Hi Quentin,

On 2024-02-09 10:50, Quentin Schulz wrote:

...

From: Quentin Schulz quentin.schulz@theobroma-systems.com

JAGUAR is a Single-Board Computer (SBC) based around the rk3588 SoC and is targeting Autonomous Mobile Robots (AMR).

It features:

• LPDDR4X (up to 16GB)
• 1Gbps Ethernet on RJ45 connector (KSZ9031 or KSZ9131)
• PCIe 3.0 4-lane on M.2 M-key connector
• PCIe 2.1 1-lane on M.2 E-key
• USB 2.0 on M.2 E-key
• 2x USB3 OTG type-c ports with DP Alt-Mode
• USB2 host port
• HDMI output
• 2x camera connectors, each exposing:
  • 2-lane MIPI-CSI
  • 1v2, 1v8, 2v8 power rails
  • I2C bus
  • GPIOs
• PPS input
• CAN
• RS485 UART
• FAN connector
• SD card slot
• eMMC (up to 256GB)
• RTC backup battery
• Companion microcontroller
  • ISL1208 RTC emulation
  • AMC6821 PWM emulation
  • On/off buzzer control
• Secure Element
• 80-pin Mezzanine connector for daughterboards:
  • GPIOs
  • 1Gbps Ethernet
  • PCIe 2.1 1-lane
  • 2x 2-lane MIPI-CSI
  • ADC channel
  • I2C bus
  • PWM
  • UART
  • SPI
  • SDIO
  • CAN
  • I2S
  • 1v8, 3v3, 5v0, dc-in (12-24V) power rails

The Device Tree comes from next-20240110 Linux kernel.

Cc: Quentin Schulz foss+uboot@0leil.net Signed-off-by: Quentin Schulz quentin.schulz@theobroma-systems.com

---

arch/arm/dts/Makefile | 1 + arch/arm/dts/rk3588-jaguar-u-boot.dtsi | 38 + arch/arm/dts/rk3588-jaguar.dts | 803 +++++++++++++++++++++ arch/arm/mach-rockchip/rk3588/Kconfig | 28 + board/theobroma-systems/jaguar_rk3588/Kconfig | 16 + board/theobroma-systems/jaguar_rk3588/MAINTAINERS | 13 + board/theobroma-systems/jaguar_rk3588/Makefile | 10 + .../jaguar_rk3588/jaguar_rk3588.c | 52 ++ configs/jaguar-rk3588_defconfig | 115 +++ doc/board/index.rst | 1 + doc/board/rockchip/rockchip.rst | 1 + doc/board/theobroma-systems/index.rst | 9 + doc/board/theobroma-systems/jaguar_rk3588.rst | 100 +++ include/configs/jaguar_rk3588.h | 15 + 14 files changed, 1202 insertions(+)

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile index 3beb8f1b9d4..7c103655e1d 100644 --- a/arch/arm/dts/Makefile +++ b/arch/arm/dts/Makefile @@ -193,6 +193,7 @@ dtb-$(CONFIG_ROCKCHIP_RK3588) += \ rk3588-edgeble-neu6a-io.dtb \ rk3588-edgeble-neu6b-io.dtb \ rk3588-evb1-v10.dtb \

• rk3588-jaguar.dtb \ rk3588-nanopc-t6.dtb \ rk3588s-orangepi-5.dtb \ rk3588-orangepi-5-plus.dtb \

diff --git a/arch/arm/dts/rk3588-jaguar-u-boot.dtsi b/arch/arm/dts/rk3588-jaguar-u-boot.dtsi new file mode 100644 index 00000000000..a0207c265c9 --- /dev/null +++ b/arch/arm/dts/rk3588-jaguar-u-boot.dtsi @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/*

• • Copyright (c) 2023 Theobroma Systems Design und Consulting GmbH
• */

+#include "rk3588-u-boot.dtsi" +#include <dt-bindings/pinctrl/rockchip.h> +#include <dt-bindings/input/input.h> +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/usb/pd.h>

These dt-bindings includes should not be needed in this file.

...

+/ {

• chosen {

• u-boot,spl-boot-order = "same-as-spl", &sdhci, &sdmmc;
  

Is there specific reasoning behind why eMMC is listed before SD-card?

Consistency with our other products (Ringneck PX30 and Puma RK3399).

rk3588-u-boot.dtsi should now contain a default with a same-as-spl > SD-card > eMMC order.

Also guess in practice it should not matter mutch, if TPL/SPL is running from eMMC it will read FIT from eMMC > eMMC > SD-card. And if TPL/SPL is running from SD-card it will read FIT from SD-card > eMMC > SD-card.

I was pondering whether we should do something about this, to avoid reading twice from the same boot medium, this unnecessarily increase the boot time in case we're entering the fallback mechanism.

Main difference would be if TPL/SPL is running from SPI, then it will read FIT from SPI > eMMC > SD-card instead of SPI > SD-card > eMMC.

Yes, we do have an SPI-NOR on Puma, and we want SPI -> eMMC -> SD card there (mainly because that's what we've always done, thus now for "backward" compatibility).

For SPI-less systems, I guess it doesn't matter much indeed.

In my mind anything after same-as-spl would mostly be used for recovery. And preferring SD-card may ease in such situation?

No, this we don't want. But this is just a different policy from Rockchip's default.

c.f. https://lore.kernel.org/u-boot/7b5cb03f-c2b2-4cf2-b06a-751d7cc978b9@theobrom...

...

• };

+};

+&emmc_pwrseq {

• bootph-all;

+};

+&emmc_reset {

• bootph-all;

+};

+&gpio0 {

• bootph-all;

+};

+&gpio2 {

• bootph-all;

I am guessing that emmc_pwrseq, gpio0 and gpio2 would not be needed in TPL so bootph-all could be too inclusive, for now I guess this does not really matter because an external TPL blob is used anyway.

Don't have much of a choice sadly, c.f. https://source.denx.de/u-boot/u-boot/-/commit/9e644284ab812f2db23f6185af77c0... Considering that we need the eMMC in U-Boot proper before relocation, I assume I need the emmc_pwrseq as well in bootph-all.

As for gpio0, I think we use one of the pins in that bank for the NVMe but we indeed wouldn't need it before relocation. Especially since I somehow didn't manage to get it to work in U-Boot yet.

Cheers, Quentin