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

[Jassi Brar]

On Fri, Jul 22, 2022 at 3:37 AM Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Jassi
>
> On Wed, 20 Jul 2022 at 17:30, Jassi Brar <jassisinghbrar at gmail.com> wrote:
> >
> > On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > Hi Jassi,
> > >
> > > On Tue, 19 Jul 2022 at 18:27, Jassi Brar <jaswinder.singh at linaro.org> wrote:
> > > >
> > > > On Mon, 18 Jul 2022 at 16:00, Tom Rini <trini at 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.