[PATCH v3 0/2] arch_timers patches to enable KVM support

[PATCH v3 0/2] arch_timers patches to enable KVM support

[Marc Zyngier]

This series is aimed at offering the minimum level of support that KVM
needs when it comes to architected timers.

The first patch changes the driver to use the virtual timer by default
instead of the physical one (access to the later is conditioned by the
hypervisor settings, while the former is always available). This could
be required even if the kernel is not running as a guest.

The second patch exposes a clockcycle counter that can be used by
other subsystems.

Tested on A15 (VE+TC2), both as a host and a KVM guest. Patches
against v3.6-rc3.

* From v2:
- Trick the compiler into generating really nice code, at the expense
  of a certain lack of abstraction
- Don't mandate physical timer non-secure PPI, in order to stay
  compatible with existing and possibly incomplete DTs.
  
Marc Zyngier (2):
  ARM: arch_timers: enable the use of the virtual timer
  ARM: arch_timers: register a time/cycle counter

 arch/arm/include/asm/arch_timer.h |   7 +
 arch/arm/kernel/arch_timer.c      | 362 +++++++++++++++++++++++++++-----------
 2 files changed, 265 insertions(+), 104 deletions(-)

-- 
1.7.11.4

[PATCH v3 1/2] ARM: arch_timers: enable the use of the virtual timer

[Marc Zyngier]

At the moment, the arch_timer driver only uses the physical timer,
which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
which is likely in a virtualized environment. Instead, the virtual
timer is always available.

This patch enables the use of the virtual timer, unless no
interrupt is provided in the DT for it, in which case it falls
back to the physical timer.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kernel/arch_timer.c | 337 ++++++++++++++++++++++++++++++-------------
 1 file changed, 233 insertions(+), 104 deletions(-)

diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index cf25880..3f63b90 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -27,13 +27,23 @@
 #include <asm/sched_clock.h>
 
 static unsigned long arch_timer_rate;
-static int arch_timer_ppi;
-static int arch_timer_ppi2;
+
+enum ppi_nr {
+	PHYS_SECURE_PPI,
+	PHYS_NONSECURE_PPI,
+	VIRT_PPI,
+	HYP_PPI,
+	MAX_TIMER_PPI
+};
+
+static int arch_timer_ppi[MAX_TIMER_PPI];
 
 static struct clock_event_device __percpu **arch_timer_evt;
 
 extern void init_current_timer_delay(unsigned long freq);
 
+static bool arch_timer_use_virtual = true;
+
 /*
  * Architected system timer support.
  */
@@ -43,53 +53,98 @@ extern void init_current_timer_delay(unsigned long freq);
 #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
 
-static void arch_timer_reg_write(int reg, u32 val)
+#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)
 {
-	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_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:
+			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();
 }
 
-static u32 arch_timer_reg_read(int reg)
+static inline u32 arch_timer_reg_read(const int access, const int reg)
 {
-	u32 val;
+	u32 val = 0;
+
+	if (access == ARCH_TIMER_PHYS_ACCESS) {
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
+			break;
+		}
+	}
 
-	switch (reg) {
-	case ARCH_TIMER_REG_CTRL:
-		asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
-		break;
-	case ARCH_TIMER_REG_FREQ:
-		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
-		break;
-	case ARCH_TIMER_REG_TVAL:
-		asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
-		break;
-	default:
-		BUG();
+	if (access == ARCH_TIMER_VIRT_ACCESS) {
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
+			break;
+		case ARCH_TIMER_REG_TVAL:
+			asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val));
+			break;
+		}
 	}
 
 	return val;
 }
 
-static irqreturn_t arch_timer_handler(int irq, void *dev_id)
+static inline cycle_t arch_counter_get_cntpct(void)
 {
-	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
-	unsigned long ctrl;
+	u32 cvall, cvalh;
+
+	asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
+
+	return ((cycle_t) cvalh << 32) | cvall;
+}
+
+static inline cycle_t arch_counter_get_cntvct(void)
+{
+	u32 cvall, cvalh;
+
+	asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
+
+	return ((cycle_t) cvalh << 32) | cvall;
+}
 
-	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
+static irqreturn_t inline timer_handler(const int access,
+					struct clock_event_device *evt)
+{
+	unsigned long ctrl;
+	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
 	if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
 		ctrl |= ARCH_TIMER_CTRL_IT_MASK;
-		arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
+		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
 		evt->event_handler(evt);
 		return IRQ_HANDLED;
 	}
