Re: [PATCH v3 4/8] drivers: arm_architected_timer: refactor for pbl compatibility

[Stefan Kerkmann <s.kerkmann@pengutronix.de>]

Hi Sascha,

On 8/20/26 09:04, Sascha Hauer wrote:
> On 2026-08-17 16:01, Stefan Kerkmann wrote:
>> All ARMv8-A cores implement the 64bit wide generic timer CNTPCT[1]. Thus
>> the driver is the candidate for all AARCH64 SoCs to implement
>> PBL_CLOCKSOURCE compatibilty.
>>
>> There is one catch though: the CNTFRQ_EL0 register[2] must be programmed
>> by the bootrom, barebox or the tf-a before the `arm_arch_timer_init`
>> call or the clocksource setup will return with -ENODEV. A later
>> re-initialization is also possible by calling `arm_arch_timer_init`
>> again.
>>
>> [1]: See "ARM Architecture Reference Manual for A-profile architecture
>> (rev L.a)", chapter D12 "The Generic Timer in AArch64 state"
>> [2]: See "ARM Architecture Reference Manual for A-profile architecture
>> (rev L.a)", chapter D12.1.2.1 "Initializing and reading the system
>> effective frequency"
>>
>> Signed-off-by: Stefan Kerkmann <s.kerkmann@pengutronix.de>
>> ---
>>  .../include/asm/hardware/arm_architected_timer.h   | 21 +++++++++++++++++++
>>  drivers/clocksource/Makefile                       |  2 +-
>>  drivers/clocksource/arm_architected_timer.c        | 24 ++++++++++++++--------
>>  3 files changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/hardware/arm_architected_timer.h b/arch/arm/include/asm/hardware/arm_architected_timer.h
>> new file mode 100644
>> index 0000000000..e0c1fd84fd
>> --- /dev/null
>> +++ b/arch/arm/include/asm/hardware/arm_architected_timer.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef __ASM_ARM_HARDWARE_ARCH_TIMER_H
>> +#define __ASM_ARM_HARDWARE_ARCH_TIMER_H
>> +
>> +#include <linux/types.h>
>> +
>> +/**
>> + * arm_arch_timer_init() - Initialize the ARM architected timer as global
>> + * clocksource
>> + * @cntfrq: The timer frequency, if zero the frequency is read from the
>> + *          CNTFRQ_EL0 register
>> + *
>> + * This function is meant to be called in a PBL constructor or in the driver
>> + * probe function.
>> + *
>> + * Return: 0 on success, -ENODEV if the timer frequency can not be determined
>> + */
>> +int arm_arch_timer_init(uint64_t cntfrq);
>> +
>> +#endif
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index dff8255650..0c92477247 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -13,7 +13,7 @@ obj-$(CONFIG_CLOCKSOURCE_ORION)   += orion.o
>>  obj-$(CONFIG_CLOCKSOURCE_UEMD)    += uemd.o
>>  obj-$(CONFIG_CLOCKSOURCE_ROCKCHIP)+= rk_timer.o
>>  obj-$(CONFIG_CLOCKSOURCE_ATMEL_PIT) += timer-atmel-pit.o
>> -obj-$(CONFIG_CLOCKSOURCE_ARM_ARCHITECTED_TIMER) += arm_architected_timer.o
>> +obj-pbl-$(CONFIG_CLOCKSOURCE_ARM_ARCHITECTED_TIMER) += arm_architected_timer.o
>>  ifneq ($(CONFIG_CPU_V8),y)
>>  CFLAGS_arm_architected_timer.o := -march=armv7-a
>>  endif
>> diff --git a/drivers/clocksource/arm_architected_timer.c b/drivers/clocksource/arm_architected_timer.c
>> index daced94c0e..ea5f5b8e05 100644
>> --- a/drivers/clocksource/arm_architected_timer.c
>> +++ b/drivers/clocksource/arm_architected_timer.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/clk.h>
>>  #include <io.h>
>>  #include <asm/system.h>
>> +#include <asm/hardware/arm_architected_timer.h>
>>  
>>  static uint64_t arm_arch_clocksource_read(void)
>>  {
>> @@ -22,22 +23,29 @@ static struct clocksource cs = {
>>  	.priority = 70,
>>  };
>>  
>> -static int arm_arch_timer_probe(struct device *dev)
>> +int arm_arch_timer_init(uint64_t cntfrq)
>>  {
>> -	u32 cntfrq;
>> -	int ret;
>> -
>> -	/* Some platforms don't set CNTFRQ_EL0 before barebox */
>> -	ret = of_property_read_u32(dev->of_node, "clock-frequency", &cntfrq);
>> -
>> -	if (ret)
>> +	if (!cntfrq)
>>  		cntfrq = get_cntfrq();
> 
> I wonder if we should just drop the cntfrq argument. All users call
> set_cntfrq() before calling this, so get_cntfrq() should return the
> correct value and we can rely on it.
> 

Maybe keep the argument and use set_cntfrq in arm_arch_timer_init if the
argument is !=0? Then arm_arch_timer_init is the function to use if a C
environment is available? This would prevent any inconsistency.

> Sascha
> 

Best regards,
Stefan

> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |