hi Etienne,
On Tue, 6 Sept 2022 at 13:07, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
>
> hi Etienne,
>
> On Tue, 6 Sept 2022 at 12:57, Etienne Carriere
> <etienne.carri...@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.g...@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.g...@linaro.org>
> > > Reviewed-by: Patrick Delaunay <patrick.delau...@foss.st.com>
> > > Acked-by: Ilias Apalodimas <ilias.apalodi...@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
> > >
