* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-10-08 7:38 [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested Sonny Rao
@ 2014-11-20 8:49 ` Maxime Ripard
2014-11-20 9:18 ` Sonny Rao
2014-11-20 9:23 ` Daniel Lezcano
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Maxime Ripard @ 2014-11-20 8:49 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Wed, Oct 08, 2014 at 12:38:57AM -0700, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false. It restores the
> arch_counter_get_cntpct() function after removal in
>
> 0d651e4e "clocksource: arch_timer: use virtual counters"
>
> We need this on certain ARMv7 systems which are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset between the
> virtual and physical counters. Each core gets a different random
> offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> One example of such as system is RK3288 where it is much simpler to
> use the physical counter since there's nobody managing the offset and
> each time a core goes down and comes back up it will get reinitialized
> to some other random value.
>
> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
> Cc: stable at vger.kernel.org
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Acked-by: Olof Johansson <olof@lixom.net>
Has this been merged yet?
If not, you can add my Tested-by, it makes the Allwinner A31 boot
flawlessly with the arch timers (together with the patch "clocksource:
arch_timer: Allow the device tree to specify uninitialized timer
registers")
Thanks!
Maxime
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141120/1344830f/attachment.sig>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-11-20 8:49 ` Maxime Ripard
@ 2014-11-20 9:18 ` Sonny Rao
0 siblings, 0 replies; 10+ messages in thread
From: Sonny Rao @ 2014-11-20 9:18 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 20, 2014 at 12:49 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Wed, Oct 08, 2014 at 12:38:57AM -0700, Sonny Rao wrote:
>> This is a bug fix for using physical arch timers when
>> the arch_timer_use_virtual boolean is false. It restores the
>> arch_counter_get_cntpct() function after removal in
>>
>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>
>> We need this on certain ARMv7 systems which are architected like this:
>>
>> * The firmware doesn't know and doesn't care about hypervisor mode and
>> we don't want to add the complexity of hypervisor there.
>>
>> * The firmware isn't involved in SMP bringup or resume.
>>
>> * The ARCH timer come up with an uninitialized offset between the
>> virtual and physical counters. Each core gets a different random
>> offset.
>>
>> * The device boots in "Secure SVC" mode.
>>
>> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>>
>> One example of such as system is RK3288 where it is much simpler to
>> use the physical counter since there's nobody managing the offset and
>> each time a core goes down and comes back up it will get reinitialized
>> to some other random value.
>>
>> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> Acked-by: Olof Johansson <olof@lixom.net>
>
> Has this been merged yet?
>
> If not, you can add my Tested-by, it makes the Allwinner A31 boot
> flawlessly with the arch timers (together with the patch "clocksource:
> arch_timer: Allow the device tree to specify uninitialized timer
> registers")
No, it has not been merged, and Doug just pinged Daniel about status
on the other patch you mentioned. I'm glad these patches are useful
to you another system, hopefully this will help the case for
inclusion.
Daniel, Mark, Will, others, is there any objection to this patch? If
not could we please merge?
> Thanks!
> Maxime
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-10-08 7:38 [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested Sonny Rao
2014-11-20 8:49 ` Maxime Ripard
@ 2014-11-20 9:23 ` Daniel Lezcano
2014-11-20 10:20 ` Daniel Lezcano
2014-11-20 16:10 ` Catalin Marinas
3 siblings, 0 replies; 10+ messages in thread
From: Daniel Lezcano @ 2014-11-20 9:23 UTC (permalink / raw)
To: linux-arm-kernel
On 10/08/2014 09:38 AM, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false. It restores the
> arch_counter_get_cntpct() function after removal in
>
> 0d651e4e "clocksource: arch_timer: use virtual counters"
>
> We need this on certain ARMv7 systems which are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset between the
> virtual and physical counters. Each core gets a different random
> offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> One example of such as system is RK3288 where it is much simpler to
> use the physical counter since there's nobody managing the offset and
> each time a core goes down and comes back up it will get reinitialized
> to some other random value.
>
> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
> Cc: stable at vger.kernel.org
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Acked-by: Olof Johansson <olof@lixom.net>
> ---
Hi Sonny,
the patch does not apply. Could you refresh it please ?
Thanks
-- Daniel
> v2: Add fixes tag to commit message, cc stable, copy Doug's
> description of the systems which need this in commit message.
> v3: Don't change the memory-mapped physical timer/counter code
> v4: remove the memory-mapped physical counter code since it's not used
> ---
> arch/arm/include/asm/arch_timer.h | 9 +++++++++
> arch/arm64/include/asm/arch_timer.h | 10 ++++++++++
> drivers/clocksource/arm_arch_timer.c | 10 +++++++---
> 3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0704e0c..e72aa4d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
> return val;
> }
>
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> + u64 cval;
> +
> + isb();
> + asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
> + return cval;
> +}
> +
> static inline u64 arch_counter_get_cntvct(void)
> {
> u64 cval;
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 9400596..58657c4 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
> #endif
> }
>
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> + u64 cval;
> +
> + isb();
> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> +
> + return cval;
> +}
> +
> static inline u64 arch_counter_get_cntvct(void)
> {
> u64 cval;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 6b50311..799139f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -429,10 +429,14 @@ static void __init arch_counter_register(unsigned type)
> u64 start_count;
>
> /* Register the CP15 based counter if we have one */
> - if (type & ARCH_CP15_TIMER)
> - arch_timer_read_counter = arch_counter_get_cntvct;
> - else
> + if (type & ARCH_CP15_TIMER) {
> + if (arch_timer_use_virtual)
> + arch_timer_read_counter = arch_counter_get_cntvct;
> + else
> + arch_timer_read_counter = arch_counter_get_cntpct;
> + } else {
> arch_timer_read_counter = arch_counter_get_cntvct_mem;
> + }
>
> start_count = arch_timer_read_counter();
> clocksource_register_hz(&clocksource_counter, arch_timer_rate);
>
--
<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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-10-08 7:38 [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested Sonny Rao
2014-11-20 8:49 ` Maxime Ripard
2014-11-20 9:23 ` Daniel Lezcano
@ 2014-11-20 10:20 ` Daniel Lezcano
2014-11-20 16:10 ` Catalin Marinas
3 siblings, 0 replies; 10+ messages in thread
From: Daniel Lezcano @ 2014-11-20 10:20 UTC (permalink / raw)
To: linux-arm-kernel
On 10/08/2014 09:38 AM, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false. It restores the
> arch_counter_get_cntpct() function after removal in
>
> 0d651e4e "clocksource: arch_timer: use virtual counters"
>
> We need this on certain ARMv7 systems which are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset between the
> virtual and physical counters. Each core gets a different random
> offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> One example of such as system is RK3288 where it is much simpler to
> use the physical counter since there's nobody managing the offset and
> each time a core goes down and comes back up it will get reinitialized
> to some other random value.
>
> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
> Cc: stable at vger.kernel.org
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Acked-by: Olof Johansson <olof@lixom.net>
> ---
Hi Sonny,
the patch does not apply. Could you refresh it please ?
Thanks
-- Daniel
> v2: Add fixes tag to commit message, cc stable, copy Doug's
> description of the systems which need this in commit message.
> v3: Don't change the memory-mapped physical timer/counter code
> v4: remove the memory-mapped physical counter code since it's not used
> ---
> arch/arm/include/asm/arch_timer.h | 9 +++++++++
> arch/arm64/include/asm/arch_timer.h | 10 ++++++++++
> drivers/clocksource/arm_arch_timer.c | 10 +++++++---
> 3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0704e0c..e72aa4d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -78,6 +78,15 @@ static inline u32 arch_timer_get_cntfrq(void)
> return val;
> }
>
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> + u64 cval;
> +
> + isb();
> + asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
> + return cval;
> +}
> +
> static inline u64 arch_counter_get_cntvct(void)
> {
> u64 cval;
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 9400596..58657c4 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
> #endif
> }
>
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> + u64 cval;
> +
> + isb();
> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> +
> + return cval;
> +}
> +
> static inline u64 arch_counter_get_cntvct(void)
> {
> u64 cval;
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 6b50311..799139f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -429,10 +429,14 @@ static void __init arch_counter_register(unsigned type)
> u64 start_count;
>
> /* Register the CP15 based counter if we have one */
> - if (type & ARCH_CP15_TIMER)
> - arch_timer_read_counter = arch_counter_get_cntvct;
> - else
> + if (type & ARCH_CP15_TIMER) {
> + if (arch_timer_use_virtual)
> + arch_timer_read_counter = arch_counter_get_cntvct;
> + else
> + arch_timer_read_counter = arch_counter_get_cntpct;
> + } else {
> arch_timer_read_counter = arch_counter_get_cntvct_mem;
> + }
>
> start_count = arch_timer_read_counter();
> clocksource_register_hz(&clocksource_counter, arch_timer_rate);
>
--
<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
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-10-08 7:38 [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested Sonny Rao
` (2 preceding siblings ...)
2014-11-20 10:20 ` Daniel Lezcano
@ 2014-11-20 16:10 ` Catalin Marinas
2014-11-20 16:24 ` Doug Anderson
3 siblings, 1 reply; 10+ messages in thread
From: Catalin Marinas @ 2014-11-20 16:10 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Oct 08, 2014 at 08:38:57AM +0100, Sonny Rao wrote:
> This is a bug fix for using physical arch timers when
> the arch_timer_use_virtual boolean is false. It restores the
> arch_counter_get_cntpct() function after removal in
>
> 0d651e4e "clocksource: arch_timer: use virtual counters"
>
> We need this on certain ARMv7 systems which are architected like this:
>
> * The firmware doesn't know and doesn't care about hypervisor mode and
> we don't want to add the complexity of hypervisor there.
>
> * The firmware isn't involved in SMP bringup or resume.
>
> * The ARCH timer come up with an uninitialized offset between the
> virtual and physical counters. Each core gets a different random
> offset.
>
> * The device boots in "Secure SVC" mode.
>
> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>
> One example of such as system is RK3288 where it is much simpler to
> use the physical counter since there's nobody managing the offset and
> each time a core goes down and comes back up it will get reinitialized
> to some other random value.
>
> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
> Cc: stable at vger.kernel.org
> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> Acked-by: Olof Johansson <olof@lixom.net>
[...]
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
> #endif
> }
>
> +static inline u64 arch_counter_get_cntpct(void)
> +{
> + u64 cval;
> +
> + isb();
> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> +
> + return cval;
> +}
Sorry but I have to NAK the arm64 changes here. If the firmware is
broken and does not initialise CNTVOFF properly, please fix it (at least
on ARMv8 hardware). Also, on arm64 the vdso gettimeofday()
implementation relies on using the virtual counter, so correct
initialisation of CNTVOFF is essential.
--
Catalin
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-11-20 16:10 ` Catalin Marinas
@ 2014-11-20 16:24 ` Doug Anderson
2014-11-20 16:58 ` Catalin Marinas
0 siblings, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2014-11-20 16:24 UTC (permalink / raw)
To: linux-arm-kernel
Catalin,
On Thu, Nov 20, 2014 at 8:10 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Wed, Oct 08, 2014 at 08:38:57AM +0100, Sonny Rao wrote:
>> This is a bug fix for using physical arch timers when
>> the arch_timer_use_virtual boolean is false. It restores the
>> arch_counter_get_cntpct() function after removal in
>>
>> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>
>> We need this on certain ARMv7 systems which are architected like this:
>>
>> * The firmware doesn't know and doesn't care about hypervisor mode and
>> we don't want to add the complexity of hypervisor there.
>>
>> * The firmware isn't involved in SMP bringup or resume.
>>
>> * The ARCH timer come up with an uninitialized offset between the
>> virtual and physical counters. Each core gets a different random
>> offset.
>>
>> * The device boots in "Secure SVC" mode.
>>
>> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>>
>> One example of such as system is RK3288 where it is much simpler to
>> use the physical counter since there's nobody managing the offset and
>> each time a core goes down and comes back up it will get reinitialized
>> to some other random value.
>>
>> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> Acked-by: Olof Johansson <olof@lixom.net>
> [...]
>> --- a/arch/arm64/include/asm/arch_timer.h
>> +++ b/arch/arm64/include/asm/arch_timer.h
>> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
>> #endif
>> }
>>
>> +static inline u64 arch_counter_get_cntpct(void)
>> +{
>> + u64 cval;
>> +
>> + isb();
>> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
>> +
>> + return cval;
>> +}
>
> Sorry but I have to NAK the arm64 changes here. If the firmware is
> broken and does not initialise CNTVOFF properly, please fix it (at least
> on ARMv8 hardware). Also, on arm64 the vdso gettimeofday()
> implementation relies on using the virtual counter, so correct
> initialisation of CNTVOFF is essential.
Sonny's patch here just makes it so that we honor the global variable.
My patch at <https://patchwork.kernel.org/patch/5051881/> is the one
that allows the global variable to be set. You can see in that patch
that it's impossible for the variable to be set on ARM64.
In previous discussions it was agreed that on ARM64 psci (or something
similar) was a requirement anyway and that gave us a way to get the
firmware involved again if we ever need to bring down a processor and
bring it back up in the kernel. PSCI is not a requirement for ARM32.
There are systems that don't get the firmware involved when a
processor loses state (like if it is powered off and powered on again,
maybe for suspend/resume) and there was pushback against the kernel
itself transitioning into monitor mode to init CNTVOFF in these cases.
People agreed a month ago that these two patches were a reasonable
approach for ARM32.
There is still a question about the ARM32 vdso gettimeofday(). That
will need to get resolved eventually to, but that should be a separate
patch I believe.
NOTE: a firmware which actually implements a monitor and somehow
solves the S2R problems could modify the device tree passed to the
kernel to remove the "arm,cpu-registers-not-fw-configured" which would
switch us back into virtual timers.
-Doug
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-11-20 16:24 ` Doug Anderson
@ 2014-11-20 16:58 ` Catalin Marinas
2014-11-21 20:58 ` Olof Johansson
0 siblings, 1 reply; 10+ messages in thread
From: Catalin Marinas @ 2014-11-20 16:58 UTC (permalink / raw)
To: linux-arm-kernel
Doug,
On Thu, Nov 20, 2014 at 04:24:09PM +0000, Doug Anderson wrote:
> On Thu, Nov 20, 2014 at 8:10 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Wed, Oct 08, 2014 at 08:38:57AM +0100, Sonny Rao wrote:
> >> This is a bug fix for using physical arch timers when
> >> the arch_timer_use_virtual boolean is false. It restores the
> >> arch_counter_get_cntpct() function after removal in
> >>
> >> 0d651e4e "clocksource: arch_timer: use virtual counters"
> >>
> >> We need this on certain ARMv7 systems which are architected like this:
> >>
> >> * The firmware doesn't know and doesn't care about hypervisor mode and
> >> we don't want to add the complexity of hypervisor there.
> >>
> >> * The firmware isn't involved in SMP bringup or resume.
> >>
> >> * The ARCH timer come up with an uninitialized offset between the
> >> virtual and physical counters. Each core gets a different random
> >> offset.
> >>
> >> * The device boots in "Secure SVC" mode.
> >>
> >> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
> >> CNTHCTL.PL1PCTEN (both default to 1 at reset)
> >>
> >> One example of such as system is RK3288 where it is much simpler to
> >> use the physical counter since there's nobody managing the offset and
> >> each time a core goes down and comes back up it will get reinitialized
> >> to some other random value.
> >>
> >> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
> >> Cc: stable at vger.kernel.org
> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
> >> Acked-by: Olof Johansson <olof@lixom.net>
> > [...]
> >> --- a/arch/arm64/include/asm/arch_timer.h
> >> +++ b/arch/arm64/include/asm/arch_timer.h
> >> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
> >> #endif
> >> }
> >>
> >> +static inline u64 arch_counter_get_cntpct(void)
> >> +{
> >> + u64 cval;
> >> +
> >> + isb();
> >> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
> >> +
> >> + return cval;
> >> +}
> >
> > Sorry but I have to NAK the arm64 changes here. If the firmware is
> > broken and does not initialise CNTVOFF properly, please fix it (at least
> > on ARMv8 hardware). Also, on arm64 the vdso gettimeofday()
> > implementation relies on using the virtual counter, so correct
> > initialisation of CNTVOFF is essential.
>
> Sonny's patch here just makes it so that we honor the global variable.
> My patch at <https://patchwork.kernel.org/patch/5051881/> is the one
> that allows the global variable to be set. You can see in that patch
> that it's impossible for the variable to be set on ARM64.
It just gives people ideas ;), thinking they only need to remove
IS_ENABLED(CONFIG_ARM) in your patch and get this working on arm64.
> In previous discussions it was agreed that on ARM64 psci (or something
> similar) was a requirement anyway and that gave us a way to get the
> firmware involved again if we ever need to bring down a processor and
> bring it back up in the kernel. PSCI is not a requirement for ARM32.
> There are systems that don't get the firmware involved when a
> processor loses state (like if it is powered off and powered on again,
> maybe for suspend/resume) and there was pushback against the kernel
> itself transitioning into monitor mode to init CNTVOFF in these cases.
> People agreed a month ago that these two patches were a reasonable
> approach for ARM32.
I'm not complaining about about arm32 here, just the arm64
implementation. If you want to avoid #ifdefs in the arch timer driver,
what about, for arm64, defining something like:
static inline u64 arch_counter_get_cntpct(void)
{
/*
* AArch64 kernel and user space mandate the use of CNTVCT.
*/
BUG();
return 0;
}
--
Catalin
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-11-20 16:58 ` Catalin Marinas
@ 2014-11-21 20:58 ` Olof Johansson
2014-11-24 2:25 ` Sonny Rao
0 siblings, 1 reply; 10+ messages in thread
From: Olof Johansson @ 2014-11-21 20:58 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Nov 20, 2014 at 8:58 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> Doug,
>
> On Thu, Nov 20, 2014 at 04:24:09PM +0000, Doug Anderson wrote:
>> On Thu, Nov 20, 2014 at 8:10 AM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On Wed, Oct 08, 2014 at 08:38:57AM +0100, Sonny Rao wrote:
>> >> This is a bug fix for using physical arch timers when
>> >> the arch_timer_use_virtual boolean is false. It restores the
>> >> arch_counter_get_cntpct() function after removal in
>> >>
>> >> 0d651e4e "clocksource: arch_timer: use virtual counters"
>> >>
>> >> We need this on certain ARMv7 systems which are architected like this:
>> >>
>> >> * The firmware doesn't know and doesn't care about hypervisor mode and
>> >> we don't want to add the complexity of hypervisor there.
>> >>
>> >> * The firmware isn't involved in SMP bringup or resume.
>> >>
>> >> * The ARCH timer come up with an uninitialized offset between the
>> >> virtual and physical counters. Each core gets a different random
>> >> offset.
>> >>
>> >> * The device boots in "Secure SVC" mode.
>> >>
>> >> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>> >> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>> >>
>> >> One example of such as system is RK3288 where it is much simpler to
>> >> use the physical counter since there's nobody managing the offset and
>> >> each time a core goes down and comes back up it will get reinitialized
>> >> to some other random value.
>> >>
>> >> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>> >> Cc: stable at vger.kernel.org
>> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>> >> Acked-by: Olof Johansson <olof@lixom.net>
>> > [...]
>> >> --- a/arch/arm64/include/asm/arch_timer.h
>> >> +++ b/arch/arm64/include/asm/arch_timer.h
>> >> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
>> >> #endif
>> >> }
>> >>
>> >> +static inline u64 arch_counter_get_cntpct(void)
>> >> +{
>> >> + u64 cval;
>> >> +
>> >> + isb();
>> >> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
>> >> +
>> >> + return cval;
>> >> +}
>> >
>> > Sorry but I have to NAK the arm64 changes here. If the firmware is
>> > broken and does not initialise CNTVOFF properly, please fix it (at least
>> > on ARMv8 hardware). Also, on arm64 the vdso gettimeofday()
>> > implementation relies on using the virtual counter, so correct
>> > initialisation of CNTVOFF is essential.
>>
>> Sonny's patch here just makes it so that we honor the global variable.
>> My patch at <https://patchwork.kernel.org/patch/5051881/> is the one
>> that allows the global variable to be set. You can see in that patch
>> that it's impossible for the variable to be set on ARM64.
>
> It just gives people ideas ;), thinking they only need to remove
> IS_ENABLED(CONFIG_ARM) in your patch and get this working on arm64.
>
>> In previous discussions it was agreed that on ARM64 psci (or something
>> similar) was a requirement anyway and that gave us a way to get the
>> firmware involved again if we ever need to bring down a processor and
>> bring it back up in the kernel. PSCI is not a requirement for ARM32.
>> There are systems that don't get the firmware involved when a
>> processor loses state (like if it is powered off and powered on again,
>> maybe for suspend/resume) and there was pushback against the kernel
>> itself transitioning into monitor mode to init CNTVOFF in these cases.
>> People agreed a month ago that these two patches were a reasonable
>> approach for ARM32.
>
> I'm not complaining about about arm32 here, just the arm64
> implementation. If you want to avoid #ifdefs in the arch timer driver,
> what about, for arm64, defining something like:
>
> static inline u64 arch_counter_get_cntpct(void)
> {
> /*
> * AArch64 kernel and user space mandate the use of CNTVCT.
> */
> BUG();
> return 0;
> }
Seems like a reasonable approach to me.
-Olof
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] clocksource: arch_timer: Fix code to use physical timers when requested
2014-11-21 20:58 ` Olof Johansson
@ 2014-11-24 2:25 ` Sonny Rao
0 siblings, 0 replies; 10+ messages in thread
From: Sonny Rao @ 2014-11-24 2:25 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 21, 2014 at 12:58 PM, Olof Johansson <olof@lixom.net> wrote:
> On Thu, Nov 20, 2014 at 8:58 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
>> Doug,
>>
>> On Thu, Nov 20, 2014 at 04:24:09PM +0000, Doug Anderson wrote:
>>> On Thu, Nov 20, 2014 at 8:10 AM, Catalin Marinas
>>> <catalin.marinas@arm.com> wrote:
>>> > On Wed, Oct 08, 2014 at 08:38:57AM +0100, Sonny Rao wrote:
>>> >> This is a bug fix for using physical arch timers when
>>> >> the arch_timer_use_virtual boolean is false. It restores the
>>> >> arch_counter_get_cntpct() function after removal in
>>> >>
>>> >> 0d651e4e "clocksource: arch_timer: use virtual counters"
>>> >>
>>> >> We need this on certain ARMv7 systems which are architected like this:
>>> >>
>>> >> * The firmware doesn't know and doesn't care about hypervisor mode and
>>> >> we don't want to add the complexity of hypervisor there.
>>> >>
>>> >> * The firmware isn't involved in SMP bringup or resume.
>>> >>
>>> >> * The ARCH timer come up with an uninitialized offset between the
>>> >> virtual and physical counters. Each core gets a different random
>>> >> offset.
>>> >>
>>> >> * The device boots in "Secure SVC" mode.
>>> >>
>>> >> * Nothing has touched the reset value of CNTHCTL.PL1PCEN or
>>> >> CNTHCTL.PL1PCTEN (both default to 1 at reset)
>>> >>
>>> >> One example of such as system is RK3288 where it is much simpler to
>>> >> use the physical counter since there's nobody managing the offset and
>>> >> each time a core goes down and comes back up it will get reinitialized
>>> >> to some other random value.
>>> >>
>>> >> Fixes: 0d651e4e65e9 ("clocksource: arch_timer: use virtual counters")
>>> >> Cc: stable at vger.kernel.org
>>> >> Signed-off-by: Sonny Rao <sonnyrao@chromium.org>
>>> >> Acked-by: Olof Johansson <olof@lixom.net>
>>> > [...]
>>> >> --- a/arch/arm64/include/asm/arch_timer.h
>>> >> +++ b/arch/arm64/include/asm/arch_timer.h
>>> >> @@ -135,6 +135,16 @@ static inline void arch_timer_evtstrm_enable(int divider)
>>> >> #endif
>>> >> }
>>> >>
>>> >> +static inline u64 arch_counter_get_cntpct(void)
>>> >> +{
>>> >> + u64 cval;
>>> >> +
>>> >> + isb();
>>> >> + asm volatile("mrs %0, cntpct_el0" : "=r" (cval));
>>> >> +
>>> >> + return cval;
>>> >> +}
>>> >
>>> > Sorry but I have to NAK the arm64 changes here. If the firmware is
>>> > broken and does not initialise CNTVOFF properly, please fix it (at least
>>> > on ARMv8 hardware). Also, on arm64 the vdso gettimeofday()
>>> > implementation relies on using the virtual counter, so correct
>>> > initialisation of CNTVOFF is essential.
>>>
>>> Sonny's patch here just makes it so that we honor the global variable.
>>> My patch at <https://patchwork.kernel.org/patch/5051881/> is the one
>>> that allows the global variable to be set. You can see in that patch
>>> that it's impossible for the variable to be set on ARM64.
>>
>> It just gives people ideas ;), thinking they only need to remove
>> IS_ENABLED(CONFIG_ARM) in your patch and get this working on arm64.
>>
>>> In previous discussions it was agreed that on ARM64 psci (or something
>>> similar) was a requirement anyway and that gave us a way to get the
>>> firmware involved again if we ever need to bring down a processor and
>>> bring it back up in the kernel. PSCI is not a requirement for ARM32.
>>> There are systems that don't get the firmware involved when a
>>> processor loses state (like if it is powered off and powered on again,
>>> maybe for suspend/resume) and there was pushback against the kernel
>>> itself transitioning into monitor mode to init CNTVOFF in these cases.
>>> People agreed a month ago that these two patches were a reasonable
>>> approach for ARM32.
>>
>> I'm not complaining about about arm32 here, just the arm64
>> implementation. If you want to avoid #ifdefs in the arch timer driver,
>> what about, for arm64, defining something like:
>>
>> static inline u64 arch_counter_get_cntpct(void)
>> {
>> /*
>> * AArch64 kernel and user space mandate the use of CNTVCT.
>> */
>> BUG();
>> return 0;
>> }
>
> Seems like a reasonable approach to me.
Ok, I will re-spin this one, sorry for the delay.
>
>
> -Olof
^ permalink raw reply [flat|nested] 10+ messages in thread