Re: [PATCH v11 05/15] stm32mp1: dk2: Add image information for capsule updates

Sughosh Ganu
2022-10-03T11:10:25Z

hi Takahiro,

On Mon, 3 Oct 2022 at 16:27, Takahiro Akashi <takahiro.akashi@linaro.org> wrote:
>
> Hi Sughosh,
>
> On Wed, Sep 28, 2022 at 02:59:46PM +0530, Sughosh Ganu wrote:
> > Enabling capsule update functionality on the platform requires
> > populating information on the images that are to be updated using the
> > functionality. Do so for the DK2 board.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes since V10:
> > * Use image_index value of 1 for the FIP image as it is now relevant
> >
> > board/st/stm32mp1/stm32mp1.c | 18 ++++++++++++++++++
> > include/configs/stm32mp15_common.h | 4 ++++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> > index 8c162b42a5..e43dab018f 100644
> > --- a/board/st/stm32mp1/stm32mp1.c
> > +++ b/board/st/stm32mp1/stm32mp1.c
> > @@ -11,6 +11,7 @@
> > #include <clk.h>
> > #include <config.h>
> > #include <dm.h>
> > +#include <efi_loader.h>
> > #include <env.h>
> > #include <env_internal.h>
> > #include <fdt_simplefb.h>
> > @@ -87,6 +88,16 @@
> > #define USB_START_LOW_THRESHOLD_UV 1230000
> > #define USB_START_HIGH_THRESHOLD_UV 2150000
> >
> > +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> > +struct efi_fw_image fw_images[1];
> > +
> > +struct efi_capsule_update_info update_info = {
> > + .images = fw_images,
> > +};
> > +
> > +u8 num_image_type_guids = ARRAY_SIZE(fw_images);
>
> The definition of num_image_type_guids is always the same
> across boards. Why do we need it for every platform?

Yes, but that is because at present every platform is declaring the
same variable name(fw_images) for the struct efi_fw_image. That is not
mandatory, and a subsequent addition can declare a variable with a
different name. But in case there is consensus on using a fixed
variable name for the structure, I can make the change that you are
suggesting subsequently. I will work on it if needed after the FWU
patches upstreaming work is done.

> I believe that we can remove it. Anyhow it's not documented
> in doc/develop/uefi/uefi.rst.
>
> > +#endif /* EFI_HAVE_CAPSULE_SUPPORT */
> > +
> > int board_early_init_f(void)
> > {
> > /* nothing to do, only used in SPL */
> > @@ -666,6 +677,13 @@ int board_init(void)
> >
> > setup_led(LEDST_ON);
> >
> > +#if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> > + efi_guid_t image_type_guid = STM32MP_FIP_IMAGE_GUID;
> > +
> > + guidcpy(&fw_images[0].image_type_id, &image_type_guid);
> > + fw_images[0].fw_name = u"STM32MP-FIP";
> > + fw_images[0].image_index = 1;
>
> Then, we should describe that image_index must be 1 to num_image_type_guids
> (or strictly, number of descriptors returned by GetImageInfo()) in
> the document above.

Okay

> (I hope that you add a sanity checker against it as well.)

Yes, in the efi_fmp_find(), there is a check for the index value
passed through the capsule against the value returned by the
GetImageInfo().

-sughosh

>
> -Takahiro Akashi
>
> > +#endif
> > return 0;
> > }
> >
> > diff --git a/include/configs/stm32mp15_common.h b/include/configs/stm32mp15_common.h
> > index c5412ffeb3..bb19dae945 100644
> > --- a/include/configs/stm32mp15_common.h
> > +++ b/include/configs/stm32mp15_common.h
> > @@ -34,6 +34,10 @@
> > #define CONFIG_SERVERIP 192.168.1.1
> > #endif
> >
> > +#define STM32MP_FIP_IMAGE_GUID \
> > + EFI_GUID(0x19d5df83, 0x11b0, 0x457b, 0xbe, 0x2c, \
> > + 0x75, 0x59, 0xc1, 0x31, 0x42, 0xa5)
> > +
> > /*****************************************************************************/
> > #ifdef CONFIG_DISTRO_DEFAULTS
> > /*****************************************************************************/
> > --
> > 2.34.1
> >