Unify arm_generic and arch_timer drivers

When we get the device_node for the arch timer, it's refcount is automatically incremented in of_find_matching_node, but it is never decremented.

This patch decrements the refcount on the node after we're finished using it.

Reported-by Will Deacon <[email protected]>

Signed-off-by Mark Rutland <[email protected]>

---

This check is a holdover from the pre-devicetree days. As the timer is not probed except by platforms which register it via devicetree, it's not strictly necessary.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---

Quoted 4 lines · Mark Rutland

This check is a holdover from the pre-devicetree days. As the timer is not probed except by platforms which register it via devicetree, it's not strictly necessary.

Quoted 4 lines

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---

Multi-platform build without DT could still benefit from the check but I guess the most of the A15 based platform are DT only, so its should be good to get rid of the check.

Regards,
Santosh

To ensure the correct size of types, use u64 for the return value of arch_timer_get_cnt{p,v}ct, and u32 for arch_timer_rate, matching the size of the registers these values are taken from. While we're changing them anyway, simplify the implementation of arch_timer_get_cnt{p,v}ct.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---

We're currently inconsistent with respect to our accesses to the physical and virtual counters, mixing and matching the two.

This patch introduces and uses a function for accessing the correct counter based on whether we're using physical or virtual interrupts. All current accesses to the counter accessors are redirected through it.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/kernel/arch_timer.c |   48 ++++++++++-------------------------------
  1 files changed, 12 insertions(+), 36 deletions(-)

arch/arm/kernel/arch_timer.c +5 -9

index 498c29f..0d2681c 100644
@@ -272,51 +272,32 @@ static int arch_timer_available(void)
  	return 0;
  }
 
-static u32 notrace arch_counter_get_cntpct32(void)
+u64 arch_timer_read_counter(void)
  {
-	cycle_t cnt = arch_counter_get_cntpct();
-
-	/*
-	 * The sched_clock infrastructure only knows about counters
-	 * with at most 32bits. Forget about the upper 24 bits for the
-	 * time being...
-	 */
-	return (u32)cnt;
+	if (arch_timer_use_virtual)
+		return arch_counter_get_cntvct();
+	else
+		return arch_counter_get_cntpct();
  }
 

arch/arm/kernel/arch_timer.c +0 -1

@@ -489,18 +470,13 @@ int __init arch_timer_of_register(void)
 
  int __init arch_timer_sched_clock_init(void)
  {
-	u32 (*cnt32)(void);
  	int err;
 
  	err = arch_timer_available();
  	if (err)
  		return err;
 

Quoted 40 lines · Mark Rutland

We're currently inconsistent with respect to our accesses to the physical and virtual counters, mixing and matching the two.

This patch introduces and uses a function for accessing the correct counter based on whether we're using physical or virtual interrupts. All current accesses to the counter accessors are redirected through it.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/kernel/arch_timer.c |   48 ++++++++++-------------------------------
  1 files changed, 12 insertions(+), 36 deletions(-)

arch/arm/kernel/arch_timer.c +5 -9

index 498c29f..0d2681c 100644
@@ -272,51 +272,32 @@ static int arch_timer_available(void)
  	return 0;
  }
 
-static u32 notrace arch_counter_get_cntpct32(void)
+u64 arch_timer_read_counter(void)
  {
-	cycle_t cnt = arch_counter_get_cntpct();
-
-	/*
-	 * The sched_clock infrastructure only knows about counters
-	 * with at most 32bits. Forget about the upper 24 bits for the
-	 * time being...
-	 */
-	return (u32)cnt;
+	if (arch_timer_use_virtual)
+		return arch_counter_get_cntvct();
+	else
+		return arch_counter_get_cntpct();
  }
 

[...]

Quoted 11 lines

arch/arm/kernel/arch_timer.c +0 -1

