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