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

[Quentin Schulz <quentin.schulz@cherry.de>]

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=...).

Maybe FIT_ARCH?

> +#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?

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.

>   / {
>   	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?

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

Cheers,
Quentin