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

Sughosh Ganu
2022-06-13T12:37:46Z

hi Ilias,

On Fri, 10 Jun 2022 at 17:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> 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

Yes, it makes sense. I will make the change that you suggest. Thanks.

-sughosh

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