RockPi4: Add UEFI capsule update support

Add support for updating the idbloader and u-boot images through the
UEFI capsule update functionality. Enable the modules required for
supporting the functionality.

The implementation is for the updatable images placed on a GPT
partitioned storage device. With the GPT partition devices, the
partition information can be obtained at runtime, and hence the
dfu_alt_info variable needed for the updates is being populated at
runtime.

This requires the partitions containing the idbloader and u-boot
images to be created with the PartitionTypeGUID values set to the
corresponding image type GUID values defined in the platform's config
header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 arch/arm/mach-rockchip/Kconfig         |   1 +
 board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
 configs/rock-pi-4-rk3399_defconfig     |   4 +
 configs/rock-pi-4c-rk3399_defconfig    |   4 +
 include/configs/rk3399_common.h        |  16 +++
 5 files changed, 214 insertions(+), 1 deletion(-)

On 5/13/22 08:50, Sughosh Ganu wrote:
> Add support for updating the idbloader and u-boot images through the
> UEFI capsule update functionality. Enable the modules required for
> supporting the functionality.
> 
> The implementation is for the updatable images placed on a GPT
> partitioned storage device. With the GPT partition devices, the
> partition information can be obtained at runtime, and hence the
> dfu_alt_info variable needed for the updates is being populated at
> runtime.
> 
> This requires the partitions containing the idbloader and u-boot
> images to be created with the PartitionTypeGUID values set to the
> corresponding image type GUID values defined in the platform's config
> header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
> ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  arch/arm/mach-rockchip/Kconfig         |   1 +
>  board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
>  configs/rock-pi-4-rk3399_defconfig     |   4 +
>  configs/rock-pi-4c-rk3399_defconfig    |   4 +
>  include/configs/rk3399_common.h        |  16 +++
>  5 files changed, 214 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index 18aff5480b..6d13e7b92e 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
>  	select DM_PMIC
>  	select DM_REGULATOR_FIXED
>  	select BOARD_LATE_INIT
> +	imply PARTITION_TYPE_GUID
>  	imply PRE_CONSOLE_BUFFER
>  	imply ROCKCHIP_COMMON_BOARD
>  	imply ROCKCHIP_SDRAM_COMMON
> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
> index abb76585cf..cc976d9471 100644
> --- a/board/rockchip/evb_rk3399/evb-rk3399.c
> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
> @@ -5,11 +5,27 @@
>  
>  #include <common.h>
>  #include <dm.h>
> +#include <efi_loader.h>
>  #include <init.h>
>  #include <log.h>
> +#include <mmc.h>
> +#include <part.h>
> +#include <uuid.h>
>  #include <asm/arch-rockchip/periph.h>
>  #include <power/regulator.h>
>  
> +#define ROCKPI4_UPDATABLE_IMAGES	2
> +
> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
> +
> +struct efi_capsule_update_info update_info = {
> +	.images = fw_images,
> +};
> +
> +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
> +#endif
> +
>  #ifndef CONFIG_SPL_BUILD
>  int board_early_init_f(void)
>  {
> @@ -29,4 +45,176 @@ int board_early_init_f(void)
>  out:
>  	return 0;
>  }
> -#endif
> +
> +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION)
> +
> +#define DFU_ALT_BUF_LEN SZ_1K
> +
> +static bool board_is_rockpi_4b(void)
> +{
> +	return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> +		of_machine_is_compatible("radxa,rockpi4b");
> +}
> +
> +static bool board_is_rockpi_4c(void)
> +{
> +	return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> +		of_machine_is_compatible("radxa,rockpi4c");
> +}
> +
> +static bool updatable_image(struct disk_partition *info)
> +{
> +	int i;
> +	bool ret = false;
> +	efi_guid_t image_type_guid;
> +
> +	uuid_str_to_bin(info->type_guid, image_type_guid.b,
> +			UUID_STR_FORMAT_GUID);
> +
> +	for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> +		if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> +			ret = true;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void set_image_index(struct disk_partition *info, int index)
> +{
> +	int i;
> +	efi_guid_t image_type_guid;
> +
> +	uuid_str_to_bin(info->type_guid, image_type_guid.b,
> +			UUID_STR_FORMAT_GUID);
> +
> +	for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> +		if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> +			fw_images[i].image_index = index;
> +			break;
> +		}
> +	}
> +}
> +
> +static int get_mmc_desc(struct blk_desc **desc)
> +{
> +	int ret;
> +	struct mmc *mmc;
> +	struct udevice *dev;
> +
> +	/*
> +	 * For now the firmware images are assumed to
> +	 * be on the SD card
> +	 */
> +	ret = uclass_get_device(UCLASS_MMC, 1, &dev);
> +	if (ret)
> +		return -1;
> +
> +	mmc = mmc_get_mmc_dev(dev);
> +	if (!mmc)
> +		return -1;
> +
> +	if (mmc_init(mmc))
> +		return -1;
> +
> +	*desc = mmc_get_blk_desc(mmc);
> +	if (!*desc)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +	const char *name;
> +	bool first = true;
> +	int p, len, devnum, ret;
> +	char buf[DFU_ALT_BUF_LEN];
> +	struct disk_partition info;
> +	struct blk_desc *desc = NULL;
> +
> +	ret = get_mmc_desc(&desc);
> +	if (ret)
> +		return;
> +
> +	memset(buf, 0, sizeof(buf));
> +	name = blk_get_if_type_name(desc->if_type);
> +	devnum = desc->devnum;
> +	len = strlen(buf);
> +
> +	len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> +			 "%s %d=", name, devnum);
> +
> +	for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +		if (part_get_info(desc, p, &info))
> +			continue;
> +
> +		/* Add entry to dfu_alt_info only for updatable images */
> +		if (updatable_image(&info)) {
> +			if (!first)
> +				len += snprintf(buf + len,
> +						DFU_ALT_BUF_LEN - len, ";");
> +
> +			len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> +					"%s%d_%s part %d %d",
> +					name, devnum, info.name, devnum, p);
> +			first = false;
> +		}
> +	}
> +
> +	log_debug("dfu_alt_info => %s\n", buf);
> +	env_set("dfu_alt_info", buf);
> +}
> +
> +int rk_board_late_init(void)
> +{
> +	int p, i, ret;
> +	struct disk_partition info;
> +	struct blk_desc *desc = NULL;
> +
> +	if (board_is_rockpi_4b()) {
> +		efi_guid_t idbldr_image_type_guid =
> +			ROCKPI_4B_IDBLOADER_IMAGE_GUID;
> +		efi_guid_t uboot_image_type_guid = ROCKPI_4B_UBOOT_IMAGE_GUID;
> +
> +		guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> +		guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> +
> +		fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
> +		fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
> +	} else if (board_is_rockpi_4c()) {
> +		efi_guid_t idbldr_image_type_guid =
> +			ROCKPI_4C_IDBLOADER_IMAGE_GUID;
> +		efi_guid_t uboot_image_type_guid = ROCKPI_4C_UBOOT_IMAGE_GUID;
> +
> +		guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> +		guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> +
> +		fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
> +		fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
> +	}
> +
> +	ret = get_mmc_desc(&desc);
> +	if (ret)
> +		return ret;
> +
> +	for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +		if (part_get_info(desc, p, &info))
> +			continue;
> +
> +		/*
> +		 * Since we have a GPT partitioned device, the updatable
> +		 * images could be stored in any order. Populate the
> +		 * image_index at runtime.
> +		 */
> +		if (updatable_image(&info)) {
> +			set_image_index(&info, i);
> +			i++;
> +		}
> +	}
> +
> +	return 0;
> +}
> +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
> +#endif /* !CONFIG_SPL_BUILD */
> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
> index 80d1e63b59..ca3e217603 100644
> --- a/configs/rock-pi-4-rk3399_defconfig
> +++ b/configs/rock-pi-4-rk3399_defconfig
> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
> +CONFIG_CMD_DFU=y
>  CONFIG_CMD_GPT=y
>  CONFIG_CMD_MMC=y
>  CONFIG_CMD_PCI=y
> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>  CONFIG_ENV_IS_IN_MMC=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_DFU_MMC=y
>  CONFIG_ROCKCHIP_GPIO=y
>  CONFIG_SYS_I2C_ROCKCHIP=y
>  CONFIG_MISC=y
> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>  CONFIG_SPL_TINY_MEMSET=y
>  CONFIG_ERRNO_STR=y
> +CONFIG_EFI_CAPSULE_ON_DISK=y
> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/configs/rock-pi-4c-rk3399_defconfig b/configs/rock-pi-4c-rk3399_defconfig
> index bda4b70dbf..4c89e7e918 100644
> --- a/configs/rock-pi-4c-rk3399_defconfig
> +++ b/configs/rock-pi-4c-rk3399_defconfig
> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
> +CONFIG_CMD_DFU=y
>  CONFIG_CMD_GPT=y
>  CONFIG_CMD_MMC=y
>  CONFIG_CMD_PCI=y
> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>  CONFIG_ENV_IS_IN_MMC=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_DFU_MMC=y
>  CONFIG_ROCKCHIP_GPIO=y
>  CONFIG_SYS_I2C_ROCKCHIP=y
>  CONFIG_MISC=y
> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>  CONFIG_SPL_TINY_MEMSET=y
>  CONFIG_ERRNO_STR=y
> +CONFIG_EFI_CAPSULE_ON_DISK=y
> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h
> index 8e13737666..28b514a4ea 100644
> --- a/include/configs/rk3399_common.h
> +++ b/include/configs/rk3399_common.h
> @@ -38,6 +38,22 @@
>  #define CONFIG_SYS_SDRAM_BASE		0
>  #define SDRAM_MAX_SIZE			0xf8000000
>  
> +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
> +	EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
> +		 0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
> +
> +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
> +	EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
> +		 0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
> +
> +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
> +	EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
> +		 0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
> +
> +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
> +	EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
> +		 0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
> +
>  #ifndef CONFIG_SPL_BUILD
>  
>  #define ENV_MEM_LAYOUT_SETTINGS \

Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (RockPi4B)

Thanks,

On Fri, May 13, 2022 at 7:50 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Add support for updating the idbloader and u-boot images through the
> UEFI capsule update functionality. Enable the modules required for
> supporting the functionality.
>
> The implementation is for the updatable images placed on a GPT
> partitioned storage device. With the GPT partition devices, the
> partition information can be obtained at runtime, and hence the
> dfu_alt_info variable needed for the updates is being populated at
> runtime.
>
> This requires the partitions containing the idbloader and u-boot
> images to be created with the PartitionTypeGUID values set to the
> corresponding image type GUID values defined in the platform's config
> header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
> ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).

I think it would be useful to break this patch down into more than one
patch. At least the core changes to evb_rk3399/evb-rk3399.c and the
board specific changes. Also given these changes arre likely of
interest to other boards should  they go to somewhere more central
than evb_rk3399/evb-rk3399.c to enable less code duplication across
rk3399 or even rockchip devices as a whole?

Peter

> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  arch/arm/mach-rockchip/Kconfig         |   1 +
>  board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
>  configs/rock-pi-4-rk3399_defconfig     |   4 +
>  configs/rock-pi-4c-rk3399_defconfig    |   4 +
>  include/configs/rk3399_common.h        |  16 +++
>  5 files changed, 214 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index 18aff5480b..6d13e7b92e 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
>         select DM_PMIC
>         select DM_REGULATOR_FIXED
>         select BOARD_LATE_INIT
> +       imply PARTITION_TYPE_GUID
>         imply PRE_CONSOLE_BUFFER
>         imply ROCKCHIP_COMMON_BOARD
>         imply ROCKCHIP_SDRAM_COMMON
> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
> index abb76585cf..cc976d9471 100644
> --- a/board/rockchip/evb_rk3399/evb-rk3399.c
> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
> @@ -5,11 +5,27 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <efi_loader.h>
>  #include <init.h>
>  #include <log.h>
> +#include <mmc.h>
> +#include <part.h>
> +#include <uuid.h>
>  #include <asm/arch-rockchip/periph.h>
>  #include <power/regulator.h>
>
> +#define ROCKPI4_UPDATABLE_IMAGES       2
> +
> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
> +
> +struct efi_capsule_update_info update_info = {
> +       .images = fw_images,
> +};
> +
> +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
> +#endif
> +
>  #ifndef CONFIG_SPL_BUILD
>  int board_early_init_f(void)
>  {
> @@ -29,4 +45,176 @@ int board_early_init_f(void)
>  out:
>         return 0;
>  }
> -#endif
> +
> +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION)
> +
> +#define DFU_ALT_BUF_LEN SZ_1K
> +
> +static bool board_is_rockpi_4b(void)
> +{
> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> +               of_machine_is_compatible("radxa,rockpi4b");
> +}
> +
> +static bool board_is_rockpi_4c(void)
> +{
> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> +               of_machine_is_compatible("radxa,rockpi4c");
> +}
> +
> +static bool updatable_image(struct disk_partition *info)
> +{
> +       int i;
> +       bool ret = false;
> +       efi_guid_t image_type_guid;
> +
> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> +                       UUID_STR_FORMAT_GUID);
> +
> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> +                       ret = true;
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +static void set_image_index(struct disk_partition *info, int index)
> +{
> +       int i;
> +       efi_guid_t image_type_guid;
> +
> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> +                       UUID_STR_FORMAT_GUID);
> +
> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> +                       fw_images[i].image_index = index;
> +                       break;
> +               }
> +       }
> +}
> +
> +static int get_mmc_desc(struct blk_desc **desc)
> +{
> +       int ret;
> +       struct mmc *mmc;
> +       struct udevice *dev;
> +
> +       /*
> +        * For now the firmware images are assumed to
> +        * be on the SD card
> +        */
> +       ret = uclass_get_device(UCLASS_MMC, 1, &dev);
> +       if (ret)
> +               return -1;
> +
> +       mmc = mmc_get_mmc_dev(dev);
> +       if (!mmc)
> +               return -1;
> +
> +       if (mmc_init(mmc))
> +               return -1;
> +
> +       *desc = mmc_get_blk_desc(mmc);
> +       if (!*desc)
> +               return -1;
> +
> +       return 0;
> +}
> +
> +void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +       const char *name;
> +       bool first = true;
> +       int p, len, devnum, ret;
> +       char buf[DFU_ALT_BUF_LEN];
> +       struct disk_partition info;
> +       struct blk_desc *desc = NULL;
> +
> +       ret = get_mmc_desc(&desc);
> +       if (ret)
> +               return;
> +
> +       memset(buf, 0, sizeof(buf));
> +       name = blk_get_if_type_name(desc->if_type);
> +       devnum = desc->devnum;
> +       len = strlen(buf);
> +
> +       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> +                        "%s %d=", name, devnum);
> +
> +       for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +               if (part_get_info(desc, p, &info))
> +                       continue;
> +
> +               /* Add entry to dfu_alt_info only for updatable images */
> +               if (updatable_image(&info)) {
> +                       if (!first)
> +                               len += snprintf(buf + len,
> +                                               DFU_ALT_BUF_LEN - len, ";");
> +
> +                       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> +                                       "%s%d_%s part %d %d",
> +                                       name, devnum, info.name, devnum, p);
> +                       first = false;
> +               }
> +       }
> +
> +       log_debug("dfu_alt_info => %s\n", buf);
> +       env_set("dfu_alt_info", buf);
> +}
> +
> +int rk_board_late_init(void)
> +{
> +       int p, i, ret;
> +       struct disk_partition info;
> +       struct blk_desc *desc = NULL;
> +
> +       if (board_is_rockpi_4b()) {
> +               efi_guid_t idbldr_image_type_guid =
> +                       ROCKPI_4B_IDBLOADER_IMAGE_GUID;
> +               efi_guid_t uboot_image_type_guid = ROCKPI_4B_UBOOT_IMAGE_GUID;
> +
> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> +
> +               fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
> +               fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
> +       } else if (board_is_rockpi_4c()) {
> +               efi_guid_t idbldr_image_type_guid =
> +                       ROCKPI_4C_IDBLOADER_IMAGE_GUID;
> +               efi_guid_t uboot_image_type_guid = ROCKPI_4C_UBOOT_IMAGE_GUID;
> +
> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> +
> +               fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
> +               fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
> +       }
> +
> +       ret = get_mmc_desc(&desc);
> +       if (ret)
> +               return ret;
> +
> +       for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +               if (part_get_info(desc, p, &info))
> +                       continue;
> +
> +               /*
> +                * Since we have a GPT partitioned device, the updatable
> +                * images could be stored in any order. Populate the
> +                * image_index at runtime.
> +                */
> +               if (updatable_image(&info)) {
> +                       set_image_index(&info, i);
> +                       i++;
> +               }
> +       }
> +
> +       return 0;
> +}
> +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
> +#endif /* !CONFIG_SPL_BUILD */
> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
> index 80d1e63b59..ca3e217603 100644
> --- a/configs/rock-pi-4-rk3399_defconfig
> +++ b/configs/rock-pi-4-rk3399_defconfig
> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
> +CONFIG_CMD_DFU=y
>  CONFIG_CMD_GPT=y
>  CONFIG_CMD_MMC=y
>  CONFIG_CMD_PCI=y
> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>  CONFIG_ENV_IS_IN_MMC=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_DFU_MMC=y
>  CONFIG_ROCKCHIP_GPIO=y
>  CONFIG_SYS_I2C_ROCKCHIP=y
>  CONFIG_MISC=y
> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>  CONFIG_SPL_TINY_MEMSET=y
>  CONFIG_ERRNO_STR=y
> +CONFIG_EFI_CAPSULE_ON_DISK=y
> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/configs/rock-pi-4c-rk3399_defconfig b/configs/rock-pi-4c-rk3399_defconfig
> index bda4b70dbf..4c89e7e918 100644
> --- a/configs/rock-pi-4c-rk3399_defconfig
> +++ b/configs/rock-pi-4c-rk3399_defconfig
> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
> +CONFIG_CMD_DFU=y
>  CONFIG_CMD_GPT=y
>  CONFIG_CMD_MMC=y
>  CONFIG_CMD_PCI=y
> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>  CONFIG_ENV_IS_IN_MMC=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_DFU_MMC=y
>  CONFIG_ROCKCHIP_GPIO=y
>  CONFIG_SYS_I2C_ROCKCHIP=y
>  CONFIG_MISC=y
> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>  CONFIG_SPL_TINY_MEMSET=y
>  CONFIG_ERRNO_STR=y
> +CONFIG_EFI_CAPSULE_ON_DISK=y
> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h
> index 8e13737666..28b514a4ea 100644
> --- a/include/configs/rk3399_common.h
> +++ b/include/configs/rk3399_common.h
> @@ -38,6 +38,22 @@
>  #define CONFIG_SYS_SDRAM_BASE          0
>  #define SDRAM_MAX_SIZE                 0xf8000000
>
> +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
> +       EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
> +                0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
> +
> +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
> +       EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
> +                0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
> +
> +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
> +       EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
> +                0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
> +
> +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
> +       EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
> +                0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
> +
>  #ifndef CONFIG_SPL_BUILD
>
>  #define ENV_MEM_LAYOUT_SETTINGS \
> --
> 2.25.1
>

hi Peter,

On Sat, 14 May 2022 at 13:44, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> On Fri, May 13, 2022 at 7:50 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > Add support for updating the idbloader and u-boot images through the
> > UEFI capsule update functionality. Enable the modules required for
> > supporting the functionality.
> >
> > The implementation is for the updatable images placed on a GPT
> > partitioned storage device. With the GPT partition devices, the
> > partition information can be obtained at runtime, and hence the
> > dfu_alt_info variable needed for the updates is being populated at
> > runtime.
> >
> > This requires the partitions containing the idbloader and u-boot
> > images to be created with the PartitionTypeGUID values set to the
> > corresponding image type GUID values defined in the platform's config
> > header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
> > ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).
>
> I think it would be useful to break this patch down into more than one
> patch. At least the core changes to evb_rk3399/evb-rk3399.c and the
> board specific changes. Also given these changes arre likely of
> interest to other boards should  they go to somewhere more central
> than evb_rk3399/evb-rk3399.c to enable less code duplication across
> rk3399 or even rockchip devices as a whole?

I believe I can move the logic to set dfu_alt_info to some place like
arch/arm/mach-rockchip/board.c. The definition of rk_board_late_init()
can stay in evb-rk3399.c since it is populating the capsule update
structures for the rk3399 family of platforms. Does that sound okay to
you? If so, I will make the changes and send a V2, or if  you have
some other solution in mind, please let me know. Thanks.

-sughosh