@@ -97,63 +152,100 @@ static irqreturn_t arch_timer_handler(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-static void arch_timer_disable(void)
+static irqreturn_t arch_timer_handler_virt(int irq, void *dev_id)
 {
-	unsigned long ctrl;
+	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
 
-	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
-	ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
-	arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
+	return timer_handler(ARCH_TIMER_VIRT_ACCESS, evt);
 }
 
-static void arch_timer_set_mode(enum clock_event_mode mode,
-				struct clock_event_device *clk)
+static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
 {
+	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
+
+	return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
+}
+
+static inline void timer_set_mode(const int access, int mode)
+{
+	unsigned long ctrl;
 	switch (mode) {
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-		arch_timer_disable();
+		ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
+		ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
+		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
 		break;
 	default:
 		break;
 	}
 }
 
-static int arch_timer_set_next_event(unsigned long evt,
-				     struct clock_event_device *unused)
+static void arch_timer_set_mode_virt(enum clock_event_mode mode,
+				     struct clock_event_device *clk)
 {
-	unsigned long ctrl;
+	timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode);
+}
+
+static void arch_timer_set_mode_phys(enum clock_event_mode mode,
+				     struct clock_event_device *clk)
+{
+	timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode);
+}
 
-	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
+static inline void set_next_event(const int access, unsigned long evt)
+{
+	unsigned long ctrl;
+	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
 	ctrl |= ARCH_TIMER_CTRL_ENABLE;
 	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
+	arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt);
+	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
+}
 
-	arch_timer_reg_write(ARCH_TIMER_REG_TVAL, evt);
-	arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
+static int arch_timer_set_next_event_virt(unsigned long evt,
+					  struct clock_event_device *unused)
+{
+	set_next_event(ARCH_TIMER_VIRT_ACCESS, evt);
+	return 0;
+}
 
+static int arch_timer_set_next_event_phys(unsigned long evt,
+					  struct clock_event_device *unused)
+{
+	set_next_event(ARCH_TIMER_PHYS_ACCESS, evt);
 	return 0;
 }
 
 static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 {
-	/* Be safe... */
-	arch_timer_disable();
-
 	clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
 	clk->name = "arch_sys_timer";
 	clk->rating = 450;
-	clk->set_mode = arch_timer_set_mode;
-	clk->set_next_event = arch_timer_set_next_event;
-	clk->irq = arch_timer_ppi;
+	if (arch_timer_use_virtual) {
+		arch_timer_set_mode_virt(CLOCK_EVT_MODE_SHUTDOWN, NULL);
+		clk->set_mode = arch_timer_set_mode_virt;
+		clk->set_next_event = arch_timer_set_next_event_virt;
+	} else {
+		arch_timer_set_mode_phys(CLOCK_EVT_MODE_SHUTDOWN, NULL);
+		clk->set_mode = arch_timer_set_mode_phys;
+		clk->set_next_event = arch_timer_set_next_event_phys;
+	}
+		
+	clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
 
 	clockevents_config_and_register(clk, arch_timer_rate,
 					0xf, 0x7fffffff);
 
 	*__this_cpu_ptr(arch_timer_evt) = clk;
 
-	enable_percpu_irq(clk->irq, 0);
-	if (arch_timer_ppi2)
-		enable_percpu_irq(arch_timer_ppi2, 0);
+	if (arch_timer_use_virtual)
+		enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0);
+	else {
+		enable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI], 0);
+		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
+			enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
+	}
 
 	return 0;
 }
@@ -173,8 +265,7 @@ static int arch_timer_available(void)
 		return -ENXIO;
 
 	if (arch_timer_rate == 0) {
-		arch_timer_reg_write(ARCH_TIMER_REG_CTRL, 0);
-		freq = arch_timer_reg_read(ARCH_TIMER_REG_FREQ);
+		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (freq));
 
 		/* Check the timer frequency. */
 		if (freq == 0) {
@@ -185,43 +276,42 @@ static int arch_timer_available(void)
 		arch_timer_rate = freq;
 	}
 
-	pr_info_once("Architected local timer running at %lu.%02luMHz.\n",
-		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100);
+	pr_info_once("Architected local timer running at %lu.%02luMHz (%s).\n",
+		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100,
+		     arch_timer_use_virtual ? "virt" : "phys");
 	return 0;
 }
 
-static inline cycle_t arch_counter_get_cntpct(void)
+static u32 notrace arch_counter_get_cntpct32(void)
 {
-	u32 cvall, cvalh;
+	cycle_t cnt = arch_counter_get_cntpct();
 
-	asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
-
-	return ((cycle_t) cvalh << 32) | cvall;
-}
-
-static inline cycle_t arch_counter_get_cntvct(void)
-{
-	u32 cvall, cvalh;
-
-	asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
-
-	return ((cycle_t) cvalh << 32) | cvall;
+	/*
+	 * 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 & (u32)~0);
 }
 
 static u32 notrace arch_counter_get_cntvct32(void)
 {
-	cycle_t cntvct = arch_counter_get_cntvct();
+	cycle_t cnt = arch_counter_get_cntvct();
 
 	/*
 	 * The sched_clock infrastructure only knows about counters
 	 * with@most 32bits. Forget about the upper 24 bits for the
 	 * time being...
 	 */
-	return (u32)(cntvct & (u32)~0);
+	return (u32)(cnt & (u32)~0);
 }
 
 static cycle_t arch_counter_read(struct clocksource *cs)
 {
+	/*
+	 * Always use the physical counter for the clocksource.
+	 * CNTHCTL.PL1PCTEN must be set to 1.
+	 */
 	return arch_counter_get_cntpct();
 }
 
