Re: [PATCH v5 06/23] FWU: stm32mp1: Add helper functions for accessing FWU metadata

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

Hi Sughosh, 

On Thu, Jun 09, 2022 at 05:59:53PM +0530, Sughosh Ganu wrote:
> Add helper functions needed for accessing the FWU metadata which
> contains information on the updatable images. These functions have
> been added for the STM32MP157C-DK2 board which has the updatable
> images on the uSD card, formatted as GPT partitions.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  board/st/stm32mp1/stm32mp1.c | 115 +++++++++++++++++++++++++++++++++++
>  include/fwu.h                |   2 +
>  2 files changed, 117 insertions(+)
> 
> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> index 62d98ad776..e68bf09955 100644
> --- a/board/st/stm32mp1/stm32mp1.c
> +++ b/board/st/stm32mp1/stm32mp1.c
> @@ -7,9 +7,11 @@
>  
>  #include <common.h>
>  #include <adc.h>
> +#include <blk.h>
>  #include <bootm.h>
>  #include <clk.h>
>  #include <config.h>
> +#include <dfu.h>
>  #include <dm.h>
>  #include <efi_loader.h>
>  #include <env.h>
> @@ -25,9 +27,11 @@
>  #include <log.h>
>  #include <malloc.h>
>  #include <misc.h>
> +#include <mmc.h>
>  #include <mtd_node.h>
>  #include <net.h>
>  #include <netdev.h>
> +#include <part.h>
>  #include <phy.h>
>  #include <remoteproc.h>
>  #include <reset.h>
> @@ -967,3 +971,114 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size)
>  }
>  
>  U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);
> +
> +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +
> +static int get_gpt_dfu_identifier(struct blk_desc *desc, efi_guid_t *image_guid)
> +{
> +	int i;
> +	struct disk_partition info;
> +	efi_guid_t unique_part_guid;
> +
> +	for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> +		if (part_get_info(desc, i, &info))
> +			continue;
> +		uuid_str_to_bin(info.uuid, unique_part_guid.b,
> +				UUID_STR_FORMAT_GUID);
> +
> +		if (!guidcmp(&unique_part_guid, image_guid))
> +			return i;
> +	}
> +
> +	log_err("No partition found with image_guid %pUs\n", image_guid);
> +	return -ENOENT;
> +}
> +
> +static int gpt_plat_get_alt_num(struct blk_desc *desc, efi_guid_t *image_guid,
> +				int *alt_num)

Does this really need to be defined per platform?

Most of the stuff in here are generic apart from the info of were  the
metadata is stored.  So wouldn't it better to move this in the generic API 
and add an argument for the dfu device type?

The platform portion would then just call this function with an extra arg
e.g DFU_DEV_MMC

> +{
> +	int ret = -1;
> +	int i, part, dev_num;
> +	int nalt;
> +	struct dfu_entity *dfu;
> +
> +	dev_num = desc->devnum;
> +	part = get_gpt_dfu_identifier(desc, image_guid);
> +	if (part < 0)
> +		return -ENOENT;
> +
> +	dfu_init_env_entities(NULL, NULL);

[...]


Regards
/Ilias