* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-01 12:00 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2016-02-01 12:00 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-01 12:00 ` Robin Murphy
@ 2016-02-01 13:44 ` Mark Rutland
-1 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2016-02-01 13:44 UTC (permalink / raw)
To: Robin Murphy; +Cc: linux-kernel, linux-arm-kernel, daniel.lezcano, tglx, sboyd
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-01 13:44 ` Mark Rutland
0 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2016-02-01 13:44 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-01 12:00 ` Robin Murphy
@ 2016-02-01 19:58 ` Stephen Boyd
-1 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2016-02-01 19:58 UTC (permalink / raw)
To: Robin Murphy
Cc: linux-kernel, linux-arm-kernel, daniel.lezcano, tglx, mark.rutland
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>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-01 19:58 ` Stephen Boyd
0 siblings, 0 replies; 18+ messages in thread
From: Stephen Boyd @ 2016-02-01 19:58 UTC (permalink / raw)
To: linux-arm-kernel
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>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-01 19:58 ` Stephen Boyd
@ 2016-02-05 15:29 ` Robin Murphy
-1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2016-02-05 15:29 UTC (permalink / raw)
To: Stephen Boyd, daniel.lezcano
Cc: linux-kernel, linux-arm-kernel, tglx, mark.rutland
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-05 15:29 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2016-02-05 15:29 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-05 15:29 ` Robin Murphy
@ 2016-02-05 15:32 ` Daniel Lezcano
-1 siblings, 0 replies; 18+ messages in thread
From: Daniel Lezcano @ 2016-02-05 15:32 UTC (permalink / raw)
To: Robin Murphy, Stephen Boyd
Cc: linux-kernel, linux-arm-kernel, tglx, mark.rutland
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
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-05 15:32 ` Daniel Lezcano
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Lezcano @ 2016-02-05 15:32 UTC (permalink / raw)
To: linux-arm-kernel
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
--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-05 15:32 ` Daniel Lezcano
@ 2016-02-05 15:34 ` Robin Murphy
-1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2016-02-05 15:34 UTC (permalink / raw)
To: Daniel Lezcano, Stephen Boyd
Cc: linux-kernel, linux-arm-kernel, tglx, mark.rutland
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
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-05 15:34 ` Robin Murphy
0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2016-02-05 15:34 UTC (permalink / raw)
To: linux-arm-kernel
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
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-01 12:00 ` Robin Murphy
@ 2016-02-08 14:00 ` Daniel Lezcano
-1 siblings, 0 replies; 18+ messages in thread
From: Daniel Lezcano @ 2016-02-08 14:00 UTC (permalink / raw)
To: Robin Murphy, linux-kernel, linux-arm-kernel
Cc: tglx, mark.rutland, sboyd, Marc Zyngier
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);
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-08 14:00 ` Daniel Lezcano
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Lezcano @ 2016-02-08 14:00 UTC (permalink / raw)
To: linux-arm-kernel
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);
>
--
<http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-08 14:00 ` Daniel Lezcano
@ 2016-02-08 14:09 ` Marc Zyngier
-1 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2016-02-08 14:09 UTC (permalink / raw)
To: Daniel Lezcano, Robin Murphy, linux-kernel, linux-arm-kernel
Cc: tglx, mark.rutland, sboyd
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.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-08 14:09 ` Marc Zyngier
0 siblings, 0 replies; 18+ messages in thread
From: Marc Zyngier @ 2016-02-08 14:09 UTC (permalink / raw)
To: linux-arm-kernel
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.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
2016-02-01 12:00 ` Robin Murphy
@ 2016-02-08 14:30 ` Daniel Lezcano
-1 siblings, 0 replies; 18+ messages in thread
From: Daniel Lezcano @ 2016-02-08 14:30 UTC (permalink / raw)
To: Robin Murphy, linux-kernel, linux-arm-kernel; +Cc: tglx, mark.rutland, sboyd
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Enable and verify MMIO access
@ 2016-02-08 14:30 ` Daniel Lezcano
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Lezcano @ 2016-02-08 14:30 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread