Re: [PATCH v5 07/23] FWU: STM32MP1: Add support to read boot index from backup register

Sughosh Ganu
2022-06-23T06:30:29Z

On Tue, 21 Jun 2022 at 16:57, Patrick DELAUNAY
<patrick.delaunay@foss.st.com> wrote:
>
> Hi,
>
> On 6/9/22 14:29, Sughosh Ganu 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>
> > ---
> > arch/arm/mach-stm32mp/include/mach/stm32.h | 4 ++++
> > board/st/stm32mp1/stm32mp1.c | 7 +++++++
> > include/fwu.h | 2 +-
> > 3 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h
> > index 47e88fc3dc..40995ee142 100644
> > --- a/arch/arm/mach-stm32mp/include/mach/stm32.h
> > +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h
> > @@ -100,6 +100,7 @@ enum boot_device {
> > #define TAMP_BACKUP_REGISTER(x) (STM32_TAMP_BASE + 0x100 + 4 * x)
> > #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)
> > @@ -118,6 +119,9 @@ enum boot_device {
> > #define TAMP_BOOT_INSTANCE_MASK GENMASK(3, 0)
> > #define TAMP_BOOT_FORCED_MASK GENMASK(7, 0)
> > #define TAMP_BOOT_DEBUG_ON BIT(16)
> > +#define TAMP_FWU_BOOT_IDX_MASK GENMASK(3, 0)
> > +
> > +#define TAMP_FWU_BOOT_IDX_OFFSET 0
> >
>
>
> please don't mix the 2 TAMP_FWU defines with define and enum for TAMP_BOOT
>
> => move the 2 defines before TAMP_COPRO defines.
>
>
> #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

Will change as per your suggestion. Thanks.

-sughosh

>
>
> #define TAMP_COPRO_STATE_OFF 0
> #define TAMP_COPRO_STATE_INIT 1
>
>
> > enum forced_boot_mode {
> > BOOT_NORMAL = 0x00,
> > diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
> > index e68bf09955..dff41ed6f6 100644
> > --- a/board/st/stm32mp1/stm32mp1.c
> > +++ b/board/st/stm32mp1/stm32mp1.c
> > @@ -1081,4 +1081,11 @@ int fwu_plat_get_update_index(u32 *update_idx)
> > return ret;
> > }
> >
> > +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 36e58afa29..41774ff9e2 100644
> > --- a/include/fwu.h
> > +++ b/include/fwu.h
> > @@ -46,7 +46,7 @@ int fwu_revert_boot_index(void);
> > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
> > int fwu_clear_accept_image(efi_guid_t *img_type_id, u32 bank);
> >
> > -
> > +void fwu_plat_get_bootidx(void *boot_idx);
> > int fwu_plat_get_alt_num(struct udevice *dev, efi_guid_t *image_guid,
> > int *alt_num);
> > int fwu_plat_get_update_index(u32 *update_idx);
>
> With the modifications:
>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>
> Thanks
> Patrick
>
>