@@ -245,10 +335,16 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 {
 	pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n",
 		 clk->irq, smp_processor_id());
-	disable_percpu_irq(clk->irq);
-	if (arch_timer_ppi2)
-		disable_percpu_irq(arch_timer_ppi2);
-	arch_timer_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
+
+	if (arch_timer_use_virtual) {
+		disable_percpu_irq(arch_timer_ppi[VIRT_PPI]);
+		arch_timer_set_mode_virt(CLOCK_EVT_MODE_UNUSED, clk);
+	} else {
+		disable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI]);
+		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
+			disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
+		arch_timer_set_mode_phys(CLOCK_EVT_MODE_UNUSED, clk);
+	}
 }
 
 static struct local_timer_ops arch_timer_ops __cpuinitdata = {
@@ -261,36 +357,44 @@ static struct clock_event_device arch_timer_global_evt;
 static int __init arch_timer_register(void)
 {
 	int err;
+	int ppi;
 
 	err = arch_timer_available();
 	if (err)
-		return err;
+		goto out;
 
 	arch_timer_evt = alloc_percpu(struct clock_event_device *);
-	if (!arch_timer_evt)
-		return -ENOMEM;
+	if (!arch_timer_evt) {
+		err = -ENOMEM;
+		goto out;
+	}
 
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
 
-	err = request_percpu_irq(arch_timer_ppi, arch_timer_handler,
-				 "arch_timer", arch_timer_evt);
+	if (arch_timer_use_virtual) {
+		ppi = arch_timer_ppi[VIRT_PPI];
+		err = request_percpu_irq(ppi, arch_timer_handler_virt,
+					 "arch_timer", arch_timer_evt);
+	} else {
+		ppi = arch_timer_ppi[PHYS_SECURE_PPI];
+		err = request_percpu_irq(ppi, arch_timer_handler_phys,
+					 "arch_timer", arch_timer_evt);
+		if (!err && arch_timer_ppi[PHYS_NONSECURE_PPI]) {
+			ppi = arch_timer_ppi[PHYS_NONSECURE_PPI];
+			err = request_percpu_irq(ppi, arch_timer_handler_phys,
+						 "arch_timer", arch_timer_evt);
+			if (err)
+				free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
+						arch_timer_evt);
+		}
+	}
+
 	if (err) {
 		pr_err("arch_timer: can't register interrupt %d (%d)\n",
-		       arch_timer_ppi, err);
+		       ppi, err);
 		goto out_free;
 	}
 
-	if (arch_timer_ppi2) {
-		err = request_percpu_irq(arch_timer_ppi2, arch_timer_handler,
-					 "arch_timer", arch_timer_evt);
-		if (err) {
-			pr_err("arch_timer: can't register interrupt %d (%d)\n",
-			       arch_timer_ppi2, err);
-			arch_timer_ppi2 = 0;
-			goto out_free_irq;
-		}
-	}
-
 	err = local_timer_register(&arch_timer_ops);
 	if (err) {
 		/*
@@ -310,13 +414,19 @@ static int __init arch_timer_register(void)
 	return 0;
 
 out_free_irq:
-	free_percpu_irq(arch_timer_ppi, arch_timer_evt);
-	if (arch_timer_ppi2)
-		free_percpu_irq(arch_timer_ppi2, arch_timer_evt);
+	if (arch_timer_use_virtual)
+		free_percpu_irq(arch_timer_ppi[VIRT_PPI], arch_timer_evt);
+	else {
+		free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
+				arch_timer_evt);
+		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
+			free_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI],
+					arch_timer_evt);
+	}
 
 out_free:
 	free_percpu(arch_timer_evt);
-
+out:
 	return err;
 }
 
@@ -329,6 +439,7 @@ int __init arch_timer_of_register(void)
 {
 	struct device_node *np;
 	u32 freq;
+	int i;
 
 	np = of_find_matching_node(NULL, arch_timer_of_match);
 	if (!np) {
@@ -340,22 +451,40 @@ int __init arch_timer_of_register(void)
 	if (!of_property_read_u32(np, "clock-frequency", &freq))
 		arch_timer_rate = freq;
 
-	arch_timer_ppi = irq_of_parse_and_map(np, 0);
-	arch_timer_ppi2 = irq_of_parse_and_map(np, 1);
-	pr_info("arch_timer: found %s irqs %d %d\n",
-		np->name, arch_timer_ppi, arch_timer_ppi2);
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
+		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
+
+	/*
+	 * If no interrupt provided for virtual timer, we'll have to
+	 * stick to the physical timer. It'd better be accessible...
+	 */
+	if (!arch_timer_ppi[VIRT_PPI]) {
+		arch_timer_use_virtual = false;
+
+		if (!arch_timer_ppi[PHYS_SECURE_PPI] ||
+		    !arch_timer_ppi[PHYS_NONSECURE_PPI]) {
+			pr_warn("arch_timer: No interrupt available, giving up\n");
+			return -EINVAL;
+		}
+	}
 
 	return arch_timer_register();
 }
 
 int __init arch_timer_sched_clock_init(void)
 {
+	u32 (*cnt32)(void);
 	int err;
 
 	err = arch_timer_available();
 	if (err)
 		return err;
 
-	setup_sched_clock(arch_counter_get_cntvct32, 32, arch_timer_rate);
+	if (arch_timer_use_virtual)
+		cnt32 = arch_counter_get_cntvct32;
+	else
+		cnt32 = arch_counter_get_cntpct32;
+
+	setup_sched_clock(cnt32, 32, arch_timer_rate);
 	return 0;
 }
