On Mon, Feb 01, 2016 at 12:00:48PM +0000, Robin Murphy wrote: > So far, we have been blindly assuming that having access to a > memory-mapped timer frame implies that the individual elements of that > frame frame are already enabled. Whilst it's the firmware's job to give > us non-secure access to frames in the first place, we should not rely > on implementations always being generous enough to also configure CNTACR > for those non-secure frames (e.g. [1]). > > Explicitly enable feature-level access per-frame, and verify that the > access we want is really implemented before trying to make use of it. > > [1]:https://github.com/ARM-software/tf-issues/issues/170 > > Signed-off-by: Robin Murphy <[email protected]>
FWIW: Acked-by: Mark Rutland <[email protected]> In future, we could probably allow for using frames as counters without timer support and vice-versa, but as-is this at leasts validates the assumptions we make today. Thanks, Mark. > --- > > Changes from v1: > - Remove CNTACR_RFRQ check since the "no access, CNTFRQ reads as zero" > case can be handled the same way the driver deals with the "CNTFRQ > accessible, but not programmed" case. > - Reword the commit message to make more sense. > > drivers/clocksource/arm_arch_timer.c | 36 > ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/drivers/clocksource/arm_arch_timer.c > b/drivers/clocksource/arm_arch_timer.c > index c64d543..7c567f0 100644 > --- a/drivers/clocksource/arm_arch_timer.c > +++ b/drivers/clocksource/arm_arch_timer.c > @@ -32,6 +32,14 @@ > #define CNTTIDR 0x08 > #define CNTTIDR_VIRT(n) (BIT(1) << ((n) * 4)) > > +#define CNTACR(n) (0x40 + ((n) * 4)) > +#define CNTACR_RPCT BIT(0) > +#define CNTACR_RVCT BIT(1) > +#define CNTACR_RFRQ BIT(2) > +#define CNTACR_RVOFF BIT(3) > +#define CNTACR_RWVT BIT(4) > +#define CNTACR_RWPT BIT(5) > + > #define CNTVCT_LO 0x08 > #define CNTVCT_HI 0x0c > #define CNTFRQ 0x10 > @@ -757,7 +765,6 @@ static void __init arch_timer_mem_init(struct device_node > *np) > } > > cnttidr = readl_relaxed(cntctlbase + CNTTIDR); > - iounmap(cntctlbase); > > /* > * Try to find a virtual capable frame. Otherwise fall back to a > @@ -765,20 +772,31 @@ static void __init arch_timer_mem_init(struct > device_node *np) > */ > for_each_available_child_of_node(np, frame) { > int n; > + u32 cntacr; > > if (of_property_read_u32(frame, "frame-number", &n)) { > pr_err("arch_timer: Missing frame-number\n"); > - of_node_put(best_frame); > of_node_put(frame); > - return; > + goto out; > } > > - if (cnttidr & CNTTIDR_VIRT(n)) { > + /* Try enabling everything, and see what sticks */ > + cntacr = CNTACR_RFRQ | CNTACR_RWPT | CNTACR_RPCT | > + CNTACR_RWVT | CNTACR_RVOFF | CNTACR_RVCT; > + writel_relaxed(cntacr, cntctlbase + CNTACR(n)); > + cntacr = readl_relaxed(cntctlbase + CNTACR(n)); > + > + if ((cnttidr & CNTTIDR_VIRT(n)) && > + !(~cntacr & (CNTACR_RWVT | CNTACR_RVCT))) { > of_node_put(best_frame); > best_frame = frame; > arch_timer_mem_use_virtual = true; > break; > } > + > + if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT)) > + continue; > + > of_node_put(best_frame); > best_frame = of_node_get(frame); > } > @@ -786,24 +804,26 @@ static void __init arch_timer_mem_init(struct > device_node *np) > base = arch_counter_base = of_iomap(best_frame, 0); > if (!base) { > pr_err("arch_timer: Can't map frame's registers\n"); > - of_node_put(best_frame); > - return; > + goto out; > } > > if (arch_timer_mem_use_virtual) > irq = irq_of_parse_and_map(best_frame, 1); > else > irq = irq_of_parse_and_map(best_frame, 0); > - of_node_put(best_frame); > + > if (!irq) { > pr_err("arch_timer: Frame missing %s irq", > arch_timer_mem_use_virtual ? "virt" : "phys"); > - return; > + goto out; > } > > arch_timer_detect_rate(base, np); > arch_timer_mem_register(base, irq); > arch_timer_common_init(); > +out: > + iounmap(cntctlbase); > + of_node_put(best_frame); > } > CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem", > arch_timer_mem_init); > -- > 2.7.0.25.gfc10eb5.dirty >
