2020-08-17 12:25:55

by Keqian Zhu

[permalink] [raw]
Subject: [PATCH 0/2] clocksource: arm_arch_timer: Some fixes and code adjustment

During picking up pvtime LPT support for arm64, I found some trivial bugs for
arm arch_timer driver.

Keqian Zhu (2):
clocksource: arm_arch_timer: Simplify and fix count reader code logic
clocksource: arm_arch_timer: Correct fault programming of
CNTKCTL_EL1.EVNTI

arch/arm/include/asm/arch_timer.h | 14 ++--------
arch/arm64/include/asm/arch_timer.h | 24 ++--------------
drivers/clocksource/arm_arch_timer.c | 53 ++++++------------------------------
3 files changed, 13 insertions(+), 78 deletions(-)

--
1.8.3.1


2020-08-17 12:26:53

by Keqian Zhu

[permalink] [raw]
Subject: [PATCH 2/2] clocksource: arm_arch_timer: Correct fault programming of CNTKCTL_EL1.EVNTI

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.

Signed-off-by: Marc Zyngier <[email protected]>
Signed-off-by: Keqian Zhu <[email protected]>
---
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 6e11c60..4140a37 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -794,10 +794,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

2020-08-17 12:57:56

by Marc Zyngier

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

On 2020-08-17 13:24, 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.
>
> Signed-off-by: Marc Zyngier <[email protected]>

I have never given you this tag, you are making it up. Please read
Documentation/process/submitting-patches.rst to understand what
tag you can put by yourself.

At best, put "Suggested-by" tag, as this is different from what
I posted anyway.

Thanks,

M.

> Signed-off-by: Keqian Zhu <[email protected]>
> ---
> 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 6e11c60..4140a37 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -794,10 +794,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));

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

2020-08-18 01:44:00

by Keqian Zhu

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

Hi Marc,

On 2020/8/17 20:56, Marc Zyngier wrote:
> On 2020-08-17 13:24, 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.
>>
>> Signed-off-by: Marc Zyngier <[email protected]>
>
> I have never given you this tag, you are making it up. Please read
> Documentation/process/submitting-patches.rst to understand what
> tag you can put by yourself.
Sorry about my mistake.
>
> At best, put "Suggested-by" tag, as this is different from what
> I posted anyway.
OK, I will use this tag.

Thanks,
Keqian
>
> Thanks,
>
> M.
>
>> Signed-off-by: Keqian Zhu <[email protected]>
>> ---
>> 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 6e11c60..4140a37 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -794,10 +794,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));
>