>
> Peter
>
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >  arch/arm/mach-rockchip/Kconfig         |   1 +
> >  board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
> >  configs/rock-pi-4-rk3399_defconfig     |   4 +
> >  configs/rock-pi-4c-rk3399_defconfig    |   4 +
> >  include/configs/rk3399_common.h        |  16 +++
> >  5 files changed, 214 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> > index 18aff5480b..6d13e7b92e 100644
> > --- a/arch/arm/mach-rockchip/Kconfig
> > +++ b/arch/arm/mach-rockchip/Kconfig
> > @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
> >         select DM_PMIC
> >         select DM_REGULATOR_FIXED
> >         select BOARD_LATE_INIT
> > +       imply PARTITION_TYPE_GUID
> >         imply PRE_CONSOLE_BUFFER
> >         imply ROCKCHIP_COMMON_BOARD
> >         imply ROCKCHIP_SDRAM_COMMON
> > diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
> > index abb76585cf..cc976d9471 100644
> > --- a/board/rockchip/evb_rk3399/evb-rk3399.c
> > +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
> > @@ -5,11 +5,27 @@
> >
> >  #include <common.h>
> >  #include <dm.h>
> > +#include <efi_loader.h>
> >  #include <init.h>
> >  #include <log.h>
> > +#include <mmc.h>
> > +#include <part.h>
> > +#include <uuid.h>
> >  #include <asm/arch-rockchip/periph.h>
> >  #include <power/regulator.h>
> >
> > +#define ROCKPI4_UPDATABLE_IMAGES       2
> > +
> > +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> > +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
> > +
> > +struct efi_capsule_update_info update_info = {
> > +       .images = fw_images,
> > +};
> > +
> > +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
> > +#endif
> > +
> >  #ifndef CONFIG_SPL_BUILD
> >  int board_early_init_f(void)
> >  {
> > @@ -29,4 +45,176 @@ int board_early_init_f(void)
> >  out:
> >         return 0;
> >  }
> > -#endif
> > +
> > +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION)
> > +
> > +#define DFU_ALT_BUF_LEN SZ_1K
> > +
> > +static bool board_is_rockpi_4b(void)
> > +{
> > +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> > +               of_machine_is_compatible("radxa,rockpi4b");
> > +}
> > +
> > +static bool board_is_rockpi_4c(void)
> > +{
> > +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> > +               of_machine_is_compatible("radxa,rockpi4c");
> > +}
> > +
> > +static bool updatable_image(struct disk_partition *info)
> > +{
> > +       int i;
> > +       bool ret = false;
> > +       efi_guid_t image_type_guid;
> > +
> > +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> > +                       UUID_STR_FORMAT_GUID);
> > +
> > +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> > +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> > +                       ret = true;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static void set_image_index(struct disk_partition *info, int index)
> > +{
> > +       int i;
> > +       efi_guid_t image_type_guid;
> > +
> > +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> > +                       UUID_STR_FORMAT_GUID);
> > +
> > +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> > +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> > +                       fw_images[i].image_index = index;
> > +                       break;
> > +               }
> > +       }
> > +}
> > +
> > +static int get_mmc_desc(struct blk_desc **desc)
> > +{
> > +       int ret;
> > +       struct mmc *mmc;
> > +       struct udevice *dev;
> > +
> > +       /*
> > +        * For now the firmware images are assumed to
> > +        * be on the SD card
> > +        */
> > +       ret = uclass_get_device(UCLASS_MMC, 1, &dev);
> > +       if (ret)
> > +               return -1;
> > +
> > +       mmc = mmc_get_mmc_dev(dev);
> > +       if (!mmc)
> > +               return -1;
> > +
> > +       if (mmc_init(mmc))
> > +               return -1;
> > +
> > +       *desc = mmc_get_blk_desc(mmc);
> > +       if (!*desc)
> > +               return -1;
> > +
> > +       return 0;
> > +}
> > +
> > +void set_dfu_alt_info(char *interface, char *devstr)
> > +{
> > +       const char *name;
> > +       bool first = true;
> > +       int p, len, devnum, ret;
> > +       char buf[DFU_ALT_BUF_LEN];
> > +       struct disk_partition info;
> > +       struct blk_desc *desc = NULL;
> > +
> > +       ret = get_mmc_desc(&desc);
> > +       if (ret)
> > +               return;
> > +
> > +       memset(buf, 0, sizeof(buf));
> > +       name = blk_get_if_type_name(desc->if_type);
> > +       devnum = desc->devnum;
> > +       len = strlen(buf);
> > +
> > +       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> > +                        "%s %d=", name, devnum);
> > +
> > +       for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > +               if (part_get_info(desc, p, &info))
> > +                       continue;
> > +
> > +               /* Add entry to dfu_alt_info only for updatable images */
> > +               if (updatable_image(&info)) {
> > +                       if (!first)
> > +                               len += snprintf(buf + len,
> > +                                               DFU_ALT_BUF_LEN - len, ";");
> > +
> > +                       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> > +                                       "%s%d_%s part %d %d",
> > +                                       name, devnum, info.name, devnum, p);
> > +                       first = false;
> > +               }
> > +       }
> > +
> > +       log_debug("dfu_alt_info => %s\n", buf);
> > +       env_set("dfu_alt_info", buf);
> > +}
> > +
> > +int rk_board_late_init(void)
> > +{
> > +       int p, i, ret;
> > +       struct disk_partition info;
> > +       struct blk_desc *desc = NULL;
> > +
> > +       if (board_is_rockpi_4b()) {
> > +               efi_guid_t idbldr_image_type_guid =
> > +                       ROCKPI_4B_IDBLOADER_IMAGE_GUID;
> > +               efi_guid_t uboot_image_type_guid = ROCKPI_4B_UBOOT_IMAGE_GUID;
> > +
> > +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> > +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> > +
> > +               fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
> > +               fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
> > +       } else if (board_is_rockpi_4c()) {
> > +               efi_guid_t idbldr_image_type_guid =
> > +                       ROCKPI_4C_IDBLOADER_IMAGE_GUID;
> > +               efi_guid_t uboot_image_type_guid = ROCKPI_4C_UBOOT_IMAGE_GUID;
> > +
> > +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> > +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> > +
> > +               fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
> > +               fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
> > +       }
> > +
> > +       ret = get_mmc_desc(&desc);
> > +       if (ret)
> > +               return ret;
> > +
> > +       for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> > +               if (part_get_info(desc, p, &info))
> > +                       continue;
> > +
> > +               /*
> > +                * Since we have a GPT partitioned device, the updatable
> > +                * images could be stored in any order. Populate the
> > +                * image_index at runtime.
> > +                */
> > +               if (updatable_image(&info)) {
> > +                       set_image_index(&info, i);
> > +                       i++;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
> > +#endif /* !CONFIG_SPL_BUILD */
> > diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
> > index 80d1e63b59..ca3e217603 100644
> > --- a/configs/rock-pi-4-rk3399_defconfig
> > +++ b/configs/rock-pi-4-rk3399_defconfig
> > @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
> >  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
> >  CONFIG_TPL=y
> >  CONFIG_CMD_BOOTZ=y
> > +CONFIG_CMD_DFU=y
> >  CONFIG_CMD_GPT=y
> >  CONFIG_CMD_MMC=y
> >  CONFIG_CMD_PCI=y
> > @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
> >  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> >  CONFIG_ENV_IS_IN_MMC=y
> >  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> > +CONFIG_DFU_MMC=y
> >  CONFIG_ROCKCHIP_GPIO=y
> >  CONFIG_SYS_I2C_ROCKCHIP=y
> >  CONFIG_MISC=y
> > @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
> >  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
> >  CONFIG_SPL_TINY_MEMSET=y
> >  CONFIG_ERRNO_STR=y
> > +CONFIG_EFI_CAPSULE_ON_DISK=y
> > +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > diff --git a/configs/rock-pi-4c-rk3399_defconfig b/configs/rock-pi-4c-rk3399_defconfig
> > index bda4b70dbf..4c89e7e918 100644
> > --- a/configs/rock-pi-4c-rk3399_defconfig
> > +++ b/configs/rock-pi-4c-rk3399_defconfig
> > @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
> >  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
> >  CONFIG_TPL=y
> >  CONFIG_CMD_BOOTZ=y
> > +CONFIG_CMD_DFU=y
> >  CONFIG_CMD_GPT=y
> >  CONFIG_CMD_MMC=y
> >  CONFIG_CMD_PCI=y
> > @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
> >  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> >  CONFIG_ENV_IS_IN_MMC=y
> >  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> > +CONFIG_DFU_MMC=y
> >  CONFIG_ROCKCHIP_GPIO=y
> >  CONFIG_SYS_I2C_ROCKCHIP=y
> >  CONFIG_MISC=y
> > @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
> >  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
> >  CONFIG_SPL_TINY_MEMSET=y
> >  CONFIG_ERRNO_STR=y
> > +CONFIG_EFI_CAPSULE_ON_DISK=y
> > +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> > diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h
> > index 8e13737666..28b514a4ea 100644
> > --- a/include/configs/rk3399_common.h
> > +++ b/include/configs/rk3399_common.h
> > @@ -38,6 +38,22 @@
> >  #define CONFIG_SYS_SDRAM_BASE          0
> >  #define SDRAM_MAX_SIZE                 0xf8000000
> >
> > +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
> > +       EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
> > +                0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
> > +
> > +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
> > +       EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
> > +                0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
> > +
> > +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
> > +       EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
> > +                0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
> > +
> > +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
> > +       EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
> > +                0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
> > +
> >  #ifndef CONFIG_SPL_BUILD
> >
> >  #define ENV_MEM_LAYOUT_SETTINGS \
> > --
> > 2.25.1
> >

Hi Sughosh,

On Fri, 13 May 2022 at 07:50, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:

> Add support for updating the idbloader and u-boot images through the
> UEFI capsule update functionality. Enable the modules required for
> supporting the functionality.
>
> The implementation is for the updatable images placed on a GPT
> partitioned storage device. With the GPT partition devices, the
> partition information can be obtained at runtime, and hence the
> dfu_alt_info variable needed for the updates is being populated at
> runtime.
>
> This requires the partitions containing the idbloader and u-boot
> images to be created with the PartitionTypeGUID values set to the
> corresponding image type GUID values defined in the platform's config
> header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
> ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>

Tested-by: Peter Griffin <peter.griffin@linaro.org> (RockPi4B)


>  arch/arm/mach-rockchip/Kconfig         |   1 +
>  board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
>  configs/rock-pi-4-rk3399_defconfig     |   4 +
>  configs/rock-pi-4c-rk3399_defconfig    |   4 +
>  include/configs/rk3399_common.h        |  16 +++
>  5 files changed, 214 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-rockchip/Kconfig
> b/arch/arm/mach-rockchip/Kconfig
> index 18aff5480b..6d13e7b92e 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
>         select DM_PMIC
>         select DM_REGULATOR_FIXED
>         select BOARD_LATE_INIT
> +       imply PARTITION_TYPE_GUID
>         imply PRE_CONSOLE_BUFFER
>         imply ROCKCHIP_COMMON_BOARD
>         imply ROCKCHIP_SDRAM_COMMON
> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c
> b/board/rockchip/evb_rk3399/evb-rk3399.c
> index abb76585cf..cc976d9471 100644
> --- a/board/rockchip/evb_rk3399/evb-rk3399.c
> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
> @@ -5,11 +5,27 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <efi_loader.h>
>  #include <init.h>
>  #include <log.h>
> +#include <mmc.h>
> +#include <part.h>
> +#include <uuid.h>
>  #include <asm/arch-rockchip/periph.h>
>  #include <power/regulator.h>
>
> +#define ROCKPI4_UPDATABLE_IMAGES       2
> +
> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
> +
> +struct efi_capsule_update_info update_info = {
> +       .images = fw_images,
> +};
> +
> +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
> +#endif
> +
>  #ifndef CONFIG_SPL_BUILD
>  int board_early_init_f(void)
>  {
> @@ -29,4 +45,176 @@ int board_early_init_f(void)
>  out:
>         return 0;
>  }
> -#endif
> +
> +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) &&
> defined(CONFIG_EFI_PARTITION)
> +
> +#define DFU_ALT_BUF_LEN SZ_1K
> +
> +static bool board_is_rockpi_4b(void)
> +{
> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> +               of_machine_is_compatible("radxa,rockpi4b");
> +}
> +
> +static bool board_is_rockpi_4c(void)
> +{
> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> +               of_machine_is_compatible("radxa,rockpi4c");
> +}
> +
> +static bool updatable_image(struct disk_partition *info)
> +{
> +       int i;
> +       bool ret = false;
> +       efi_guid_t image_type_guid;
> +
> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> +                       UUID_STR_FORMAT_GUID);
> +
> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> +               if (!guidcmp(&fw_images[i].image_type_id,
> &image_type_guid)) {
> +                       ret = true;
> +                       break;
> +               }
> +       }
> +
> +       return ret;
> +}
> +
> +static void set_image_index(struct disk_partition *info, int index)
> +{
> +       int i;
> +       efi_guid_t image_type_guid;
> +
> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> +                       UUID_STR_FORMAT_GUID);
> +
> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> +               if (!guidcmp(&fw_images[i].image_type_id,
> &image_type_guid)) {
> +                       fw_images[i].image_index = index;
> +                       break;
> +               }
> +       }
> +}
> +
> +static int get_mmc_desc(struct blk_desc **desc)
> +{
> +       int ret;
> +       struct mmc *mmc;
> +       struct udevice *dev;
> +
> +       /*
> +        * For now the firmware images are assumed to
> +        * be on the SD card
> +        */
> +       ret = uclass_get_device(UCLASS_MMC, 1, &dev);
> +       if (ret)
> +               return -1;
> +
> +       mmc = mmc_get_mmc_dev(dev);
> +       if (!mmc)
> +               return -1;
> +
> +       if (mmc_init(mmc))
> +               return -1;
> +
> +       *desc = mmc_get_blk_desc(mmc);
> +       if (!*desc)
> +               return -1;
> +
> +       return 0;
> +}
> +
> +void set_dfu_alt_info(char *interface, char *devstr)
> +{
> +       const char *name;
> +       bool first = true;
> +       int p, len, devnum, ret;
> +       char buf[DFU_ALT_BUF_LEN];
> +       struct disk_partition info;
> +       struct blk_desc *desc = NULL;
> +
> +       ret = get_mmc_desc(&desc);
> +       if (ret)
> +               return;
> +
> +       memset(buf, 0, sizeof(buf));
> +       name = blk_get_if_type_name(desc->if_type);
> +       devnum = desc->devnum;
> +       len = strlen(buf);
> +
> +       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> +                        "%s %d=", name, devnum);
> +
> +       for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +               if (part_get_info(desc, p, &info))
> +                       continue;
> +
> +               /* Add entry to dfu_alt_info only for updatable images */
> +               if (updatable_image(&info)) {
> +                       if (!first)
> +                               len += snprintf(buf + len,
> +                                               DFU_ALT_BUF_LEN - len,
> ";");
> +
> +                       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> +                                       "%s%d_%s part %d %d",
> +                                       name, devnum, info.name, devnum,
> p);
> +                       first = false;
> +               }
> +       }
> +
> +       log_debug("dfu_alt_info => %s\n", buf);
> +       env_set("dfu_alt_info", buf);
> +}
> +
> +int rk_board_late_init(void)
> +{
> +       int p, i, ret;
> +       struct disk_partition info;
> +       struct blk_desc *desc = NULL;
> +
> +       if (board_is_rockpi_4b()) {
> +               efi_guid_t idbldr_image_type_guid =
> +                       ROCKPI_4B_IDBLOADER_IMAGE_GUID;
> +               efi_guid_t uboot_image_type_guid =
> ROCKPI_4B_UBOOT_IMAGE_GUID;
> +
> +               guidcpy(&fw_images[0].image_type_id,
> &idbldr_image_type_guid);
> +               guidcpy(&fw_images[1].image_type_id,
> &uboot_image_type_guid);
> +
> +               fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
> +               fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
> +       } else if (board_is_rockpi_4c()) {
> +               efi_guid_t idbldr_image_type_guid =
> +                       ROCKPI_4C_IDBLOADER_IMAGE_GUID;
> +               efi_guid_t uboot_image_type_guid =
> ROCKPI_4C_UBOOT_IMAGE_GUID;
> +
> +               guidcpy(&fw_images[0].image_type_id,
> &idbldr_image_type_guid);
> +               guidcpy(&fw_images[1].image_type_id,
> &uboot_image_type_guid);
> +
> +               fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
> +               fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
> +       }
> +
> +       ret = get_mmc_desc(&desc);
> +       if (ret)
> +               return ret;
> +
> +       for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> +               if (part_get_info(desc, p, &info))
> +                       continue;
> +
> +               /*
> +                * Since we have a GPT partitioned device, the updatable
> +                * images could be stored in any order. Populate the
> +                * image_index at runtime.
> +                */
> +               if (updatable_image(&info)) {
> +                       set_image_index(&info, i);
> +                       i++;
> +               }
> +       }
> +
> +       return 0;
> +}
> +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
> +#endif /* !CONFIG_SPL_BUILD */
> diff --git a/configs/rock-pi-4-rk3399_defconfig
> b/configs/rock-pi-4-rk3399_defconfig
> index 80d1e63b59..ca3e217603 100644
> --- a/configs/rock-pi-4-rk3399_defconfig
> +++ b/configs/rock-pi-4-rk3399_defconfig
> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
> +CONFIG_CMD_DFU=y
>  CONFIG_CMD_GPT=y
>  CONFIG_CMD_MMC=y
>  CONFIG_CMD_PCI=y
> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names
> interrupt-parent assigned-clocks assigned-clock-rates
> assigned-clock-parents"
>  CONFIG_ENV_IS_IN_MMC=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_DFU_MMC=y
>  CONFIG_ROCKCHIP_GPIO=y
>  CONFIG_SYS_I2C_ROCKCHIP=y
>  CONFIG_MISC=y
> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>  CONFIG_SPL_TINY_MEMSET=y
>  CONFIG_ERRNO_STR=y
> +CONFIG_EFI_CAPSULE_ON_DISK=y
> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/configs/rock-pi-4c-rk3399_defconfig
> b/configs/rock-pi-4c-rk3399_defconfig
> index bda4b70dbf..4c89e7e918 100644
> --- a/configs/rock-pi-4c-rk3399_defconfig
> +++ b/configs/rock-pi-4c-rk3399_defconfig
> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>  CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>  CONFIG_TPL=y
>  CONFIG_CMD_BOOTZ=y
> +CONFIG_CMD_DFU=y
>  CONFIG_CMD_GPT=y
>  CONFIG_CMD_MMC=y
>  CONFIG_CMD_PCI=y
> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>  CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names
> interrupt-parent assigned-clocks assigned-clock-rates
> assigned-clock-parents"
>  CONFIG_ENV_IS_IN_MMC=y
>  CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_DFU_MMC=y
>  CONFIG_ROCKCHIP_GPIO=y
>  CONFIG_SYS_I2C_ROCKCHIP=y
>  CONFIG_MISC=y
> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>  CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>  CONFIG_SPL_TINY_MEMSET=y
>  CONFIG_ERRNO_STR=y
> +CONFIG_EFI_CAPSULE_ON_DISK=y
> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> diff --git a/include/configs/rk3399_common.h
> b/include/configs/rk3399_common.h
> index 8e13737666..28b514a4ea 100644
> --- a/include/configs/rk3399_common.h
> +++ b/include/configs/rk3399_common.h
> @@ -38,6 +38,22 @@
>  #define CONFIG_SYS_SDRAM_BASE          0
>  #define SDRAM_MAX_SIZE                 0xf8000000
>
> +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
> +       EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
> +                0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
> +
> +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
> +       EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
> +                0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
> +
> +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
> +       EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
> +                0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
> +
> +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
> +       EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
> +                0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
> +
>  #ifndef CONFIG_SPL_BUILD
>
>  #define ENV_MEM_LAYOUT_SETTINGS \
> --
> 2.25.1
>
>