@@ -489,18 +470,13 @@ int __init arch_timer_of_register(void)
 
  int __init arch_timer_sched_clock_init(void)
  {
-	u32 (*cnt32)(void);
  	int err;
 
  	err = arch_timer_available();
  	if (err)
  		return err;
 

arch/arm/kernel/arch_timer.c int __init arch_timer_of_register(void) · line 482

Quoted 11 lines

arch/arm/kernel/arch_timer.c +2 -6

-	if (arch_timer_use_virtual)
-		cnt32 = arch_counter_get_cntvct32;
-	else
-		cnt32 = arch_counter_get_cntpct32;
-
-	setup_sched_clock(cnt32, 32, arch_timer_rate);
+	setup_sched_clock(arch_timer_read_counter32,
+			  32, arch_timer_rate);
  	return 0;
  }

I think the original idea had merit since the check was needed in init code instead of proposed one which has if check for every counter read function. No ?

Quoted 55 lines · Santosh Shilimkar

Quoted 40 lines · Mark Rutland

We're currently inconsistent with respect to our accesses to the physical and virtual counters, mixing and matching the two.

This patch introduces and uses a function for accessing the correct counter based on whether we're using physical or virtual interrupts. All current accesses to the counter accessors are redirected through it.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
   arch/arm/kernel/arch_timer.c |   48 ++++++++++-------------------------------
   1 files changed, 12 insertions(+), 36 deletions(-)

arch/arm/kernel/arch_timer.c +5 -9

index 498c29f..0d2681c 100644
@@ -272,51 +272,32 @@ static int arch_timer_available(void)
   	return 0;
   }
 
-static u32 notrace arch_counter_get_cntpct32(void)
+u64 arch_timer_read_counter(void)
   {
-	cycle_t cnt = arch_counter_get_cntpct();
-
-	/*
-	 * The sched_clock infrastructure only knows about counters
-	 * with at most 32bits. Forget about the upper 24 bits for the
-	 * time being...
-	 */
-	return (u32)cnt;
+	if (arch_timer_use_virtual)
+		return arch_counter_get_cntvct();
+	else
+		return arch_counter_get_cntpct();
   }
 

[...]

Quoted 11 lines

arch/arm/kernel/arch_timer.c +0 -1

@@ -489,18 +470,13 @@ int __init arch_timer_of_register(void)
 
   int __init arch_timer_sched_clock_init(void)
   {
-	u32 (*cnt32)(void);
   	int err;
 
   	err = arch_timer_available();
   	if (err)
   		return err;
 

Quoted 15 lines

arch/arm/kernel/arch_timer.c int __init arch_timer_of_register(void) · line 482

Quoted 11 lines

arch/arm/kernel/arch_timer.c +2 -6

-	if (arch_timer_use_virtual)
-		cnt32 = arch_counter_get_cntvct32;
-	else
-		cnt32 = arch_counter_get_cntpct32;
-
-	setup_sched_clock(cnt32, 32, arch_timer_rate);
+	setup_sched_clock(arch_timer_read_counter32,
+			  32, arch_timer_rate);
   	return 0;
   }

I think the original idea had merit since the check was needed in init code instead of proposed one which has if check for every counter read function. No ?

The original idea was good in that it avoided the check on each read path, but in several places the logic got duplicated (e.g. for choosing which width-altering wrapper in the above block). I'd like ensure this logic is consolidated.

I'll change arch_timer_read_counter to a function pointer, and set this in arch_timer_of_register before registering anything. Everything would still be indirected through it, but it won't have to do a check on every read.

The CNTFRQ register is not duplicated for physical and virtual timers, and accessing it as if it were is confusing.

Instead, use a separate accessor which doesn't take the access type as a parameter.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/kernel/arch_timer.c |   17 +++++++++--------
  1 files changed, 9 insertions(+), 8 deletions(-)

arch/arm/kernel/arch_timer.c +8 -5

