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

[Ilias Apalodimas]

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.

Regards
/Ilias
>
> thnx