[PATCH v2 0/2] clocksource: arm_arch_timer: Some fixes

[PATCH v2 0/2] clocksource: arm_arch_timer: Some fixes

[Keqian Zhu]

change log:

v2:
 - Do not revert commit 0ea415390cd3, fix it instead.
 - Correct the tags of second patch.

Keqian Zhu (2):
  clocksource: arm_arch_timer: Use stable count reader in erratum sne
  clocksource: arm_arch_timer: Correct fault programming of
    CNTKCTL_EL1.EVNTI

 drivers/clocksource/arm_arch_timer.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/2] clocksource: arm_arch_timer: Use stable count reader in erratum sne

[Keqian Zhu]

In commit 0ea415390cd3 ("clocksource/arm_arch_timer: Use arch_timer_read_counter
to access stable counters"), we separate stable and normal count reader to omit
unnecessary overhead on systems that have no timer erratum.

However, in erratum_set_next_event_tval_generic(), count reader becomes normal
reader. This converts it to stable reader.

Fixes: 0ea415390cd3 ("clocksource/arm_arch_timer: Use
       arch_timer_read_counter to access stable counters")
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
 drivers/clocksource/arm_arch_timer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 6c3e841..777d38c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -396,10 +396,10 @@ static void erratum_set_next_event_tval_generic(const int access, unsigned long
 	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
 
 	if (access == ARCH_TIMER_PHYS_ACCESS) {
-		cval = evt + arch_counter_get_cntpct();
+		cval = evt + arch_counter_get_cntpct_stable();
 		write_sysreg(cval, cntp_cval_el0);
 	} else {
-		cval = evt + arch_counter_get_cntvct();
+		cval = evt + arch_counter_get_cntvct_stable();
 		write_sysreg(cval, cntv_cval_el0);
 	}
 
-- 
1.8.3.1

[PATCH v2 2/2] clocksource: arm_arch_timer: Correct fault programming of CNTKCTL_EL1.EVNTI

[Keqian Zhu]

ARM virtual counter supports event stream, it can only trigger an event
when the trigger bit (the value of CNTKCTL_EL1.EVNTI) of CNTVCT_EL0 changes,
so the actual period of event stream is 2^(cntkctl_evnti + 1). For example,
when the trigger bit is 0, then virtual counter trigger an event for every
two cycles.

Fixes: 037f637767a8 ("drivers: clocksource: add support for
       ARM architected timer event stream")
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
 drivers/clocksource/arm_arch_timer.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 777d38c..e3b2ee0 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -824,10 +824,14 @@ static void arch_timer_configure_evtstream(void)
 {
 	int evt_stream_div, pos;
 
-	/* Find the closest power of two to the divisor */
-	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
+	/*
+	 * Find the closest power of two to the divisor. As the event
+	 * stream can at most be generated at half the frequency of the
+	 * counter, use half the frequency when computing the divider.
+	 */
+	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2;
 	pos = fls(evt_stream_div);
-	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
+	if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))))
 		pos--;
 	/* enable event stream */
 	arch_timer_evtstrm_enable(min(pos, 15));
-- 
1.8.3.1

Re: [PATCH v2 0/2] clocksource: arm_arch_timer: Some fixes

[zhukeqian]

Hi Marc,

Found that this bugfix series is not applied for now.
Does it need some modification? Wish you can pick it up :-)

Thanks,
Keqian

On 2020/8/18 11:28, Keqian Zhu wrote:
> change log:
> 
> v2:
>  - Do not revert commit 0ea415390cd3, fix it instead.
>  - Correct the tags of second patch.
> 
> Keqian Zhu (2):
>   clocksource: arm_arch_timer: Use stable count reader in erratum sne
>   clocksource: arm_arch_timer: Correct fault programming of
>     CNTKCTL_EL1.EVNTI
> 
>  drivers/clocksource/arm_arch_timer.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 

