Re: [PATCH v11 05/15] stm32mp1: dk2: Add image information for capsule updates
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 > 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 > > > Reviewed-by: Patrick Delaunay > > > Reviewed-by: Ilias Apalodimas > > > --- > > > 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 > > > #include > > > #include > > > +#include > > > #include > > > #include > > > #include > > > @@ -87,6 +88,16 @@ > > > #define USB_START_LOW_THRESHOLD_UV 123 > > > #define USB_START_HIGH_THRESHOLD_UV 215 > > > > > > +#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(_images[0].image_type_id, _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 > > >
Re: [PATCH v11 05/15] stm32mp1: dk2: Add image information for capsule updates
hi Takahiro, On Mon, 3 Oct 2022 at 16:27, Takahiro Akashi 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 > > Reviewed-by: Patrick Delaunay > > Reviewed-by: Ilias Apalodimas > > --- > > 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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -87,6 +88,16 @@ > > #define USB_START_LOW_THRESHOLD_UV 123 > > #define USB_START_HIGH_THRESHOLD_UV 215 > > > > +#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(_images[0].image_type_id, _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 > >
Re: [PATCH v11 05/15] stm32mp1: dk2: Add image information for capsule updates
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 > Reviewed-by: Patrick Delaunay > Reviewed-by: Ilias Apalodimas > --- > 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 > #include > #include > +#include > #include > #include > #include > @@ -87,6 +88,16 @@ > #define USB_START_LOW_THRESHOLD_UV 123 > #define USB_START_HIGH_THRESHOLD_UV 215 > > +#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(_images[0].image_type_id, _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 >
Re: [PATCH v11 05/15] stm32mp1: dk2: Add image information for capsule updates
On Wed, 28 Sept 2022 at 11:30, 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 > Reviewed-by: Patrick Delaunay > Reviewed-by: Ilias Apalodimas > --- Reviewed-by: Etienne Carriere 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 > #include > #include > +#include > #include > #include > #include > @@ -87,6 +88,16 @@ > #define USB_START_LOW_THRESHOLD_UV 123 > #define USB_START_HIGH_THRESHOLD_UV215 > > +#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(_images[0].image_type_id, _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 >