[PATCH] arm64: dts: rockchip: remove rk3588 optee node

[PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Remove Optee node from rk3588 devicetree. When Optee is present and
used the node will be added automatically by U-Boot when
CONFIG_OPTEE_LIB=y and CONFIG_SPL_ATF_NO_PLATFORM_PARAM is not set.
When Optee is not present or used, the node will trigger a probe
that generates a (harmless) message on the kernel log.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index 8cfa30837ce7..245a6cabdc8e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -358,11 +358,6 @@ display_subsystem: display-subsystem {
 	};
 
 	firmware {
-		optee: optee {
-			compatible = "linaro,optee-tz";
-			method = "smc";
-		};
-
 		scmi: scmi {
 			compatible = "arm,scmi-smc";
 			arm,smc-id = <0x82000010>;
-- 
2.43.0

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Quentin Schulz]

Hi Chris,

On 1/30/25 7:10 PM, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Remove Optee node from rk3588 devicetree. When Optee is present and
> used the node will be added automatically by U-Boot when
> CONFIG_OPTEE_LIB=y and CONFIG_SPL_ATF_NO_PLATFORM_PARAM is not set.

This is too big to ask for right now, 100% of the RK3588 products in 
upstream U-Boot have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set.

Does it hurt to keep it forever?

> When Optee is not present or used, the node will trigger a probe
> that generates a (harmless) message on the kernel log.
> 

And what if we have OP-TEE without this node present, which would be 
possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?

I guess we could detect from U-Boot if a TEE is loaded as part of 
u-boot.itb and fixup the DTB otherwise to mark this node as status = 
"disabled"?

Cheers,
Quentin

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Chris Morgan]

On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
> Hi Chris,
> 
> On 1/30/25 7:10 PM, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan@hotmail.com>
> > 
> > Remove Optee node from rk3588 devicetree. When Optee is present and
> > used the node will be added automatically by U-Boot when
> > CONFIG_OPTEE_LIB=y and CONFIG_SPL_ATF_NO_PLATFORM_PARAM is not set.
> 
> This is too big to ask for right now, 100% of the RK3588 products in
> upstream U-Boot have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set.

You don't have to change it if you don't want to. But if you want the
devicetree to pass through from A-TF to Optee back to U-Boot, you do.
I wasn't even planning to make this the default change for the board
on which I'm doing this work (the Indiedroid Nova), but I was going
to update the documentation to note these steps.

> 
> Does it hurt to keep it forever?

CONFIG_SPL_ATF_NO_PLATFORM_PARAM=y is a perfectly fine default,
especially if you plan on using the BSP A-TF binary.

> 
> > When Optee is not present or used, the node will trigger a probe
> > that generates a (harmless) message on the kernel log.
> > 
> 
> And what if we have OP-TEE without this node present, which would be
> possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
> 
> I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
> and fixup the DTB otherwise to mark this node as status = "disabled"?
> 

Based on my understanding of how each of these projects communicate
with each other, Optee can only work if you either include both the
Optee node in the firmware AND the reserved memory nodes yourself, or
if you have neither and let U-Boot do it (by including each of these
patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
basically just this node alone is insufficient for it to work today.

The core issue is that Optee requires you to map the memory as
reserved so that Linux doesn't try to use it. You can either do that
yourself or you can have U-Boot do it automatically. Having the Optee
node in the devicetree makes no sense today unless we also carve out
the memory.

Thank you,
Chris

> Cheers,
> Quentin

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Quentin Schulz]

Hi Chris,

On 1/31/25 5:59 PM, Chris Morgan wrote:
> On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
>> Hi Chris,
>>
>> On 1/30/25 7:10 PM, Chris Morgan wrote:
[...]
>>> When Optee is not present or used, the node will trigger a probe
>>> that generates a (harmless) message on the kernel log.
>>>
>>
>> And what if we have OP-TEE without this node present, which would be
>> possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
>>
>> I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
>> and fixup the DTB otherwise to mark this node as status = "disabled"?
>>
> 
> Based on my understanding of how each of these projects communicate
> with each other, Optee can only work if you either include both the
> Optee node in the firmware AND the reserved memory nodes yourself, or
> if you have neither and let U-Boot do it (by including each of these
> patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
> basically just this node alone is insufficient for it to work today.
> 

Therefore I think we should either have this node defined + the 
reserved-memory node (with hardcoded address and size if guaranteed to 
be stable forever) added or nothing.

Can we mark a reserved-memory node as "disabled"? If not, then we should 
rather have nothing at all I believe.

> The core issue is that Optee requires you to map the memory as
> reserved so that Linux doesn't try to use it. You can either do that
> yourself or you can have U-Boot do it automatically. Having the Optee
> node in the devicetree makes no sense today unless we also carve out
> the memory.
> 

We should patch U-boot to add these nodes to the DT if an OP-TEE OS is 
passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the 
OP-TEE nodes in the kernel DT. What do you think?

Cheers,
Quentin

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Chris Morgan]

On Mon, Feb 03, 2025 at 05:32:53PM +0100, Quentin Schulz wrote:
> Hi Chris,
> 
> On 1/31/25 5:59 PM, Chris Morgan wrote:
> > On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
> > > Hi Chris,
> > > 
> > > On 1/30/25 7:10 PM, Chris Morgan wrote:
> [...]
> > > > When Optee is not present or used, the node will trigger a probe
> > > > that generates a (harmless) message on the kernel log.
> > > > 
> > > 
> > > And what if we have OP-TEE without this node present, which would be
> > > possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
> > > 
> > > I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
> > > and fixup the DTB otherwise to mark this node as status = "disabled"?
> > > 
> > 
> > Based on my understanding of how each of these projects communicate
> > with each other, Optee can only work if you either include both the
> > Optee node in the firmware AND the reserved memory nodes yourself, or
> > if you have neither and let U-Boot do it (by including each of these
> > patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
> > basically just this node alone is insufficient for it to work today.
> > 
> 
> Therefore I think we should either have this node defined + the
> reserved-memory node (with hardcoded address and size if guaranteed to be
> stable forever) added or nothing.
> 
> Can we mark a reserved-memory node as "disabled"? If not, then we should
> rather have nothing at all I believe.

I honestly would just rather remove this node. The reason I say that is
while we support Optee with the RK3399, RK3228, the PX30, and the
RK3588; howver only the RK3588 has the node already populated in Linux.

> 
> > The core issue is that Optee requires you to map the memory as
> > reserved so that Linux doesn't try to use it. You can either do that
> > yourself or you can have U-Boot do it automatically. Having the Optee
> > node in the devicetree makes no sense today unless we also carve out
> > the memory.
> > 
> 
> We should patch U-boot to add these nodes to the DT if an OP-TEE OS is
> passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the OP-TEE
> nodes in the kernel DT. What do you think?
> 

We would have to assume some hard coded values in that event as I'm not
aware of a mechanism to grab them at runtime from Optee except when you
pass it a device tree. That said I think the concern above where you
mention "guaranteed to be stable forever" is the problem, as even now
the current address in the Optee upstream project conflicts with the
kernel_comp_addr_r in upstream U-Boot, necessesitating one of them be
changed (I'm attempting to change the Optee one, for what it's worth).

Thank you,
Chris

> Cheers,
> Quentin

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Quentin Schulz]

Hi Chris,

On 2/3/25 10:27 PM, Chris Morgan wrote:
> On Mon, Feb 03, 2025 at 05:32:53PM +0100, Quentin Schulz wrote:
>> Hi Chris,
>>
>> On 1/31/25 5:59 PM, Chris Morgan wrote:
>>> On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
>>>> Hi Chris,
>>>>
>>>> On 1/30/25 7:10 PM, Chris Morgan wrote:
>> [...]
>>>>> When Optee is not present or used, the node will trigger a probe
>>>>> that generates a (harmless) message on the kernel log.
>>>>>
>>>>
>>>> And what if we have OP-TEE without this node present, which would be
>>>> possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
>>>>
>>>> I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
>>>> and fixup the DTB otherwise to mark this node as status = "disabled"?
>>>>
>>>
>>> Based on my understanding of how each of these projects communicate
>>> with each other, Optee can only work if you either include both the
>>> Optee node in the firmware AND the reserved memory nodes yourself, or
>>> if you have neither and let U-Boot do it (by including each of these
>>> patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
>>> basically just this node alone is insufficient for it to work today.
>>>
>>
>> Therefore I think we should either have this node defined + the
>> reserved-memory node (with hardcoded address and size if guaranteed to be
>> stable forever) added or nothing.
>>
>> Can we mark a reserved-memory node as "disabled"? If not, then we should
>> rather have nothing at all I believe.
> 
> I honestly would just rather remove this node. The reason I say that is
> while we support Optee with the RK3399, RK3228, the PX30, and the
> RK3588; howver only the RK3588 has the node already populated in Linux.
> 

And we have a product based on PX30 that has OP-TEE OS running without a 
hardcoded node in the DTS, so that's a valid point of comparison to me. 
That seems to justify the deletion to me!

>>
>>> The core issue is that Optee requires you to map the memory as
>>> reserved so that Linux doesn't try to use it. You can either do that
>>> yourself or you can have U-Boot do it automatically. Having the Optee
>>> node in the devicetree makes no sense today unless we also carve out
>>> the memory.
>>>
>>
>> We should patch U-boot to add these nodes to the DT if an OP-TEE OS is
>> passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the OP-TEE
>> nodes in the kernel DT. What do you think?
>>
> 
> We would have to assume some hard coded values in that event as I'm not
> aware of a mechanism to grab them at runtime from Optee except when you
> pass it a device tree. That said I think the concern above where you

Yes, but that would be the same as BL31 for example and an expected side 
effect of using CONFIG_SPL_ATF_NO_PLATFORM_PARAM except if there's a way 
to provide information back from TEE to U-Boot without using the params 
that would be passed by U-Boot to TF-A had we 
CONFIG_SPL_ATF_NO_PLATFORM_PARAM disabled.

> mention "guaranteed to be stable forever" is the problem, as even now
> the current address in the Optee upstream project conflicts with the
> kernel_comp_addr_r in upstream U-Boot, necessesitating one of them be
> changed (I'm attempting to change the Optee one, for what it's worth).
> 

I think it makes more sense to change the load addresses in U-Boot, 
especially since those are just the default values for variables which 
are configurable per board type, per board or even per boot, so it's 
something one would be able to modify without necessarily rebuilding 
anything. Essentially, it's easier to move that around than checking 
which OP-TEE OS version one is booting before providing advice on where 
to load the image? Up to you though, no strong opinion there.

Cheers,
Quentin

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Chris Morgan]

On Tue, Feb 04, 2025 at 10:12:26AM +0100, Quentin Schulz wrote:
> Hi Chris,
> 
> On 2/3/25 10:27 PM, Chris Morgan wrote:
> > On Mon, Feb 03, 2025 at 05:32:53PM +0100, Quentin Schulz wrote:
> > > Hi Chris,
> > > 
> > > On 1/31/25 5:59 PM, Chris Morgan wrote:
> > > > On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
> > > > > Hi Chris,
> > > > > 
> > > > > On 1/30/25 7:10 PM, Chris Morgan wrote:
> > > [...]
> > > > > > When Optee is not present or used, the node will trigger a probe
> > > > > > that generates a (harmless) message on the kernel log.
> > > > > > 
> > > > > 
> > > > > And what if we have OP-TEE without this node present, which would be
> > > > > possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
> > > > > 
> > > > > I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
> > > > > and fixup the DTB otherwise to mark this node as status = "disabled"?
> > > > > 
> > > > 
> > > > Based on my understanding of how each of these projects communicate
> > > > with each other, Optee can only work if you either include both the
> > > > Optee node in the firmware AND the reserved memory nodes yourself, or
> > > > if you have neither and let U-Boot do it (by including each of these
> > > > patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
> > > > basically just this node alone is insufficient for it to work today.
> > > > 
> > > 
> > > Therefore I think we should either have this node defined + the
> > > reserved-memory node (with hardcoded address and size if guaranteed to be
> > > stable forever) added or nothing.
> > > 
> > > Can we mark a reserved-memory node as "disabled"? If not, then we should
> > > rather have nothing at all I believe.
> > 
> > I honestly would just rather remove this node. The reason I say that is
> > while we support Optee with the RK3399, RK3228, the PX30, and the
> > RK3588; howver only the RK3588 has the node already populated in Linux.
> > 
> 
> And we have a product based on PX30 that has OP-TEE OS running without a
> hardcoded node in the DTS, so that's a valid point of comparison to me. That
> seems to justify the deletion to me!
> 
> > > 
> > > > The core issue is that Optee requires you to map the memory as
> > > > reserved so that Linux doesn't try to use it. You can either do that
> > > > yourself or you can have U-Boot do it automatically. Having the Optee
> > > > node in the devicetree makes no sense today unless we also carve out
> > > > the memory.
> > > > 
> > > 
> > > We should patch U-boot to add these nodes to the DT if an OP-TEE OS is
> > > passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the OP-TEE
> > > nodes in the kernel DT. What do you think?
> > > 
> > 
> > We would have to assume some hard coded values in that event as I'm not
> > aware of a mechanism to grab them at runtime from Optee except when you
> > pass it a device tree. That said I think the concern above where you
> 
> Yes, but that would be the same as BL31 for example and an expected side
> effect of using CONFIG_SPL_ATF_NO_PLATFORM_PARAM except if there's a way to
> provide information back from TEE to U-Boot without using the params that
> would be passed by U-Boot to TF-A had we CONFIG_SPL_ATF_NO_PLATFORM_PARAM
> disabled.
> 
> > mention "guaranteed to be stable forever" is the problem, as even now
> > the current address in the Optee upstream project conflicts with the
> > kernel_comp_addr_r in upstream U-Boot, necessesitating one of them be
> > changed (I'm attempting to change the Optee one, for what it's worth).
> > 
> 
> I think it makes more sense to change the load addresses in U-Boot,
> especially since those are just the default values for variables which are
> configurable per board type, per board or even per boot, so it's something
> one would be able to modify without necessarily rebuilding anything.
> Essentially, it's easier to move that around than checking which OP-TEE OS
> version one is booting before providing advice on where to load the image?
> Up to you though, no strong opinion there.

I think I mentioned this but in IRC but the consensus was to change Optee
to match the same addresses as the PX30 and RK3399. No strong opinion from
me either, just trying to get it working without stepping on toes anywhere.

Thank you,
Chris

> 
> Cheers,
> Quentin

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Heiko Stübner]

Hi,

Am Dienstag, 4. Februar 2025, 17:16:22 MEZ schrieb Chris Morgan:
> On Tue, Feb 04, 2025 at 10:12:26AM +0100, Quentin Schulz wrote:
> > Hi Chris,
> > 
> > On 2/3/25 10:27 PM, Chris Morgan wrote:
> > > On Mon, Feb 03, 2025 at 05:32:53PM +0100, Quentin Schulz wrote:
> > > > Hi Chris,
> > > > 
> > > > On 1/31/25 5:59 PM, Chris Morgan wrote:
> > > > > On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
> > > > > > Hi Chris,
> > > > > > 
> > > > > > On 1/30/25 7:10 PM, Chris Morgan wrote:
> > > > [...]
> > > > > > > When Optee is not present or used, the node will trigger a probe
> > > > > > > that generates a (harmless) message on the kernel log.
> > > > > > > 
> > > > > > 
> > > > > > And what if we have OP-TEE without this node present, which would be
> > > > > > possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
> > > > > > 
> > > > > > I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
> > > > > > and fixup the DTB otherwise to mark this node as status = "disabled"?
> > > > > > 
> > > > > 
> > > > > Based on my understanding of how each of these projects communicate
> > > > > with each other, Optee can only work if you either include both the
> > > > > Optee node in the firmware AND the reserved memory nodes yourself, or
> > > > > if you have neither and let U-Boot do it (by including each of these
> > > > > patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
> > > > > basically just this node alone is insufficient for it to work today.
> > > > > 
> > > > 
> > > > Therefore I think we should either have this node defined + the
> > > > reserved-memory node (with hardcoded address and size if guaranteed to be
> > > > stable forever) added or nothing.
> > > > 
> > > > Can we mark a reserved-memory node as "disabled"? If not, then we should
> > > > rather have nothing at all I believe.
> > > 
> > > I honestly would just rather remove this node. The reason I say that is
> > > while we support Optee with the RK3399, RK3228, the PX30, and the
> > > RK3588; howver only the RK3588 has the node already populated in Linux.
> > > 
> > 
> > And we have a product based on PX30 that has OP-TEE OS running without a
> > hardcoded node in the DTS, so that's a valid point of comparison to me. That
> > seems to justify the deletion to me!
> > 
> > > > 
> > > > > The core issue is that Optee requires you to map the memory as
> > > > > reserved so that Linux doesn't try to use it. You can either do that
> > > > > yourself or you can have U-Boot do it automatically. Having the Optee
> > > > > node in the devicetree makes no sense today unless we also carve out
> > > > > the memory.
> > > > > 
> > > > 
> > > > We should patch U-boot to add these nodes to the DT if an OP-TEE OS is
> > > > passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the OP-TEE
> > > > nodes in the kernel DT. What do you think?
> > > > 
> > > 
> > > We would have to assume some hard coded values in that event as I'm not
> > > aware of a mechanism to grab them at runtime from Optee except when you
> > > pass it a device tree. That said I think the concern above where you
> > 
> > Yes, but that would be the same as BL31 for example and an expected side
> > effect of using CONFIG_SPL_ATF_NO_PLATFORM_PARAM except if there's a way to
> > provide information back from TEE to U-Boot without using the params that
> > would be passed by U-Boot to TF-A had we CONFIG_SPL_ATF_NO_PLATFORM_PARAM
> > disabled.
> > 
> > > mention "guaranteed to be stable forever" is the problem, as even now
> > > the current address in the Optee upstream project conflicts with the
> > > kernel_comp_addr_r in upstream U-Boot, necessesitating one of them be
> > > changed (I'm attempting to change the Optee one, for what it's worth).
> > > 
> > 
> > I think it makes more sense to change the load addresses in U-Boot,
> > especially since those are just the default values for variables which are
> > configurable per board type, per board or even per boot, so it's something
> > one would be able to modify without necessarily rebuilding anything.
> > Essentially, it's easier to move that around than checking which OP-TEE OS
> > version one is booting before providing advice on where to load the image?
> > Up to you though, no strong opinion there.
> 
> I think I mentioned this but in IRC but the consensus was to change Optee
> to match the same addresses as the PX30 and RK3399. No strong opinion from
> me either, just trying to get it working without stepping on toes anywhere.

glancing through the thread, did you come to a conclusion whether to drop
or keep the optee node?

I.e. from what I see, having this default optee node _without_ the needed
memory reservation would mean that _if_ an optee runs, the kernel would
either possibly write over its memory, or we'd end up in an exception thing
because the kernel would write over firewall'd memory.

So at this point, having the optee node here, makes no real sense to me,
as firmware that would need to add reserved memory to the dt, could also
just add the optee node in the same run - as we do on other socs already?


Heiko

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Chris Morgan]

On Mon, Feb 10, 2025 at 11:20:27AM +0100, Heiko Stübner wrote:
> Hi,
> 
> Am Dienstag, 4. Februar 2025, 17:16:22 MEZ schrieb Chris Morgan:
> > On Tue, Feb 04, 2025 at 10:12:26AM +0100, Quentin Schulz wrote:
> > > Hi Chris,
> > > 
> > > On 2/3/25 10:27 PM, Chris Morgan wrote:
> > > > On Mon, Feb 03, 2025 at 05:32:53PM +0100, Quentin Schulz wrote:
> > > > > Hi Chris,
> > > > > 
> > > > > On 1/31/25 5:59 PM, Chris Morgan wrote:
> > > > > > On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
> > > > > > > Hi Chris,
> > > > > > > 
> > > > > > > On 1/30/25 7:10 PM, Chris Morgan wrote:
> > > > > [...]
> > > > > > > > When Optee is not present or used, the node will trigger a probe
> > > > > > > > that generates a (harmless) message on the kernel log.
> > > > > > > > 
> > > > > > > 
> > > > > > > And what if we have OP-TEE without this node present, which would be
> > > > > > > possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
> > > > > > > 
> > > > > > > I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
> > > > > > > and fixup the DTB otherwise to mark this node as status = "disabled"?
> > > > > > > 
> > > > > > 
> > > > > > Based on my understanding of how each of these projects communicate
> > > > > > with each other, Optee can only work if you either include both the
> > > > > > Optee node in the firmware AND the reserved memory nodes yourself, or
> > > > > > if you have neither and let U-Boot do it (by including each of these
> > > > > > patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
> > > > > > basically just this node alone is insufficient for it to work today.
> > > > > > 
> > > > > 
> > > > > Therefore I think we should either have this node defined + the
> > > > > reserved-memory node (with hardcoded address and size if guaranteed to be
> > > > > stable forever) added or nothing.
> > > > > 
> > > > > Can we mark a reserved-memory node as "disabled"? If not, then we should
> > > > > rather have nothing at all I believe.
> > > > 
> > > > I honestly would just rather remove this node. The reason I say that is
> > > > while we support Optee with the RK3399, RK3228, the PX30, and the
> > > > RK3588; howver only the RK3588 has the node already populated in Linux.
> > > > 
> > > 
> > > And we have a product based on PX30 that has OP-TEE OS running without a
> > > hardcoded node in the DTS, so that's a valid point of comparison to me. That
> > > seems to justify the deletion to me!
> > > 
> > > > > 
> > > > > > The core issue is that Optee requires you to map the memory as
> > > > > > reserved so that Linux doesn't try to use it. You can either do that
> > > > > > yourself or you can have U-Boot do it automatically. Having the Optee
> > > > > > node in the devicetree makes no sense today unless we also carve out
> > > > > > the memory.
> > > > > > 
> > > > > 
> > > > > We should patch U-boot to add these nodes to the DT if an OP-TEE OS is
> > > > > passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the OP-TEE
> > > > > nodes in the kernel DT. What do you think?
> > > > > 
> > > > 
> > > > We would have to assume some hard coded values in that event as I'm not
> > > > aware of a mechanism to grab them at runtime from Optee except when you
> > > > pass it a device tree. That said I think the concern above where you
> > > 
> > > Yes, but that would be the same as BL31 for example and an expected side
> > > effect of using CONFIG_SPL_ATF_NO_PLATFORM_PARAM except if there's a way to
> > > provide information back from TEE to U-Boot without using the params that
> > > would be passed by U-Boot to TF-A had we CONFIG_SPL_ATF_NO_PLATFORM_PARAM
> > > disabled.
> > > 
> > > > mention "guaranteed to be stable forever" is the problem, as even now
> > > > the current address in the Optee upstream project conflicts with the
> > > > kernel_comp_addr_r in upstream U-Boot, necessesitating one of them be
> > > > changed (I'm attempting to change the Optee one, for what it's worth).
> > > > 
> > > 
> > > I think it makes more sense to change the load addresses in U-Boot,
> > > especially since those are just the default values for variables which are
> > > configurable per board type, per board or even per boot, so it's something
> > > one would be able to modify without necessarily rebuilding anything.
> > > Essentially, it's easier to move that around than checking which OP-TEE OS
> > > version one is booting before providing advice on where to load the image?
> > > Up to you though, no strong opinion there.
> > 
> > I think I mentioned this but in IRC but the consensus was to change Optee
> > to match the same addresses as the PX30 and RK3399. No strong opinion from
> > me either, just trying to get it working without stepping on toes anywhere.
> 
> glancing through the thread, did you come to a conclusion whether to drop
> or keep the optee node?
> 
> I.e. from what I see, having this default optee node _without_ the needed
> memory reservation would mean that _if_ an optee runs, the kernel would
> either possibly write over its memory, or we'd end up in an exception thing
> because the kernel would write over firewall'd memory.
> 
> So at this point, having the optee node here, makes no real sense to me,
> as firmware that would need to add reserved memory to the dt, could also
> just add the optee node in the same run - as we do on other socs already?
> 
> 
> Heiko
> 
> 

I think if I am hearing correctly we decided to drop this node.
In truth whether or not we have the OP-TEE node if OP-TEE is running
and there is no memory defined we'll get an exception when the kernel
tries to overwrite firewalled memory.

Like you said, what makes the most sense (to me) is to just do what we
do for other SoCs and that is to drop this node and rely on the
bootloader to add both it and the correct memory reservations when
OP-TEE is in use. U-Boot will already do that as long as certain
conditions are met which I'm attempting to get OP-TEE and Arm Trusted
Firmware to do; namely increasing the FDT buffer size and updating the
OP-TEE shared memory locations to match the RK3399 and PX30. I owe
U-Boot some proper documentation on that front...

Thank you,
Chris

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Quentin Schulz]

Hi Heiko, Chris,

On 2/10/25 6:06 PM, Chris Morgan wrote:
> On Mon, Feb 10, 2025 at 11:20:27AM +0100, Heiko Stübner wrote:
>> Hi,
>>
>> Am Dienstag, 4. Februar 2025, 17:16:22 MEZ schrieb Chris Morgan:
>>> On Tue, Feb 04, 2025 at 10:12:26AM +0100, Quentin Schulz wrote:
>>>> Hi Chris,
>>>>
>>>> On 2/3/25 10:27 PM, Chris Morgan wrote:
>>>>> On Mon, Feb 03, 2025 at 05:32:53PM +0100, Quentin Schulz wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> On 1/31/25 5:59 PM, Chris Morgan wrote:
>>>>>>> On Fri, Jan 31, 2025 at 05:46:20PM +0100, Quentin Schulz wrote:
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> On 1/30/25 7:10 PM, Chris Morgan wrote:
>>>>>> [...]
>>>>>>>>> When Optee is not present or used, the node will trigger a probe
>>>>>>>>> that generates a (harmless) message on the kernel log.
>>>>>>>>>
>>>>>>>>
>>>>>>>> And what if we have OP-TEE without this node present, which would be
>>>>>>>> possible if we have CONFIG_SPL_ATF_NO_PLATFORM_PARAM set in U-Boot?
>>>>>>>>
>>>>>>>> I guess we could detect from U-Boot if a TEE is loaded as part of u-boot.itb
>>>>>>>> and fixup the DTB otherwise to mark this node as status = "disabled"?
>>>>>>>>
>>>>>>>
>>>>>>> Based on my understanding of how each of these projects communicate
>>>>>>> with each other, Optee can only work if you either include both the
>>>>>>> Optee node in the firmware AND the reserved memory nodes yourself, or
>>>>>>> if you have neither and let U-Boot do it (by including each of these
>>>>>>> patches as well as setting the CONFIG_SPL_ATF_NO_PLATFORM_PARAM). So
>>>>>>> basically just this node alone is insufficient for it to work today.
>>>>>>>
>>>>>>
>>>>>> Therefore I think we should either have this node defined + the
>>>>>> reserved-memory node (with hardcoded address and size if guaranteed to be
>>>>>> stable forever) added or nothing.
>>>>>>
>>>>>> Can we mark a reserved-memory node as "disabled"? If not, then we should
>>>>>> rather have nothing at all I believe.
>>>>>
>>>>> I honestly would just rather remove this node. The reason I say that is
>>>>> while we support Optee with the RK3399, RK3228, the PX30, and the
>>>>> RK3588; howver only the RK3588 has the node already populated in Linux.
>>>>>
>>>>
>>>> And we have a product based on PX30 that has OP-TEE OS running without a
>>>> hardcoded node in the DTS, so that's a valid point of comparison to me. That
>>>> seems to justify the deletion to me!
>>>>
>>>>>>
>>>>>>> The core issue is that Optee requires you to map the memory as
>>>>>>> reserved so that Linux doesn't try to use it. You can either do that
>>>>>>> yourself or you can have U-Boot do it automatically. Having the Optee
>>>>>>> node in the devicetree makes no sense today unless we also carve out
>>>>>>> the memory.
>>>>>>>
>>>>>>
>>>>>> We should patch U-boot to add these nodes to the DT if an OP-TEE OS is
>>>>>> passed and either SPL_ATF_NO_PLATFORM_PARAM=y or we cannot detect the OP-TEE
>>>>>> nodes in the kernel DT. What do you think?
>>>>>>
>>>>>
>>>>> We would have to assume some hard coded values in that event as I'm not
>>>>> aware of a mechanism to grab them at runtime from Optee except when you
>>>>> pass it a device tree. That said I think the concern above where you
>>>>
>>>> Yes, but that would be the same as BL31 for example and an expected side
>>>> effect of using CONFIG_SPL_ATF_NO_PLATFORM_PARAM except if there's a way to
>>>> provide information back from TEE to U-Boot without using the params that
>>>> would be passed by U-Boot to TF-A had we CONFIG_SPL_ATF_NO_PLATFORM_PARAM
>>>> disabled.
>>>>
>>>>> mention "guaranteed to be stable forever" is the problem, as even now
>>>>> the current address in the Optee upstream project conflicts with the
>>>>> kernel_comp_addr_r in upstream U-Boot, necessesitating one of them be
>>>>> changed (I'm attempting to change the Optee one, for what it's worth).
>>>>>
>>>>
>>>> I think it makes more sense to change the load addresses in U-Boot,
>>>> especially since those are just the default values for variables which are
>>>> configurable per board type, per board or even per boot, so it's something
>>>> one would be able to modify without necessarily rebuilding anything.
>>>> Essentially, it's easier to move that around than checking which OP-TEE OS
>>>> version one is booting before providing advice on where to load the image?
>>>> Up to you though, no strong opinion there.
>>>
>>> I think I mentioned this but in IRC but the consensus was to change Optee
>>> to match the same addresses as the PX30 and RK3399. No strong opinion from
>>> me either, just trying to get it working without stepping on toes anywhere.
>>
>> glancing through the thread, did you come to a conclusion whether to drop
>> or keep the optee node?
>>
>> I.e. from what I see, having this default optee node _without_ the needed
>> memory reservation would mean that _if_ an optee runs, the kernel would
>> either possibly write over its memory, or we'd end up in an exception thing
>> because the kernel would write over firewall'd memory.
>>
>> So at this point, having the optee node here, makes no real sense to me,
>> as firmware that would need to add reserved memory to the dt, could also
>> just add the optee node in the same run - as we do on other socs already?
>>
>>
>> Heiko
>>
>>
> 
> I think if I am hearing correctly we decided to drop this node.

Agreed.

Thanks!
Quentin

Re: [PATCH] arm64: dts: rockchip: remove rk3588 optee node

[Heiko Stuebner]

On Thu, 30 Jan 2025 12:10:04 -0600, Chris Morgan wrote:
> Remove Optee node from rk3588 devicetree. When Optee is present and
> used the node will be added automatically by U-Boot when
> CONFIG_OPTEE_LIB=y and CONFIG_SPL_ATF_NO_PLATFORM_PARAM is not set.
> When Optee is not present or used, the node will trigger a probe
> that generates a (harmless) message on the kernel log.
> 
> 
> [...]

Applied, thanks!

[1/1] arm64: dts: rockchip: remove rk3588 optee node
      commit: b3dc2a9315c4046b330a784c0527c671fd236414

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>