index 0d2681c..fc87d3d 100644
@@ -51,8 +51,7 @@ static bool arch_timer_use_virtual = true;
  #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
 
  #define ARCH_TIMER_REG_CTRL		0
-#define ARCH_TIMER_REG_FREQ		1
-#define ARCH_TIMER_REG_TVAL		2
+#define ARCH_TIMER_REG_TVAL		1
 
  #define ARCH_TIMER_PHYS_ACCESS		0
  #define ARCH_TIMER_VIRT_ACCESS		1

@@ -101,9 +100,6 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
  		case ARCH_TIMER_REG_TVAL:
  			asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
  			break;
-		case ARCH_TIMER_REG_FREQ:
-			asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
-			break;
  		}
  	}
 

@@ -121,6 +117,13 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
  	return val;
  }
 
+static inline u32 arch_timer_get_cntfrq(void)
+{
+	u32 val;
+	asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
+	return val;
+}
+
  static inline u64 arch_counter_get_cntpct(void)
  {
  	u64 cval;

Quoted 54 lines · Mark Rutland

The CNTFRQ register is not duplicated for physical and virtual timers, and accessing it as if it were is confusing.

Instead, use a separate accessor which doesn't take the access type as a parameter.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/kernel/arch_timer.c |   17 +++++++++--------
  1 files changed, 9 insertions(+), 8 deletions(-)

arch/arm/kernel/arch_timer.c +8 -5

index 0d2681c..fc87d3d 100644
@@ -51,8 +51,7 @@ static bool arch_timer_use_virtual = true;
  #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
 
  #define ARCH_TIMER_REG_CTRL		0
-#define ARCH_TIMER_REG_FREQ		1
-#define ARCH_TIMER_REG_TVAL		2
+#define ARCH_TIMER_REG_TVAL		1
 
  #define ARCH_TIMER_PHYS_ACCESS		0
  #define ARCH_TIMER_VIRT_ACCESS		1

@@ -101,9 +100,6 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
  		case ARCH_TIMER_REG_TVAL:
  			asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
  			break;
-		case ARCH_TIMER_REG_FREQ:
-			asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
-			break;
  		}
  	}
 

@@ -121,6 +117,13 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
  	return val;
  }
 
+static inline u32 arch_timer_get_cntfrq(void)
+{
+	u32 val;
+	asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
+	return val;
+}
+
  static inline u64 arch_counter_get_cntpct(void)
  {
  	u64 cval;

@@ -253,9 +256,7 @@ static int arch_timer_available(void)
  	u32 freq;
 

arch/arm/kernel/arch_timer.c static int arch_timer_available(void) · line 259

Quoted 5 lines

arch/arm/kernel/arch_timer.c +1 -3

  	if (arch_timer_rate == 0) {
-		freq = arch_timer_reg_read(ARCH_TIMER_PHYS_ACCESS,
-					   ARCH_TIMER_REG_FREQ);
-
+		freq = arch_timer_get_cntfrq();

Not related to this patch a new line here will be good.

arch/arm/kernel/arch_timer.c static int arch_timer_available(void) · line 262

Quoted 4 lines

arch/arm/kernel/arch_timer.c +0 -0

  		/* Check the timer frequency. */
  		if (freq == 0) {
  			pr_warn("Architected timer frequency not available\n");

Otherwise patch looks fine to me.

Acked-by Santosh Shilimkar<[email protected]>

Currently the arch_timer register accessors are thrown together with the main driver, preventing us from porting the driver to other architectures.

This patch moves the register accessors into a header file, as with the arm64 version. Constants required by the accessors are also moved.

Additionally isbs are added in arch_timer_get_cnt{v,p}ct to prevent the cpu from speculating the reads and returning stale values.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/include/asm/arch_timer.h |  101 +++++++++++++++++++++++++++++++++++++
  arch/arm/kernel/arch_timer.c      |   92 ---------------------------------
  2 files changed, 101 insertions(+), 92 deletions(-)

arch/arm/include/asm/arch_timer.h +35 -0

index d40229d..701f2b7 100644
@@ -1,13 +1,114 @@
  #ifndef __ASMARM_ARCH_TIMER_H
  #define __ASMARM_ARCH_TIMER_H
 
+#include <asm/barrier.h>
  #include <asm/errno.h>
+
  #include <linux/clocksource.h>
+#include <linux/types.h>
 
  #ifdef CONFIG_ARM_ARCH_TIMER
  int arch_timer_of_register(void);
  int arch_timer_sched_clock_init(void);
  struct timecounter *arch_timer_get_timecounter(void);
+
+#define ARCH_TIMER_CTRL_ENABLE		(1 << 0)
+#define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
+#define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
+
+#define ARCH_TIMER_REG_CTRL		0
+#define ARCH_TIMER_REG_TVAL		1
+
+#define ARCH_TIMER_PHYS_ACCESS		0
+#define ARCH_TIMER_VIRT_ACCESS		1
+
+/*
+ * These register accessors are marked inline so the compiler can
+ * nicely work out which register we want, and chuck away the rest of
+ * the code. At least it does so with a recent GCC (4.6.3).
+ */
+static inline void arch_timer_reg_write(const int access, const int reg, u32 val)
+{
+	if (access == ARCH_TIMER_PHYS_ACCESS) {
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" (val));
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
+			break;
+		}
+	}
+
+	if (access == ARCH_TIMER_VIRT_ACCESS) {
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:

Quoted 68 lines · Mark Rutland

Currently the arch_timer register accessors are thrown together with the main driver, preventing us from porting the driver to other architectures.

This patch moves the register accessors into a header file, as with the arm64 version. Constants required by the accessors are also moved.

Additionally isbs are added in arch_timer_get_cnt{v,p}ct to prevent the cpu from speculating the reads and returning stale values.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/include/asm/arch_timer.h |  101 +++++++++++++++++++++++++++++++++++++
  arch/arm/kernel/arch_timer.c      |   92 ---------------------------------
  2 files changed, 101 insertions(+), 92 deletions(-)

arch/arm/include/asm/arch_timer.h +35 -0

index d40229d..701f2b7 100644
@@ -1,13 +1,114 @@
  #ifndef __ASMARM_ARCH_TIMER_H
  #define __ASMARM_ARCH_TIMER_H
 
+#include <asm/barrier.h>
  #include <asm/errno.h>
+
  #include <linux/clocksource.h>
+#include <linux/types.h>
 
  #ifdef CONFIG_ARM_ARCH_TIMER
  int arch_timer_of_register(void);
  int arch_timer_sched_clock_init(void);
  struct timecounter *arch_timer_get_timecounter(void);
+
+#define ARCH_TIMER_CTRL_ENABLE		(1 << 0)
+#define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
+#define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
+
+#define ARCH_TIMER_REG_CTRL		0
+#define ARCH_TIMER_REG_TVAL		1
+
+#define ARCH_TIMER_PHYS_ACCESS		0
+#define ARCH_TIMER_VIRT_ACCESS		1
+
+/*
+ * These register accessors are marked inline so the compiler can
+ * nicely work out which register we want, and chuck away the rest of
+ * the code. At least it does so with a recent GCC (4.6.3).
+ */
+static inline void arch_timer_reg_write(const int access, const int reg, u32 val)
+{
+	if (access == ARCH_TIMER_PHYS_ACCESS) {
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" (val));
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
+			break;
+		}
+	}
+
+	if (access == ARCH_TIMER_VIRT_ACCESS) {
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:

arch/arm/include/asm/arch_timer.h line 55

Quoted 10 lines

arch/arm/include/asm/arch_timer.h +10 -0

+			asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" (val));
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" (val));
+			break;
+		}
+	}
+
+	isb();
+}

The isb() additions is actually a sepoerate fix. I suggest you to split the subject patch into 1) Movement of header data 2) isb() additions.

