Re: [PATCH v4 15/19] board: am62px: Define capsule update firmware info

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Jonathan

Thanks for working on this

On Thu, May 09, 2024 at 11:41:19AM -0500, Jonathan Humphreys wrote:
> Define the firmware components updatable via EFI capsule update, including
> defining capsule GUIDs for the various firmware components for the AM62px
> SK.
>
> Signed-off-by: Jonathan Humphreys <j-humphreys@ti.com>
> ---
>  board/ti/am62px/evm.c        | 32 ++++++++++++++++++++++++++++++++
>  include/configs/am62px_evm.h | 24 ++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/board/ti/am62px/evm.c b/board/ti/am62px/evm.c
> index 97a95ce8cc2..6d0f66e5dc0 100644
> --- a/board/ti/am62px/evm.c
> +++ b/board/ti/am62px/evm.c
> @@ -6,6 +6,7 @@
>   *
>   */
>
> +#include <efi_loader.h>
>  #include <asm/arch/hardware.h>
>  #include <asm/io.h>
>  #include <dm/uclass.h>
> @@ -13,6 +14,37 @@
>  #include <fdt_support.h>
>  #include <spl.h>
>
> +struct efi_fw_image fw_images[] = {

It's better if we add an
#if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT)
for both of the structs that follow (and it applies to all your patches)

> +	{
> +		.image_type_id = AM62PX_SK_TIBOOT3_IMAGE_GUID,
> +		.fw_name = u"AM62PX_SK_TIBOOT3",
> +		.image_index = 1,
> +	},
> +	{
> +		.image_type_id = AM62PX_SK_SPL_IMAGE_GUID,
> +		.fw_name = u"AM62PX_SK_SPL",
> +		.image_index = 2,
> +	},
> +	{
> +		.image_type_id = AM62PX_SK_UBOOT_IMAGE_GUID,
> +		.fw_name = u"AM62PX_SK_UBOOT",
> +		.image_index = 3,
> +	}
> +};
> +
> +struct efi_capsule_update_info update_info = {
> +	.dfu_string = "sf 0:0=tiboot3.bin raw 0 80000;"
> +	"tispl.bin raw 80000 200000;u-boot.img raw 280000 400000",
> +	.num_images = ARRAY_SIZE(fw_images),
> +	.images = fw_images,
> +};

I haven't worked on any TI platforms lately so I cant say much about the
naming and the flash regions. The definition seems correct


> +
> +void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +	if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
> +		env_set("dfu_alt_info", update_info.dfu_string);
> +}

There's a CONFIG_SET_DFU_ALT_INFO symbol. This better if we add a check here
as well

> +
>  int board_init(void)
>  {
>  	return 0;
> diff --git a/include/configs/am62px_evm.h b/include/configs/am62px_evm.h
> index 06b12860e21..57a1ba9dc3c 100644
> --- a/include/configs/am62px_evm.h
> +++ b/include/configs/am62px_evm.h
> @@ -8,6 +8,30 @@
>  #ifndef __CONFIG_AM62PX_EVM_H
>  #define __CONFIG_AM62PX_EVM_H
>
[...]

Regards
/Ilias