Skip to contentSkip to conversation

Unify arm_generic and arch_timer drivers

PATCH Superseded Mark Rutland 3 participants 17 messages Nov 30, 2012 – Dec 18, 2012

Subsystems (7)

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Mark Rutland

author maintainer
#0 lore raw

cover Unify arm_generic and arch_timer drivers

Currently we have two drivers for the ARM generic / architected timer, arch_timer in arch/arm, and arm_generic in drivers/clocksource. This is an unnecessary duplication of code, and will only lead to maintenance headaches later.

This patch series splits the generic portion of the arch_timer out to drivers/clocksource, and ports the arm64 code to use it. The now unused arm_generic driver is removed in the process.

The series applies to v3.7-rc7, and has been tested on a TC2 test chip. Aside from a slight conflict in arch/arm64/Kconfig, it also applies to Catalin's soc-armv8-model branch [1], the result of which has been tested on an AArch64 model.

Obviously I'm not aiming for the merge window, but I'm posting now in the hope this can get some early feedback.

Thanks,
Mark

[1] git://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/linux-aarch64.git soc-armv8-model

Mark Rutland (10):
arm: arch_timer: remove redundant available check
arm: arch_timer: use u64/u32 for register data
arm: arch_timer: standardise counter reading
arm: arch_timer: split cntfrq accessor
arm: arch_timer: factor out register accessors
arm: arch_timer: divorce from local_timer api
arm: arch_timer: add arch_counter_enable_user_access
arm: arch_timer: move core to drivers/clocksource
arm64: move from arm_generic to arm_arch_timer
Documentation: Add ARMv8 to arch_timer devicetree

 .../devicetree/bindings/arm/arch_timer.txt         |    7 +-
 arch/arm/Kconfig                                   |    3 +-
 arch/arm/include/asm/arch_timer.h                  |  107 ++++-
 arch/arm/kernel/arch_timer.c                       |  504 +-------------------
 arch/arm/mach-omap2/Kconfig                        |    2 +-
 arch/arm64/Kconfig                                 |    1 +
 arch/arm64/include/asm/arch_timer.h                |  133 +++++
 arch/arm64/include/asm/arm_generic.h               |  100 ----
 arch/arm64/kernel/time.c                           |   29 +-
 drivers/clocksource/Kconfig                        |    6 +-
 drivers/clocksource/Makefile                       |    2 +-
 drivers/clocksource/arm_arch_timer.c               |  375 +++++++++++++++
 drivers/clocksource/arm_generic.c                  |  232 ---------
 include/clocksource/arm_arch_timer.h               |   63 +++
 include/clocksource/arm_generic.h                  |   21 -
 15 files changed, 730 insertions(+), 855 deletions(-)
 create mode 100644 arch/arm64/include/asm/arch_timer.h
 delete mode 100644 arch/arm64/include/asm/arm_generic.h
 create mode 100644 drivers/clocksource/arm_arch_timer.c
 delete mode 100644 drivers/clocksource/arm_generic.c
 create mode 100644 include/clocksource/arm_arch_timer.h
 delete mode 100644 include/clocksource/arm_generic.h
author maintainer
#1 lore raw

PATCH 1/10 arm: arch_timer: remove redundant available check

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.

---
 arch/arm/kernel/arch_timer.c |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)
arch/arm/kernel/arch_timer.c +0 -12
index c8ef207..a4d3b5c 100644
@@ -20,11 +20,9 @@
 #include <linux/of_irq.h>
 #include <linux/io.h>
 
-#include <asm/cputype.h>
 #include <asm/delay.h>
 #include <asm/localtimer.h>
 #include <asm/arch_timer.h>
-#include <asm/system_info.h>
 #include <asm/sched_clock.h>
 
 static unsigned long arch_timer_rate;
@@ -259,20 +257,10 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 	return 0;
 }
 
