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

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

[Robin Murphy]

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

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

[Robin Murphy]

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

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

[Mark Rutland]

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
> 

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

[Mark Rutland]

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
> 

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

[Stephen Boyd]

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

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

[Stephen Boyd]

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

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

[Robin Murphy]

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.

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

[Robin Murphy]

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.

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

[Daniel Lezcano]

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

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

[Daniel Lezcano]

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

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

[Robin Murphy]

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
>
>
>

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

[Robin Murphy]

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
>
>
>

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

[Daniel Lezcano]

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

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

[Daniel Lezcano]

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

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

[Marc Zyngier]

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...

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

[Marc Zyngier]

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...

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

[Daniel Lezcano]

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

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

[Daniel Lezcano]

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