diff mbox series

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

Message ID 20220928092956.2535777-6-sughosh.ganu@linaro.org
State Superseded
Headers show
Series FWU: Add FWU Multi Bank Update feature support | expand

Commit Message

Sughosh Ganu Sept. 28, 2022, 9:29 a.m. UTC 
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(+)

Comments

Etienne Carriere Sept. 30, 2022, 5:51 a.m. UTC | #1 
On Wed, 28 Sept 2022 at 11:30, Sughosh Ganu <sughosh.ganu@linaro.org> 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>
> ---

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> with 1 comment:

Could the commit header line be updated? the patch no more targets
only DK2 among stm32mp1 platforms:
- stm32mp1: dk2: Add image information for capsule updates
+stm32mp1: Add image information for capsule updates

Regards,
Etienne


> 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);
> +#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;
> +#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
>
AKASHI Takahiro Oct. 3, 2022, 10:56 a.m. UTC | #2 
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?
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.
(I hope that you add a sanity checker against it as well.)

-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
>
Sughosh Ganu Oct. 3, 2022, 11:10 a.m. UTC | #3 
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
> >
AKASHI Takahiro Oct. 4, 2022, 3:04 a.m. UTC | #4 
On Mon, Oct 03, 2022 at 04:40:04PM +0530, Sughosh Ganu wrote:
> 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's because fw_images is hard-coded in efi_firmware.c by your change.

> That is not mandatory,

In this sense, it is mandatory unless you want to write a new FMP driver
from the scratch.

> 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().

I meant that the check takes place at some time in initialization,
but yes, efi_fmp_find() works as a safe guard.

-Takahiro Akashi

> -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
> > >
diff mbox series

Patch

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);
+#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;
+#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
 /*****************************************************************************/