[PATCH] clocksource/arm_arch_timer: Enable and verify MMIO access

[PATCH] clocksource/arm_arch_timer: Enable and verify MMIO access

[Robin Murphy]

So far, we have been blindly assuming that if a memory-mapped timer
frame exists, then we have access to it. Whilst it's the firmware's
job to give us non-secure access to frames in the first place, we
should not rely on it always being generous enough to also configure
CNTACR if it's not even using those frames itself.

Explicitly enable feature-level access per-frame, and verify that the
access we want is really implemented before trying to make use of it.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 39 ++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index c64d543..c88485d 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,34 @@ 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 (~cntacr & CNTACR_RFRQ)
+			continue;
+
+		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 +807,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

Re: [PATCH] clocksource/arm_arch_timer: Enable and verify MMIO access

[Stephen Boyd]

On 01/29, Robin Murphy wrote:
> So far, we have been blindly assuming that if a memory-mapped timer
> frame exists, then we have access to it. Whilst it's the firmware's
> job to give us non-secure access to frames in the first place, we
> should not rely on it always being generous enough to also configure
> CNTACR if it's not even using those frames itself.

Hm, that first sentence is sort of misleading. We've been blindly
assuming that the firmware has configured CNTACR to have the
correct bits set for virtual/physical access. We've always relied
on status = "disabled" to figure out if we can access an entire
frame or not.

> 
> Explicitly enable feature-level access per-frame, and verify that the
> access we want is really implemented before trying to make use of it.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/clocksource/arm_arch_timer.c | 39 ++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index c64d543..c88485d 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -765,20 +772,34 @@ 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 (~cntacr & CNTACR_RFRQ)
> +			continue;

Do we need this check? If we can't read the frequency we fall
back to looking for the DT property, so it shouldn't matter if we
can't read the hardware there.

> +
> +		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);
>  	}

Otherwise the patch looks fine and passes some light testing on
qcom devices.

BTW, I'd like to add this patch on top so that we get some info
in /proc/iomem about which frame region is in use.

---8<---
From: Stephen Boyd <sboyd@codeaurora.org>
Subject: [PATCH] clocksource/arm_arch_timer: Map frame with
 of_io_request_and_map()

Let's use the of_io_request_and_map() API so that the frame
region is protected and shows up in /proc/iomem.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clocksource/arm_arch_timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index c88485d489bf..59a08fd4f76a 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -804,7 +804,8 @@ static void __init arch_timer_mem_init(struct device_node *np)
 		best_frame = of_node_get(frame);
 	}
 
-	base = arch_counter_base = of_iomap(best_frame, 0);
+	base = arch_counter_base = of_io_request_and_map(best_frame, 0,
+							 "arch_mem_timer");
 	if (!base) {
 		pr_err("arch_timer: Can't map frame's registers\n");
 		goto out;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clocksource/arm_arch_timer: Enable and verify MMIO access

[Robin Murphy]

On 29/01/16 18:30, Stephen Boyd wrote:
> On 01/29, Robin Murphy wrote:
>> So far, we have been blindly assuming that if a memory-mapped timer
>> frame exists, then we have access to it. Whilst it's the firmware's
>> job to give us non-secure access to frames in the first place, we
>> should not rely on it always being generous enough to also configure
>> CNTACR if it's not even using those frames itself.
>
> Hm, that first sentence is sort of misleading. We've been blindly
> assuming that the firmware has configured CNTACR to have the
> correct bits set for virtual/physical access. We've always relied
> on status = "disabled" to figure out if we can access an entire
> frame or not.

Yeah, now that I read it back that sentence is nonsense for anything 
other than the very specific ideas of "frame" and "exists" that were 
passing through my head at some point last week - how about this instead?

"So far, we have been blindly assuming that having access to a 
memory-mapped timer frame implies that the individual elements of that 
frame are already enabled."

>>
>> Explicitly enable feature-level access per-frame, and verify that the
>> access we want is really implemented before trying to make use of it.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/clocksource/arm_arch_timer.c | 39 ++++++++++++++++++++++++++++--------
>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index c64d543..c88485d 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -765,20 +772,34 @@ 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 (~cntacr & CNTACR_RFRQ)
>> +			continue;
>
> Do we need this check? If we can't read the frequency we fall
> back to looking for the DT property, so it shouldn't matter if we
> can't read the hardware there.

I was really just playing safe to start with. If we don't have cause to 
care about the difference between not having access vs. not having a 
frequency programmed then I'd agree it can probably go.

>> +
>> +		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);
>>   	}
>
> Otherwise the patch looks fine and passes some light testing on
> qcom devices.