Hi Sughosh,

      Thanks for your patch.

On 2022/5/16 14:12, Sughosh Ganu wrote:
> hi Peter,
>
> On Sat, 14 May 2022 at 13:44, Peter Robinson <pbrobinson@gmail.com> wrote:
>> On Fri, May 13, 2022 at 7:50 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>>> Add support for updating the idbloader and u-boot images through the
>>> UEFI capsule update functionality. Enable the modules required for
>>> supporting the functionality.
>>>
>>> The implementation is for the updatable images placed on a GPT
>>> partitioned storage device. With the GPT partition devices, the
>>> partition information can be obtained at runtime, and hence the
>>> dfu_alt_info variable needed for the updates is being populated at
>>> runtime.
>>>
>>> This requires the partitions containing the idbloader and u-boot
>>> images to be created with the PartitionTypeGUID values set to the
>>> corresponding image type GUID values defined in the platform's config
>>> header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
>>> ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).
>> I think it would be useful to break this patch down into more than one
>> patch. At least the core changes to evb_rk3399/evb-rk3399.c and the
>> board specific changes. Also given these changes arre likely of
>> interest to other boards should  they go to somewhere more central
>> than evb_rk3399/evb-rk3399.c to enable less code duplication across
>> rk3399 or even rockchip devices as a whole?
> I believe I can move the logic to set dfu_alt_info to some place like
> arch/arm/mach-rockchip/board.c. The definition of rk_board_late_init()
> can stay in evb-rk3399.c since it is populating the capsule update
> structures for the rk3399 family of platforms. Does that sound okay to
> you? If so, I will make the changes and send a V2, or if  you have
> some other solution in mind, please let me know. Thanks.

Other than Peter's command, please make sure:

All the source code relate to rockpi should go to board/radxa instead of 
board/rockchip/evb-rk3399.c


Thanks,

- Kever

>
> -sughosh
>
>> Peter
>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>> ---
>>>   arch/arm/mach-rockchip/Kconfig         |   1 +
>>>   board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
>>>   configs/rock-pi-4-rk3399_defconfig     |   4 +
>>>   configs/rock-pi-4c-rk3399_defconfig    |   4 +
>>>   include/configs/rk3399_common.h        |  16 +++
>>>   5 files changed, 214 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
>>> index 18aff5480b..6d13e7b92e 100644
>>> --- a/arch/arm/mach-rockchip/Kconfig
>>> +++ b/arch/arm/mach-rockchip/Kconfig
>>> @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
>>>          select DM_PMIC
>>>          select DM_REGULATOR_FIXED
>>>          select BOARD_LATE_INIT
>>> +       imply PARTITION_TYPE_GUID
>>>          imply PRE_CONSOLE_BUFFER
>>>          imply ROCKCHIP_COMMON_BOARD
>>>          imply ROCKCHIP_SDRAM_COMMON
>>> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
>>> index abb76585cf..cc976d9471 100644
>>> --- a/board/rockchip/evb_rk3399/evb-rk3399.c
>>> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
>>> @@ -5,11 +5,27 @@
>>>
>>>   #include <common.h>
>>>   #include <dm.h>
>>> +#include <efi_loader.h>
>>>   #include <init.h>
>>>   #include <log.h>
>>> +#include <mmc.h>
>>> +#include <part.h>
>>> +#include <uuid.h>
>>>   #include <asm/arch-rockchip/periph.h>
>>>   #include <power/regulator.h>
>>>
>>> +#define ROCKPI4_UPDATABLE_IMAGES       2
>>> +
>>> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
>>> +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
>>> +
>>> +struct efi_capsule_update_info update_info = {
>>> +       .images = fw_images,
>>> +};
>>> +
>>> +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
>>> +#endif
>>> +
>>>   #ifndef CONFIG_SPL_BUILD
>>>   int board_early_init_f(void)
>>>   {
>>> @@ -29,4 +45,176 @@ int board_early_init_f(void)
>>>   out:
>>>          return 0;
>>>   }
>>> -#endif
>>> +
>>> +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION)
>>> +
>>> +#define DFU_ALT_BUF_LEN SZ_1K
>>> +
>>> +static bool board_is_rockpi_4b(void)
>>> +{
>>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
>>> +               of_machine_is_compatible("radxa,rockpi4b");
>>> +}
>>> +
>>> +static bool board_is_rockpi_4c(void)
>>> +{
>>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
>>> +               of_machine_is_compatible("radxa,rockpi4c");
>>> +}
>>> +
>>> +static bool updatable_image(struct disk_partition *info)
>>> +{
>>> +       int i;
>>> +       bool ret = false;
>>> +       efi_guid_t image_type_guid;
>>> +
>>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
>>> +                       UUID_STR_FORMAT_GUID);
>>> +
>>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
>>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
>>> +                       ret = true;
>>> +                       break;
>>> +               }
>>> +       }
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static void set_image_index(struct disk_partition *info, int index)
>>> +{
>>> +       int i;
>>> +       efi_guid_t image_type_guid;
>>> +
>>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
>>> +                       UUID_STR_FORMAT_GUID);
>>> +
>>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
>>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
>>> +                       fw_images[i].image_index = index;
>>> +                       break;
>>> +               }
>>> +       }
>>> +}
>>> +
>>> +static int get_mmc_desc(struct blk_desc **desc)
>>> +{
>>> +       int ret;
>>> +       struct mmc *mmc;
>>> +       struct udevice *dev;
>>> +
>>> +       /*
>>> +        * For now the firmware images are assumed to
>>> +        * be on the SD card
>>> +        */
>>> +       ret = uclass_get_device(UCLASS_MMC, 1, &dev);
>>> +       if (ret)
>>> +               return -1;
>>> +
>>> +       mmc = mmc_get_mmc_dev(dev);
>>> +       if (!mmc)
>>> +               return -1;
>>> +
>>> +       if (mmc_init(mmc))
>>> +               return -1;
>>> +
>>> +       *desc = mmc_get_blk_desc(mmc);
>>> +       if (!*desc)
>>> +               return -1;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +void set_dfu_alt_info(char *interface, char *devstr)
>>> +{
>>> +       const char *name;
>>> +       bool first = true;
>>> +       int p, len, devnum, ret;
>>> +       char buf[DFU_ALT_BUF_LEN];
>>> +       struct disk_partition info;
>>> +       struct blk_desc *desc = NULL;
>>> +
>>> +       ret = get_mmc_desc(&desc);
>>> +       if (ret)
>>> +               return;
>>> +
>>> +       memset(buf, 0, sizeof(buf));
>>> +       name = blk_get_if_type_name(desc->if_type);
>>> +       devnum = desc->devnum;
>>> +       len = strlen(buf);
>>> +
>>> +       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
>>> +                        "%s %d=", name, devnum);
>>> +
>>> +       for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
>>> +               if (part_get_info(desc, p, &info))
>>> +                       continue;
>>> +
>>> +               /* Add entry to dfu_alt_info only for updatable images */
>>> +               if (updatable_image(&info)) {
>>> +                       if (!first)
>>> +                               len += snprintf(buf + len,
>>> +                                               DFU_ALT_BUF_LEN - len, ";");
>>> +
>>> +                       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
>>> +                                       "%s%d_%s part %d %d",
>>> +                                       name, devnum, info.name, devnum, p);
>>> +                       first = false;
>>> +               }
>>> +       }
>>> +
>>> +       log_debug("dfu_alt_info => %s\n", buf);
>>> +       env_set("dfu_alt_info", buf);
>>> +}
>>> +
>>> +int rk_board_late_init(void)
>>> +{
>>> +       int p, i, ret;
>>> +       struct disk_partition info;
>>> +       struct blk_desc *desc = NULL;
>>> +
>>> +       if (board_is_rockpi_4b()) {
>>> +               efi_guid_t idbldr_image_type_guid =
>>> +                       ROCKPI_4B_IDBLOADER_IMAGE_GUID;
>>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4B_UBOOT_IMAGE_GUID;
>>> +
>>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
>>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
>>> +
>>> +               fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
>>> +               fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
>>> +       } else if (board_is_rockpi_4c()) {
>>> +               efi_guid_t idbldr_image_type_guid =
>>> +                       ROCKPI_4C_IDBLOADER_IMAGE_GUID;
>>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4C_UBOOT_IMAGE_GUID;
>>> +
>>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
>>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
>>> +
>>> +               fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
>>> +               fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
>>> +       }
>>> +
>>> +       ret = get_mmc_desc(&desc);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>> +       for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
>>> +               if (part_get_info(desc, p, &info))
>>> +                       continue;
>>> +
>>> +               /*
>>> +                * Since we have a GPT partitioned device, the updatable
>>> +                * images could be stored in any order. Populate the
>>> +                * image_index at runtime.
>>> +                */
>>> +               if (updatable_image(&info)) {
>>> +                       set_image_index(&info, i);
>>> +                       i++;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
>>> +#endif /* !CONFIG_SPL_BUILD */
>>> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
>>> index 80d1e63b59..ca3e217603 100644
>>> --- a/configs/rock-pi-4-rk3399_defconfig
>>> +++ b/configs/rock-pi-4-rk3399_defconfig
>>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>>>   CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>>>   CONFIG_TPL=y
>>>   CONFIG_CMD_BOOTZ=y
>>> +CONFIG_CMD_DFU=y
>>>   CONFIG_CMD_GPT=y
>>>   CONFIG_CMD_MMC=y
>>>   CONFIG_CMD_PCI=y
>>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>>>   CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>>>   CONFIG_ENV_IS_IN_MMC=y
>>>   CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>>> +CONFIG_DFU_MMC=y
>>>   CONFIG_ROCKCHIP_GPIO=y
>>>   CONFIG_SYS_I2C_ROCKCHIP=y
>>>   CONFIG_MISC=y
>>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>>>   CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>>>   CONFIG_SPL_TINY_MEMSET=y
>>>   CONFIG_ERRNO_STR=y
>>> +CONFIG_EFI_CAPSULE_ON_DISK=y
>>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
>>> diff --git a/configs/rock-pi-4c-rk3399_defconfig b/configs/rock-pi-4c-rk3399_defconfig
>>> index bda4b70dbf..4c89e7e918 100644
>>> --- a/configs/rock-pi-4c-rk3399_defconfig
>>> +++ b/configs/rock-pi-4c-rk3399_defconfig
>>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>>>   CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>>>   CONFIG_TPL=y
>>>   CONFIG_CMD_BOOTZ=y
>>> +CONFIG_CMD_DFU=y
>>>   CONFIG_CMD_GPT=y
>>>   CONFIG_CMD_MMC=y
>>>   CONFIG_CMD_PCI=y
>>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>>>   CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>>>   CONFIG_ENV_IS_IN_MMC=y
>>>   CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>>> +CONFIG_DFU_MMC=y
>>>   CONFIG_ROCKCHIP_GPIO=y
>>>   CONFIG_SYS_I2C_ROCKCHIP=y
>>>   CONFIG_MISC=y
>>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>>>   CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>>>   CONFIG_SPL_TINY_MEMSET=y
>>>   CONFIG_ERRNO_STR=y
>>> +CONFIG_EFI_CAPSULE_ON_DISK=y
>>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
>>> diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h
>>> index 8e13737666..28b514a4ea 100644
>>> --- a/include/configs/rk3399_common.h
>>> +++ b/include/configs/rk3399_common.h
>>> @@ -38,6 +38,22 @@
>>>   #define CONFIG_SYS_SDRAM_BASE          0
>>>   #define SDRAM_MAX_SIZE                 0xf8000000
>>>
>>> +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
>>> +       EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
>>> +                0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
>>> +
>>> +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
>>> +       EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
>>> +                0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
>>> +
>>> +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
>>> +       EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
>>> +                0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
>>> +
>>> +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
>>> +       EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
>>> +                0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
>>> +
>>>   #ifndef CONFIG_SPL_BUILD
>>>
>>>   #define ENV_MEM_LAYOUT_SETTINGS \
>>> --
>>> 2.25.1
>>>

hi Kever,

On Wed, 29 Jun 2022 at 09:11, Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Hi Sughosh,
>
>       Thanks for your patch.
>
> On 2022/5/16 14:12, Sughosh Ganu wrote:
> > hi Peter,
> >
> > On Sat, 14 May 2022 at 13:44, Peter Robinson <pbrobinson@gmail.com> wrote:
> >> On Fri, May 13, 2022 at 7:50 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >>> Add support for updating the idbloader and u-boot images through the
> >>> UEFI capsule update functionality. Enable the modules required for
> >>> supporting the functionality.
> >>>
> >>> The implementation is for the updatable images placed on a GPT
> >>> partitioned storage device. With the GPT partition devices, the
> >>> partition information can be obtained at runtime, and hence the
> >>> dfu_alt_info variable needed for the updates is being populated at
> >>> runtime.
> >>>
> >>> This requires the partitions containing the idbloader and u-boot
> >>> images to be created with the PartitionTypeGUID values set to the
> >>> corresponding image type GUID values defined in the platform's config
> >>> header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
> >>> ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).
> >> I think it would be useful to break this patch down into more than one
> >> patch. At least the core changes to evb_rk3399/evb-rk3399.c and the
> >> board specific changes. Also given these changes arre likely of
> >> interest to other boards should  they go to somewhere more central
> >> than evb_rk3399/evb-rk3399.c to enable less code duplication across
> >> rk3399 or even rockchip devices as a whole?
> > I believe I can move the logic to set dfu_alt_info to some place like
> > arch/arm/mach-rockchip/board.c. The definition of rk_board_late_init()
> > can stay in evb-rk3399.c since it is populating the capsule update
> > structures for the rk3399 family of platforms. Does that sound okay to
> > you? If so, I will make the changes and send a V2, or if  you have
> > some other solution in mind, please let me know. Thanks.
>
> Other than Peter's command, please make sure:
>
> All the source code relate to rockpi should go to board/radxa instead of
> board/rockchip/evb-rk3399.c

Apologies for the late reply. I was working on some other stuff and
hence this got put on the backburner. With regards to your review
comment of putting the board code under board/radxa/, how do I get the
code under board/radxa to compile. The RockPi4 boards have
CONFIG_SYS_VENDOR="rockchip" and CONFIG_SYS_BOARD="evb_rk3399". With
these configs, code under board/rockchip/evb_rk3399/ gets built. Is
there some way I can get code under some other board directory to
build?

I was thinking of putting the common code under
arch/arm/mach-rockchip/board.c, and the board specific declarations
under board/rockchip/evb_rk3399/evb_rk3399.c, along with the splitting
of the changes in two patches. Is that fine with you?

-sughosh

>
>
> Thanks,
>
> - Kever
>
> >
> > -sughosh
> >
> >> Peter
> >>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>> ---
> >>>   arch/arm/mach-rockchip/Kconfig         |   1 +
> >>>   board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
> >>>   configs/rock-pi-4-rk3399_defconfig     |   4 +
> >>>   configs/rock-pi-4c-rk3399_defconfig    |   4 +
> >>>   include/configs/rk3399_common.h        |  16 +++
> >>>   5 files changed, 214 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> >>> index 18aff5480b..6d13e7b92e 100644
> >>> --- a/arch/arm/mach-rockchip/Kconfig
> >>> +++ b/arch/arm/mach-rockchip/Kconfig
> >>> @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
> >>>          select DM_PMIC
> >>>          select DM_REGULATOR_FIXED
> >>>          select BOARD_LATE_INIT
> >>> +       imply PARTITION_TYPE_GUID
> >>>          imply PRE_CONSOLE_BUFFER
> >>>          imply ROCKCHIP_COMMON_BOARD
> >>>          imply ROCKCHIP_SDRAM_COMMON
> >>> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
> >>> index abb76585cf..cc976d9471 100644
> >>> --- a/board/rockchip/evb_rk3399/evb-rk3399.c
> >>> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
> >>> @@ -5,11 +5,27 @@
> >>>
> >>>   #include <common.h>
> >>>   #include <dm.h>
> >>> +#include <efi_loader.h>
> >>>   #include <init.h>
> >>>   #include <log.h>
> >>> +#include <mmc.h>
> >>> +#include <part.h>
> >>> +#include <uuid.h>
> >>>   #include <asm/arch-rockchip/periph.h>
> >>>   #include <power/regulator.h>
> >>>
> >>> +#define ROCKPI4_UPDATABLE_IMAGES       2
> >>> +
> >>> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> >>> +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
> >>> +
> >>> +struct efi_capsule_update_info update_info = {
> >>> +       .images = fw_images,
> >>> +};
> >>> +
> >>> +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
> >>> +#endif
> >>> +
> >>>   #ifndef CONFIG_SPL_BUILD
> >>>   int board_early_init_f(void)
> >>>   {
> >>> @@ -29,4 +45,176 @@ int board_early_init_f(void)
> >>>   out:
> >>>          return 0;
> >>>   }
> >>> -#endif
> >>> +
> >>> +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION)
> >>> +
> >>> +#define DFU_ALT_BUF_LEN SZ_1K
> >>> +
> >>> +static bool board_is_rockpi_4b(void)
> >>> +{
> >>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> >>> +               of_machine_is_compatible("radxa,rockpi4b");
> >>> +}
> >>> +
> >>> +static bool board_is_rockpi_4c(void)
> >>> +{
> >>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> >>> +               of_machine_is_compatible("radxa,rockpi4c");
> >>> +}
> >>> +
> >>> +static bool updatable_image(struct disk_partition *info)
> >>> +{
> >>> +       int i;
> >>> +       bool ret = false;
> >>> +       efi_guid_t image_type_guid;
> >>> +
> >>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> >>> +                       UUID_STR_FORMAT_GUID);
> >>> +
> >>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> >>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> >>> +                       ret = true;
> >>> +                       break;
> >>> +               }
> >>> +       }
> >>> +
> >>> +       return ret;
> >>> +}
> >>> +
> >>> +static void set_image_index(struct disk_partition *info, int index)
> >>> +{
> >>> +       int i;
> >>> +       efi_guid_t image_type_guid;
> >>> +
> >>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> >>> +                       UUID_STR_FORMAT_GUID);
> >>> +
> >>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> >>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> >>> +                       fw_images[i].image_index = index;
> >>> +                       break;
> >>> +               }
> >>> +       }
> >>> +}
> >>> +
> >>> +static int get_mmc_desc(struct blk_desc **desc)
> >>> +{
> >>> +       int ret;
> >>> +       struct mmc *mmc;
> >>> +       struct udevice *dev;
> >>> +
> >>> +       /*
> >>> +        * For now the firmware images are assumed to
> >>> +        * be on the SD card
> >>> +        */
> >>> +       ret = uclass_get_device(UCLASS_MMC, 1, &dev);
> >>> +       if (ret)
> >>> +               return -1;
> >>> +
> >>> +       mmc = mmc_get_mmc_dev(dev);
> >>> +       if (!mmc)
> >>> +               return -1;
> >>> +
> >>> +       if (mmc_init(mmc))
> >>> +               return -1;
> >>> +
> >>> +       *desc = mmc_get_blk_desc(mmc);
> >>> +       if (!*desc)
> >>> +               return -1;
> >>> +
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +void set_dfu_alt_info(char *interface, char *devstr)
> >>> +{
> >>> +       const char *name;
> >>> +       bool first = true;
> >>> +       int p, len, devnum, ret;
> >>> +       char buf[DFU_ALT_BUF_LEN];
> >>> +       struct disk_partition info;
> >>> +       struct blk_desc *desc = NULL;
> >>> +
> >>> +       ret = get_mmc_desc(&desc);
> >>> +       if (ret)
> >>> +               return;
> >>> +
> >>> +       memset(buf, 0, sizeof(buf));
> >>> +       name = blk_get_if_type_name(desc->if_type);
> >>> +       devnum = desc->devnum;
> >>> +       len = strlen(buf);
> >>> +
> >>> +       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> >>> +                        "%s %d=", name, devnum);
> >>> +
> >>> +       for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> >>> +               if (part_get_info(desc, p, &info))
> >>> +                       continue;
> >>> +
> >>> +               /* Add entry to dfu_alt_info only for updatable images */
> >>> +               if (updatable_image(&info)) {
> >>> +                       if (!first)
> >>> +                               len += snprintf(buf + len,
> >>> +                                               DFU_ALT_BUF_LEN - len, ";");
> >>> +
> >>> +                       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> >>> +                                       "%s%d_%s part %d %d",
> >>> +                                       name, devnum, info.name, devnum, p);
> >>> +                       first = false;
> >>> +               }
> >>> +       }
> >>> +
> >>> +       log_debug("dfu_alt_info => %s\n", buf);
> >>> +       env_set("dfu_alt_info", buf);
> >>> +}
> >>> +
> >>> +int rk_board_late_init(void)
> >>> +{
> >>> +       int p, i, ret;
> >>> +       struct disk_partition info;
> >>> +       struct blk_desc *desc = NULL;
> >>> +
> >>> +       if (board_is_rockpi_4b()) {
> >>> +               efi_guid_t idbldr_image_type_guid =
> >>> +                       ROCKPI_4B_IDBLOADER_IMAGE_GUID;
> >>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4B_UBOOT_IMAGE_GUID;
> >>> +
> >>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> >>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> >>> +
> >>> +               fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
> >>> +               fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
> >>> +       } else if (board_is_rockpi_4c()) {
> >>> +               efi_guid_t idbldr_image_type_guid =
> >>> +                       ROCKPI_4C_IDBLOADER_IMAGE_GUID;
> >>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4C_UBOOT_IMAGE_GUID;
> >>> +
> >>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> >>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> >>> +
> >>> +               fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
> >>> +               fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
> >>> +       }
> >>> +
> >>> +       ret = get_mmc_desc(&desc);
> >>> +       if (ret)
> >>> +               return ret;
> >>> +
> >>> +       for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> >>> +               if (part_get_info(desc, p, &info))
> >>> +                       continue;
> >>> +
> >>> +               /*
> >>> +                * Since we have a GPT partitioned device, the updatable
> >>> +                * images could be stored in any order. Populate the
> >>> +                * image_index at runtime.
> >>> +                */
> >>> +               if (updatable_image(&info)) {
> >>> +                       set_image_index(&info, i);
> >>> +                       i++;
> >>> +               }
> >>> +       }
> >>> +
> >>> +       return 0;
> >>> +}
> >>> +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
> >>> +#endif /* !CONFIG_SPL_BUILD */
> >>> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
> >>> index 80d1e63b59..ca3e217603 100644
> >>> --- a/configs/rock-pi-4-rk3399_defconfig
> >>> +++ b/configs/rock-pi-4-rk3399_defconfig
> >>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
> >>>   CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
> >>>   CONFIG_TPL=y
> >>>   CONFIG_CMD_BOOTZ=y
> >>> +CONFIG_CMD_DFU=y
> >>>   CONFIG_CMD_GPT=y
> >>>   CONFIG_CMD_MMC=y
> >>>   CONFIG_CMD_PCI=y
> >>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
> >>>   CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> >>>   CONFIG_ENV_IS_IN_MMC=y
> >>>   CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> >>> +CONFIG_DFU_MMC=y
> >>>   CONFIG_ROCKCHIP_GPIO=y
> >>>   CONFIG_SYS_I2C_ROCKCHIP=y
> >>>   CONFIG_MISC=y
> >>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
> >>>   CONFIG_DISPLAY_ROCKCHIP_HDMI=y
> >>>   CONFIG_SPL_TINY_MEMSET=y
> >>>   CONFIG_ERRNO_STR=y
> >>> +CONFIG_EFI_CAPSULE_ON_DISK=y
> >>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> >>> diff --git a/configs/rock-pi-4c-rk3399_defconfig b/configs/rock-pi-4c-rk3399_defconfig
> >>> index bda4b70dbf..4c89e7e918 100644
> >>> --- a/configs/rock-pi-4c-rk3399_defconfig
> >>> +++ b/configs/rock-pi-4c-rk3399_defconfig
> >>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
> >>>   CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
> >>>   CONFIG_TPL=y
> >>>   CONFIG_CMD_BOOTZ=y
> >>> +CONFIG_CMD_DFU=y
> >>>   CONFIG_CMD_GPT=y
> >>>   CONFIG_CMD_MMC=y
> >>>   CONFIG_CMD_PCI=y
> >>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
> >>>   CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> >>>   CONFIG_ENV_IS_IN_MMC=y
> >>>   CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> >>> +CONFIG_DFU_MMC=y
> >>>   CONFIG_ROCKCHIP_GPIO=y
> >>>   CONFIG_SYS_I2C_ROCKCHIP=y
> >>>   CONFIG_MISC=y
> >>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
> >>>   CONFIG_DISPLAY_ROCKCHIP_HDMI=y
> >>>   CONFIG_SPL_TINY_MEMSET=y
> >>>   CONFIG_ERRNO_STR=y
> >>> +CONFIG_EFI_CAPSULE_ON_DISK=y
> >>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> >>> diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h
> >>> index 8e13737666..28b514a4ea 100644
> >>> --- a/include/configs/rk3399_common.h
> >>> +++ b/include/configs/rk3399_common.h
> >>> @@ -38,6 +38,22 @@
> >>>   #define CONFIG_SYS_SDRAM_BASE          0
> >>>   #define SDRAM_MAX_SIZE                 0xf8000000
> >>>
> >>> +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
> >>> +       EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
> >>> +                0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
> >>> +
> >>> +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
> >>> +       EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
> >>> +                0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
> >>> +
> >>> +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
> >>> +       EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
> >>> +                0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
> >>> +
> >>> +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
> >>> +       EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
> >>> +                0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
> >>> +
> >>>   #ifndef CONFIG_SPL_BUILD
> >>>
> >>>   #define ENV_MEM_LAYOUT_SETTINGS \
> >>> --
> >>> 2.25.1
> >>>

Hi Sughosh,

On 2022/9/2 15:52, Sughosh Ganu wrote:
> hi Kever,
>
> On Wed, 29 Jun 2022 at 09:11, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hi Sughosh,
>>
>>        Thanks for your patch.
>>
>> On 2022/5/16 14:12, Sughosh Ganu wrote:
>>> hi Peter,
>>>
>>> On Sat, 14 May 2022 at 13:44, Peter Robinson <pbrobinson@gmail.com> wrote:
>>>> On Fri, May 13, 2022 at 7:50 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>>>>> Add support for updating the idbloader and u-boot images through the
>>>>> UEFI capsule update functionality. Enable the modules required for
>>>>> supporting the functionality.
>>>>>
>>>>> The implementation is for the updatable images placed on a GPT
>>>>> partitioned storage device. With the GPT partition devices, the
>>>>> partition information can be obtained at runtime, and hence the
>>>>> dfu_alt_info variable needed for the updates is being populated at
>>>>> runtime.
>>>>>
>>>>> This requires the partitions containing the idbloader and u-boot
>>>>> images to be created with the PartitionTypeGUID values set to the
>>>>> corresponding image type GUID values defined in the platform's config
>>>>> header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
>>>>> ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).
>>>> I think it would be useful to break this patch down into more than one
>>>> patch. At least the core changes to evb_rk3399/evb-rk3399.c and the
>>>> board specific changes. Also given these changes arre likely of
>>>> interest to other boards should  they go to somewhere more central
>>>> than evb_rk3399/evb-rk3399.c to enable less code duplication across
>>>> rk3399 or even rockchip devices as a whole?
>>> I believe I can move the logic to set dfu_alt_info to some place like
>>> arch/arm/mach-rockchip/board.c. The definition of rk_board_late_init()
>>> can stay in evb-rk3399.c since it is populating the capsule update
>>> structures for the rk3399 family of platforms. Does that sound okay to
>>> you? If so, I will make the changes and send a V2, or if  you have
>>> some other solution in mind, please let me know. Thanks.
>> Other than Peter's command, please make sure:
>>
>> All the source code relate to rockpi should go to board/radxa instead of
>> board/rockchip/evb-rk3399.c
> Apologies for the late reply. I was working on some other stuff and
> hence this got put on the backburner. With regards to your review
> comment of putting the board code under board/radxa/, how do I get the
> code under board/radxa to compile. The RockPi4 boards have
> CONFIG_SYS_VENDOR="rockchip" and CONFIG_SYS_BOARD="evb_rk3399". With
> these configs, code under board/rockchip/evb_rk3399/ gets built. Is
> there some way I can get code under some other board directory to
> build?
>
> I was thinking of putting the common code under
> arch/arm/mach-rockchip/board.c, and the board specific declarations
> under board/rockchip/evb_rk3399/evb_rk3399.c, along with the splitting
> of the changes in two patches. Is that fine with you?

Yes, this sounds OK for me. Does it means it can be used for all rk3399, 
but not only rockpi board?

I have no idea about "UEFI capsule update" till now, where can I find 
the requirement for it?

I have two question for the implement thought:

1. Why does it need to use different GUID for different board?

2. The GPT table GUID should be write when the table is generated, your 
patch seems like a workaround?


Thanks,
- Kever
>
> -sughosh
>
>>
>> Thanks,
>>
>> - Kever
>>
>>> -sughosh
>>>
>>>> Peter
>>>>
>>>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>>>> ---
>>>>>    arch/arm/mach-rockchip/Kconfig         |   1 +
>>>>>    board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
>>>>>    configs/rock-pi-4-rk3399_defconfig     |   4 +
>>>>>    configs/rock-pi-4c-rk3399_defconfig    |   4 +
>>>>>    include/configs/rk3399_common.h        |  16 +++
>>>>>    5 files changed, 214 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
>>>>> index 18aff5480b..6d13e7b92e 100644
>>>>> --- a/arch/arm/mach-rockchip/Kconfig
>>>>> +++ b/arch/arm/mach-rockchip/Kconfig
>>>>> @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
>>>>>           select DM_PMIC
>>>>>           select DM_REGULATOR_FIXED
>>>>>           select BOARD_LATE_INIT
>>>>> +       imply PARTITION_TYPE_GUID
>>>>>           imply PRE_CONSOLE_BUFFER
>>>>>           imply ROCKCHIP_COMMON_BOARD
>>>>>           imply ROCKCHIP_SDRAM_COMMON
>>>>> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
>>>>> index abb76585cf..cc976d9471 100644
>>>>> --- a/board/rockchip/evb_rk3399/evb-rk3399.c
>>>>> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
>>>>> @@ -5,11 +5,27 @@
>>>>>
>>>>>    #include <common.h>
>>>>>    #include <dm.h>
>>>>> +#include <efi_loader.h>
>>>>>    #include <init.h>
>>>>>    #include <log.h>
>>>>> +#include <mmc.h>
>>>>> +#include <part.h>
>>>>> +#include <uuid.h>
>>>>>    #include <asm/arch-rockchip/periph.h>
>>>>>    #include <power/regulator.h>
>>>>>
>>>>> +#define ROCKPI4_UPDATABLE_IMAGES       2
>>>>> +
>>>>> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
>>>>> +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
>>>>> +
>>>>> +struct efi_capsule_update_info update_info = {
>>>>> +       .images = fw_images,
>>>>> +};
>>>>> +
>>>>> +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
>>>>> +#endif
>>>>> +
>>>>>    #ifndef CONFIG_SPL_BUILD
>>>>>    int board_early_init_f(void)
>>>>>    {
>>>>> @@ -29,4 +45,176 @@ int board_early_init_f(void)
>>>>>    out:
>>>>>           return 0;
>>>>>    }
>>>>> -#endif
>>>>> +
>>>>> +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION)
>>>>> +
>>>>> +#define DFU_ALT_BUF_LEN SZ_1K
>>>>> +
>>>>> +static bool board_is_rockpi_4b(void)
>>>>> +{
>>>>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
>>>>> +               of_machine_is_compatible("radxa,rockpi4b");
>>>>> +}
>>>>> +
>>>>> +static bool board_is_rockpi_4c(void)
>>>>> +{
>>>>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
>>>>> +               of_machine_is_compatible("radxa,rockpi4c");
>>>>> +}
>>>>> +
>>>>> +static bool updatable_image(struct disk_partition *info)
>>>>> +{
>>>>> +       int i;
>>>>> +       bool ret = false;
>>>>> +       efi_guid_t image_type_guid;
>>>>> +
>>>>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
>>>>> +                       UUID_STR_FORMAT_GUID);
>>>>> +
>>>>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
>>>>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
>>>>> +                       ret = true;
>>>>> +                       break;
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>> +       return ret;
>>>>> +}
>>>>> +
>>>>> +static void set_image_index(struct disk_partition *info, int index)
>>>>> +{
>>>>> +       int i;
>>>>> +       efi_guid_t image_type_guid;
>>>>> +
>>>>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
>>>>> +                       UUID_STR_FORMAT_GUID);
>>>>> +
>>>>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
>>>>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
>>>>> +                       fw_images[i].image_index = index;
>>>>> +                       break;
>>>>> +               }
>>>>> +       }
>>>>> +}
>>>>> +
>>>>> +static int get_mmc_desc(struct blk_desc **desc)
>>>>> +{
>>>>> +       int ret;
>>>>> +       struct mmc *mmc;
>>>>> +       struct udevice *dev;
>>>>> +
>>>>> +       /*
>>>>> +        * For now the firmware images are assumed to
>>>>> +        * be on the SD card
>>>>> +        */
>>>>> +       ret = uclass_get_device(UCLASS_MMC, 1, &dev);
>>>>> +       if (ret)
>>>>> +               return -1;
>>>>> +
>>>>> +       mmc = mmc_get_mmc_dev(dev);
>>>>> +       if (!mmc)
>>>>> +               return -1;
>>>>> +
>>>>> +       if (mmc_init(mmc))
>>>>> +               return -1;
>>>>> +
>>>>> +       *desc = mmc_get_blk_desc(mmc);
>>>>> +       if (!*desc)
>>>>> +               return -1;
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +void set_dfu_alt_info(char *interface, char *devstr)
>>>>> +{
>>>>> +       const char *name;
>>>>> +       bool first = true;
>>>>> +       int p, len, devnum, ret;
>>>>> +       char buf[DFU_ALT_BUF_LEN];
>>>>> +       struct disk_partition info;
>>>>> +       struct blk_desc *desc = NULL;
>>>>> +
>>>>> +       ret = get_mmc_desc(&desc);
>>>>> +       if (ret)
>>>>> +               return;
>>>>> +
>>>>> +       memset(buf, 0, sizeof(buf));
>>>>> +       name = blk_get_if_type_name(desc->if_type);
>>>>> +       devnum = desc->devnum;
>>>>> +       len = strlen(buf);
>>>>> +
>>>>> +       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
>>>>> +                        "%s %d=", name, devnum);
>>>>> +
>>>>> +       for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
>>>>> +               if (part_get_info(desc, p, &info))
>>>>> +                       continue;
>>>>> +
>>>>> +               /* Add entry to dfu_alt_info only for updatable images */
>>>>> +               if (updatable_image(&info)) {
>>>>> +                       if (!first)
>>>>> +                               len += snprintf(buf + len,
>>>>> +                                               DFU_ALT_BUF_LEN - len, ";");
>>>>> +
>>>>> +                       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
>>>>> +                                       "%s%d_%s part %d %d",
>>>>> +                                       name, devnum, info.name, devnum, p);
>>>>> +                       first = false;
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>> +       log_debug("dfu_alt_info => %s\n", buf);
>>>>> +       env_set("dfu_alt_info", buf);
>>>>> +}
>>>>> +
>>>>> +int rk_board_late_init(void)
>>>>> +{
>>>>> +       int p, i, ret;
>>>>> +       struct disk_partition info;
>>>>> +       struct blk_desc *desc = NULL;
>>>>> +
>>>>> +       if (board_is_rockpi_4b()) {
>>>>> +               efi_guid_t idbldr_image_type_guid =
>>>>> +                       ROCKPI_4B_IDBLOADER_IMAGE_GUID;
>>>>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4B_UBOOT_IMAGE_GUID;
>>>>> +
>>>>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
>>>>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
>>>>> +
>>>>> +               fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
>>>>> +               fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
>>>>> +       } else if (board_is_rockpi_4c()) {
>>>>> +               efi_guid_t idbldr_image_type_guid =
>>>>> +                       ROCKPI_4C_IDBLOADER_IMAGE_GUID;
>>>>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4C_UBOOT_IMAGE_GUID;
>>>>> +
>>>>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
>>>>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
>>>>> +
>>>>> +               fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
>>>>> +               fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
>>>>> +       }
>>>>> +
>>>>> +       ret = get_mmc_desc(&desc);
>>>>> +       if (ret)
>>>>> +               return ret;
>>>>> +
>>>>> +       for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
>>>>> +               if (part_get_info(desc, p, &info))
>>>>> +                       continue;
>>>>> +
>>>>> +               /*
>>>>> +                * Since we have a GPT partitioned device, the updatable
>>>>> +                * images could be stored in any order. Populate the
>>>>> +                * image_index at runtime.
>>>>> +                */
>>>>> +               if (updatable_image(&info)) {
>>>>> +                       set_image_index(&info, i);
>>>>> +                       i++;
>>>>> +               }
>>>>> +       }
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
>>>>> +#endif /* !CONFIG_SPL_BUILD */
>>>>> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
>>>>> index 80d1e63b59..ca3e217603 100644
>>>>> --- a/configs/rock-pi-4-rk3399_defconfig
>>>>> +++ b/configs/rock-pi-4-rk3399_defconfig
>>>>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>>>>>    CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>>>>>    CONFIG_TPL=y
>>>>>    CONFIG_CMD_BOOTZ=y
>>>>> +CONFIG_CMD_DFU=y
>>>>>    CONFIG_CMD_GPT=y
>>>>>    CONFIG_CMD_MMC=y
>>>>>    CONFIG_CMD_PCI=y
>>>>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>>>>>    CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>>>>>    CONFIG_ENV_IS_IN_MMC=y
>>>>>    CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>>>>> +CONFIG_DFU_MMC=y
>>>>>    CONFIG_ROCKCHIP_GPIO=y
>>>>>    CONFIG_SYS_I2C_ROCKCHIP=y
>>>>>    CONFIG_MISC=y
>>>>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>>>>>    CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>>>>>    CONFIG_SPL_TINY_MEMSET=y
>>>>>    CONFIG_ERRNO_STR=y
>>>>> +CONFIG_EFI_CAPSULE_ON_DISK=y
>>>>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
>>>>> diff --git a/configs/rock-pi-4c-rk3399_defconfig b/configs/rock-pi-4c-rk3399_defconfig
>>>>> index bda4b70dbf..4c89e7e918 100644
>>>>> --- a/configs/rock-pi-4c-rk3399_defconfig
>>>>> +++ b/configs/rock-pi-4c-rk3399_defconfig
>>>>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
>>>>>    CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
>>>>>    CONFIG_TPL=y
>>>>>    CONFIG_CMD_BOOTZ=y
>>>>> +CONFIG_CMD_DFU=y
>>>>>    CONFIG_CMD_GPT=y
>>>>>    CONFIG_CMD_MMC=y
>>>>>    CONFIG_CMD_PCI=y
>>>>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
>>>>>    CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>>>>>    CONFIG_ENV_IS_IN_MMC=y
>>>>>    CONFIG_SYS_RELOC_GD_ENV_ADDR=y
>>>>> +CONFIG_DFU_MMC=y
>>>>>    CONFIG_ROCKCHIP_GPIO=y
>>>>>    CONFIG_SYS_I2C_ROCKCHIP=y
>>>>>    CONFIG_MISC=y
>>>>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
>>>>>    CONFIG_DISPLAY_ROCKCHIP_HDMI=y
>>>>>    CONFIG_SPL_TINY_MEMSET=y
>>>>>    CONFIG_ERRNO_STR=y
>>>>> +CONFIG_EFI_CAPSULE_ON_DISK=y
>>>>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
>>>>> diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h
>>>>> index 8e13737666..28b514a4ea 100644
>>>>> --- a/include/configs/rk3399_common.h
>>>>> +++ b/include/configs/rk3399_common.h
>>>>> @@ -38,6 +38,22 @@
>>>>>    #define CONFIG_SYS_SDRAM_BASE          0
>>>>>    #define SDRAM_MAX_SIZE                 0xf8000000
>>>>>
>>>>> +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
>>>>> +       EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
>>>>> +                0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
>>>>> +
>>>>> +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
>>>>> +       EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
>>>>> +                0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
>>>>> +
>>>>> +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
>>>>> +       EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
>>>>> +                0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
>>>>> +
>>>>> +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
>>>>> +       EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
>>>>> +                0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
>>>>> +
>>>>>    #ifndef CONFIG_SPL_BUILD
>>>>>
>>>>>    #define ENV_MEM_LAYOUT_SETTINGS \
>>>>> --
>>>>> 2.25.1
>>>>>

