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

FWIW:

Acked-by: Mark Rutland <mark.rutland@xxxxxxx>

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
>