Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Fri, Jul 22, 2022 at 3:37 AM Ilias Apalodimas
wrote:
>
> Hi Jassi
>
> On Wed, 20 Jul 2022 at 17:30, Jassi Brar wrote:
> >
> > On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas
> > wrote:
> > >
> > > Hi Jassi,
> > >
> > > On Tue, 19 Jul 2022 at 18:27, Jassi Brar
> > > wrote:
> > > >
> > > > On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> > > > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> > > >
> > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > > > > > +#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 = <_flash>;
> >mdata-offsets = <0x50 0x53>;
> >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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi Jassi
On Wed, 20 Jul 2022 at 17:30, Jassi Brar wrote:
>
> On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas
> wrote:
> >
> > Hi Jassi,
> >
> > On Tue, 19 Jul 2022 at 18:27, Jassi Brar wrote:
> > >
> > > On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> > > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> > >
> > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > > > > +#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 = <_flash>;
>mdata-offsets = <0x50 0x53>;
>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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Wed, Jul 20, 2022 at 2:54 AM Ilias Apalodimas
wrote:
>
> Hi Jassi,
>
> On Tue, 19 Jul 2022 at 18:27, Jassi Brar wrote:
> >
> > On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> > > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> >
> > > > > > > >
> > > > > > > > > > +
> > > > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > > > +#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 = <_flash>;
mdata-offsets = <0x50 0x53>;
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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi Jassi,
On Tue, 19 Jul 2022 at 18:27, Jassi Brar wrote:
>
> On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
>
> > > > > > >
> > > > > > > > > +
> > > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > > +#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
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Tue, Jul 19, 2022 at 10:23:08AM -0500, Jassi Brar wrote:
> On Mon, Jul 18, 2022 at 4:00 PM Tom Rini wrote:
> > On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
>
> > > > > > > > > +
> > > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > > +#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.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, Jul 18, 2022 at 4:00 PM Tom Rini wrote:
> On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> > > > > > > > +
> > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > +#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
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 18 Jul 2022 at 16:00, Tom Rini wrote:
> On Mon, Jul 18, 2022 at 10:31:56AM -0500, Jassi Brar wrote:
> > > > > >
> > > > > > > > +
> > > > > > > > +#define PLAT_METADATA_OFFSET 0x51
> > > > > > > > +#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
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
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
> wrote:
> >
> > Hi Jassi
> >
> > On Mon, 18 Jul 2022 at 18:08, Jassi Brar wrote:
> > >
> > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > > wrote:
> > > >
> > > > Hi all,
> > > >
> > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar
> > > > wrote:
> > > > >
> > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > > From: Masami Hiramatsu
> > > > > > >
> > > > >
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +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();
> > > > > > > + 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();
> > > > > > > + 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 0x51
> > > > > > > +#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.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi Jassi
On Mon, 18 Jul 2022 at 18:34, Jassi Brar wrote:
>
> On Mon, 18 Jul 2022 at 10:31, Jassi Brar wrote:
> >
> > Hi Ilias,
> >
> > On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas
> > wrote:
> > >
> > > Hi Jassi
> > >
> > > On Mon, 18 Jul 2022 at 18:08, Jassi Brar
> > > wrote:
> > > >
> > > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > > > wrote:
> > > > >
> > > > > Hi all,
> > > > >
> > > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar
> > > > > wrote:
> > > > > >
> > > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > > > From: Masami Hiramatsu
> > > > > > > >
> > > > > >
> > > > > >
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +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();
> > > > > > > > + 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();
> > > > > > > > + 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 0x51
> > > > > > > > +#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
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi Jassi,
On Mon, 18 Jul 2022 at 18:32, Jassi Brar wrote:
>
> Hi Ilias,
>
> On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas
> wrote:
> >
> > Hi Jassi
> >
> > On Mon, 18 Jul 2022 at 18:08, Jassi Brar wrote:
> > >
> > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > > wrote:
> > > >
> > > > Hi all,
> > > >
> > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar
> > > > wrote:
> > > > >
> > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > > From: Masami Hiramatsu
> > > > > > >
> > > > >
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +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();
> > > > > > > + 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();
> > > > > > > + 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 0x51
> > > > > > > +#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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 18 Jul 2022 at 10:31, Jassi Brar wrote:
>
> Hi Ilias,
>
> On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas
> wrote:
> >
> > Hi Jassi
> >
> > On Mon, 18 Jul 2022 at 18:08, Jassi Brar wrote:
> > >
> > > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > > wrote:
> > > >
> > > > Hi all,
> > > >
> > > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar
> > > > wrote:
> > > > >
> > > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > > From: Masami Hiramatsu
> > > > > > >
> > > > >
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +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();
> > > > > > > + 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();
> > > > > > > + 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 0x51
> > > > > > > +#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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi Ilias,
On Mon, 18 Jul 2022 at 10:16, Ilias Apalodimas
wrote:
>
> Hi Jassi
>
> On Mon, 18 Jul 2022 at 18:08, Jassi Brar wrote:
> >
> > On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> > wrote:
> > >
> > > Hi all,
> > >
> > > On Mon, 18 Jul 2022 at 17:43, Jassi Brar
> > > wrote:
> > > >
> > > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > > From: Masami Hiramatsu
> > > > > >
> > > >
> > > >
> > > > > > +}
> > > > > > +
> > > > > > +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();
> > > > > > + 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();
> > > > > > + 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 0x51
> > > > > > +#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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi Jassi
On Mon, 18 Jul 2022 at 18:08, Jassi Brar wrote:
>
> On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
> wrote:
> >
> > Hi all,
> >
> > On Mon, 18 Jul 2022 at 17:43, Jassi Brar wrote:
> > >
> > > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > > From: Masami Hiramatsu
> > > > >
> > >
> > >
> > > > > +}
> > > > > +
> > > > > +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();
> > > > > + 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();
> > > > > + 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 0x51
> > > > > +#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
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 18 Jul 2022 at 09:47, Ilias Apalodimas
wrote:
>
> Hi all,
>
> On Mon, 18 Jul 2022 at 17:43, Jassi Brar wrote:
> >
> > On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > > From: Masami Hiramatsu
> > > >
> >
> >
> > > > +}
> > > > +
> > > > +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();
> > > > + 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();
> > > > + 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 0x51
> > > > +#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
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
Hi all,
On Mon, 18 Jul 2022 at 17:43, Jassi Brar wrote:
>
> On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> > On 6/9/22 14:30, Sughosh Ganu wrote:
> > > From: Masami Hiramatsu
> > >
>
>
> > > +}
> > > +
> > > +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();
> > > + 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();
> > > + 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 0x51
> > > +#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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On Mon, 20 Jun 2022 at 03:23, Michal Simek wrote:
> On 6/9/22 14:30, Sughosh Ganu wrote:
> > From: Masami Hiramatsu
> >
> > +}
> > +
> > +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();
> > + 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();
> > + 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 0x51
> > +#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.
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On 6/9/22 14:30, Sughosh Ganu wrote:
From: Masami Hiramatsu
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
0x60 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
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
---
.../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 = <0x58 0x8>;
};
-
+ /* FWU Multi bank update partitions */
partition@60 {
- label = "FIP";
+ label = "FIP-Bank0";
reg = <0x60 0x40>;
};
+ partition@a0 {
+ label = "FIP-Bank1";
+ reg = <0xa0 0x40>;
+ };
};
};
};
@@ -114,6 +118,11 @@
optee {
status = "okay";
};
+ fwu-mdata {
+ compatible = "u-boot,fwu-mdata-mtd";
+ fwu-mdata-store = <_flash>;
+ mdata-offsets = <0x50 0x53>;
+ };
};
};
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 00..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+/* 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
Re: [PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
On 6/9/22 14:30, Sughosh Ganu wrote:
From: Masami Hiramatsu
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
0x60 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
---
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
---
.../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 = <0x58 0x8>;
};
-
+ /* FWU Multi bank update partitions */
partition@60 {
- label = "FIP";
+ label = "FIP-Bank0";
reg = <0x60 0x40>;
};
+ partition@a0 {
+ label = "FIP-Bank1";
+ reg = <0xa0 0x40>;
+ };
};
};
};
@@ -114,6 +118,11 @@
optee {
status = "okay";
};
+ fwu-mdata {
+ compatible = "u-boot,fwu-mdata-mtd";
+ fwu-mdata-store = <_flash>;
+ mdata-offsets = <0x50 0x53>;
+ };
};
};
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 00..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+/* 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();
+ if (ret < 0)
+ return ret;
+
+ *data =
[PATCH v5 19/23] FWU: synquacer: Add FWU Multi bank update support for DeveloperBox
From: Masami Hiramatsu
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
0x60 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
---
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
---
.../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 = <0x58 0x8>;
};
-
+ /* FWU Multi bank update partitions */
partition@60 {
- label = "FIP";
+ label = "FIP-Bank0";
reg = <0x60 0x40>;
};
+ partition@a0 {
+ label = "FIP-Bank1";
+ reg = <0xa0 0x40>;
+ };
};
};
};
@@ -114,6 +118,11 @@
optee {
status = "okay";
};
+ fwu-mdata {
+ compatible = "u-boot,fwu-mdata-mtd";
+ fwu-mdata-store = <_flash>;
+ mdata-offsets = <0x50 0x53>;
+ };
};
};
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 00..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+/* 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();
+ if (ret < 0)
+ return ret;
+
+ *data = memalign(ARCH_DMA_MINALIGN, size);
+ if (!*data)
+
