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

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

Posted by Stephen Eta Zhou 8 months, 1 week ago

Register a valid read_current_timer() function for the
SP804 timer on ARM32.

On ARM32 platforms, when the SP804 timer is selected as the clocksource,
the driver does not register a valid read_current_timer() function.
As a result, features that rely on this API—such as rdseed—consistently
return incorrect values.

To fix this, a delay_timer structure is registered during the SP804
driver's initialization. The read_current_timer() function is implemented
using the existing sp804_read() logic, and the timer frequency is reused
from the already-initialized clocksource.

Signed-off-by: Stephen Eta Zhou <stephen.eta.zhou@gmail.com>
---
Changes in v3:
- Updated the commit message for clarity and structure
- Link to v2: https://lore.kernel.org/all/BYAPR12MB3205D7A2BAA2712C89E03C4FD5D42@BYAPR12MB3205.namprd12.prod.outlook.com

Changes in v2:
- Added static keyword to struct delay_timer delay
- Integrate sp804_read_delay_timer_read and
  struct delay_timer delay together
- I moved the acquisition of delay.freq to
  sp804_clocksource_and_sched_clock_init.
  sp804_clocksource_and_sched_clock_init has already
  acquired and judged freq, so I can use it directly,
  and in this way I don’t need to consider whether to
  use clk1 or clk2, which can ensure that the clock source
  is available and reliable.
- Added detailed description information in Commit
- Link to v1: https://lore.kernel.org/all/BYAPR12MB3205C9C87EB560CA0CC4984BD5FB2@BYAPR12MB3205.namprd12.prod.outlook.com
---
 drivers/clocksource/timer-sp804.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c
index cd1916c0532507fb3ce7a11bfab4815906e326d5..3194c743ad1d7d3862c63a32fb1297d8db291008 100644
--- a/drivers/clocksource/timer-sp804.c
+++ b/drivers/clocksource/timer-sp804.c
@@ -21,6 +21,10 @@
 #include <linux/of_irq.h>
 #include <linux/sched_clock.h>
 
+#ifdef CONFIG_ARM
+#include <linux/delay.h>
+#endif
+
 #include "timer-sp.h"
 
 /* Hisilicon 64-bit timer(a variant of ARM SP804) */
@@ -102,6 +106,15 @@ static u64 notrace sp804_read(void)
 	return ~readl_relaxed(sched_clkevt->value);
 }
 
+#ifdef CONFIG_ARM
+static struct delay_timer delay;
+
+static unsigned long sp804_read_delay_timer_read(void)
+{
+	return sp804_read();
+}
+#endif
+
 static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
 							 const char *name,
 							 struct clk *clk,
@@ -114,6 +127,10 @@ static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
 	if (rate < 0)
 		return -EINVAL;
 
+#ifdef CONFIG_ARM
+	delay.freq = rate;
+#endif
+
 	clkevt = sp804_clkevt_get(base);
 
 	writel(0, clkevt->ctrl);
@@ -318,6 +335,12 @@ static int __init sp804_of_init(struct device_node *np, struct sp804_timer *time
 		if (ret)
 			goto err;
 	}
+
+#ifdef CONFIG_ARM
+	delay.read_current_timer = sp804_read_delay_timer_read;
+	register_current_timer_delay(&delay);
+#endif
+
 	initialized = true;
 
 	return 0;

---
base-commit: 7ee983c850b40043ac4751836fbd9a2b4d0c5937
change-id: 20250411-sp804-fix-read_current_timer-1c7c5a448c6c

Best regards,
-- 
Stephen Eta Zhou <stephen.eta.zhou@gmail.com>

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

Posted by Daniel Lezcano 7 months, 2 weeks ago