-- 
1.7.11.4

[PATCH v3 1/2] ARM: arch_timers: enable the use of the virtual timer

[Cyril Chemparathy]

Hi Marc,

On 8/24/2012 10:52 AM, Marc Zyngier wrote:
> At the moment, the arch_timer driver only uses the physical timer,
> which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
> which is likely in a virtualized environment. Instead, the virtual
> timer is always available.
>
> This patch enables the use of the virtual timer, unless no
> interrupt is provided in the DT for it, in which case it falls
> back to the physical timer.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Looks very nice...  Minor comments below.

> ---
>   arch/arm/kernel/arch_timer.c | 337 ++++++++++++++++++++++++++++++-------------
>   1 file changed, 233 insertions(+), 104 deletions(-)
>
> diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
> index cf25880..3f63b90 100644
> --- a/arch/arm/kernel/arch_timer.c
> +++ b/arch/arm/kernel/arch_timer.c
> @@ -27,13 +27,23 @@
>   #include <asm/sched_clock.h>
>
>   static unsigned long arch_timer_rate;
> -static int arch_timer_ppi;
> -static int arch_timer_ppi2;
> +
> +enum ppi_nr {
> +	PHYS_SECURE_PPI,
> +	PHYS_NONSECURE_PPI,
> +	VIRT_PPI,
> +	HYP_PPI,
> +	MAX_TIMER_PPI
> +};
> +
> +static int arch_timer_ppi[MAX_TIMER_PPI];
>
>   static struct clock_event_device __percpu **arch_timer_evt;
>
>   extern void init_current_timer_delay(unsigned long freq);
>
> +static bool arch_timer_use_virtual = true;
> +
>   /*
>    * Architected system timer support.
>    */
> @@ -43,53 +53,98 @@ extern void init_current_timer_delay(unsigned long freq);
>   #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
>
> -static void arch_timer_reg_write(int reg, u32 val)
> +#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)
>   {
> -	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_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:
> +			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();
>   }
>
> -static u32 arch_timer_reg_read(int reg)
> +static inline u32 arch_timer_reg_read(const int access, const int reg)
>   {
> -	u32 val;
> +	u32 val = 0;
> +
> +	if (access == ARCH_TIMER_PHYS_ACCESS) {
> +		switch (reg) {
> +		case ARCH_TIMER_REG_CTRL:
> +			asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
> +			break;
> +		case ARCH_TIMER_REG_TVAL:
> +			asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
> +			break;
> +		}
> +	}
>
> -	switch (reg) {
> -	case ARCH_TIMER_REG_CTRL:
> -		asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
> -		break;
> -	case ARCH_TIMER_REG_FREQ:
> -		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (val));
> -		break;
> -	case ARCH_TIMER_REG_TVAL:
> -		asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
> -		break;
> -	default:
> -		BUG();
> +	if (access == ARCH_TIMER_VIRT_ACCESS) {
> +		switch (reg) {
> +		case ARCH_TIMER_REG_CTRL:
> +			asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
> +			break;
> +		case ARCH_TIMER_REG_TVAL:
> +			asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val));
> +			break;
> +		}
>   	}
>
>   	return val;
>   }
>
> -static irqreturn_t arch_timer_handler(int irq, void *dev_id)
> +static inline cycle_t arch_counter_get_cntpct(void)
>   {
> -	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
> -	unsigned long ctrl;
> +	u32 cvall, cvalh;
> +
> +	asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
> +
> +	return ((cycle_t) cvalh << 32) | cvall;
> +}
> +
> +static inline cycle_t arch_counter_get_cntvct(void)
> +{
> +	u32 cvall, cvalh;
> +
> +	asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
> +
> +	return ((cycle_t) cvalh << 32) | cvall;
> +}

We should use the Q and R qualifiers to avoid compiler misbehavior:
	u64 cval;
	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cvall));

