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

[Jassi Brar]

On Mon, 18 Jul 2022 at 10:31, Jassi Brar <jaswinder.singh at linaro.org> wrote:
>
> Hi Ilias,
>
> On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Jassi
> >
> > On Mon, 18 Jul 2022 at 18:08, Jassi Brar <jaswinder.singh at linaro.org> wrote:
> > >
> > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > Hi all,
> > > >
> > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar <jaswinder.singh at linaro.org> wrote:
> > > > >
> > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek <monstr at monstr.eu> wrote:
> > > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > > From: Masami Hiramatsu <masami.hiramatsu at 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.