[U-Boot] [PATCH v4 8/9] spl: dm: Make it possible for the SPL to pick its own DTB from a FIT
Jean-Jacques Hiblot
jjhiblot at ti.com
Thu Aug 24 10:52:38 UTC 2017
Hi Simon,
On 13/08/2017 23:35, Simon Glass wrote:
> 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.
>
Thanks
>> 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?
Unfortunately we cannot. The data itself is not part of the fdt, it is
appended after it. In the fdt, we find the descriptions, and for each
binary the size and the offset relative to the end of the fdt.
JJ
>
>> + */
>> + 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