Re: [PATCH v4 1/5] ARM/ARM64: arch_timer: add macros for bits incontrol register

[Catalin Marinas]

On Tue, Aug 27, 2013 at 12:37:38PM +0100, Sudeep KarkadaNagesha wrote:
> On 27/08/13 12:21, Catalin Marinas wrote:
> > On Fri, Aug 23, 2013 at 05:19:05PM +0100, Sudeep KarkadaNagesha wrote:
> >> From: Sudeep KarkadaNagesha <sudeep.karkadanagesha@xxxxxxx>
> >>
> >> Add macros to describe the bitfields in the ARM architected timer
> >> control register to make code easy to understand.
> >>
> >> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> >> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> >> Reviewed-by: Will Deacon <will.deacon@xxxxxxx>
> >> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@xxxxxxx>
> >> ---
> >>  arch/arm/include/asm/arch_timer.h    |  9 +++++++--
> >>  arch/arm64/include/asm/arch_timer.h  | 12 ++++++++----
> >>  include/clocksource/arm_arch_timer.h |  8 ++++++++
> >>  3 files changed, 23 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> >> index e406d57..9ef74da 100644
> >> --- a/arch/arm/include/asm/arch_timer.h
> >> +++ b/arch/arm/include/asm/arch_timer.h
> >> @@ -95,8 +95,13 @@ static inline void arch_counter_set_user_access(void)
> >>  
> >>  	asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
> >>  
> >> -	/* disable user access to everything */
> >> -	cntkctl &= ~((3 << 8) | (7 << 0));
> >> +	/* Disable user access to both physical/virtual counters/timers. */
> >> +	/* Also disable virtual event stream */
> >> +	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
> >> +			| ARCH_TIMER_USR_VT_ACCESS_EN
> >> +			| ARCH_TIMER_VIRT_EVT_EN
> >> +			| ARCH_TIMER_USR_VCT_ACCESS_EN
> >> +			| ARCH_TIMER_USR_PCT_ACCESS_EN);
> >>  
> >>  	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
> >>  }
> >> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> >> index 98abd47..00b09d0 100644
> >> --- a/arch/arm64/include/asm/arch_timer.h
> >> +++ b/arch/arm64/include/asm/arch_timer.h
> >> @@ -101,12 +101,16 @@ static inline void arch_counter_set_user_access(void)
> >>  {
> >>  	u32 cntkctl;
> >>  
> >> -	/* Disable user access to the timers and the physical counter. */
> >>  	asm volatile("mrs	%0, cntkctl_el1" : "=r" (cntkctl));
> >> -	cntkctl &= ~((3 << 8) | (1 << 0));
> >>  
> >> -	/* Enable user access to the virtual counter and frequency. */
> >> -	cntkctl |= (1 << 1);
> >> +	/* Disable user access to the timers and the physical counter. */
> >> +	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
> >> +			| ARCH_TIMER_USR_VT_ACCESS_EN
> >> +			| ARCH_TIMER_USR_PCT_ACCESS_EN);
> >> +
> >> +	/* Enable user access to the virtual counter. */
> >> +	cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
> >> +
> >>  	asm volatile("msr	cntkctl_el1, %0" : : "r" (cntkctl));
> > 
> > For consistency with arm, I think we should also disable the event
> > stream explicitly here.
> > 
> Yes it's done. In PATCH 3/5, a new function arch_timer_evtstrm_config is
> added which is always called(PATCH 4/5). It's either enabled or disabled
> explicitly based on config option for event stream.

OK, just that in this patch arm and arm64 had different settings with
regards to the event stream.

BTW, can we not avoid clearing the event stream via the
arch_timer_evtstrm_config(false) and always assume the default as
disabled? Do you ever go back into low power mode with event stream
disabled and come back with it enabled?

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