mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE
@ 2025-05-26 14:38 Michael Tretter
  2025-05-26 14:38 ` [PATCH 1/9] ARM: rockchip: fix formatting Michael Tretter
                   ` (8 more replies)
  0 siblings, 9 replies; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

On rk3588, OP-TEE uses the device tree to detect the available SDRAM
that should be used for dynamic shared memory. Since the amount of SDRAM
is detected during boot, the rk3588 device tree doesn't contain memory
nodes. Therefore, barebox has to detect the memory and add memory nodes
the device tree in the PBL before passing the device tree to the TF-A,
which passes it on to OP-TEE.

Adding nodes to the fdt increases the size of the fdt. As OP-TEE also
modifies and extends the passed device tree, the fdt grows even further
in OP-TEE. Therefore, there must be some extra reserved space in the
fdt. OP-TEE has CFG_DTB_MAX_SIZE as limit, which is the minimum amount
of space that must be reserved by barebox. The PBL_FDT_MIN_SIZE option
allows to configure this size as minimum.

I thought about adding disabled memory nodes to the dts and
enabling/fixing the existing nodes instead of adding new nodes. Finding
and fixing the correct existing node turned out to be more complex than
adding a new node. As we have to reserve some space in the fdt for
OP-TEE anyway, adding new nodes looks like the better solution.

The downstream TF-A may crash if barebox passes a device tree to it.
barebox cannot detect, if the loaded TF-A is downstream or upstream.
Therefore, add a config item to enable passing the device tree.

Patches 1 and 2 are some cleanup as preparation for actual changes.

Patches 3 to 6 extend the libfdt integration with helpers to modify and
add memory dt nodes.

Patches 7 to 9 add the rockchip specific code to read the memory size
from the DRAM controller, add memory dt nodes to the fdt, and pass the
fdt to the TF-A and OP-TEE.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
Michael Tretter (9):
      ARM: rockchip: fix formatting
      ARM: rockchip: dmc: use RK3588_INT_REG_START for rk3588
      lib: fdt: add fdt_addresses
      PBL: fdt: refactor helper for reading nr of cells
      PBL: fdt: make minimum fdt size configurable
      PBL: fdt: add fdt_fixup_mem to fixup memory nodes
      ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size
      ARM: rockchip: pass device tree to TF-A
      ARM: rockchip: fixup memory in device tree for TF-A

 arch/arm/mach-rockchip/Kconfig | 13 +++++++
 arch/arm/mach-rockchip/atf.c   | 62 ++++++++++++++++++------------
 arch/arm/mach-rockchip/dmc.c   | 37 ++++++++++++++++--
 include/mach/rockchip/dmc.h    |  2 +
 include/pbl.h                  |  1 +
 lib/Makefile                   |  2 +-
 lib/fdt_addresses.c            |  4 ++
 pbl/Kconfig                    | 12 ++++++
 pbl/fdt.c                      | 86 +++++++++++++++++++++++++++++++++++++-----
 scripts/Makefile.lib           |  2 +
 10 files changed, 184 insertions(+), 37 deletions(-)
---
base-commit: fb35d7a2fe800260ca1ce2e189731e48ccefcf7b
change-id: 20250515-rk3588-optee-08cc33320963

Best regards,
-- 
Michael Tretter <m.tretter@pengutronix.de>




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/9] ARM: rockchip: fix formatting
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-26 17:30   ` Marco Felsch
  2025-05-26 14:38 ` [PATCH 2/9] ARM: rockchip: dmc: use RK3588_INT_REG_START for rk3588 Michael Tretter
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

Replace spaces with tabs and remove a double newline. No semantic
changes.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 arch/arm/mach-rockchip/atf.c | 47 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
index d3a40e3c1b19db51b9bf620648176c54493c3afb..cfa6df043b34c0f36919048237c7ecf33dfe0724 100644
--- a/arch/arm/mach-rockchip/atf.c
+++ b/arch/arm/mach-rockchip/atf.c
@@ -101,7 +101,6 @@ static uintptr_t rk_load_optee(uintptr_t bl32, const void *bl32_image,
 
 	rk_scratch_save_optee_hdr(hdr);
 
-
 	memcpy((void *)bl32, bl32_image, bl32_size);
 
 	return bl32;
@@ -176,32 +175,32 @@ void rk3588_atf_load_bl31(void *fdt)
 
 void __noreturn rk3588_barebox_entry(void *fdt)
 {
-       unsigned long membase, endmem;
+	unsigned long membase, endmem;
 
-       membase = RK3588_DRAM_BOTTOM;
-       endmem = rk3588_ram0_size();
+	membase = RK3588_DRAM_BOTTOM;
+	endmem = rk3588_ram0_size();
 
-       rk_scratch = (void *)arm_mem_scratch(endmem);
+	rk_scratch = (void *)arm_mem_scratch(endmem);
 
-       if (current_el() == 3) {
-               rk3588_lowlevel_init();
-               rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
+	if (current_el() == 3) {
+		rk3588_lowlevel_init();
+		rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
 
-               /*
-                * The downstream TF-A doesn't cope with our device tree when
-                * CONFIG_OF_OVERLAY_LIVE is enabled, supposedly because it is
-                * too big for some reason. Otherwise it doesn't have any visible
-                * effect if we pass a device tree or not, except that the TF-A
-                * fills in the ethernet MAC address into the device tree.
-                * The upstream TF-A doesn't use the device tree at all.
-                *
-                * Pass NULL for now until we have a good reason to pass a real
-                * device tree.
-                */
-               rk3588_atf_load_bl31(NULL);
-               /* not reached when CONFIG_ARCH_ROCKCHIP_ATF */
-       }
+		/*
+		 * The downstream TF-A doesn't cope with our device tree when
+		 * CONFIG_OF_OVERLAY_LIVE is enabled, supposedly because it is
+		 * too big for some reason. Otherwise it doesn't have any visible
+		 * effect if we pass a device tree or not, except that the TF-A
+		 * fills in the ethernet MAC address into the device tree.
+		 * The upstream TF-A doesn't use the device tree at all.
+		 *
+		 * Pass NULL for now until we have a good reason to pass a real
+		 * device tree.
+		 */
+		rk3588_atf_load_bl31(NULL);
+		/* not reached when CONFIG_ARCH_ROCKCHIP_ATF */
+	}
 
-       optee_set_membase(rk_scratch_get_optee_hdr());
-       barebox_arm_entry(membase, endmem - membase, fdt);
+	optee_set_membase(rk_scratch_get_optee_hdr());
+	barebox_arm_entry(membase, endmem - membase, fdt);
 }

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 2/9] ARM: rockchip: dmc: use RK3588_INT_REG_START for rk3588
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
  2025-05-26 14:38 ` [PATCH 1/9] ARM: rockchip: fix formatting Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-26 17:29   ` Marco Felsch
  2025-05-26 14:38 ` [PATCH 3/9] lib: fdt: add fdt_addresses Michael Tretter
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

While RK3588_INT_REG_START and RK3568_INT_REG_START are the same value,
the implementation for rk3588 should use RK3588_INT_REG_START to avoid
confusion.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 arch/arm/mach-rockchip/dmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-rockchip/dmc.c b/arch/arm/mach-rockchip/dmc.c
index cf63fdb5a8bb6ea5a7474fcaa1590d4a7428ceab..62a7ef8f1e38e989e84f08f6f4a6586be3e85532 100644
--- a/arch/arm/mach-rockchip/dmc.c
+++ b/arch/arm/mach-rockchip/dmc.c
@@ -187,7 +187,7 @@ resource_size_t rk3588_ram0_size(void)
 
 	pr_info("%s() size1 = 0x%08llx, size2 = 0x%08llx\n", __func__, (u64)size1, (u64)size2);
 
-	size = min_t(resource_size_t, RK3568_INT_REG_START, size1 + size2);
+	size = min_t(resource_size_t, RK3588_INT_REG_START, size1 + size2);
 
 	return size;
 }

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 3/9] lib: fdt: add fdt_addresses
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
  2025-05-26 14:38 ` [PATCH 1/9] ARM: rockchip: fix formatting Michael Tretter
  2025-05-26 14:38 ` [PATCH 2/9] ARM: rockchip: dmc: use RK3588_INT_REG_START for rk3588 Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-26 17:29   ` Marco Felsch
  2025-05-26 14:38 ` [PATCH 4/9] PBL: fdt: refactor helper for reading nr of cells Michael Tretter
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

fdt_addresses contains some helpers for handling addresses and sizes.
Add it to the local libfdt.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 lib/Makefile        | 2 +-
 lib/fdt_addresses.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/Makefile b/lib/Makefile
index 4e75f2467ac1c05edb1489a997e0767f9e9972ba..0d1d22c0845b61962a36bc7549722570f7475685 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -100,7 +100,7 @@ KASAN_SANITIZE_ubsan.o := n
 CFLAGS_ubsan.o := -fno-stack-protector
 
 libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o \
-	                      fdt_empty_tree.o
+	                      fdt_empty_tree.o fdt_addresses.o
 $(foreach file, $(libfdt_files), \
 	$(eval CFLAGS_$(file) = -I $(srctree)/scripts/dtc/libfdt) \
 	$(eval CFLAGS_$(file:%.o=%.pbl.o) = -I $(srctree)/scripts/dtc/libfdt))
diff --git a/lib/fdt_addresses.c b/lib/fdt_addresses.c
new file mode 100644
index 0000000000000000000000000000000000000000..4bde1bb186559fea9aa6b842de8b96e9bc9bcc4a
--- /dev/null
+++ b/lib/fdt_addresses.c
@@ -0,0 +1,4 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt_addresses.c"

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 4/9] PBL: fdt: refactor helper for reading nr of cells
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
                   ` (2 preceding siblings ...)
  2025-05-26 14:38 ` [PATCH 3/9] lib: fdt: add fdt_addresses Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-26 17:30   ` Marco Felsch
  2025-05-26 14:38 ` [PATCH 5/9] PBL: fdt: make minimum fdt size configurable Michael Tretter
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

Since libfdt now contains fdt_addresses, the fdt_find_mem function can
use the already existing helpers to read #size-cells and #address-cells.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 pbl/fdt.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/pbl/fdt.c b/pbl/fdt.c
index 8e4d1295074a6b8dc4ad07ad2ada5882148c9ce6..3b1783152a8cb81f3eda1187b2f7bf998c4addf4 100644
--- a/pbl/fdt.c
+++ b/pbl/fdt.c
@@ -17,8 +17,8 @@ static const __be32 *fdt_parse_reg(const __be32 *reg, uint32_t n,
 
 void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsize)
 {
-	const __be32 *nap, *nsp, *reg;
-	uint32_t na, ns;
+	const __be32 *reg;
+	int na, ns;
 	uint64_t memsize64, membase64;
 	int node, size;
 
@@ -28,26 +28,23 @@ void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsiz
 		goto err;
 	}
 
-	/* Find the #address-cells and #size-cells properties */
 	node = fdt_path_offset(fdt, "/");
 	if (node < 0) {
 		pr_err("Cannot find root node\n");
 		goto err;
 	}
 
-	nap = fdt_getprop(fdt, node, "#address-cells", &size);
-	if (!nap || (size != 4)) {
+	na = fdt_address_cells(fdt, node);
+	if (na < 0) {
 		pr_err("Cannot find #address-cells property");
 		goto err;
 	}
-	na = fdt32_to_cpu(*nap);
 
-	nsp = fdt_getprop(fdt, node, "#size-cells", &size);
-	if (!nsp || (size != 4)) {
+	ns = fdt_size_cells(fdt, node);
+	if (ns < 0) {
 		pr_err("Cannot find #size-cells property");
 		goto err;
 	}
-	ns = fdt32_to_cpu(*nsp);
 
 	/* Find the memory range */
 	node = fdt_node_offset_by_prop_value(fdt, -1, "device_type",

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 5/9] PBL: fdt: make minimum fdt size configurable
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
                   ` (3 preceding siblings ...)
  2025-05-26 14:38 ` [PATCH 4/9] PBL: fdt: refactor helper for reading nr of cells Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-27  6:41   ` Sascha Hauer
  2025-05-26 14:38 ` [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes Michael Tretter
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

Adding or modifying nodes in the fdt may change the size of the fdt.
This needs some reserved space in the fdt to avoid overriding memory
that comes after the fdt and is already used.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 pbl/Kconfig          | 11 +++++++++++
 scripts/Makefile.lib |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/pbl/Kconfig b/pbl/Kconfig
index 6e3581829d589c7b06ed878b09bf74e16a0c3086..489b2001a855d62e11a2159311332b0e67f3a754 100644
--- a/pbl/Kconfig
+++ b/pbl/Kconfig
@@ -60,6 +60,17 @@ config PBL_VERIFY_PIGGY
 config PBL_CLOCKSOURCE
 	bool
 
+config PBL_FDT_MIN_SIZE
+	hex
+	default 0x0
+	prompt "Minimum size of the FDT blob"
+	help
+	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This
+	  may increases the size of the device tree. This may override the
+	  barebox binary.
+
+	  The minimum size should be at least CFG_DTB_MAX_SIZE for OP-TEE.
+
 config BOARD_GENERIC_DT
 	bool
 	select LIBFDT
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index b10119797686ea31fe927d29a7849ad525c8c835..f50006f57200a76a86c7e29175dd7e35ab138e26 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -396,6 +396,8 @@ ifeq ($(CONFIG_OF_OVERLAY_LIVE), y)
 DTC_FLAGS.dtb += -@
 endif
 
+DTC_FLAGS.dtb += --space $(CONFIG_PBL_FDT_MIN_SIZE)
+
 DTC_FLAGS.dtbo += -Wno-avoid_default_addr_size -Wno-reg_format
 
 # Generate an assembly file to wrap the output of the device tree compiler

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
                   ` (4 preceding siblings ...)
  2025-05-26 14:38 ` [PATCH 5/9] PBL: fdt: make minimum fdt size configurable Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-27  6:22   ` Sascha Hauer
  2025-05-26 14:38 ` [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size Michael Tretter
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

Board code in the PBL may use fdt_fixup_mem() to write the base
addresses and sizes of detected SDRAM to the fdt before passing the fdt
to other software like the TF-A and OP-TEE.

This is implemented on the fdt to avoid that the PBL needs to unpack the
device tree, use the of function for manipulation and repack the device
tree.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 include/pbl.h |  1 +
 pbl/fdt.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/include/pbl.h b/include/pbl.h
index abac3458593af2cec972a54ce9fb69e344179670..b330010562c4aba5fccbb4c421bb95291fa0bea1 100644
--- a/include/pbl.h
+++ b/include/pbl.h
@@ -17,6 +17,7 @@ extern unsigned long free_mem_end_ptr;
 void pbl_barebox_uncompress(void *dest, void *compressed_start, unsigned int len);
 
 void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsize);
+int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num);
 
 struct fdt_device_id {
 	const char *compatible;
diff --git a/pbl/fdt.c b/pbl/fdt.c
index 3b1783152a8cb81f3eda1187b2f7bf998c4addf4..40564952acfcd88bef050966720e76b7378bc720 100644
--- a/pbl/fdt.c
+++ b/pbl/fdt.c
@@ -2,6 +2,7 @@
 #include <linux/libfdt.h>
 #include <pbl.h>
 #include <linux/printk.h>
+#include <stdio.h>
 
 static const __be32 *fdt_parse_reg(const __be32 *reg, uint32_t n,
 				   uint64_t *val)
@@ -73,6 +74,76 @@ void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsiz
 	while (1);
 }
 
+int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num)
+{
+	int na, ns;
+	int node, root;
+	int err;
+	int i;
+
+	err = fdt_check_header(fdt);
+	if (err != 0) {
+		pr_err("Invalid device tree blob\n");
+		return err;
+	}
+
+	root = fdt_path_offset(fdt, "/");
+	if (root < 0) {
+		pr_err("Cannot find root node: %s\n", fdt_strerror(root));
+		return root;
+	}
+
+	na = fdt_address_cells(fdt, root);
+	if (na < 0) {
+		pr_err("Cannot find #address-cells property: %s\n",
+		       fdt_strerror(na));
+		return na;
+	}
+
+	ns = fdt_size_cells(fdt, root);
+	if (ns < 0) {
+		pr_err("Cannot find #size-cells property: %s\n",
+				fdt_strerror(ns));
+		return ns;
+	}
+
+	for (i = 0; i < num; i++) {
+		char name[32];
+
+		if (membase[i] == 0 || memsize[i] == 0)
+			continue;
+
+		snprintf(name, sizeof(name), "memory@%lx", membase[i]);
+		node = fdt_add_subnode(fdt, root, name);
+		if (node < 0) {
+			pr_err("Cannot to add node %s: %s\n",
+			       name, fdt_strerror(node));
+			err = node;
+			break;
+		}
+
+		pr_debug("Add memory node %s (0x%lx, size 0x%lx)\n",
+			 name, membase[i], memsize[i]);
+
+		err = fdt_setprop(fdt, node,
+				  "device_type", "memory", sizeof("memory"));
+		if (err < 0) {
+			pr_err("%s: Cannot set device_type: %s\n",
+			       name, fdt_strerror(err));
+			return err;
+		}
+
+		err = fdt_appendprop_addrrange(fdt, root, node, "reg", membase[i], memsize[i]);
+		if (err < 0) {
+			pr_err("Failed to fixup memory %lx %lx: %s\n",
+			       membase[i], memsize[i], fdt_strerror(err));
+			continue;
+		}
+	}
+
+	return err;
+}
+
 const void *fdt_device_get_match_data(const void *fdt, const char *nodepath,
 				      const struct fdt_device_id ids[])
 {

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
                   ` (5 preceding siblings ...)
  2025-05-26 14:38 ` [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-26 16:33   ` Marco Felsch
  2025-05-26 14:38 ` [PATCH 8/9] ARM: rockchip: pass device tree to TF-A Michael Tretter
  2025-05-26 14:38 ` [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A Michael Tretter
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

The PBL has to pass a full description of the SDRAM to OP-TEE to allow
OP-TEE to handle dynamic shared memory in the entire SDRAM. Thus, the
PBL needs to read the full memory configuration.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 arch/arm/mach-rockchip/dmc.c | 37 ++++++++++++++++++++++++++++++++++---
 include/mach/rockchip/dmc.h  |  2 ++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-rockchip/dmc.c b/arch/arm/mach-rockchip/dmc.c
index 62a7ef8f1e38e989e84f08f6f4a6586be3e85532..260b8be9c7da9e8f83323822eab92a439ef4e2ef 100644
--- a/arch/arm/mach-rockchip/dmc.c
+++ b/arch/arm/mach-rockchip/dmc.c
@@ -171,11 +171,12 @@ resource_size_t rk3568_ram0_size(void)
 #define RK3588_PMUGRF_OS_REG4           0x210
 #define RK3588_PMUGRF_OS_REG5           0x214
 
-resource_size_t rk3588_ram0_size(void)
+size_t rk3588_ram_sizes(phys_addr_t *base, resource_size_t *size, size_t n)
 {
 	void __iomem *pmugrf = IOMEM(RK3588_PMUGRF_BASE);
 	u32 sys_reg2, sys_reg3, sys_reg4, sys_reg5;
-	resource_size_t size, size1, size2;
+	resource_size_t memsize, size1, size2;
+	size_t i = 0;
 
 	sys_reg2 = readl(pmugrf + RK3588_PMUGRF_OS_REG2);
 	sys_reg3 = readl(pmugrf + RK3588_PMUGRF_OS_REG3);
@@ -187,7 +188,37 @@ resource_size_t rk3588_ram0_size(void)
 
 	pr_info("%s() size1 = 0x%08llx, size2 = 0x%08llx\n", __func__, (u64)size1, (u64)size2);
 
-	size = min_t(resource_size_t, RK3588_INT_REG_START, size1 + size2);
+	memsize = size1 + size2;
+
+	base[i] = 0xa00000;
+	size[i] = min_t(resource_size_t, RK3588_INT_REG_START, memsize) - 0xa00000;
+	i++;
+
+	if (i < n && memsize > SZ_4G) {
+		base[i] = SZ_4G;
+		size[i] = min_t(unsigned long, DRAM_GAP1_START, memsize) - SZ_4G;
+		i++;
+	}
+	if (i < n && memsize > DRAM_GAP1_END) {
+		base[i] = DRAM_GAP1_END;
+		size[i] = min_t(unsigned long, DRAM_GAP2_START, memsize) - DRAM_GAP1_END;
+		i++;
+	}
+	if (i < n && memsize > DRAM_GAP2_END) {
+		base[i] = DRAM_GAP2_END;
+		size[i] = memsize - DRAM_GAP2_END;
+		i++;
+	}
+
+	return i;
+}
+
+resource_size_t rk3588_ram0_size(void)
+{
+	phys_addr_t base;
+	resource_size_t size;
+
+	rk3588_ram_sizes(&base, &size, 1);
 
 	return size;
 }
diff --git a/include/mach/rockchip/dmc.h b/include/mach/rockchip/dmc.h
index 3df9aa5e9c87857e9ac3994e0e40c60d8c91b0a4..bb5a5afd02417920446c729b96eeb90f1fa0ead0 100644
--- a/include/mach/rockchip/dmc.h
+++ b/include/mach/rockchip/dmc.h
@@ -87,4 +87,6 @@ resource_size_t rk3399_ram0_size(void);
 resource_size_t rk3568_ram0_size(void);
 resource_size_t rk3588_ram0_size(void);
 
+size_t rk3588_ram_sizes(phys_addr_t *base, resource_size_t *size, size_t n);
+
 #endif

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 8/9] ARM: rockchip: pass device tree to TF-A
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
                   ` (6 preceding siblings ...)
  2025-05-26 14:38 ` [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-26 16:37   ` Marco Felsch
  2025-05-26 14:38 ` [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A Michael Tretter
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

The upstream OP-TEE expects a device tree to be able to initialize the
dynamic shared memory. Therefore, barebox should pass a device tree that
contains memory nodes through the TF-A to OP-TEE.

Unfortunately, the downstream TF-A fails to start if barebox passes its
device tree and it is not possible to detect if the loaded TF-A is able
to handle the device tree. Add a config option to pass the device tree
if it is possible.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 arch/arm/mach-rockchip/Kconfig | 13 +++++++++++++
 arch/arm/mach-rockchip/atf.c   | 15 ++++-----------
 pbl/Kconfig                    |  1 +
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index 98dfd11c182b9fee6e3c958653ad4fa8a7d98d84..257c13bcf4846e805a7803105c71edeff92c534d 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -141,6 +141,19 @@ config ARCH_ROCKCHIP_ATF
 	  useful for debugging early startup, but for all other cases,
 	  say y here.
 
+config ARCH_ROCKCHIP_ATF_PASS_FDT
+	bool "Pass device tree to TF-A"
+	depends on ARCH_ROCKCHIP_ATF
+	help
+	  Enable this option if you are using an upstream OP-TEE that uses the
+	  device tree to initialize dynamic shared memory, which is passed
+	  through the upstream TF-A.
+
+	  Disable the option if you are using a downstream TF-A since it
+	  doesn't always cope with device trees. Supposedly this happens if the
+	  device tree is too large, for example if CONFIG_OF_OVERLAY_LIVE is
+	  enabled.
+
 config ARCH_ROCKCHIP_OPTEE
 	bool "Build rockchip OP-TEE binary into barebox"
 	depends on ARCH_ROCKCHIP_ATF
diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
index cfa6df043b34c0f36919048237c7ecf33dfe0724..12cf13717b6972c2eafc5a044ae8d0b4de029c32 100644
--- a/arch/arm/mach-rockchip/atf.c
+++ b/arch/arm/mach-rockchip/atf.c
@@ -186,18 +186,11 @@ void __noreturn rk3588_barebox_entry(void *fdt)
 		rk3588_lowlevel_init();
 		rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
 
-		/*
-		 * The downstream TF-A doesn't cope with our device tree when
-		 * CONFIG_OF_OVERLAY_LIVE is enabled, supposedly because it is
-		 * too big for some reason. Otherwise it doesn't have any visible
-		 * effect if we pass a device tree or not, except that the TF-A
-		 * fills in the ethernet MAC address into the device tree.
-		 * The upstream TF-A doesn't use the device tree at all.
-		 *
-		 * Pass NULL for now until we have a good reason to pass a real
-		 * device tree.
-		 */
+#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
+		rk3588_atf_load_bl31(fdt);
+#else
 		rk3588_atf_load_bl31(NULL);
+#endif
 		/* not reached when CONFIG_ARCH_ROCKCHIP_ATF */
 	}
 
diff --git a/pbl/Kconfig b/pbl/Kconfig
index 489b2001a855d62e11a2159311332b0e67f3a754..3dfc7de3e80da12a6313e84175275070274b9a5d 100644
--- a/pbl/Kconfig
+++ b/pbl/Kconfig
@@ -63,6 +63,7 @@ config PBL_CLOCKSOURCE
 config PBL_FDT_MIN_SIZE
 	hex
 	default 0x0
+	default 0x60000 if ARCH_ROCKCHIP_ATF_PASS_FDT
 	prompt "Minimum size of the FDT blob"
 	help
 	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A
  2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
                   ` (7 preceding siblings ...)
  2025-05-26 14:38 ` [PATCH 8/9] ARM: rockchip: pass device tree to TF-A Michael Tretter
@ 2025-05-26 14:38 ` Michael Tretter
  2025-05-26 17:25   ` Marco Felsch
  8 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-26 14:38 UTC (permalink / raw)
  To: BAREBOX; +Cc: Michael Tretter

Add the memory nodes for the detected SDRAM configuration to the fdt
before passing it to the TF-A.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 arch/arm/mach-rockchip/atf.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
index 12cf13717b6972c2eafc5a044ae8d0b4de029c32..342af302aa25089acce3c91df0ea38cbe71e0add 100644
--- a/arch/arm/mach-rockchip/atf.c
+++ b/arch/arm/mach-rockchip/atf.c
@@ -173,6 +173,26 @@ void rk3588_atf_load_bl31(void *fdt)
 	rockchip_atf_load_bl31(RK3588, rk3588_bl31_bin, rk3588_bl32_bin, fdt);
 }
 
+#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
+static int rk3588_fixup_mem(void *fdt)
+{
+	/* Use 4 blocks since rk3588 has 3 gaps in the address space */
+	unsigned long base[4];
+	unsigned long size[ARRAY_SIZE(base)];
+	phys_addr_t base_tmp[ARRAY_SIZE(base)];
+	resource_size_t size_tmp[ARRAY_SIZE(base_tmp)];
+	int i, n;
+
+	n = rk3588_ram_sizes(base_tmp, size_tmp, ARRAY_SIZE(base_tmp));
+	for (i = 0; i < n; i++) {
+		base[i] = base_tmp[i];
+		size[i] = size_tmp[i];
+	}
+
+	return fdt_fixup_mem(fdt, base, size, i);
+}
+#endif
+
 void __noreturn rk3588_barebox_entry(void *fdt)
 {
 	unsigned long membase, endmem;
@@ -187,6 +207,8 @@ void __noreturn rk3588_barebox_entry(void *fdt)
 		rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
 
 #ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
+		if (rk3588_fixup_mem(fdt) != 0)
+			pr_warn("Failed to fixup memory\n");
 		rk3588_atf_load_bl31(fdt);
 #else
 		rk3588_atf_load_bl31(NULL);

-- 
2.39.5




^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size
  2025-05-26 14:38 ` [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size Michael Tretter
@ 2025-05-26 16:33   ` Marco Felsch
  2025-05-27  6:25     ` Sascha Hauer
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2025-05-26 16:33 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-26, Michael Tretter wrote:
> The PBL has to pass a full description of the SDRAM to OP-TEE to allow
> OP-TEE to handle dynamic shared memory in the entire SDRAM. Thus, the
> PBL needs to read the full memory configuration.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  arch/arm/mach-rockchip/dmc.c | 37 ++++++++++++++++++++++++++++++++++---
>  include/mach/rockchip/dmc.h  |  2 ++
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/dmc.c b/arch/arm/mach-rockchip/dmc.c
> index 62a7ef8f1e38e989e84f08f6f4a6586be3e85532..260b8be9c7da9e8f83323822eab92a439ef4e2ef 100644
> --- a/arch/arm/mach-rockchip/dmc.c
> +++ b/arch/arm/mach-rockchip/dmc.c
> @@ -171,11 +171,12 @@ resource_size_t rk3568_ram0_size(void)
>  #define RK3588_PMUGRF_OS_REG4           0x210
>  #define RK3588_PMUGRF_OS_REG5           0x214
>  
> -resource_size_t rk3588_ram0_size(void)
> +size_t rk3588_ram_sizes(phys_addr_t *base, resource_size_t *size, size_t n)

Nit: could be void since you don't check the return value.

Regards,
  Marco

>  {
>  	void __iomem *pmugrf = IOMEM(RK3588_PMUGRF_BASE);
>  	u32 sys_reg2, sys_reg3, sys_reg4, sys_reg5;
> -	resource_size_t size, size1, size2;
> +	resource_size_t memsize, size1, size2;
> +	size_t i = 0;
>  
>  	sys_reg2 = readl(pmugrf + RK3588_PMUGRF_OS_REG2);
>  	sys_reg3 = readl(pmugrf + RK3588_PMUGRF_OS_REG3);
> @@ -187,7 +188,37 @@ resource_size_t rk3588_ram0_size(void)
>  
>  	pr_info("%s() size1 = 0x%08llx, size2 = 0x%08llx\n", __func__, (u64)size1, (u64)size2);
>  
> -	size = min_t(resource_size_t, RK3588_INT_REG_START, size1 + size2);
> +	memsize = size1 + size2;
> +
> +	base[i] = 0xa00000;
> +	size[i] = min_t(resource_size_t, RK3588_INT_REG_START, memsize) - 0xa00000;
> +	i++;
> +
> +	if (i < n && memsize > SZ_4G) {
> +		base[i] = SZ_4G;
> +		size[i] = min_t(unsigned long, DRAM_GAP1_START, memsize) - SZ_4G;
> +		i++;
> +	}
> +	if (i < n && memsize > DRAM_GAP1_END) {
> +		base[i] = DRAM_GAP1_END;
> +		size[i] = min_t(unsigned long, DRAM_GAP2_START, memsize) - DRAM_GAP1_END;
> +		i++;
> +	}
> +	if (i < n && memsize > DRAM_GAP2_END) {
> +		base[i] = DRAM_GAP2_END;
> +		size[i] = memsize - DRAM_GAP2_END;
> +		i++;
> +	}
> +
> +	return i;
> +}
> +
> +resource_size_t rk3588_ram0_size(void)
> +{
> +	phys_addr_t base;
> +	resource_size_t size;
> +
> +	rk3588_ram_sizes(&base, &size, 1);
>  
>  	return size;
>  }
> diff --git a/include/mach/rockchip/dmc.h b/include/mach/rockchip/dmc.h
> index 3df9aa5e9c87857e9ac3994e0e40c60d8c91b0a4..bb5a5afd02417920446c729b96eeb90f1fa0ead0 100644
> --- a/include/mach/rockchip/dmc.h
> +++ b/include/mach/rockchip/dmc.h
> @@ -87,4 +87,6 @@ resource_size_t rk3399_ram0_size(void);
>  resource_size_t rk3568_ram0_size(void);
>  resource_size_t rk3588_ram0_size(void);
>  
> +size_t rk3588_ram_sizes(phys_addr_t *base, resource_size_t *size, size_t n);
> +
>  #endif
> 
> -- 
> 2.39.5
> 
> 
> 



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 8/9] ARM: rockchip: pass device tree to TF-A
  2025-05-26 14:38 ` [PATCH 8/9] ARM: rockchip: pass device tree to TF-A Michael Tretter
@ 2025-05-26 16:37   ` Marco Felsch
  2025-05-27  8:03     ` Michael Tretter
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2025-05-26 16:37 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-26, Michael Tretter wrote:
> The upstream OP-TEE expects a device tree to be able to initialize the
> dynamic shared memory. Therefore, barebox should pass a device tree that
> contains memory nodes through the TF-A to OP-TEE.
> 
> Unfortunately, the downstream TF-A fails to start if barebox passes its
> device tree and it is not possible to detect if the loaded TF-A is able
> to handle the device tree. Add a config option to pass the device tree
> if it is possible.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  arch/arm/mach-rockchip/Kconfig | 13 +++++++++++++
>  arch/arm/mach-rockchip/atf.c   | 15 ++++-----------
>  pbl/Kconfig                    |  1 +
>  3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index 98dfd11c182b9fee6e3c958653ad4fa8a7d98d84..257c13bcf4846e805a7803105c71edeff92c534d 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -141,6 +141,19 @@ config ARCH_ROCKCHIP_ATF
>  	  useful for debugging early startup, but for all other cases,
>  	  say y here.
>  
> +config ARCH_ROCKCHIP_ATF_PASS_FDT
> +	bool "Pass device tree to TF-A"
> +	depends on ARCH_ROCKCHIP_ATF
> +	help
> +	  Enable this option if you are using an upstream OP-TEE that uses the
> +	  device tree to initialize dynamic shared memory, which is passed
> +	  through the upstream TF-A.

Would be nice to document this within the Documentation/ dir too.

Regards,
  Marco

> +
> +	  Disable the option if you are using a downstream TF-A since it
> +	  doesn't always cope with device trees. Supposedly this happens if the
> +	  device tree is too large, for example if CONFIG_OF_OVERLAY_LIVE is
> +	  enabled.
> +
>  config ARCH_ROCKCHIP_OPTEE
>  	bool "Build rockchip OP-TEE binary into barebox"
>  	depends on ARCH_ROCKCHIP_ATF
> diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
> index cfa6df043b34c0f36919048237c7ecf33dfe0724..12cf13717b6972c2eafc5a044ae8d0b4de029c32 100644
> --- a/arch/arm/mach-rockchip/atf.c
> +++ b/arch/arm/mach-rockchip/atf.c
> @@ -186,18 +186,11 @@ void __noreturn rk3588_barebox_entry(void *fdt)
>  		rk3588_lowlevel_init();
>  		rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
>  
> -		/*
> -		 * The downstream TF-A doesn't cope with our device tree when
> -		 * CONFIG_OF_OVERLAY_LIVE is enabled, supposedly because it is
> -		 * too big for some reason. Otherwise it doesn't have any visible
> -		 * effect if we pass a device tree or not, except that the TF-A
> -		 * fills in the ethernet MAC address into the device tree.
> -		 * The upstream TF-A doesn't use the device tree at all.
> -		 *
> -		 * Pass NULL for now until we have a good reason to pass a real
> -		 * device tree.
> -		 */
> +#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> +		rk3588_atf_load_bl31(fdt);
> +#else
>  		rk3588_atf_load_bl31(NULL);
> +#endif
>  		/* not reached when CONFIG_ARCH_ROCKCHIP_ATF */
>  	}
>  
> diff --git a/pbl/Kconfig b/pbl/Kconfig
> index 489b2001a855d62e11a2159311332b0e67f3a754..3dfc7de3e80da12a6313e84175275070274b9a5d 100644
> --- a/pbl/Kconfig
> +++ b/pbl/Kconfig
> @@ -63,6 +63,7 @@ config PBL_CLOCKSOURCE
>  config PBL_FDT_MIN_SIZE
>  	hex
>  	default 0x0
> +	default 0x60000 if ARCH_ROCKCHIP_ATF_PASS_FDT
>  	prompt "Minimum size of the FDT blob"
>  	help
>  	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This
> 
> -- 
> 2.39.5
> 
> 
> 



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A
  2025-05-26 14:38 ` [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A Michael Tretter
@ 2025-05-26 17:25   ` Marco Felsch
  2025-05-27  8:16     ` Michael Tretter
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2025-05-26 17:25 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

Hi Michael,

On 25-05-26, Michael Tretter wrote:
> Add the memory nodes for the detected SDRAM configuration to the fdt
> before passing it to the TF-A.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  arch/arm/mach-rockchip/atf.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
> index 12cf13717b6972c2eafc5a044ae8d0b4de029c32..342af302aa25089acce3c91df0ea38cbe71e0add 100644
> --- a/arch/arm/mach-rockchip/atf.c
> +++ b/arch/arm/mach-rockchip/atf.c
> @@ -173,6 +173,26 @@ void rk3588_atf_load_bl31(void *fdt)
>  	rockchip_atf_load_bl31(RK3588, rk3588_bl31_bin, rk3588_bl32_bin, fdt);
>  }
>  
> +#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> +static int rk3588_fixup_mem(void *fdt)
> +{
> +	/* Use 4 blocks since rk3588 has 3 gaps in the address space */
> +	unsigned long base[4];
> +	unsigned long size[ARRAY_SIZE(base)];
> +	phys_addr_t base_tmp[ARRAY_SIZE(base)];
> +	resource_size_t size_tmp[ARRAY_SIZE(base_tmp)];
> +	int i, n;
> +
> +	n = rk3588_ram_sizes(base_tmp, size_tmp, ARRAY_SIZE(base_tmp));
> +	for (i = 0; i < n; i++) {
> +		base[i] = base_tmp[i];
> +		size[i] = size_tmp[i];
> +	}
> +
> +	return fdt_fixup_mem(fdt, base, size, i);

This fixup will run on a RO marked section if I got the code correct.
Also the fixup logic doesn't work for compressed device-tree's.

I had an offlist discussion with Ahmad last week exactly targeting such
use-case (passing the dt from firmware to firmware). The conclusion was
that each firmware should generate an overlay which will be applied to
the barebox live-dt and the kernel-dt later on.

Question: Is this to late for OP-TEE? I don't know the RK3588 nor the
OP-TEE integration for it, but on i.MX8M the OP-TEE SHM doesn't require
any device-tree yet.

Regards,
  Marco

> +}
> +#endif
> +
>  void __noreturn rk3588_barebox_entry(void *fdt)
>  {
>  	unsigned long membase, endmem;
> @@ -187,6 +207,8 @@ void __noreturn rk3588_barebox_entry(void *fdt)
>  		rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
>  
>  #ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> +		if (rk3588_fixup_mem(fdt) != 0)
> +			pr_warn("Failed to fixup memory\n");
>  		rk3588_atf_load_bl31(fdt);
>  #else
>  		rk3588_atf_load_bl31(NULL);
> 
> -- 
> 2.39.5
> 
> 
> 



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/9] ARM: rockchip: dmc: use RK3588_INT_REG_START for rk3588
  2025-05-26 14:38 ` [PATCH 2/9] ARM: rockchip: dmc: use RK3588_INT_REG_START for rk3588 Michael Tretter