-/* Is the optional system timer available? */
-static int local_timer_is_architected(void)
-{
-	return (cpu_architecture() >= CPU_ARCH_ARMv7) &&
-	       ((read_cpuid_ext(CPUID_EXT_PFR1) >> 16) & 0xf) == 1;
-}
-
 static int arch_timer_available(void)
 {
 	unsigned long freq;
 
-	if (!local_timer_is_architected())
-		return -ENXIO;
-
 	if (arch_timer_rate == 0) {
 		freq = arch_timer_reg_read(ARCH_TIMER_PHYS_ACCESS,
 					   ARCH_TIMER_REG_FREQ);
Signature · 1 line
1.7.0.4
author maintainer
#2 lore raw

PATCH 2/10 arm: arch_timer: use u64/u32 for register data

In reply to Mark Rutland

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.

---
 arch/arm/kernel/arch_timer.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)
arch/arm/kernel/arch_timer.c +11 -19
index a4d3b5c..1d11f2f 100644
@@ -25,7 +25,7 @@
 #include <asm/arch_timer.h>
 #include <asm/sched_clock.h>
 
-static unsigned long arch_timer_rate;
+static u32 arch_timer_rate;
 
 enum ppi_nr {
 	PHYS_SECURE_PPI,
@@ -121,27 +121,18 @@ static inline u32 arch_timer_reg_read(const int access, const int reg)
 	return val;
 }
 
-static inline cycle_t arch_timer_counter_read(const int access)
+static inline u64 arch_counter_get_cntpct(void)
 {
-	cycle_t cval = 0;
-
-	if (access == ARCH_TIMER_PHYS_ACCESS)
-		asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
-
-	if (access == ARCH_TIMER_VIRT_ACCESS)
-		asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
-
+	u64 cval;
+	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
 	return cval;
 }
 
-static inline cycle_t arch_counter_get_cntpct(void)
-{
-	return arch_timer_counter_read(ARCH_TIMER_PHYS_ACCESS);
-}
-
-static inline cycle_t arch_counter_get_cntvct(void)
+static inline u64 arch_counter_get_cntvct(void)
 {
-	return arch_timer_counter_read(ARCH_TIMER_VIRT_ACCESS);
+	u64 cval;
+	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
 }
 
 static irqreturn_t inline timer_handler(const int access,
@@ -259,7 +250,7 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 
 static int arch_timer_available(void)
 {
-	unsigned long freq;
+	u32 freq;
 
 	if (arch_timer_rate == 0) {
 		freq = arch_timer_reg_read(ARCH_TIMER_PHYS_ACCESS,
@@ -275,7 +266,8 @@ static int arch_timer_available(void)
 	}
 
 	pr_info_once("Architected local timer running at %lu.%02luMHz (%s).\n",
-		     arch_timer_rate / 1000000, (arch_timer_rate / 10000) % 100,
+		     (unsigned long)arch_timer_rate / 1000000,
+		     (unsigned long)(arch_timer_rate / 10000) % 100,
 		     arch_timer_use_virtual ? "virt" : "phys");
 	return 0;
 }
Signature · 1 line
1.7.0.4
author maintainer
#3 lore raw

PATCH 3/10 arm: arch_timer: standardise counter reading

In reply to 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.

---
 arch/arm/kernel/arch_timer.c |   48 ++++++++++-------------------------------
 1 files changed, 12 insertions(+), 36 deletions(-)
arch/arm/kernel/arch_timer.c +12 -36
index 1d11f2f..a498ab9 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();
 }
 
-static u32 notrace arch_counter_get_cntvct32(void)
+static u32 arch_timer_read_counter32(void)
 {
-	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)cnt;
+	return arch_timer_read_counter();
 }
 
 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();
+	return arch_timer_read_counter();
 }
 
 static unsigned long arch_timer_read_current_timer(void)
 {
-	return arch_counter_get_cntpct();
+	return arch_timer_read_counter();
 }
 
 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();
+	return arch_timer_read_counter();
 }
 
 static struct clocksource clocksource_counter = {
@@ -487,18 +468,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;
 
-	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;
 }
Signature · 1 line
1.7.0.4
author maintainer
#4 lore raw

PATCH 4/10 arm: arch_timer: split cntfrq accessor

