[U-Boot] [PATCH v4 8/9] spl: dm: Make it possible for the SPL to pick its own DTB from a FIT
Simon Glass
sjg at chromium.org
Sun Aug 13 21:35:05 UTC 2017
Hi Jean-Jacques,
On 7 August 2017 at 04:07, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
> u-boot can be embedded within a FIT image with multiple DTBs. It then
> selects at run-time which one is best suited for the platform.
> Use the same principle here for the SPL: put the DTBs in a FIT image,
> compress it (LZO, GZIP, or no compression) and append it at the end of the
> SPL.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
> ---
>
> change since v3: updated the help in Kconfig to take in account commit f1896c
> ("spl: make SPL and normal u-boot stage use independent SYS_MALLOC_F_LEN")
>
> doc/README.multi-dtb-fit | 44 +++++++++++++++++++++++--
> dts/Kconfig | 83 ++++++++++++++++++++++++++++++++++++++++++++++
> lib/fdtdec.c | 85 +++++++++++++++++++++++++++++++++++++++++++-----
> scripts/Makefile.spl | 35 +++++++++++++++++++-
> 4 files changed, 235 insertions(+), 12 deletions(-)
>
Unfortunately I have quite a few comments and a few nits below. I feel
this is an important feature and most of my comments relate to polish.
It's a nice implementation.
> diff --git a/doc/README.multi-dtb-fit b/doc/README.multi-dtb-fit
> index d182d4e..5e593c8 100644
> --- a/doc/README.multi-dtb-fit
> +++ b/doc/README.multi-dtb-fit
> @@ -1,9 +1,49 @@
> MULTI DTB FIT
>
> -The purpose of this feature is to enable u-boot to select its DTB from a FIT
> -image appended at the end of the binary.
> +The purpose of this feature is to enable u-boot or the SPL to select its DTB
> +from a FIT image appended at the end of the binary.
Since FIT means Flat Image Tree (I think) we should probably write
'FIT' everywhere instead of 'FIT image'.
> +It comes in two flavor: u-boot (CONFIG_MULTI_DTB_FIT) and SPL
> +(CONFIG_SPL_MULTI_DTB_FIT)
>
> +u-boot flavor:
U-Boot (please fix throughout)
> Usually the DTB is selected by the SPL and passed down to u-boot. But some
> platforms don't use the SPL. In this case MULTI_DTB_FIT can used to provide
> u-boot with a choice of DTBs. The FIT image is automatically image at the end
> of the compilation and appended to u-boot.bin
'automatically image' - does that mean automatically built? Or
(looking below) generated?
> +
> +
> +
> +SPL flavor:
> +the SPL uses only a small subset of the DTB and it usually depends more
> +on the SOC than on the board. So it's usually fine to include a DTB in the
> +SPL that doesn't exactly match the board. There are howerver somes cases
however some
> +where it's not possible. In the later case, in order to support multiple
> +boards (or board revisions) with the same SPL binary, SPL_MULTI_DTB_FIT
> +can be used. The list of DTB is given in CONFIG_SPL_OF_LIST which by default
> +is the same list as CONFIG_OF_LIST
The DTB files are packed into a FIT
> +The FIT image is automatically generated at the end of the compilation,
> +compressed and appended to u-boot-spl.bin
so that SPL can locate it and select the correct DTB from inside the FIT.
I think this needs expanding a bit:
- board_fit_config_name_match() is used to find the correct DTB within the FIT
- mention an example board that uses this feature (so people can trace the code)
- how memory is allocated for decompression
- mention compression - it is automatic in the build and SPL
decompresses the correct DTB
> +
> +The impact of this option is relatively small. Here are some numbers measured
> +for a TI DRA7 platform:
> +
> + +----------------------------------------+
> + | Size |delta |boot-time| delta |
> + | (bytes) |(bytes) |(ms) | (ms) |
> ++---------------------------------------------------------------+
> +| reference | 120185 | | 1331 | |
> ++---------------------------------------------------------------+
> +| feature | | | | |
> +| deactiVated | 120185 | 0 | 1330 | -1 |
> ++---------------------------------------------------------------+
> +| 1 DTB LZO | 120208 | 23 | 1331 | 0 |
> ++---------------------------------------------------------------+
> +| 4 DTB LZO | 120810 | 625 | 1336 | 5 |
> ++---------------------------------------------------------------+
> +| 4 DTB LZO | | | | |
> +| no malloc | 120746 | 561 | 1343 | 12 |
> ++---------------------------------------------------------------+
> +| 4 DTB GZIP | 128552 | 8367 | 1353 | 22 |
> ++---------------------------------------------------------------+
> +| 4 DTB No comp | 132352 | 12167 | 1351 | 20 |
> ++----------------------+----------+----------+---------+--------+
> diff --git a/dts/Kconfig b/dts/Kconfig
> index c78438a..ec91a71 100644
> --- a/dts/Kconfig
> +++ b/dts/Kconfig
> @@ -118,6 +118,89 @@ config MULTI_DTB_FIT
> the correct DTB to be used. Use this if you need to support
> multiple DTBs but don't use the SPL.
>
> +
> +config SPL_MULTI_DTB_FIT
> + depends on SPL_LOAD_FIT && SPL_OF_CONTROL && !SPL_OF_PLATDATA
> + bool "support embedding several DTBs in a FIT image for the SPL"
Can you please capitalise the options in this file, so 'Bool'
> + help
> + This option provides the SPL with the ability to select its own
> + DTB at runtime from an appended FIT image containing several DTBs.
> + This allows using the same SPL binary on multiple platforms.
> + The primary purpose is to handle different versions of
> + the same platform without tweaking the platform code if the
> + differences can be expressed in the DTBs (common examples are: bus
> + capabilities, pad configurations).
> +
> +config SPL_OF_LIST
> + string "List of device tree files to include for DT control in SPL"
> + depends on SPL_MULTI_DTB_FIT
> + default OF_LIST
> + help
> + This option specifies a list of device tree files to use for DT
> + control in the SPL. These will be packaged into a FIT. At run-time,
> + the SPL will select the correct DT to use by examining the
> + hardware (e.g. reading a board ID value). This is a list of
> + device tree files (without the directory or .dtb suffix)
> + separated by <space>.
> +
> +choice
> + prompt "SPL OF LIST compression"
> + depends on SPL_MULTI_DTB_FIT
> + default SPL_MULTI_DTB_FIT_LZO
> +
> +config SPL_MULTI_DTB_FIT_LZO
> + bool "LZO"
> + depends on SYS_MALLOC_F
> + select SPL_LZO
> + help
> + Compress the FIT image containing the DTBs available for the SPL
> + using LZO compression. (requires lzop on host).
> +
> +config SPL_MULTI_DTB_FIT_GZ
We seem to use GZIP so could you update this to SPL_MULTI_DTB_FIT_GZIP?
> + bool "GZIP"
> + depends on SYS_MALLOC_F
> + select SPL_GZIP
> + help
> + Compress the FIT image containing the DTBs available for the SPL
> + using GZIP compression. (requires gzip on host)
> +
> +config SPL_MULTI_DTB_FIT_NO_COMPRESSION
> + bool "No compression"
> + help
> + Do not compress the FIT image containing the DTBs available for the SPL.
> + Use this options only if LZO is not available and the DTBs are very small.
> +endchoice
> +
> +choice
> + prompt "location of uncompressed DTBs "
> + depends on (SPL_MULTI_DTB_FIT_GZ || SPL_MULTI_DTB_FIT_LZO)
> +
> +config SPL_MULTI_DTB_FIT_DYN_ALLOC
> + bool "Dynamically allocate the memory"
> + depends on SYS_MALLOC_F
> + default y
> +
> +config SPL_MULTI_DTB_FIT_USER_DEFINED_AREA
> + bool "user-defined location"
> +endchoice
> +
> +config SPL_MULTI_DTB_FIT_UNCOMPRESS_SZ
> + hex "Size of memory reserved to uncompress the DTBs"
> + depends on (SPL_MULTI_DTB_FIT_GZ || SPL_MULTI_DTB_FIT_LZO)
> + default 0x10000
> + help
> + This is the size of this area where the DTBs are uncompressed.
> + If this area is dynamically allocated, make sure that
> + SPL_SYS_MALLOC_F_LEN is big enough to contain it.
> +
> +config SPL_MULTI_DTB_FIT_USER_DEF_ADDR
> + hex "Address of memory where dtbs are uncompressed"
> + depends on SPL_MULTI_DTB_FIT_USER_DEFINED_AREA
> + help
> + the FIT image containing the DTBs is uncompressed in an area defined
> + at compilation time. This is the address of this area. It must be
> + aligned on 2-byte boundary.
Great to see a malloc() option here and that being the default. I feel
we have too many hard-coded addresses.
Also can you clarify in the README that this decompression happens
during spl_init() or spl_early_init()? I think it is important to know
when the DT is available.
> +
> config OF_SPL_REMOVE_PROPS
> string "List of device tree properties to drop for SPL"
> depends on SPL_OF_CONTROL
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 0374f21..e7a1df6 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -8,6 +8,8 @@
> #include <common.h>
> #include <dm.h>
> #include <errno.h>
> +#include <spl.h>
> +#include <linux/lzo.h>
> #include <serial.h>
> #include <libfdt.h>
> #include <fdt_support.h>
If you have time you could send a patch to sort these correctly.
> @@ -1203,9 +1205,65 @@ int fdtdec_setup_memory_banksize(void)
> }
> #endif
>
> +#if CONFIG_IS_ENABLED(MULTI_DTB_FIT)
> +# if CONFIG_IS_ENABLED(GZIP) || CONFIG_IS_ENABLED(LZO)
> +static int uncompress_blob(const void *src, ulong sz_src, void **dstp)
> +{
> + size_t sz_out = CONFIG_SPL_MULTI_DTB_FIT_UNCOMPRESS_SZ;
> + ulong sz_in = sz_src;
> + void *dst;
> + int rc;
> +
> +# if CONFIG_IS_ENABLED(GZIP)
can we use:
if (CONFIG_IS_ENABLED(GZIP))
here? and below? We should try to avoid extreme #ifdeffing.
> + if (gzip_parse_header(src, sz_in) < 0)
> + return -1;
> +# elif CONFIG_IS_ENABLED(LZO)
> + if (!lzop_is_valid_header(src))
> + return -EBADMSG;
> +# endif
> +
> +# if CONFIG_IS_ENABLED(MULTI_DTB_FIT_DYN_ALLOC)
> + dst = malloc(sz_out);
> + if (!dst) {
> + puts("uncompress_blob: Unable to allocate memory\n");
> + return -ENOMEM;
> + }
> +# elif CONFIG_IS_ENABLED(MULTI_DTB_FIT_USER_DEFINED_AREA)
> + dst = (void *)CONFIG_VAL(MULTI_DTB_FIT_USER_DEF_ADDR);
> +# else
> + return -ENOTSUPP;
> +# endif
> +
> +# if CONFIG_IS_ENABLED(GZIP)
> + rc = gunzip(dst, sz_out, (u8 *)src, &sz_in);
> +# elif CONFIG_IS_ENABLED(LZO)
> + rc = lzop_decompress(src, sz_in, dst, &sz_out);
> +# endif
> + if (rc < 0) {
> + /* not a valid compressed blob */
> + puts("uncompress_blob: Unable to uncompress\n");
> +# if CONFIG_IS_ENABLED(MULTI_DTB_FIT_DYN_ALLOC)
> + free(dst);
> +# endif
> + return -EBADMSG;
> + }
> + *dstp = dst;
> + return 0;
> +}
> +# else
> +static int uncompress_blob(const void *src, ulong sz_src, void **dstp)
> +{
> + return -ENOTSUPP;
> +}
> +# endif
> +#endif
> +
> int fdtdec_setup(void)
> {
> #if CONFIG_IS_ENABLED(OF_CONTROL)
> +# if CONFIG_IS_ENABLED(MULTI_DTB_FIT)
> + void *fdt_blob;
> +# endif
> # ifdef CONFIG_OF_EMBED
> /* Get a pointer to the FDT */
> gd->fdt_blob = __dtb_dt_begin;
> @@ -1216,15 +1274,6 @@ int fdtdec_setup(void)
> gd->fdt_blob = (ulong *)&_image_binary_end;
> else
> gd->fdt_blob = (ulong *)&__bss_end;
> -
> -# elif defined CONFIG_MULTI_DTB_FIT
> - gd->fdt_blob = locate_dtb_in_fit(&_end);
> -
> - if (gd->fdt_blob == NULL || gd->fdt_blob <= ((void *)&_end)) {
> - puts("Failed to find proper dtb in embedded FIT Image\n");
> - return -1;
> - }
> -
> # else
> /* FDT is at end of image */
> gd->fdt_blob = (ulong *)&_end;
> @@ -1243,7 +1292,25 @@ int fdtdec_setup(void)
> gd->fdt_blob = (void *)getenv_ulong("fdtcontroladdr", 16,
> (uintptr_t)gd->fdt_blob);
> # endif
> +
> +# if CONFIG_IS_ENABLED(MULTI_DTB_FIT)
> + /*
> + *try and uncompress the blob.
> + * max input size is set arbitrarily to 16MB (should more than enough)
This seems ugly. Can you call fdt_totalsize() instead?
> + */
> + if (uncompress_blob(gd->fdt_blob, 0x1000000, &fdt_blob) == 0)
> + gd->fdt_blob = fdt_blob;
> +
> + /*
> + * Check if blob is a FIT images containings DTBs.
> + * If so, pick the most relevant
> + */
> + fdt_blob = locate_dtb_in_fit(gd->fdt_blob);
> + if (fdt_blob)
> + gd->fdt_blob = fdt_blob;
> +# endif
> #endif
> +
> return fdtdec_prepare_fdt();
> }
>
> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
> index ac3c2c7..18316a9 100644
> --- a/scripts/Makefile.spl
> +++ b/scripts/Makefile.spl
> @@ -202,10 +202,21 @@ cmd_cat = cat $(filter-out $(PHONY), $^) > $@
> quiet_cmd_copy = COPY $@
> cmd_copy = cp $< $@
>
> +ifneq ($(CONFIG_SPL_MULTI_DTB_FIT),y)
> +FINAL_DTB_CONTAINER = $(obj)/$(SPL_BIN).dtb
> +else ifeq ($(CONFIG_SPL_MULTI_DTB_FIT_LZO),y)
> +FINAL_DTB_CONTAINER = $(obj)/$(SPL_BIN).multidtb.fit.lzo
> +else ifeq ($(CONFIG_SPL_MULTI_DTB_FIT_GZ),y)
> +FINAL_DTB_CONTAINER = $(obj)/$(SPL_BIN).multidtb.fit.gz
> +else
> +FINAL_DTB_CONTAINER = $(obj)/$(SPL_BIN).multidtb.fit
> +endif
> +
> +
> ifeq ($(CONFIG_SPL_OF_CONTROL)$(CONFIG_OF_SEPARATE)$(CONFIG_SPL_OF_PLATDATA),yy)
> $(obj)/$(SPL_BIN)-dtb.bin: $(obj)/$(SPL_BIN)-nodtb.bin \
> $(if $(CONFIG_SPL_SEPARATE_BSS),,$(obj)/$(SPL_BIN)-pad.bin) \
> - $(obj)/$(SPL_BIN).dtb FORCE
> + $(FINAL_DTB_CONTAINER) FORCE
> $(call if_changed,cat)
>
> $(obj)/$(SPL_BIN).bin: $(obj)/$(SPL_BIN)-dtb.bin FORCE
> @@ -369,6 +380,28 @@ checkdtoc: tools
> PHONY += FORCE
> FORCE:
>
> +PHONY += dtbs
> +dtbs:
> + $(Q)$(MAKE) $(build)=dts dtbs
> +
> # Declare the contents of the .PHONY variable as phony. We keep that
> # information in a variable so we can use it in if_changed and friends.
> .PHONY: $(PHONY)
> +
> +SHRUNK_ARCH_DTB = $(patsubst %,$(obj)/dts/%.dtb,$(subst ",,$(CONFIG_SPL_OF_LIST)))
> +.SECONDEXPANSION:
> +$(SHRUNK_ARCH_DTB): $$(patsubst $(obj)/dts/%, arch/$(ARCH)/dts/%, $$@)
> + $(call if_changed,fdtgrep)
> +
> +MKIMAGEFLAGS_$(SPL_BIN).multidtb.fit = -f auto -A $(ARCH) -T firmware -C none -O u-boot \
> + -n "Multi DTB fit image for $(SPL_BIN)" -E \
> + $(patsubst %,-b %,$(SHRUNK_ARCH_DTB))
> +
> +$(obj)/$(SPL_BIN).multidtb.fit: /dev/null $(SHRUNK_ARCH_DTB) FORCE
> + $(call if_changed,mkimage)
> +
> +$(obj)/$(SPL_BIN).multidtb.fit.gz: $(obj)/$(SPL_BIN).multidtb.fit
> + @gzip -kf9 $< > $@
> +
> +$(obj)/$(SPL_BIN).multidtb.fit.lzo: $(obj)/$(SPL_BIN).multidtb.fit
> + @lzop -f9 $< > $@
> --
> 1.9.1
>
Regards,
Simon
More information about the U-Boot
mailing list