[PATCH] clocksource/drivers/arm_arch_timer: Use event stream scaling when available

[PATCH] clocksource/drivers/arm_arch_timer: Use event stream scaling when available

[Marc Zyngier]

With FEAT_ECV and the 1GHz counter, it is pretty likely that the
event stream divider doesn't fit in the field that holds the
divider value (we only have 4 bits to describe counter bits [15:0]

Thankfully, FEAT_ECV also provides a scaling mechanism to switch
the field to cover counter bits [23:8] instead.

Enable this on arm64 when ECV is available (32bit doesn't have
any detection infrastructure and is unlikely to be run on an
ARMv8.6 system anyway).

Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c | 13 +++++++++++--
 include/clocksource/arm_arch_timer.h |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 1ecd52f903b8..9ab8221ee3c6 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -880,10 +880,19 @@ static void __arch_timer_setup(unsigned type,
 	clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
 }
 
-static void arch_timer_evtstrm_enable(int divider)
+static void arch_timer_evtstrm_enable(unsigned int divider)
 {
 	u32 cntkctl = arch_timer_get_cntkctl();
 
+#ifdef CONFIG_ARM64
+	/* ECV is likely to require a large divider. Use the EVNTIS flag. */
+	if (cpus_have_const_cap(ARM64_HAS_ECV) && divider > 15) {
+		cntkctl |= ARCH_TIMER_EVT_INTERVAL_SCALE;
+		divider -= 8;
+	}
+#endif
+
+	divider = min(divider, 15U);
 	cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
 	/* Set the divider and enable virtual event stream */
 	cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
@@ -912,7 +921,7 @@ static void arch_timer_configure_evtstream(void)
 		lsb++;
 
 	/* enable event stream */
-	arch_timer_evtstrm_enable(max(0, min(lsb, 15)));
+	arch_timer_evtstrm_enable(max(0, lsb));
 }
 
 static void arch_counter_set_user_access(void)
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index e715bdb720d5..057c8964aefb 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -56,6 +56,7 @@ enum arch_timer_spi_nr {
 #define ARCH_TIMER_EVT_TRIGGER_MASK	(0xF << ARCH_TIMER_EVT_TRIGGER_SHIFT)
 #define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
 #define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
+#define ARCH_TIMER_EVT_INTERVAL_SCALE	(1 << 17) /* EVNTIS in the ARMv8 ARM */
 
 #define ARCH_TIMER_EVT_STREAM_PERIOD_US	100
 #define ARCH_TIMER_EVT_STREAM_FREQ				\
-- 
2.34.1

Re: [PATCH] clocksource/drivers/arm_arch_timer: Use event stream scaling when available

[Mark Rutland]

On Thu, Feb 03, 2022 at 05:05:02PM +0000, Marc Zyngier wrote:
> With FEAT_ECV and the 1GHz counter, it is pretty likely that the
> event stream divider doesn't fit in the field that holds the
> divider value (we only have 4 bits to describe counter bits [15:0]

I was worried we'd be orders of magnitude out, but it looks like we're on the
borderline here. IIUC if we have a 1GHz counter and *don't* have ECV, the best
we can do is count bit 15 transitioning (one-way), and the minimal event
frequency we can get is:

  1_000_000_000 / 2 / (1 << 15))

Which is ~15KHz.

With ECV we can make that, using bit 16 instead we get ~7.5KHz.

Is that right? Could we note this in the commit message?

Given that, this seems reasonable, but not crititcally important for
backporting or whatever.

Longer term, for WFET we'll need to consider disabling the event stream (either
by default or opt-in).

> Thankfully, FEAT_ECV also provides a scaling mechanism to switch
> the field to cover counter bits [23:8] instead.
> 
> Enable this on arm64 when ECV is available (32bit doesn't have
> any detection infrastructure and is unlikely to be run on an
> ARMv8.6 system anyway).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>

The logic looks right to me, so FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/clocksource/arm_arch_timer.c | 13 +++++++++++--
>  include/clocksource/arm_arch_timer.h |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 1ecd52f903b8..9ab8221ee3c6 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -880,10 +880,19 @@ static void __arch_timer_setup(unsigned type,
>  	clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
>  }
>  
> -static void arch_timer_evtstrm_enable(int divider)
> +static void arch_timer_evtstrm_enable(unsigned int divider)
>  {
>  	u32 cntkctl = arch_timer_get_cntkctl();
>  
> +#ifdef CONFIG_ARM64
> +	/* ECV is likely to require a large divider. Use the EVNTIS flag. */
> +	if (cpus_have_const_cap(ARM64_HAS_ECV) && divider > 15) {
> +		cntkctl |= ARCH_TIMER_EVT_INTERVAL_SCALE;
> +		divider -= 8;
> +	}
> +#endif
> +
> +	divider = min(divider, 15U);
>  	cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
>  	/* Set the divider and enable virtual event stream */
>  	cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
> @@ -912,7 +921,7 @@ static void arch_timer_configure_evtstream(void)
>  		lsb++;
>  
>  	/* enable event stream */
> -	arch_timer_evtstrm_enable(max(0, min(lsb, 15)));
> +	arch_timer_evtstrm_enable(max(0, lsb));
>  }
>  
>  static void arch_counter_set_user_access(void)
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index e715bdb720d5..057c8964aefb 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -56,6 +56,7 @@ enum arch_timer_spi_nr {
>  #define ARCH_TIMER_EVT_TRIGGER_MASK	(0xF << ARCH_TIMER_EVT_TRIGGER_SHIFT)
>  #define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
>  #define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
> +#define ARCH_TIMER_EVT_INTERVAL_SCALE	(1 << 17) /* EVNTIS in the ARMv8 ARM */
>  
>  #define ARCH_TIMER_EVT_STREAM_PERIOD_US	100
>  #define ARCH_TIMER_EVT_STREAM_FREQ				\
> -- 
> 2.34.1
> 

Re: [PATCH] clocksource/drivers/arm_arch_timer: Use event stream scaling when available

[Daniel Lezcano]

On 03/02/2022 18:05, Marc Zyngier wrote:
> With FEAT_ECV and the 1GHz counter, it is pretty likely that the
> event stream divider doesn't fit in the field that holds the
> divider value (we only have 4 bits to describe counter bits [15:0]
> 
> Thankfully, FEAT_ECV also provides a scaling mechanism to switch
> the field to cover counter bits [23:8] instead.
> 
> Enable this on arm64 when ECV is available (32bit doesn't have
> any detection infrastructure and is unlikely to be run on an
> ARMv8.6 system anyway).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---

Applied, thanks


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

[tip: timers/core] clocksource/drivers/arm_arch_timer: Use event stream scaling when available

[tip-bot2 for Marc Zyngier]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     8c4b810a87005eb46564a48a69b5b255e515fa62
Gitweb:        https://git.kernel.org/tip/8c4b810a87005eb46564a48a69b5b255e515fa62
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Thu, 03 Feb 2022 17:05:02 
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Mon, 07 Mar 2022 18:27:22 +01:00

clocksource/drivers/arm_arch_timer: Use event stream scaling when available

With FEAT_ECV and the 1GHz counter, it is pretty likely that the
event stream divider doesn't fit in the field that holds the
divider value (we only have 4 bits to describe counter bits [15:0]

Thankfully, FEAT_ECV also provides a scaling mechanism to switch
the field to cover counter bits [23:8] instead.

Enable this on arm64 when ECV is available (32bit doesn't have
any detection infrastructure and is unlikely to be run on an
ARMv8.6 system anyway).

Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Link: https://lore.kernel.org/r/20220203170502.2694422-1-maz@kernel.org
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c | 13 +++++++++++--
 include/clocksource/arm_arch_timer.h |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 1ecd52f..9ab8221 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -880,10 +880,19 @@ static void __arch_timer_setup(unsigned type,
 	clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
 }
 
-static void arch_timer_evtstrm_enable(int divider)
+static void arch_timer_evtstrm_enable(unsigned int divider)
 {
 	u32 cntkctl = arch_timer_get_cntkctl();
 
+#ifdef CONFIG_ARM64
+	/* ECV is likely to require a large divider. Use the EVNTIS flag. */
+	if (cpus_have_const_cap(ARM64_HAS_ECV) && divider > 15) {
+		cntkctl |= ARCH_TIMER_EVT_INTERVAL_SCALE;
+		divider -= 8;
+	}
+#endif
+
+	divider = min(divider, 15U);
 	cntkctl &= ~ARCH_TIMER_EVT_TRIGGER_MASK;
 	/* Set the divider and enable virtual event stream */
 	cntkctl |= (divider << ARCH_TIMER_EVT_TRIGGER_SHIFT)
@@ -912,7 +921,7 @@ static void arch_timer_configure_evtstream(void)
 		lsb++;
 
 	/* enable event stream */
-	arch_timer_evtstrm_enable(max(0, min(lsb, 15)));
+	arch_timer_evtstrm_enable(max(0, lsb));
 }
 
 static void arch_counter_set_user_access(void)
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index e715bdb..057c896 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -56,6 +56,7 @@ enum arch_timer_spi_nr {
 #define ARCH_TIMER_EVT_TRIGGER_MASK	(0xF << ARCH_TIMER_EVT_TRIGGER_SHIFT)
 #define ARCH_TIMER_USR_VT_ACCESS_EN	(1 << 8) /* virtual timer registers */
 #define ARCH_TIMER_USR_PT_ACCESS_EN	(1 << 9) /* physical timer registers */
+#define ARCH_TIMER_EVT_INTERVAL_SCALE	(1 << 17) /* EVNTIS in the ARMv8 ARM */
 
 #define ARCH_TIMER_EVT_STREAM_PERIOD_US	100
 #define ARCH_TIMER_EVT_STREAM_FREQ				\