In reply to 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.

---
 arch/arm/kernel/arch_timer.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)
arch/arm/kernel/arch_timer.c +9 -8
index a498ab9..961f046 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;
 
 	if (arch_timer_rate == 0) {
-		freq = arch_timer_reg_read(ARCH_TIMER_PHYS_ACCESS,
-					   ARCH_TIMER_REG_FREQ);
-
+		freq = arch_timer_get_cntfrq();
 		/* Check the timer frequency. */
 		if (freq == 0) {
 			pr_warn("Architected timer frequency not available\n");
Signature · 1 line
1.7.0.4
author maintainer
#5 lore raw

PATCH 5/10 arm: arch_timer: factor out register accessors

In reply to 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.

---
 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 +101 -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:
+			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 inline u32 arch_timer_reg_read(const int access, const int reg)
+{
+	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;
+		}
+	}
+
+	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 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;
+
+	isb();
+	asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
+}
+
+static inline u64 arch_counter_get_cntvct(void)
+{
+	u64 cval;
+
+	isb();
+	asm volatile("mrrc p15, 1, %Q0, %R0, c14" : "=r" (cval));
+	return cval;
+}
+
+
 #else
 static inline int arch_timer_of_register(void)
 {
arch/arm/kernel/arch_timer.c +0 -92

98 lines not on this page · raw message

Signature · 1 line
1.7.0.4
author maintainer
#6 lore raw

PATCH 6/10 arm: arch_timer: divorce from local_timer api

In reply to 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.

---
 arch/arm/kernel/arch_timer.c |   54 ++++++++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 23 deletions(-)
arch/arm/kernel/arch_timer.c +31 -23
index fda8382..10e1657 100644
@@ -21,7 +21,6 @@
 #include <linux/io.h>
 
 #include <asm/delay.h>
-#include <asm/localtimer.h>
 #include <asm/arch_timer.h>
 #include <asm/sched_clock.h>
 
@@ -37,7 +36,7 @@ enum ppi_nr {
 
 static int arch_timer_ppi[MAX_TIMER_PPI];
 
-static struct clock_event_device __percpu **arch_timer_evt;
+static struct clock_event_device __percpu *arch_timer_evt;
 static struct delay_timer arch_delay_timer;
 
 static bool arch_timer_use_virtual = true;
@@ -63,14 +62,14 @@ static irqreturn_t inline timer_handler(const int access,
 
 static irqreturn_t arch_timer_handler_virt(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
+	struct clock_event_device *evt = dev_id;
 
 	return timer_handler(ARCH_TIMER_VIRT_ACCESS, evt);
 }
 
 static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
 {
-	struct clock_event_device *evt = *(struct clock_event_device **)dev_id;
+	struct clock_event_device *evt = dev_id;
 
 	return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
 }
@@ -141,13 +140,13 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
 		clk->set_next_event = arch_timer_set_next_event_phys;
 	}
 
+	clk->cpumask = cpumask_of(smp_processor_id());
+
 	clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, NULL);
 
 	clockevents_config_and_register(clk, arch_timer_rate,
 					0xf, 0x7fffffff);
 
-	*__this_cpu_ptr(arch_timer_evt) = clk;
-
 	if (arch_timer_use_virtual)
 		enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0);
 	else {
@@ -245,12 +244,28 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 	clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk);
 }
 
