This patchset adds support for memory mapped architected timers. We
don't have any other global broadcast timer in our system, so we use the
mmio timer during low power modes. The first patch is the binding.
The next two patches lay some groundwork so that the last patch is simpler.
The final patch adds support for mmio timers.
Patches are based on a recent patch from Mark that removes the
physical count reading (clocksource: arch_timer: use virtual counter,
message id <[email protected]>).
Stephen Boyd (4):
Documentation: Add memory mapped ARM architected timer binding
ARM: arch_timers: Pass clock event to set_mode callback
clocksource: arch_timer: Push the read/write wrappers deeper
clocksource: arch_timer: Add support for memory mapped timers
.../devicetree/bindings/arm/arch_timer.txt | 62 ++-
arch/arm/include/asm/arch_timer.h | 5 +-
arch/arm64/include/asm/arch_timer.h | 4 +-
drivers/clocksource/arm_arch_timer.c | 451 +++++++++++++++++----
include/clocksource/arm_arch_timer.h | 4 +-
5 files changed, 448 insertions(+), 78 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
We're going to introduce support to read and write the memory
mapped timer registers in the next patch, so push the cp15
read/write functions one level deeper. This simplifies the next
patch and makes it clearer what's going on.
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
arch/arm/include/asm/arch_timer.h | 5 ++--
arch/arm64/include/asm/arch_timer.h | 4 ++--
drivers/clocksource/arm_arch_timer.c | 44 ++++++++++++++++++++++++------------
3 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 35fea17..560f8a0 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -18,7 +18,8 @@ int arch_timer_sched_clock_init(void);
* 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)
+static inline void __arch_timer_reg_write(const int access, const int reg,
+ u32 val)
{
if (access == ARCH_TIMER_PHYS_ACCESS) {
switch (reg) {
@@ -45,7 +46,7 @@ static inline void arch_timer_reg_write(const int access, const int reg, u32 val
isb();
}
-static inline u32 arch_timer_reg_read(const int access, const int reg)
+static inline u32 __arch_timer_reg_read(const int access, const int reg)
{
u32 val = 0;
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 5307737..78b0379 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -26,7 +26,7 @@
#include <clocksource/arm_arch_timer.h>
-static inline void arch_timer_reg_write(int access, int reg, u32 val)
+static inline void __arch_timer_reg_write(int access, int reg, u32 val)
{
if (access == ARCH_TIMER_PHYS_ACCESS) {
switch (reg) {
@@ -57,7 +57,7 @@ static inline void arch_timer_reg_write(int access, int reg, u32 val)
isb();
}
-static inline u32 arch_timer_reg_read(int access, int reg)
+static inline u32 __arch_timer_reg_read(int access, int reg)
{
u32 val;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 545891b..38e0efc 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -43,14 +43,26 @@ static bool arch_timer_use_virtual = true;
* Architected system timer support.
*/
+static inline void arch_timer_reg_write(int access, int reg, u32 val,
+ struct clock_event_device *clk)
+{
+ __arch_timer_reg_write(access, reg, val);
+}
+
+static inline u32 arch_timer_reg_read(int access, int reg,
+ struct clock_event_device *clk)
+{
+ return __arch_timer_reg_read(access, reg);
+}
+
static inline irqreturn_t timer_handler(const int access,
struct clock_event_device *evt)
{
unsigned long ctrl;
- ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
+ ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
ctrl |= ARCH_TIMER_CTRL_IT_MASK;
- arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
+ arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
evt->event_handler(evt);
return IRQ_HANDLED;
}
@@ -72,15 +84,16 @@ static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
}
-static inline void timer_set_mode(const int access, int mode)
+static inline void timer_set_mode(const int access, int mode,
+ struct clock_event_device *clk)
{
unsigned long ctrl;
switch (mode) {
case CLOCK_EVT_MODE_UNUSED:
case CLOCK_EVT_MODE_SHUTDOWN:
- ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
+ ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
- arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl);
+ arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
break;
default:
break;
@@ -90,36 +103,37 @@ static inline void timer_set_mode(const int access, int mode)
static void arch_timer_set_mode_virt(enum clock_event_mode mode,
struct clock_event_device *clk)
{
- timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode);
+ timer_set_mode(ARCH_TIMER_VIRT_ACCESS, mode, clk);
}
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);
+ timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk);
}
-static inline void set_next_event(const int access, unsigned long evt)
+static inline void set_next_event(const int access, unsigned long evt,
+ struct clock_event_device *clk)
{
unsigned long ctrl;
- ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL);
+ ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
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(access, ARCH_TIMER_REG_TVAL, evt, clk);
+ arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
}
static int arch_timer_set_next_event_virt(unsigned long evt,
- struct clock_event_device *unused)
+ struct clock_event_device *clk)
{
- set_next_event(ARCH_TIMER_VIRT_ACCESS, evt);
+ set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
return 0;
}
static int arch_timer_set_next_event_phys(unsigned long evt,
- struct clock_event_device *unused)
+ struct clock_event_device *clk)
{
- set_next_event(ARCH_TIMER_PHYS_ACCESS, evt);
+ set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
return 0;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
Add support for the memory mapped timers by filling in the
read/write functions and adding some parsing code. Note that we
only register one clocksource, preferring the cp15 based
clocksource over the mmio one.
To keep things simple we register one global clockevent. This
covers the case of UP and SMP systems with only mmio hardware and
systems where the memory mapped timers are used as the broadcast
timer in low power modes. The DT binding allows for per-CPU
memory mapped timers in case we want to support that in the
future, but the code isn't added here.
We also punt on hypervisor support, although it should be
possible to support it by searching for the hyp frame instead of
the kernel frame and updating KVM timers to support it.
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clocksource/arm_arch_timer.c | 411 ++++++++++++++++++++++++++++++-----
include/clocksource/arm_arch_timer.h | 4 +-
2 files changed, 357 insertions(+), 58 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 38e0efc..6158422 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -16,13 +16,43 @@
#include <linux/clockchips.h>
#include <linux/interrupt.h>
#include <linux/of_irq.h>
+#include <linux/of_address.h>
#include <linux/io.h>
+#include <linux/slab.h>
#include <asm/arch_timer.h>
#include <asm/virt.h>
#include <clocksource/arm_arch_timer.h>
+#define FRAME_NUM_MASK 0x7
+#define FRAME_TYPE_MASK (0x3 << 8)
+#define FRAME_TYPE_KERNEL_USER (0 << 8)
+
+#define CNTTIDR 0x08
+#define CNTTIDR_VIRT(n) (BIT(1) << ((n) * 4))
+
+#define CNTVCT_LO 0x08
+#define CNTVCT_HI 0x0c
+#define CNTFRQ 0x10
+#define CNTP_TVAL 0x28
+#define CNTP_CTL 0x2c
+#define CNTV_TVAL 0x38
+#define CNTV_CTL 0x3c
+
+#define ARCH_CP15_TIMER BIT(0)
+#define ARCH_MEM_TIMER BIT(1)
+static unsigned arch_timers_present;
+
+static void __iomem *arch_counter_base;
+
+struct arch_timer {
+ void __iomem *base;
+ struct clock_event_device evt;
+};
+
+#define to_arch_timer(e) container_of(e, struct arch_timer, evt)
+
static u32 arch_timer_rate;
enum ppi_nr {
@@ -38,6 +68,7 @@ static int arch_timer_ppi[MAX_TIMER_PPI];
static struct clock_event_device __percpu *arch_timer_evt;
static bool arch_timer_use_virtual = true;
+static bool arch_timer_mem_use_virtual = true;
/*
* Architected system timer support.
@@ -46,13 +77,69 @@ static bool arch_timer_use_virtual = true;
static inline void arch_timer_reg_write(int access, int reg, u32 val,
struct clock_event_device *clk)
{
- __arch_timer_reg_write(access, reg, val);
+ if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
+ struct arch_timer *timer = to_arch_timer(clk);
+ switch (reg) {
+ case ARCH_TIMER_REG_CTRL:
+ writel_relaxed(val, timer->base + CNTP_CTL);
+ break;
+ case ARCH_TIMER_REG_TVAL:
+ writel_relaxed(val, timer->base + CNTP_TVAL);
+ break;
+ default:
+ BUILD_BUG();
+ }
+ } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
+ struct arch_timer *timer = to_arch_timer(clk);
+ switch (reg) {
+ case ARCH_TIMER_REG_CTRL:
+ writel_relaxed(val, timer->base + CNTV_CTL);
+ break;
+ case ARCH_TIMER_REG_TVAL:
+ writel_relaxed(val, timer->base + CNTV_TVAL);
+ break;
+ default:
+ BUILD_BUG();
+ }
+ } else {
+ __arch_timer_reg_write(access, reg, val);
+ }
}
static inline u32 arch_timer_reg_read(int access, int reg,
struct clock_event_device *clk)
{
- return __arch_timer_reg_read(access, reg);
+ u32 val;
+
+ if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
+ struct arch_timer *timer = to_arch_timer(clk);
+ switch (reg) {
+ case ARCH_TIMER_REG_CTRL:
+ val = readl_relaxed(timer->base + CNTP_CTL);
+ break;
+ case ARCH_TIMER_REG_TVAL:
+ val = readl_relaxed(timer->base + CNTP_TVAL);
+ break;
+ default:
+ BUILD_BUG();
+ }
+ } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
+ struct arch_timer *timer = to_arch_timer(clk);
+ switch (reg) {
+ case ARCH_TIMER_REG_CTRL:
+ val = readl_relaxed(timer->base + CNTV_CTL);
+ break;
+ case ARCH_TIMER_REG_TVAL:
+ val = readl_relaxed(timer->base + CNTV_TVAL);
+ break;
+ default:
+ BUILD_BUG();
+ }
+ } else {
+ val = __arch_timer_reg_read(access, reg);
+ }
+
+ return val;
}
static inline irqreturn_t timer_handler(const int access,
@@ -84,6 +171,20 @@ static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
}
+static irqreturn_t arch_timer_handler_phys_mem(int irq, void *dev_id)
+{
+ struct clock_event_device *evt = dev_id;
+
+ return timer_handler(ARCH_TIMER_MEM_PHYS_ACCESS, evt);
+}
+
+static irqreturn_t arch_timer_handler_virt_mem(int irq, void *dev_id)
+{
+ struct clock_event_device *evt = dev_id;
+
+ return timer_handler(ARCH_TIMER_MEM_VIRT_ACCESS, evt);
+}
+
static inline void timer_set_mode(const int access, int mode,
struct clock_event_device *clk)
{
@@ -112,6 +213,18 @@ static void arch_timer_set_mode_phys(enum clock_event_mode mode,
timer_set_mode(ARCH_TIMER_PHYS_ACCESS, mode, clk);
}
+static void arch_timer_set_mode_virt_mem(enum clock_event_mode mode,
+ struct clock_event_device *clk)
+{
+ timer_set_mode(ARCH_TIMER_MEM_VIRT_ACCESS, mode, clk);
+}
+
+static void arch_timer_set_mode_phys_mem(enum clock_event_mode mode,
+ struct clock_event_device *clk)
+{
+ timer_set_mode(ARCH_TIMER_MEM_PHYS_ACCESS, mode, clk);
+}
+
static inline void set_next_event(const int access, unsigned long evt,
struct clock_event_device *clk)
{
@@ -137,27 +250,62 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
return 0;
}
-static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
+static int arch_timer_set_next_event_virt_mem(unsigned long evt,
+ struct clock_event_device *clk)
{
- clk->features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP;
- clk->name = "arch_sys_timer";
- clk->rating = 450;
- if (arch_timer_use_virtual) {
- clk->irq = arch_timer_ppi[VIRT_PPI];
- clk->set_mode = arch_timer_set_mode_virt;
- clk->set_next_event = arch_timer_set_next_event_virt;
+ set_next_event(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
+ return 0;
+}
+
+static int arch_timer_set_next_event_phys_mem(unsigned long evt,
+ struct clock_event_device *clk)
+{
+ set_next_event(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
+ return 0;
+}
+
+static void __cpuinit __arch_timer_setup(unsigned type,
+ struct clock_event_device *clk)
+{
+ clk->features = CLOCK_EVT_FEAT_ONESHOT;
+
+ if (type == ARCH_CP15_TIMER) {
+ clk->features |= CLOCK_EVT_FEAT_C3STOP;
+ clk->name = "arch_sys_timer";
+ clk->rating = 450;
+ clk->cpumask = cpumask_of(smp_processor_id());
+ if (arch_timer_use_virtual) {
+ clk->irq = arch_timer_ppi[VIRT_PPI];
+ clk->set_mode = arch_timer_set_mode_virt;
+ clk->set_next_event = arch_timer_set_next_event_virt;
+ } else {
+ clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
+ clk->set_mode = arch_timer_set_mode_phys;
+ clk->set_next_event = arch_timer_set_next_event_phys;
+ }
} else {
- clk->irq = arch_timer_ppi[PHYS_SECURE_PPI];
- clk->set_mode = arch_timer_set_mode_phys;
- clk->set_next_event = arch_timer_set_next_event_phys;
+ clk->name = "arch_mem_timer";
+ clk->rating = 400;
+ clk->cpumask = cpu_all_mask;
+ if (arch_timer_mem_use_virtual) {
+ clk->set_mode = arch_timer_set_mode_virt_mem;
+ clk->set_next_event =
+ arch_timer_set_next_event_virt_mem;
+ } else {
+ clk->set_mode = arch_timer_set_mode_phys_mem;
+ clk->set_next_event =
+ arch_timer_set_next_event_phys_mem;
+ }
}
- clk->cpumask = cpumask_of(smp_processor_id());
-
clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, clk);
- clockevents_config_and_register(clk, arch_timer_rate,
- 0xf, 0x7fffffff);
+ clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff);
+}
+
+static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
+{
+ __arch_timer_setup(ARCH_CP15_TIMER, clk);
if (arch_timer_use_virtual)
enable_percpu_irq(arch_timer_ppi[VIRT_PPI], 0);
@@ -172,27 +320,41 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
return 0;
}
-static int arch_timer_available(void)
+static void
+arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
{
- u32 freq;
-
- if (arch_timer_rate == 0) {
- freq = arch_timer_get_cntfrq();
-
- /* Check the timer frequency. */
- if (freq == 0) {
- pr_warn("Architected timer frequency not available\n");
- return -EINVAL;
- }
+ /* Who has more than one independent system counters? */
+ if (arch_timer_rate)
+ return;
- arch_timer_rate = freq;
+ /* Try to determine the frequency from the device tree or CNTFRQ */
+ if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+ if (cntbase)
+ arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
+ else
+ arch_timer_rate = arch_timer_get_cntfrq();
}
- pr_info_once("Architected local timer running at %lu.%02luMHz (%s).\n",
+ /* Check the timer frequency. */
+ if (arch_timer_rate == 0)
+ pr_warn("Architected timer frequency not available\n");
+}
+
+static void arch_timer_banner(unsigned type)
+{
+ pr_info("Architected %s%s%s timer(s) running at %lu.%02luMHz (%s%s%s).\n",
+ type & ARCH_CP15_TIMER ? "cp15" : "",
+ type == (ARCH_CP15_TIMER | ARCH_MEM_TIMER) ? " and " : "",
+ type & ARCH_MEM_TIMER ? "mmio" : "",
(unsigned long)arch_timer_rate / 1000000,
(unsigned long)(arch_timer_rate / 10000) % 100,
- arch_timer_use_virtual ? "virt" : "phys");
- return 0;
+ type & ARCH_CP15_TIMER ?
+ arch_timer_use_virtual ? "virt" : "phys" :
+ "",
+ type == (ARCH_CP15_TIMER | ARCH_MEM_TIMER) ? "/" : "",
+ type & ARCH_MEM_TIMER ?
+ arch_timer_mem_use_virtual ? "virt" : "phys" :
+ "");
}
u32 arch_timer_get_rate(void)
@@ -200,19 +362,29 @@ u32 arch_timer_get_rate(void)
return arch_timer_rate;
}
-u64 arch_timer_read_counter(void)
+static u64 arch_counter_get_mem_cntvct(void)
{
- return arch_counter_get_cntvct();
+ u32 vct_lo, vct_hi, tmp_hi;
+
+ do {
+ vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
+ vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
+ tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
+ } while (vct_hi != tmp_hi);
+
+ return ((u64) vct_hi << 32) | vct_lo;
}
+u64 (*arch_timer_read_counter)(void);
+
static cycle_t arch_counter_read(struct clocksource *cs)
{
- return arch_counter_get_cntvct();
+ return arch_timer_read_counter();
}
static cycle_t arch_counter_read_cc(const struct cyclecounter *cc)
{
- return arch_counter_get_cntvct();
+ return arch_timer_read_counter();
}
static struct clocksource clocksource_counter = {
@@ -235,6 +407,23 @@ struct timecounter *arch_timer_get_timecounter(void)
return &timecounter;
}
+static void __init arch_counter_register(unsigned type)
+{
+ u64 start_count;
+
+ /* Register the CP15 based counter if we have one */
+ if (type & ARCH_CP15_TIMER)
+ arch_timer_read_counter = arch_counter_get_cntvct;
+ else
+ arch_timer_read_counter = arch_counter_get_mem_cntvct;
+
+ start_count = arch_timer_read_counter();
+ clocksource_register_hz(&clocksource_counter, arch_timer_rate);
+ cyclecounter.mult = clocksource_counter.mult;
+ cyclecounter.shift = clocksource_counter.shift;
+ timecounter_init(&timecounter, &cyclecounter, start_count);
+}
+
static void __cpuinit arch_timer_stop(struct clock_event_device *clk)
{
pr_debug("arch_timer_teardown disable IRQ%d cpu #%d\n",
@@ -277,22 +466,12 @@ static int __init arch_timer_register(void)
int err;
int ppi;
- err = arch_timer_available();
- if (err)
- goto out;
-
arch_timer_evt = alloc_percpu(struct clock_event_device);
if (!arch_timer_evt) {
err = -ENOMEM;
goto out;
}
- 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_cntvct());
-
if (arch_timer_use_virtual) {
ppi = arch_timer_ppi[VIRT_PPI];
err = request_percpu_irq(ppi, arch_timer_handler_virt,
@@ -343,31 +522,50 @@ out:
return err;
}
+static int __init arch_timer_mem_register(void __iomem *base, unsigned irq)
+{
+ int ret;
+ irq_handler_t func;
+ struct arch_timer *t;
+
+ t = kzalloc(sizeof(*t), GFP_KERNEL);
+ if (!t)
+ return -ENOMEM;
+
+ t->base = base;
+ t->evt.irq = irq;
+ __arch_timer_setup(ARCH_MEM_TIMER, &t->evt);
+
+ if (arch_timer_mem_use_virtual)
+ func = arch_timer_handler_virt_mem;
+ else
+ func = arch_timer_handler_phys_mem;
+
+ ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt);
+ if (ret)
+ kfree(t);
+ return ret;
+}
+
static const struct of_device_id arch_timer_of_match[] __initconst = {
{ .compatible = "arm,armv7-timer", },
{ .compatible = "arm,armv8-timer", },
{},
};
-int __init arch_timer_init(void)
+static int __init arch_timer_parse(void)
{
struct device_node *np;
- u32 freq;
int i;
np = of_find_matching_node(NULL, arch_timer_of_match);
- if (!np) {
- pr_err("arch_timer: can't find DT node\n");
- return -ENODEV;
- }
-
- /* Try to determine the frequency from the device tree or CNTFRQ */
- if (!of_property_read_u32(np, "clock-frequency", &freq))
- arch_timer_rate = freq;
+ if (!np)
+ return 0;
+ arch_timers_present |= ARCH_CP15_TIMER;
for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
-
+ arch_timer_detect_rate(NULL, np);
of_node_put(np);
/*
@@ -390,3 +588,102 @@ int __init arch_timer_init(void)
return arch_timer_register();
}
+
+static const struct of_device_id arch_timer_mem_of_match[] __initconst = {
+ { .compatible = "arm,armv7-timer-mem", },
+ {},
+};
+
+static int __init arch_timer_mem_parse(void)
+{
+ struct device_node *np, *frame, *best_frame = NULL;
+ void __iomem *cntctlbase, *base;
+ unsigned irq;
+ u32 cnttidr;
+
+ np = of_find_matching_node(NULL, arch_timer_mem_of_match);
+ if (!np)
+ return 0;
+
+ arch_timers_present |= ARCH_MEM_TIMER;
+ cntctlbase = of_iomap(np, 0);
+ if (!cntctlbase) {
+ pr_err("arch_timer: Can't find CNTCTLBase\n");
+ of_node_put(np);
+ return -ENOMEM;
+ }
+
+ cnttidr = readl_relaxed(cntctlbase + CNTTIDR);
+ /*
+ * Try to find a virtual capable kernel/user frame. Otherwise fall back
+ * to a physical kernel/user frame.
+ */
+ for_each_child_of_node(np, frame) {
+ u32 id;
+ int n;
+
+ if (of_property_read_u32(frame, "frame-id", &id)) {
+ pr_warn("arch_timer: Missing frame-id\n");
+ of_node_put(frame);
+ of_node_put(np);
+ return -EINVAL;
+ }
+
+ n = id & FRAME_NUM_MASK;
+ if ((id & FRAME_TYPE_MASK) == FRAME_TYPE_KERNEL_USER) {
+ if (cnttidr & CNTTIDR_VIRT(n)) {
+ of_node_put(best_frame);
+ best_frame = frame;
+ break;
+ }
+ of_node_put(best_frame);
+ best_frame = of_node_get(frame);
+ }
+ }
+ of_node_put(np);
+
+ base = arch_counter_base = of_iomap(best_frame, 0);
+ if (!base) {
+ pr_warn("arch_timer: Can't map frame's registers\n");
+ of_node_put(best_frame);
+ return -ENOMEM;
+ }
+
+ irq = irq_of_parse_and_map(best_frame, 1);
+ if (!irq) {
+ arch_timer_mem_use_virtual = false;
+ irq = irq_of_parse_and_map(best_frame, 0);
+ }
+ if (!irq) {
+ pr_warn("arch_timer: Missing timer irq\n");
+ of_node_put(best_frame);
+ return -EINVAL;
+ }
+
+ arch_timer_detect_rate(base, best_frame);
+ of_node_put(best_frame);
+
+ return arch_timer_mem_register(base, irq);
+}
+
+int __init arch_timer_init(void)
+{
+ int err;
+
+ err = arch_timer_parse();
+ if (err)
+ return err;
+
+ err = arch_timer_mem_parse();
+ if (err)
+ return err;
+
+ if (!(arch_timers_present & (ARCH_CP15_TIMER | ARCH_MEM_TIMER))) {
+ pr_err("arch_timer: can't find DT node\n");
+ return -ENODEV;
+ }
+
+ arch_timer_banner(arch_timers_present);
+ arch_counter_register(arch_timers_present);
+ return 0;
+}
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 3883812..c3ff7fe 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -28,12 +28,14 @@
#define ARCH_TIMER_PHYS_ACCESS 0
#define ARCH_TIMER_VIRT_ACCESS 1
+#define ARCH_TIMER_MEM_PHYS_ACCESS 2
+#define ARCH_TIMER_MEM_VIRT_ACCESS 3
#ifdef CONFIG_ARM_ARCH_TIMER
extern int arch_timer_init(void);
extern u32 arch_timer_get_rate(void);
-extern u64 arch_timer_read_counter(void);
+extern u64 (*arch_timer_read_counter)(void);
extern struct timecounter *arch_timer_get_timecounter(void);
#else
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
There isn't any reason why we don't pass the event here and we'll
need it in the near future for memory mapped arch timers anyway.
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/clocksource/arm_arch_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2abb861..545891b 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -140,7 +140,7 @@ static int __cpuinit arch_timer_setup(struct clock_event_device *clk)
clk->cpumask = cpumask_of(smp_processor_id());
- clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, NULL);
+ clk->set_mode(CLOCK_EVT_MODE_SHUTDOWN, clk);
clockevents_config_and_register(clk, arch_timer_rate,
0xf, 0x7fffffff);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
Add a binding for the arm architected timer hardware's memory
mapped interface. The mmio timer hardware is made up of one base
frame and a collection of up to 8 timer frames, where each of the
8 timer frames can have either one or two views. A frame
typically maps to a privilege level (user/kernel, hypervisor,
secure). The first view has full access to the registers within a
frame, while the second view can be restricted to particular
registers within a frame. Each frame must support a physical
timer. It's optional for a frame to support a virtual timer.
Cc: [email protected]
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
.../devicetree/bindings/arm/arch_timer.txt | 62 ++++++++++++++++++++--
1 file changed, 59 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
index 20746e5..69ef711 100644
--- a/Documentation/devicetree/bindings/arm/arch_timer.txt
+++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
@@ -1,10 +1,14 @@
* ARM architected timer
-ARM cores may 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,
+or a memory mapped architected timer, which provides up to 8 frames with a
+physical and optional virtual timer per frame.
-The timer is attached to a GIC to deliver its per-processor interrupts.
+The per-core architected timer is attached to a GIC to deliver its
+per-processor interrupts via PPIs. The memory mapped timer is attached to a GIC
+to deliver its interrupts via SPIs.
-** Timer node properties:
+** CP15 Timer node properties:
- compatible : Should at least contain one of
"arm,armv7-timer"
@@ -26,3 +30,55 @@ Example:
<1 10 0xf08>;
clock-frequency = <100000000>;
};
+
+** Memory mapped timer node properties
+
+- compatible : Should at least contain "arm,armv7-timer-mem".
+
+- #address-cells : Must be 1.
+
+- #size-cells : Must be 1.
+
+- ranges : Indicates parent and child bus address space are the same.
+
+- clock-frequency : The frequency of the main counter, in Hz. Optional.
+
+- reg : The control frame base address.
+
+Frame:
+
+- frame-id: Encoded as follows:
+ bits[3:0] frame number: 0 to 7.
+ bits[10:8] frame usage:
+ 0 - user/kernel
+ 1 - hyp
+ 2 - secure
+
+- interrupts : Interrupt list for physical and virtual timers in that order.
+ The virtual timer interrupt is optional.
+
+- reg : The first and second view base addresses in that order. The second view
+ base address is optional.
+
+Example:
+
+ timer@f0000000 {
+ compatible = "arm,armv7-timer-mem";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ reg = <0xf0000000 0x1000>;
+ clock-frequency = <50000000>;
+ frame0@f0001000 {
+ frame-id = <0x0>
+ interrupts = <0 13 0x8>,
+ <0 14 0x8>;
+ reg = <0xf0001000 0x1000>,
+ <0xf0002000 0x1000>;
+ };
+ frame1@f0003000 {
+ frame-id = <0x11>
+ interrupts = <0 15 0x8>;
+ reg = <0xf0003000 0x1000>;
+ };
+ };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
Hi Stephen,
On Tue, Apr 09, 2013 at 03:30:20AM +0100, Stephen Boyd wrote:
> Add a binding for the arm architected timer hardware's memory
> mapped interface. The mmio timer hardware is made up of one base
> frame and a collection of up to 8 timer frames, where each of the
> 8 timer frames can have either one or two views. A frame
> typically maps to a privilege level (user/kernel, hypervisor,
> secure). The first view has full access to the registers within a
> frame, while the second view can be restricted to particular
> registers within a frame. Each frame must support a physical
> timer. It's optional for a frame to support a virtual timer.
>
> Cc: [email protected]
> Cc: Mark Rutland <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> .../devicetree/bindings/arm/arch_timer.txt | 62 ++++++++++++++++++++--
> 1 file changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
> index 20746e5..69ef711 100644
> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
> @@ -1,10 +1,14 @@
> * ARM architected timer
>
> -ARM cores may 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,
> +or a memory mapped architected timer, which provides up to 8 frames with a
> +physical and optional virtual timer per frame.
>
> -The timer is attached to a GIC to deliver its per-processor interrupts.
> +The per-core architected timer is attached to a GIC to deliver its
> +per-processor interrupts via PPIs. The memory mapped timer is attached to a GIC
> +to deliver its interrupts via SPIs.
>
> -** Timer node properties:
> +** CP15 Timer node properties:
>
> - compatible : Should at least contain one of
> "arm,armv7-timer"
> @@ -26,3 +30,55 @@ Example:
> <1 10 0xf08>;
> clock-frequency = <100000000>;
> };
> +
> +** Memory mapped timer node properties
> +
> +- compatible : Should at least contain "arm,armv7-timer-mem".
> +
> +- #address-cells : Must be 1.
What about LPAE systems?
How about something like the following:
#address-cells : If the ranges property is empty, the same value as the
parent node's #address-cells property. Otherwise, a
value such that the ranges property specifies a
mapping to the parent node's address space.
> +
> +- #size-cells : Must be 1.
> +
> +- ranges : Indicates parent and child bus address space are the same.
> +
Similarly, what if someone wants to write a more complex mapping for some
reason?
We should be able to handle it if we use the standard accessors.
> +- clock-frequency : The frequency of the main counter, in Hz. Optional.
> +
> +- reg : The control frame base address.
> +
> +Frame:
> +
> +- frame-id: Encoded as follows:
> + bits[3:0] frame number: 0 to 7.
> + bits[10:8] frame usage:
> + 0 - user/kernel
> + 1 - hyp
> + 2 - secure
> +
Could we not just have a disabled status property for those frames claimed by a
higher level (either secure firmware or hypervisor)? Or have I missed something
here?
Then we'd just have a frame-number property, which is easier for humans to read
and understand.
> +- interrupts : Interrupt list for physical and virtual timers in that order.
> + The virtual timer interrupt is optional.
> +
> +- reg : The first and second view base addresses in that order. The second view
> + base address is optional.
> +
> +Example:
> +
> + timer@f0000000 {
> + compatible = "arm,armv7-timer-mem";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + reg = <0xf0000000 0x1000>;
> + clock-frequency = <50000000>;
> + frame0@f0001000 {
> + frame-id = <0x0>
> + interrupts = <0 13 0x8>,
> + <0 14 0x8>;
> + reg = <0xf0001000 0x1000>,
> + <0xf0002000 0x1000>;
> + };
> + frame1@f0003000 {
> + frame-id = <0x11>
> + interrupts = <0 15 0x8>;
> + reg = <0xf0003000 0x1000>;
> + };
> + };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
>
>
Mark.
Hi,
On Tue, Apr 09, 2013 at 03:30:22AM +0100, Stephen Boyd wrote:
> We're going to introduce support to read and write the memory
> mapped timer registers in the next patch, so push the cp15
> read/write functions one level deeper. This simplifies the next
> patch and makes it clearer what's going on.
>
> Cc: Mark Rutland <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> arch/arm/include/asm/arch_timer.h | 5 ++--
> arch/arm64/include/asm/arch_timer.h | 4 ++--
> drivers/clocksource/arm_arch_timer.c | 44 ++++++++++++++++++++++++------------
> 3 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 35fea17..560f8a0 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -18,7 +18,8 @@ int arch_timer_sched_clock_init(void);
> * 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)
> +static inline void __arch_timer_reg_write(const int access, const int reg,
> + u32 val)
> {
> if (access == ARCH_TIMER_PHYS_ACCESS) {
> switch (reg) {
> @@ -45,7 +46,7 @@ static inline void arch_timer_reg_write(const int access, const int reg, u32 val
> isb();
> }
>
> -static inline u32 arch_timer_reg_read(const int access, const int reg)
> +static inline u32 __arch_timer_reg_read(const int access, const int reg)
Rather than prefixing all of these with "__", why not add "cp15" to the names?
I think that'd make the end result more consistent and clearer, as we'd have a
_mem version and a _cp15 version of each function.
Mark.
On 04/09/13 02:38, Mark Rutland wrote:
> On Tue, Apr 09, 2013 at 03:30:22AM +0100, Stephen Boyd wrote:
>> -static inline u32 arch_timer_reg_read(const int access, const int reg)
>> +static inline u32 __arch_timer_reg_read(const int access, const int reg)
> Rather than prefixing all of these with "__", why not add "cp15" to the names?
>
> I think that'd make the end result more consistent and clearer, as we'd have a
> _mem version and a _cp15 version of each function.
Sounds fair. I'll make the change.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 04/09/13 02:08, Mark Rutland wrote:
> On Tue, Apr 09, 2013 at 03:30:20AM +0100, Stephen Boyd wrote:
>>
>> -** Timer node properties:
>> +** CP15 Timer node properties:
>>
>> - compatible : Should at least contain one of
>> "arm,armv7-timer"
>> @@ -26,3 +30,55 @@ Example:
>> <1 10 0xf08>;
>> clock-frequency = <100000000>;
>> };
>> +
>> +** Memory mapped timer node properties
>> +
>> +- compatible : Should at least contain "arm,armv7-timer-mem".
>> +
>> +- #address-cells : Must be 1.
> What about LPAE systems?
>
> How about something like the following:
>
> #address-cells : If the ranges property is empty, the same value as the
> parent node's #address-cells property. Otherwise, a
> value such that the ranges property specifies a
> mapping to the parent node's address space.
Yes that is much better. I wasn't trying to preclude LPAE or 64 bit systems.
>> +
>> +- #size-cells : Must be 1.
>> +
>> +- ranges : Indicates parent and child bus address space are the same.
>> +
> Similarly, what if someone wants to write a more complex mapping for some
> reason?
>
> We should be able to handle it if we use the standard accessors.
Maybe I should just leave this part out? They are standard DT properties
so I could assume DT writers know what to do.
>> +- clock-frequency : The frequency of the main counter, in Hz. Optional.
>> +
>> +- reg : The control frame base address.
>> +
>> +Frame:
>> +
>> +- frame-id: Encoded as follows:
>> + bits[3:0] frame number: 0 to 7.
>> + bits[10:8] frame usage:
>> + 0 - user/kernel
>> + 1 - hyp
>> + 2 - secure
>> +
> Could we not just have a disabled status property for those frames claimed by a
> higher level (either secure firmware or hypervisor)? Or have I missed something
> here?
Using disabled status would work. I was also thinking maybe we should
use a compatible string in each frame's node. Then we could match
against compatible children like "frame-user", "frame-kernel",
"frame-hyp", "frame-secure", "frame-user-kernel", etc. It allows us
flexibility if we should need to add something else in the future.
Also to get the frame number, I was thinking maybe we should expand the
reg property to have two address cells. Then we could have reg = <0
0xf0001000 0x1000>.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Apr 09, 2013 at 05:42:38PM +0100, Stephen Boyd wrote:
> On 04/09/13 02:08, Mark Rutland wrote:
> > On Tue, Apr 09, 2013 at 03:30:20AM +0100, Stephen Boyd wrote:
> >>
> >> -** Timer node properties:
> >> +** CP15 Timer node properties:
> >>
> >> - compatible : Should at least contain one of
> >> "arm,armv7-timer"
> >> @@ -26,3 +30,55 @@ Example:
> >> <1 10 0xf08>;
> >> clock-frequency = <100000000>;
> >> };
> >> +
> >> +** Memory mapped timer node properties
> >> +
> >> +- compatible : Should at least contain "arm,armv7-timer-mem".
> >> +
> >> +- #address-cells : Must be 1.
> > What about LPAE systems?
> >
> > How about something like the following:
> >
> > #address-cells : If the ranges property is empty, the same value as the
> > parent node's #address-cells property. Otherwise, a
> > value such that the ranges property specifies a
> > mapping to the parent node's address space.
>
> Yes that is much better. I wasn't trying to preclude LPAE or 64 bit systems.
Great.
>
> >> +
> >> +- #size-cells : Must be 1.
> >> +
> >> +- ranges : Indicates parent and child bus address space are the same.
> >> +
> > Similarly, what if someone wants to write a more complex mapping for some
> > reason?
> >
> > We should be able to handle it if we use the standard accessors.
>
> Maybe I should just leave this part out? They are standard DT properties
> so I could assume DT writers know what to do.
I'd be happy with that. It may be worth describing them as "as necessary" or
something to that effect.
>
> >> +- clock-frequency : The frequency of the main counter, in Hz. Optional.
> >> +
> >> +- reg : The control frame base address.
> >> +
> >> +Frame:
> >> +
> >> +- frame-id: Encoded as follows:
> >> + bits[3:0] frame number: 0 to 7.
> >> + bits[10:8] frame usage:
> >> + 0 - user/kernel
> >> + 1 - hyp
> >> + 2 - secure
> >> +
> > Could we not just have a disabled status property for those frames claimed by a
> > higher level (either secure firmware or hypervisor)? Or have I missed something
> > here?
>
> Using disabled status would work. I was also thinking maybe we should
> use a compatible string in each frame's node. Then we could match
> against compatible children like "frame-user", "frame-kernel",
> "frame-hyp", "frame-secure", "frame-user-kernel", etc. It allows us
> flexibility if we should need to add something else in the future.
I can see why we need to specify secure/non-secure, but I'm not sure why we
need to specify hyp/user/kernel usage. Could we not leave this up to the kernel
to figure out?
A basic overveiew for those that don't know about the memory mapped timers:
* There's one control frame CNTCTLBase. Some registers in this frame are only
available for secure accesses, including CNTNSAR which sets whether the
counter frames are accessible from the non-secure side.
* There are up to 8 timer frames, which have their own CNTVOFF and
physical/virtual timers. Each frame CNTBaseN is duplicated at CNTPL0BaseN
with CNTVOFF and CNTPL0ACR (which controls PL0 accesses) inaccessible.
I can see that we might have frames/registers we can't access (if we were
booted on the non-secure side), but I can't see anything limiting whether we
use a frame for kernel/hyp/user beyond that. Have I missed something?
Could we not have something like the following for each frame:
frame0 {
frame-id = <0>;
status = "disabled"; /* booted NS, secure firmware has not enabled access */
reg = <0x... 0x1000>, /* CNTBase0 */
<0x... 0x1000>; /* CNTPL0Base0 */
};
>
> Also to get the frame number, I was thinking maybe we should expand the
> reg property to have two address cells. Then we could have reg = <0
> 0xf0001000 0x1000>.
We could do that, but then you definitely need a more complex ranges property,
and additional parsing code to handle grabbing it out of the reg property. I
can't see what it buys us.
Mark.
On 04/10/13 03:13, Mark Rutland wrote:
>>>> +
>>>> +- #size-cells : Must be 1.
>>>> +
>>>> +- ranges : Indicates parent and child bus address space are the same.
>>>> +
>>> Similarly, what if someone wants to write a more complex mapping for some
>>> reason?
>>>
>>> We should be able to handle it if we use the standard accessors.
>> Maybe I should just leave this part out? They are standard DT properties
>> so I could assume DT writers know what to do.
> I'd be happy with that. It may be worth describing them as "as necessary" or
> something to that effect.
Ok. I added this and removed the property descriptions:
Note that #address-cells, #size-cells, and ranges shall be present to ensure
the CPU can address a frame's registers.
> I can see why we need to specify secure/non-secure, but I'm not sure why we
> need to specify hyp/user/kernel usage. Could we not leave this up to the kernel
> to figure out?
>
> A basic overveiew for those that don't know about the memory mapped timers:
>
> * There's one control frame CNTCTLBase. Some registers in this frame are only
> available for secure accesses, including CNTNSAR which sets whether the
> counter frames are accessible from the non-secure side.
>
> * There are up to 8 timer frames, which have their own CNTVOFF and
> physical/virtual timers. Each frame CNTBaseN is duplicated at CNTPL0BaseN
> with CNTVOFF and CNTPL0ACR (which controls PL0 accesses) inaccessible.
>
> I can see that we might have frames/registers we can't access (if we were
> booted on the non-secure side), but I can't see anything limiting whether we
> use a frame for kernel/hyp/user beyond that. Have I missed something?
>
> Could we not have something like the following for each frame:
>
> frame0 {
> frame-id = <0>;
> status = "disabled"; /* booted NS, secure firmware has not enabled access */
> reg = <0x... 0x1000>, /* CNTBase0 */
> <0x... 0x1000>; /* CNTPL0Base0 */
> };
>
I don't think you're missing anything. Technically the second view is
not always implemented though. Using the status property should be
sufficient I think.
>> Also to get the frame number, I was thinking maybe we should expand the
>> reg property to have two address cells. Then we could have reg = <0
>> 0xf0001000 0x1000>.
> We could do that, but then you definitely need a more complex ranges property,
> and additional parsing code to handle grabbing it out of the reg property. I
> can't see what it buys us.
Ok. It would mandate node names like "frame@0", "frame@1", but I'll drop
the idea unless someone else finds it useful.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Thu, Apr 11, 2013 at 03:52:52AM +0100, Stephen Boyd wrote:
> On 04/10/13 03:13, Mark Rutland wrote:
> >>>> +
> >>>> +- #size-cells : Must be 1.
> >>>> +
> >>>> +- ranges : Indicates parent and child bus address space are the same.
> >>>> +
> >>> Similarly, what if someone wants to write a more complex mapping for some
> >>> reason?
> >>>
> >>> We should be able to handle it if we use the standard accessors.
> >> Maybe I should just leave this part out? They are standard DT properties
> >> so I could assume DT writers know what to do.
> > I'd be happy with that. It may be worth describing them as "as necessary" or
> > something to that effect.
>
> Ok. I added this and removed the property descriptions:
>
> Note that #address-cells, #size-cells, and ranges shall be present to ensure
> the CPU can address a frame's registers.
Sounds good to me.
>
> > I can see why we need to specify secure/non-secure, but I'm not sure why we
> > need to specify hyp/user/kernel usage. Could we not leave this up to the kernel
> > to figure out?
> >
> > A basic overveiew for those that don't know about the memory mapped timers:
> >
> > * There's one control frame CNTCTLBase. Some registers in this frame are only
> > available for secure accesses, including CNTNSAR which sets whether the
> > counter frames are accessible from the non-secure side.
> >
> > * There are up to 8 timer frames, which have their own CNTVOFF and
> > physical/virtual timers. Each frame CNTBaseN is duplicated at CNTPL0BaseN
> > with CNTVOFF and CNTPL0ACR (which controls PL0 accesses) inaccessible.
> >
> > I can see that we might have frames/registers we can't access (if we were
> > booted on the non-secure side), but I can't see anything limiting whether we
> > use a frame for kernel/hyp/user beyond that. Have I missed something?
> >
> > Could we not have something like the following for each frame:
> >
> > frame0 {
> > frame-id = <0>;
> > status = "disabled"; /* booted NS, secure firmware has not enabled access */
> > reg = <0x... 0x1000>, /* CNTBase0 */
> > <0x... 0x1000>; /* CNTPL0Base0 */
> > };
> >
>
> I don't think you're missing anything. Technically the second view is
> not always implemented though. Using the status property should be
> sufficient I think.
Could we say the reg for the second view is optional?
Might we have a hardware / firmware configuration where the kernel can only access
the secondary view?
>
> >> Also to get the frame number, I was thinking maybe we should expand the
> >> reg property to have two address cells. Then we could have reg = <0
> >> 0xf0001000 0x1000>.
> > We could do that, but then you definitely need a more complex ranges property,
> > and additional parsing code to handle grabbing it out of the reg property. I
> > can't see what it buys us.
>
> Ok. It would mandate node names like "frame@0", "frame@1", but I'll drop
> the idea unless someone else finds it useful.
I see. I'd prefer to use a separate property for the id. Placing it in the reg
and then requiring a mapping sounds like it's going to cause a lot of pain.
Cheers,
Mark.
On 04/11/13 04:24, Mark Rutland wrote:
> Could we say the reg for the second view is optional?
>
Yes, that's already covered in the binding.
> Might we have a hardware / firmware configuration where the kernel can only access
> the secondary view?
>
I don't see how this is possible. The CNTACRn register controls secure
vs. non-secure access for particular registers in a frame regardless of
which view is used. The CNTPL0ACRn is not a security restricted register
and it can only restrict access to certain registers in the second view.
No combination of settings in these registers can restrict access to
only the second view in a frame with two views.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation