Re: [PATCH v4 02/10] rockchip: binman: Factor out arch and compression

[Jonas Karlman <jonas@kwiboo.se>]

Hi Quentin,

On 2025-04-09 14:27, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 4/9/25 1:56 PM, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 2025-04-09 11:28, Quentin Schulz wrote:
>>> Hi Jonas, Simon,
>>>
>>> On 3/29/25 4:06 PM, Jonas Karlman wrote:
>>>> From: Simon Glass <sjg@chromium.org>
>>>>
>>>> Declare arch and compression at the top of the file to avoid needing
>>>> ifdefs in every usage.
>>>>
>>>> Add a few comments to help with the remaining #ifdefs.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>> Changes in v4:
>>>> - Split from "VBE serial part H: Implement VBE on Rockchip RK3399"
>>>> ---
>>>>    arch/arm/dts/rockchip-u-boot.dtsi | 44 +++++++++++++++----------------
>>>>    1 file changed, 22 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
>>>> index e9ed1d4b5738..2b01dc660562 100644
>>>> --- a/arch/arm/dts/rockchip-u-boot.dtsi
>>>> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
>>>> @@ -5,6 +5,20 @@
>>>>    
>>>>    #include <config.h>
>>>>    
>>>> +#ifdef CONFIG_ARM64
>>>> +#define ARCH	"arm64"
>>>> +#else
>>>> +#define ARCH	"arm"
>>>> +#endif
>>>> +
>>>
>>> I would refrain from using ARCH here as it's something we already use to
>>> specify the architecture to build (e.g. make ARCH=arm64 CROSS_COMPILE=...).
>>>
> 
> Actually you don't need (or even shouldn't?) provide ARCH to the make 
> command, got confused because I'm compiling the kernel right now :)
> 
>>> Maybe FIT_ARCH?
>>
>> sunxi-u-boot.dtsi is also using ARCH so I figured it was also safe here,
>> we can change to FIT_ARCH for a v5.
>>
> 
> Indeed. I would prefer something not clashing with other 
> environment/make variables :)
> 
> [...]
> 
>>>> @@ -84,7 +84,7 @@
>>>>    					fit,operation = "split-elf";
>>>>    					description = "ARM Trusted Firmware";
>>>>    					type = "firmware";
>>>> -					arch = "arm64";
>>>> +					arch = ARCH;
>>>>    					os = "arm-trusted-firmware";
>>>>    					compression = "none";
>>>>    					fit,load;
>>>> @@ -103,7 +103,7 @@
>>>>    					fit,operation = "split-elf";
>>>>    					description = "TEE";
>>>>    					type = "tee";
>>>> -					arch = "arm64";
>>>> +					arch = ARCH;
>>>>    					os = "tee";
>>>>    					compression = "none";
>>>>    					fit,load;
>>>> @@ -119,11 +119,11 @@
>>>>    					};
>>>>    #endif
>>>>    				};
>>>> -#else
>>>> +#else /* !CONFIG_ARM64 */
>>>>    				op-tee {
>>>>    					description = "OP-TEE";
>>>>    					type = "tee";
>>>> -					arch = "arm";
>>>> +					arch = ARCH;
>>>>    					os = "tee";
>>>>    					compression = "none";
>>>>    					load = <(CFG_SYS_SDRAM_BASE + 0x8400000)>;
>>>
>>> Wondering if we couldn't put some of the Aarch32 and Aarch64 OP-TEE OS
>>> node(s) in common?
>>
>> Sounds like a good idea to maybe put op-tee in a template, personally I
>> never use op-tee so typically try to minimize any change/impact related
>> to op-tee.
>>
>> The RK3506 does use op-tee so I may need to dig more into the op-tee
>> parts in a future RK3506 enablement series, initially [1] was enough.
>> Could look more into using a op-tee template in such future series.
>>
>> [1] https://source.denx.de/u-boot/contributors/kwiboo/u-boot/-/commit/3d683f3b717de010fffeece8712373892a599905
>>
> 
> Interesting, is it required for RK3506? Do they do things in secure 
> world and since it's Aarch32, no TF-A loaded by U-Boot?

To my knowledge the vendor OP-TEE blob implement PSCI for cpu core mgmt
and it also does some sort of initialization for OTP.

Without starting OP-TEE before U-Boot proper, reading from OTP only
return 0x00 for each byte read.

So will probably be much easier to just run OP-TEE similar to what some
other Rockchip ARMv7 SoCs does/require.

> 
> You can play with OP-TEE on RK3588 from master, c.f. 
> https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-rockchip/platform_rk3588.c
> 
> I haven't even built it but there's been some work on it since it was 
> merged early December last year, so possibly people are using it.

Thanks, will take a closer look at some point in future :-)

Regards,
Jonas

> 
> Cheers,
> Quentin