The compiler generates sad looking code when constructing 64-bit 
quantities with shifts and ORs.  We found this while implementing the 
phys-virt patching for 64-bit phys_addr_t.

Is there value in unifying the physical and virtual counter read 
functions using the access specifier as you've done for the register 
read and write functions?

>
> -	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
> +static irqreturn_t inline timer_handler(const int access,
> +					struct clock_event_device *evt)
> +{
> +	unsigned long ctrl;
> +	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
>   	if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
>   		ctrl |= ARCH_TIMER_CTRL_IT_MASK;
> -		arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
> +		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
>   		evt->event_handler(evt);
>   		return IRQ_HANDLED;
>   	}
> @@ -97,63 +152,100 @@ static irqreturn_t arch_timer_handler(int irq, void *dev_id)
>   	return IRQ_NONE;
>   }
>
> -static void arch_timer_disable(void)
> +static irqreturn_t arch_timer_handler_virt(int irq, void *dev_id)
>   {
> -	unsigned long ctrl;
> +	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
>
> -	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
> -	ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
> -	arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
> +	return timer_handler(ARCH_TIMER_VIRT_ACCESS, evt);
>   }
>
> -static void arch_timer_set_mode(enum clock_event_mode mode,
> -				struct clock_event_device *clk)
> +static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
>   {
> +	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
> +
> +	return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
> +}
> +
> +static inline void timer_set_mode(const int access, int mode)
> +{
> +	unsigned long ctrl;
>   	switch (mode) {
>   	case CLOCK_EVT_MODE_UNUSED:
>   	case CLOCK_EVT_MODE_SHUTDOWN:
> -		arch_timer_disable();
> +		ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
> +		ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
> +		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
>   		break;
>   	default:
>   		break;
>   	}
>   }
>
> -static int arch_timer_set_next_event(unsigned long evt,
> -				     struct clock_event_device *unused)
> +static void arch_timer_set_mode_virt(enum clock_event_mode mode,
> +				     struct clock_event_device *clk)
>   {
> -	unsigned long ctrl;
> +	timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode);
> +}
> +
> +static void arch_timer_set_mode_phys(enum clock_event_mode mode,
> +				     struct clock_event_device *clk)
> +{
> +	timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode);
> +}
>
> -	ctrl = arch_timer_reg_read(ARCH_TIMER_REG_CTRL);
> +static inline void set_next_event(const int access, unsigned long evt)
> +{
> +	unsigned long ctrl;
> +	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
>   	ctrl |= ARCH_TIMER_CTRL_ENABLE;
>   	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
> +	arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt);
> +	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
> +}
>
> -	arch_timer_reg_write(ARCH_TIMER_REG_TVAL, evt);
> -	arch_timer_reg_write(ARCH_TIMER_REG_CTRL, ctrl);
> +static int arch_timer_set_next_event_virt(unsigned long evt,
> +					  struct clock_event_device *unused)
> +{
> +	set_next_event(ARCH_TIMER_VIRT_ACCESS, evt);
> +	return 0;
> +}
>
> +static int arch_timer_set_next_event_phys(unsigned long evt,
> +					  struct clock_event_device *unused)
> +{
> +	set_next_event(ARCH_TIMER_PHYS_ACCESS, evt);
>   	return 0;
>   }
>
>   static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
>   {
> -	/* Be safe... */
> -	arch_timer_disable();
> -
>   	clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
>   	clk->name = "arch_sys_timer";
>   	clk->rating = 450;
> -	clk->set_mode = arch_timer_set_mode;
> -	clk->set_next_event = arch_timer_set_next_event;
> -	clk->irq = arch_timer_ppi;
> +	if (arch_timer_use_virtual) {
> +		arch_timer_set_mode_virt(CLOCK_EVT_MODE_SHUTDOWN, NULL);
> +		clk->set_mode = arch_timer_set_mode_virt;
> +		clk->set_next_event = arch_timer_set_next_event_virt;
> +	} else {
> +		arch_timer_set_mode_phys(CLOCK_EVT_MODE_SHUTDOWN, NULL);
> +		clk->set_mode = arch_timer_set_mode_phys;
> +		clk->set_next_event = arch_timer_set_next_event_phys;
> +	}
> +		
> +	clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
>
>   	clockevents_config_and_register(clk, arch_timer_rate,
>   					0xf, 0x7fffffff);
>
>   	*__this_cpu_ptr(arch_timer_evt) = clk;
>
> -	enable_percpu_irq(clk->irq, 0);
> -	if (arch_timer_ppi2)
> -		enable_percpu_irq(arch_timer_ppi2, 0);
> +	if (arch_timer_use_virtual)
> +		enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0);
> +	else {
> +		enable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI], 0);
> +		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> +			enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
> +	}
>
>   	return 0;
>   }
> @@ -173,8 +265,7 @@ static int arch_timer_available(void)
>   		return -ENXIO;
>
>   	if (arch_timer_rate == 0) {
> -		arch_timer_reg_write(ARCH_TIMER_REG_CTRL, 0);
> -		freq = arch_timer_reg_read(ARCH_TIMER_REG_FREQ);
> +		asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (freq));
>
>   		/* Check the timer frequency. */
>   		if (freq == 0) {
> @@ -185,43 +276,42 @@ static int arch_timer_available(void)
>   		arch_timer_rate = freq;
>   	}
>
> -	pr_info_once("Architected local timer running at %lu.%02luMHz.\n",
> -		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100);
> +	pr_info_once("Architected local timer running at %lu.%02luMHz (%s).\n",
> +		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100,
> +		     arch_timer_use_virtual ? "virt" : "phys");
>   	return 0;
>   }
>
> -static inline cycle_t arch_counter_get_cntpct(void)
> +static u32 notrace arch_counter_get_cntpct32(void)
>   {
> -	u32 cvall, cvalh;
> +	cycle_t cnt = arch_counter_get_cntpct();
>
> -	asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
> -
> -	return ((cycle_t) cvalh << 32) | cvall;
> -}
> -
> -static inline cycle_t arch_counter_get_cntvct(void)
> -{
> -	u32 cvall, cvalh;
> -
> -	asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
> -
> -	return ((cycle_t) cvalh << 32) | cvall;
> +	/*
> +	 * 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 & (u32)~0);

Wouldn't a return (u32)cnt be sufficient here?

>   }
>
>   static u32 notrace arch_counter_get_cntvct32(void)
>   {
> -	cycle_t cntvct = arch_counter_get_cntvct();
> +	cycle_t cnt = arch_counter_get_cntvct();
>
>   	/*
>   	 * The sched_clock infrastructure only knows about counters
>   	 * with at most 32bits. Forget about the upper 24 bits for the
>   	 * time being...
>   	 */
> -	return (u32)(cntvct & (u32)~0);
> +	return (u32)(cnt & (u32)~0);
>   }
>
>   static cycle_t arch_counter_read(struct clocksource *cs)
>   {
> +	/*
> +	 * Always use the physical counter for the clocksource.
> +	 * CNTHCTL.PL1PCTEN must be set to 1.
> +	 */
>   	return arch_counter_get_cntpct();
>   }
>
> @@ -245,10 +335,16 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
>   {
>   	pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n",
>   		 clk->irq, smp_processor_id());
> -	disable_percpu_irq(clk->irq);
> -	if (arch_timer_ppi2)
> -		disable_percpu_irq(arch_timer_ppi2);
> -	arch_timer_set_mode(CLOCK_EVT_MODE_UNUSED, clk);
> +
> +	if (arch_timer_use_virtual) {
> +		disable_percpu_irq(arch_timer_ppi[VIRT_PPI]);
> +		arch_timer_set_mode_virt(CLOCK_EVT_MODE_UNUSED, clk);
> +	} else {
> +		disable_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI]);
> +		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> +			disable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> +		arch_timer_set_mode_phys(CLOCK_EVT_MODE_UNUSED, clk);
> +	}
>   }
>
>   static struct local_timer_ops arch_timer_ops __cpuinitdata = {
> @@ -261,36 +357,44 @@ static struct clock_event_device arch_timer_global_evt;
>   static int __init arch_timer_register(void)
>   {
>   	int err;
> +	int ppi;
>
>   	err = arch_timer_available();
>   	if (err)
> -		return err;
> +		goto out;
>
>   	arch_timer_evt = alloc_percpu(struct clock_event_device *);
> -	if (!arch_timer_evt)
> -		return -ENOMEM;
> +	if (!arch_timer_evt) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
>
>   	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
>
> -	err = request_percpu_irq(arch_timer_ppi, arch_timer_handler,
> -				 "arch_timer", arch_timer_evt);
> +	if (arch_timer_use_virtual) {
> +		ppi = arch_timer_ppi[VIRT_PPI];
> +		err = request_percpu_irq(ppi, arch_timer_handler_virt,
> +					 "arch_timer", arch_timer_evt);
> +	} else {
> +		ppi = arch_timer_ppi[PHYS_SECURE_PPI];
> +		err = request_percpu_irq(ppi, arch_timer_handler_phys,
> +					 "arch_timer", arch_timer_evt);
> +		if (!err && arch_timer_ppi[PHYS_NONSECURE_PPI]) {
> +			ppi = arch_timer_ppi[PHYS_NONSECURE_PPI];
> +			err = request_percpu_irq(ppi, arch_timer_handler_phys,
> +						 "arch_timer", arch_timer_evt);
> +			if (err)
> +				free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
> +						arch_timer_evt);
> +		}
> +	}
> +
>   	if (err) {
>   		pr_err("arch_timer: can't register interrupt %d (%d)\n",
> -		       arch_timer_ppi, err);
> +		       ppi, err);
>   		goto out_free;
>   	}
>
> -	if (arch_timer_ppi2) {
> -		err = request_percpu_irq(arch_timer_ppi2, arch_timer_handler,
> -					 "arch_timer", arch_timer_evt);
> -		if (err) {
> -			pr_err("arch_timer: can't register interrupt %d (%d)\n",
> -			       arch_timer_ppi2, err);
> -			arch_timer_ppi2 = 0;
> -			goto out_free_irq;
> -		}
> -	}
> -
>   	err = local_timer_register(&arch_timer_ops);
>   	if (err) {
>   		/*
> @@ -310,13 +414,19 @@ static int __init arch_timer_register(void)
>   	return 0;
>
>   out_free_irq:
> -	free_percpu_irq(arch_timer_ppi, arch_timer_evt);
> -	if (arch_timer_ppi2)
> -		free_percpu_irq(arch_timer_ppi2, arch_timer_evt);
> +	if (arch_timer_use_virtual)
> +		free_percpu_irq(arch_timer_ppi[VIRT_PPI], arch_timer_evt);
> +	else {
> +		free_percpu_irq(arch_timer_ppi[PHYS_SECURE_PPI],
> +				arch_timer_evt);
> +		if (arch_timer_ppi[PHYS_NONSECURE_PPI])
> +			free_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI],
> +					arch_timer_evt);
> +	}
>
>   out_free:
>   	free_percpu(arch_timer_evt);
> -
> +out:
>   	return err;
>   }
>
> @@ -329,6 +439,7 @@ int __init arch_timer_of_register(void)
>   {
>   	struct device_node *np;
>   	u32 freq;
> +	int i;
>
>   	np = of_find_matching_node(NULL, arch_timer_of_match);
>   	if (!np) {
> @@ -340,22 +451,40 @@ int __init arch_timer_of_register(void)
>   	if (!of_property_read_u32(np, "clock-frequency", &freq))
>   		arch_timer_rate = freq;
>
> -	arch_timer_ppi = irq_of_parse_and_map(np, 0);
> -	arch_timer_ppi2 = irq_of_parse_and_map(np, 1);
> -	pr_info("arch_timer: found %s irqs %d %d\n",
> -		np->name, arch_timer_ppi, arch_timer_ppi2);
> +	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
> +		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
> +
> +	/*
> +	 * If no interrupt provided for virtual timer, we'll have to
> +	 * stick to the physical timer. It'd better be accessible...
> +	 */
> +	if (!arch_timer_ppi[VIRT_PPI]) {
> +		arch_timer_use_virtual = false;
> +
> +		if (!arch_timer_ppi[PHYS_SECURE_PPI] ||
> +		    !arch_timer_ppi[PHYS_NONSECURE_PPI]) {
> +			pr_warn("arch_timer: No interrupt available, giving up\n");
> +			return -EINVAL;
> +		}
> +	}
>
>   	return arch_timer_register();
>   }
>
>   int __init arch_timer_sched_clock_init(void)
>   {
> +	u32 (*cnt32)(void);
>   	int err;
>
>   	err = arch_timer_available();
>   	if (err)
>   		return err;
>
> -	setup_sched_clock(arch_counter_get_cntvct32, 32, arch_timer_rate);
> +	if (arch_timer_use_virtual)
> +		cnt32 = arch_counter_get_cntvct32;
> +	else
> +		cnt32 = arch_counter_get_cntpct32;
> +
> +	setup_sched_clock(cnt32, 32, arch_timer_rate);
>   	return 0;
>   }
>

