Hello Peter,

On 2024-02-12 14:56, Peter Robinson wrote:
On Fri, 9 Feb 2024 at 18:57, Dragan Simic <dsi...@manjaro.org> wrote:
On 2024-02-09 19:36, Mark Kettenis wrote:
>> Date: Fri, 09 Feb 2024 18:58:01 +0100
>> From: Dragan Simic <dsi...@manjaro.org>
>> Please, see my comments below.
>>
>> On 2024-02-09 10:50, Quentin Schulz wrote:
>> > From: Quentin Schulz <quentin.sch...@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+ub...@0leil.net>
>> > Reviewed-by: Kever Yang <kever.y...@rock-chips.com>
>> > Signed-off-by: Quentin Schulz <quentin.sch...@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.g...@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.g...@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.g...@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;
>> > -}
>>

Reply via email to