Great, thanks. The Trusted Firmware guys' warning shot has gone upstream 
already, if it helps:

https://github.com/ARM-software/arm-trusted-firmware/commit/01fc3f7300e86b0b672977133c3028d638d0c672

> BTW, I'd like to add this patch on top so that we get some info
> in /proc/iomem about which frame region is in use.

It's about time I educated myself on what all the resource stuff really 
means, but superficially it seems reasonable, and it certainly makes the 
expected "2a830000-2a83ffff : arch_mem_timer" show up on my Juno.

Thanks,
Robin.

> ---8<---
> From: Stephen Boyd <sboyd@codeaurora.org>
> Subject: [PATCH] clocksource/arm_arch_timer: Map frame with
>   of_io_request_and_map()
>
> Let's use the of_io_request_and_map() API so that the frame
> region is protected and shows up in /proc/iomem.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>   drivers/clocksource/arm_arch_timer.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index c88485d489bf..59a08fd4f76a 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -804,7 +804,8 @@ static void __init arch_timer_mem_init(struct device_node *np)
>   		best_frame = of_node_get(frame);
>   	}
>
> -	base = arch_counter_base = of_iomap(best_frame, 0);
> +	base = arch_counter_base = of_io_request_and_map(best_frame, 0,
> +							 "arch_mem_timer");
>   	if (!base) {
>   		pr_err("arch_timer: Can't map frame's registers\n");
>   		goto out;
>

Re: [PATCH] clocksource/arm_arch_timer: Enable and verify MMIO access

[Stephen Boyd]

On 01/29, Robin Murphy wrote:
> On 29/01/16 18:30, Stephen Boyd wrote:
> >On 01/29, Robin Murphy wrote:
> >Hm, that first sentence is sort of misleading. We've been blindly
> >assuming that the firmware has configured CNTACR to have the
> >correct bits set for virtual/physical access. We've always relied
> >on status = "disabled" to figure out if we can access an entire
> >frame or not.
> 
> Yeah, now that I read it back that sentence is nonsense for anything
> other than the very specific ideas of "frame" and "exists" that were
> passing through my head at some point last week - how about this
> instead?
> 
> "So far, we have been blindly assuming that having access to a
> memory-mapped timer frame implies that the individual elements of
> that frame are already enabled."

Sounds good.

> 
> >>
> >>Explicitly enable feature-level access per-frame, and verify that the
> >>access we want is really implemented before trying to make use of it.
> >>
> >>Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >>---
> >>  drivers/clocksource/arm_arch_timer.c | 39 ++++++++++++++++++++++++++++--------
> >>  1 file changed, 31 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> >>index c64d543..c88485d 100644
> >>--- a/drivers/clocksource/arm_arch_timer.c
> >>+++ b/drivers/clocksource/arm_arch_timer.c
> >>@@ -765,20 +772,34 @@ 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 (~cntacr & CNTACR_RFRQ)
> >>+			continue;
> >
> >Do we need this check? If we can't read the frequency we fall
> >back to looking for the DT property, so it shouldn't matter if we
> >can't read the hardware there.
> 
> I was really just playing safe to start with. If we don't have cause
> to care about the difference between not having access vs. not
> having a frequency programmed then I'd agree it can probably go.
> 

Yeah I'm mostly worried that we'll break something somewhere
because that bit doesn't stick and it's the only frame we can
use. Or we can give it a shot and then remove clock-frequency
from the dts binding for mmio timers.

> 
> Great, thanks. The Trusted Firmware guys' warning shot has gone
> upstream already, if it helps:
> 
> https://github.com/ARM-software/arm-trusted-firmware/commit/01fc3f7300e86b0b672977133c3028d638d0c672

Ah I see. It would be nice if that bug gave a reason why it
should be done, instead of just saying it must be this way. O
well.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project