Re: [PATCH v2 2/5] boot: add a firmware-owned devicetree source

Hi Carlo,

On 2026-07-28T13:20:42, Carlo Caione <[email protected]> wrote:
> boot: add a firmware-owned devicetree source
>
> Platforms following EBBR / Arm SystemReady IR keep the devicetree on a
> firmware-owned partition, updated independently of the operating system,
> rather than shipping it in the OS image. U-Boot has no generic way to
> source and assemble a devicetree from such a partition.
>
> Add firmware_fdt_load(), a loader for a FIT from a firmware-owned
> devicetree source. The FIT images hold the base DTB and its overlays,
> while each configuration names one valid combination through its standard
> 'fdt' property. Select an explicit 'fw_fdt_config' when set, otherwise
> use compatible best-match against the control devicetree and fall back to
> the FIT default.
>
> Describe the source with a standalone control-DT node. This patch
> implements the first source backend, 'u-boot,firmware-fdt-block', which
> reads the FIT from a filesystem on a GPT partition of a block device:
>
>     firmware-fdt {
>         compatible = "u-boot,firmware-fdt-block";
> [...]
>
> MAINTAINERS                               |   3 +
>  boot/Kconfig                              |  24 ++
>  boot/Makefile                             |   1 +
>  boot/firmware_fdt.c                       | 420 ++++++++++++++++++++++++++++++
>  boot/image-fdt.c                          |   3 +-
>  boot/image-fit.c                          |   9 +-
>  doc/develop/uefi/firmware_fdt.rst         | 112 ++++++++
>  doc/develop/uefi/index.rst                |   1 +
>  doc/device-tree-bindings/firmware-fdt.txt | 150 +++++++++++
>  doc/usage/environment.rst                 |  11 +
>  include/firmware_fdt.h                    |  95 +++++++
>  include/image.h                           |   4 +-
>  12 files changed, 829 insertions(+), 4 deletions(-)

> diff --git a/boot/firmware_fdt.c b/boot/firmware_fdt.c
> @@ -0,0 +1,420 @@
> +static int fw_fdt_get_source(ofnode *srcp)
> +{
> +     ofnode node;
> +
> +     node = ofnode_by_compatible(ofnode_null(), FW_FDT_COMPAT_BLOCK);
> +     while (ofnode_valid(node) && !ofnode_is_enabled(node))
> +             node = ofnode_by_compatible(node, FW_FDT_COMPAT_BLOCK);
> +
> +     if (!ofnode_valid(node))
> +             return -ENOENT;
> +
> +     *srcp = node;
> +
> +     return 0;
> +}

If a control DT has two enabled u-boot,firmware-fdt-block nodes this
silently picks the first, which is traversal-order dependent and hard
to debug. Is your intention that only one is permitted?

Also, wouldn't it be better to make this a driver so that driver model
can handle discovery, binding, etc.?

> diff --git a/boot/firmware_fdt.c b/boot/firmware_fdt.c
> @@ -0,0 +1,420 @@
> +     fdt = map_sysmem(data, len);
> +
> +     out->fdt = fdt;
> +     out->size = len;
> +
> +     if (len > FIRMWARE_FDT_MAX_SIZE)
> +             return log_msg_ret("bigfdt", -E2BIG);
> +
> +     ret = fdt_check_full(fdt, len);
> +     if (ret)
> +             return log_msg_ret("chk", -EINVAL);
> +
> +     out->name = strdup(fname);

Size and integrity checks belong before the assignments to out->fdt / out->size

> diff --git a/include/firmware_fdt.h b/include/firmware_fdt.h
> @@ -0,0 +1,95 @@
> +/* Maximum size of both the firmware FIT and the assembled devicetree */
> +#define FIRMWARE_FDT_MAX_SIZE        SZ_4M

4 MiB is a policy decision - some platforms will want a smaller cap,
others may need more once overlays multiply. Please make this a
Kconfig knob (default SZ_4M) so boards can tune it without patching a
header. The name also implies an FDT-only limit while the comment says
it covers the FIT too; FIRMWARE_FDT_FIT_MAX_SIZE would be less
surprising.

> diff --git a/boot/firmware_fdt.c b/boot/firmware_fdt.c
> @@ -0,0 +1,420 @@
> +     if (namep) {
> +             name = strdup(fw.name);
> +             if (!name) {
> +                     ret = -ENOMEM;
> +                     goto out;
> +             }
> +     }

fw.name is itself a strdup() of the FIT filename made inside
fw_fdt_assemble(); duplicating it a second time here just so
firmware_fdt_free() can free the original is wasteful. Consider
transferring ownership - pass fw.name out and clear it in the struct
so firmware_fdt_free() leaves it alone - or don't allocate fw.name
until it is needed.

> diff --git a/include/image.h b/include/image.h
> @@ -719,12 +719,14 @@ int boot_get_setup_fit(struct bootm_headers *images, uint8_t arch,
> + * @param ownedp     Returns true if the loaded image is separately allocated
> + *                   and must be freed by the caller, or NULL

The "or NULL" trails 'must be freed by the caller' and reads as if the
returned image can be NULL. It refers to the parameter itself being
optional - please reword as a separate sentence, e.g. 'may be NULL if
the caller does not need this information'.

Regards,
Simon