| Message ID | 20250130181005.6319-1-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: remove rk3588 optee node | expand |
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
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
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
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
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
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
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>;