Feel free to add my ack on updated patches if you agree.

Regards,
Santosh

Currently, the arch_timer driver is tied to the arm port, as it relies on code in arch/arm/smp.c to setup and teardown timers as cores are hotplugged on and off. The timer is registered through an arm-specific registration mechanism, preventing sharing the driver with the arm64 port.

This patch moves the driver to using a cpu notifier instead, making it easier to port.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---

Quoted 9 lines · Mark Rutland

Currently, the arch_timer driver is tied to the arm port, as it relies on code in arch/arm/smp.c to setup and teardown timers as cores are hotplugged on and off. The timer is registered through an arm-specific registration mechanism, preventing sharing the driver with the arm64 port.

This patch moves the driver to using a cpu notifier instead, making it easier to port.

Quoted 4 lines

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---

This is really a nit idea. I think we should do the same for ARM gic code.

Quoted 14 lines · Santosh Shilimkar

Quoted 13 lines · Mark Rutland

Currently, the arch_timer driver is tied to the arm port, as it relies on code in arch/arm/smp.c to setup and teardown timers as cores are hotplugged on and off. The timer is registered through an arm-specific registration mechanism, preventing sharing the driver with the arm64 port.

This patch moves the driver to using a cpu notifier instead, making it easier to port.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---

Quoted 2 lines

This is really a nit idea. I think we should do the same for ARM gic code.

I plan to do the same once Rob's GIC patches get merged. In my soc-armv8-model branch I have a copy of gic.c into drivers/irqchip and the CPU interface is done automatically via a notifier. The only trick is to set the priority of the GIC notifier higher than the timer one.

Several bits in CNTKCTL reset to 0, including PL0VTEN. For platforms using the generic timer which wish to have a fast gettimeofday vDSO implementation, these bits must be set to 1 by the kernel. On other platforms, the bootloader might enable userspace access when we don't want it.

This patch adds arch_counter_set_user_access, which sets the PL0 access permissions to that required by the platform. For arm, this currently

means disabling all userspace access.

Signed-off-by Mark Rutland <[email protected]>

---
  arch/arm/include/asm/arch_timer.h |   11 +++++++++++
  arch/arm/kernel/arch_timer.c      |    2 ++
  2 files changed, 13 insertions(+), 0 deletions(-)

arch/arm/include/asm/arch_timer.h +11 -0

index 701f2b7..05e3593 100644
@@ -108,6 +108,17 @@ static inline u64 arch_counter_get_cntvct(void)
  	return cval;
  }
 
+static inline void __cpuinit arch_counter_set_user_access(void)
+{
+	u32 cntkctl;
+
+	asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
+
+	/* disable user access to everything */
+	cntkctl &= ~((3 << 8) | (7 << 0));
+
+	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+}
 
  #else
  static inline int arch_timer_of_register(void)

arch/arm/kernel/arch_timer.c +0 -0

index 834d347..4f39e68 100644

Quoted 8 lines · Mark Rutland

Several bits in CNTKCTL reset to 0, including PL0VTEN. For platforms using the generic timer which wish to have a fast gettimeofday vDSO implementation, these bits must be set to 1 by the kernel. On other platforms, the bootloader might enable userspace access when we don't want it.

This patch adds arch_counter_set_user_access, which sets the PL0 access permissions to that required by the platform. For arm, this currently

minor nit.
s/arm/ARM

Quoted 38 lines

means disabling all userspace access.

Signed-off-by Mark Rutland <[email protected]>

---
  arch/arm/include/asm/arch_timer.h |   11 +++++++++++
  arch/arm/kernel/arch_timer.c      |    2 ++
  2 files changed, 13 insertions(+), 0 deletions(-)

arch/arm/include/asm/arch_timer.h +11 -0

index 701f2b7..05e3593 100644
@@ -108,6 +108,17 @@ static inline u64 arch_counter_get_cntvct(void)
  	return cval;
  }
 
