[PATCH v2 09/20] rockchip: merge misc.c into board.c
Dragan Simic
dsimic at manjaro.org
Mon Feb 12 15:30:46 CET 2024
On 2024-02-12 15:21, Peter Robinson wrote:
> On Mon, 12 Feb 2024 at 14:16, Dragan Simic <dsimic at manjaro.org> wrote:
>> On 2024-02-12 14:56, Peter Robinson wrote:
>> > On Fri, 9 Feb 2024 at 18:57, Dragan Simic <dsimic at 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 at manjaro.org>
>> >> >> Please, see my comments below.
>> >> >>
>> >> >> On 2024-02-09 10:50, Quentin Schulz wrote:
>> >> >> > From: Quentin Schulz <quentin.schulz at 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 at 0leil.net>
>> >> >> > Reviewed-by: Kever Yang <kever.yang at rock-chips.com>
>> >> >> > Signed-off-by: Quentin Schulz <quentin.schulz at 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 at 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 at 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 at 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;
>> >> >> > -}
>> >> >>
More information about the U-Boot
mailing list