@ 2025-05-26 17:29   ` Marco Felsch
  0 siblings, 0 replies; 31+ messages in thread
From: Marco Felsch @ 2025-05-26 17:29 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-26, Michael Tretter wrote:
> While RK3588_INT_REG_START and RK3568_INT_REG_START are the same value,
> the implementation for rk3588 should use RK3588_INT_REG_START to avoid
> confusion.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/9] lib: fdt: add fdt_addresses
  2025-05-26 14:38 ` [PATCH 3/9] lib: fdt: add fdt_addresses Michael Tretter
@ 2025-05-26 17:29   ` Marco Felsch
  0 siblings, 0 replies; 31+ messages in thread
From: Marco Felsch @ 2025-05-26 17:29 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-26, Michael Tretter wrote:
> fdt_addresses contains some helpers for handling addresses and sizes.
> Add it to the local libfdt.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 4/9] PBL: fdt: refactor helper for reading nr of cells
  2025-05-26 14:38 ` [PATCH 4/9] PBL: fdt: refactor helper for reading nr of cells Michael Tretter
@ 2025-05-26 17:30   ` Marco Felsch
  0 siblings, 0 replies; 31+ messages in thread
From: Marco Felsch @ 2025-05-26 17:30 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-26, Michael Tretter wrote:
> Since libfdt now contains fdt_addresses, the fdt_find_mem function can
> use the already existing helpers to read #size-cells and #address-cells.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/9] ARM: rockchip: fix formatting
  2025-05-26 14:38 ` [PATCH 1/9] ARM: rockchip: fix formatting Michael Tretter
@ 2025-05-26 17:30   ` Marco Felsch
  0 siblings, 0 replies; 31+ messages in thread
From: Marco Felsch @ 2025-05-26 17:30 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-26, Michael Tretter wrote:
> Replace spaces with tabs and remove a double newline. No semantic
> changes.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes
  2025-05-26 14:38 ` [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes Michael Tretter
@ 2025-05-27  6:22   ` Sascha Hauer
  2025-05-27  8:34     ` Michael Tretter
  0 siblings, 1 reply; 31+ messages in thread
From: Sascha Hauer @ 2025-05-27  6:22 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On Mon, May 26, 2025 at 04:38:12PM +0200, Michael Tretter wrote:
> Board code in the PBL may use fdt_fixup_mem() to write the base
> addresses and sizes of detected SDRAM to the fdt before passing the fdt
> to other software like the TF-A and OP-TEE.
> 
> This is implemented on the fdt to avoid that the PBL needs to unpack the
> device tree, use the of function for manipulation and repack the device
> tree.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  include/pbl.h |  1 +
>  pbl/fdt.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/include/pbl.h b/include/pbl.h
> index abac3458593af2cec972a54ce9fb69e344179670..b330010562c4aba5fccbb4c421bb95291fa0bea1 100644
> --- a/include/pbl.h
> +++ b/include/pbl.h
> @@ -17,6 +17,7 @@ extern unsigned long free_mem_end_ptr;
>  void pbl_barebox_uncompress(void *dest, void *compressed_start, unsigned int len);
>  
>  void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsize);
> +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num);
>  
>  struct fdt_device_id {
>  	const char *compatible;
> diff --git a/pbl/fdt.c b/pbl/fdt.c
> index 3b1783152a8cb81f3eda1187b2f7bf998c4addf4..40564952acfcd88bef050966720e76b7378bc720 100644
> --- a/pbl/fdt.c
> +++ b/pbl/fdt.c
> @@ -2,6 +2,7 @@
>  #include <linux/libfdt.h>
>  #include <pbl.h>
>  #include <linux/printk.h>
> +#include <stdio.h>
>  
>  static const __be32 *fdt_parse_reg(const __be32 *reg, uint32_t n,
>  				   uint64_t *val)
> @@ -73,6 +74,76 @@ void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsiz
>  	while (1);
>  }
>  
> +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num)
> +{
> +	int na, ns;
> +	int node, root;
> +	int err;
> +	int i;
> +
> +	err = fdt_check_header(fdt);
> +	if (err != 0) {
> +		pr_err("Invalid device tree blob\n");
> +		return err;
> +	}
> +
> +	root = fdt_path_offset(fdt, "/");
> +	if (root < 0) {
> +		pr_err("Cannot find root node: %s\n", fdt_strerror(root));
> +		return root;
> +	}
> +
> +	na = fdt_address_cells(fdt, root);
> +	if (na < 0) {
> +		pr_err("Cannot find #address-cells property: %s\n",
> +		       fdt_strerror(na));
> +		return na;
> +	}
> +
> +	ns = fdt_size_cells(fdt, root);
> +	if (ns < 0) {
> +		pr_err("Cannot find #size-cells property: %s\n",
> +				fdt_strerror(ns));
> +		return ns;
> +	}

fdt_appendprop_addrrange() handles #address-cells and #size-cells
internally, consequently na and ns are not used in this function.

> +
> +	for (i = 0; i < num; i++) {
> +		char name[32];
> +
> +		if (membase[i] == 0 || memsize[i] == 0)
> +			continue;
> +
> +		snprintf(name, sizeof(name), "memory@%lx", membase[i]);
> +		node = fdt_add_subnode(fdt, root, name);

There are some pitfalls in this function when the device tree already
has memory nodes. It seems fdt_add_subnode() returns -FDT_ERR_EXISTS
in case the node already exists. You should likely check that.

Some device trees have a plain "memory" node without the @address
postfix. These should be deleted before adding other memory nodes.

> +		if (node < 0) {
> +			pr_err("Cannot to add node %s: %s\n",
> +			       name, fdt_strerror(node));
> +			err = node;
> +			break;
> +		}
> +
> +		pr_debug("Add memory node %s (0x%lx, size 0x%lx)\n",
> +			 name, membase[i], memsize[i]);
> +
> +		err = fdt_setprop(fdt, node,
> +				  "device_type", "memory", sizeof("memory"));
> +		if (err < 0) {
> +			pr_err("%s: Cannot set device_type: %s\n",
> +			       name, fdt_strerror(err));
> +			return err;
> +		}
> +
> +		err = fdt_appendprop_addrrange(fdt, root, node, "reg", membase[i], memsize[i]);

This sounds like it appends the range to ranges already existing in the
node. Do you have to call fdt_delprop(..., "reg") beforehand?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size
  2025-05-26 16:33   ` Marco Felsch
@ 2025-05-27  6:25     ` Sascha Hauer
  2025-05-27  8:39       ` Michael Tretter
  0 siblings, 1 reply; 31+ messages in thread
From: Sascha Hauer @ 2025-05-27  6:25 UTC (permalink / raw)
  To: Marco Felsch; +Cc: Michael Tretter, BAREBOX

On Mon, May 26, 2025 at 06:33:28PM +0200, Marco Felsch wrote:
> On 25-05-26, Michael Tretter wrote:
> > The PBL has to pass a full description of the SDRAM to OP-TEE to allow
> > OP-TEE to handle dynamic shared memory in the entire SDRAM. Thus, the
> > PBL needs to read the full memory configuration.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  arch/arm/mach-rockchip/dmc.c | 37 ++++++++++++++++++++++++++++++++++---
> >  include/mach/rockchip/dmc.h  |  2 ++
> >  2 files changed, 36 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mach-rockchip/dmc.c b/arch/arm/mach-rockchip/dmc.c
> > index 62a7ef8f1e38e989e84f08f6f4a6586be3e85532..260b8be9c7da9e8f83323822eab92a439ef4e2ef 100644
> > --- a/arch/arm/mach-rockchip/dmc.c
> > +++ b/arch/arm/mach-rockchip/dmc.c
> > @@ -171,11 +171,12 @@ resource_size_t rk3568_ram0_size(void)
> >  #define RK3588_PMUGRF_OS_REG4           0x210
> >  #define RK3588_PMUGRF_OS_REG5           0x214
> >  
> > -resource_size_t rk3588_ram0_size(void)
> > +size_t rk3588_ram_sizes(phys_addr_t *base, resource_size_t *size, size_t n)
> 
> Nit: could be void since you don't check the return value.

The return value is used in a later patch. There the array size is
passed in 'n' and the actually used array entries are returned from
rk3588_ram_sizes().

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 5/9] PBL: fdt: make minimum fdt size configurable
  2025-05-26 14:38 ` [PATCH 5/9] PBL: fdt: make minimum fdt size configurable Michael Tretter
@ 2025-05-27  6:41   ` Sascha Hauer
  2025-05-27  8:48     ` Michael Tretter
  0 siblings, 1 reply; 31+ messages in thread
From: Sascha Hauer @ 2025-05-27  6:41 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On Mon, May 26, 2025 at 04:38:11PM +0200, Michael Tretter wrote:
> Adding or modifying nodes in the fdt may change the size of the fdt.
> This needs some reserved space in the fdt to avoid overriding memory
> that comes after the fdt and is already used.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  pbl/Kconfig          | 11 +++++++++++
>  scripts/Makefile.lib |  2 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/pbl/Kconfig b/pbl/Kconfig
> index 6e3581829d589c7b06ed878b09bf74e16a0c3086..489b2001a855d62e11a2159311332b0e67f3a754 100644
> --- a/pbl/Kconfig
> +++ b/pbl/Kconfig
> @@ -60,6 +60,17 @@ config PBL_VERIFY_PIGGY
>  config PBL_CLOCKSOURCE
>  	bool
>  
> +config PBL_FDT_MIN_SIZE
> +	hex
> +	default 0x0
> +	prompt "Minimum size of the FDT blob"
> +	help
> +	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This
> +	  may increases the size of the device tree. This may override the
> +	  barebox binary.
> +
> +	  The minimum size should be at least CFG_DTB_MAX_SIZE for OP-TEE.
> +
>  config BOARD_GENERIC_DT
>  	bool
>  	select LIBFDT
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index b10119797686ea31fe927d29a7849ad525c8c835..f50006f57200a76a86c7e29175dd7e35ab138e26 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -396,6 +396,8 @@ ifeq ($(CONFIG_OF_OVERLAY_LIVE), y)
>  DTC_FLAGS.dtb += -@
>  endif
>  
> +DTC_FLAGS.dtb += --space $(CONFIG_PBL_FDT_MIN_SIZE)

--space sets the minimum fdt size. The fdt size will vary over time and
different boards. Isn't --pad more suitable for what you want to
archieve?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 8/9] ARM: rockchip: pass device tree to TF-A
  2025-05-26 16:37   ` Marco Felsch
@ 2025-05-27  8:03     ` Michael Tretter
  0 siblings, 0 replies; 31+ messages in thread
From: Michael Tretter @ 2025-05-27  8:03 UTC (permalink / raw)
  To: Marco Felsch; +Cc: BAREBOX

On Mon, 26 May 2025 18:37:23 +0200, Marco Felsch wrote:
> On 25-05-26, Michael Tretter wrote:
> > The upstream OP-TEE expects a device tree to be able to initialize the
> > dynamic shared memory. Therefore, barebox should pass a device tree that
> > contains memory nodes through the TF-A to OP-TEE.
> > 
> > Unfortunately, the downstream TF-A fails to start if barebox passes its
> > device tree and it is not possible to detect if the loaded TF-A is able
> > to handle the device tree. Add a config option to pass the device tree
> > if it is possible.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  arch/arm/mach-rockchip/Kconfig | 13 +++++++++++++
> >  arch/arm/mach-rockchip/atf.c   | 15 ++++-----------
> >  pbl/Kconfig                    |  1 +
> >  3 files changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> > index 98dfd11c182b9fee6e3c958653ad4fa8a7d98d84..257c13bcf4846e805a7803105c71edeff92c534d 100644
> > --- a/arch/arm/mach-rockchip/Kconfig
> > +++ b/arch/arm/mach-rockchip/Kconfig
> > @@ -141,6 +141,19 @@ config ARCH_ROCKCHIP_ATF
> >  	  useful for debugging early startup, but for all other cases,
> >  	  say y here.
> >  
> > +config ARCH_ROCKCHIP_ATF_PASS_FDT
> > +	bool "Pass device tree to TF-A"
> > +	depends on ARCH_ROCKCHIP_ATF
> > +	help
> > +	  Enable this option if you are using an upstream OP-TEE that uses the
> > +	  device tree to initialize dynamic shared memory, which is passed
> > +	  through the upstream TF-A.
> 
> Would be nice to document this within the Documentation/ dir too.

Ack. There are already some notes about the binaries in
Documentation/boards/rockchip.rst. I'll add some documentation about the
device tree size there.

Michael

> 
> Regards,
>   Marco
> 
> > +
> > +	  Disable the option if you are using a downstream TF-A since it
> > +	  doesn't always cope with device trees. Supposedly this happens if the
> > +	  device tree is too large, for example if CONFIG_OF_OVERLAY_LIVE is
> > +	  enabled.
> > +
> >  config ARCH_ROCKCHIP_OPTEE
> >  	bool "Build rockchip OP-TEE binary into barebox"
> >  	depends on ARCH_ROCKCHIP_ATF
> > diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
> > index cfa6df043b34c0f36919048237c7ecf33dfe0724..12cf13717b6972c2eafc5a044ae8d0b4de029c32 100644
> > --- a/arch/arm/mach-rockchip/atf.c
> > +++ b/arch/arm/mach-rockchip/atf.c
> > @@ -186,18 +186,11 @@ void __noreturn rk3588_barebox_entry(void *fdt)
> >  		rk3588_lowlevel_init();
> >  		rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
> >  
> > -		/*
> > -		 * The downstream TF-A doesn't cope with our device tree when
> > -		 * CONFIG_OF_OVERLAY_LIVE is enabled, supposedly because it is
> > -		 * too big for some reason. Otherwise it doesn't have any visible
> > -		 * effect if we pass a device tree or not, except that the TF-A
> > -		 * fills in the ethernet MAC address into the device tree.
> > -		 * The upstream TF-A doesn't use the device tree at all.
> > -		 *
> > -		 * Pass NULL for now until we have a good reason to pass a real
> > -		 * device tree.
> > -		 */
> > +#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> > +		rk3588_atf_load_bl31(fdt);
> > +#else
> >  		rk3588_atf_load_bl31(NULL);
> > +#endif
> >  		/* not reached when CONFIG_ARCH_ROCKCHIP_ATF */
> >  	}
> >  
> > diff --git a/pbl/Kconfig b/pbl/Kconfig
> > index 489b2001a855d62e11a2159311332b0e67f3a754..3dfc7de3e80da12a6313e84175275070274b9a5d 100644
> > --- a/pbl/Kconfig
> > +++ b/pbl/Kconfig
> > @@ -63,6 +63,7 @@ config PBL_CLOCKSOURCE
> >  config PBL_FDT_MIN_SIZE
> >  	hex
> >  	default 0x0
> > +	default 0x60000 if ARCH_ROCKCHIP_ATF_PASS_FDT
> >  	prompt "Minimum size of the FDT blob"
> >  	help
> >  	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This
> > 
> > -- 
> > 2.39.5
> > 
> > 
> > 
> 



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A
  2025-05-26 17:25   ` Marco Felsch
@ 2025-05-27  8:16     ` Michael Tretter
  2025-05-27  9:40       ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-27  8:16 UTC (permalink / raw)
  To: Marco Felsch; +Cc: BAREBOX

