[PATCH v1 0/1] bootcount: zynqmp: Add bootcount API

[PATCH v1 0/1] bootcount: zynqmp: Add bootcount API

[Vasileios Amoiridis]

From: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>

Add support for the bootcount API by utilizing the internal Persistent
Global General Registers of the PMU. These registers get reset whenever
there is a Power-On-Reset so it makes them suitable for the API.

Questions towards reviewers:

	1) Is the licensing of the file ok?
	2) How am I supposed to add a maintainer for this file?
	3) Is this the correct place for this macro? (Maybe this one
	   goes more towards Michal.)

Cheers,
Vasilis

Vasileios Amoiridis (1):
  drivers: bootcount: Add ZynqMP specific bootcount support

 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


base-commit: 3df6145db0ed3430a2af089db5a82372bea3f4d5
-- 
2.34.1

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

[Vasileios Amoiridis]

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
+
 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;
+}
-- 
2.34.1

Re: [PATCH v1 0/1] bootcount: zynqmp: Add bootcount API

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On Tue, Oct 29, 2024 at 07:58:13PM +0100, Vasileios Amoiridis wrote:
> From: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> 
> Add support for the bootcount API by utilizing the internal Persistent
> Global General Registers of the PMU. These registers get reset whenever
> there is a Power-On-Reset so it makes them suitable for the API.
> 
> Questions towards reviewers:
> 
> 	1) Is the licensing of the file ok?

Yes.

> 	2) How am I supposed to add a maintainer for this file?

This should be caught already under the zynqmp regex in the top-level
MAINTAINERS file.

The code itself looks fine, but I'll leave it to Michal to further
review.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Heiko Schocher]

Hello Vasileios,

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


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?

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@denx.de

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

[Michal Simek]

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

Re: [PATCH v1 0/1] bootcount: zynqmp: Add bootcount API

[Vasileios Amoiridis]

On Tue, Oct 29, 2024 at 01:12:30PM -0600, Tom Rini wrote:
> On Tue, Oct 29, 2024 at 07:58:13PM +0100, Vasileios Amoiridis wrote:
> > From: Vasileios Amoiridis <vasileios.amoiridis@cern.ch>
> > 
> > Add support for the bootcount API by utilizing the internal Persistent
> > Global General Registers of the PMU. These registers get reset whenever
> > there is a Power-On-Reset so it makes them suitable for the API.
> > 
> > Questions towards reviewers:
> > 
> > 	1) Is the licensing of the file ok?
> 
> Yes.
> 
> > 	2) How am I supposed to add a maintainer for this file?
> 
> This should be caught already under the zynqmp regex in the top-level
> MAINTAINERS file.
> 
> The code itself looks fine, but I'll leave it to Michal to further
> review.
> 
> -- 
> Tom

Hi Tom,

Thank you very much for the reply.

Cheers,
Vasilis

Re: [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@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
> > +
> >   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@denx.de

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

[Vasileios Amoiridis]

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

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

[Michal Simek]

On 10/30/24 15:38, Vasileios Amoiridis wrote:
> 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.

you are right but easy to extend that structure to have it there.

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

in embedded sw you can find this
lib/sw_apps/zynqmp_pmufw/src/xpfw_start.S:77:           addik   r3,     r0, 
0xffd80050
lib/sw_apps/zynqmp_pmufw/src/xpfw_start.S:81:           addik   r3,     r0, 
0xffd80054

And there are 2 pages which describe their usage

https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18841724/PMU+Firmware#PMUFirmware-RegistersreservedforPMUFirmware

https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842019/Zynq+UltraScale+FSBL#ZynqUltraScale+FSBL-WhatarethememoryregionsandregistersreservedforFSBL?

But as I said I am fine if you choose one and you will use it for bootcount.

Thanks
M

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

[Vasileios Amoiridis]

On Wed, Oct 30, 2024 at 04:49:10PM +0100, Michal Simek wrote:
> 
> 
> On 10/30/24 15:38, Vasileios Amoiridis wrote:
> > 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.
> 
> you are right but easy to extend that structure to have it there.
> 
> > 
> > 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
> 
> in embedded sw you can find this
> lib/sw_apps/zynqmp_pmufw/src/xpfw_start.S:77:           addik   r3,     r0,
> 0xffd80050
> lib/sw_apps/zynqmp_pmufw/src/xpfw_start.S:81:           addik   r3,     r0,
> 0xffd80054
> 
> And there are 2 pages which describe their usage
> 
> https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18841724/PMU+Firmware#PMUFirmware-RegistersreservedforPMUFirmware
> 
> https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842019/Zynq+UltraScale+FSBL#ZynqUltraScale+FSBL-WhatarethememoryregionsandregistersreservedforFSBL?
> 
> But as I said I am fine if you choose one and you will use it for bootcount.
> 
> Thanks
> M
>

Ah okay I see. It was much easier to look on the UG1087 document, I
didn't think of looking somewhere else. Thanks!

In that case I would prefer to use maybe use one of the 2 registers
that are not used, like pers_gen_storage{2,3}. Would that still be
okay for you?

Cheers,
Vasilis

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

[Michal Simek]

On 10/30/24 17:04, Vasileios Amoiridis wrote:
> On Wed, Oct 30, 2024 at 04:49:10PM +0100, Michal Simek wrote:
>>
>>
>> On 10/30/24 15:38, Vasileios Amoiridis wrote:
>>> 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.
>>
>> you are right but easy to extend that structure to have it there.
>>
>>>
>>> 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
>>
>> in embedded sw you can find this
>> lib/sw_apps/zynqmp_pmufw/src/xpfw_start.S:77:           addik   r3,     r0,
>> 0xffd80050
>> lib/sw_apps/zynqmp_pmufw/src/xpfw_start.S:81:           addik   r3,     r0,
>> 0xffd80054
>>
>> And there are 2 pages which describe their usage
>>
>> https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18841724/PMU+Firmware#PMUFirmware-RegistersreservedforPMUFirmware
>>
>> https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842019/Zynq+UltraScale+FSBL#ZynqUltraScale+FSBL-WhatarethememoryregionsandregistersreservedforFSBL?
>>
>> But as I said I am fine if you choose one and you will use it for bootcount.
>>
>> Thanks
>> M
>>
> 
> Ah okay I see. It was much easier to look on the UG1087 document, I
> didn't think of looking somewhere else. Thanks!
> 
> In that case I would prefer to use maybe use one of the 2 registers
> that are not used, like pers_gen_storage{2,3}. Would that still be
> okay for you?

No problem from my side.

M