Re: [PATCH] clocksource: timer-sp804: Fix read_current_timer() issue when clock source is not registered

[Daniel Lezcano <daniel.lezcano@linaro.org>]

Hi Stephen,

thanks for the proposed fix

On 21/02/2025 04:46, stephen eta zhou wrote:
> Hi daniel
> 
> While debugging on the vexpress-v2p-ca9 platform, I discovered that the read_current_timer API wasn't functioning correctly. The issue was that the SP804 driver lacked ARM32 support and did not register read_current_timer. To add ARM32 compatibility, I’ve submitted this patch. Without it, using SP804 as the timer on ARM32 causes issues with boot_init_stack_canary when inserting the canary value into the interrupt stack, and also affects entropy generation and collection, resulting in incorrect rdseed values.

It is better to put that information in the changelog and provide a 
fixed format of the patch description.

>  From 9dd9b5bd7ab1638990176f7171417c83ddb7a221 Mon Sep 17 00:00:00 2001
> From: Stephen Eta Zhou <stephen.eta.zhou@outlook.com>
> Date: Fri, 21 Feb 2025 11:15:40 +0800
> Subject: [PATCH] clocksource: timer-sp804: Fix read_current_timer() issue when
>   clock source is not registered
> 
> Fix read_current_timer() on ARM32 by adding support in the SP804 driver.
> 
> Signed-off-by: Stephen Eta Zhou <stephen.eta.zhou@outlook.com>
> ---
>   drivers/clocksource/timer-sp804.c | 36 +++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c
> index cd1916c05325..b98a14d24874 100644
> --- a/drivers/clocksource/timer-sp804.c
> +++ b/drivers/clocksource/timer-sp804.c
> @@ -21,6 +21,11 @@
>   #include <linux/of_irq.h>
>   #include <linux/sched_clock.h>
>   
> +#ifdef CONFIG_ARM
> +#include <linux/delay.h>
> +#include "timer-of.h"
> +#endif
> +
>   #include "timer-sp.h"
>   
>   /* Hisilicon 64-bit timer(a variant of ARM SP804) */
> @@ -59,6 +64,10 @@ static struct sp804_timer hisi_sp804_timer __initdata = {
>   
>   static struct sp804_clkevt sp804_clkevt[NR_TIMERS];
>   
> +#ifdef CONFIG_ARM
> +	struct delay_timer delay;

static ...

> +#endif
> +
 >
>   static long __init sp804_get_clock_rate(struct clk *clk, const char *name)
>   {
>   	int err;
> @@ -102,6 +111,13 @@ static u64 notrace sp804_read(void)
>   	return ~readl_relaxed(sched_clkevt->value);
>   }
>   
> +#ifdef CONFIG_ARM
> +static unsigned long sp804_read_delay_timer_read(void)
> +{
> +	return sp804_read();
> +}
> +#endif

Group this function with the global delay variable above.

>   static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
>   							 const char *name,
>   							 struct clk *clk,
> @@ -259,6 +275,10 @@ static int __init sp804_of_init(struct device_node *np, struct sp804_timer *time
>   	struct clk *clk1, *clk2;
>   	const char *name = of_get_property(np, "compatible", NULL);
>   
> +#ifdef CONFIG_ARM
> +	struct timer_of to = { .flags = TIMER_OF_CLOCK };
> +#endif
> +
>   	if (initialized) {
>   		pr_debug("%pOF: skipping further SP804 timer device\n", np);
>   		return 0;
> @@ -318,6 +338,22 @@ static int __init sp804_of_init(struct device_node *np, struct sp804_timer *time
>   		if (ret)
>   			goto err;
>   	}
> +
> +#ifdef CONFIG_ARM
> +	ret = timer_of_init(np, &to);

The clock is already retrieved from the initialization code before

> +	if (ret) {
> +		pr_err("Failed to initialize the Timer device tree: %d\n", ret);
> +		return ret;
> +	}
> +
> +	delay.read_current_timer = sp804_read_delay_timer_read;
> +	delay.freq = timer_of_rate(&to);
> +	if (delay.freq <= 0)
> +		pr_warn("Failed to obtain the freq of the clock source: %d\n", ret);
> +
> +	register_current_timer_delay(&delay);
> +#endif>   	initialized = true;
>   
>   	return 0;


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog