| Message ID | 71bf617f4083b116b2aeab24f13fc5fed99a816f.1454327031.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <robin.murphy@arm.com> FWIW: Acked-by: Mark Rutland <mark.rutland@arm.com> 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 >
On 02/01, 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 <robin.murphy@arm.com> > --- Reviewed-by: Stephen Boyd <sboyd@codeaurora.org> Tested-by: Stephen Boyd <sboyd@codeaurora.org>
On 01/02/16 19:58, Stephen Boyd wrote: > On 02/01, 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 <robin.murphy@arm.com> >> --- > > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org> > Tested-by: Stephen Boyd <sboyd@codeaurora.org> Great, thanks! Daniel, am I right in hoping this is something you'll pick up, or should I be resending it to arm-soc? Thanks, Robin.
On 02/05/2016 04:29 PM, Robin Murphy wrote: > On 01/02/16 19:58, Stephen Boyd wrote: >> On 02/01, 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 <robin.murphy@arm.com> >>> --- >> >> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org> >> Tested-by: Stephen Boyd <sboyd@codeaurora.org> > > Great, thanks! > > Daniel, am I right in hoping this is something you'll pick up, or should > I be resending it to arm-soc? I will be reviewing timers patches next week. I will take care of this one. Thanks -- Daniel
On 05/02/16 15:32, Daniel Lezcano wrote: > On 02/05/2016 04:29 PM, Robin Murphy wrote: >> On 01/02/16 19:58, Stephen Boyd wrote: >>> On 02/01, 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 <robin.murphy@arm.com> >>>> --- >>> >>> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org> >>> Tested-by: Stephen Boyd <sboyd@codeaurora.org> >> >> Great, thanks! >> >> Daniel, am I right in hoping this is something you'll pick up, or should >> I be resending it to arm-soc? > > I will be reviewing timers patches next week. I will take care of this one. Cool, thanks for the confirmation. Robin. > > Thanks > -- Daniel > > >
On 02/01/2016 01:00 PM, 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 <robin.murphy@arm.com> > --- Hi Marc, can you give your opinion on this patch ? Thanks -- Daniel > 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); >
On 08/02/16 14:00, Daniel Lezcano wrote: > On 02/01/2016 01:00 PM, 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 <robin.murphy@arm.com> >> --- > > Hi Marc, > > can you give your opinion on this patch ? This seems to do the right thing, so FWIW: Acked-by: Marc Zyngier <marc.zyngier@arm.com> Thanks, M.
On 02/01/2016 01:00 PM, 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 <robin.murphy@arm.com> > --- Applied to my tree for 4.6. Thanks - Daniel
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);
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 <robin.murphy@arm.com> --- 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(-)