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 :)
> 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.
> > };
> > #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.
Right, thanks. The SPL_FIT symbol made my mind wander :-D
> > + 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.
Ack
> > + 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.
I've checked the current defconfigs, and none of them enable
compression. Worse, LZMA had been broken and couldn't link for some
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.
> > + 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.
> ?
Ack
> > +
> > + 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.
Ack
> > + 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.
Ack
> > +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.
Ack
> > +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.
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.
> > + 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 :)
Tom acked a similar help text on the SPL_ZSTD option itself, but maybe
duplicating it all over the place is an overkill. Happy to distill it
in the next version.
> > +
> > +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.
> > +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.
Ack
Best regards,
Alexey
