Re: [PATCH 07/10] stm32mp1: spl: Configure MAC address when booting OP-TEE

[Alex G.]

On 8/31/21 12:10 PM, Patrick DELAUNAY wrote:
Hi,

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
When OP-TEE is booted as the SPL payload, the stage after OP-TEE is
not guaranteed to be u-boot. Thus the FDT patching in u-boot is not
guaranteed to occur. Add this step to SPL.

The patching by stm32_fdt_setup_mac_addr() is done in SPL, and patches
the target FDT directly. This differs is different from
setup_mac_address(), which sets the "ethaddr" env variable, and does
not work in SPL.

Signed-off-by: Alexandru Gagniuc <mr.nuke...@gmail.com>
---
  arch/arm/mach-stm32mp/cpu.c                   | 22 +++++++++++++++++++
  .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 +++
  arch/arm/mach-stm32mp/spl.c                   |  1 +
  3 files changed, 26 insertions(+)

diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index 8727de513c..2b8b67bb40 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -10,6 +10,7 @@
  #include <cpu_func.h>
  #include <debug_uart.h>
  #include <env.h>
+#include <fdt_support.h>
  #include <init.h>
  #include <log.h>
  #include <lmb.h>
@@ -646,6 +647,27 @@ __weak int setup_mac_address(void)
      return 0;
  }
+int stm32_fdt_setup_mac_addr(void *fdt)
+{
+    int ret;
+    uchar enetaddr[ARP_HLEN];
+
+    ret = stm32_read_otp_mac(enetaddr);
+    if (ret < 0)
+        return ret;
+
+    if (!is_valid_ethaddr(enetaddr)) {
+        printf("invalid MAC address in OTP\n");
+        return -EINVAL;
+    }
+
+    ret = fdt_ethernet_set_macaddr(fdt, 0, enetaddr);
+    if (ret)
+        debug("Failed to set mac address from OTP: %d\n", ret);
+
+    return ret;
+}
+
  static int setup_serial_number(void)
  {
      char serial_string[25];
diff --git a/arch/arm/mach-stm32mp/include/mach/sys_proto.h 
b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
index 4149d3a133..2d24cfee3f 100644
--- a/arch/arm/mach-stm32mp/include/mach/sys_proto.h
+++ b/arch/arm/mach-stm32mp/include/mach/sys_proto.h
@@ -47,7 +47,10 @@ void get_soc_name(char name[SOC_NAME_SIZE]);
  /* return boot mode */
  u32 get_bootmode(void);
+/* Set 'ethaddr' env variable with MAC from OTP (useful for u-boot 
proper) */
  int setup_mac_address(void);
+/* Patch the first 'ethernet' node of FDT with MAC from OTP (useful 
for SPL) */
+int stm32_fdt_setup_mac_addr(void *fdt);
  /* board power management : configure vddcore according OPP */
  void board_vddcore_init(u32 voltage_mv);
diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
index 405eff68a3..d9fdc5926c 100644
--- a/arch/arm/mach-stm32mp/spl.c
+++ b/arch/arm/mach-stm32mp/spl.c
@@ -181,6 +181,7 @@ void stm32_init_tzc_for_optee(void)
  void spl_board_prepare_for_optee(void *fdt)
  {
+    stm32_fdt_setup_mac_addr(fdt);
      stm32_init_tzc_for_optee();
  }


I think that all this part is not required for falcon mode:

let the Linux driver fallback to nvmem access to read the MAC address in 
OTP when the DT is not updated by boot loader

remove also the patch 06/10 and 04/40

PS: it is already working in OpenSTLinux based on the device tree 
modification for your board

&bsec{
             ethernet_mac_address: mac@e4 {
             reg = <0xe4 0x6>;
};

&ethernet0 {
     nvmem-cells = <&ethernet_mac_address>;
     nvmem-cell-names = "mac-address";
};

I was looking at this, following Sean's suggestion from last week. The 
linux BSEC driver backends on stm32_bsec_smc(). This requires a 
corresponding secure-world callout. which is not currently implemented 
for SPL. We'd just get "Can't read data57 (-5)" error from linux, and a 
random MAC address.

Is this change as proposed ideal? Probably not. It works with SPL and 
the current LTS linux (v5.10), and is likely the least intensive 
solution in terms of lines of code.

Alex

PS: this part is not yet upstreamed in Linux

That's not problematic. I can accommodate devicetree changes by using 
overlays in the FIT image. This flexibility is also a huge reason why 
I've chosen to go with FIT versus other image formats.