[PATCH v1 0/1] bootcount: zynqmp: Add bootcount API
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
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; +}
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
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
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
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_...
+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
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?
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#...
https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842019/Zynq+UltraSca...
But as I said I am fine if you choose one and you will use it for bootcount.
Thanks M
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?
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#...
https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842019/Zynq+UltraSca...
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
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.cdiff --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.odiff --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?
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#...
https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842019/Zynq+UltraSca...
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
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:
- Is the licensing of the file ok?
Yes.
- 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.
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:
- Is the licensing of the file ok?
Yes.
- 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
participants (4)
-
Heiko Schocher -
Michal Simek -
Tom Rini -
Vasileios Amoiridis