Re: [PATCH v2 03/10] spl: ufs: add Falcon mode load path

Hi Simon,

On Tue, Aug 4, 2026 at 5:02 PM Simon Glass <[email protected]> wrote:
>
> Hi Alexey,
>
> On 2026-07-31T17:10:44, Alexey Charkov <[email protected]> wrote:
> > spl: ufs: add Falcon mode load path
> >
> > In Falcon mode, the SPL loads a FIT image containing a Linux kernel
> > instead of U-boot proper, and it may need to fall back to loading U-boot
> > if Linux is unavailable.
> >
> > Add support for images containing a Linux kernel, optionally on a
> > different UFS LUN and/or offset vs. U-boot proper to enable fallback.
> >
> > Signed-off-by: Alexey Charkov <[email protected]>
> >
> > common/spl/Kconfig   | 23 +++++++++++++++++++++
> >  common/spl/spl_ufs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 79 insertions(+), 2 deletions(-)
>
> >     In Falcon mode, the SPL loads a FIT image containing a Linux kernel
> >     instead of U-boot proper, and it may need to fall back to loading U-boot
> >     if Linux is unavailable.
>
> Please spell it 'U-Boot' (both occurrences). Since this patch actually
> accepts TF-A and TEE payloads too, please mention that here - the
> current wording suggests only a Linux kernel is expected.

Ack

> > diff --git a/common/spl/spl_ufs.c b/common/spl/spl_ufs.c
> > @@ -20,13 +20,38 @@ static ulong spl_ufs_load_read(struct spl_load_info *load, ulong off, ulong size
> > +     if (spl_image->os != IH_OS_LINUX && spl_image->os != IH_OS_TEE &&
> > +         spl_image->os != IH_OS_ARM_TRUSTED_FIRMWARE) {
> > +             puts("Expected OS image is not found\n");
> > +             return -ENOENT;
> > +     }
>
> The equivalent check in mmc_load_image_raw_os() only accepts LINUX and
> TEE - the ARM_TRUSTED_FIRMWARE addition is new and specific to this
> series. Please mention that in the commit message so it is not
> mistaken for a copy/paste divergence, and consider whether the
> mmc/nand/spi paths should have the same relaxation in a follow-up.

Will do. I only tested the UFS and RAM boot paths, so I haven't
touched the others. I'll see if I can test MMC/SPI too (I don't have
any NAND-equipped board).

> > diff --git a/common/spl/spl_ufs.c b/common/spl/spl_ufs.c
> > @@ -20,13 +20,38 @@ static ulong spl_ufs_load_read(struct spl_load_info *load, ulong off, ulong size
> > +static int spl_ufs_load_image_raw_os(struct spl_image_info *spl_image,
> > +                                  struct spl_boot_device *bootdev,
> > +                                  struct spl_load_info *load,
> > +                                  struct blk_desc *bd)
> > +{
> > +     ulong sector = config_opt_enabled(CONFIG_SPL_OS_BOOT,
> > +                                       CONFIG_SPL_UFS_RAW_OS_SECTOR,
> > +                                       CONFIG_SPL_UFS_RAW_U_BOOT_SECTOR);
>
> bd here is just load->priv - please drop the extra parameter and fetch
> log2blksz from load->priv, matching how spl_ufs_load_read() already
> does it. Also, config_opt_enabled() is redundant: this helper is only
> reachable from the OS_BOOT branch below, so
> CONFIG_SPL_UFS_RAW_OS_SECTOR is always defined - just use it directly.

I believe dropping the guard caused compilation errors for the
non-Falcon case, as apparently GCC 15 didn't dead-code-eliminate it.
Will re-test.

> > diff --git a/common/spl/spl_ufs.c b/common/spl/spl_ufs.c
> > @@ -36,6 +61,35 @@ static int spl_ufs_load_image(struct spl_image_info *spl_image,
> > +             if (os_devnum == devnum) {
> > +                     os_bd = bd;
> > +                     os_load = load;
> > +             } else {
>
> The struct copy of load into os_load is unusual - spl_load_info is
> meant to be initialised through spl_load_init(). Since the two
> branches only diverge when the device differs, please just pass &load,
> bd to the helper in the equal case and drop os_load entirely; only the
> != branch needs a second spl_load_info.

Ack

> > diff --git a/common/spl/spl_ufs.c b/common/spl/spl_ufs.c
> > @@ -36,6 +61,35 @@ static int spl_ufs_load_image(struct spl_image_info *spl_image,
> > +             puts("spl_ufs_load_image: Failed to load falcon payload\n");
> > +             log_debug("(error=%d)\n", err);
> > +             if (CONFIG_IS_ENABLED(OS_BOOT_SECURE))
> > +                     return err;
> > +     }
> > +
> > +load_uboot:
>
> The other backends (spl_mmc, spl_nor, ...) print 'Fallback to U-Boot'
> before dropping through - please do the same here for consistency,
> otherwise the boot log just shows the failure with no indication that
> fallback is happening.

Ack

Thanks a lot for looking into this!

Best regards,
Alexey