On Mon, Apr 14, 2025 at 10:55:59AM +0800, Stephen Eta Zhou wrote:
> Register a valid read_current_timer() function for the
> SP804 timer on ARM32.
> 
> On ARM32 platforms, when the SP804 timer is selected as the clocksource,
> the driver does not register a valid read_current_timer() function.
> As a result, features that rely on this API—such as rdseed—consistently
> return incorrect values.
> 
> To fix this, a delay_timer structure is registered during the SP804
> driver's initialization. The read_current_timer() function is implemented
> using the existing sp804_read() logic, and the timer frequency is reused
> from the already-initialized clocksource.
> 
> Signed-off-by: Stephen Eta Zhou <stephen.eta.zhou@gmail.com>
> ---
> Changes in v3:
> - Updated the commit message for clarity and structure
> - Link to v2: https://lore.kernel.org/all/BYAPR12MB3205D7A2BAA2712C89E03C4FD5D42@BYAPR12MB3205.namprd12.prod.outlook.com
> 
> Changes in v2:
> - Added static keyword to struct delay_timer delay
> - Integrate sp804_read_delay_timer_read and
>   struct delay_timer delay together
> - I moved the acquisition of delay.freq to
>   sp804_clocksource_and_sched_clock_init.
>   sp804_clocksource_and_sched_clock_init has already
>   acquired and judged freq, so I can use it directly,
>   and in this way I don’t need to consider whether to
>   use clk1 or clk2, which can ensure that the clock source
>   is available and reliable.
> - Added detailed description information in Commit
> - Link to v1: https://lore.kernel.org/all/BYAPR12MB3205C9C87EB560CA0CC4984BD5FB2@BYAPR12MB3205.namprd12.prod.outlook.com
> ---
>  drivers/clocksource/timer-sp804.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-sp804.c b/drivers/clocksource/timer-sp804.c
> index cd1916c0532507fb3ce7a11bfab4815906e326d5..3194c743ad1d7d3862c63a32fb1297d8db291008 100644
> --- a/drivers/clocksource/timer-sp804.c
> +++ b/drivers/clocksource/timer-sp804.c
> @@ -21,6 +21,10 @@
>  #include <linux/of_irq.h>
>  #include <linux/sched_clock.h>
>  
> +#ifdef CONFIG_ARM
> +#include <linux/delay.h>
> +#endif
> +
>  #include "timer-sp.h"
>  
>  /* Hisilicon 64-bit timer(a variant of ARM SP804) */
> @@ -102,6 +106,15 @@ static u64 notrace sp804_read(void)
>  	return ~readl_relaxed(sched_clkevt->value);
>  }
>  
> +#ifdef CONFIG_ARM
> +static struct delay_timer delay;
> +
> +static unsigned long sp804_read_delay_timer_read(void)
> +{
> +	return sp804_read();
> +}
> +#endif
> +

#ifdef CONFIG_ARM
static struct delay_timer delay;
static unsigned long sp804_read_delay_timer_read(void)
{
	return sp804_read();
}

static void sp804_register_delay_timer(int freq)
{
	delay.freq = freq;
	delay.read_current_timer = sp804_read_delay_timer_read;
	register_current_timer_delay(&delay);
}
#else
static inline void sp804_register_delay_timer(int freq) {}
#endif

>  static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
>  							 const char *name,
>  							 struct clk *clk,
> @@ -114,6 +127,10 @@ static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
>  	if (rate < 0)
>  		return -EINVAL;
>  
> +#ifdef CONFIG_ARM
> +	delay.freq = rate;
> +#endif

drop the above

        sp804_register_delay_timer(rate);

>  	clkevt = sp804_clkevt_get(base);
>  
>  	writel(0, clkevt->ctrl);
> @@ -318,6 +335,12 @@ static int __init sp804_of_init(struct device_node *np, struct sp804_timer *time
>  		if (ret)
>  			goto err;
>  	}
> +
> +#ifdef CONFIG_ARM
> +	delay.read_current_timer = sp804_read_delay_timer_read;
> +	register_current_timer_delay(&delay);
> +#endif
> +

drop the above

>  	initialized = true;
>  
>  	return 0;
> 
> ---
> base-commit: 7ee983c850b40043ac4751836fbd9a2b4d0c5937
> change-id: 20250411-sp804-fix-read_current_timer-1c7c5a448c6c
> 
> Best regards,
> -- 
> Stephen Eta Zhou <stephen.eta.zhou@gmail.com>
> 

-- 

 <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

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

Posted by stephen.eta.zhou@gmail.com 7 months ago

Hi Daniel,

On Wed, May 7, 2025 at 10:11:29AM +0200, Daniel Lezcano:
> #ifdef CONFIG_ARM
> static struct delay_timer delay;
> static unsigned long sp804_read_delay_timer_read(void)
> {
>         return sp804_read();
> }
> 
> static void sp804_register_delay_timer(int freq)
> {
>         delay.freq = freq;
>         delay.read_current_timer = sp804_read_delay_timer_read;
>         register_current_timer_delay(&delay);
> }
> #else
> static inline void sp804_register_delay_timer(int freq) {}
> #endif

Thank you for taking the time to help me improve this patch.

>>  static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
>>                                                         const char *name,
>>                                                         struct clk *clk,
>> @@ -114,6 +127,10 @@ static int __init sp804_clocksource_and_sched_clock_init(void __iomem *base,
>>        if (rate < 0)
>>                return -EINVAL;
>> 
>> +#ifdef CONFIG_ARM
>> +     delay.freq = rate;
>> +#endif
> 
> drop the above
> 
>         sp804_register_delay_timer(rate);
>>        clkevt = sp804_clkevt_get(base);
>> 
>>        writel(0, clkevt->ctrl);
>> @@ -318,6 +335,12 @@ static int __init sp804_of_init(struct device_node *np, struct sp804_timer *time
>>                if (ret)
>>                        goto err;
>>        }
>> +
>> +#ifdef CONFIG_ARM
>> +     delay.read_current_timer = sp804_read_delay_timer_read;
>> +     register_current_timer_delay(&delay);
>> +#endif
>> +
> 
> drop the above

Thanks for your feedback.

I'll make the changes accordingly and send out v4 soon.

Best regards,
Stephen