hi Kever,

On Fri, 9 Sept 2022 at 15:38, Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Hi Sughosh,
>
> On 2022/9/2 15:52, Sughosh Ganu wrote:
> > hi Kever,
> >
> > On Wed, 29 Jun 2022 at 09:11, Kever Yang <kever.yang@rock-chips.com> wrote:
> >> Hi Sughosh,
> >>
> >>        Thanks for your patch.
> >>
> >> On 2022/5/16 14:12, Sughosh Ganu wrote:
> >>> hi Peter,
> >>>
> >>> On Sat, 14 May 2022 at 13:44, Peter Robinson <pbrobinson@gmail.com> wrote:
> >>>> On Fri, May 13, 2022 at 7:50 AM Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >>>>> Add support for updating the idbloader and u-boot images through the
> >>>>> UEFI capsule update functionality. Enable the modules required for
> >>>>> supporting the functionality.
> >>>>>
> >>>>> The implementation is for the updatable images placed on a GPT
> >>>>> partitioned storage device. With the GPT partition devices, the
> >>>>> partition information can be obtained at runtime, and hence the
> >>>>> dfu_alt_info variable needed for the updates is being populated at
> >>>>> runtime.
> >>>>>
> >>>>> This requires the partitions containing the idbloader and u-boot
> >>>>> images to be created with the PartitionTypeGUID values set to the
> >>>>> corresponding image type GUID values defined in the platform's config
> >>>>> header(ROCKPI_4{B,C}_IDBLOADER_IMAGE_GUID and
> >>>>> ROCKPI_4{B,C}_UBOOT_IMAGE_GUID).
> >>>> I think it would be useful to break this patch down into more than one
> >>>> patch. At least the core changes to evb_rk3399/evb-rk3399.c and the
> >>>> board specific changes. Also given these changes arre likely of
> >>>> interest to other boards should  they go to somewhere more central
> >>>> than evb_rk3399/evb-rk3399.c to enable less code duplication across
> >>>> rk3399 or even rockchip devices as a whole?
> >>> I believe I can move the logic to set dfu_alt_info to some place like
> >>> arch/arm/mach-rockchip/board.c. The definition of rk_board_late_init()
> >>> can stay in evb-rk3399.c since it is populating the capsule update
> >>> structures for the rk3399 family of platforms. Does that sound okay to
> >>> you? If so, I will make the changes and send a V2, or if  you have
> >>> some other solution in mind, please let me know. Thanks.
> >> Other than Peter's command, please make sure:
> >>
> >> All the source code relate to rockpi should go to board/radxa instead of
> >> board/rockchip/evb-rk3399.c
> > Apologies for the late reply. I was working on some other stuff and
> > hence this got put on the backburner. With regards to your review
> > comment of putting the board code under board/radxa/, how do I get the
> > code under board/radxa to compile. The RockPi4 boards have
> > CONFIG_SYS_VENDOR="rockchip" and CONFIG_SYS_BOARD="evb_rk3399". With
> > these configs, code under board/rockchip/evb_rk3399/ gets built. Is
> > there some way I can get code under some other board directory to
> > build?
> >
> > I was thinking of putting the common code under
> > arch/arm/mach-rockchip/board.c, and the board specific declarations
> > under board/rockchip/evb_rk3399/evb_rk3399.c, along with the splitting
> > of the changes in two patches. Is that fine with you?
>
> Yes, this sounds OK for me. Does it means it can be used for all rk3399,
> but not only rockpi board?

Yes, the code can be used on all the boards which build the
arch/arm/mach-rockchip/board.c and
board/rockchip/evb_rk3399/evb-rk3399.c files. Also, currently, the
support is being enabled for firmware images stored on GPT partitioned
storage devices. It can easily be extended if it is needed on any
board which does not support GPT partitioned devices.

>
> I have no idea about "UEFI capsule update" till now, where can I find
> the requirement for it?

This is a feature defined in the UEFI specification. This is used to
update firmware images on the platform. There is some documentation
about the feature in the u-boot tree [1] including the requirements
for enabling the feature.

>
> I have two question for the implement thought:
>
> 1. Why does it need to use different GUID for different board?

The image GUID is used to identify the image for a given board. If an
image can be used across multiple boards, the same GUID can be used
for that image. So for e.g. if a u-boot image can be used on the
RockPi4B and any other RockPi variant, it can use the same GUID value
for that u-boot image.

>
> 2. The GPT table GUID should be write when the table is generated, your
> patch seems like a workaround?

The GUID value for the partition is indeed written when the GPT
partition is generated. What the patch does is populate the structures
and other objects like dfu_alt_info variable which are needed for the
capsule update. There are two ways the capsule update related
structures can be populated, at build time, and at runtime for boards
which have firmware on GPT partitioned devices. These patches are
supporting the later scenario. Thanks.

-sughosh

[1] - https://u-boot.readthedocs.io/en/latest/develop/uefi/uefi.html?highlight=capsule%20upd#enabling-uefi-capsule-update-feature

