[PATCH v7 5/7] misc: fw_loader: introduce FIP loader driver

[Simon Glass]

Hi Christian,

On 2026-06-16T19:18:35, Christian Marangi <ansuelsmth at gmail.com> wrote:
> misc: fw_loader: introduce FIP loader driver
>
> Introduce a variant of the FS loader driver to extract images from FIP
> image. These image can contain additional binary used to init Network
> accelerator or PHY firmware blob.
>
> The way FIP handle image type is with the usage of UUID.
>
> This FIP loader driver implement a simple FIP image parser that check
> every entry for a matching UUID.
>
> Similar to FS loader, this driver also support both UBI and Block
> devices.
>
> Also an additional property is added to handle special case with eMMC
> that doesn't have a GPT partition and require a global offset to
> reference the FIP partition.
>
> An example usage of this driver is the following:
>
> [...]
>
> drivers/misc/Kconfig                      |  11 +
>  drivers/misc/fw_loader/Makefile           |   1 +
>  drivers/misc/fw_loader/fip_loader.c       | 544 ++++++++++++++++++++++++++++++
>  drivers/misc/fw_loader/fw_loader-uclass.c |   3 +
>  drivers/misc/fw_loader/internal.h         |   2 +
>  5 files changed, 561 insertions(+)

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,544 @@
> +static int fw_parse_storage_info(struct udevice *dev,
> +                              struct fip_storage_info *info)
> +{
> +     char *storage_interface, *dev_part, *ubi_mtdpart, *ubi_volume;
> +     struct device_plat *plat = dev_get_uclass_plat(dev);
> +     int ret;
> +
> +     storage_interface = env_get('storage_interface');
> +     dev_part = env_get('fw_dev_part');
> +     ubi_mtdpart = env_get('fw_ubi_mtdpart');
> +     ubi_volume = env_get('fw_ubi_volume');
> +     info->part_offset = env_get_hex('fw_partoffset', 0);

A large chunk of this - the env lookups, the phandlepart resolution
and the mtdpart/ubivol fallback - duplicates
fw_get_filesystem_prepare()/select_fs_dev() in fs_loader.c almost
line-for-line. Please factor the common storage-selection logic into
the uclass (or into a helper in internal.h) so both drivers share one
implementation.

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,544 @@
> +static int blk_parse_fip_firmware(struct firmware *firmwarep,
> +                               struct blk_desc *desc,
> +                               struct disk_partition *part_info,
> +                               unsigned int part_offset,
> +                               struct fip_toc_entry *dent)
> +{
> +     unsigned int offset = part_info->start + part_offset;

part_info->start is lbaint_t, which is 64-bit on most modern targets.
Storing it in unsigned int silently truncates partitions whose start
LBA is beyond 4G blocks; the same applies to blkstart further down and
to part_offset in struct fip_storage_info. Please use lbaint_t for the
on-device block coordinates.

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,544 @@
> +#define TOC_HEADER_NAME      0xaa640001
> +
> +struct fip_toc_header {
> +     u32 name;
> +     u32 serial_number;
> +     u64 flags;
> +};
> +
> +struct fip_toc_entry {
> +     struct uuid uuid;
> +     u64 offset_address;
> +     u64 size;
> +     u64 flags;
> +};

These describe an on-disk format that TF-A defines as little-endian,
but the code reads them straight into native u32/u64 and compares with
TOC_HEADER_NAME directly. Do we have to worry about big-endian
targets?

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,544 @@
> +#ifdef CONFIG_CMD_UBIFS
> +static int ubi_parse_fip_firmware(struct firmware *firmwarep,
> +                               char *ubi_vol,
> +                               struct fip_toc_entry *dent)

The rest of the FW loader code gates UBI support with
CONFIG_IS_ENABLED(CMD_UBIFS) so it works correctly across SPL/VPL
phases. Please use the same idiom here, otherwise an SPL build with
CONFIG_SPL_CMD_UBIFS but no CONFIG_CMD_UBIFS will silently drop the
UBI path.

> diff --git a/drivers/misc/fw_loader/Makefile b/drivers/misc/fw_loader/Makefile
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0+
>
>  obj-y += fw_loader-uclass.o
> +obj-$(CONFIG_FIP_LOADER) += fip_loader.o
>  obj-$(CONFIG_$(PHASE_)FS_LOADER) += fs_loader.o

FS_LOADER uses the $(PHASE_) prefix so it can be enabled per-phase,
but FIP_LOADER is gated on a single non-phased symbol. Was that
intentional? If FIP is meant to be usable in SPL/TPL/VPL too (the
cover letter suggests it is), this should probably be
CONFIG_$(PHASE_)FIP_LOADER

> diff --git a/drivers/misc/fw_loader/fw_loader-uclass.c b/drivers/misc/fw_loader/fw_loader-uclass.c
> @@ -87,6 +87,9 @@ static int fw_loader_pre_probe(struct udevice *dev)
>
>               plat->ubivol = (char *)ofnode_read_string(fw_loader_node,
>                                                         'ubivol');
> +
> +             ofnode_read_u32(fw_loader_node, 'partoffset',
> +                             &plat->partoffset);
>       }

partoffset is a FIP-specific concept (a byte offset inside a partition
where the FIP image lives), but it is being parsed in the generic
uclass and stored in the common device_plat - either document it as
applying to every loader and have fs_loader honour it, or move the DT
read into the FIP driver so the uclass stays neutral.

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,544 @@
> +static int fw_get_fip_firmware_size(struct udevice *dev)
> +{
> +     struct fip_storage_info info = { };
> +     struct firmware *firmwarep;
> +     struct fip_toc_entry ent;
> +     int ret;
> +
> +     ret = fw_parse_storage_info(dev, &info);
> +     if (ret)
> +             return ret;
> +
> +     firmwarep = dev_get_uclass_priv(dev);
> +     if (!firmwarep)
> +             return -EINVAL;
> +
> +     ret = parse_fip_firmware(firmwarep, &info, &ent);
> +     if (ret)
> +             return ret;
> +
> +     return ent.size;
> +}

get_size() returns ent.size, but the get_firmware() path treats the
usable size as ent.size - firmwarep->offset. A caller using
request_firmware_size() to size a buffer and then passing a non-zero
offset to request_firmware_into_buf() will get an over-large
allocation, or interpret size inconsistently between the two calls.
Please pick one definition (most likely ent.size - firmwarep->offset)
and use it in both ops.

> diff --git a/drivers/misc/fw_loader/fip_loader.c b/drivers/misc/fw_loader/fip_loader.c
> @@ -0,0 +1,544 @@
> +#include <dm.h>
> +#include <div64.h>
> +#include <env.h>
> +#include <errno.h>
> +#include <blk.h>
> +#include <fs.h>
> +#include <linux/kernel.h>
> +#include <log.h>
> +#include <mapmem.h>
> +#include <malloc.h>
> +#include <memalign.h>
> +#include <part.h>
> +#include <u-boot/uuid.h>

map_to_sysmem()/memalign() are not used; please drop mapmem.h and
memalign.h. Also please sort the includes properly - e.g. blk.h should
come before dm.h

Regards,
Simon