-static struct local_timer_ops arch_timer_ops __cpuinitdata = {
-	.setup	= arch_timer_setup,
-	.stop	= arch_timer_stop,
-};
+static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self,
+					   unsigned long action, void *hcpu)
+{
+	struct clock_event_device *evt = this_cpu_ptr(arch_timer_evt);
+
+	switch (action) {
+	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
+		arch_timer_setup(evt);
+		break;
+	case CPU_DYING:
+	case CPU_DYING_FROZEN:
+		arch_timer_stop(evt);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
 
-static struct clock_event_device arch_timer_global_evt;
+static struct notifier_block __cpuinitdata arch_timer_cpu_nb = {
+	.notifier_call = arch_timer_cpu_notify,
+};
 
 static int __init arch_timer_register(void)
 {
@@ -261,7 +276,7 @@ static int __init arch_timer_register(void)
 	if (err)
 		goto out;
 
-	arch_timer_evt = alloc_percpu(struct clock_event_device *);
+	arch_timer_evt = alloc_percpu(struct clock_event_device);
 	if (!arch_timer_evt) {
 		err = -ENOMEM;
 		goto out;
@@ -297,20 +312,13 @@ static int __init arch_timer_register(void)
 		goto out_free;
 	}
 
-	err = local_timer_register(&arch_timer_ops);
-	if (err) {
-		/*
-		 * We couldn't register as a local timer (could be
-		 * because we're on a UP platform, or because some
-		 * other local timer is already present...). Try as a
-		 * global timer instead.
-		 */
-		arch_timer_global_evt.cpumask = cpumask_of(0);
-		err = arch_timer_setup(&arch_timer_global_evt);
-	}
+	err = register_cpu_notifier(&arch_timer_cpu_nb);
 	if (err)
 		goto out_free_irq;
 
+	/* Immediately configure the timer on the boot CPU */
+	arch_timer_setup(this_cpu_ptr(arch_timer_evt));
+
 	/* Use the architected timer for the delay loop. */
 	arch_delay_timer.read_current_timer = &arch_timer_read_current_timer;
 	arch_delay_timer.freq = arch_timer_rate;
Signature · 1 line
1.7.0.4
#7 lore raw

PATCH 6/10 arm: arch_timer: divorce from local_timer api

Quoted 8 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.

How does ipi_timer() work after this change? Don't we need it because of FEAT_C3_STOP?

Quoted 5 lines

---

[snip]

Quoted 13 lines
arch/arm/kernel/arch_timer.c +5 -4
@@ -245,12 +244,28 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 	clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk);
 }
 
-static struct local_timer_ops arch_timer_ops __cpuinitdata = {
-	.setup	= arch_timer_setup,
-	.stop	= arch_timer_stop,
-};
+static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self,
+					   unsigned long action, void *hcpu)
+{
+	struct clock_event_device *evt = this_cpu_ptr(arch_timer_evt);
+

arch/arm/kernel/arch_timer.c static void __cpuinit arch_timer_stop(struct clock_event_device *clk) · line 261

Quoted 10 lines
arch/arm/kernel/arch_timer.c +10 -0
+	switch (action) {
+	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
+		arch_timer_setup(evt);
+		break;
+	case CPU_DYING:
+	case CPU_DYING_FROZEN:
+		arch_timer_stop(evt);
+		break;
+	}

You can save 2 lines with a switch (action & ~CPU_TASKS_FROZEN) here.

arch/arm/kernel/arch_timer.c static void __cpuinit arch_timer_stop(struct clock_event_device *clk) · line 266

Quoted 6 lines
arch/arm/kernel/arch_timer.c +4 -1
+
+	return NOTIFY_OK;
+}
 
-static struct clock_event_device arch_timer_global_evt;
+static struct notifier_block __cpuinitdata arch_timer_cpu_nb = {

__cpuinitdata goes last before the equals sign.

Quoted 6 lines
arch/arm/kernel/arch_timer.c +2 -0
+	.notifier_call = arch_timer_cpu_notify,
+};
 
 static int __init arch_timer_register(void)
 {
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
author maintainer
#8 lore raw

PATCH 6/10 arm: arch_timer: divorce from local_timer api

Quoted 7 lines · Stephen Boyd
Quoted 6 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.

Quoted 5 lines
Quoted 2 lines

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

How does ipi_timer() work after this change? Don't we need it because of FEAT_C3_STOP?

The unfortunate answer is it doesn't, and we'll need broadcast for any systems where the timers turn off in low power states.

I'll take a look at decoupling the broadcast mechanism from the drivers, it's not really a property of the clock hardware and it would be nice to split it.

Quoted 23 lines
Quoted 5 lines

---

[snip]

Quoted 17 lines
arch/arm/kernel/arch_timer.c +9 -4
@@ -245,12 +244,28 @@ static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
 	clk->set_mode(CLOCK_EVT_MODE_UNUSED, clk);
 }
 
