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.amoiri...@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.amoiri...@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