Re: [PATCH v2 2/2] clocksource: arm_arch_timer: Correct fault programming of CNTKCTL_EL1.EVNTI

[Marc Zyngier]

On 2020-08-18 04:28, Keqian Zhu wrote:
> ARM virtual counter supports event stream, it can only trigger an event
> when the trigger bit (the value of CNTKCTL_EL1.EVNTI) of CNTVCT_EL0 
> changes,
> so the actual period of event stream is 2^(cntkctl_evnti + 1). For 
> example,
> when the trigger bit is 0, then virtual counter trigger an event for 
> every
> two cycles.
> 
> Fixes: 037f637767a8 ("drivers: clocksource: add support for
>        ARM architected timer event stream")

Fixes: tags should on a single line.

> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c
> b/drivers/clocksource/arm_arch_timer.c
> index 777d38c..e3b2ee0 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -824,10 +824,14 @@ static void arch_timer_configure_evtstream(void)
>  {
>  	int evt_stream_div, pos;
> 
> -	/* Find the closest power of two to the divisor */
> -	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
> +	/*
> +	 * Find the closest power of two to the divisor. As the event
> +	 * stream can at most be generated at half the frequency of the
> +	 * counter, use half the frequency when computing the divider.
> +	 */
> +	evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2;
>  	pos = fls(evt_stream_div);
> -	if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
> +	if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))))
>  		pos--;

You don't explain why you are special-casing pos == 1.

>  	/* enable event stream */
>  	arch_timer_evtstrm_enable(min(pos, 15));

Also, please Cc the subsystem maintainers:

CLOCKSOURCE, CLOCKEVENT DRIVERS
M:      Daniel Lezcano <daniel.lezcano@linaro.org>
M:      Thomas Gleixner <tglx@linutronix.de>
L:      linux-kernel@vger.kernel.org
S:      Supported
T:      git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 
timers/core
F:      Documentation/devicetree/bindings/timer/
F:      drivers/clocksource/

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 1/2] clocksource: arm_arch_timer: Use stable count reader in erratum sne

[Marc Zyngier]