>
>
> Thanks,
> - Kever
> >
> > -sughosh
> >
> >>
> >> Thanks,
> >>
> >> - Kever
> >>
> >>> -sughosh
> >>>
> >>>> Peter
> >>>>
> >>>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>>>> ---
> >>>>>    arch/arm/mach-rockchip/Kconfig         |   1 +
> >>>>>    board/rockchip/evb_rk3399/evb-rk3399.c | 190 ++++++++++++++++++++++++-
> >>>>>    configs/rock-pi-4-rk3399_defconfig     |   4 +
> >>>>>    configs/rock-pi-4c-rk3399_defconfig    |   4 +
> >>>>>    include/configs/rk3399_common.h        |  16 +++
> >>>>>    5 files changed, 214 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> >>>>> index 18aff5480b..6d13e7b92e 100644
> >>>>> --- a/arch/arm/mach-rockchip/Kconfig
> >>>>> +++ b/arch/arm/mach-rockchip/Kconfig
> >>>>> @@ -246,6 +246,7 @@ config ROCKCHIP_RK3399
> >>>>>           select DM_PMIC
> >>>>>           select DM_REGULATOR_FIXED
> >>>>>           select BOARD_LATE_INIT
> >>>>> +       imply PARTITION_TYPE_GUID
> >>>>>           imply PRE_CONSOLE_BUFFER
> >>>>>           imply ROCKCHIP_COMMON_BOARD
> >>>>>           imply ROCKCHIP_SDRAM_COMMON
> >>>>> diff --git a/board/rockchip/evb_rk3399/evb-rk3399.c b/board/rockchip/evb_rk3399/evb-rk3399.c
> >>>>> index abb76585cf..cc976d9471 100644
> >>>>> --- a/board/rockchip/evb_rk3399/evb-rk3399.c
> >>>>> +++ b/board/rockchip/evb_rk3399/evb-rk3399.c
> >>>>> @@ -5,11 +5,27 @@
> >>>>>
> >>>>>    #include <common.h>
> >>>>>    #include <dm.h>
> >>>>> +#include <efi_loader.h>
> >>>>>    #include <init.h>
> >>>>>    #include <log.h>
> >>>>> +#include <mmc.h>
> >>>>> +#include <part.h>
> >>>>> +#include <uuid.h>
> >>>>>    #include <asm/arch-rockchip/periph.h>
> >>>>>    #include <power/regulator.h>
> >>>>>
> >>>>> +#define ROCKPI4_UPDATABLE_IMAGES       2
> >>>>> +
> >>>>> +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> >>>>> +struct efi_fw_image fw_images[ROCKPI4_UPDATABLE_IMAGES] = {0};
> >>>>> +
> >>>>> +struct efi_capsule_update_info update_info = {
> >>>>> +       .images = fw_images,
> >>>>> +};
> >>>>> +
> >>>>> +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
> >>>>> +#endif
> >>>>> +
> >>>>>    #ifndef CONFIG_SPL_BUILD
> >>>>>    int board_early_init_f(void)
> >>>>>    {
> >>>>> @@ -29,4 +45,176 @@ int board_early_init_f(void)
> >>>>>    out:
> >>>>>           return 0;
> >>>>>    }
> >>>>> -#endif
> >>>>> +
> >>>>> +#if defined(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) && defined(CONFIG_EFI_PARTITION)
> >>>>> +
> >>>>> +#define DFU_ALT_BUF_LEN SZ_1K
> >>>>> +
> >>>>> +static bool board_is_rockpi_4b(void)
> >>>>> +{
> >>>>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> >>>>> +               of_machine_is_compatible("radxa,rockpi4b");
> >>>>> +}
> >>>>> +
> >>>>> +static bool board_is_rockpi_4c(void)
> >>>>> +{
> >>>>> +       return CONFIG_IS_ENABLED(TARGET_EVB_RK3399) &&
> >>>>> +               of_machine_is_compatible("radxa,rockpi4c");
> >>>>> +}
> >>>>> +
> >>>>> +static bool updatable_image(struct disk_partition *info)
> >>>>> +{
> >>>>> +       int i;
> >>>>> +       bool ret = false;
> >>>>> +       efi_guid_t image_type_guid;
> >>>>> +
> >>>>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> >>>>> +                       UUID_STR_FORMAT_GUID);
> >>>>> +
> >>>>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> >>>>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> >>>>> +                       ret = true;
> >>>>> +                       break;
> >>>>> +               }
> >>>>> +       }
> >>>>> +
> >>>>> +       return ret;
> >>>>> +}
> >>>>> +
> >>>>> +static void set_image_index(struct disk_partition *info, int index)
> >>>>> +{
> >>>>> +       int i;
> >>>>> +       efi_guid_t image_type_guid;
> >>>>> +
> >>>>> +       uuid_str_to_bin(info->type_guid, image_type_guid.b,
> >>>>> +                       UUID_STR_FORMAT_GUID);
> >>>>> +
> >>>>> +       for (i = 0; i < ROCKPI4_UPDATABLE_IMAGES; i++) {
> >>>>> +               if (!guidcmp(&fw_images[i].image_type_id, &image_type_guid)) {
> >>>>> +                       fw_images[i].image_index = index;
> >>>>> +                       break;
> >>>>> +               }
> >>>>> +       }
> >>>>> +}
> >>>>> +
> >>>>> +static int get_mmc_desc(struct blk_desc **desc)
> >>>>> +{
> >>>>> +       int ret;
> >>>>> +       struct mmc *mmc;
> >>>>> +       struct udevice *dev;
> >>>>> +
> >>>>> +       /*
> >>>>> +        * For now the firmware images are assumed to
> >>>>> +        * be on the SD card
> >>>>> +        */
> >>>>> +       ret = uclass_get_device(UCLASS_MMC, 1, &dev);
> >>>>> +       if (ret)
> >>>>> +               return -1;
> >>>>> +
> >>>>> +       mmc = mmc_get_mmc_dev(dev);
> >>>>> +       if (!mmc)
> >>>>> +               return -1;
> >>>>> +
> >>>>> +       if (mmc_init(mmc))
> >>>>> +               return -1;
> >>>>> +
> >>>>> +       *desc = mmc_get_blk_desc(mmc);
> >>>>> +       if (!*desc)
> >>>>> +               return -1;
> >>>>> +
> >>>>> +       return 0;
> >>>>> +}
> >>>>> +
> >>>>> +void set_dfu_alt_info(char *interface, char *devstr)
> >>>>> +{
> >>>>> +       const char *name;
> >>>>> +       bool first = true;
> >>>>> +       int p, len, devnum, ret;
> >>>>> +       char buf[DFU_ALT_BUF_LEN];
> >>>>> +       struct disk_partition info;
> >>>>> +       struct blk_desc *desc = NULL;
> >>>>> +
> >>>>> +       ret = get_mmc_desc(&desc);
> >>>>> +       if (ret)
> >>>>> +               return;
> >>>>> +
> >>>>> +       memset(buf, 0, sizeof(buf));
> >>>>> +       name = blk_get_if_type_name(desc->if_type);
> >>>>> +       devnum = desc->devnum;
> >>>>> +       len = strlen(buf);
> >>>>> +
> >>>>> +       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> >>>>> +                        "%s %d=", name, devnum);
> >>>>> +
> >>>>> +       for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> >>>>> +               if (part_get_info(desc, p, &info))
> >>>>> +                       continue;
> >>>>> +
> >>>>> +               /* Add entry to dfu_alt_info only for updatable images */
> >>>>> +               if (updatable_image(&info)) {
> >>>>> +                       if (!first)
> >>>>> +                               len += snprintf(buf + len,
> >>>>> +                                               DFU_ALT_BUF_LEN - len, ";");
> >>>>> +
> >>>>> +                       len += snprintf(buf + len, DFU_ALT_BUF_LEN - len,
> >>>>> +                                       "%s%d_%s part %d %d",
> >>>>> +                                       name, devnum, info.name, devnum, p);
> >>>>> +                       first = false;
> >>>>> +               }
> >>>>> +       }
> >>>>> +
> >>>>> +       log_debug("dfu_alt_info => %s\n", buf);
> >>>>> +       env_set("dfu_alt_info", buf);
> >>>>> +}
> >>>>> +
> >>>>> +int rk_board_late_init(void)
> >>>>> +{
> >>>>> +       int p, i, ret;
> >>>>> +       struct disk_partition info;
> >>>>> +       struct blk_desc *desc = NULL;
> >>>>> +
> >>>>> +       if (board_is_rockpi_4b()) {
> >>>>> +               efi_guid_t idbldr_image_type_guid =
> >>>>> +                       ROCKPI_4B_IDBLOADER_IMAGE_GUID;
> >>>>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4B_UBOOT_IMAGE_GUID;
> >>>>> +
> >>>>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> >>>>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> >>>>> +
> >>>>> +               fw_images[0].fw_name = u"ROCKPI4B-IDBLOADER";
> >>>>> +               fw_images[1].fw_name = u"ROCKPI4B-UBOOT";
> >>>>> +       } else if (board_is_rockpi_4c()) {
> >>>>> +               efi_guid_t idbldr_image_type_guid =
> >>>>> +                       ROCKPI_4C_IDBLOADER_IMAGE_GUID;
> >>>>> +               efi_guid_t uboot_image_type_guid = ROCKPI_4C_UBOOT_IMAGE_GUID;
> >>>>> +
> >>>>> +               guidcpy(&fw_images[0].image_type_id, &idbldr_image_type_guid);
> >>>>> +               guidcpy(&fw_images[1].image_type_id, &uboot_image_type_guid);
> >>>>> +
> >>>>> +               fw_images[0].fw_name = u"ROCKPI4C-IDBLOADER";
> >>>>> +               fw_images[1].fw_name = u"ROCKPI4C-UBOOT";
> >>>>> +       }
> >>>>> +
> >>>>> +       ret = get_mmc_desc(&desc);
> >>>>> +       if (ret)
> >>>>> +               return ret;
> >>>>> +
> >>>>> +       for (p = 1, i = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
> >>>>> +               if (part_get_info(desc, p, &info))
> >>>>> +                       continue;
> >>>>> +
> >>>>> +               /*
> >>>>> +                * Since we have a GPT partitioned device, the updatable
> >>>>> +                * images could be stored in any order. Populate the
> >>>>> +                * image_index at runtime.
> >>>>> +                */
> >>>>> +               if (updatable_image(&info)) {
> >>>>> +                       set_image_index(&info, i);
> >>>>> +                       i++;
> >>>>> +               }
> >>>>> +       }
> >>>>> +
> >>>>> +       return 0;
> >>>>> +}
> >>>>> +#endif /* CONFIG_EFI_HAVE_CAPSULE_SUPPORT && CONFIG_EFI_PARTITION */
> >>>>> +#endif /* !CONFIG_SPL_BUILD */
> >>>>> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
> >>>>> index 80d1e63b59..ca3e217603 100644
> >>>>> --- a/configs/rock-pi-4-rk3399_defconfig
> >>>>> +++ b/configs/rock-pi-4-rk3399_defconfig
> >>>>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
> >>>>>    CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
> >>>>>    CONFIG_TPL=y
> >>>>>    CONFIG_CMD_BOOTZ=y
> >>>>> +CONFIG_CMD_DFU=y
> >>>>>    CONFIG_CMD_GPT=y
> >>>>>    CONFIG_CMD_MMC=y
> >>>>>    CONFIG_CMD_PCI=y
> >>>>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
> >>>>>    CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> >>>>>    CONFIG_ENV_IS_IN_MMC=y
> >>>>>    CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> >>>>> +CONFIG_DFU_MMC=y
> >>>>>    CONFIG_ROCKCHIP_GPIO=y
> >>>>>    CONFIG_SYS_I2C_ROCKCHIP=y
> >>>>>    CONFIG_MISC=y
> >>>>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
> >>>>>    CONFIG_DISPLAY_ROCKCHIP_HDMI=y
> >>>>>    CONFIG_SPL_TINY_MEMSET=y
> >>>>>    CONFIG_ERRNO_STR=y
> >>>>> +CONFIG_EFI_CAPSULE_ON_DISK=y
> >>>>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> >>>>> diff --git a/configs/rock-pi-4c-rk3399_defconfig b/configs/rock-pi-4c-rk3399_defconfig
> >>>>> index bda4b70dbf..4c89e7e918 100644
> >>>>> --- a/configs/rock-pi-4c-rk3399_defconfig
> >>>>> +++ b/configs/rock-pi-4c-rk3399_defconfig
> >>>>> @@ -21,6 +21,7 @@ CONFIG_SPL_STACK_R=y
> >>>>>    CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
> >>>>>    CONFIG_TPL=y
> >>>>>    CONFIG_CMD_BOOTZ=y
> >>>>> +CONFIG_CMD_DFU=y
> >>>>>    CONFIG_CMD_GPT=y
> >>>>>    CONFIG_CMD_MMC=y
> >>>>>    CONFIG_CMD_PCI=y
> >>>>> @@ -31,6 +32,7 @@ CONFIG_SPL_OF_CONTROL=y
> >>>>>    CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> >>>>>    CONFIG_ENV_IS_IN_MMC=y
> >>>>>    CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> >>>>> +CONFIG_DFU_MMC=y
> >>>>>    CONFIG_ROCKCHIP_GPIO=y
> >>>>>    CONFIG_SYS_I2C_ROCKCHIP=y
> >>>>>    CONFIG_MISC=y
> >>>>> @@ -76,3 +78,5 @@ CONFIG_VIDEO_ROCKCHIP=y
> >>>>>    CONFIG_DISPLAY_ROCKCHIP_HDMI=y
> >>>>>    CONFIG_SPL_TINY_MEMSET=y
> >>>>>    CONFIG_ERRNO_STR=y
> >>>>> +CONFIG_EFI_CAPSULE_ON_DISK=y
> >>>>> +CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
> >>>>> diff --git a/include/configs/rk3399_common.h b/include/configs/rk3399_common.h
> >>>>> index 8e13737666..28b514a4ea 100644
> >>>>> --- a/include/configs/rk3399_common.h
> >>>>> +++ b/include/configs/rk3399_common.h
> >>>>> @@ -38,6 +38,22 @@
> >>>>>    #define CONFIG_SYS_SDRAM_BASE          0
> >>>>>    #define SDRAM_MAX_SIZE                 0xf8000000
> >>>>>
> >>>>> +#define ROCKPI_4B_IDBLOADER_IMAGE_GUID \
> >>>>> +       EFI_GUID(0x02f4d760, 0xcfd5, 0x43bd, 0x8e, 0x2d, \
> >>>>> +                0xa4, 0x2a, 0xcb, 0x33, 0xc6, 0x60)
> >>>>> +
> >>>>> +#define ROCKPI_4B_UBOOT_IMAGE_GUID \
> >>>>> +       EFI_GUID(0x4ce292da, 0x1dd8, 0x428d, 0xa1, 0xc2, \
> >>>>> +                0x77, 0x74, 0x3e, 0xf8, 0xb9, 0x6e)
> >>>>> +
> >>>>> +#define ROCKPI_4C_IDBLOADER_IMAGE_GUID \
> >>>>> +       EFI_GUID(0xfd68510c, 0x12d3, 0x4f0a, 0xb8, 0xd3, \
> >>>>> +                0xd8, 0x79, 0xe1, 0xd3, 0xa5, 0x40)
> >>>>> +
> >>>>> +#define ROCKPI_4C_UBOOT_IMAGE_GUID \
> >>>>> +       EFI_GUID(0xb81fb4ae, 0xe4f3, 0x471b, 0x99, 0xb4, \
> >>>>> +                0x0b, 0x3d, 0xa5, 0x49, 0xce, 0x13)
> >>>>> +
> >>>>>    #ifndef CONFIG_SPL_BUILD
> >>>>>
> >>>>>    #define ENV_MEM_LAYOUT_SETTINGS \
> >>>>> --
> >>>>> 2.25.1
> >>>>>