Re: [PATCHv3 4/5] fwu: DeveloperBox: add support for FWU

[Jassi Brar <jassisinghbrar@gmail.com>]

On Wed, Jan 18, 2023 at 8:46 AM Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 1/9/23 02:07, Jassi Brar wrote:
> > Add code to support FWU_MULTI_BANK_UPDATE.
> > The platform does not have gpt-partition storage for
> > Banks and MetaData, rather it used SPI-NOR backed
> > mtd regions for the purpose.
> >
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >   board/socionext/developerbox/Makefile       |  1 +
> >   board/socionext/developerbox/developerbox.c |  8 ++
> >   board/socionext/developerbox/fwu_plat.c     | 57 ++++++++++++
> >   configs/synquacer_developerbox_defconfig    | 13 ++-
> >   doc/board/socionext/developerbox.rst        | 96 +++++++++++++++++++++
> >   include/configs/synquacer.h                 | 10 +++
> >   6 files changed, 183 insertions(+), 2 deletions(-)
> >   create mode 100644 board/socionext/developerbox/fwu_plat.c
> >
> > diff --git a/board/socionext/developerbox/Makefile b/board/socionext/developerbox/Makefile
> > index 4a46de995a..9b80ee38e7 100644
> > --- a/board/socionext/developerbox/Makefile
> > +++ b/board/socionext/developerbox/Makefile
> > @@ -7,3 +7,4 @@
> >   #
> >
> >   obj-y       := developerbox.o
> > +obj-$(CONFIG_FWU_MULTI_BANK_UPDATE) += fwu_plat.o
> > diff --git a/board/socionext/developerbox/developerbox.c b/board/socionext/developerbox/developerbox.c
> > index 6415c90c1c..2123914f21 100644
> > --- a/board/socionext/developerbox/developerbox.c
> > +++ b/board/socionext/developerbox/developerbox.c
> > @@ -20,6 +20,13 @@
> >
> >   #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
> >   struct efi_fw_image fw_images[] = {
> > +#if defined(CONFIG_FWU_MULTI_BANK_UPDATE)
> > +     {
> > +             .image_type_id = DEVELOPERBOX_FIP_IMAGE_GUID,
> > +             .fw_name = u"DEVELOPERBOX-FIP",
> > +             .image_index = 1,
> > +     },
> > +#else
> >       {
> >               .image_type_id = DEVELOPERBOX_UBOOT_IMAGE_GUID,
> >               .fw_name = u"DEVELOPERBOX-UBOOT",
> > @@ -35,6 +42,7 @@ struct efi_fw_image fw_images[] = {
> >               .fw_name = u"DEVELOPERBOX-OPTEE",
> >               .image_index = 3,
> >       },
> > +#endif
> >   };
> >
> >   struct efi_capsule_update_info update_info = {
> > diff --git a/board/socionext/developerbox/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c
> > new file mode 100644
> > index 0000000000..29b258f86c
> > --- /dev/null
> > +++ b/board/socionext/developerbox/fwu_plat.c
> > @@ -0,0 +1,57 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023, Linaro Limited
> > + */
> > +
> > +#include <efi_loader.h>
> > +#include <fwu.h>
> > +#include <fwu_mdata.h>
> > +#include <memalign.h>
> > +#include <mtd.h>
> > +
> > +#define DFU_ALT_BUF_LEN 256
> > +
> > +/* Generate dfu_alt_info from partitions */
> > +void set_dfu_alt_info(char *interface, char *devstr)
> > +{
> > +     int ret;
> > +     struct mtd_info *mtd;
> > +
> > +     ALLOC_CACHE_ALIGN_BUFFER(char, buf, DFU_ALT_BUF_LEN);
> > +     memset(buf, 0, sizeof(buf));
> > +
> > +     mtd_probe_devices();
>
>
> When you disable CONFIG_MTD for this board this code will fail.
> You need to cover this dependency via different Kconfig symbol or somewhere else.
> Maybe this file should be compiled only when CONFIG_FWU_MDATA_MTD is present.
>
Yes, i agree.


> > +
> > +     mtd = get_mtd_device_nm("nor1");
> > +     if (IS_ERR_OR_NULL(mtd))
> > +             return;
> > +
> > +     ret = fwu_gen_alt_info_from_mtd(buf, DFU_ALT_BUF_LEN, mtd);
> > +     if (ret < 0) {
> > +             log_err("Error: Failed to generate dfu_alt_info. (%d)\n", ret);
> > +             return;
> > +     }
> > +     log_debug("Make dfu_alt_info: '%s'\n", buf);
> > +
> > +     env_set("dfu_alt_info", buf);
> > +}
> > +
> > +int fwu_plat_get_alt_num(struct udevice __always_unused *dev,
> > +                      efi_guid_t *image_id, u8 *alt_num)
> > +{
> > +     return fwu_mtd_get_alt_num(image_id, alt_num, "nor1");
> > +}
> > +
> > +void fwu_plat_get_bootidx(uint *boot_idx)
> > +{
> > +     int ret;
> > +     u32 active_idx;
> > +     u32 *bootidx = boot_idx;
> > +
> > +     ret = fwu_get_active_index(&active_idx);
> > +
> > +     if (ret < 0)
> > +             *bootidx = -1;
> > +
> > +     *bootidx = active_idx;
> > +}
> > diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
> > index f69b873a36..b1085a388e 100644
> > --- a/configs/synquacer_developerbox_defconfig
> > +++ b/configs/synquacer_developerbox_defconfig
> > @@ -1,10 +1,11 @@
> >   CONFIG_ARM=y
> >   CONFIG_ARCH_SYNQUACER=y
> > -CONFIG_TEXT_BASE=0x08200000
> > +CONFIG_POSITION_INDEPENDENT=y
> > +CONFIG_SYS_TEXT_BASE=0
>
> How is this related to subject? I can't see any single line of description why
> you are doing this change.
>
> >   CONFIG_SYS_MALLOC_LEN=0x1000000
> >   CONFIG_SYS_MALLOC_F_LEN=0x400
> >   CONFIG_ENV_SIZE=0x30000
> > -CONFIG_ENV_OFFSET=0x300000
> > +CONFIG_ENV_OFFSET=0x580000
>
> Why are you changing this offset?
>
Synquacer's boot flow is revamped with FWU support. So I think these
should probably come in a separate patch


> > diff --git a/doc/board/socionext/developerbox.rst b/doc/board/socionext/developerbox.rst
> > index 2d943c23be..be872aa79d 100644
> > --- a/doc/board/socionext/developerbox.rst
> > +++ b/doc/board/socionext/developerbox.rst
> > @@ -85,3 +85,99 @@ Once the flasher tool is running we are ready flash the UEFI image::
> >
> >   After transferring the SPI_NOR_UBOOT.fd, turn off the DSW2-7 and reset the board.
> >
> > +
> > +Enable FWU Multi Bank Update
> > +============================
> > +
> > +DeveloperBox supports the FWU Multi Bank Update. You *MUST* update both *SCP firmware* and *TF-A* for this feature. This will change the layout and the boot process but you can switch back to the normal one by changing the DSW 1-4 off.
>
> Isn't here also certain limit for number of chars on the line?
>
ok.


> > +
> > +By default, the CONFIG_FWU_NUM_BANKS and COFNIG_FWU_NUM_IMAGES_PER_BANKS are set to 2 and 1 respectively. This uses FIP (Firmware Image Package) type image which contains TF-A, U-Boot and OP-TEE (the OP-TEE is optional.)
>
> long line and type CONFIG
>
ok

>
> I would also prefer to describe the logic used on TF-A side. I saw that you are
> using one register to find out how to boot it.
>
That is not a register, but a location in NOR that contains flag to

> commit a19382521c583b3dde89df14678b011960097f6c
> Author:     Jassi Brar <jaswinder.singh@linaro.org>
> AuthorDate: Mon May 23 13:16:01 2022 -0500
> Commit:     Jassi Brar <jaswinder.singh@linaro.org>
> CommitDate: Mon Jun 27 13:12:24 2022 -0500
>
>      feat(synquacer): add FWU Multi Bank Update support
>
>      Add FWU Multi Bank Update support. This reads the platform metadata
>      and update the FIP base address so that BL2 can load correct BL3X
>      based on the boot index.
>
>      Cc: Sumit Garg <sumit.garg@linaro.org>
>      Cc: Masahisa Kojima <masahisa.kojima@linaro.org>
>      Cc: Manish V Badarkhe <manish.badarkhe@arm.com>
>      Cc: Leonardo Sandoval <leonardo.sandoval@linaro.org>
>      Change-Id: I5d96972bc4b3b9a12a8157117e53a05da5ce89f6
>      Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>      Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>
>
> but it is not obviously the code which is decoding mdata structure.
> It means boot flow description would be good to have.
>
The 'platform metadata' is not the fwu-mdata. It is a choice of fip
(with different bootloader) to boot from. FWU A/B support is not yet
implemented.

thanks.