On Mon, 26 May 2025 19:25:01 +0200, Marco Felsch wrote:
> On 25-05-26, Michael Tretter wrote:
> > Add the memory nodes for the detected SDRAM configuration to the fdt
> > before passing it to the TF-A.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  arch/arm/mach-rockchip/atf.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
> > index 12cf13717b6972c2eafc5a044ae8d0b4de029c32..342af302aa25089acce3c91df0ea38cbe71e0add 100644
> > --- a/arch/arm/mach-rockchip/atf.c
> > +++ b/arch/arm/mach-rockchip/atf.c
> > @@ -173,6 +173,26 @@ void rk3588_atf_load_bl31(void *fdt)
> >  	rockchip_atf_load_bl31(RK3588, rk3588_bl31_bin, rk3588_bl32_bin, fdt);
> >  }
> >  
> > +#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> > +static int rk3588_fixup_mem(void *fdt)
> > +{
> > +	/* Use 4 blocks since rk3588 has 3 gaps in the address space */
> > +	unsigned long base[4];
> > +	unsigned long size[ARRAY_SIZE(base)];
> > +	phys_addr_t base_tmp[ARRAY_SIZE(base)];
> > +	resource_size_t size_tmp[ARRAY_SIZE(base_tmp)];
> > +	int i, n;
> > +
> > +	n = rk3588_ram_sizes(base_tmp, size_tmp, ARRAY_SIZE(base_tmp));
> > +	for (i = 0; i < n; i++) {
> > +		base[i] = base_tmp[i];
> > +		size[i] = size_tmp[i];
> > +	}
> > +
> > +	return fdt_fixup_mem(fdt, base, size, i);
> 
> This fixup will run on a RO marked section if I got the code correct.
> Also the fixup logic doesn't work for compressed device-tree's.
> 
> I had an offlist discussion with Ahmad last week exactly targeting such
> use-case (passing the dt from firmware to firmware). The conclusion was
> that each firmware should generate an overlay which will be applied to
> the barebox live-dt and the kernel-dt later on.
> 
> Question: Is this to late for OP-TEE? I don't know the RK3588 nor the
> OP-TEE integration for it, but on i.MX8M the OP-TEE SHM doesn't require
> any device-tree yet.

This is about passing a device tree from barebox to OP-TEE. OP-TEE uses
the device tree and especially the memory nodes to initialize the
dynamic shared memory. This is a platform independent implementation in
OP-TEE.  It's also possible to configure the addresses and sizes via
config parameter, but that doesn't work if a board supports different
SDRAM configurations.

Im not sure if OP-TEE accepts an dt overlay here and if this is
compliant with the Firmware handoff specification. If this is possible,
OP-TEE needs its own device tree for the platform and be able to apply
an overlay. Furthermore, the barebox PBL would have to generate an
overlay fdt with the memory configuration. Not sure if this is actually
better than fixing the device tree before passing it to OP-TEE.

Michael

> 
> Regards,
>   Marco
> 
> > +}
> > +#endif
> > +
> >  void __noreturn rk3588_barebox_entry(void *fdt)
> >  {
> >  	unsigned long membase, endmem;
> > @@ -187,6 +207,8 @@ void __noreturn rk3588_barebox_entry(void *fdt)
> >  		rockchip_store_bootrom_iram(IOMEM(RK3588_IRAM_BASE));
> >  
> >  #ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> > +		if (rk3588_fixup_mem(fdt) != 0)
> > +			pr_warn("Failed to fixup memory\n");
> >  		rk3588_atf_load_bl31(fdt);
> >  #else
> >  		rk3588_atf_load_bl31(NULL);
> > 
> > -- 
> > 2.39.5
> > 
> > 
> > 
> 



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes
  2025-05-27  6:22   ` Sascha Hauer
@ 2025-05-27  8:34     ` Michael Tretter
  2025-05-27 18:51       ` Sascha Hauer
  0 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-27  8:34 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: BAREBOX

On Tue, 27 May 2025 08:22:14 +0200, Sascha Hauer wrote:
> On Mon, May 26, 2025 at 04:38:12PM +0200, Michael Tretter wrote:
> > Board code in the PBL may use fdt_fixup_mem() to write the base
> > addresses and sizes of detected SDRAM to the fdt before passing the fdt
> > to other software like the TF-A and OP-TEE.
> > 
> > This is implemented on the fdt to avoid that the PBL needs to unpack the
> > device tree, use the of function for manipulation and repack the device
> > tree.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  include/pbl.h |  1 +
> >  pbl/fdt.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/include/pbl.h b/include/pbl.h
> > index abac3458593af2cec972a54ce9fb69e344179670..b330010562c4aba5fccbb4c421bb95291fa0bea1 100644
> > --- a/include/pbl.h
> > +++ b/include/pbl.h
> > @@ -17,6 +17,7 @@ extern unsigned long free_mem_end_ptr;
> >  void pbl_barebox_uncompress(void *dest, void *compressed_start, unsigned int len);
> >  
> >  void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsize);
> > +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num);
> >  
> >  struct fdt_device_id {
> >  	const char *compatible;
> > diff --git a/pbl/fdt.c b/pbl/fdt.c
> > index 3b1783152a8cb81f3eda1187b2f7bf998c4addf4..40564952acfcd88bef050966720e76b7378bc720 100644
> > --- a/pbl/fdt.c
> > +++ b/pbl/fdt.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/libfdt.h>
> >  #include <pbl.h>
> >  #include <linux/printk.h>
> > +#include <stdio.h>
> >  
> >  static const __be32 *fdt_parse_reg(const __be32 *reg, uint32_t n,
> >  				   uint64_t *val)
> > @@ -73,6 +74,76 @@ void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsiz
> >  	while (1);
> >  }
> >  
> > +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num)
> > +{
> > +	int na, ns;
> > +	int node, root;
> > +	int err;
> > +	int i;
> > +
> > +	err = fdt_check_header(fdt);
> > +	if (err != 0) {
> > +		pr_err("Invalid device tree blob\n");
> > +		return err;
> > +	}
> > +
> > +	root = fdt_path_offset(fdt, "/");
> > +	if (root < 0) {
> > +		pr_err("Cannot find root node: %s\n", fdt_strerror(root));
> > +		return root;
> > +	}
> > +
> > +	na = fdt_address_cells(fdt, root);
> > +	if (na < 0) {
> > +		pr_err("Cannot find #address-cells property: %s\n",
> > +		       fdt_strerror(na));
> > +		return na;
> > +	}
> > +
> > +	ns = fdt_size_cells(fdt, root);
> > +	if (ns < 0) {
> > +		pr_err("Cannot find #size-cells property: %s\n",
> > +				fdt_strerror(ns));
> > +		return ns;
> > +	}
> 
> fdt_appendprop_addrrange() handles #address-cells and #size-cells
> internally, consequently na and ns are not used in this function.

That's a leftover of a previous implementation. I'll drop it.

> 
> > +
> > +	for (i = 0; i < num; i++) {
> > +		char name[32];
> > +
> > +		if (membase[i] == 0 || memsize[i] == 0)
> > +			continue;
> > +
> > +		snprintf(name, sizeof(name), "memory@%lx", membase[i]);
> > +		node = fdt_add_subnode(fdt, root, name);
> 
> There are some pitfalls in this function when the device tree already
> has memory nodes. It seems fdt_add_subnode() returns -FDT_ERR_EXISTS
> in case the node already exists. You should likely check that.

In this case, the error handling gets even more complex. If the device
tree node already exists and the size specified in the node doesn't
match the size passed to the function, the function has to decide which
value should get precedence. Probably the value passed to the function.

In this case, fdt_fixup_mem() will be able to fixup existing nodes and
add new nodes if the nodes do not exist.

If fdt_fixup_mem() is able to fixup existing nodes, it should really try
hard to find the correct existing node, which will add more complexity.
This needs some tests.

However, I'm not sure if we really want this complexity in the PBL.

> 
> Some device trees have a plain "memory" node without the @address
> postfix. These should be deleted before adding other memory nodes.

OK. That's similar to what of_memory_fixup is doing. I'll add this.

> 
> > +		if (node < 0) {
> > +			pr_err("Cannot to add node %s: %s\n",
> > +			       name, fdt_strerror(node));
> > +			err = node;
> > +			break;
> > +		}
> > +
> > +		pr_debug("Add memory node %s (0x%lx, size 0x%lx)\n",
> > +			 name, membase[i], memsize[i]);
> > +
> > +		err = fdt_setprop(fdt, node,
> > +				  "device_type", "memory", sizeof("memory"));
> > +		if (err < 0) {
> > +			pr_err("%s: Cannot set device_type: %s\n",
> > +			       name, fdt_strerror(err));
> > +			return err;
> > +		}
> > +
> > +		err = fdt_appendprop_addrrange(fdt, root, node, "reg", membase[i], memsize[i]);
> 
> This sounds like it appends the range to ranges already existing in the
> node. Do you have to call fdt_delprop(..., "reg") beforehand?

Right now, this is not necessary, because the node is a newly created
node and doesn't have the "reg" property. If the function is changed to
update existing nodes, the property probably has to be cleared.

Michael



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size
  2025-05-27  6:25     ` Sascha Hauer
@ 2025-05-27  8:39       ` Michael Tretter
  2025-05-27  9:06         ` Marco Felsch
  0 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-27  8:39 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: Marco Felsch, BAREBOX

On Tue, 27 May 2025 08:25:07 +0200, Sascha Hauer wrote:
> On Mon, May 26, 2025 at 06:33:28PM +0200, Marco Felsch wrote:
> > On 25-05-26, Michael Tretter wrote:
> > > The PBL has to pass a full description of the SDRAM to OP-TEE to allow
> > > OP-TEE to handle dynamic shared memory in the entire SDRAM. Thus, the
> > > PBL needs to read the full memory configuration.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  arch/arm/mach-rockchip/dmc.c | 37 ++++++++++++++++++++++++++++++++++---
> > >  include/mach/rockchip/dmc.h  |  2 ++
> > >  2 files changed, 36 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-rockchip/dmc.c b/arch/arm/mach-rockchip/dmc.c
> > > index 62a7ef8f1e38e989e84f08f6f4a6586be3e85532..260b8be9c7da9e8f83323822eab92a439ef4e2ef 100644
> > > --- a/arch/arm/mach-rockchip/dmc.c
> > > +++ b/arch/arm/mach-rockchip/dmc.c
> > > @@ -171,11 +171,12 @@ resource_size_t rk3568_ram0_size(void)
> > >  #define RK3588_PMUGRF_OS_REG4           0x210
> > >  #define RK3588_PMUGRF_OS_REG5           0x214
> > >  
> > > -resource_size_t rk3588_ram0_size(void)
> > > +size_t rk3588_ram_sizes(phys_addr_t *base, resource_size_t *size, size_t n)
> > 
> > Nit: could be void since you don't check the return value.
> 
> The return value is used in a later patch. There the array size is
> passed in 'n' and the actually used array entries are returned from
> rk3588_ram_sizes().

I could add some error handling in rk3588_ram0_size(), but it's
pointless, since the caller of that function doesn't handle errors, too.

Michael



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 5/9] PBL: fdt: make minimum fdt size configurable
  2025-05-27  6:41   ` Sascha Hauer
@ 2025-05-27  8:48     ` Michael Tretter
  2025-05-27 18:45       ` Sascha Hauer
  0 siblings, 1 reply; 31+ messages in thread
From: Michael Tretter @ 2025-05-27  8:48 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: BAREBOX

On Tue, 27 May 2025 08:41:16 +0200, Sascha Hauer wrote:
> On Mon, May 26, 2025 at 04:38:11PM +0200, Michael Tretter wrote:
> > Adding or modifying nodes in the fdt may change the size of the fdt.
> > This needs some reserved space in the fdt to avoid overriding memory
> > that comes after the fdt and is already used.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  pbl/Kconfig          | 11 +++++++++++
> >  scripts/Makefile.lib |  2 ++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/pbl/Kconfig b/pbl/Kconfig
> > index 6e3581829d589c7b06ed878b09bf74e16a0c3086..489b2001a855d62e11a2159311332b0e67f3a754 100644
> > --- a/pbl/Kconfig
> > +++ b/pbl/Kconfig
> > @@ -60,6 +60,17 @@ config PBL_VERIFY_PIGGY
> >  config PBL_CLOCKSOURCE
> >  	bool
> >  
> > +config PBL_FDT_MIN_SIZE
> > +	hex
> > +	default 0x0
> > +	prompt "Minimum size of the FDT blob"
> > +	help
> > +	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This
> > +	  may increases the size of the device tree. This may override the
> > +	  barebox binary.
> > +
> > +	  The minimum size should be at least CFG_DTB_MAX_SIZE for OP-TEE.
> > +
> >  config BOARD_GENERIC_DT
> >  	bool
> >  	select LIBFDT
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index b10119797686ea31fe927d29a7849ad525c8c835..f50006f57200a76a86c7e29175dd7e35ab138e26 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -396,6 +396,8 @@ ifeq ($(CONFIG_OF_OVERLAY_LIVE), y)
> >  DTC_FLAGS.dtb += -@
> >  endif
> >  
> > +DTC_FLAGS.dtb += --space $(CONFIG_PBL_FDT_MIN_SIZE)
> 
> --space sets the minimum fdt size. The fdt size will vary over time and
> different boards. Isn't --pad more suitable for what you want to
> archieve?

--pad would be more appropriate if it is used to reserve extra space for
the memory nodes added by barebox.

However, OP-TEE also adds properties and nodes to the passed fdt. During
initialization it does a fdt_open_into() with size CFG_DTB_MAX_SIZE on
the blob. This allows OP-TEE to write up to CFG_DTB_MAX_SIZE into the
blob.

Using --space with the same value as CFG_DTB_MAX_SIZE ensures that
barebox reserves enough space that a properly configured OP-TEE is not
able to accidentally override anything else by modifying the device
tree.

This could be solved with transfer lists for passing the device tree,
because then barebox would be able to tell OP-TEE about the reserved
size for the fdt, but transfer lists are an experimental feature and as
far as I can tell it's advised against using them.

Michael



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size
  2025-05-27  8:39       ` Michael Tretter