-static struct local_timer_ops arch_timer_ops __cpuinitdata = {
-	.setup	= arch_timer_setup,
-	.stop	= arch_timer_stop,
-};
+static int __cpuinit arch_timer_cpu_notify(struct notifier_block *self,
+					   unsigned long action, void *hcpu)
+{
+	struct clock_event_device *evt = this_cpu_ptr(arch_timer_evt);
+
+	switch (action) {
+	case CPU_STARTING:
+	case CPU_STARTING_FROZEN:
+		arch_timer_setup(evt);
Quoted 8 lines

arch/arm/kernel/arch_timer.c static void __cpuinit arch_timer_stop(struct clock_event_device *clk) · line 261

Quoted 6 lines
arch/arm/kernel/arch_timer.c +6 -0
+		break;
+	case CPU_DYING:
+	case CPU_DYING_FROZEN:
+		arch_timer_stop(evt);
+		break;
+	}

You can save 2 lines with a switch (action & ~CPU_TASKS_FROZEN) here.

Sure. I'll fold that in.

Quoted 8 lines

arch/arm/kernel/arch_timer.c static void __cpuinit arch_timer_stop(struct clock_event_device *clk) · line 266

Quoted 6 lines
arch/arm/kernel/arch_timer.c +4 -1
+
+	return NOTIFY_OK;
+}
 
-static struct clock_event_device arch_timer_global_evt;
+static struct notifier_block __cpuinitdata arch_timer_cpu_nb = {

__cpuinitdata goes last before the equals sign.

I'll fix that up.

Quoted 12 lines
Quoted 6 lines
arch/arm/kernel/arch_timer.c +2 -0
+	.notifier_call = arch_timer_cpu_notify,
+};
 
 static int __init arch_timer_register(void)
 {
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Thanks,
Mark.

author maintainer
#9 lore raw

PATCH 6/10 arm: arch_timer: divorce from local_timer api

Stephen,

Quoted 11 lines · Mark Rutland
Quoted 10 lines · Stephen Boyd
Quoted 8 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 8 lines
Quoted 2 lines

How does ipi_timer() work after this change? Don't we need it because of FEAT_C3_STOP?

The unfortunate answer is it doesn't, and we'll need broadcast for any systems where the timers turn off in low power states.

I'll take a look at decoupling the broadcast mechanism from the drivers, it's not really a property of the clock hardware and it would be nice to split it.

I've had a go at splitting the broadcast mechanism in another series:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/137929.html

I'm evidently not proficient with git send-email -- I'd intended for you to be on Cc.

Thanks,
Mark

author maintainer
#10 lore raw

PATCH 7/10 arm: arch_timer: add arch_counter_enable_user_access

In reply to 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.

Add an arch_counter_enable_user_access function for this. As we don't (currently) use the counter from PL0 on arm, the implementation is an empty stub.

---
 arch/arm/include/asm/arch_timer.h |    7 +++++++
 arch/arm/kernel/arch_timer.c      |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)
arch/arm/include/asm/arch_timer.h +7 -0
index 701f2b7..c50e415 100644
@@ -108,6 +108,13 @@ static inline u64 arch_counter_get_cntvct(void)
 	return cval;
 }
 
+static inline void __cpuinit arch_counter_enable_user_access(void)
+{
+	/*
+	 * Do nothing - the counter is never mapped to userspace threads,
+	 * so we don't care whether access is enabled or not.
+	 */
+}
 
 #else
 static inline int arch_timer_of_register(void)
arch/arm/kernel/arch_timer.c +2 -0
index 10e1657..28b29f0 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_counter_enable_user_access();
+
 	return 0;
 }
 
Signature · 1 line
1.7.0.4
#11 lore raw

PATCH 7/10 arm: arch_timer: add arch_counter_enable_user_access

Quoted 21 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.

