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

[Michal Simek]

On 6/9/22 14:30, Sughosh Ganu wrote:
> From: Masami Hiramatsu <masami.hiramatsu at 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 at 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 at 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 at 0 {
> +		spi_flash: spi-flash at 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 at 600000 {
> -					label = "FIP";
> +					label = "FIP-Bank0";
>   					reg = <0x600000 0x400000>;
>   				};
> +				partition at 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