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

[Jonas Karlman]

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 at 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 at chromium.org>
>> Signed-off-by: Jonas Karlman <jonas at 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=...).
> 
> 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.

> 
>> +#if defined(CONFIG_SPL_GZIP)
>> +#define COMP	"gzip"
>> +#elif defined(CONFIG_SPL_LZMA)
>> +#define COMP	"lzma"
>> +#else
>> +#define COMP	"none"
>> +#endif
>> +
> 
> This is specific to the U-Boot proper image node only, maybe we should 
> have this constant named more appropriately, e.g. FIT_PROPER_COMP or 
> something like that?

Sounds good, will change in a v5.

> 
> Reading the symbols also, I believe their help text is misleading or 
> confusing (at least the _GZIP ones, _ZSTD seems okay).
> 
> Also, I'm wondering if it actually makes sense to force a specific 
> U-Boot proper compression algorithm based on a symbol of what 
> decompression algorithms are supported in U-Boot SPL. Like, why should 
> gzip be picked if I also have lzma enabled? In any case, nothing related 
> to this patch.

Agree, use of compressed image should probably be enabled using a
dedicated Kconfig symbol, something for a different series :-)

> 
>>   / {
>>   	binman: binman {
>>   		multiple-images;
>> @@ -51,26 +65,12 @@
>>   					description = "U-Boot";
>>   					type = "standalone";
>>   					os = "u-boot";
>> -#ifdef CONFIG_ARM64
>> -					arch = "arm64";
>> -#else
>> -					arch = "arm";
>> -#endif
>> -#if defined(CONFIG_SPL_GZIP)
>> -					compression = "gzip";
>> -#elif defined(CONFIG_SPL_LZMA)
>> -					compression = "lzma";
>> -#else
>> -					compression = "none";
>> -#endif
>> +					arch = ARCH;
>> +					compression = COMP;
>>   					load = <CONFIG_TEXT_BASE>;
>>   					entry = <CONFIG_TEXT_BASE>;
>>   					u-boot-nodtb {
>> -#if defined(CONFIG_SPL_GZIP)
>> -					compress = "gzip";
>> -#elif defined(CONFIG_SPL_LZMA)
>> -					compress = "lzma";
>> -#endif
>> +					compress = COMP;
>>   					};
>>   #ifdef CONFIG_SPL_FIT_SIGNATURE
>>   					hash {
>> @@ -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

> 
> The change is fine, finding a more appropriate name for COMP and ARCH 
> and it's all good.

I will use different names in a v5.

Regards,
Jonas

> 
> Cheers,
> Quentin