Add an arch_counter_enable_user_access function for this. As we don't (currently) use the counter from PL0 on arm, the implementation is an empty stub.

---
 arch/arm/include/asm/arch_timer.h |    7 +++++++
 arch/arm/kernel/arch_timer.c      |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)
arch/arm/include/asm/arch_timer.h +0 -0
index 701f2b7..c50e415 100644

arch/arm/include/asm/arch_timer.h static inline u64 arch_counter_get_cntvct(void) · line 117

Quoted 10 lines
arch/arm/include/asm/arch_timer.h +7 -0
@@ -108,6 +108,13 @@ static inline u64 arch_counter_get_cntvct(void)
 	return cval;
 }
 
+static inline void __cpuinit arch_counter_enable_user_access(void)
+{
+	/*
+	 * Do nothing - the counter is never mapped to userspace threads,
+	 * so we don't care whether access is enabled or not.
+	 */
+}

Might be worth explicitly *disabling* userspace access for the time being then (and rename this function to arch_counter_set_user_access?).

Will

author maintainer
#12 lore raw

PATCH 7/10 arm: arch_timer: add arch_counter_enable_user_access

Quoted 25 lines · Will Deacon
Quoted 24 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.

Add an arch_counter_enable_user_access function for this. As we don't (currently) use the counter from PL0 on arm, the implementation is an empty stub.

---
 arch/arm/include/asm/arch_timer.h |    7 +++++++
 arch/arm/kernel/arch_timer.c      |    2 ++
 2 files changed, 9 insertions(+), 0 deletions(-)
arch/arm/include/asm/arch_timer.h +0 -0
index 701f2b7..c50e415 100644
@@ -108,6 +108,13 @@ static inline u64 arch_counter_get_cntvct(void)
 	return cval;
 }
 
Quoted 10 lines

arch/arm/include/asm/arch_timer.h static inline u64 arch_counter_get_cntvct(void) · line 117

Quoted 7 lines
arch/arm/include/asm/arch_timer.h +7 -0
+static inline void __cpuinit arch_counter_enable_user_access(void)
+{
+	/*
+	 * Do nothing - the counter is never mapped to userspace threads,
+	 * so we don't care whether access is enabled or not.
+	 */
+}

Might be worth explicitly *disabling* userspace access for the time being then (and rename this function to arch_counter_set_user_access?).

Agreed on the name. The current form was an attempt to minimise churn, but given half the code gets moved later anyway, there's no real benefit.

I'm happy with whatever actually gets written to PL0{V,P}{C,}TEN.

Thanks,
Mark

author maintainer
#13 lore raw

PATCH 8/10 arm: arch_timer: move core to drivers/clocksource

In reply to 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.

---
 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 +2 -1
index ade7e92..781725e 100644
@@ -1561,9 +1561,10 @@ config HAVE_ARM_SCU
 	help
 	  This option enables support for the ARM system coherency unit
 
-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
 
arch/arm/include/asm/arch_timer.h +3 -16
index c50e415..a32d932 100644
@@ -5,22 +5,14 @@
 #include <asm/errno.h>
 
 #include <linux/clocksource.h>
+#include <linux/init.h>
 #include <linux/types.h>
 
+#include <clocksource/arm_arch_timer.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
@@ -126,11 +118,6 @@ static inline int arch_timer_sched_clock_init(void)
 {
 	return -ENXIO;
 }
-
-static inline struct timecounter *arch_timer_get_timecounter(void)
-{
-	return NULL;
-}
 #endif
 
 #endif
arch/arm/kernel/arch_timer.c +18 -357

409 lines not on this page · raw message

arch/arm/mach-omap2/Kconfig +1 -1
index d669e22..7b029f0 100644
@@ -71,12 +71,12 @@ config ARCH_OMAP4
 
 config SOC_OMAP5
 	bool "TI OMAP5"
-	select ARM_ARCH_TIMER
 	select ARM_CPU_SUSPEND if PM
 	select ARM_GIC
 	select CPU_V7
 	select HAVE_SMP
 	select SOC_HAS_REALTIME_COUNTER