+static inline void __cpuinit arch_counter_set_user_access(void)
+{
+	u32 cntkctl;
+
+	asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
+
+	/* disable user access to everything */
+	cntkctl &= ~((3 << 8) | (7 << 0));
+
+	asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
+}
 
  #else
  static inline int arch_timer_of_register(void)

arch/arm/kernel/arch_timer.c +0 -0

index 834d347..4f39e68 100644
@@ -155,6 +155,8 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
  			enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
  	}
 

arch/arm/kernel/arch_timer.c static int __cpuinit arch_timer_setup(struct clock_event_device *clk) · line 158

Quoted 1 line

arch/arm/kernel/arch_timer.c +1 -0

+	arch_counter_set_user_access();

So how do you expect platform to enabled the user-space access in case they want to access it for some cases.

Regards
Santosh

Quoted 2 lines · Santosh Shilimkar

So how do you expect platform to enabled the user-space access in case they want to access it for some cases.

Unlike AArch64, at the moment we don't have the infrastructure to map this for userspace accesses, so it isn't much of a problem.

If in future we wish to map it on 32bit platforms, the arm implementation of arch_counter_set_user_access can be modified to allow userspace access to specific registers, and additional code would be required to actually map it into the user address space, etc.

Quoted 11 lines · Mark Rutland

Quoted 2 lines · Santosh Shilimkar

So how do you expect platform to enabled the user-space access in case they want to access it for some cases.

Unlike AArch64, at the moment we don't have the infrastructure to map this for userspace accesses, so it isn't much of a problem.

If in future we wish to map it on 32bit platforms, the arm implementation of arch_counter_set_user_access can be modified to allow userspace access to specific registers, and additional code would be required to actually map it into the user address space, etc.

I'd also add that it's not up to a platform to decide whether to expose this to userspace: it needs to be an architecture-wide decision. Otherwise, userspace becomes SoC-specific, which is a complete disaster.

So, if userspace people want these available, they need to convince us to flip the switch. In the meantime, it should default to off so that if/when we do enable it we can do it in a sane manner for ARM (perhaps via the vectors page).

Quoted 8 lines · Mark Rutland

Several bits in CNTKCTL reset to 0, including PL0VTEN. For platforms using the generic timer which wish to have a fast gettimeofday vDSO implementation, these bits must be set to 1 by the kernel. On other platforms, the bootloader might enable userspace access when we don't want it.

This patch adds arch_counter_set_user_access, which sets the PL0 access permissions to that required by the platform. For arm, this currently

minor nit.
s/arm/ARM

Quoted 7 lines · Santosh Shilimkar

Quoted 6 lines · Mark Rutland

Several bits in CNTKCTL reset to 0, including PL0VTEN. For platforms using the generic timer which wish to have a fast gettimeofday vDSO implementation, these bits must be set to 1 by the kernel. On other platforms, the bootloader might enable userspace access when we don't want it.

Quoted 4 lines

Quoted 2 lines

This patch adds arch_counter_set_user_access, which sets the PL0 access permissions to that required by the platform. For arm, this currently

minor nit.
s/arm/ARM

I think Mark meant arch/arm. Or we could call it AArch32 where we don't use this feature.

Quoted 7 lines · Santosh Shilimkar

Quoted 6 lines · Mark Rutland

Several bits in CNTKCTL reset to 0, including PL0VTEN. For platforms using the generic timer which wish to have a fast gettimeofday vDSO implementation, these bits must be set to 1 by the kernel. On other platforms, the bootloader might enable userspace access when we don't want it.

Quoted 4 lines

Quoted 2 lines

This patch adds arch_counter_set_user_access, which sets the PL0 access permissions to that required by the platform. For arm, this currently

minor nit.
s/arm/ARM

I think Mark meant arch/arm. Or we could call it AArch32 where we don't use this feature.

