[PATCH v2 15/21] spl: Plumb in the Universal Payload handoff

[Simon Glass]

Hi Sean,

On Sat, 20 Jul 2024 at 15:44, Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 7/20/24 08:36, Simon Glass wrote:
> > Hi Sean,
> >
> > On Thu, 18 Jul 2024 at 14:54, Sean Anderson <seanga2 at gmail.com> wrote:
> >>
> >> On 7/13/24 03:00, Simon Glass wrote:
> >>> Specify the FIT and include information about each loaded image, as
> >>> required by the UPL handoff.
> >>>
> >>> Write the UPL handoff into the bloblist before jumping to the next phase.
> >>>
> >>> Control this using a runtime flag to avoid conflicting with other
> >>> handoff mechanisms.
> >>>
> >>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Hang when something goes wrong, to avoid a broken boot
> >>> - Add a runtime flag to enable UPL
> >>>
> >>>    common/spl/spl.c                  |  8 ++++++++
> >>>    common/spl/spl_fit.c              | 22 ++++++++++++++++++++++
> >>>    include/asm-generic/global_data.h |  4 ++++
> >>>    3 files changed, 34 insertions(+)
> >>>
> >>> diff --git a/common/spl/spl.c b/common/spl/spl.c
> >>> index 7794ddccade..d6a364de6ee 100644
> >>> --- a/common/spl/spl.c
> >>> +++ b/common/spl/spl.c
> >>> @@ -810,6 +810,14 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> >>>                        printf(SPL_TPL_PROMPT
> >>>                               "SPL hand-off write failed (err=%d)\n", ret);
> >>>        }
> >>> +     if (CONFIG_IS_ENABLED(UPL_OUT) && (gd->flags & GD_FLG_UPL)) {
> >>> +             ret = spl_write_upl_handoff(&spl_image);
> >>> +             if (ret) {
> >>> +                     printf(SPL_TPL_PROMPT
> >>> +                            "UPL hand-off write failed (err=%d)\n", ret);
> >>> +                     hang();
> >>> +             }
> >>> +     }
> >>>        if (CONFIG_IS_ENABLED(BLOBLIST)) {
> >>>                ret = bloblist_finish();
> >>>                if (ret)
> >>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> >>> index 2a097f4464c..b288f675ae3 100644
> >>> --- a/common/spl/spl_fit.c
> >>> +++ b/common/spl/spl_fit.c
> >>> @@ -12,6 +12,7 @@
> >>>    #include <memalign.h>
> >>>    #include <mapmem.h>
> >>>    #include <spl.h>
> >>> +#include <upl.h>
> >>>    #include <sysinfo.h>
> >>>    #include <asm/global_data.h>
> >>>    #include <asm/io.h>
> >>> @@ -645,6 +646,8 @@ static int spl_fit_load_fpga(struct spl_fit_info *ctx,
> >>>                printf("%s: Cannot load the FPGA: %i\n", __func__, ret);
> >>>                return ret;
> >>>        }
> >>> +     upl_add_image(node, fpga_image.load_addr, fpga_image.size,
> >>> +                   fdt_getprop(ctx->fit, node, FIT_DESC_PROP, NULL));
> >>
> >> Does load_addr even make sense for FPGAs?
> >
> > Well the images do get loaded into RAM at a particular address.
>
> No they don't. I mean, technically yes, but it's just an intermediate step
> before being programmed into the FPGA. Much like how executable boot images
> may be loaded to an intermediate address before being copied to their final
> location. And the load_addr is set to 0 a few lines above.
>
> But don't bother special-casing this if you do the next part.

OK. I see the FPGA-loading happening right away. IMO this should be
done later, after all images are loaded. But I suppose the code is
easier the way it is.

>
> >>
> >> And I noticed that you always call this after load_simple_fit/fit_image_load.
> >> Could we call upl_add_image in those functions instead?
> >
> > Yes, although it means compiling upl.h for the host. Perhaps it isn't
> > that bad though and it avoids repeating code.
>
> I would prefer that to help ensure that future programmers don't forget it.

Yes.

>
> >>
> >>>        return spl_fit_upload_fpga(ctx, node, &fpga_image);
> >>>    }
> >>> @@ -768,6 +771,9 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
> >>>        if (ret)
> >>>                return ret;
> >>>
> >>> +     upl_add_image(node, spl_image->load_addr, spl_image->size,
> >>> +                   fdt_getprop(ctx.fit, node, FIT_DESC_PROP, NULL));
> >>> +
> >>>        /*
> >>>         * For backward compatibility, we treat the first node that is
> >>>         * as a U-Boot image, if no OS-type has been declared.
> >>> @@ -811,6 +817,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
> >>>                               __func__, index, ret);
> >>>                        return ret;
> >>>                }
> >>> +             upl_add_image(node, image_info.load_addr, image_info.size,
> >>> +                           fdt_getprop(ctx.fit, node, FIT_DESC_PROP, NULL));
> >>>
> >>>                if (spl_fit_image_is_fpga(ctx.fit, node))
> >>>                        spl_fit_upload_fpga(&ctx, node, &image_info);
> >>> @@ -847,6 +855,8 @@ int spl_load_simple_fit(struct spl_image_info *spl_image,
> >>>                spl_image->entry_point = spl_image->load_addr;
> >>>
> >>>        spl_image->flags |= SPL_FIT_FOUND;
> >>> +     upl_set_fit_info(map_to_sysmem(ctx.fit), ctx.conf_node,
> >>> +                      spl_image->entry_point);
> >>
> >> I think this should be virt_to_phys, since we aren't really mapping it.
> >
> > We want to pass the address, which (for sandbox) has to be able to be
> > mapped back to a pointer. Everywhere else in sandbox this is how we
> > handle that...
>
> This is really a nit. But I thought that map_to_sysmem implied a pairing with
> unmap. But I guess that's map_physmem.

Yes, map_sysmem() is the 'sandbox mapping' from an U-Boot address to a
pointer, and map_to_sysmem() reverses that.

Regards,
Simon