Re: [PATCH v2] clocksource: arch_timer: Fix code to use physical timers when requested

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 26, 2014 at 2:47 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> Hi Sonny,
>
> Apologies for the delay in replying, I'd hoped to cover this at Connect,
> but we didn't seem to get the time, and since I've been back in the UK
> it slipped my mind.

Hi Mark, no problem, thanks for following up.

>
> On Thu, Sep 11, 2014 at 11:18:15PM +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"
>
> Given we cannot get firmware involved, I am happy to use the physical
> cp15 counters when we can't guarantee the value of CNTVOFF.
>
>> and completes the implementation of memory mapped access for physical
>> timers, so if a system is trying to use physical timers, it will
>> function properly.
>
> I don't see why we need to change the MMIO timers. Those are global
> rather than per-cpu, aren't turned off when CPUs go down (or they'd be
> useless), and we only use a single frame, so I don't see why the value
> of the virtual offset should matter.
>
> Additionally, the CP15 and MMIO timers could be configured separately
> w.r.t. timer and counter access, and for the MMIO timers we can
> determine which we can access by reading a register.
>
> I do not think the selection of physical/virtual timers should be shared
> by the CP15 and MMIO timers.

Ok, I see what you're saying.  I'll remove the physical memory mapped
access code and re-post.

> Mark.
>
>>
>> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Sonny Rao <sonnyrao@xxxxxxxxxxxx>
>> Acked-by: Olof Johansson <olof@xxxxxxxxx>
>> ---
>> v2: Add fixes tag to commit message, cc stable, copy Doug's
>>     description of the systems which need this in commit message.
>> ---
>>  arch/arm/include/asm/arch_timer.h    |  9 +++++++++
>>  arch/arm64/include/asm/arch_timer.h  | 10 ++++++++++
>>  drivers/clocksource/arm_arch_timer.c | 30 ++++++++++++++++++++++++++----
>>  3 files changed, 45 insertions(+), 4 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 5163ec1..ad723cb 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -30,6 +30,8 @@
>>  #define CNTTIDR              0x08
>>  #define CNTTIDR_VIRT(n)      (BIT(1) << ((n) * 4))
>>
>> +#define CNTPCT_LO    0x00
>> +#define CNTPCT_HI    0x04
>>  #define CNTVCT_LO    0x08
>>  #define CNTVCT_HI    0x0c
>>  #define CNTFRQ               0x10
>> @@ -386,6 +388,19 @@ static u64 arch_counter_get_cntvct_mem(void)
>>       return ((u64) vct_hi << 32) | vct_lo;
>>  }
>>
>> +static u64 arch_counter_get_cntpct_mem(void)
>> +{
>> +     u32 pct_lo, pct_hi, tmp_hi;
>> +
>> +     do {
>> +             pct_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
>> +             pct_lo = readl_relaxed(arch_counter_base + CNTPCT_LO);
>> +             tmp_hi = readl_relaxed(arch_counter_base + CNTPCT_HI);
>> +     } while (pct_hi != tmp_hi);
>> +
>> +     return ((u64) pct_hi << 32) | pct_lo;
>> +}
>> +
>>  /*
>>   * Default to cp15 based access because arm64 uses this function for
>>   * sched_clock() before DT is probed and the cp15 method is guaranteed
>> @@ -429,10 +444,17 @@ 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
>> -             arch_timer_read_counter = arch_counter_get_cntvct_mem;
>> +     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 {
>> +             if (arch_timer_use_virtual)
>> +                     arch_timer_read_counter = arch_counter_get_cntvct_mem;
>> +             else
>> +                     arch_timer_read_counter = arch_counter_get_cntpct_mem;
>> +     }
>>
>>       start_count = arch_timer_read_counter();
>>       clocksource_register_hz(&clocksource_counter, arch_timer_rate);
>> --
>> 1.8.3.2
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]