The core functionality of the arch_timer driver is not directly tied to anything under arch/arm, and can be split out.

This patch factors out the core of the arch_timer driver, so it can be shared with other architectures. A couple of functions are added so that architecture-specific code can interact with the driver without needing to touch its internals.

The ARM_ARCH_TIMER config variable is moved out to drivers/clocksource/Kconfig, existing uses in arch/arm are replaced with USE_ARM_ARCH_TIMER, which selects it.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/Kconfig                     |    3 +-
  arch/arm/include/asm/arch_timer.h    |   19 +--
  arch/arm/kernel/arch_timer.c         |  375 ++--------------------------------
  arch/arm/mach-omap2/Kconfig          |    2 +-
  drivers/clocksource/Kconfig          |    3 +
  drivers/clocksource/Makefile         |    1 +
  drivers/clocksource/arm_arch_timer.c |  374 +++++++++++++++++++++++++++++++++
  include/clocksource/arm_arch_timer.h |   63 ++++++
  8 files changed, 465 insertions(+), 375 deletions(-)

create mode 100644 drivers/clocksource/arm_arch_timer.c create mode 100644 include/clocksource/arm_arch_timer.h

arch/arm/Kconfig +0 -0

index f95ba14..487696a 100644
@@ -1567,9 +1567,10 @@ config HAVE_ARM_SCU
  	help

Quoted 25 lines · Mark Rutland

The core functionality of the arch_timer driver is not directly tied to anything under arch/arm, and can be split out.

This patch factors out the core of the arch_timer driver, so it can be shared with other architectures. A couple of functions are added so that architecture-specific code can interact with the driver without needing to touch its internals.

The ARM_ARCH_TIMER config variable is moved out to drivers/clocksource/Kconfig, existing uses in arch/arm are replaced with USE_ARM_ARCH_TIMER, which selects it.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---
  arch/arm/Kconfig                     |    3 +-
  arch/arm/include/asm/arch_timer.h    |   19 +--
  arch/arm/kernel/arch_timer.c         |  375 ++--------------------------------
  arch/arm/mach-omap2/Kconfig          |    2 +-
  drivers/clocksource/Kconfig          |    3 +
  drivers/clocksource/Makefile         |    1 +
  drivers/clocksource/arm_arch_timer.c |  374 +++++++++++++++++++++++++++++++++
  include/clocksource/arm_arch_timer.h |   63 ++++++
  8 files changed, 465 insertions(+), 375 deletions(-)

Quoted 3 lines

create mode 100644 drivers/clocksource/arm_arch_timer.c create mode 100644 include/clocksource/arm_arch_timer.h

It would have been easy if you have formated the patch with -C option. That will just leave the delta changes only and hiding the file movement related diff.

Quoted 8 lines

arch/arm/Kconfig +0 -0

index f95ba14..487696a 100644
@@ -1567,9 +1567,10 @@ config HAVE_ARM_SCU
  	help
  	  This option enables support for the ARM system coherency unit
 

arch/arm/Kconfig config HAVE_ARM_SCU · line 1576

Quoted 8 lines

arch/arm/Kconfig +2 -1

-config ARM_ARCH_TIMER
+config USE_ARM_ARCH_TIMER
  	bool "Architected timer support"
  	depends on CPU_V7
+	select ARM_ARCH_TIMER
  	help
  	  This option enables support for the ARM architected timer
 

How about HAVE_ARM_ARCH_TIMER in-line with HAVE_ARM_TWD. No strong opinion though.

Regards
Santosh

The arch_timer driver supports a superset of the functionality of the arm_generic driver, and is not tied to a particular arch.

This patch moves arm64 to use the arch_timer driver, gaining additional functionality in doing so, and removes the (now unused) arm_generic driver. Timer-related hooks specific to arm64 are moved into arch/arm64/kernel/time.c.

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---

Signed-off-by Mark Rutland <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Acked-by Marc Zyngier <[email protected]>

---