On 27/08/13 15:52, Catalin Marinas wrote:

On Tue, Aug 27, 2013 at 12:37:38PM +0100, Sudeep KarkadaNagesha wrote:

quoted

On 27/08/13 12:21, Catalin Marinas wrote:

quoted

On Fri, Aug 23, 2013 at 05:19:05PM +0100, Sudeep KarkadaNagesha wrote:

quoted

From: Sudeep KarkadaNagesha <redacted>

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 <redacted>
Reviewed-by: Will Deacon <redacted>
Signed-off-by: Sudeep KarkadaNagesha <redacted>
---
 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.

Regards,
Sudeep