diff mbox series

[v9,07/15] FWU: STM32MP1: Add support to read boot index from backup register

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

Commit Message

Sughosh Ganu Aug. 26, 2022, 9:57 a.m. UTC 
The FWU Multi Bank Update feature allows the platform to boot the
firmware images from one of the partitions(banks). The first stage
bootloader(fsbl) passes the value of the boot index, i.e. the bank
from which the firmware images were booted from to U-Boot. On the
STM32MP157C-DK2 board, this value is passed through one of the SoC's
backup register. Add a function to read the boot index value from the
backup register.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
Changes since V8: None

 arch/arm/mach-stm32mp/include/mach/stm32.h |  5 +++++
 board/st/stm32mp1/stm32mp1.c               | 23 ++++++++++++++++++++++
 include/fwu.h                              | 12 +++++++++++
 3 files changed, 40 insertions(+)

Comments

Etienne Carriere Sept. 6, 2022, 7:27 a.m. UTC | #1 
Hi Sughosh,

I have a last comment on this series, related to the below patch and
patch "test: dm: Add test cases for FWU Metadata uclass".

On Fri, 26 Aug 2022 at 11:58, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The FWU Multi Bank Update feature allows the platform to boot the
> firmware images from one of the partitions(banks). The first stage
> bootloader(fsbl) passes the value of the boot index, i.e. the bank
> from which the firmware images were booted from to U-Boot. On the
> STM32MP157C-DK2 board, this value is passed through one of the SoC's
> backup register. Add a function to read the boot index value from the
> backup register.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> Changes since V8: None
>
>  arch/arm/mach-stm32mp/include/mach/stm32.h |  5 +++++
>  board/st/stm32mp1/stm32mp1.c               | 23 ++++++++++++++++++++++
>  include/fwu.h                              | 12 +++++++++++
>  3 files changed, 40 insertions(+)
>
> diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h
> index c70375a723..c85ae6a34e 100644
> --- a/arch/arm/mach-stm32mp/include/mach/stm32.h
> +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h
> @@ -112,11 +112,16 @@ enum boot_device {
>  #ifdef CONFIG_STM32MP15x
>  #define TAMP_BACKUP_MAGIC_NUMBER       TAMP_BACKUP_REGISTER(4)
>  #define TAMP_BACKUP_BRANCH_ADDRESS     TAMP_BACKUP_REGISTER(5)
> +#define TAMP_FWU_BOOT_INFO_REG         TAMP_BACKUP_REGISTER(10)
>  #define TAMP_COPRO_RSC_TBL_ADDRESS     TAMP_BACKUP_REGISTER(17)
>  #define TAMP_COPRO_STATE               TAMP_BACKUP_REGISTER(18)
>  #define TAMP_BOOT_CONTEXT              TAMP_BACKUP_REGISTER(20)
>  #define TAMP_BOOTCOUNT                 TAMP_BACKUP_REGISTER(21)
>
> +#define TAMP_FWU_BOOT_IDX_MASK         GENMASK(3, 0)
> +
> +#define TAMP_FWU_BOOT_IDX_OFFSET       0
> +
>  #define TAMP_COPRO_STATE_OFF           0
>  #define TAMP_COPRO_STATE_INIT          1
>  #define TAMP_COPRO_STATE_CRUN          2
> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> index bfec0a710d..dd95babb49 100644
> --- a/board/st/stm32mp1/stm32mp1.c
> +++ b/board/st/stm32mp1/stm32mp1.c
> @@ -966,3 +966,26 @@ 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>
> +
> +/**
> + * fwu_plat_get_bootidx() - Get the value of the boot index
> + * @boot_idx: Boot index value
> + *
> + * Get the value of the bank(partition) from which the platform
> + * has booted. This value is passed to U-Boot from the earlier
> + * stage bootloader which loads and boots all the relevant
> + * firmware images
> + *
> + */
> +void fwu_plat_get_bootidx(void *boot_idx)
> +{
> +       u32 *bootidx = boot_idx;
> +
> +       *bootidx = (readl(TAMP_FWU_BOOT_INFO_REG) >>
> +                   TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK;
> +}
> +#endif /* CONFIG_FWU_MULTI_BANK_UPDATE */
> diff --git a/include/fwu.h b/include/fwu.h
> index 7ebd3a7115..b8c207deaf 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -240,4 +240,16 @@ int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
>   *
>   */
>  int fwu_plat_get_update_index(uint *update_idx);
> +
> +/**
> + * fwu_plat_get_bootidx() - Get the value of the boot index
> + * @boot_idx: Boot index value
> + *
> + * Get the value of the bank(partition) from which the platform
> + * has booted. This value is passed to U-Boot from the earlier
> + * stage bootloader which loads and boots all the relevant
> + * firmware images
> + *
> + */
> +void fwu_plat_get_bootidx(void *boot_idx);

It's maybe not clean to pass a void * to something callee shall write to.
Could the reference be passed with an explicit type pointer as 'unsigned int *'?

Best regards,
Etienne

>  #endif /* _FWU_H_ */
> --
> 2.34.1
>
Sughosh Ganu Sept. 6, 2022, 7:37 a.m. UTC | #2 
hi Etienne,

On Tue, 6 Sept 2022 at 12:57, Etienne Carriere
<etienne.carriere@linaro.org> wrote:
>
> Hi Sughosh,
>
> I have a last comment on this series, related to the below patch and
> patch "test: dm: Add test cases for FWU Metadata uclass".
>
> On Fri, 26 Aug 2022 at 11:58, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The FWU Multi Bank Update feature allows the platform to boot the
> > firmware images from one of the partitions(banks). The first stage
> > bootloader(fsbl) passes the value of the boot index, i.e. the bank
> > from which the firmware images were booted from to U-Boot. On the
> > STM32MP157C-DK2 board, this value is passed through one of the SoC's
> > backup register. Add a function to read the boot index value from the
> > backup register.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > Changes since V8: None
> >
> >  arch/arm/mach-stm32mp/include/mach/stm32.h |  5 +++++
> >  board/st/stm32mp1/stm32mp1.c               | 23 ++++++++++++++++++++++
> >  include/fwu.h                              | 12 +++++++++++
> >  3 files changed, 40 insertions(+)
> >
> > diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h
> > index c70375a723..c85ae6a34e 100644
> > --- a/arch/arm/mach-stm32mp/include/mach/stm32.h
> > +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h
> > @@ -112,11 +112,16 @@ enum boot_device {
> >  #ifdef CONFIG_STM32MP15x
> >  #define TAMP_BACKUP_MAGIC_NUMBER       TAMP_BACKUP_REGISTER(4)
> >  #define TAMP_BACKUP_BRANCH_ADDRESS     TAMP_BACKUP_REGISTER(5)
> > +#define TAMP_FWU_BOOT_INFO_REG         TAMP_BACKUP_REGISTER(10)
> >  #define TAMP_COPRO_RSC_TBL_ADDRESS     TAMP_BACKUP_REGISTER(17)
> >  #define TAMP_COPRO_STATE               TAMP_BACKUP_REGISTER(18)
> >  #define TAMP_BOOT_CONTEXT              TAMP_BACKUP_REGISTER(20)
> >  #define TAMP_BOOTCOUNT                 TAMP_BACKUP_REGISTER(21)
> >
> > +#define TAMP_FWU_BOOT_IDX_MASK         GENMASK(3, 0)
> > +
> > +#define TAMP_FWU_BOOT_IDX_OFFSET       0
> > +
> >  #define TAMP_COPRO_STATE_OFF           0
> >  #define TAMP_COPRO_STATE_INIT          1
> >  #define TAMP_COPRO_STATE_CRUN          2
> > diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> > index bfec0a710d..dd95babb49 100644
> > --- a/board/st/stm32mp1/stm32mp1.c
> > +++ b/board/st/stm32mp1/stm32mp1.c
> > @@ -966,3 +966,26 @@ 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>
> > +
> > +/**
> > + * fwu_plat_get_bootidx() - Get the value of the boot index
> > + * @boot_idx: Boot index value
> > + *
> > + * Get the value of the bank(partition) from which the platform
> > + * has booted. This value is passed to U-Boot from the earlier
> > + * stage bootloader which loads and boots all the relevant
> > + * firmware images
> > + *
> > + */
> > +void fwu_plat_get_bootidx(void *boot_idx)
> > +{
> > +       u32 *bootidx = boot_idx;
> > +
> > +       *bootidx = (readl(TAMP_FWU_BOOT_INFO_REG) >>
> > +                   TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK;
> > +}
> > +#endif /* CONFIG_FWU_MULTI_BANK_UPDATE */
> > diff --git a/include/fwu.h b/include/fwu.h
> > index 7ebd3a7115..b8c207deaf 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -240,4 +240,16 @@ int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> >   *
> >   */
> >  int fwu_plat_get_update_index(uint *update_idx);
> > +
> > +/**
> > + * fwu_plat_get_bootidx() - Get the value of the boot index
> > + * @boot_idx: Boot index value
> > + *
> > + * Get the value of the bank(partition) from which the platform
> > + * has booted. This value is passed to U-Boot from the earlier
> > + * stage bootloader which loads and boots all the relevant
> > + * firmware images
> > + *
> > + */
> > +void fwu_plat_get_bootidx(void *boot_idx);
>
> It's maybe not clean to pass a void * to something callee shall write to.
> Could the reference be passed with an explicit type pointer as 'unsigned int *'?

The reason I chose this to be a void pointer is that different
platforms might have different means of passing the boot index value
-- on certain platforms this might be passed through a 32 bit
register, 8 bit register on some other. Since this is a platform
function, the corresponding platform would know how the index value is
being passed and would use the correct sized pointer.

-sughosh

>
> Best regards,
> Etienne
>
> >  #endif /* _FWU_H_ */
> > --
> > 2.34.1
> >
Sughosh Ganu Sept. 6, 2022, 7:44 a.m. UTC | #3 
hi Etienne,

On Tue, 6 Sept 2022 at 13:07, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Etienne,
>
> On Tue, 6 Sept 2022 at 12:57, Etienne Carriere
> <etienne.carriere@linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > I have a last comment on this series, related to the below patch and
> > patch "test: dm: Add test cases for FWU Metadata uclass".
> >
> > On Fri, 26 Aug 2022 at 11:58, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The FWU Multi Bank Update feature allows the platform to boot the
> > > firmware images from one of the partitions(banks). The first stage
> > > bootloader(fsbl) passes the value of the boot index, i.e. the bank
> > > from which the firmware images were booted from to U-Boot. On the
> > > STM32MP157C-DK2 board, this value is passed through one of the SoC's
> > > backup register. Add a function to read the boot index value from the
> > > backup register.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > > Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since V8: None
> > >
> > >  arch/arm/mach-stm32mp/include/mach/stm32.h |  5 +++++
> > >  board/st/stm32mp1/stm32mp1.c               | 23 ++++++++++++++++++++++
> > >  include/fwu.h                              | 12 +++++++++++
> > >  3 files changed, 40 insertions(+)
> > >
> > > diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h
> > > index c70375a723..c85ae6a34e 100644
> > > --- a/arch/arm/mach-stm32mp/include/mach/stm32.h
> > > +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h
> > > @@ -112,11 +112,16 @@ enum boot_device {
> > >  #ifdef CONFIG_STM32MP15x
> > >  #define TAMP_BACKUP_MAGIC_NUMBER       TAMP_BACKUP_REGISTER(4)
> > >  #define TAMP_BACKUP_BRANCH_ADDRESS     TAMP_BACKUP_REGISTER(5)
> > > +#define TAMP_FWU_BOOT_INFO_REG         TAMP_BACKUP_REGISTER(10)
> > >  #define TAMP_COPRO_RSC_TBL_ADDRESS     TAMP_BACKUP_REGISTER(17)
> > >  #define TAMP_COPRO_STATE               TAMP_BACKUP_REGISTER(18)
> > >  #define TAMP_BOOT_CONTEXT              TAMP_BACKUP_REGISTER(20)
> > >  #define TAMP_BOOTCOUNT                 TAMP_BACKUP_REGISTER(21)
> > >
> > > +#define TAMP_FWU_BOOT_IDX_MASK         GENMASK(3, 0)
> > > +
> > > +#define TAMP_FWU_BOOT_IDX_OFFSET       0
> > > +
> > >  #define TAMP_COPRO_STATE_OFF           0
> > >  #define TAMP_COPRO_STATE_INIT          1
> > >  #define TAMP_COPRO_STATE_CRUN          2
> > > diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> > > index bfec0a710d..dd95babb49 100644
> > > --- a/board/st/stm32mp1/stm32mp1.c
> > > +++ b/board/st/stm32mp1/stm32mp1.c
> > > @@ -966,3 +966,26 @@ 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>
> > > +
> > > +/**
> > > + * fwu_plat_get_bootidx() - Get the value of the boot index
> > > + * @boot_idx: Boot index value
> > > + *
> > > + * Get the value of the bank(partition) from which the platform
> > > + * has booted. This value is passed to U-Boot from the earlier
> > > + * stage bootloader which loads and boots all the relevant
> > > + * firmware images
> > > + *
> > > + */
> > > +void fwu_plat_get_bootidx(void *boot_idx)
> > > +{
> > > +       u32 *bootidx = boot_idx;
> > > +
> > > +       *bootidx = (readl(TAMP_FWU_BOOT_INFO_REG) >>
> > > +                   TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK;
> > > +}
> > > +#endif /* CONFIG_FWU_MULTI_BANK_UPDATE */
> > > diff --git a/include/fwu.h b/include/fwu.h
> > > index 7ebd3a7115..b8c207deaf 100644
> > > --- a/include/fwu.h
> > > +++ b/include/fwu.h
> > > @@ -240,4 +240,16 @@ int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> > >   *
> > >   */
> > >  int fwu_plat_get_update_index(uint *update_idx);
> > > +
> > > +/**
> > > + * fwu_plat_get_bootidx() - Get the value of the boot index
> > > + * @boot_idx: Boot index value
> > > + *
> > > + * Get the value of the bank(partition) from which the platform
> > > + * has booted. This value is passed to U-Boot from the earlier
> > > + * stage bootloader which loads and boots all the relevant
> > > + * firmware images
> > > + *
> > > + */
> > > +void fwu_plat_get_bootidx(void *boot_idx);
> >
> > It's maybe not clean to pass a void * to something callee shall write to.
> > Could the reference be passed with an explicit type pointer as 'unsigned int *'?
>
> The reason I chose this to be a void pointer is that different
> platforms might have different means of passing the boot index value
> -- on certain platforms this might be passed through a 32 bit
> register, 8 bit register on some other. Since this is a platform
> function, the corresponding platform would know how the index value is
> being passed and would use the correct sized pointer.

Sorry, I got this wrong. This can indeed be passed as an unsigned int
pointer. Will fix this in the next version. Thanks.

-sughosh

>
> -sughosh
>
> >
> > Best regards,
> > Etienne
> >
> > >  #endif /* _FWU_H_ */
> > > --
> > > 2.34.1
> > >
diff mbox series

Patch

diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h
index c70375a723..c85ae6a34e 100644
--- a/arch/arm/mach-stm32mp/include/mach/stm32.h
+++ b/arch/arm/mach-stm32mp/include/mach/stm32.h
@@ -112,11 +112,16 @@  enum boot_device {
 #ifdef CONFIG_STM32MP15x
 #define TAMP_BACKUP_MAGIC_NUMBER	TAMP_BACKUP_REGISTER(4)
 #define TAMP_BACKUP_BRANCH_ADDRESS	TAMP_BACKUP_REGISTER(5)
+#define TAMP_FWU_BOOT_INFO_REG		TAMP_BACKUP_REGISTER(10)
 #define TAMP_COPRO_RSC_TBL_ADDRESS	TAMP_BACKUP_REGISTER(17)
 #define TAMP_COPRO_STATE		TAMP_BACKUP_REGISTER(18)
 #define TAMP_BOOT_CONTEXT		TAMP_BACKUP_REGISTER(20)
 #define TAMP_BOOTCOUNT			TAMP_BACKUP_REGISTER(21)
 
+#define TAMP_FWU_BOOT_IDX_MASK		GENMASK(3, 0)
+
+#define TAMP_FWU_BOOT_IDX_OFFSET	0
+
 #define TAMP_COPRO_STATE_OFF		0
 #define TAMP_COPRO_STATE_INIT		1
 #define TAMP_COPRO_STATE_CRUN		2
diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
index bfec0a710d..dd95babb49 100644
--- a/board/st/stm32mp1/stm32mp1.c
+++ b/board/st/stm32mp1/stm32mp1.c
@@ -966,3 +966,26 @@  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>
+
+/**
+ * fwu_plat_get_bootidx() - Get the value of the boot index
+ * @boot_idx: Boot index value
+ *
+ * Get the value of the bank(partition) from which the platform
+ * has booted. This value is passed to U-Boot from the earlier
+ * stage bootloader which loads and boots all the relevant
+ * firmware images
+ *
+ */
+void fwu_plat_get_bootidx(void *boot_idx)
+{
+	u32 *bootidx = boot_idx;
+
+	*bootidx = (readl(TAMP_FWU_BOOT_INFO_REG) >>
+		    TAMP_FWU_BOOT_IDX_OFFSET) & TAMP_FWU_BOOT_IDX_MASK;
+}
+#endif /* CONFIG_FWU_MULTI_BANK_UPDATE */
diff --git a/include/fwu.h b/include/fwu.h
index 7ebd3a7115..b8c207deaf 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -240,4 +240,16 @@  int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
  *
  */
 int fwu_plat_get_update_index(uint *update_idx);
+
+/**
+ * fwu_plat_get_bootidx() - Get the value of the boot index
+ * @boot_idx: Boot index value
+ *
+ * Get the value of the bank(partition) from which the platform
+ * has booted. This value is passed to U-Boot from the earlier
+ * stage bootloader which loads and boots all the relevant
+ * firmware images
+ *
+ */
+void fwu_plat_get_bootidx(void *boot_idx);
 #endif /* _FWU_H_ */