Hi Alexey,
On 9/18/26 9:12 PM, Alexey Charkov wrote:
Hi Quentin,
On Fri, Sep 18, 2026 at 9:57 PM Quentin Schulz <[email protected]> wrote:
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.
The Kconfig selection itself is surely arch-agnostic, but I have no
idea if the handling of the resulting images will work out fine on
other architectures or what their constraints are, beyond what the
build system can tell me. Rockchip is the only one I can test, so I'd
rather start here :)
If it's the same architecture(s) as Rockchip, I would assume that if the
SPL loads a FIT image, it can load compressed images too and uncompress
them on the fly as I don't see why anything would be different here (but
maybe I'm wrong :) ).
I understand getting cold feet on enabling this U-Boot-wide but it would
be nice to notify other architectures that this is now (well, will be)
available and they can add support for it/test it. I'm thinking we (you)
could do an RFC series that depend on this series here, that enables
compression = CONFIG_FIT_UBOOT_COMP and compress-level =
CONFIG_FIT_UBOOT_COMP_LEVEL for other architectures that use binman and
generate a U-Boot FIT. Then you let the maintainers figure out the rest
whether they want to take it or not (after testing themselves). This RFC
series wouldn't be a blocker for this series, but I really think we
should implement this in a generic way at least on the Kconfig-side so
that other vendors can implement it similarly.
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.
Not sure that's a universally good idea: the FIT needs the compression
attribute anyway, and it comes via the DTS. Passing the compression
argument to binman via the same DTS reduces the chance of the two
going out of sync. The alternative is passing the compression to
binman via a separate back-channel and having it insert the FIT
attributes based on that. To me, that sounds strictly worse than the
current arrangement because it turns explicit settings visible in a
textual config into automagic.
Yes, but at the same time, you get a truly generic solution without
requiring architecture/vendor/board maintainers to implement the same
details in their DTS (reducing boilerplate in essence). I'm not sold on
my own idea anyway so not a blocker for me (and we can anyway decide to
go this route later on).
[...]
+ 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).
That was my first attempt, but it introduced a circular dependency.
Do you still have the dependency loop by any chance? I'm assuming this
is something maybe some architecture/board maintainers will hit when
wanting to default to something else than COMP_NONE?
I've checked the current defconfigs, and none of them enable
compression. Worse, LZMA had been broken and couldn't link for some
confirmed with:
./tools/qconfig.py -b
./tools/qconfig.py -f ARCH_ROCKCHIP SPL_GZIP
./tools/qconfig.py -f ARCH_ROCKCHIP SPL_LZMA
time until I fixed it separately (it's merged now), so I'm reasonably
sure there have been no active users of these settings lately.
We would "break" (change behavior for) existing users which use a
non-upstreamed defconfig, but we generally don't care too much about
those usecases.
Fine with me, but this needs to be documented in the commit log as from
the diff itself one could think this was not done on purpose (and we're
technically changing the behavior even if it doesn't apply to any of the
upstreamed defconfigs).
[...]
+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.
The peculiar host binary which nobody ships these days is precisely
why I decided to mention it by the name. On my main Gentoo system the
legacy lzma container is only provided by the
not-so-easy-to-figure-out lzmacon package, which then requires
symlinking under a different name for the U-Boot build system to pick
it up. With this help text, a user at least knows what weird binary to
google for, or what substitutes might be available on their distro.
Can always be improved later on, so fine with me.
[...]
+
+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).
Noooo - do you really like #ifdefs? :) Having this here means one file
to update when another algorithm gets added, as opposed to multiple,
which I believe is a worthy quality in itself.
People have various different opinions on what can be part of Kconfig
and the above will likely not make some happy. I don't like it, but I
also don't have a strong opinion. Also, if we're going for a more
generic approach, then this might become an issue rather quickly as all
base DTS defining a U-Boot FIT to generate with binman would have to
declare those strings again. Can always be changed later on in any case.
Cheers,
Quentin
