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

[Sean Anderson <seanga2@gmail.com>]

On 7/20/24 08:36, Simon Glass wrote:
> Hi Sean,
> 
> On Thu, 18 Jul 2024 at 14:54, Sean Anderson <seanga2@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@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.

>>
>> 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.

>>
>>>        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.

--Sean