+	select USE_ARM_ARCH_TIMER
 
 comment "OMAP Core Type"
 	depends on ARCH_OMAP2
drivers/clocksource/Kconfig +3 -0
index 6a78073..92ceeb1 100644
@@ -40,3 +40,6 @@ config CLKSRC_ARM_GENERIC
 	def_bool y if ARM64
 	help
 	  This option enables support for the ARM generic timer.
+
+config ARM_ARCH_TIMER
+	bool
drivers/clocksource/Makefile +1 -0
index 603be36..25e97e7 100644
@@ -16,3 +16,4 @@ obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 
 obj-$(CONFIG_CLKSRC_ARM_GENERIC)	+= arm_generic.o
+obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
drivers/clocksource/arm_arch_timer.c new file +374 -0

374 lines not on this page · raw message

include/clocksource/arm_arch_timer.h new file +63 -0

63 lines not on this page · raw message

Signature · 1 line
1.7.0.4
author maintainer
#14 lore raw

PATCH 9/10 arm64: move from arm_generic to arm_arch_timer

In reply to Mark Rutland

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.

---
 arch/arm64/Kconfig                   |    1 +
 arch/arm64/include/asm/arch_timer.h  |  133 +++++++++++++++++++
 arch/arm64/include/asm/arm_generic.h |  100 ---------------
 arch/arm64/kernel/time.c             |   29 ++++-
 drivers/clocksource/Kconfig          |    5 -
 drivers/clocksource/Makefile         |    1 -
 drivers/clocksource/arm_arch_timer.c |    1 +
 drivers/clocksource/arm_generic.c    |  232 ----------------------------------
 include/clocksource/arm_generic.h    |   21 ---
 9 files changed, 162 insertions(+), 361 deletions(-)
 create mode 100644 arch/arm64/include/asm/arch_timer.h
 delete mode 100644 arch/arm64/include/asm/arm_generic.h
 delete mode 100644 drivers/clocksource/arm_generic.c
 delete mode 100644 include/clocksource/arm_generic.h
arch/arm64/Kconfig +1 -0
index 15ac18a..52c5882 100644
@@ -2,6 +2,7 @@ config ARM64
 	def_bool y
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_WANT_COMPAT_IPC_PARSE_VERSION
+	select ARM_ARCH_TIMER
 	select GENERIC_CLOCKEVENTS
 	select GENERIC_HARDIRQS_NO_DEPRECATED
 	select GENERIC_IOMAP
arch/arm64/include/asm/arch_timer.h new file +133 -0

133 lines not on this page · raw message

arch/arm64/include/asm/arm_generic.h deleted +0 -100

100 lines not on this page · raw message

arch/arm64/kernel/time.c +27 -2
index 3b4b725..b0ef18d 100644
@@ -31,8 +31,9 @@
 #include <linux/syscore_ops.h>
 #include <linux/timer.h>
 #include <linux/irq.h>
+#include <linux/delay.h>
 
-#include <clocksource/arm_generic.h>
+#include <clocksource/arm_arch_timer.h>
 
 #include <asm/thread_info.h>
 #include <asm/stacktrace.h>
@@ -59,7 +60,31 @@ unsigned long profile_pc(struct pt_regs *regs)
 EXPORT_SYMBOL(profile_pc);
 #endif
 
+static u64 sched_clock_mult __read_mostly;
+
+unsigned long long notrace sched_clock(void)
+{
+	return arch_timer_read_counter() * sched_clock_mult;
+}
+
+int read_current_timer(unsigned long *timer_value)
+{
+	*timer_value = arch_timer_read_counter();
+	return 0;
+}
+
 void __init time_init(void)
 {
-	arm_generic_timer_init();
+	u32 arch_timer_rate;
+
+	if (arch_timer_init())
+		panic("Unable to initialise architected timer.\n");
+
+	arch_timer_rate = arch_timer_get_rate();
+
+	/* Cache the sched_clock multiplier to save a divide in the hot path. */
+	sched_clock_mult = NSEC_PER_SEC / arch_timer_rate;
+
+	/* Calibrate the delay loop directly */
+	lpj_fine = arch_timer_rate / HZ;
 }
