Re: [PATCH] RockPi4: Add UEFI capsule update support

[Kever Yang <kever.yang@rock-chips.com>]

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