On 2020-08-18 04:28, Keqian Zhu wrote:
> In commit 0ea415390cd3 ("clocksource/arm_arch_timer: Use 
> arch_timer_read_counter
> to access stable counters"), we separate stable and normal count reader 
> to omit
> unnecessary overhead on systems that have no timer erratum.
> 
> However, in erratum_set_next_event_tval_generic(), count reader becomes 
> normal
> reader. This converts it to stable reader.
> 
> Fixes: 0ea415390cd3 ("clocksource/arm_arch_timer: Use
>        arch_timer_read_counter to access stable counters")

On a single line.

> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c
> b/drivers/clocksource/arm_arch_timer.c
> index 6c3e841..777d38c 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -396,10 +396,10 @@ static void
> erratum_set_next_event_tval_generic(const int access, unsigned long
>  	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
> 
>  	if (access == ARCH_TIMER_PHYS_ACCESS) {
> -		cval = evt + arch_counter_get_cntpct();
> +		cval = evt + arch_counter_get_cntpct_stable();
>  		write_sysreg(cval, cntp_cval_el0);
>  	} else {
> -		cval = evt + arch_counter_get_cntvct();
> +		cval = evt + arch_counter_get_cntvct_stable();
>  		write_sysreg(cval, cntv_cval_el0);
>  	}

With that fixed:

Acked-by: Marc Zyngier <maz@kernel.org>

This should go via the clocksource tree.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2 2/2] clocksource: arm_arch_timer: Correct fault programming of CNTKCTL_EL1.EVNTI

[zhukeqian]

Hi Marc,

On 2020/12/3 22:57, Marc Zyngier wrote:
> On 2020-08-18 04:28, Keqian Zhu wrote:
>> ARM virtual counter supports event stream, it can only trigger an event
>> when the trigger bit (the value of CNTKCTL_EL1.EVNTI) of CNTVCT_EL0 changes,
>> so the actual period of event stream is 2^(cntkctl_evnti + 1). For example,
>> when the trigger bit is 0, then virtual counter trigger an event for every
>> two cycles.
>>
>> Fixes: 037f637767a8 ("drivers: clocksource: add support for
>>        ARM architected timer event stream")
> 
> Fixes: tags should on a single line.
Will do.

> 
>> Suggested-by: Marc Zyngier <maz@kernel.org>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c
>> b/drivers/clocksource/arm_arch_timer.c
>> index 777d38c..e3b2ee0 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -824,10 +824,14 @@ static void arch_timer_configure_evtstream(void)
>>  {
>>      int evt_stream_div, pos;
>>
>> -    /* Find the closest power of two to the divisor */
>> -    evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ;
>> +    /*
>> +     * Find the closest power of two to the divisor. As the event
>> +     * stream can at most be generated at half the frequency of the
>> +     * counter, use half the frequency when computing the divider.
>> +     */
>> +    evt_stream_div = arch_timer_rate / ARCH_TIMER_EVT_STREAM_FREQ / 2;
>>      pos = fls(evt_stream_div);
>> -    if (pos > 1 && !(evt_stream_div & (1 << (pos - 2))))
>> +    if ((pos == 1) || (pos > 1 && !(evt_stream_div & (1 << (pos - 2)))))
>>          pos--;
> 
> You don't explain why you are special-casing pos == 1.
The logic is not clear here, I will try to reform it.

> 
>>      /* enable event stream */
>>      arch_timer_evtstrm_enable(min(pos, 15));
> 
> Also, please Cc the subsystem maintainers:
> 
> CLOCKSOURCE, CLOCKEVENT DRIVERS
> M:      Daniel Lezcano <daniel.lezcano@linaro.org>
> M:      Thomas Gleixner <tglx@linutronix.de>
> L:      linux-kernel@vger.kernel.org
> S:      Supported
> T:      git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git timers/core
> F:      Documentation/devicetree/bindings/timer/
> F:      drivers/clocksource/
> 
Will do.

> Thanks,
> 
>         M.

Thanks,
Keqian

Re: [PATCH v2 1/2] clocksource: arm_arch_timer: Use stable count reader in erratum sne

[zhukeqian]

Hi Marc,

On 2020/12/3 22:58, Marc Zyngier wrote:
> On 2020-08-18 04:28, Keqian Zhu wrote:
>> In commit 0ea415390cd3 ("clocksource/arm_arch_timer: Use arch_timer_read_counter
>> to access stable counters"), we separate stable and normal count reader to omit
>> unnecessary overhead on systems that have no timer erratum.
>>
>> However, in erratum_set_next_event_tval_generic(), count reader becomes normal
>> reader. This converts it to stable reader.
>>
>> Fixes: 0ea415390cd3 ("clocksource/arm_arch_timer: Use
>>        arch_timer_read_counter to access stable counters")
> 
> On a single line.
Addressed. Thanks.

> 
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c
>> b/drivers/clocksource/arm_arch_timer.c
>> index 6c3e841..777d38c 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -396,10 +396,10 @@ static void
>> erratum_set_next_event_tval_generic(const int access, unsigned long
>>      ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
>>
>>      if (access == ARCH_TIMER_PHYS_ACCESS) {
>> -        cval = evt + arch_counter_get_cntpct();
>> +        cval = evt + arch_counter_get_cntpct_stable();
>>          write_sysreg(cval, cntp_cval_el0);
>>      } else {
>> -        cval = evt + arch_counter_get_cntvct();
>> +        cval = evt + arch_counter_get_cntvct_stable();
>>          write_sysreg(cval, cntv_cval_el0);
>>      }
> 
> With that fixed:
> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> 
> This should go via the clocksource tree.
Added Cc to it's maintainers, thanks.

> 
> Thanks,
> 
>         M.
Cheers,
Keqian