[EXT] Re: [RFC] spl_fit.c: SPL Falcon Mode return to U-Boot

[Tom Rini]

On Fri, Aug 18, 2023 at 05:00:05PM +0000, Elena Popa wrote:
> 
> 
> > > When Falcon Mode is enabled, spl_start_uboot() function must be defined.
> > If this function returns 0 then SPL should start the kernel, if it returns 1 then
> > U-Boot must be started.
> > >
> > > When spl_start_uboot() returns 1, then U-Boot must be loaded, and as far
> > as I can tell spl_image->os should be set to IH_OS_U_BOOT before:
> > >
> > > 	if (os_takes_devicetree(spl_image->os)) {
> > > 		ret = spl_fit_append_fdt(spl_image, info, sector, &ctx);
> > >
> > > in common/spl/spl_fit.c/ int spl_load_simple_fit(...)
> > >
> > > this is not the case currently, and the loading fails. If set manually
> > spl_image->os = IH_OS_U_BOOT, it works fine.
> > >
> > > The only place I could see where it could be set is a few lines above in:
> > >
> > > if (!spl_fit_image_get_os(ctx.fit, node, &spl_image->os))
> > > 	debug("Image OS is %s\n", genimg_get_os_name(spl_image->os));
> > > else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT))
> > > 	spl_image->os = IH_OS_U_BOOT;
> > >
> > > but the second "if" does not execute, obviously (since we are in falcon,
> > CONFIG_SPL_OS_BOOT is enabled). I see a few options, but I am not sure how
> > best to fix this:
> > >
> > > 1) add something like:
> > > else if (!IS_ENABLED(CONFIG_SPL_OS_BOOT) || spl_start_uboot())
> > > 	spl_image->os = IH_OS_U_BOOT;
> > > but this would mean calling spl_start_uboot() twice, requiring it to return
> > the same value. It works in my case.
> > >
> > > 2) set spl_image->os to IH_OS_U_BOOT when the spl_start_uboot() is called
> > the first time, in:
> > > common/spl/spl_mmc.c/spl_mmc_do_fs_boot(..)    - in several places:
> > > #ifdef CONFIG_SPL_FS_FAT
> > > 	if (!spl_start_uboot()) {
> > > 		err = spl_load_image_fat_os(spl_image, bootdev,
> > > mmc_get_blk_desc(mmc), ...
> > >
> > > #ifdef CONFIG_SPL_FS_EXT4
> > > 	if (!spl_start_uboot()) {
> > > 		err = spl_load_image_ext_os(spl_image, bootdev,
> > > mmc_get_blk_desc(mmc),
> > >
> > > ...
> > >
> > > Any insight on how to proceed? I've implemented spl_start_uboot() to
> > decide on whether to boot the kernel or the U-Boot based on key presses on
> > the console.
> > 
> > Are you talking about the case where we tried to load (or verify) the kernel
> > image and that failed, and so should fall back to U-Boot? Or the case where
> > we're going to U-Boot instead of trying the kernel. For the second case there,
> > we should be doing the same thing we do in the case of not-falcon mode. In
> > the first case however, we might well have a bug or two there and aren't
> > falling back to reporting the error and then loading U-Boot just like we hadn't
> > tried to load the kernel before.
> 
> I was referring to the second case: we are in Falcon Mode, but
> spl_start_uboot() returns 1, so we should load U-Boot instead of the
> Kernel. However, I found a solution that works with the current
> implementation: adding the "os" property into the u-boot node from
> U-Boot FIT image. Then, spl_fit_image_get_os(...) sets spl_image->os
> correctly.

Ah, OK.  I think this may be partly that the fall-back case wasn't
tested as much with FIT images as it was with legacy ones at the time,
so it might well be that the code needs a bit of fixing.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230818/4ba57efa/attachment.sig>