Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox

Michal Simek
2022-06-20T08:23:54Z

On 6/9/22 14:30, Sughosh Ganu wrote:
> From: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>
> The DeveloperBox platform can support the FWU Multi bank
> update. SCP firmware will switch the boot mode by DSW3-4
> and load the Multi bank update supported TF-A BL2 from
> 0x600000 offset on the SPI flash. Thus it can co-exist
> with the legacy boot mode (legacy U-Boot or EDK2).
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>

I am looking at this code again while trying on xilinx HW.

> ---
> Changes in v3:
> - Change devicetree to add partitions.
> - Update fwu_plat_get_alt_num() to find the alt number from the bank index.
> - Use only 2 partitions for AB update.
> - Clear platform-mdata's boot_count to finish platform trial boot.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> .../synquacer-sc2a11-developerbox-u-boot.dtsi | 15 +-
> board/socionext/developerbox/Kconfig | 13 ++
> board/socionext/developerbox/Makefile | 1 +
> board/socionext/developerbox/fwu_plat.c | 207 ++++++++++++++++++
> include/configs/synquacer.h | 8 +
> 5 files changed, 241 insertions(+), 3 deletions(-)
> create mode 100644 board/socionext/developerbox/fwu_plat.c
>
> diff --git a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> index 095727e03c..ab4e3d1c2b 100644
> --- a/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> +++ b/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> @@ -23,7 +23,7 @@
> active_clk_edges;
> chipselect_num = <1>;
>
> - spi-flash@0 {
> + spi_flash: spi-flash@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "jedec,spi-nor";
> @@ -84,11 +84,15 @@
> label = "UBoot-Env";
> reg = <0x580000 0x80000>;
> };
> -
> + /* FWU Multi bank update partitions */
> partition@600000 {
> - label = "FIP";
> + label = "FIP-Bank0";
> reg = <0x600000 0x400000>;
> };
> + partition@a00000 {
> + label = "FIP-Bank1";
> + reg = <0xa00000 0x400000>;
> + };
> };
> };
> };
> @@ -114,6 +118,11 @@
> optee {
> status = "okay";
> };
> + fwu-mdata {
> + compatible = "u-boot,fwu-mdata-mtd";
> + fwu-mdata-store = <&spi_flash>;
> + mdata-offsets = <0x500000 0x530000>;
> + };
> };
> };
>
> diff --git a/board/socionext/developerbox/Kconfig b/board/socionext/developerbox/Kconfig
> index c181d26a44..7df6750baf 100644
> --- a/board/socionext/developerbox/Kconfig
> +++ b/board/socionext/developerbox/Kconfig
> @@ -32,4 +32,17 @@ config SYS_CONFIG_NAME
> default "synquacer"
>
> endif
> +
> +config FWU_MULTI_BANK_UPDATE
> + select FWU_MDATA_MTD
> + select DM_SPI_FLASH
> + select DM_FWU_MDATA
> + select BOARD_LATE_INIT
> +
> +config FWU_NUM_BANKS
> + default 2
> +
> +config FWU_NUM_IMAGES_PER_BANK
> + default 1
> +
> endif
> 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/fwu_plat.c b/board/socionext/developerbox/fwu_plat.c
> new file mode 100644
> index 0000000000..fd6d0e3659
> --- /dev/null
> +++ b/board/socionext/developerbox/fwu_plat.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2021, Linaro Limited
> + */
> +
> +#include <dfu.h>
> +#include <efi_loader.h>
> +#include <flash.h>
> +#include <fwu.h>
> +#include <fwu_mdata.h>
> +#include <malloc.h>
> +#include <memalign.h>
> +#include <spi.h>
> +#include <spi_flash.h>
> +
> +#include <linux/errno.h>
> +#include <linux/types.h>
> +#include <u-boot/crc.h>
> +
> +/* SPI Flash accessors */
> +static struct spi_flash *plat_spi_flash;
> +
> +static int __plat_sf_get_flash(void)
> +{
> + /* TODO: define platform spi-flash somewhere. */
> + plat_spi_flash = spi_flash_probe(CONFIG_SF_DEFAULT_BUS,
> + CONFIG_SF_DEFAULT_CS,
> + CONFIG_SF_DEFAULT_SPEED,
> + CONFIG_SF_DEFAULT_MODE);
> +
> + return 0;

What about if spi_flash_probe() fails?

You are returning 0 here all the time and below you are propagating it that
everything is fine.

> +}
> +
> +static int plat_sf_get_flash(struct spi_flash **flash)
> +{
> + int ret = 0;
> +
> + if (!plat_spi_flash)
> + ret = __plat_sf_get_flash();
> +
> + *flash = plat_spi_flash;
> +
> + return ret;
> +}
> +
> +static int sf_load_data(u32 offs, u32 size, void **data)
> +{
> + struct spi_flash *flash;
> + int ret;
> +
> + ret = plat_sf_get_flash(&flash);
> + if (ret < 0)
> + return ret;
> +
> + *data = memalign(ARCH_DMA_MINALIGN, size);
> + if (!*data)
> + return -ENOMEM;
> +
> + ret = spi_flash_read(flash, offs, size, *data);
> + if (ret < 0) {
> + free(*data);
> + *data = NULL;
> + }
> +
> + return ret;
> +}
> +
> +static int sf_save_data(u32 offs, u32 size, void *data)
> +{
> + struct spi_flash *flash;
> + u32 sect_size, nsect;
> + void *buf;
> + int ret;
> +
> + ret = plat_sf_get_flash(&flash);
> + if (ret < 0)
> + return ret;
> +
> + sect_size = flash->mtd.erasesize;
> + nsect = DIV_ROUND_UP(size, sect_size);
> + ret = spi_flash_erase(flash, offs, nsect * sect_size);

What it is interesting here that framework itself is using mtd infrastructure
but this platform driver is calling spi functions directly.
It looks a little bit nonstandard way. What's the reason for it?

> + if (ret < 0)
> + return ret;
> +
> + buf = memalign(ARCH_DMA_MINALIGN, size);
> + if (!buf)
> + return -ENOMEM;
> + memcpy(buf, data, size);
> +
> + ret = spi_flash_write(flash, offs, size, buf);
> +
> + free(buf);
> +
> + return ret;
> +}
> +
> +#define PLAT_METADATA_OFFSET 0x510000
> +#define PLAT_METADATA_SIZE (sizeof(struct devbox_metadata))
> +
> +struct __packed devbox_metadata {
> + u32 boot_index;
> + u32 boot_count;

There is the whole bootcount infrastructure for this. I think it would be much
better to use that framework instead of creating parallel one.

Thanks,
Michal

--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs