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

[Catalin Marinas <catalin.marinas@arm.com>]

On Tue, Aug 27, 2013 at 04:19:04PM +0100, Sudeep KarkadaNagesha wrote:
> On 27/08/13 15:52, Catalin Marinas wrote:
> > 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@arm.com>
> >>>>
> >>>> Add macros to describe the bitfields in the ARM architected timer
> >>>> control register to make code easy to understand.
> >>>>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>>> Reviewed-by: Will Deacon <will.deacon@arm.com>
> >>>> Signed-off-by: Sudeep KarkadaNagesha <sudeep.karkadanagesha@arm.com>
> >>>> ---
> >>>>  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.
> 
> Yes correct but that's how it is currently. This patch is not modifying
> anything functional, just adding macros.
> 
> > 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?
> 
> Yes that can be done. But since the cold/warm reset involves other
> firmware which can modify that bit, IMO it would be better to do it
> explicitly as CONFIG_ARM_ARCH_TIMER_EVTSTREAM=n has to ensure its
> disabled. Though I don't have a strong opinion on this, the reason for
> my inclination towards explicit disable is because of platform firmware.
> e.g. on V2P_CA15_A7/TC2 bootmon/secure firmware enables this bit based
> on some settings in board.txt
> I can remove as it's disabled on reset(ARM ARM says its reset value is
> 0) but with the assumption that other firmware don't modify that bit.

For initial boot we can initialise it and clear the EVNTEN bit. For
subsequent CPU low power events I assume we save/restore the cntkctl
register? If we do, we only need to check
CONFIG_ARM_ARCH_TIMER_EVTSTREAM once during initialisation.

-- 
Catalin