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

[Michal Simek <michal.simek@amd.com>]

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.

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",
};


> +
>   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.

> +
> +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.

> +
> +	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