Re: [PATCH 9/9] ARM: rockchip: fixup memory in device tree for TF-A

[Michael Tretter <m.tretter@pengutronix.de>]

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