[PATCH 00/10] stm32mp1: Support falcon mode with OP-TEE payloads

[PATCH 00/10] stm32mp1: Support falcon mode with OP-TEE payloads

[Alexandru Gagniuc]

My goal when I started on this project a year ago was to get to linux
userspace within a second from power on. Oh, and it had to be secure!
Contrast that to the two minutes it took the STLinux demo to come up.

It was obvious that the accepted way of running an FSBL, then SSBL was
going to blow the time budget. There really wasn't a good solution,
and traditional falcon mode with "spl export" command was not secure.

I chose to use SPL with a FIT payload. We have to add certain logic to
SPL, as well as some FDT modifications that would be normally done in
u-boot. The boot flow is

	SPL -> OP-TEE -> Linux

Incidentally, these patches are some of the earlier ones I wrote for
this project. It didn't make sense to publish them at the time, as the
supporting infrastructure was not in place then

I decided not to separate these patches into mini-series.

Alexandru Gagniuc (10):
  stm32mp1: Add support for baudrates higher than 115200
  stm32mp1: Add support for falcon mode boot from SD card
  board: stm32mp1:  Implement board_fit_config_name_match() for SPL
  fdt_support: Implement fdt_ethernet_set_macaddr()
  arm: stm32mp: bsec: Do not skip .probe() for SPL
  arm: stm32mp: Factor out reading MAC address from OTP
  stm32mp1: spl: Configure MAC address when booting OP-TEE
  lib: Makefile: Make optee library available in SPL
  ARM: dts: stm32mp: Add OP-TEE "/firmware" node to SPL dtb
  stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

 arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi      |  1 +
 arch/arm/mach-stm32mp/bsec.c                  |  2 +-
 arch/arm/mach-stm32mp/cpu.c                   | 59 ++++++++++++++-----
 .../arm/mach-stm32mp/include/mach/sys_proto.h |  3 +
 arch/arm/mach-stm32mp/spl.c                   |  3 +
 board/st/stm32mp1/spl.c                       | 49 +++++++++++++++
 common/fdt_support.c                          | 30 ++++++++++
 include/configs/stm32mp1.h                    | 17 ++++++
 include/fdt_support.h                         | 17 ++++++
 lib/Makefile                                  |  2 +-
 10 files changed, 167 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH 01/10] stm32mp1: Add support for baudrates higher than 115200

[Alexandru Gagniuc]

The UART can reliably go up to 2000000 baud when connected to the
on-board st-link. Unfortunately u-boot will fall back to 115200 unless
higher rates are declared via CONFIG_SYS_BAUDRATE_TABLE.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/configs/stm32mp1.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h
index b372838be8..9fcd60285a 100644
--- a/include/configs/stm32mp1.h
+++ b/include/configs/stm32mp1.h
@@ -16,6 +16,10 @@
 #define CONFIG_ARMV7_SECURE_MAX_SIZE		STM32_SYSRAM_SIZE
 #endif
 
+#define CONFIG_SYS_BAUDRATE_TABLE      { 9600, 19200, 38400, 57600, 115200, \
+					 230400, 460800, 921600, \
+					 1000000, 2000000 }
+
 /*
  * Configuration of the external SRAM memory used by U-Boot
  */
-- 
2.31.1

Re: [PATCH 01/10] stm32mp1: Add support for baudrates higher than 115200

[Patrick DELAUNAY]

Hi,

I add in CC the ARM STM STM32MP Maintainers...

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
> The UART can reliably go up to 2000000 baud when connected to the
> on-board st-link. Unfortunately u-boot will fall back to 115200 unless
> higher rates are declared via CONFIG_SYS_BAUDRATE_TABLE.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   include/configs/stm32mp1.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h
> index b372838be8..9fcd60285a 100644
> --- a/include/configs/stm32mp1.h
> +++ b/include/configs/stm32mp1.h
> @@ -16,6 +16,10 @@
>   #define CONFIG_ARMV7_SECURE_MAX_SIZE		STM32_SYSRAM_SIZE
>   #endif
>   
> +#define CONFIG_SYS_BAUDRATE_TABLE      { 9600, 19200, 38400, 57600, 115200, \
> +					 230400, 460800, 921600, \
> +					 1000000, 2000000 }
> +
>   /*
>    * Configuration of the external SRAM memory used by U-Boot
>    */

Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

[PATCH 02/10] stm32mp1: Add support for falcon mode boot from SD card

[Alexandru Gagniuc]

Falcon mode requires a board-specific mechanism to select between
fast and normal boot. This is done via spl_start_uboot()

Use the B2 button as the selection mechanism. This is connected to
GPIO PA13. Incidentally, this GPIO is already accessible via the
"st,fastboot-gpios" devicetree node.

Offsets for raw MMC loading are defined. These point to the partition
after "ssbl".

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 board/st/stm32mp1/spl.c    | 39 ++++++++++++++++++++++++++++++++++++++
 include/configs/stm32mp1.h | 13 +++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/board/st/stm32mp1/spl.c b/board/st/stm32mp1/spl.c
index 8e4549a1b3..bb210d7727 100644
--- a/board/st/stm32mp1/spl.c
+++ b/board/st/stm32mp1/spl.c
@@ -8,6 +8,7 @@
 #include <init.h>
 #include <asm/io.h>
 #include <asm/arch/sys_proto.h>
+#include <asm/gpio.h>
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include "../common/stpmic1.h"
@@ -29,6 +30,44 @@ int board_early_init_f(void)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_SPL_OS_BOOT)
+int spl_start_uboot(void)
+{
+	ofnode node;
+	struct gpio_desc gpio;
+	int boot_uboot = 1;
+
+	node = ofnode_path("/config");
+	if (!ofnode_valid(node)) {
+		pr_warn("%s: no /config node?\n", __func__);
+		return 0;
+	}
+
+	if (gpio_request_by_name_nodev(node, "st,fastboot-gpios", 0, &gpio,
+				       GPIOD_IS_IN)) {
+		pr_warn("%s: could not find a /config/st,fastboot-gpios\n",
+			__func__);
+		return 1;
+	}
+
+	boot_uboot = dm_gpio_get_value(&gpio);
+	dm_gpio_free(NULL, &gpio);
+
+	return boot_uboot;
+}
+
+#if IS_ENABLED(CONFIG_ARMV7_NONSEC)
+/*
+ * A bit of a hack, but armv7_boot_nonsec() is provided by bootm.c. This is not
+ * available in SPL, so we have to provide an implementation.
+ */
+bool armv7_boot_nonsec(void)
+{
+	return 0;
+}
+#endif /* CONFIG_ARMV7_NONSEC */
+#endif /* CONFIG_SPL_OS_BOOT */
+
 #ifdef CONFIG_DEBUG_UART_BOARD_INIT
 void board_debug_uart_init(void)
 {
diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h
index 9fcd60285a..0849a1bddb 100644
--- a/include/configs/stm32mp1.h
+++ b/include/configs/stm32mp1.h
@@ -10,6 +10,19 @@
 #include <linux/sizes.h>
 #include <asm/arch/stm32.h>
 
+/*
+ * Arguments if falcon mode is used
+ * CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR is the partition after "ssbl"
+ * CONFIG_SYS_SPL_ARGS_ADDR is not used, but needs to point to valid RAM.
+ */
+#define CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR 5154
+#define CONFIG_SYS_SPL_ARGS_ADDR 0xc4000000
+
+/* Falcon mode from SPI is not supported, but the defines are needed */
+#define CONFIG_SYS_SPI_KERNEL_OFFS	(~0)
+#define CONFIG_SYS_SPI_ARGS_OFFS	(~0)
+#define CONFIG_SYS_SPI_ARGS_SIZE	0
+
 #ifndef CONFIG_TFABOOT
 /* PSCI support */
 #define CONFIG_ARMV7_SECURE_BASE		STM32_SYSRAM_BASE
-- 
2.31.1

Re: [PATCH 02/10] stm32mp1: Add support for falcon mode boot from SD card

[Patrick DELAUNAY]

Hi,

Add in CC the MAINTAINERS

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
> Falcon mode requires a board-specific mechanism to select between
> fast and normal boot. This is done via spl_start_uboot()
>
> Use the B2 button as the selection mechanism. This is connected to
> GPIO PA13. Incidentally, this GPIO is already accessible via the
> "st,fastboot-gpios" devicetree node.
>
> Offsets for raw MMC loading are defined. These point to the partition
> after "ssbl".
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   board/st/stm32mp1/spl.c    | 39 ++++++++++++++++++++++++++++++++++++++
>   include/configs/stm32mp1.h | 13 +++++++++++++
>   2 files changed, 52 insertions(+)
>
> diff --git a/board/st/stm32mp1/spl.c b/board/st/stm32mp1/spl.c
> index 8e4549a1b3..bb210d7727 100644
> --- a/board/st/stm32mp1/spl.c
> +++ b/board/st/stm32mp1/spl.c
> @@ -8,6 +8,7 @@
>   #include <init.h>
>   #include <asm/io.h>
>   #include <asm/arch/sys_proto.h>
> +#include <asm/gpio.h>
>   #include <linux/bitops.h>
>   #include <linux/delay.h>
>   #include "../common/stpmic1.h"
> @@ -29,6 +30,44 @@ int board_early_init_f(void)
>   	return 0;
>   }
>   
> +#if IS_ENABLED(CONFIG_SPL_OS_BOOT)
> +int spl_start_uboot(void)
> +{
> +	ofnode node;
> +	struct gpio_desc gpio;
> +	int boot_uboot = 1;
> +
> +	node = ofnode_path("/config");
> +	if (!ofnode_valid(node)) {
> +		pr_warn("%s: no /config node?\n", __func__);
> +		return 0;
> +	}
> +
> +	if (gpio_request_by_name_nodev(node, "st,fastboot-gpios", 0, &gpio,
> +				       GPIOD_IS_IN)) {
> +		pr_warn("%s: could not find a /config/st,fastboot-gpios\n",
> +			__func__);
> +		return 1;
> +	}

The node "st,fastboot-gpios" is used in STMicroelectronics devicetree / board
to select the KEY to launch the ANDROID command fastboot in board_key_check()

=> it can't be re-used to other purpose else you will have conflict when the key
    is pressed:
    
    what append when key pressed in basic boot mode when KEY is pressed ?
    => falcon mode selected in SPL or FASTBOOT mode selected ....

"st,fastboot-gpios" meaning is defined in:

https://wiki.st.com/stm32mpu/wiki/How_to_configure_U-Boot_for_your_board#Config_node

you should use a other config ?
=> "mrnuke,falcon-gpios" managed in your board .c file / dts file

or hardcoded in your board.c ?

or use environment....
if (env_get_yesno("boot_os") != 0)

> +
> +	boot_uboot = dm_gpio_get_value(&gpio);
> +	dm_gpio_free(NULL, &gpio);
> +
> +	return boot_uboot;
> +}
> +
> +#if IS_ENABLED(CONFIG_ARMV7_NONSEC)
> +/*
> + * A bit of a hack, but armv7_boot_nonsec() is provided by bootm.c. This is not
> + * available in SPL, so we have to provide an implementation.
> + */
> +bool armv7_boot_nonsec(void)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_ARMV7_NONSEC */
> +#endif /* CONFIG_SPL_OS_BOOT */
> +
>   #ifdef CONFIG_DEBUG_UART_BOARD_INIT
>   void board_debug_uart_init(void)
>   {
> diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h
> index 9fcd60285a..0849a1bddb 100644
> --- a/include/configs/stm32mp1.h
> +++ b/include/configs/stm32mp1.h
> @@ -10,6 +10,19 @@
>   #include <linux/sizes.h>
>   #include <asm/arch/stm32.h>
>   
> +/*
> + * Arguments if falcon mode is used
> + * CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR is the partition after "ssbl"
> + * CONFIG_SYS_SPL_ARGS_ADDR is not used, but needs to point to valid RAM.
> + */
> +#define CONFIG_SYS_MMCSD_RAW_MODE_KERNEL_SECTOR 5154

The offset of kernel is hardcoded

=> it is not acceptable here (generic stm32mp1 file)

> +#define CONFIG_SYS_SPL_ARGS_ADDR 0xc4000000
> +
> +/* Falcon mode from SPI is not supported, but the defines are needed */
> +#define CONFIG_SYS_SPI_KERNEL_OFFS	(~0)
> +#define CONFIG_SYS_SPI_ARGS_OFFS	(~0)
> +#define CONFIG_SYS_SPI_ARGS_SIZE	0
> +

Falcon mode is not supported by ST Microelectronics
and this file is expected to support the ST boards (board/st/stm32mp1)

=> these defines should be in your configuration not in the GENERIC stm32mp1 files

for example in

include/configs/stm32mp15_falcon.h

#include "stm32mp1.h"

....

with CONFIG_SYS_CONFIG_NAME =  "stm32mp15_falcon" in your Kconfig


>   #ifndef CONFIG_TFABOOT
>   /* PSCI support */
>   #define CONFIG_ARMV7_SECURE_BASE		STM32_SYSRAM_BASE


Patrick

[PATCH 03/10] board: stm32mp1: Implement board_fit_config_name_match() for SPL

[Alexandru Gagniuc]

This function is needed when loading a FIT image from SPL. It selects
the correct configuration node for the current board. Implement it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 board/st/stm32mp1/spl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/board/st/stm32mp1/spl.c b/board/st/stm32mp1/spl.c
index bb210d7727..543c037ad8 100644
--- a/board/st/stm32mp1/spl.c
+++ b/board/st/stm32mp1/spl.c
@@ -5,6 +5,7 @@
 
 #include <config.h>
 #include <common.h>
+#include <dm/device.h>
 #include <init.h>
 #include <asm/io.h>
 #include <asm/arch/sys_proto.h>
@@ -92,3 +93,12 @@ void board_debug_uart_init(void)
 #endif
 }
 #endif
+
+int board_fit_config_name_match(const char *name)
+{
+	if (of_machine_is_compatible("st,stm32mp157c-dk2"))
+		return !strstr(name, "stm32mp157c-dk2");
+
+	/* Okay, it's most likely an EV board */
+	return !strstr(name, "stm32mp157") + !strstr(name, "-ev");
+}
-- 
2.31.1

Re: [PATCH 03/10] board: stm32mp1: Implement board_fit_config_name_match() for SPL

[Patrick DELAUNAY]

Hi

Add in CC the MAINTAINERS.

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
> This function is needed when loading a FIT image from SPL. It selects
> the correct configuration node for the current board. Implement it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   board/st/stm32mp1/spl.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/board/st/stm32mp1/spl.c b/board/st/stm32mp1/spl.c
> index bb210d7727..543c037ad8 100644
> --- a/board/st/stm32mp1/spl.c
> +++ b/board/st/stm32mp1/spl.c
> @@ -5,6 +5,7 @@
>   
>   #include <config.h>
>   #include <common.h>
> +#include <dm/device.h>
>   #include <init.h>
>   #include <asm/io.h>
>   #include <asm/arch/sys_proto.h>
> @@ -92,3 +93,12 @@ void board_debug_uart_init(void)
>   #endif
>   }
>   #endif
> +
> +int board_fit_config_name_match(const char *name)
> +{
> +	if (of_machine_is_compatible("st,stm32mp157c-dk2"))
> +		return !strstr(name, "stm32mp157c-dk2");
> +
> +	/* Okay, it's most likely an EV board */
> +	return !strstr(name, "stm32mp157") + !strstr(name, "-ev");
> +}

It is not working for all STMicroelectronics boards....

=> st,stm32mp157a-dk1 for example

based on board_late_init => I propose board_name extraction from compatible :

#ifdef CONFIG_SPL_LOAD_FIT
int board_fit_config_name_match(const char *name)
{
	const void *fdt_compat;
	int fdt_compat_len;

	fdt_compat = fdt_getprop(gd->fdt_blob, 0, "compatible", &fdt_compat_len);

	/* only STMicrolectronics board are supported */
	if (strncmp(fdt_compat, "st,", 3) != 0)
		return 1;
	
	return !strstr(name, fdt_compat + 3);
}
#endif

[PATCH 04/10] fdt_support: Implement fdt_ethernet_set_macaddr()

[Alexandru Gagniuc]

Oftentimes we have MAC address information stored in a ROM or OTP. The
way to add that to the FDT would be through the u-boot environment,
and then fdt_fixup_ethernet(). This is not very useful in SPL.

It would be more helpful to be able to "set interface x to MAC y".
This is where fdt_ethernet_set_macaddr() comes in. It is similar in
function to fdt_fixup_ethernet(), but only updates one interface,
without using the u-boot env, and without string processing.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/fdt_support.c  | 30 ++++++++++++++++++++++++++++++
 include/fdt_support.h | 17 +++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 4341d84bd5..c4cbd4060e 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -592,6 +592,36 @@ void fdt_fixup_ethernet(void *fdt)
 	}
 }
 
+int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t *mac_addr)
+{
+	const char *path, *name;
+	int prop, aliases_node;
+	char eth_name[16] = "ethernet";
+
+	aliases_node = fdt_path_offset(fdt, "/aliases");
+	if (aliases_node < 0)
+		return aliases_node;
+
+	if (ethnum >= 0)
+		sprintf(eth_name, "ethernet%d", ethnum);
+
+	fdt_for_each_property_offset(prop, fdt, aliases_node) {
+		path = fdt_getprop_by_offset(fdt, prop, &name, NULL);
+		if (!strcmp(name, eth_name))
+			break;
+
+		path = NULL;
+	}
+
+	if (!path)
+		return -FDT_ERR_NOTFOUND;
+
+	do_fixup_by_path(fdt, path, "mac-address", mac_addr, 6, 0);
+	do_fixup_by_path(fdt, path, "local-mac-address", mac_addr, 6, 1);
+
+	return 0;
+}
+
 int fdt_record_loadable(void *blob, u32 index, const char *name,
 			uintptr_t load_addr, u32 size, uintptr_t entry_point,
 			const char *type, const char *os, const char *arch)
diff --git a/include/fdt_support.h b/include/fdt_support.h
index f6f46bb8e9..3f0bcb5a00 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -119,6 +119,23 @@ static inline int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[],
 #endif
 
 void fdt_fixup_ethernet(void *fdt);
+
+/**
+ * Set the "mac-address" and "local-mac-address" of ethernet node
+ * The ethernet node is located from the "/aliases" section of the fdt. When
+ * 'ethnum' is positive, then the name is matched exactly, e.g "ethernet0".
+ * When ethnum is negative, the first ethernet alias is updated.
+ * Unlike fdt_fixup_ethernet(), this function only updates one ethernet node,
+ * and soes not use the "ethaddr" from the u-boot environment. This is useful,
+ * for example, in SPL, when the environment is not initialized or available.
+ *
+ * @param fdt		FDT blob to update
+ * @param ethnum	Ethernet device index, or negative for any ethernet
+ * @param mac_addr	Pointer to 6-byte array containing the MAC address
+ *
+ * @return 0 if ok, or -FDT_ERR_... on error
+ */
+int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t *mac_addr);
 int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
 			 const void *val, int len, int create);
 void fdt_fixup_qe_firmware(void *fdt);
-- 
2.31.1

Re: [PATCH 04/10] fdt_support: Implement fdt_ethernet_set_macaddr()

[Sean Anderson]

On 8/26/21 5:42 PM, Alexandru Gagniuc wrote:
> Oftentimes we have MAC address information stored in a ROM or OTP. The
> way to add that to the FDT would be through the u-boot environment,
> and then fdt_fixup_ethernet(). This is not very useful in SPL.
>
> It would be more helpful to be able to "set interface x to MAC y".
> This is where fdt_ethernet_set_macaddr() comes in. It is similar in
> function to fdt_fixup_ethernet(), but only updates one interface,
> without using the u-boot env, and without string processing.

Have you considered adopting the nvmem-cells property for ethernet
controllers (added in Linux commit 0e839df92cf3 ("net: ethernet: provide
nvmem_get_mac_address()"))?

--Sean

> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   common/fdt_support.c  | 30 ++++++++++++++++++++++++++++++
>   include/fdt_support.h | 17 +++++++++++++++++
>   2 files changed, 47 insertions(+)
>
> diff --git a/common/fdt_support.c b/common/fdt_support.c
> index 4341d84bd5..c4cbd4060e 100644
> --- a/common/fdt_support.c
> +++ b/common/fdt_support.c
> @@ -592,6 +592,36 @@ void fdt_fixup_ethernet(void *fdt)
>   	}
>   }
>
> +int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t *mac_addr)
> +{
> +	const char *path, *name;
> +	int prop, aliases_node;
> +	char eth_name[16] = "ethernet";
> +
> +	aliases_node = fdt_path_offset(fdt, "/aliases");
> +	if (aliases_node < 0)
> +		return aliases_node;
> +
> +	if (ethnum >= 0)
> +		sprintf(eth_name, "ethernet%d", ethnum);
> +
> +	fdt_for_each_property_offset(prop, fdt, aliases_node) {
> +		path = fdt_getprop_by_offset(fdt, prop, &name, NULL);
> +		if (!strcmp(name, eth_name))
> +			break;
> +
> +		path = NULL;
> +	}
> +
> +	if (!path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	do_fixup_by_path(fdt, path, "mac-address", mac_addr, 6, 0);
> +	do_fixup_by_path(fdt, path, "local-mac-address", mac_addr, 6, 1);
> +
> +	return 0;
> +}
> +
>   int fdt_record_loadable(void *blob, u32 index, const char *name,
>   			uintptr_t load_addr, u32 size, uintptr_t entry_point,
>   			const char *type, const char *os, const char *arch)
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index f6f46bb8e9..3f0bcb5a00 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -119,6 +119,23 @@ static inline int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[],
>   #endif
>
>   void fdt_fixup_ethernet(void *fdt);
> +
> +/**
> + * Set the "mac-address" and "local-mac-address" of ethernet node
> + * The ethernet node is located from the "/aliases" section of the fdt. When
> + * 'ethnum' is positive, then the name is matched exactly, e.g "ethernet0".
> + * When ethnum is negative, the first ethernet alias is updated.
> + * Unlike fdt_fixup_ethernet(), this function only updates one ethernet node,
> + * and soes not use the "ethaddr" from the u-boot environment. This is useful,
> + * for example, in SPL, when the environment is not initialized or available.
> + *
> + * @param fdt		FDT blob to update
> + * @param ethnum	Ethernet device index, or negative for any ethernet
> + * @param mac_addr	Pointer to 6-byte array containing the MAC address
> + *
> + * @return 0 if ok, or -FDT_ERR_... on error
> + */
> +int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t *mac_addr);
>   int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
>   			 const void *val, int len, int create);
>   void fdt_fixup_qe_firmware(void *fdt);
>

Re: [PATCH 04/10] fdt_support: Implement fdt_ethernet_set_macaddr()

[Alex G.]

Hi Sean,

On 8/26/21 6:35 PM, Sean Anderson wrote:
> 
> 
> On 8/26/21 5:42 PM, Alexandru Gagniuc wrote:
>> Oftentimes we have MAC address information stored in a ROM or OTP. The
>> way to add that to the FDT would be through the u-boot environment,
>> and then fdt_fixup_ethernet(). This is not very useful in SPL.
>>
>> It would be more helpful to be able to "set interface x to MAC y".
>> This is where fdt_ethernet_set_macaddr() comes in. It is similar in
>> function to fdt_fixup_ethernet(), but only updates one interface,
>> without using the u-boot env, and without string processing.
> 
> Have you considered adopting the nvmem-cells property for ethernet
> controllers (added in Linux commit 0e839df92cf3 ("net: ethernet: provide
> nvmem_get_mac_address()"))?

Obviously I haven't. It sounds like a great idea. Thank you for pointing 
me to it.

Alex

> --Sean
> 
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   common/fdt_support.c  | 30 ++++++++++++++++++++++++++++++
>>   include/fdt_support.h | 17 +++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/common/fdt_support.c b/common/fdt_support.c
>> index 4341d84bd5..c4cbd4060e 100644
>> --- a/common/fdt_support.c
>> +++ b/common/fdt_support.c
>> @@ -592,6 +592,36 @@ void fdt_fixup_ethernet(void *fdt)
>>       }
>>   }
>>
>> +int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t 
>> *mac_addr)
>> +{
>> +    const char *path, *name;
>> +    int prop, aliases_node;
>> +    char eth_name[16] = "ethernet";
>> +
>> +    aliases_node = fdt_path_offset(fdt, "/aliases");
>> +    if (aliases_node < 0)
>> +        return aliases_node;
>> +
>> +    if (ethnum >= 0)
>> +        sprintf(eth_name, "ethernet%d", ethnum);
>> +
>> +    fdt_for_each_property_offset(prop, fdt, aliases_node) {
>> +        path = fdt_getprop_by_offset(fdt, prop, &name, NULL);
>> +        if (!strcmp(name, eth_name))
>> +            break;
>> +
>> +        path = NULL;
>> +    }
>> +
>> +    if (!path)
>> +        return -FDT_ERR_NOTFOUND;
>> +
>> +    do_fixup_by_path(fdt, path, "mac-address", mac_addr, 6, 0);
>> +    do_fixup_by_path(fdt, path, "local-mac-address", mac_addr, 6, 1);
>> +
>> +    return 0;
>> +}
>> +
>>   int fdt_record_loadable(void *blob, u32 index, const char *name,
>>               uintptr_t load_addr, u32 size, uintptr_t entry_point,
>>               const char *type, const char *os, const char *arch)
>> diff --git a/include/fdt_support.h b/include/fdt_support.h
>> index f6f46bb8e9..3f0bcb5a00 100644
>> --- a/include/fdt_support.h
>> +++ b/include/fdt_support.h
>> @@ -119,6 +119,23 @@ static inline int fdt_fixup_memory_banks(void 
>> *blob, u64 start[], u64 size[],
>>   #endif
>>
>>   void fdt_fixup_ethernet(void *fdt);
>> +
>> +/**
>> + * Set the "mac-address" and "local-mac-address" of ethernet node
>> + * The ethernet node is located from the "/aliases" section of the 
>> fdt. When
>> + * 'ethnum' is positive, then the name is matched exactly, e.g 
>> "ethernet0".
>> + * When ethnum is negative, the first ethernet alias is updated.
>> + * Unlike fdt_fixup_ethernet(), this function only updates one 
>> ethernet node,
>> + * and soes not use the "ethaddr" from the u-boot environment. This 
>> is useful,
>> + * for example, in SPL, when the environment is not initialized or 
>> available.
>> + *
>> + * @param fdt        FDT blob to update
>> + * @param ethnum    Ethernet device index, or negative for any ethernet
>> + * @param mac_addr    Pointer to 6-byte array containing the MAC address
>> + *
>> + * @return 0 if ok, or -FDT_ERR_... on error
>> + */
>> +int fdt_ethernet_set_macaddr(void *fdt, int ethnum, const uint8_t 
>> *mac_addr);
>>   int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
>>                const void *val, int len, int create);
>>   void fdt_fixup_qe_firmware(void *fdt);
>>

[PATCH 05/10] arm: stm32mp: bsec: Do not skip .probe() for SPL

[Alexandru Gagniuc]

stm32mp_bsec_probe() was skipped for TFABOOT and SPL_BUILD. The idea
of skipping probe() is that we can't access BSEC from the normal
world. This is true with TFABOOT. However, in SPL, we are in the
secure world, so skipping probe is incorrect. In fact, SPL is not
even built when TFABOOT is selected.

Thus, only skip probe with TFABOOT, but not SPL_BUILD.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm/mach-stm32mp/bsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-stm32mp/bsec.c b/arch/arm/mach-stm32mp/bsec.c
index fe39bd80cf..a02d19c1b9 100644
--- a/arch/arm/mach-stm32mp/bsec.c
+++ b/arch/arm/mach-stm32mp/bsec.c
@@ -506,7 +506,7 @@ static int stm32mp_bsec_probe(struct udevice *dev)
 	 * only executed in U-Boot proper when TF-A is not used
 	 */
 
-	if (!IS_ENABLED(CONFIG_TFABOOT) && !IS_ENABLED(CONFIG_SPL_BUILD)) {
+	if (!IS_ENABLED(CONFIG_TFABOOT)) {
 		plat = dev_get_plat(dev);
 
 		for (otp = 57; otp <= BSEC_OTP_MAX_VALUE; otp++)
-- 
2.31.1

Re: [PATCH 05/10] arm: stm32mp: bsec: Do not skip .probe() for SPL

[Patrick DELAUNAY]

Hi

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:

> stm32mp_bsec_probe() was skipped for TFABOOT and SPL_BUILD. The idea
> of skipping probe() is that we can't access BSEC from the normal
> world. This is true with TFABOOT. However, in SPL, we are in the
> secure world, so skipping probe is incorrect. In fact, SPL is not
> even built when TFABOOT is selected.
>
> Thus, only skip probe with TFABOOT, but not SPL_BUILD.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   arch/arm/mach-stm32mp/bsec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-stm32mp/bsec.c b/arch/arm/mach-stm32mp/bsec.c
> index fe39bd80cf..a02d19c1b9 100644
> --- a/arch/arm/mach-stm32mp/bsec.c
> +++ b/arch/arm/mach-stm32mp/bsec.c
> @@ -506,7 +506,7 @@ static int stm32mp_bsec_probe(struct udevice *dev)
>   	 * only executed in U-Boot proper when TF-A is not used
>   	 */
>   
> -	if (!IS_ENABLED(CONFIG_TFABOOT) && !IS_ENABLED(CONFIG_SPL_BUILD)) {
> +	if (!IS_ENABLED(CONFIG_TFABOOT)) {
>   		plat = dev_get_plat(dev);
>   
>   		for (otp = 57; otp <= BSEC_OTP_MAX_VALUE; otp++)


in this part the dirver don't skip the BSEC probe (still return 0)

BUT the BSEC driver skip only the shadow of the OTP

=> this initialisation is necessary only one time to save time

a/ for TF-A boot it is done in TF-A

b/ for basic boot (SPL + U-Boot) is only done in U-Boot proper (because 
is not mandatory in SPL

and the U-Boot execution was faster - before the data cache activation 
in SPL)


With this patch the copy of the OTP in shadow memory is done 2 times

1/ in SPL (BSEC probe) = with CONFIG_SPL_BUILD

2/ in U-Boot (BSEC probe)

see comment:

     /*
      * update unlocked shadow for OTP cleared by the rom code
      * only executed in U-Boot proper when TF-A is not used
      */


but for falcon mode, as the Linux driver expected the OTP are shadowed,

the operation need to be done in SPL and in U-Boot proper

I propose


     /*

      * update unlocked shadow for OTP cleared by the rom code

-     * only executed in U-Boot proper when TF-A is not used

+     * only executed in U-Boot SPL, it is done in TF-A when used

      */

-	if (!IS_ENABLED(CONFIG_TFABOOT) && !IS_ENABLED(CONFIG_SPL_BUILD)) {
+	if (IS_ENABLED(CONFIG_SPL_BUILD)) {


Regards

Patrick

[PATCH 06/10] arm: stm32mp: Factor out reading MAC address from OTP

[Alexandru Gagniuc]

Move the reading the OTP into a separate function. This is
required for a subsequent change which sets the MAC in SPL.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm/mach-stm32mp/cpu.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index eb79f3ffd2..8727de513c 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -593,6 +593,28 @@ static void setup_boot_mode(void)
 	clrsetbits_le32(TAMP_BOOT_CONTEXT, TAMP_BOOT_FORCED_MASK, BOOT_NORMAL);
 }
 
+static int stm32_read_otp_mac(uint8_t enetaddr[ARP_HLEN])
+{
+	struct udevice *dev;
+	int ret, i;
+	u32 otp[2];
+
+	ret = uclass_get_device_by_driver(UCLASS_MISC,
+					  DM_DRIVER_GET(stm32mp_bsec),
+					  &dev);
+	if (ret)
+		return ret;
+
+	ret = misc_read(dev, STM32_BSEC_SHADOW(BSEC_OTP_MAC), otp, sizeof(otp));
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < ARP_HLEN; i++)
+		enetaddr[i] = ((uint8_t *)&otp)[i];
+
+	return 0;
+}
+
 /*
  * If there is no MAC address in the environment, then it will be initialized
  * (silently) from the value in the OTP.
@@ -601,29 +623,16 @@ __weak int setup_mac_address(void)
 {
 #if defined(CONFIG_NET)
 	int ret;
-	int i;
-	u32 otp[2];
 	uchar enetaddr[6];
-	struct udevice *dev;
 
 	/* MAC already in environment */
 	if (eth_env_get_enetaddr("ethaddr", enetaddr))
 		return 0;
 
-	ret = uclass_get_device_by_driver(UCLASS_MISC,
-					  DM_DRIVER_GET(stm32mp_bsec),
-					  &dev);
-	if (ret)
-		return ret;
-
-	ret = misc_read(dev, STM32_BSEC_SHADOW(BSEC_OTP_MAC),
-			otp, sizeof(otp));
+	ret = stm32_read_otp_mac(enetaddr);
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < 6; i++)
-		enetaddr[i] = ((uint8_t *)&otp)[i];
-
 	if (!is_valid_ethaddr(enetaddr)) {
 		log_err("invalid MAC address in OTP %pM\n", enetaddr);
 		return -EINVAL;
-- 
2.31.1

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

[Alexandru Gagniuc]

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.me@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();
 }
 
-- 
2.31.1

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

[Patrick DELAUNAY]

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.me@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";
};

PS: this part is not yet upstreamed in Linux

Linux reference = 
Documentation/devicetree/bindings/net/ethernet-controller.yaml


   nvmem-cells:
     maxItems: 1
     description:
       Reference to an nvmem node for the MAC address

   nvmem-cell-names:
     const: mac-address


Patrick

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.me@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.

[PATCH 08/10] lib: Makefile: Make optee library available in SPL

[Alexandru Gagniuc]

We want the optee_copy_fdt_nodes symbols in SPL. This is for cases
when booting an OPTEE payload directly.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 lib/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Makefile b/lib/Makefile
index 8ba745faa0..73dacbb01b 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -16,7 +16,6 @@ obj-$(CONFIG_FIT) += libfdt/
 obj-$(CONFIG_OF_LIVE) += of_live.o
 obj-$(CONFIG_CMD_DHRYSTONE) += dhry/
 obj-$(CONFIG_ARCH_AT91) += at91/
-obj-$(CONFIG_OPTEE) += optee/
 obj-$(CONFIG_ASN1_DECODER) += asn1_decoder.o
 obj-y += crypto/
 
@@ -74,6 +73,7 @@ obj-$(CONFIG_$(SPL_)GZIP) += gunzip.o
 obj-$(CONFIG_$(SPL_)LZO) += lzo/
 obj-$(CONFIG_$(SPL_)LZMA) += lzma/
 obj-$(CONFIG_$(SPL_)LZ4) += lz4_wrapper.o
+obj-$(CONFIG_OPTEE) += optee/
 
 obj-$(CONFIG_$(SPL_)LIB_RATIONAL) += rational.o
 
-- 
2.31.1

Re: [PATCH 08/10] lib: Makefile: Make optee library available in SPL

[Patrick DELAUNAY]

Hi

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
> We want the optee_copy_fdt_nodes symbols in SPL. This is for cases
> when booting an OPTEE payload directly.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   lib/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/Makefile b/lib/Makefile
> index 8ba745faa0..73dacbb01b 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -16,7 +16,6 @@ obj-$(CONFIG_FIT) += libfdt/
>   obj-$(CONFIG_OF_LIVE) += of_live.o
>   obj-$(CONFIG_CMD_DHRYSTONE) += dhry/
>   obj-$(CONFIG_ARCH_AT91) += at91/
> -obj-$(CONFIG_OPTEE) += optee/
>   obj-$(CONFIG_ASN1_DECODER) += asn1_decoder.o
>   obj-y += crypto/
>   
> @@ -74,6 +73,7 @@ obj-$(CONFIG_$(SPL_)GZIP) += gunzip.o
>   obj-$(CONFIG_$(SPL_)LZO) += lzo/
>   obj-$(CONFIG_$(SPL_)LZMA) += lzma/
>   obj-$(CONFIG_$(SPL_)LZ4) += lz4_wrapper.o
> +obj-$(CONFIG_OPTEE) += optee/
>   
>   obj-$(CONFIG_$(SPL_)LIB_RATIONAL) += rational.o
>   

I will propose in other thread to have a specific CONFIG to compile the the OP-TEE library

CONFIG_OPTEE => CONFIG_OPTEE_LIB

CONFIG_OPTEE will be kept only to manage the OP-TEE driver compilation
(depending TEE uclass / CONFIG_TEE)

but if you need to have the LIBRARY available in SPL
(to avoid to increase the SPL size of other board)

I think the configuraiton CONFIG_SPL_OPTEE_LIB should be added
then the Makefile becomes
  
+obj-$(CONFIG_$(SPL_)OPTEE) += optee/

Patrick

[PATCH 09/10] ARM: dts: stm32mp: Add OP-TEE "/firmware" node to SPL dtb

[Alexandru Gagniuc]

This node is required in SPL when booting an OP-TEE payload. Add it to
the SPL devicetree.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
index 0101962ea5..2e65b9b4d5 100644
--- a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
+++ b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
@@ -31,6 +31,7 @@
 		optee {
 			compatible = "linaro,optee-tz";
 			method = "smc";
+			u-boot,dm-spl;
 		};
 	};
 
-- 
2.31.1

Re: [PATCH 09/10] ARM: dts: stm32mp: Add OP-TEE "/firmware" node to SPL dtb

[Patrick DELAUNAY]

Hi,

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
> This node is required in SPL when booting an OP-TEE payload. Add it to
> the SPL devicetree.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> index 0101962ea5..2e65b9b4d5 100644
> --- a/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> +++ b/arch/arm/dts/stm32mp157a-dk1-u-boot.dtsi
> @@ -31,6 +31,7 @@
>   		optee {
>   			compatible = "linaro,optee-tz";
>   			method = "smc";
> +			u-boot,dm-spl;
>   		};
>   	};
>   


