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

[Vasileios Amoiridis]

On Wed, Oct 30, 2024 at 06:26:08AM +0100, Heiko Schocher wrote:
> Hello Vasileios,
> 
> On 29.10.24 19:58, Vasileios Amoiridis wrote:
> > From: Vasileios Amoiridis <vasileios.amoiridis at 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 at 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
> > +
> >   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
> >   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
> > +
> > +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;
> > +
> > +	ret = zynqmp_mmio_read(PMU_PERS_GLOB_GEN_STORAGE0, &val);
> > +	if (ret)
> > +		printf("Failed: mmio read\n");
> > +	return val;
> 
> Makes it sense to return val, when ret != 0 ? Maybe val is uninitialzed
> in this case... at least please return ret.

Hi Heiko,

I see your point. Initializing wouldn't hurt, and indeed I could return
the error value no problem!
> 
> 
> just Nitpick: Also may at least add the function name to the printf,
> that you have a chance to see on console output, that bootcount read
> gone wrong... may the zynqmp_mmio_write/read functions have already an
> error message on error?

Just by checking other usages of this function, indeed it would be
helpful to use function name and line number and then propagate the
error value all the way up.

Cheers,
Vasilis
> 
> Thanks!
> 
> bye,
> Heiko
> -- 
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de