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