@ 2025-05-27  9:06         ` Marco Felsch
  0 siblings, 0 replies; 31+ messages in thread
From: Marco Felsch @ 2025-05-27  9:06 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-27, Michael Tretter wrote:
> On Tue, 27 May 2025 08:25:07 +0200, Sascha Hauer wrote:
> > On Mon, May 26, 2025 at 06:33:28PM +0200, Marco Felsch wrote:
> > > On 25-05-26, Michael Tretter wrote:
> > > > The PBL has to pass a full description of the SDRAM to OP-TEE to allow
> > > > OP-TEE to handle dynamic shared memory in the entire SDRAM. Thus, the
> > > > PBL needs to read the full memory configuration.
> > > > 
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > ---
> > > >  arch/arm/mach-rockchip/dmc.c | 37 ++++++++++++++++++++++++++++++++++---
> > > >  include/mach/rockchip/dmc.h  |  2 ++
> > > >  2 files changed, 36 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-rockchip/dmc.c b/arch/arm/mach-rockchip/dmc.c
> > > > index 62a7ef8f1e38e989e84f08f6f4a6586be3e85532..260b8be9c7da9e8f83323822eab92a439ef4e2ef 100644
> > > > --- a/arch/arm/mach-rockchip/dmc.c
> > > > +++ b/arch/arm/mach-rockchip/dmc.c
> > > > @@ -171,11 +171,12 @@ resource_size_t rk3568_ram0_size(void)
> > > >  #define RK3588_PMUGRF_OS_REG4           0x210
> > > >  #define RK3588_PMUGRF_OS_REG5           0x214
> > > >  
> > > > -resource_size_t rk3588_ram0_size(void)
> > > > +size_t rk3588_ram_sizes(phys_addr_t *base, resource_size_t *size, size_t n)
> > > 
> > > Nit: could be void since you don't check the return value.
> > 
> > The return value is used in a later patch. There the array size is
> > passed in 'n' and the actually used array entries are returned from
> > rk3588_ram_sizes().
> 
> I could add some error handling in rk3588_ram0_size(), but it's
> pointless, since the caller of that function doesn't handle errors, too.

Sure, didn't saw the usage in the next commit Sascha mentioned. Please
ignore this comment.

Regards,
  Marco



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A
  2025-05-27  8:16     ` Michael Tretter
@ 2025-05-27  9:40       ` Marco Felsch
  2025-05-27 10:19         ` Michael Tretter
  0 siblings, 1 reply; 31+ messages in thread