NAK = the OP-TEE nodes are ADDED by the OP-TEE firmware

when SPL is running the OP-TEE firmware is not running and the OP-TEE 
driver is not probed

=> this nodes are not required

This node should be present ONLY for backyard compatibility (STM32IMAGE 
support in U-Boot trusted boot)


regards


Patrick

[PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

[Alexandru Gagniuc]

OP-TEE does not take a devicetree for its own use. However, it does
pass the devicetree to the normal world OS. In most cases that will
be some other devicetree-bearing platform, such as linux.

As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
it is required to copy the optee nodes to he target's FDT. Do this as
part of spl_board_prepare_for_optee().

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm/mach-stm32mp/spl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
index d9fdc5926c..94fbb45cf9 100644
--- a/arch/arm/mach-stm32mp/spl.c
+++ b/arch/arm/mach-stm32mp/spl.c
@@ -19,6 +19,7 @@
 #include <asm/arch/sys_proto.h>
 #include <mach/tzc.h>
 #include <linux/libfdt.h>
+#include <tee/optee.h>
 
 u32 spl_boot_device(void)
 {
@@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
 void spl_board_prepare_for_optee(void *fdt)
 {
 	stm32_fdt_setup_mac_addr(fdt);
+	optee_copy_fdt_nodes(fdt);
 	stm32_init_tzc_for_optee();
 }
 
-- 
2.31.1

Re: [PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

[Patrick DELAUNAY]

Hi,

On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
> OP-TEE does not take a devicetree for its own use. However, it does
> pass the devicetree to the normal world OS. In most cases that will
> be some other devicetree-bearing platform, such as linux.
>
> As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
> it is required to copy the optee nodes to he target's FDT. Do this as
> part of spl_board_prepare_for_optee().
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>   arch/arm/mach-stm32mp/spl.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
> index d9fdc5926c..94fbb45cf9 100644
> --- a/arch/arm/mach-stm32mp/spl.c
> +++ b/arch/arm/mach-stm32mp/spl.c
> @@ -19,6 +19,7 @@
>   #include <asm/arch/sys_proto.h>
>   #include <mach/tzc.h>
>   #include <linux/libfdt.h>
> +#include <tee/optee.h>
>   
>   u32 spl_boot_device(void)
>   {
> @@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
>   void spl_board_prepare_for_optee(void *fdt)
>   {
>   	stm32_fdt_setup_mac_addr(fdt);
> +	optee_copy_fdt_nodes(fdt);
>   	stm32_init_tzc_for_optee();
>   }
>   


NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the unsecure 
device tree (when CFG_DT is activated)

before to jump to the kernel (that remove the need to have DT 
allignement with U-Boot SPL device tree)

=> SPL should not copy the OP-TEE nodes in next stage DT

reference in optee_os = core/arch/arm/kernel/boot.c: add_optee_dt_node()

add_optee_dt_node() <= update_external_dt() <= paged_init_primary()

It is the expected configuration for OP-TEE on STM32MP15 platform.

regards


Patrick

Re: [PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

[Alex G.]

Hi Patrick,

On 8/31/21 12:24 PM, Patrick DELAUNAY wrote:
> Hi,
> 
> On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
>> OP-TEE does not take a devicetree for its own use. However, it does
>> pass the devicetree to the normal world OS. In most cases that will
>> be some other devicetree-bearing platform, such as linux.
>>
>> As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
>> it is required to copy the optee nodes to he target's FDT. Do this as
>> part of spl_board_prepare_for_optee().
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   arch/arm/mach-stm32mp/spl.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
>> index d9fdc5926c..94fbb45cf9 100644
>> --- a/arch/arm/mach-stm32mp/spl.c
>> +++ b/arch/arm/mach-stm32mp/spl.c
>> @@ -19,6 +19,7 @@
>>   #include <asm/arch/sys_proto.h>
>>   #include <mach/tzc.h>
>>   #include <linux/libfdt.h>
>> +#include <tee/optee.h>
>>   u32 spl_boot_device(void)
>>   {
>> @@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
>>   void spl_board_prepare_for_optee(void *fdt)
>>   {
>>       stm32_fdt_setup_mac_addr(fdt);
>> +    optee_copy_fdt_nodes(fdt);
>>       stm32_init_tzc_for_optee();
>>   }
> 
> 
> NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the unsecure 
> device tree (when CFG_DT is activated)
> 
> before to jump to the kernel (that remove the need to have DT 
> allignement with U-Boot SPL device tree)
> 
> => SPL should not copy the OP-TEE nodes in next stage DT
> 
> reference in optee_os = core/arch/arm/kernel/boot.c: add_optee_dt_node()
> 
> add_optee_dt_node() <= update_external_dt() <= paged_init_primary()
> 
> It is the expected configuration for OP-TEE on STM32MP15 platform.

I agree that if a component modifies the platform, it should be 
responsible for updating the devicetree. Two issues though


1. Consistency

The STM32IMAGE boot path expects u-boot to add the optee nodes, while 
the FIP path expects OP-TEE to add the nodes. Which one do we stick to?


2. Memory access

I don't understand is how OP-TEE would get past the TZC.
If we look at optee_os => plat-0stm32mp1/plat_tzc400:
     "Early boot stage is in charge of configuring memory regions"
The following memory has been set up by SPL (or TF-A for that matter):

     Nonsec: c0000000->ddffffff
     Sec:    de000000->dfdfffff
     SHMEM:  dfe00000->dfffffff

The external DTB will be in the nonsec region, which OP-TEE allegedly 
can't access. So how would this get patched?

Alex

Re: [PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

[Alex G.]

Hi Patrick,

On 9/1/21 10:10 AM, Alex G. wrote:
> Hi Patrick,
> 
> On 8/31/21 12:24 PM, Patrick DELAUNAY wrote:
>> Hi,
>>
>> On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
>>> OP-TEE does not take a devicetree for its own use. However, it does
>>> pass the devicetree to the normal world OS. In most cases that will
>>> be some other devicetree-bearing platform, such as linux.
>>>
>>> As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
>>> it is required to copy the optee nodes to he target's FDT. Do this as
>>> part of spl_board_prepare_for_optee().
>>>
>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>> ---
>>>   arch/arm/mach-stm32mp/spl.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
>>> index d9fdc5926c..94fbb45cf9 100644
>>> --- a/arch/arm/mach-stm32mp/spl.c
>>> +++ b/arch/arm/mach-stm32mp/spl.c
>>> @@ -19,6 +19,7 @@
>>>   #include <asm/arch/sys_proto.h>
>>>   #include <mach/tzc.h>
>>>   #include <linux/libfdt.h>
>>> +#include <tee/optee.h>
>>>   u32 spl_boot_device(void)
>>>   {
>>> @@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
>>>   void spl_board_prepare_for_optee(void *fdt)
>>>   {
>>>       stm32_fdt_setup_mac_addr(fdt);
>>> +    optee_copy_fdt_nodes(fdt);
>>>       stm32_init_tzc_for_optee();
>>>   }
>>
>>
>> NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the unsecure 
>> device tree (when CFG_DT is activated)
>>
>> before to jump to the kernel (that remove the need to have DT 
>> allignement with U-Boot SPL device tree)
>>
>> => SPL should not copy the OP-TEE nodes in next stage DT
>>
>> reference in optee_os = core/arch/arm/kernel/boot.c: add_optee_dt_node()
>>
>> add_optee_dt_node() <= update_external_dt() <= paged_init_primary()
>>
>> It is the expected configuration for OP-TEE on STM32MP15 platform.
> 
> I agree that if a component modifies the platform, it should be 
> responsible for updating the devicetree. Two issues though
> 
> 
> 1. Consistency
> 
> The STM32IMAGE boot path expects u-boot to add the optee nodes, while 
> the FIP path expects OP-TEE to add the nodes. Which one do we stick to?
> 
> 
> 2. Memory access
> 
> I don't understand is how OP-TEE would get past the TZC.
> If we look at optee_os => plat-0stm32mp1/plat_tzc400:
>      "Early boot stage is in charge of configuring memory regions"
> The following memory has been set up by SPL (or TF-A for that matter):
> 
>      Nonsec: c0000000->ddffffff
>      Sec:    de000000->dfdfffff
>      SHMEM:  dfe00000->dfffffff
> 
> The external DTB will be in the nonsec region, which OP-TEE allegedly 
> can't access. So how would this get patched?

Let's set aside the above for a bit. I tried the following OP-TEE configs:


"CFG_DT=y CFG_DTB_MAX_SIZE=0x80000"

Crashes with 'unhandled pageable abort' somewhere in 
core/arch/arm/plat-stm32mp1/ . I don't have much more time to spend 
debugging this.


"CFG_DT=y CFG_DTB_MAX_SIZE=0x80000 PLATFORM_FLAVOR=157C_DK2"

This adds about 800 ms to the boot time. (power on to kernel printing 
the first line). I have an allowance of 1 second to get to the kernel. I 
cannot just add 800ms to the boot time and hope nobody notices. However, 
calling optee_copy_fdt_nodes() adds no measureable amount of time.

Unless OP-TEE can modify the devicetree without increasing boot time, I 
will not go down this path. This is an engineering decision driven by 
the product requirements.

That being said, I request that you reconsider your NAK based on this 
new data.

Alex

Re: [PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

[Patrick DELAUNAY]

Hi Alex

On 9/2/21 7:34 PM, Alex G. wrote:
> Hi Patrick,
>
> On 9/1/21 10:10 AM, Alex G. wrote:
>> Hi Patrick,
>>
>> On 8/31/21 12:24 PM, Patrick DELAUNAY wrote:
>>> Hi,
>>>
>>> On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
>>>> OP-TEE does not take a devicetree for its own use. However, it does
>>>> pass the devicetree to the normal world OS. In most cases that will
>>>> be some other devicetree-bearing platform, such as linux.
>>>>
>>>> As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
>>>> it is required to copy the optee nodes to he target's FDT. Do this as
>>>> part of spl_board_prepare_for_optee().
>>>>
>>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>>> ---
>>>>   arch/arm/mach-stm32mp/spl.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
>>>> index d9fdc5926c..94fbb45cf9 100644
>>>> --- a/arch/arm/mach-stm32mp/spl.c
>>>> +++ b/arch/arm/mach-stm32mp/spl.c
>>>> @@ -19,6 +19,7 @@
>>>>   #include <asm/arch/sys_proto.h>
>>>>   #include <mach/tzc.h>
>>>>   #include <linux/libfdt.h>
>>>> +#include <tee/optee.h>
>>>>   u32 spl_boot_device(void)
>>>>   {
>>>> @@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
>>>>   void spl_board_prepare_for_optee(void *fdt)
>>>>   {
>>>>       stm32_fdt_setup_mac_addr(fdt);
>>>> +    optee_copy_fdt_nodes(fdt);
>>>>       stm32_init_tzc_for_optee();
>>>>   }
>>>
>>>
>>> NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the 
>>> unsecure device tree (when CFG_DT is activated)
>>>
>>> before to jump to the kernel (that remove the need to have DT 
>>> allignement with U-Boot SPL device tree)
>>>
>>> => SPL should not copy the OP-TEE nodes in next stage DT
>>>
>>> reference in optee_os = core/arch/arm/kernel/boot.c: 
>>> add_optee_dt_node()
>>>
>>> add_optee_dt_node() <= update_external_dt() <= paged_init_primary()
>>>
>>> It is the expected configuration for OP-TEE on STM32MP15 platform.
>>
>> I agree that if a component modifies the platform, it should be 
>> responsible for updating the devicetree. Two issues though
>>
>>
>> 1. Consistency
>>
>> The STM32IMAGE boot path expects u-boot to add the optee nodes, while 
>> the FIP path expects OP-TEE to add the nodes. Which one do we stick to?
>>
>>

STM32MP platform expects U-Boot to copy the OP-TEE nodes because it is a 
generic U-Boot behavior when OP-TEE is supported

FIP path expects OP-TEE to add their nodes because it is a generic 
OP-TEE behavior

then the added nodes are copied by U-Boot in kernel DT


It is generic, flexible (OP-TEE configuration update wihout change in 
kernel DT or in U-Boot)

but not time optimized...


for STM32IMAGE image support, I have a issue (restriction)  = the U-Boot 
DT can't updated by

OP-TEE (U-Boot DT is embedded in u-boot.stm32 not available by OP-TEE)

=> I force presence of OP-TEE node in U-Boot DT for STM32IMAGE support

       to allow copy to kernel DT.


But it is less generic (dependency between U-Boot device tree and OP-TEE):

it is one reason we switch to FIP support


in you use case: SPL -> OP-TEE -> kernel


I think the kernel device tree can be updated by OP-TEE (to be generic) or

the kernel DT have already the correct OP-TEE nodes to reduce the DT 
update time penalty,

as you are sure of the OP-TEE support / configuration for you board.


drawback: the kernel device tree need to be aligned with the OP-TEE 
configuration.


>> 2. Memory access
>>
>> I don't understand is how OP-TEE would get past the TZC.
>> If we look at optee_os => plat-0stm32mp1/plat_tzc400:
>>      "Early boot stage is in charge of configuring memory regions"
>> The following memory has been set up by SPL (or TF-A for that matter):
>>
>>      Nonsec: c0000000->ddffffff
>>      Sec:    de000000->dfdfffff
>>      SHMEM:  dfe00000->dfffffff
>>
>> The external DTB will be in the nonsec region, which OP-TEE allegedly 
>> can't access. So how would this get patched?
>
> Let's set aside the above for a bit. I tried the following OP-TEE 
> configs:
>
>
> "CFG_DT=y CFG_DTB_MAX_SIZE=0x80000"
>
> Crashes with 'unhandled pageable abort' somewhere in 
> core/arch/arm/plat-stm32mp1/ . I don't have much more time to spend 
> debugging this.
>
>
> "CFG_DT=y CFG_DTB_MAX_SIZE=0x80000 PLATFORM_FLAVOR=157C_DK2"
>
> This adds about 800 ms to the boot time. (power on to kernel printing 
> the first line). I have an allowance of 1 second to get to the kernel. 
> I cannot just add 800ms to the boot time and hope nobody notices. 
> However, calling optee_copy_fdt_nodes() adds no measureable amount of 
> time.
>
> Unless OP-TEE can modify the devicetree without increasing boot time, 
> I will not go down this path. This is an engineering decision driven 
> by the product requirements.
>
> That being said, I request that you reconsider your NAK based on this 
> new data.


in my previous answer I think on generic support and you have specific 
boot time requirement.


but after cross-check with TF-A boot in FIP mode,

in TF-A set the correct firewall configuration at the end of BL2 (it is 
not done in OP-TEE as I expect)

based on configuration description found in FIP.


To be coherent with you configuration (SPL->OP-TEE->kernel),

this firewall configuration should be done by SPL before to start OP-TEE.


And to avoid to lost time in device tree parsing the DT modification or 
copy of node should be avoid:

the OP-TEE reserved should be already present in your kernel device tree 
=> no more need to copied them


I dig again with OPTEE features support in SPL and to avoid dependancy 
issue I pushed a serie:

"lib: optee: remove the duplicate CONFIG_OPTEE"

http://patchwork.ozlabs.org/project/uboot/list/?series=260701&state=*


=> in your use case and for reduce the boot time

- activate CONFIG_SPL_OPTEE_IMAGE : load OP-TEE image by SPL

- manage OPTEE / firewall configuration with your defconfig

   (=> no more need of OP-TEE nodes in SPL DT / reduced SPL size and 
parsing time)

   by using the existing configurations in lib/optee/Kconfig instead DT 
information:

    => CONFIG_OPTEE_LOAD_ADDR

    => CONFIG_OPTEE_TZDRAM_SIZE

    => CONFIG_OPTEE_TZDRAM_BASE

    PS: need to add a configuration for OPTEE SHMEM_SIZE ?


- manually add OP-TEE nodes / reserved memory in the kernel (and U-Boot)

   device tree and OP-TEE config CFG_DT is not activated 
(update_external_dt not used)

=> OP-TEE don't modify the devicetre (as nodes are already present in NS DT)

=> in spl_board_prepare_for_optee, avoid to update device tree to lost time


in all the case : optee_copy_fdt_nodes in not done by SPL but by U-Boot 
started

by OP-TEE (for dynamic support)  or not done (no static configuration)


The only reason to copy the OP-TEE node for FIP support or for your use 
case is

to copy the nodes dynamically added by OP-TEE.


SPL / TFA -> OP-TEE -> U-Boot -> Kernel

                         (1)           (2)              (3)


OP-TEE only the U-Boot DT (non secure DT) in boot parameter

(1) => OP-TEE add the node in U-Boot DT

(2) => U-Boot load the kernel DT / copied the OP-TEE in kernel DT

(3) => kernel use the DT with correct OP-TEE node


The OP-TEE node copy is not necessary if the kernel start with the U-Boot DT

this sequence allow to have dynamic support of OP-TEE configuration.


But for product with fixed configuration it is not required.


I hope it is more clear.


one question:

why spl_fixup_fdt isn't called (for generic fixup = arch_fixup_fdt) for 
OP-TEE boot

before to call spl_board_prepare_for_optee ?


>
> Alex
>
>

Patrick

Re: [PATCH 10/10] stm32mp1: spl: Copy optee nodes to target FDT for OP-TEE payloads

[Etienne Carriere]

Hello,

On Wed, 1 Sept 2021 at 17:10, Alex G. <mr.nuke.me@gmail.com> wrote:

> Hi Patrick,
>
> On 8/31/21 12:24 PM, Patrick DELAUNAY wrote:
> > Hi,
> >
> > On 8/26/21 11:42 PM, Alexandru Gagniuc wrote:
> >> OP-TEE does not take a devicetree for its own use. However, it does
> >> pass the devicetree to the normal world OS. In most cases that will
> >> be some other devicetree-bearing platform, such as linux.
> >>
> >> As in other cases where there's an OPTEE payload (e.g. BOOTM_OPTEE),
> >> it is required to copy the optee nodes to he target's FDT. Do this as
> >> part of spl_board_prepare_for_optee().
> >>
> >> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> >> ---
> >>   arch/arm/mach-stm32mp/spl.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
> >> index d9fdc5926c..94fbb45cf9 100644
> >> --- a/arch/arm/mach-stm32mp/spl.c
> >> +++ b/arch/arm/mach-stm32mp/spl.c
> >> @@ -19,6 +19,7 @@
> >>   #include <asm/arch/sys_proto.h>
> >>   #include <mach/tzc.h>
> >>   #include <linux/libfdt.h>
> >> +#include <tee/optee.h>
> >>   u32 spl_boot_device(void)
> >>   {
> >> @@ -182,6 +183,7 @@ void stm32_init_tzc_for_optee(void)
> >>   void spl_board_prepare_for_optee(void *fdt)
> >>   {
> >>       stm32_fdt_setup_mac_addr(fdt);
> >> +    optee_copy_fdt_nodes(fdt);
> >>       stm32_init_tzc_for_optee();
> >>   }
> >
> >
> > NAK the OP-TEE nodes are ADDED by the OP-TEE firmware in the unsecure
> > device tree (when CFG_DT is activated)
> >
> > before to jump to the kernel (that remove the need to have DT
> > allignement with U-Boot SPL device tree)
> >
> > => SPL should not copy the OP-TEE nodes in next stage DT
> >
> > reference in optee_os = core/arch/arm/kernel/boot.c: add_optee_dt_node()
> >
> > add_optee_dt_node() <= update_external_dt() <= paged_init_primary()
> >
> > It is the expected configuration for OP-TEE on STM32MP15 platform.
>
> I agree that if a component modifies the platform, it should be
> responsible for updating the devicetree. Two issues though
>
>
> 1. Consistency
>
> The STM32IMAGE boot path expects u-boot to add the optee nodes, while
> the FIP path expects OP-TEE to add the nodes. Which one do we stick to?
>

U-boot already handles this, no? optee_copy_fdt_nodes() already skips
addition of optee nodes when it's already in.
So if OP-TEE has already filled the DT, u-boot won't add anything, which
looks good (to me).



>
>
> 2. Memory access
>
> I don't understand is how OP-TEE would get past the TZC.
> If we look at optee_os => plat-0stm32mp1/plat_tzc400:
>      "Early boot stage is in charge of configuring memory regions"
> The following memory has been set up by SPL (or TF-A for that matter):
>
>      Nonsec: c0000000->ddffffff
>      Sec:    de000000->dfdfffff
>      SHMEM:  dfe00000->dfffffff
>
> The external DTB will be in the nonsec region, which OP-TEE allegedly
> can't access. So how would this get patched?
>

OP-TEE secure world can access non-secure memory, provided it maps it as
non-secure memory.

regards,
etienne


> Alex
>