Re: [PATCH v1 1/1] drivers: bootcount: Add ZynqMP specific bootcount support

[Vasileios Amoiridis <vassilisamir@gmail.com>]

On Wed, Oct 30, 2024 at 02:13:43PM +0100, Michal Simek wrote:
> 
> 
> On 10/29/24 19:58, Vasileios Amoiridis wrote:
> > From: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> > 
> > Add native support of the bootcount mechanism in the ZynqMP by utilising internal
> > PMU registers. The Persistent Global Storage Registers of the Platform Management
> > Unit can keep their value during reboot cycles unless there is a POR reset, making
> > them appropriate for the bootcount mechanism.
> > 
> > Signed-off-by: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> > ---
> >   drivers/bootcount/Kconfig            |  4 ++++
> >   drivers/bootcount/Makefile           |  1 +
> >   drivers/bootcount/bootcount_zynqmp.c | 28 ++++++++++++++++++++++++++++
> >   3 files changed, 33 insertions(+)
> >   create mode 100644 drivers/bootcount/bootcount_zynqmp.c
> > 
> > diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> > index fa6d8e7128..95b6a9541a 100644
> > --- a/drivers/bootcount/Kconfig
> > +++ b/drivers/bootcount/Kconfig
> > @@ -82,6 +82,10 @@ config BOOTCOUNT_AT91
> >   	bool "Boot counter for Atmel AT91SAM9XE"
> >   	depends on AT91SAM9XE
> > +config BOOTCOUNT_ZYNQMP
> > +	bool "Boot counter for Zynq UltraScale+ MPSoC"
> > +	depends on ARCH_ZYNQMP
> 
>
> help would help to also described where that count is stored.
> 

Hi Michal,

thanks for the review. Indeed I could add it.

> And why not to have it under DM/U_BOOT_DRIVER?
> You don't need to create compatible string for it just instantiate it.
> 
> Look at:
> 
> U_BOOT_DRVINFO(soc_xilinx_zynqmp) = {
>          .name = "soc_xilinx_zynqmp",
> };
> 

I was not fully aware of this, I could try it. Just out of curiosity, is
this the new/prefered way of adding new drivers?

> 
> > +
> >   config DM_BOOTCOUNT
> >           bool "Boot counter in a device-model device"
> >   	help
> > diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
> > index 245f879633..487adc1212 100644
> > --- a/drivers/bootcount/Makefile
> > +++ b/drivers/bootcount/Makefile
> > @@ -3,6 +3,7 @@
> >   obj-$(CONFIG_BOOTCOUNT_GENERIC)	+= bootcount.o
> >   obj-$(CONFIG_BOOTCOUNT_MEM)	+= bootcount.o
> >   obj-$(CONFIG_BOOTCOUNT_AT91)	+= bootcount_at91.o
> > +obj-$(CONFIG_BOOTCOUNT_ZYNQMP)  += bootcount_zynqmp.o
> 
> please put it to the end of CONFIG_BOOTCOUNT to have it at least in correct location
> 
> >   obj-$(CONFIG_BOOTCOUNT_AM33XX)	+= bootcount_davinci.o
> >   obj-$(CONFIG_BOOTCOUNT_RAM)	+= bootcount_ram.o
> >   obj-$(CONFIG_BOOTCOUNT_ENV)	+= bootcount_env.o
> > diff --git a/drivers/bootcount/bootcount_zynqmp.c b/drivers/bootcount/bootcount_zynqmp.c
> > new file mode 100644
> > index 0000000000..9bb801e188
> > --- /dev/null
> > +++ b/drivers/bootcount/bootcount_zynqmp.c
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +// SPDX-FileCopyrightText: 2024 CERN (home.cern)
> > +
> > +#include <bootcount.h>
> > +#include <stdio.h>
> > +#include <zynqmp_firmware.h>
> > +
> > +#define PMU_PERS_GLOB_GEN_STORAGE0 0x00FFD80050
> 
> In arch/arm/mach-zynqmp/include/mach/hardware.h
> there is already structure defined and gen_storage0 is there too.
> Please use it.
> 
> Regarding this location. Exception in PMUFW is using this reg.
> It means at the end of day it is up to you if you want to use it or not.
> 

In master branch (commit 5cca0e3f6e0ff17db92476235ea1bb9cd8cbc9eb) there
is no gen_storage0 in the structure pmu_regs, only gen_storage4 and
gen_storage6. 

But in any case though, I don't use gen_storage0 which is the
Global General Storage 0 but the Persistent Global General Storage 0
which is in the in the offset 0x50 and is defined in [1]. According to
this document, only registers {4:7} are used by FSBL and other Xilinx
software products so from what I understand registers {0:3} are free to
use. If that's not the case, which one of them is free for use?

[1]: https://docs.amd.com/r/en-US/ug1087-zynq-ultrascale-registers/PERS_GLOB_GEN_STORAGE0-PMU_GLOBAL-Register

> > +
> > +void bootcount_store(ulong a)
> > +{
> > +	int ret;
> > +
> > +	ret = zynqmp_mmio_write(PMU_PERS_GLOB_GEN_STORAGE0, 0xFF, a);
> > +	if (ret)
> > +		printf("Failed: mmio write\n");
> > +}
> > +
> > +ulong bootcount_load(void)
> > +{
> > +	int ret;
> > +	u32 val;
> 
> as was said val should be initialized here too.
> 

Yes indeed.

> > +
> > +	ret = zynqmp_mmio_read(PMU_PERS_GLOB_GEN_STORAGE0, &val);
> > +	if (ret)
> > +		printf("Failed: mmio read\n");
> > +	return val;
> > +}
> 
> Anyway I expect this driver to be under DM that v2 will change.
> 
> Thanks,
> Michal
> 
> 
> 

Yes, I will try and come back to you in case I don't get something.
Thanks again for the review.

Cheers,
Vasilis