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

[Sean Anderson <seanga2@gmail.com>]

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?

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?

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

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

> +	upl_set_fit_info(map_to_sysmem(header), conf_noffset,
> +			 spl_image->entry_point);
> +
>   	return 0;
>   }
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 98d822530d8..37643a4bbb9 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -716,6 +716,10 @@ enum gd_flags {
>   	 * @GD_FLG_HUSH_MODERN_PARSER: Use hush 2021 parser.
>   	 */
>   	GD_FLG_HUSH_MODERN_PARSER = 0x2000000,
> +	/**
> +	 * @GD_FLG_UPL: Read/write a Universal Payload (UPL) handoff
> +	 */
> +	GD_FLG_UPL = 0x4000000,
>   };
>   
>   #endif /* __ASSEMBLY__ */