From: Marco Felsch @ 2025-05-27  9:40 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On 25-05-27, Michael Tretter wrote:
> On Mon, 26 May 2025 19:25:01 +0200, Marco Felsch wrote:
> > On 25-05-26, Michael Tretter wrote:
> > > Add the memory nodes for the detected SDRAM configuration to the fdt
> > > before passing it to the TF-A.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  arch/arm/mach-rockchip/atf.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
> > > index 12cf13717b6972c2eafc5a044ae8d0b4de029c32..342af302aa25089acce3c91df0ea38cbe71e0add 100644
> > > --- a/arch/arm/mach-rockchip/atf.c
> > > +++ b/arch/arm/mach-rockchip/atf.c
> > > @@ -173,6 +173,26 @@ void rk3588_atf_load_bl31(void *fdt)
> > >  	rockchip_atf_load_bl31(RK3588, rk3588_bl31_bin, rk3588_bl32_bin, fdt);
> > >  }
> > >  
> > > +#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> > > +static int rk3588_fixup_mem(void *fdt)
> > > +{
> > > +	/* Use 4 blocks since rk3588 has 3 gaps in the address space */
> > > +	unsigned long base[4];
> > > +	unsigned long size[ARRAY_SIZE(base)];
> > > +	phys_addr_t base_tmp[ARRAY_SIZE(base)];
> > > +	resource_size_t size_tmp[ARRAY_SIZE(base_tmp)];
> > > +	int i, n;
> > > +
> > > +	n = rk3588_ram_sizes(base_tmp, size_tmp, ARRAY_SIZE(base_tmp));
> > > +	for (i = 0; i < n; i++) {
> > > +		base[i] = base_tmp[i];
> > > +		size[i] = size_tmp[i];
> > > +	}
> > > +
> > > +	return fdt_fixup_mem(fdt, base, size, i);
> > 
> > This fixup will run on a RO marked section if I got the code correct.
> > Also the fixup logic doesn't work for compressed device-tree's.
> > 
> > I had an offlist discussion with Ahmad last week exactly targeting such
> > use-case (passing the dt from firmware to firmware). The conclusion was
> > that each firmware should generate an overlay which will be applied to
> > the barebox live-dt and the kernel-dt later on.
> > 
> > Question: Is this to late for OP-TEE? I don't know the RK3588 nor the
> > OP-TEE integration for it, but on i.MX8M the OP-TEE SHM doesn't require
> > any device-tree yet.
> 
> This is about passing a device tree from barebox to OP-TEE. OP-TEE uses
> the device tree and especially the memory nodes to initialize the
> dynamic shared memory. This is a platform independent implementation in
> OP-TEE.  It's also possible to configure the addresses and sizes via
> config parameter, but that doesn't work if a board supports different
> SDRAM configurations.

Because OP-TEE is loaded at the SDRAM end per default?

If this is the reason, we already do support putting OP-TEE at the SDRAM
start followed by barebox on i.MX8M* SoCs:

   OP-TEE membase parsing:
 - https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/mach-imx/atf.c#L168

   OP-TEE set membase:
 - https://elixir.bootlin.com/barebox/v2025.05.0/source/common/optee.c#L52
   https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/mach-imx/esdctl.c#L1016
   https://elixir.bootlin.com/barebox/v2025.05.0/source/drivers/soc/imx/soc-imx8m.c#L217

   Barebox bin loadaddr:
 - https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/mach-imx/atf.c#L218

> Im not sure if OP-TEE accepts an dt overlay here and if this is
> compliant with the Firmware handoff specification. If this is possible,
> OP-TEE needs its own device tree for the platform and be able to apply
> an overlay. Furthermore, the barebox PBL would have to generate an
> overlay fdt with the memory configuration. Not sure if this is actually
> better than fixing the device tree before passing it to OP-TEE.

The idea was more like this:

   OP-TEE:
 - https://elixir.bootlin.com/op-tee/4.6.0/source/mk/config.mk#L553

   Barebox:
 - https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/boards/webasto-ccbv2/board.c#L33

If I get the OP-TEE code correct, we can pass the location for the
overlay via boot-argument arg2. So barebox can do all the RAM detection
and pass the correct location for the DT overlay. This can be handed
over to barebox proper via the barebox handoff-data mechanism:

 - https://elixir.bootlin.com/barebox/v2025.05.0/source/include/pbl/handoff-data.h

Regards,
  Marco



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A
  2025-05-27  9:40       ` Marco Felsch
@ 2025-05-27 10:19         ` Michael Tretter
  0 siblings, 0 replies; 31+ messages in thread
From: Michael Tretter @ 2025-05-27 10:19 UTC (permalink / raw)
  To: Marco Felsch; +Cc: BAREBOX

On Tue, 27 May 2025 11:40:56 +0200, Marco Felsch wrote:
> On 25-05-27, Michael Tretter wrote:
> > On Mon, 26 May 2025 19:25:01 +0200, Marco Felsch wrote:
> > > On 25-05-26, Michael Tretter wrote:
> > > > Add the memory nodes for the detected SDRAM configuration to the fdt
> > > > before passing it to the TF-A.
> > > > 
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > ---
> > > >  arch/arm/mach-rockchip/atf.c | 22 ++++++++++++++++++++++
> > > >  1 file changed, 22 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/mach-rockchip/atf.c b/arch/arm/mach-rockchip/atf.c
> > > > index 12cf13717b6972c2eafc5a044ae8d0b4de029c32..342af302aa25089acce3c91df0ea38cbe71e0add 100644
> > > > --- a/arch/arm/mach-rockchip/atf.c
> > > > +++ b/arch/arm/mach-rockchip/atf.c
> > > > @@ -173,6 +173,26 @@ void rk3588_atf_load_bl31(void *fdt)
> > > >  	rockchip_atf_load_bl31(RK3588, rk3588_bl31_bin, rk3588_bl32_bin, fdt);
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_ARCH_ROCKCHIP_ATF_PASS_FDT
> > > > +static int rk3588_fixup_mem(void *fdt)
> > > > +{
> > > > +	/* Use 4 blocks since rk3588 has 3 gaps in the address space */
> > > > +	unsigned long base[4];
> > > > +	unsigned long size[ARRAY_SIZE(base)];
> > > > +	phys_addr_t base_tmp[ARRAY_SIZE(base)];
> > > > +	resource_size_t size_tmp[ARRAY_SIZE(base_tmp)];
> > > > +	int i, n;
> > > > +
> > > > +	n = rk3588_ram_sizes(base_tmp, size_tmp, ARRAY_SIZE(base_tmp));
> > > > +	for (i = 0; i < n; i++) {
> > > > +		base[i] = base_tmp[i];
> > > > +		size[i] = size_tmp[i];
> > > > +	}
> > > > +
> > > > +	return fdt_fixup_mem(fdt, base, size, i);
> > > 
> > > This fixup will run on a RO marked section if I got the code correct.
> > > Also the fixup logic doesn't work for compressed device-tree's.
> > > 
> > > I had an offlist discussion with Ahmad last week exactly targeting such
> > > use-case (passing the dt from firmware to firmware). The conclusion was
> > > that each firmware should generate an overlay which will be applied to
> > > the barebox live-dt and the kernel-dt later on.
> > > 
> > > Question: Is this to late for OP-TEE? I don't know the RK3588 nor the
> > > OP-TEE integration for it, but on i.MX8M the OP-TEE SHM doesn't require
> > > any device-tree yet.
> > 
> > This is about passing a device tree from barebox to OP-TEE. OP-TEE uses
> > the device tree and especially the memory nodes to initialize the
> > dynamic shared memory. This is a platform independent implementation in
> > OP-TEE.  It's also possible to configure the addresses and sizes via
> > config parameter, but that doesn't work if a board supports different
> > SDRAM configurations.
> 
> Because OP-TEE is loaded at the SDRAM end per default?

No, it's not about loading OP-TEE to the SDRAM, but about OP-TEE
initializing its internal mapping of dynamic memory. This mapping is
used to resolve addresses passed to OP-TEE via SMC from barebox or
Linux. Without this, OP-TEE may be unable to access data that is passed
by Linux via shared memory.

> 
> If this is the reason, we already do support putting OP-TEE at the SDRAM
> start followed by barebox on i.MX8M* SoCs:
> 
>    OP-TEE membase parsing:
>  - https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/mach-imx/atf.c#L168
> 
>    OP-TEE set membase:
>  - https://elixir.bootlin.com/barebox/v2025.05.0/source/common/optee.c#L52
>    https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/mach-imx/esdctl.c#L1016
>    https://elixir.bootlin.com/barebox/v2025.05.0/source/drivers/soc/imx/soc-imx8m.c#L217
> 
>    Barebox bin loadaddr:
>  - https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/mach-imx/atf.c#L218
> 
> > Im not sure if OP-TEE accepts an dt overlay here and if this is
> > compliant with the Firmware handoff specification. If this is possible,
> > OP-TEE needs its own device tree for the platform and be able to apply
> > an overlay. Furthermore, the barebox PBL would have to generate an
> > overlay fdt with the memory configuration. Not sure if this is actually
> > better than fixing the device tree before passing it to OP-TEE.
> 
> The idea was more like this:
> 
>    OP-TEE:
>  - https://elixir.bootlin.com/op-tee/4.6.0/source/mk/config.mk#L553
> 
>    Barebox:
>  - https://elixir.bootlin.com/barebox/v2025.05.0/source/arch/arm/boards/webasto-ccbv2/board.c#L33

This passes an overlay that is generated by OP-TEE back to barebox.

rk3588 needs the other direction of passing a device tree / overlay from
barebox to OP-TEE.

OP-TEE uses the external dt to discover the non-secure memory:

https://elixir.bootlin.com/op-tee/4.6.0/source/core/kernel/boot.c#L136

> 
> If I get the OP-TEE code correct, we can pass the location for the
> overlay via boot-argument arg2. So barebox can do all the RAM detection
> and pass the correct location for the DT overlay. This can be handed
> over to barebox proper via the barebox handoff-data mechanism:
> 
>  - https://elixir.bootlin.com/barebox/v2025.05.0/source/include/pbl/handoff-data.h

Correct, but arg2 is also used to pass an external dt to OP-TEE. Thus,
OP-TEE may update the passed external dt or generate an overlay at that
location. That's a problem if the reserved size is too small, but
unrelated to issue that barebox needs to pass a device tree to OP-TEE.

Michael



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 5/9] PBL: fdt: make minimum fdt size configurable
  2025-05-27  8:48     ` Michael Tretter
@ 2025-05-27 18:45       ` Sascha Hauer
  2025-05-28  8:24         ` Michael Tretter
  0 siblings, 1 reply; 31+ messages in thread
From: Sascha Hauer @ 2025-05-27 18:45 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On Tue, May 27, 2025 at 10:48:02AM +0200, Michael Tretter wrote:
> On Tue, 27 May 2025 08:41:16 +0200, Sascha Hauer wrote:
> > On Mon, May 26, 2025 at 04:38:11PM +0200, Michael Tretter wrote:
> > > Adding or modifying nodes in the fdt may change the size of the fdt.
> > > This needs some reserved space in the fdt to avoid overriding memory
> > > that comes after the fdt and is already used.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  pbl/Kconfig          | 11 +++++++++++
> > >  scripts/Makefile.lib |  2 ++
> > >  2 files changed, 13 insertions(+)
> > > 
> > > diff --git a/pbl/Kconfig b/pbl/Kconfig
> > > index 6e3581829d589c7b06ed878b09bf74e16a0c3086..489b2001a855d62e11a2159311332b0e67f3a754 100644
> > > --- a/pbl/Kconfig
> > > +++ b/pbl/Kconfig
> > > @@ -60,6 +60,17 @@ config PBL_VERIFY_PIGGY
> > >  config PBL_CLOCKSOURCE
> > >  	bool
> > >  
> > > +config PBL_FDT_MIN_SIZE
> > > +	hex
> > > +	default 0x0
> > > +	prompt "Minimum size of the FDT blob"
> > > +	help
> > > +	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This
> > > +	  may increases the size of the device tree. This may override the
> > > +	  barebox binary.
> > > +
> > > +	  The minimum size should be at least CFG_DTB_MAX_SIZE for OP-TEE.
> > > +
> > >  config BOARD_GENERIC_DT
> > >  	bool
> > >  	select LIBFDT
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index b10119797686ea31fe927d29a7849ad525c8c835..f50006f57200a76a86c7e29175dd7e35ab138e26 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -396,6 +396,8 @@ ifeq ($(CONFIG_OF_OVERLAY_LIVE), y)
> > >  DTC_FLAGS.dtb += -@
> > >  endif
> > >  
> > > +DTC_FLAGS.dtb += --space $(CONFIG_PBL_FDT_MIN_SIZE)
> > 
> > --space sets the minimum fdt size. The fdt size will vary over time and
> > different boards. Isn't --pad more suitable for what you want to
> > archieve?
> 
> --pad would be more appropriate if it is used to reserve extra space for
> the memory nodes added by barebox.
> 
> However, OP-TEE also adds properties and nodes to the passed fdt. During
> initialization it does a fdt_open_into() with size CFG_DTB_MAX_SIZE on
> the blob. This allows OP-TEE to write up to CFG_DTB_MAX_SIZE into the
> blob.
> 
> Using --space with the same value as CFG_DTB_MAX_SIZE ensures that
> barebox reserves enough space that a properly configured OP-TEE is not
> able to accidentally override anything else by modifying the device
> tree.

Makes sense. What's not so nice is that this --space option inflates all
dtbs from all boards of the current config.

Would fdt_resize() be an option?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes
  2025-05-27  8:34     ` Michael Tretter
@ 2025-05-27 18:51       ` Sascha Hauer
  0 siblings, 0 replies; 31+ messages in thread
From: Sascha Hauer @ 2025-05-27 18:51 UTC (permalink / raw)
  To: Michael Tretter; +Cc: BAREBOX

On Tue, May 27, 2025 at 10:34:58AM +0200, Michael Tretter wrote:
> On Tue, 27 May 2025 08:22:14 +0200, Sascha Hauer wrote:
> > On Mon, May 26, 2025 at 04:38:12PM +0200, Michael Tretter wrote:
> > > Board code in the PBL may use fdt_fixup_mem() to write the base
> > > addresses and sizes of detected SDRAM to the fdt before passing the fdt
> > > to other software like the TF-A and OP-TEE.
> > > 
> > > This is implemented on the fdt to avoid that the PBL needs to unpack the
> > > device tree, use the of function for manipulation and repack the device
> > > tree.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  include/pbl.h |  1 +
> > >  pbl/fdt.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 72 insertions(+)
> > > 
> > > diff --git a/include/pbl.h b/include/pbl.h
> > > index abac3458593af2cec972a54ce9fb69e344179670..b330010562c4aba5fccbb4c421bb95291fa0bea1 100644
> > > --- a/include/pbl.h
> > > +++ b/include/pbl.h
> > > @@ -17,6 +17,7 @@ extern unsigned long free_mem_end_ptr;
> > >  void pbl_barebox_uncompress(void *dest, void *compressed_start, unsigned int len);
> > >  
> > >  void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsize);
> > > +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num);
> > >  
> > >  struct fdt_device_id {
> > >  	const char *compatible;
> > > diff --git a/pbl/fdt.c b/pbl/fdt.c
> > > index 3b1783152a8cb81f3eda1187b2f7bf998c4addf4..40564952acfcd88bef050966720e76b7378bc720 100644
> > > --- a/pbl/fdt.c
> > > +++ b/pbl/fdt.c
> > > @@ -2,6 +2,7 @@
> > >  #include <linux/libfdt.h>
> > >  #include <pbl.h>
> > >  #include <linux/printk.h>
> > > +#include <stdio.h>
> > >  
> > >  static const __be32 *fdt_parse_reg(const __be32 *reg, uint32_t n,
> > >  				   uint64_t *val)
> > > @@ -73,6 +74,76 @@ void fdt_find_mem(const void *fdt, unsigned long *membase, unsigned long *memsiz
> > >  	while (1);
> > >  }
> > >  
> > > +int fdt_fixup_mem(void *fdt, unsigned long membase[], unsigned long memsize[], size_t num)
> > > +{
> > > +	int na, ns;
> > > +	int node, root;
> > > +	int err;
> > > +	int i;
> > > +
> > > +	err = fdt_check_header(fdt);
> > > +	if (err != 0) {
> > > +		pr_err("Invalid device tree blob\n");
> > > +		return err;
> > > +	}
> > > +
> > > +	root = fdt_path_offset(fdt, "/");
> > > +	if (root < 0) {
> > > +		pr_err("Cannot find root node: %s\n", fdt_strerror(root));
> > > +		return root;
> > > +	}
> > > +
> > > +	na = fdt_address_cells(fdt, root);
> > > +	if (na < 0) {
> > > +		pr_err("Cannot find #address-cells property: %s\n",
> > > +		       fdt_strerror(na));
> > > +		return na;
> > > +	}
> > > +
> > > +	ns = fdt_size_cells(fdt, root);
> > > +	if (ns < 0) {
> > > +		pr_err("Cannot find #size-cells property: %s\n",
> > > +				fdt_strerror(ns));
> > > +		return ns;
> > > +	}
> > 
> > fdt_appendprop_addrrange() handles #address-cells and #size-cells
> > internally, consequently na and ns are not used in this function.
> 
> That's a leftover of a previous implementation. I'll drop it.
> 
> > 
> > > +
> > > +	for (i = 0; i < num; i++) {
> > > +		char name[32];
> > > +
> > > +		if (membase[i] == 0 || memsize[i] == 0)
> > > +			continue;
> > > +
> > > +		snprintf(name, sizeof(name), "memory@%lx", membase[i]);
> > > +		node = fdt_add_subnode(fdt, root, name);
> > 
> > There are some pitfalls in this function when the device tree already
> > has memory nodes. It seems fdt_add_subnode() returns -FDT_ERR_EXISTS
> > in case the node already exists. You should likely check that.
> 
> In this case, the error handling gets even more complex. If the device
> tree node already exists and the size specified in the node doesn't
> match the size passed to the function, the function has to decide which
> value should get precedence. Probably the value passed to the function.

I was not aiming for merging the data in the nodes, more like: If that
node already exists, re-use it and overwrite the content with the data
passed to this function.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 5/9] PBL: fdt: make minimum fdt size configurable
  2025-05-27 18:45       ` Sascha Hauer
@ 2025-05-28  8:24         ` Michael Tretter
  0 siblings, 0 replies; 31+ messages in thread
From: Michael Tretter @ 2025-05-28  8:24 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: BAREBOX

On Tue, 27 May 2025 20:45:34 +0200, Sascha Hauer wrote:
> On Tue, May 27, 2025 at 10:48:02AM +0200, Michael Tretter wrote:
> > On Tue, 27 May 2025 08:41:16 +0200, Sascha Hauer wrote:
> > > On Mon, May 26, 2025 at 04:38:11PM +0200, Michael Tretter wrote:
> > > > Adding or modifying nodes in the fdt may change the size of the fdt.
> > > > This needs some reserved space in the fdt to avoid overriding memory
> > > > that comes after the fdt and is already used.
> > > > 
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > ---
> > > >  pbl/Kconfig          | 11 +++++++++++
> > > >  scripts/Makefile.lib |  2 ++
> > > >  2 files changed, 13 insertions(+)
> > > > 
> > > > diff --git a/pbl/Kconfig b/pbl/Kconfig
> > > > index 6e3581829d589c7b06ed878b09bf74e16a0c3086..489b2001a855d62e11a2159311332b0e67f3a754 100644
> > > > --- a/pbl/Kconfig
> > > > +++ b/pbl/Kconfig
> > > > @@ -60,6 +60,17 @@ config PBL_VERIFY_PIGGY
> > > >  config PBL_CLOCKSOURCE
> > > >  	bool
> > > >  
> > > > +config PBL_FDT_MIN_SIZE
> > > > +	hex
> > > > +	default 0x0
> > > > +	prompt "Minimum size of the FDT blob"
> > > > +	help
> > > > +	  The TF-A or OP-TEE may modify the FDT or add nodes to the FDT. This
> > > > +	  may increases the size of the device tree. This may override the
> > > > +	  barebox binary.
> > > > +
> > > > +	  The minimum size should be at least CFG_DTB_MAX_SIZE for OP-TEE.
> > > > +
> > > >  config BOARD_GENERIC_DT
> > > >  	bool
> > > >  	select LIBFDT
> > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > index b10119797686ea31fe927d29a7849ad525c8c835..f50006f57200a76a86c7e29175dd7e35ab138e26 100644
> > > > --- a/scripts/Makefile.lib
> > > > +++ b/scripts/Makefile.lib
> > > > @@ -396,6 +396,8 @@ ifeq ($(CONFIG_OF_OVERLAY_LIVE), y)
> > > >  DTC_FLAGS.dtb += -@
> > > >  endif
> > > >  
> > > > +DTC_FLAGS.dtb += --space $(CONFIG_PBL_FDT_MIN_SIZE)
> > > 
> > > --space sets the minimum fdt size. The fdt size will vary over time and
> > > different boards. Isn't --pad more suitable for what you want to
> > > archieve?
> > 
> > --pad would be more appropriate if it is used to reserve extra space for
> > the memory nodes added by barebox.
> > 
> > However, OP-TEE also adds properties and nodes to the passed fdt. During
> > initialization it does a fdt_open_into() with size CFG_DTB_MAX_SIZE on
> > the blob. This allows OP-TEE to write up to CFG_DTB_MAX_SIZE into the
> > blob.
> > 
> > Using --space with the same value as CFG_DTB_MAX_SIZE ensures that
> > barebox reserves enough space that a properly configured OP-TEE is not
> > able to accidentally override anything else by modifying the device
> > tree.
> 
> Makes sense. What's not so nice is that this --space option inflates all
> dtbs from all boards of the current config.
> 
> Would fdt_resize() be an option?

fdt_resize() still needs some space for resizing the fdt.

I don't like the reserved space in the binary at all, because it also
increases the size of the binary images.

In v2, I will reserve some extra space in the scratch area (and make its
size configurable), copy the fdt to the scratch area and modify it there
(or create a small new fdt).

This would also solve the issue that the fdt is in a rodata section and
may be compressed. Furthermore, this area could be passed to barebox
proper via handoff data and barebox could apply the dt modifications by
OPTEE to its own device tree.

The drawback of this solution is that the barebox PBL has to copy the
fdt while the mmu is off.

Michael



^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2025-05-28  8:28 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-05-26 14:38 [PATCH 0/9] ARM: rockchip: fix dynamic shared memory in OP-TEE Michael Tretter
2025-05-26 14:38 ` [PATCH 1/9] ARM: rockchip: fix formatting Michael Tretter
2025-05-26 17:30   ` Marco Felsch
2025-05-26 14:38 ` [PATCH 2/9] ARM: rockchip: dmc: use RK3588_INT_REG_START for rk3588 Michael Tretter
2025-05-26 17:29   ` Marco Felsch
2025-05-26 14:38 ` [PATCH 3/9] lib: fdt: add fdt_addresses Michael Tretter
2025-05-26 17:29   ` Marco Felsch
2025-05-26 14:38 ` [PATCH 4/9] PBL: fdt: refactor helper for reading nr of cells Michael Tretter
2025-05-26 17:30   ` Marco Felsch
2025-05-26 14:38 ` [PATCH 5/9] PBL: fdt: make minimum fdt size configurable Michael Tretter
2025-05-27  6:41   ` Sascha Hauer
2025-05-27  8:48     ` Michael Tretter
2025-05-27 18:45       ` Sascha Hauer
2025-05-28  8:24         ` Michael Tretter
2025-05-26 14:38 ` [PATCH 6/9] PBL: fdt: add fdt_fixup_mem to fixup memory nodes Michael Tretter
2025-05-27  6:22   ` Sascha Hauer
2025-05-27  8:34     ` Michael Tretter
2025-05-27 18:51       ` Sascha Hauer
2025-05-26 14:38 ` [PATCH 7/9] ARM: rockchip: dmc: add rk3588_ram_sizes to get full ram size Michael Tretter
2025-05-26 16:33   ` Marco Felsch
2025-05-27  6:25     ` Sascha Hauer
2025-05-27  8:39       ` Michael Tretter
2025-05-27  9:06         ` Marco Felsch
2025-05-26 14:38 ` [PATCH 8/9] ARM: rockchip: pass device tree to TF-A Michael Tretter
2025-05-26 16:37   ` Marco Felsch
2025-05-27  8:03     ` Michael Tretter
2025-05-26 14:38 ` [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A Michael Tretter
2025-05-26 17:25   ` Marco Felsch
2025-05-27  8:16     ` Michael Tretter
2025-05-27  9:40       ` Marco Felsch
2025-05-27 10:19         ` Michael Tretter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox