Hi Sean,
On Thu, 18 Jul 2024 at 14:54, Sean Anderson <sean...@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 <s...@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.
>
> 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.
>
> > 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...
>
> >
> > return 0;
> > }
> > @@ -891,6 +901,9 @@ int spl_load_fit_image(struct spl_image_info *spl_image,
> > if (ret < 0)
> > return ret;
> >
> > + upl_add_image(ret, fw_data, fw_len,
> > + fdt_getprop((void *)header, ret, FIT_DESC_PROP, NULL));
> > +
> > spl_image->size = fw_len;
> > spl_image->load_addr = fw_data;
> > if (fit_image_get_entry(header, ret, &spl_image->entry_point))
> > @@ -911,6 +924,9 @@ int spl_load_fit_image(struct spl_image_info *spl_image,
> > if (ret >= 0) {
> > spl_image->fdt_addr = (void *)dt_data;
> >
> > + upl_add_image(ret, dt_data, dt_len,
> > + fdt_getprop((void *)header, ret, FIT_DESC_PROP, NULL));
> > +
> > if (spl_image->os == IH_OS_U_BOOT) {
> > /* HACK: U-Boot expects FDT at a specific address */
> > fdt_hack = spl_image->load_addr + spl_image->size;
> > @@ -940,7 +956,13 @@ int spl_load_fit_image(struct spl_image_info
> > *spl_image,
> > &img_data, &img_len);
> > if (ret < 0)
> > return ret;
> > + upl_add_image(ret, img_data, img_len,
> > + fdt_getprop((void *)header, ret, FIT_DESC_PROP, NULL));
> > }
> >
> > + spl_image->flags |= SPL_FIT_FOUND;
>
> This change is unrelated and should be moved into a separate commit.
OK
Regards,
Simon
