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

From: Robin Murphy
Date: Fri Jan 29 2016 - 14:45:09 EST

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@xxxxxxx>
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);
- return;
+ goto out;

- if (cnttidr & CNTTIDR_VIRT(n)) {
+ /* Try enabling everything, and see what sticks */
+ 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))) {
best_frame = frame;
arch_timer_mem_use_virtual = true;
+ if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
+ continue;
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:

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.


From: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
Subject: [PATCH] clocksource/arm_arch_timer: Map frame with

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@xxxxxxxxxxxxxx>
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;