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

[Sughosh Ganu]

hi Etienne,

On Tue, 6 Sept 2022 at 12:57, Etienne Carriere
<etienne.carriere at 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 at 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 at linaro.org>
> > Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > Acked-by: Ilias Apalodimas <ilias.apalodimas at 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
> >