-- 
Thanks
- Cyril

[PATCH v3 1/2] ARM: arch_timers: enable the use of the virtual timer

[Marc Zyngier]

Hi Cyril,

On 30/08/12 19:28, Cyril Chemparathy wrote:
> On 8/24/2012 10:52 AM, Marc Zyngier wrote:
>> At the moment, the arch_timer driver only uses the physical timer,
>> which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
>> which is likely in a virtualized environment. Instead, the virtual
>> timer is always available.
>>
>> This patch enables the use of the virtual timer, unless no
>> interrupt is provided in the DT for it, in which case it falls
>> back to the physical timer.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Looks very nice...  Minor comments below.
> 

[...]

>>
>> -static irqreturn_t arch_timer_handler(int irq, void *dev_id)
>> +static inline cycle_t arch_counter_get_cntpct(void)
>>   {
>> -     struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
>> -     unsigned long ctrl;
>> +     u32 cvall, cvalh;
>> +
>> +     asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
>> +
>> +     return ((cycle_t) cvalh << 32) | cvall;
>> +}
>> +
>> +static inline cycle_t arch_counter_get_cntvct(void)
>> +{
>> +     u32 cvall, cvalh;
>> +
>> +     asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
>> +
>> +     return ((cycle_t) cvalh << 32) | cvall;
>> +}
> 
> We should use the Q and R qualifiers to avoid compiler misbehavior:
>         u64 cval;
>         asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cvall));
> 
> The compiler generates sad looking code when constructing 64-bit
> quantities with shifts and ORs.  We found this while implementing the
> phys-virt patching for 64-bit phys_addr_t.