drivers/clocksource/Kconfig +0 -5
index 92ceeb1..f01aec1 100644
@@ -36,10 +36,5 @@ config CLKSRC_DBX500_PRCMU_SCHED_CLOCK
 	help
 	  Use the always on PRCMU Timer as sched_clock
 
-config CLKSRC_ARM_GENERIC
-	def_bool y if ARM64
-	help
-	  This option enables support for the ARM generic timer.
-
 config ARM_ARCH_TIMER
 	bool
drivers/clocksource/Makefile +0 -1
index 25e97e7..875bf62 100644
@@ -15,5 +15,4 @@ obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 
-obj-$(CONFIG_CLKSRC_ARM_GENERIC)	+= arm_generic.o
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
drivers/clocksource/arm_arch_timer.c +1 -0
index 14e3e57..99743b2 100644
@@ -334,6 +334,7 @@ out:
 
 static const struct of_device_id arch_timer_of_match[] __initconst = {
 	{ .compatible	= "arm,armv7-timer",	},
+	{ .compatible	= "arm,armv8-timer",	},
 	{},
 };
 
drivers/clocksource/arm_generic.c deleted +0 -232

232 lines not on this page · raw message

include/clocksource/arm_generic.h deleted +0 -21
deleted file mode 100644
index 5b41b0d..0000000
@@ -1,21 +0,0 @@
-/*
- * Copyright (C) 2012 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#ifndef __CLKSOURCE_ARM_GENERIC_H
-#define __CLKSOURCE_ARM_GENERIC_H
-
-extern int arm_generic_timer_init(void);
-
-#endif
Signature · 1 line
1.7.0.4
#15 lore raw

PATCH 9/10 arm64: move from arm_generic to arm_arch_timer

Quoted 11 lines · Mark Rutland

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.

[...]

arch/arm64/include/asm/arch_timer.h line 111

Quoted 12 lines
arch/arm64/include/asm/arch_timer.h new file +12 -0
+static inline void __cpuinit arch_counter_enable_user_access(void)
+{
+       u32 cntkctl;
+
+       /* Disable user access to the timers and the virtual counter. */
+       asm volatile("mrs       %0, cntkctl_el1" : "=r" (cntkctl));
+       cntkctl &= ~((3 << 8) | (1 << 1));
+
+       /* Enable user access to the physical counter and frequency. */
+       cntkctl |= 1;
+       asm volatile("msr       cntkctl_el1, %0" : : "r" (cntkctl));
+}

We need to be careful here as I have some patches pending which change the vdso to use the virtual counter, rather than the physical one (updating drivers/clocksource/arm_generic.c in the process).

*Hopefully* we'll conflict and the fixup will do the right thing, but I thought I'd better raise it in case the vdso and the timer driver get out-of-sync and we start SIGILLing userspace!

Take a look at my aarch64/vdso branch.

Will

author maintainer
#16 lore raw

PATCH 10/10 Documentation: Add ARMv8 to arch_timer devicetree

In reply to Mark Rutland
---
 .../devicetree/bindings/arm/arch_timer.txt         |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/arm/arch_timer.txt +4 -3
index 52478c8..20746e5 100644
@@ -1,13 +1,14 @@
 * ARM architected timer
 
-ARM Cortex-A7 and Cortex-A15 have a per-core architected timer, which
-provides per-cpu timers.
+ARM cores may have a per-core architected timer, which provides per-cpu timers.
 
 The timer is attached to a GIC to deliver its per-processor interrupts.
 
 ** Timer node properties:
 
-- compatible : Should at least contain "arm,armv7-timer".
+- compatible : Should at least contain one of
+	"arm,armv7-timer"
+	"arm,armv8-timer"
 
 - interrupts : Interrupt list for secure, non-secure, virtual and
   hypervisor timers, in that order.
Signature · 1 line
1.7.0.4