Re: [PATCH 5/5] spl: fit: add ramdisk load

[Francesco Valla <francesco@valla.it>]

Hi Simon,

On Mon, May 04, 2026 at 06:09:31AM -0600, Simon Glass wrote:
> Hi Francesco,
> 
> On 2026-04-28T20:24:41, Francesco Valla <francesco@valla.it> wrote:
> > spl: fit: add ramdisk load
> >
> > Add ramdisk loading logic to the 'full' SPL FIT loader, as well as the
> > corresponding FDT fixup. This is required for proper support of falcon
> > boot using FIT images, but is useless for a U-Boot launch, so make it
> > depend on SPL_OS_BOOT.
> >
> > Signed-off-by: Francesco Valla <francesco@valla.it>
> >
> > common/spl/spl.c     | 17 ++++++++++++++---
> >  common/spl/spl_fit.c | 18 ++++++++++++++++--
> >  include/spl.h        | 20 ++++++++++++++++++++
> >  3 files changed, 50 insertions(+), 5 deletions(-)
> 
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > @@ -172,6 +172,14 @@ void spl_fixup_fdt(void *fdt_blob)
> >                       return;
> >               }
> >       }
> > +
> > +#if IS_ENABLED(CONFIG_SPL_OS_BOOT)
> > +     err = fdt_initrd(fdt_blob, initrd_start, initrd_end);
> > +     if (err) {
> > +             printf(PHASE_PROMPT "fdt_initrd err - %d\n", err);
> > +             return;
> > +     }
> > +#endif
> >  #endif
> >  }
> 
> Please us 'if (IS_ENABLED(CONFIG_SPL_OS_BOOT))' - we try to avoid #if
> in C files for better build-coverage.
> 

Ok.

> > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> > @@ -1011,6 +1011,20 @@ int spl_load_fit_image(struct spl_image_info *spl_image,
> >               }
> >       }
> >
> > +     if (spl_image->os != IH_OS_U_BOOT) {
> > +#ifdef CONFIG_SPL_FIT_SIGNATURE
> > +             images.verify = 1;
> > +#endif
> > +             ret = fit_image_load(&images, virt_to_phys((void *)header), NULL,
> > +                                  &fit_uname_config, IH_ARCH_DEFAULT,
> > +                                  IH_TYPE_RAMDISK, -1, FIT_LOAD_OPTIONAL,
> > +                                  &rd_data, &rd_len);
> > +             if (ret >= 0) {
> > +                     spl_image->ramdisk_addr = rd_data;
> > +                     spl_image->ramdisk_size = rd_len;
> > +             }
> > +     }
> > +
> 
> There is no SPL_OS_BOOT guard here, so the ramdisk lookup is attempted
> for any non-U-Boot OS (ATF, OP-TEE, OpenSBI, Linux). Is that
> intentional? With FIT_LOAD_OPTIONAL it is harmless when no ramdisk
> node exists, but gating on CONFIG_IS_ENABLED(OS_BOOT) would match the
> consumer side in spl_fixup_fdt()
> 

It _was_ intentional, since when I wrote initially this patch I was
experimenting with different options for the FIT image format (including
e.g. having TF-A instead of Linux marked as the OS).

But in the end a guard is probably a better option, I agree. 

> > diff --git a/include/spl.h b/include/spl.h
> > @@ -288,6 +288,8 @@ struct spl_image_info {
> >       ulong entry_point;
> >  #if CONFIG_IS_ENABLED(LOAD_FIT) || CONFIG_IS_ENABLED(LOAD_FIT_FULL)
> >       void *fdt_addr;
> > +     ulong ramdisk_addr;
> > +     ulong ramdisk_size;
> >  #endif
> 
> Only the LOAD_FIT_FULL path populates these (spl_load_simple_fit()
> does not load a ramdisk), so they could be gated on LOAD_FIT_FULL
> only. Please also add a kernel-doc comment to the new
> spl_image_ramdisk_start()/spl_image_ramdisk_end() helpers describing
> the units and what they return when the FIT loader is not enabled -
> 'end' in particular is ambiguous (inclusive vs exclusive), so please
> state that it is the exclusive end address expected by fdt_initrd().
> 

Ok.

> Regards,
> Simon

Thank you!

Regards,
Francesco