Hi Alexey,
On 9/18/26 12:52 PM, Alexey Charkov wrote:
The dtsi infers how to compress from whichever SPL decompressor happens
to be enabled, so there is no way to ask for one in particular, turning
a decompressor on for something else silently changes the image, and the
conditional has to be updated with every new algorithm.
Thanks, this was something of an eye-sore to me for a while already :)
Move the selection to Kconfig instead, with proper help texts and support
for all available compression algorithms. Each choice enables the
decompressor that provides it, and carries the compression level.
Signed-off-by: Alexey Charkov <[email protected]>
---
arch/arm/dts/rockchip-u-boot.dtsi | 16 +++-----
arch/arm/mach-rockchip/Kconfig | 78 +++++++++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+), 10 deletions(-)
diff --git a/arch/arm/dts/rockchip-u-boot.dtsi
b/arch/arm/dts/rockchip-u-boot.dtsi
index 4ba6a87e78ab..a72b55cec05a 100644
--- a/arch/arm/dts/rockchip-u-boot.dtsi
+++ b/arch/arm/dts/rockchip-u-boot.dtsi
@@ -11,14 +11,6 @@
#define FIT_ARCH "arm"
#endif
-#if defined(CONFIG_SPL_GZIP)
-#define FIT_UBOOT_COMP "gzip"
-#elif defined(CONFIG_SPL_LZMA)
-#define FIT_UBOOT_COMP "lzma"
-#else
-#define FIT_UBOOT_COMP "none"
-#endif
-
/*
* SHA256 should be enabled in SPL when signature validation is involved,
* CRC32 should only be used for basic checksum validation of FIT images.
@@ -61,11 +53,15 @@
type = "standalone";
os = "u-boot";
arch = FIT_ARCH;
- compression = FIT_UBOOT_COMP;
+ compression = CONFIG_ROCKCHIP_UBOOT_COMP;
load = <CONFIG_TEXT_BASE>;
entry = <CONFIG_TEXT_BASE>;
u-boot-nodtb {
- compress = FIT_UBOOT_COMP;
+ compress = CONFIG_ROCKCHIP_UBOOT_COMP;
+#if CONFIG_ROCKCHIP_UBOOT_COMP_LEVEL != 0
+ compress-level =
+
<CONFIG_ROCKCHIP_UBOOT_COMP_LEVEL>;
+#endif
OK so the obvious question here is "why Rockchip-specific?". I
appreciate you're mostly working with the RK3576 but I don't see why
this (the kconfig symbol definition that happens later in this patch in
arch/arm/mach-rockchip/Kconfig) needs to be Rockchip-specific, we have
other architectures that are also building a FIT image with U-Boot
proper in it, so I think it'd be nice to have this setting apply to all
of them so that it's a more generic approach. I'm wondering whether a
direct argument to binman would be more appropriate here instead of
requiring all binman nodes to not forget to add these compression
properties for the setting to actually apply. Of course this would still
need some logic handling a binman argument and the compression
properties both being set, and likely remove the properties from
existing DTS.
};
#ifdef FIT_HASH_ALGO
hash {
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index 1a2e7847c9e7..6e98534f28d7 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -758,6 +758,84 @@ config ROCKCHIP_MASKROM_IMAGE
Rockchip SoCs support maskrom mode boot over USB. Enable this
option to produce maskrom mode boot images containing U-Boot.
+choice
+ prompt "Compression for U-Boot proper in the SPL FIT"
It's not the SPL FIT.
+ depends on SPL_FIT
This only applies when we build a FIT image with binman for Rockchip,
which happens only if HAS_FIT is defined, which is:
defined(CONFIG_SPL_FIT) && (defined(CONFIG_ARM64) ||
defined(CONFIG_SPL_OPTEE_IMAGE))
so you are missing:
depends on ARM64 || SPL_OPTEE_IMAGE
here.
+ default ROCKCHIP_UBOOT_COMP_NONE
This diverges from the current state, instead do:
default ROCKCHIP_UBOOT_COMP_GZIP if SPL_GZIP
default ROCKCHIP_UBOOT_COMP_LZMA if SPL_LZMA
default ROCKCHIP_UBOOT_COMP_NONE
to keep the same behavior (not tested but I believe this should work).
+ help
+ Pick how U-Boot proper is stored in the FIT which SPL loads it from.
Don't like "how [it] is stored in the fit", I would prefer to make it
explicit we're talking about compression here.
Maybe:
Compression algorithm for U-Boot proper as stored in the FIT loaded by SPL.
?
+
+ It is around a megabyte, so what SPL spends unpacking it is close to
Size depends on what you enable so I don't think it's relevant to put
this in a help text.
+ what it saves reading a smaller image, and leaving it alone is a
+ reasonable answer. Compress it where the room it takes up matters
+ more than the time.
+
I think what matters is the following:
- on space-constrained systems, enabling compression may help making
sure the bootloader fits in the storage medium,
- compression could help with boot time if the storage medium is slow
enough that reading a compressed image from it and uncompressing it is
faster than reading the uncompressed image directly. Whether it's
beneficial needs to be experimentally checked on each device as it
depends on the SoC and selected storage medium.
+config ROCKCHIP_UBOOT_COMP_NONE
+ bool "none"
+ help
+ Store it as it is. SPL copies it into place rather than unpacking
+ it, and the build needs no host tool.
+
Please add a proper help text that is understandable on its own. It is
not uncommon (I do it more often than not) that people will look up a
symbol with the search function in menuconfig and will only get this
information, it's therefore important to know what we're talking about
here without having to look up the choice help text. Ditto for other
help texts below.
+config ROCKCHIP_UBOOT_COMP_GZIP
+ bool "gzip"
+ select SPL_GZIP
+ help
+ Compress with gzip, run from the host 'gzip' binary. A middling
+ ratio, quick to unpack, and it needs no SPL heap.
+
+config ROCKCHIP_UBOOT_COMP_LZMA
+ bool "lzma"
+ select SPL_LZMA
+ help
+ Compress with LZMA, run from the host 'lzma_alone' binary. The best
I'm not sure we need to specify the implementation detail of the tool
used to generate the compressed binary? lzma_alone is also only
available on Debian-based systems (e.g. Fedora doesn't have it), so I'm
not entirely sure whether it'll stay this way forever (maybe we should
switch to another implementation that is available on more/most
distros?). I understand you probably did this because it's somewhat an
important piece of information to then decide which level to use.
+ ratio on offer, and by far the slowest to unpack.
+
+config ROCKCHIP_UBOOT_COMP_LZO
+ bool "lzo"
+ select SPL_LZO
+ help
+ Compress with LZO, run from the host 'lzop' binary. A poor ratio in
+ exchange for fast decompression.
+
+config ROCKCHIP_UBOOT_COMP_LZ4
+ bool "lz4"
+ select SPL_LZ4
+ help
+ Compress with LZ4, run from the host 'lz4' binary. The fastest to
+ unpack and the worst ratio, so it suits a fast boot medium.
+
+config ROCKCHIP_UBOOT_COMP_ZSTD
+ bool "zstd"
+ select SPL_ZSTD
+ help
+ Compress with Zstandard, run from the host 'zstd' binary. Close to
+ the LZMA ratio for a fraction of the time, but its decompressor
+ wants around 94KB of SPL heap - see SPL_ZSTD.
I don't like specifying numbers that aren't guaranteed to stay at that
value. I understand the appeal to provide some number to give a rough
idea of the size impact, so I don't think it needs to be a blocker,
maybe Tom would have some opinion there, otherwise ignore this :)
+
+endchoice
+
+config ROCKCHIP_UBOOT_COMP
+ string
+ default "gzip" if ROCKCHIP_UBOOT_COMP_GZIP
+ default "lzma" if ROCKCHIP_UBOOT_COMP_LZMA
+ default "lzo" if ROCKCHIP_UBOOT_COMP_LZO
+ default "lz4" if ROCKCHIP_UBOOT_COMP_LZ4
+ default "zstd" if ROCKCHIP_UBOOT_COMP_ZSTD
+ default "none"
+
I'm tempted to say this is not the place for this and rather should be
in rockchip-u-boot.dtsi for example as a series of #ifdef (but if we go
for a generic implementation that applies to other architectures, then
maybe it's fine like this).
+config ROCKCHIP_UBOOT_COMP_LEVEL
+ int "Compression level for U-Boot proper"
+ depends on !ROCKCHIP_UBOOT_COMP_NONE && !ROCKCHIP_UBOOT_COMP_LZMA
+ range 0 9 if ROCKCHIP_UBOOT_COMP_GZIP || ROCKCHIP_UBOOT_COMP_LZO
+ range 0 12 if ROCKCHIP_UBOOT_COMP_LZ4
+ range 0 19 if ROCKCHIP_UBOOT_COMP_ZSTD
+ default 0
+ help
+ How hard to compress, numbered the way the algorithm's own tool
+ numbers it. Leave it at 0 to take the tool's default.
+
+ lzma has no such knob, so it is not offered there.
The option won't be shown anyway and it's clear from the dependencies
that it's not supported so I don't think it makes sense to have it in
the help text, however the commit log would be a very good place to have
this piece of information.
Cheers,
Quentin