Very good point. Looks a lot nicer.

> Is there value in unifying the physical and virtual counter read
> functions using the access specifier as you've done for the register
> read and write functions?

Not a bad idea. At least it makes the access look almost uniform.

>>
>> -static inline cycle_t arch_counter_get_cntpct(void)
>> +static u32 notrace arch_counter_get_cntpct32(void)
>>   {
>> -     u32 cvall, cvalh;
>> +     cycle_t cnt = arch_counter_get_cntpct();
>>
>> -     asm volatile("mrrc p15, 0, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
>> -
>> -     return ((cycle_t) cvalh << 32) | cvall;
>> -}
>> -
>> -static inline cycle_t arch_counter_get_cntvct(void)
>> -{
>> -     u32 cvall, cvalh;
>> -
>> -     asm volatile("mrrc p15, 1, %0, %1, c14" : "=r" (cvall), "=r" (cvalh));
>> -
>> -     return ((cycle_t) cvalh << 32) | cvall;
>> +     /*
>> +      * 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 & (u32)~0);
> 
> Wouldn't a return (u32)cnt be sufficient here?

Yup, fixed.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH v3 1/2] ARM: arch_timers: enable the use of the virtual timer

[Stephen Boyd]

On 8/24/2012 7:52 AM, Marc Zyngier wrote:
> At the moment, the arch_timer driver only uses the physical timer,
> which can cause problem if PL2 hasn't enabled PL1 access in CNTHCTL,
> which is likely in a virtualized environment. Instead, the virtual
> timer is always available.
>
> This patch enables the use of the virtual timer, unless no
> interrupt is provided in the DT for it, in which case it falls
> back to the physical timer.

Is this a good candidate for static keys? It looks like we make the
decision fairly early and then we're done and have a bunch of if
statements. Maybe its premature optimization though...

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v3 2/2] ARM: arch_timers: register a time/cycle counter

[Marc Zyngier]

Some subsystems (KVM for example) need access to a cycle counter.
In the KVM case, this is used to measure the time delta between
host and guest in order to accurately generate timer events for
the guest.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/arch_timer.h |  7 +++++++
 arch/arm/kernel/arch_timer.c      | 25 +++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 62e7547..ad9b155 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -2,11 +2,13 @@
 #define __ASMARM_ARCH_TIMER_H
 
 #include <asm/errno.h>
+#include <linux/clocksource.h>
 
 #ifdef CONFIG_ARM_ARCH_TIMER
 #define ARCH_HAS_READ_CURRENT_TIMER
 int arch_timer_of_register(void);
 int arch_timer_sched_clock_init(void);
+struct timecounter *arch_timer_get_timecounter(void);
 #else
 static inline int arch_timer_of_register(void)
 {
@@ -17,6 +19,11 @@ static inline int arch_timer_sched_clock_init(void)
 {
 	return -ENXIO;
 }
+
+static inline struct timecounter *arch_timer_get_timecounter(void)
+{
+	return NULL;
+}
 #endif
 
 #endif
diff --git a/arch/arm/kernel/arch_timer.c b/arch/arm/kernel/arch_timer.c
index 3f63b90..f999d11 100644
--- a/arch/arm/kernel/arch_timer.c
+++ b/arch/arm/kernel/arch_timer.c
@@ -323,6 +323,15 @@ int read_current_timer(unsigned long *timer_val)
 	return 0;
 }
 
+static cycle_t arch_counter_read_cc(const struct cyclecounter *cc)
+{
+	/*
+	 * Always use the physical counter for the clocksource.
+	 * CNTHCTL.PL1PCTEN must be set to 1.
+	 */
+	return arch_counter_get_cntpct();
+}
+
 static struct clocksource clocksource_counter = {
 	.name	= "arch_sys_counter",
 	.rating	= 400,
@@ -331,6 +340,18 @@ static struct clocksource clocksource_counter = {
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
+static struct cyclecounter cyclecounter = {
+	.read	= arch_counter_read_cc,
+	.mask	= CLOCKSOURCE_MASK(56),
+};
+
+static struct timecounter timecounter;
+
+struct timecounter *arch_timer_get_timecounter(void)
+{
+	return &timecounter;
+}
+
 static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 {
 	pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n",
@@ -370,6 +391,10 @@ static int __init arch_timer_register(void)
 	}
 
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
+	cyclecounter.mult = clocksource_counter.mult;
+	cyclecounter.shift = clocksource_counter.shift;
+	timecounter_init(&timecounter, &cyclecounter,
+			 arch_counter_get_cntpct());
 
 	if (arch_timer_use_virtual) {
 		ppi = arch_timer_ppi[VIRT_PPI];
-- 
1.7.11.4