| Message ID | 20220609123010.1017463-20-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add FWU Multi Bank Update feature support | expand |
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> > --- > 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; > +} > + > +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); > + 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 Meta data offsets based on DT is somewhere else. I would expect that this is read from DT instead of hardcoding it here. 124 fwu-mdata { 125 compatible = "u-boot,fwu-mdata-mtd"; 126 fwu-mdata-store = <&spi_flash>; 127 mdata-offsets = <0x500000 0x530000>; 128 }; Thanks, Michal
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
On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > On 6/9/22 14:30, Sughosh Ganu wrote: > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > .... > > +} > > + > > +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? > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > + > > +#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. > Yes, this goes too. Thanks.
Hi all, On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > .... > > > > +} > > > + > > > +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? > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > + > > > +#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. > > > Yes, this goes too. Is bootcount really suited for this case? AFAIK bootcount either requires device specific registers (which won't reset on reboots), or an environment you can write data to. But what if a user wants to disable writing the env variables and the device doesn't have a set of registers we can use? Thanks /Ilias > > Thanks.
On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi all, > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > > .... > > > > > > +} > > > > + > > > > +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? > > > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > > > + > > > > +#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. > > > > > Yes, this goes too. > > Is bootcount really suited for this case? > AFAIK bootcount either requires device specific registers (which won't > reset on reboots), or an environment you can write data to. > But what if a user wants to disable writing the env variables and the > device doesn't have a set of registers we can use? > Maybe it should be moved in 'struct fwu_mdata' ? thnx
Hi Jassi On Mon, 18 Jul 2022 at 18:08, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi all, > > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > > > > .... > > > > > > > > +} > > > > > + > > > > > +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? > > > > > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > > > > > + > > > > > +#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. > > > > > > > Yes, this goes too. > > > > Is bootcount really suited for this case? > > AFAIK bootcount either requires device specific registers (which won't > > reset on reboots), or an environment you can write data to. > > But what if a user wants to disable writing the env variables and the > > device doesn't have a set of registers we can use? > > > Maybe it should be moved in 'struct fwu_mdata' ? I was mostly thinking on moving this count as another 'bootcount' method. So in case the user has disabled writing evn variables but he is booting with EFI he can use that. Regards /Ilias > > thnx
Hi Ilias, On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Jassi > > On Mon, 18 Jul 2022 at 18:08, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi all, > > > > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > > > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > > > > > > .... > > > > > > > > > > +} > > > > > > + > > > > > > +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? > > > > > > > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > > > > > > > + > > > > > > +#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. > > > > > > > > > Yes, this goes too. > > > > > > Is bootcount really suited for this case? > > > AFAIK bootcount either requires device specific registers (which won't > > > reset on reboots), or an environment you can write data to. > > > But what if a user wants to disable writing the env variables and the > > > device doesn't have a set of registers we can use? > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > I was mostly thinking on moving this count as another 'bootcount' > method. So in case the user has disabled writing evn variables but he > is booting with EFI he can use that. > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. Of the three options - registers, efi-env and mdata, I think the last one is more robust. For ex, if BL33 isn't reached after an update. We want BL2 (which may not have access to efi variables) to be able to revert the active index. thanks.
Hi Jassi, On Mon, 18 Jul 2022 at 18:32, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > Hi Ilias, > > On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Jassi > > > > On Mon, 18 Jul 2022 at 18:08, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi all, > > > > > > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > > > > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > > > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > > > > > > > > .... > > > > > > > > > > > > +} > > > > > > > + > > > > > > > +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? > > > > > > > > > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > > > > > > > > > + > > > > > > > +#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. > > > > > > > > > > > Yes, this goes too. > > > > > > > > Is bootcount really suited for this case? > > > > AFAIK bootcount either requires device specific registers (which won't > > > > reset on reboots), or an environment you can write data to. > > > > But what if a user wants to disable writing the env variables and the > > > > device doesn't have a set of registers we can use? > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > I was mostly thinking on moving this count as another 'bootcount' > > method. So in case the user has disabled writing evn variables but he > > is booting with EFI he can use that. > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > Of the three options - registers, efi-env and mdata, I think the last > one is more robust. > For ex, if BL33 isn't reached after an update. We want BL2 (which may > not have access to efi variables) > to be able to revert the active index. I think BL2 has it's own set of internal counters for the number of reboots already (and I think on the stmp32mp1 is based on a cpu scratch register) This is supposed with BL33 reboots only. Sughosh do I remember this wrong? Regards /Ilias > > thanks.
On Mon, 18 Jul 2022 at 10:31, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > Hi Ilias, > > On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Jassi > > > > On Mon, 18 Jul 2022 at 18:08, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi all, > > > > > > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > > > > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > > > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > > > > > > > > .... > > > > > > > > > > > > +} > > > > > > > + > > > > > > > +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? > > > > > > > > > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > > > > > > > > > + > > > > > > > +#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. > > > > > > > > > > > Yes, this goes too. > > > > > > > > Is bootcount really suited for this case? > > > > AFAIK bootcount either requires device specific registers (which won't > > > > reset on reboots), or an environment you can write data to. > > > > But what if a user wants to disable writing the env variables and the > > > > device doesn't have a set of registers we can use? > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > I was mostly thinking on moving this count as another 'bootcount' > > method. So in case the user has disabled writing evn variables but he > > is booting with EFI he can use that. > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > Of the three options - registers, efi-env and mdata, I think the last > one is more robust. > For ex, if BL33 isn't reached after an update. We want BL2 (which may > not have access to efi variables) > to be able to revert the active index. > and which requires a bootcount for each stage. hmm... probably I am overlooking something.
Hi Jassi On Mon, 18 Jul 2022 at 18:34, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > On Mon, 18 Jul 2022 at 10:31, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > Hi Ilias, > > > > On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Jassi > > > > > > On Mon, 18 Jul 2022 at 18:08, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Hi all, > > > > > > > > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > > > > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > > > > > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > > > > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > > > > > > > > > > .... > > > > > > > > > > > > > > +} > > > > > > > > + > > > > > > > > +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? > > > > > > > > > > > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > > > > > > > > > > > + > > > > > > > > +#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. > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > Is bootcount really suited for this case? > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > reset on reboots), or an environment you can write data to. > > > > > But what if a user wants to disable writing the env variables and the > > > > > device doesn't have a set of registers we can use? > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > method. So in case the user has disabled writing evn variables but he > > > is booting with EFI he can use that. > > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > > > Of the three options - registers, efi-env and mdata, I think the last > > one is more robust. > > For ex, if BL33 isn't reached after an update. We want BL2 (which may > > not have access to efi variables) > > to be able to revert the active index. > > > and which requires a bootcount for each stage. hmm... > probably I am overlooking something. Well it's indeed more complicated, but the reasoning was something along the lines of - What if BL2 crashes really early, before it can access storage? - BL2 doesn't have code to write that data only read it (in some cases, depends on how the data is stored) So the solution was to have individual counters Cheers /Ilias
On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > Hi Ilias, > > On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Jassi > > > > On Mon, 18 Jul 2022 at 18:08, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Hi all, > > > > > > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr@monstr.eu> wrote: > > > > > > On 6/9/22 14:30, Sughosh Ganu wrote: > > > > > > > From: Masami Hiramatsu <masami.hiramatsu@linaro.org> > > > > > > > > > > > > .... > > > > > > > > > > > > +} > > > > > > > + > > > > > > > +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? > > > > > > > > > > > Yup, this whole sf shebang is unnecessary, and removed for next revision. > > > > > > > > > > > > + > > > > > > > +#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. > > > > > > > > > > > Yes, this goes too. > > > > > > > > Is bootcount really suited for this case? > > > > AFAIK bootcount either requires device specific registers (which won't > > > > reset on reboots), or an environment you can write data to. > > > > But what if a user wants to disable writing the env variables and the > > > > device doesn't have a set of registers we can use? > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > I was mostly thinking on moving this count as another 'bootcount' > > method. So in case the user has disabled writing evn variables but he > > is booting with EFI he can use that. > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. No, there just has to be "somewhere" to do the counting. We've got a DDR backed driver, for example. So yes, I think we should try and use the bootcount framework here.
On Mon, Jul 18, 2022 at 4:00 PM Tom Rini <trini@konsulko.com> wrote: > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > > > > > > > > + > > > > > > > > +#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. > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > Is bootcount really suited for this case? > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > reset on reboots), or an environment you can write data to. > > > > > But what if a user wants to disable writing the env variables and the > > > > > device doesn't have a set of registers we can use? > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > method. So in case the user has disabled writing evn variables but he > > > is booting with EFI he can use that. > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > No, there just has to be "somewhere" to do the counting. We've got a > DDR backed driver, for example. So yes, I think we should try and use > the bootcount framework here. > OK, for platforms that can preserve ram across reboot, using non-persistent storage can work. My platform neither preserves ram, nor has any warmreset-proof registers. So I have to choose between saving the bootcount in efi-env or in vendor specific structure next to the metadata. I prefer metadata because it is common to all stages of boot. Any corrections to this approach? Thanks
On Mon, 18 Jul 2022 at 16:00, Tom Rini <trini@konsulko.com> wrote: > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > > > > > > > > > > > > > > + > > > > > > > > +#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. > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > Is bootcount really suited for this case? > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > reset on reboots), or an environment you can write data to. > > > > > But what if a user wants to disable writing the env variables and the > > > > > device doesn't have a set of registers we can use? > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > method. So in case the user has disabled writing evn variables but he > > > is booting with EFI he can use that. > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > No, there just has to be "somewhere" to do the counting. We've got a > DDR backed driver, for example. So yes, I think we should try and use > the bootcount framework here. > OK, for platforms that can preserve ram across reboot, using non-persistent storage can work. My platform neither preserves ram, nor has any warmreset-proof registers. So I have to choose between saving the bootcount in efi-env or in vendor specific structure next to the metadata. I prefer metadata because it is common to all stages of boot. Any corrections to this approach? Thanks
On Tue, Jul 19, 2022 at 10:23:08AM -0500, Jassi Brar wrote: > On Mon, Jul 18, 2022 at 4:00 PM Tom Rini <trini@konsulko.com> wrote: > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > > > > > > > > > > + > > > > > > > > > +#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. > > > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > > > Is bootcount really suited for this case? > > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > > reset on reboots), or an environment you can write data to. > > > > > > But what if a user wants to disable writing the env variables and the > > > > > > device doesn't have a set of registers we can use? > > > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > > method. So in case the user has disabled writing evn variables but he > > > > is booting with EFI he can use that. > > > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > > > No, there just has to be "somewhere" to do the counting. We've got a > > DDR backed driver, for example. So yes, I think we should try and use > > the bootcount framework here. > > > OK, for platforms that can preserve ram across reboot, using > non-persistent storage can work. > My platform neither preserves ram, nor has any warmreset-proof > registers. So I have to choose between saving the bootcount in efi-env > or in vendor specific structure next to the metadata. I prefer > metadata because it is common to all stages of boot. Any corrections > to this approach? What I'm trying to say is that we have an abstraction for counting the number of times the system has booted since something reset the counter to zero, to signal the system is up and functional. I'll leave the details of how it's used here, and how / what backend is used or created for it up to everyone else on the thread.
Hi Jassi, On Tue, 19 Jul 2022 at 18:27, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > On Mon, 18 Jul 2022 at 16:00, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > > > > > > > > > > > > > > > > > + > > > > > > > > > +#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. > > > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > > > Is bootcount really suited for this case? > > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > > reset on reboots), or an environment you can write data to. > > > > > > But what if a user wants to disable writing the env variables and the > > > > > > device doesn't have a set of registers we can use? > > > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > > method. So in case the user has disabled writing evn variables but he > > > > is booting with EFI he can use that. > > > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > > > No, there just has to be "somewhere" to do the counting. We've got a > > DDR backed driver, for example. So yes, I think we should try and use > > the bootcount framework here. > > > OK, for platforms that can preserve ram across reboot, using > non-persistent storage can work. > My platform neither preserves ram, nor has any warmreset-proof > registers. So I have to choose between saving the bootcount in efi-env > or in vendor specific structure next to the metadata. I prefer > metadata because it is common to all stages of boot. Any corrections > to this approach? The metadata is defined by a spec and they don't have a field for bootcounting. Once Sughosh resends his patches he'll include a bootcount backend that reuses EFI variables. Can't we just use that? > > Thanks
On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Jassi, > > On Tue, 19 Jul 2022 at 18:27, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > On Mon, 18 Jul 2022 at 16:00, Tom Rini <trini@konsulko.com> wrote: > > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > +#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. > > > > > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > > > > > Is bootcount really suited for this case? > > > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > > > reset on reboots), or an environment you can write data to. > > > > > > > But what if a user wants to disable writing the env variables and the > > > > > > > device doesn't have a set of registers we can use? > > > > > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > > > method. So in case the user has disabled writing evn variables but he > > > > > is booting with EFI he can use that. > > > > > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > > > > > No, there just has to be "somewhere" to do the counting. We've got a > > > DDR backed driver, for example. So yes, I think we should try and use > > > the bootcount framework here. > > > > > OK, for platforms that can preserve ram across reboot, using > > non-persistent storage can work. > > My platform neither preserves ram, nor has any warmreset-proof > > registers. So I have to choose between saving the bootcount in efi-env > > or in vendor specific structure next to the metadata. I prefer > > metadata because it is common to all stages of boot. Any corrections > > to this approach? > > The metadata is defined by a spec and they don't have a field for > bootcounting. Once Sughosh resends his patches he'll include a > bootcount backend that reuses EFI variables. Can't we just use that? > Yes, I am aware metadata spec has no provision of vendor data. But there is nothing illegal in appending vendor-data to metadata and that is trivial to implement ... basically use sizeof(struct fwu_mdata) + sizeof(struct sni_vendor_mdata) while read/write meta-data. That will also be zero extra-overhead. fwu-mdata { compatible = "u-boot,fwu-mdata-mtd"; fwu-mdata-store = <&spi_flash>; mdata-offsets = <0x500000 0x530000>; vendor-data-size = <0x100>; // optional }; Sure we can use an efi variable, but I see more uses of vendor-data :- shared among BL1/BL2/BL3x/OS so we can emulate reset-syndrome, crash-logging, per-image bootcount etc when the h/w doesn't support these features. Ofcourse, please feel free to implement efi-variables still. thanks.
Hi Jassi On Wed, 20 Jul 2022 at 17:30, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Jassi, > > > > On Tue, 19 Jul 2022 at 18:27, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > On Mon, 18 Jul 2022 at 16:00, Tom Rini <trini@konsulko.com> wrote: > > > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > +#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. > > > > > > > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > > > > > > > Is bootcount really suited for this case? > > > > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > > > > reset on reboots), or an environment you can write data to. > > > > > > > > But what if a user wants to disable writing the env variables and the > > > > > > > > device doesn't have a set of registers we can use? > > > > > > > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > > > > method. So in case the user has disabled writing evn variables but he > > > > > > is booting with EFI he can use that. > > > > > > > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > > > > > > > No, there just has to be "somewhere" to do the counting. We've got a > > > > DDR backed driver, for example. So yes, I think we should try and use > > > > the bootcount framework here. > > > > > > > OK, for platforms that can preserve ram across reboot, using > > > non-persistent storage can work. > > > My platform neither preserves ram, nor has any warmreset-proof > > > registers. So I have to choose between saving the bootcount in efi-env > > > or in vendor specific structure next to the metadata. I prefer > > > metadata because it is common to all stages of boot. Any corrections > > > to this approach? > > > > The metadata is defined by a spec and they don't have a field for > > bootcounting. Once Sughosh resends his patches he'll include a > > bootcount backend that reuses EFI variables. Can't we just use that? > > > Yes, I am aware metadata spec has no provision of vendor data. But > there is nothing illegal in appending vendor-data to metadata and that > is trivial to implement ... basically use sizeof(struct fwu_mdata) + > sizeof(struct sni_vendor_mdata) while read/write meta-data. That will > also be zero extra-overhead. > > fwu-mdata { > compatible = "u-boot,fwu-mdata-mtd"; > fwu-mdata-store = <&spi_flash>; > mdata-offsets = <0x500000 0x530000>; > vendor-data-size = <0x100>; // optional > }; > > Sure we can use an efi variable, but I see more uses of vendor-data > :- shared among BL1/BL2/BL3x/OS so we can emulate reset-syndrome, > crash-logging, per-image bootcount etc when the h/w doesn't support > these features. > > Ofcourse, please feel free to implement efi-variables still. Ok, in that case, you'll still have to implement this as a 'special' bootcount method since the A/B updates code will use that API to get/set the values. Thanks /Ilias > > thanks.
On Fri, Jul 22, 2022 at 3:37 AM Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Jassi > > On Wed, 20 Jul 2022 at 17:30, Jassi Brar <jassisinghbrar@gmail.com> wrote: > > > > On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Jassi, > > > > > > On Tue, 19 Jul 2022 at 18:27, Jassi Brar <jaswinder.singh@linaro.org> wrote: > > > > > > > > On Mon, 18 Jul 2022 at 16:00, Tom Rini <trini@konsulko.com> wrote: > > > > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > +#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. > > > > > > > > > > > > > > > > > > > > > Yes, this goes too. > > > > > > > > > > > > > > > > > > Is bootcount really suited for this case? > > > > > > > > > AFAIK bootcount either requires device specific registers (which won't > > > > > > > > > reset on reboots), or an environment you can write data to. > > > > > > > > > But what if a user wants to disable writing the env variables and the > > > > > > > > > device doesn't have a set of registers we can use? > > > > > > > > > > > > > > > > > Maybe it should be moved in 'struct fwu_mdata' ? > > > > > > > > > > > > > > I was mostly thinking on moving this count as another 'bootcount' > > > > > > > method. So in case the user has disabled writing evn variables but he > > > > > > > is booting with EFI he can use that. > > > > > > > > > > > > Sorry, not sure I understand.... IIUIC there has to be some persistent storage. > > > > > > > > > > No, there just has to be "somewhere" to do the counting. We've got a > > > > > DDR backed driver, for example. So yes, I think we should try and use > > > > > the bootcount framework here. > > > > > > > > > OK, for platforms that can preserve ram across reboot, using > > > > non-persistent storage can work. > > > > My platform neither preserves ram, nor has any warmreset-proof > > > > registers. So I have to choose between saving the bootcount in efi-env > > > > or in vendor specific structure next to the metadata. I prefer > > > > metadata because it is common to all stages of boot. Any corrections > > > > to this approach? > > > > > > The metadata is defined by a spec and they don't have a field for > > > bootcounting. Once Sughosh resends his patches he'll include a > > > bootcount backend that reuses EFI variables. Can't we just use that? > > > > > Yes, I am aware metadata spec has no provision of vendor data. But > > there is nothing illegal in appending vendor-data to metadata and that > > is trivial to implement ... basically use sizeof(struct fwu_mdata) + > > sizeof(struct sni_vendor_mdata) while read/write meta-data. That will > > also be zero extra-overhead. > > > > fwu-mdata { > > compatible = "u-boot,fwu-mdata-mtd"; > > fwu-mdata-store = <&spi_flash>; > > mdata-offsets = <0x500000 0x530000>; > > vendor-data-size = <0x100>; // optional > > }; > > > > Sure we can use an efi variable, but I see more uses of vendor-data > > :- shared among BL1/BL2/BL3x/OS so we can emulate reset-syndrome, > > crash-logging, per-image bootcount etc when the h/w doesn't support > > these features. > > > > Ofcourse, please feel free to implement efi-variables still. > > Ok, in that case, you'll still have to implement this as a 'special' > bootcount method since the A/B updates code will use that API to > get/set the values. > I thought the bootcount mechanism would always be platform specific? But ok. thanks.
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; +} + +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); + 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; +} *devbox_plat_metadata; + +int fwu_plat_get_alt_num(struct udevice __always_unused *dev, + efi_guid_t *image_id, int *alt_num) +{ + struct fwu_image_bank_info *bank; + struct fwu_mdata *mdata; + int i, ret; + + ret = fwu_get_mdata(&mdata); + if (ret < 0) + return ret; + + /* + * DeveloperBox FWU expects Bank:Image = 1:1, and the dfu_alt_info + * only has the entries for banks. Thus the alt_no should be equal + * to the bank index number. + */ + ret = -ENOENT; + for (i = 0; i < CONFIG_FWU_NUM_BANKS; i++) { + bank = &mdata->img_entry[0].img_bank_info[i]; + if (guidcmp(image_id, &bank->image_uuid) == 0) { + *alt_num = i; + ret = 0; + break; + } + } + + free(mdata); + + return ret; +} + +/* This assumes that user doesn't change system default dfu_alt_info */ +efi_status_t fill_image_type_guid_array(const efi_guid_t __always_unused + *default_guid, + efi_guid_t **part_guid_arr) +{ + int i; + + *part_guid_arr = malloc(sizeof(efi_guid_t) * DEFAULT_DFU_ALT_NUM); + if (!*part_guid_arr) + return EFI_OUT_OF_RESOURCES; + + for (i = 0; i < DEFAULT_DFU_ALT_NUM; i++) + guidcpy((*part_guid_arr + i), &devbox_fip_image_type_guid); + + return EFI_SUCCESS; +} + +int fwu_plat_get_update_index(u32 *update_idx) +{ + int ret; + u32 active_idx; + + ret = fwu_get_active_index(&active_idx); + + if (ret < 0) + return ret; + + *update_idx = (active_idx + 1) % CONFIG_FWU_NUM_BANKS; + + return ret; +} + +static int devbox_load_plat_metadata(void) +{ + if (devbox_plat_metadata) + return 0; + + return sf_load_data(PLAT_METADATA_OFFSET, PLAT_METADATA_SIZE, + (void **)&devbox_plat_metadata); +} + +void fwu_plat_get_bootidx(void *boot_idx) +{ + u32 *bootidx = boot_idx; + + if (devbox_load_plat_metadata() < 0) + *bootidx = 0; + else + *bootidx = devbox_plat_metadata->boot_index; +} + +int board_late_init(void) +{ + int ret; + + ret = devbox_load_plat_metadata(); + if (ret < 0) + return ret; + + if (devbox_plat_metadata->boot_count) { + /* We are in the platform trial boot. Finish it. */ + devbox_plat_metadata->boot_count = 0; + ret = sf_save_data(PLAT_METADATA_OFFSET, PLAT_METADATA_SIZE, + (void *)devbox_plat_metadata); + if (ret < 0) + return ret; + + pr_debug("FWU: Finish platform trial boot safely.\n"); + } + + return 0; +} diff --git a/include/configs/synquacer.h b/include/configs/synquacer.h index eafcc69e12..14eeb3f57e 100644 --- a/include/configs/synquacer.h +++ b/include/configs/synquacer.h @@ -46,8 +46,16 @@ /* Since U-Boot 64bit PCIe support is limited, disable 64bit MMIO support */ +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE +#define DEFAULT_DFU_ALT_NUM 2 +#define DEFAULT_DFU_ALT_INFO "dfu_alt_info=" \ + "mtd nor1=bank0 raw 600000 400000;" \ + "bank1 raw a00000 400000\0" +#else +#define DEFAULT_DFU_ALT_NUM 1 #define DEFAULT_DFU_ALT_INFO "dfu_alt_info=" \ "mtd nor1=fip.bin raw 600000 400000\0" +#endif /* GUIDs for capsule updatable firmware images */ #define DEVELOPERBOX_FIP_IMAGE_GUID \