[PATCH 0/4] Memory mapped architected timers

[PATCH 0/4] Memory mapped architected timers

[Stephen Boyd]

This patchset adds support for memory mapped architected timers. We
don't have any other global broadcast timer in our system, so we use the
mmio timer during low power modes. The first patch is the binding.
The next two patches lay some groundwork so that the last patch is simpler.
The final patch adds support for mmio timers.

Patches are based on a recent patch from Mark that removes the
physical count reading (clocksource: arch_timer: use virtual counter,
message id <1364404312-4427-4-git-send-email-mark.rutland@arm.com>).

Stephen Boyd (4):
  Documentation: Add memory mapped ARM architected timer binding
  ARM: arch_timers: Pass clock event to set_mode callback
  clocksource: arch_timer: Push the read/write wrappers deeper
  clocksource: arch_timer: Add support for memory mapped timers

 .../devicetree/bindings/arm/arch_timer.txt         |  62 ++-
 arch/arm/include/asm/arch_timer.h                  |   5 +-
 arch/arm64/include/asm/arch_timer.h                |   4 +-
 drivers/clocksource/arm_arch_timer.c               | 451 +++++++++++++++++----
 include/clocksource/arm_arch_timer.h               |   4 +-
 5 files changed, 448 insertions(+), 78 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/4] Documentation: Add memory mapped ARM architected timer binding

[Stephen Boyd]

Add a binding for the arm architected timer hardware's memory
mapped interface. The mmio timer hardware is made up of one base
frame and a collection of up to 8 timer frames, where each of the
8 timer frames can have either one or two views. A frame
typically maps to a privilege level (user/kernel, hypervisor,
secure). The first view has full access to the registers within a
frame, while the second view can be restricted to particular
registers within a frame. Each frame must support a physical
timer. It's optional for a frame to support a virtual timer.

Cc: devicetree-discuss@lists.ozlabs.org
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <Marc.Zyngier@arm.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 .../devicetree/bindings/arm/arch_timer.txt         | 62 ++++++++++++++++++++--
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
index 20746e5..69ef711 100644
--- a/Documentation/devicetree/bindings/arm/arch_timer.txt
+++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
@@ -1,10 +1,14 @@
 * ARM architected timer
 
-ARM cores may have a per-core architected timer, which provides per-cpu timers.
+ARM cores may have a per-core architected timer, which provides per-cpu timers,
+or a memory mapped architected timer, which provides up to 8 frames with a
+physical and optional virtual timer per frame.
 
-The timer is attached to a GIC to deliver its per-processor interrupts.
+The per-core architected timer is attached to a GIC to deliver its
+per-processor interrupts via PPIs. The memory mapped timer is attached to a GIC
+to deliver its interrupts via SPIs.
 
-** Timer node properties:
+** CP15 Timer node properties:
 
 - compatible : Should at least contain one of
 	"arm,armv7-timer"
@@ -26,3 +30,55 @@ Example:
 			     <1 10 0xf08>;
 		clock-frequency = <100000000>;
 	};
+
+** Memory mapped timer node properties
+
+- compatible : Should at least contain "arm,armv7-timer-mem".
+
+- #address-cells : Must be 1.
+
+- #size-cells : Must be 1.
+
+- ranges : Indicates parent and child bus address space are the same.
+
+- clock-frequency : The frequency of the main counter, in Hz. Optional.
+
+- reg : The control frame base address.
+
+Frame:
+
+- frame-id: Encoded as follows:
+		bits[3:0]  frame number: 0 to 7.
+		bits[10:8] frame usage:
+			0 - user/kernel
+			1 - hyp
+			2 - secure
+
+- interrupts : Interrupt list for physical and virtual timers in that order.
+  The virtual timer interrupt is optional.
+
+- reg : The first and second view base addresses in that order. The second view
+  base address is optional.
+
+Example:
+
+	timer@f0000000 {
+		compatible = "arm,armv7-timer-mem";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		reg = <0xf0000000 0x1000>;
+		clock-frequency = <50000000>;
+		frame0@f0001000 {
+			frame-id = <0x0>
+			interrupts = <0 13 0x8>,
+				     <0 14 0x8>;
+			reg = <0xf0001000 0x1000>,
+			      <0xf0002000 0x1000>;
+		};
+		frame1@f0003000 {
+			frame-id = <0x11>
+			interrupts = <0 15 0x8>;
+			reg = <0xf0003000 0x1000>;
+		};
+	};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/4] Documentation: Add memory mapped ARM architected timer binding

[Mark Rutland]

Hi Stephen,

On Tue, Apr 09, 2013 at 03:30:20AM +0100, Stephen Boyd wrote:
> Add a binding for the arm architected timer hardware's memory
> mapped interface. The mmio timer hardware is made up of one base
> frame and a collection of up to 8 timer frames, where each of the
> 8 timer frames can have either one or two views. A frame
> typically maps to a privilege level (user/kernel, hypervisor,
> secure). The first view has full access to the registers within a
> frame, while the second view can be restricted to particular
> registers within a frame. Each frame must support a physical
> timer. It's optional for a frame to support a virtual timer.
> 
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Marc Zyngier <Marc.Zyngier@arm.com>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  .../devicetree/bindings/arm/arch_timer.txt         | 62 ++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
> index 20746e5..69ef711 100644
> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
> @@ -1,10 +1,14 @@
>  * ARM architected timer
>  
> -ARM cores may have a per-core architected timer, which provides per-cpu timers.
> +ARM cores may have a per-core architected timer, which provides per-cpu timers,
> +or a memory mapped architected timer, which provides up to 8 frames with a
> +physical and optional virtual timer per frame.
>  
> -The timer is attached to a GIC to deliver its per-processor interrupts.
> +The per-core architected timer is attached to a GIC to deliver its
> +per-processor interrupts via PPIs. The memory mapped timer is attached to a GIC
> +to deliver its interrupts via SPIs.
>  
> -** Timer node properties:
> +** CP15 Timer node properties:
>  
>  - compatible : Should at least contain one of
>  	"arm,armv7-timer"
> @@ -26,3 +30,55 @@ Example:
>  			     <1 10 0xf08>;
>  		clock-frequency = <100000000>;
>  	};
> +
> +** Memory mapped timer node properties
> +
> +- compatible : Should at least contain "arm,armv7-timer-mem".
> +
> +- #address-cells : Must be 1.

What about LPAE systems?

How about something like the following:

#address-cells : If the ranges property is empty, the same value as the
                 parent node's #address-cells property. Otherwise, a
		 value such that the ranges property specifies a
		 mapping to the parent node's address space.            

> +
> +- #size-cells : Must be 1.
> +
> +- ranges : Indicates parent and child bus address space are the same.
> +

Similarly, what if someone wants to write a more complex mapping for some
reason?

We should be able to handle it if we use the standard accessors.

> +- clock-frequency : The frequency of the main counter, in Hz. Optional.
> +
> +- reg : The control frame base address.
> +
> +Frame:
> +
> +- frame-id: Encoded as follows:
> +		bits[3:0]  frame number: 0 to 7.
> +		bits[10:8] frame usage:
> +			0 - user/kernel
> +			1 - hyp
> +			2 - secure
> +

Could we not just have a disabled status property for those frames claimed by a
higher level (either secure firmware or hypervisor)? Or have I missed something
here?

Then we'd just have a frame-number property, which is easier for humans to read
and understand.

> +- interrupts : Interrupt list for physical and virtual timers in that order.
> +  The virtual timer interrupt is optional.
> +
> +- reg : The first and second view base addresses in that order. The second view
> +  base address is optional.
> +
> +Example:
> +
> +	timer@f0000000 {
> +		compatible = "arm,armv7-timer-mem";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +		reg = <0xf0000000 0x1000>;
> +		clock-frequency = <50000000>;
> +		frame0@f0001000 {
> +			frame-id = <0x0>
> +			interrupts = <0 13 0x8>,
> +				     <0 14 0x8>;
> +			reg = <0xf0001000 0x1000>,
> +			      <0xf0002000 0x1000>;
> +		};
> +		frame1@f0003000 {
> +			frame-id = <0x11>
> +			interrupts = <0 15 0x8>;
> +			reg = <0xf0003000 0x1000>;
> +		};
> +	};
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 

Mark.

Re: [PATCH 1/4] Documentation: Add memory mapped ARM architected timer binding

[Stephen Boyd]

On 04/09/13 02:08, Mark Rutland wrote:
> On Tue, Apr 09, 2013 at 03:30:20AM +0100, Stephen Boyd wrote:
>>  
>> -** Timer node properties:
>> +** CP15 Timer node properties:
>>  
>>  - compatible : Should at least contain one of
>>  	"arm,armv7-timer"
>> @@ -26,3 +30,55 @@ Example:
>>  			     <1 10 0xf08>;
>>  		clock-frequency = <100000000>;
>>  	};
>> +
>> +** Memory mapped timer node properties
>> +
>> +- compatible : Should at least contain "arm,armv7-timer-mem".
>> +
>> +- #address-cells : Must be 1.
> What about LPAE systems?
>
> How about something like the following:
>
> #address-cells : If the ranges property is empty, the same value as the
>                  parent node's #address-cells property. Otherwise, a
> 		 value such that the ranges property specifies a
> 		 mapping to the parent node's address space.            

Yes that is much better. I wasn't trying to preclude LPAE or 64 bit systems.

>> +
>> +- #size-cells : Must be 1.
>> +
>> +- ranges : Indicates parent and child bus address space are the same.
>> +
> Similarly, what if someone wants to write a more complex mapping for some
> reason?
>
> We should be able to handle it if we use the standard accessors.

Maybe I should just leave this part out? They are standard DT properties
so I could assume DT writers know what to do.

>> +- clock-frequency : The frequency of the main counter, in Hz. Optional.
>> +
>> +- reg : The control frame base address.
>> +
>> +Frame:
>> +
>> +- frame-id: Encoded as follows:
>> +		bits[3:0]  frame number: 0 to 7.
>> +		bits[10:8] frame usage:
>> +			0 - user/kernel
>> +			1 - hyp
>> +			2 - secure
>> +
> Could we not just have a disabled status property for those frames claimed by a
> higher level (either secure firmware or hypervisor)? Or have I missed something
> here?

Using disabled status would work. I was also thinking maybe we should
use a compatible string in each frame's node. Then we could match
against compatible children like "frame-user", "frame-kernel",
"frame-hyp", "frame-secure", "frame-user-kernel", etc. It allows us
flexibility if we should need to add something else in the future.

Also to get the frame number, I was thinking maybe we should expand the
reg property to have two address cells. Then we could have reg = <0
0xf0001000 0x1000>.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/4] Documentation: Add memory mapped ARM architected timer binding

[Mark Rutland]

On Tue, Apr 09, 2013 at 05:42:38PM +0100, Stephen Boyd wrote:
> On 04/09/13 02:08, Mark Rutland wrote:
> > On Tue, Apr 09, 2013 at 03:30:20AM +0100, Stephen Boyd wrote:
> >>  
> >> -** Timer node properties:
> >> +** CP15 Timer node properties:
> >>  
> >>  - compatible : Should at least contain one of
> >>  	"arm,armv7-timer"
> >> @@ -26,3 +30,55 @@ Example:
> >>  			     <1 10 0xf08>;
> >>  		clock-frequency = <100000000>;
> >>  	};
> >> +
> >> +** Memory mapped timer node properties
> >> +
> >> +- compatible : Should at least contain "arm,armv7-timer-mem".
> >> +
> >> +- #address-cells : Must be 1.
> > What about LPAE systems?
> >
> > How about something like the following:
> >
> > #address-cells : If the ranges property is empty, the same value as the
> >                  parent node's #address-cells property. Otherwise, a
> > 		 value such that the ranges property specifies a
> > 		 mapping to the parent node's address space.            
> 
> Yes that is much better. I wasn't trying to preclude LPAE or 64 bit systems.

Great.

> 
> >> +
> >> +- #size-cells : Must be 1.
> >> +
> >> +- ranges : Indicates parent and child bus address space are the same.
> >> +
> > Similarly, what if someone wants to write a more complex mapping for some
> > reason?
> >
> > We should be able to handle it if we use the standard accessors.
> 
> Maybe I should just leave this part out? They are standard DT properties
> so I could assume DT writers know what to do.

I'd be happy with that. It may be worth describing them as "as necessary" or
something to that effect.

> 
> >> +- clock-frequency : The frequency of the main counter, in Hz. Optional.
> >> +
> >> +- reg : The control frame base address.
> >> +
> >> +Frame:
> >> +
> >> +- frame-id: Encoded as follows:
> >> +		bits[3:0]  frame number: 0 to 7.
> >> +		bits[10:8] frame usage:
> >> +			0 - user/kernel
> >> +			1 - hyp
> >> +			2 - secure
> >> +
> > Could we not just have a disabled status property for those frames claimed by a
> > higher level (either secure firmware or hypervisor)? Or have I missed something
> > here?
> 
> Using disabled status would work. I was also thinking maybe we should
> use a compatible string in each frame's node. Then we could match
> against compatible children like "frame-user", "frame-kernel",
> "frame-hyp", "frame-secure", "frame-user-kernel", etc. It allows us
> flexibility if we should need to add something else in the future.

I can see why we need to specify secure/non-secure, but I'm not sure why we
need to specify hyp/user/kernel usage. Could we not leave this up to the kernel
to figure out?

A basic overveiew for those that don't know about the memory mapped timers:

* There's one control frame CNTCTLBase. Some registers in this frame are only
  available for secure accesses, including CNTNSAR which sets whether the
  counter frames are accessible from the non-secure side.

* There are up to 8 timer frames, which have their own CNTVOFF and
  physical/virtual timers. Each frame CNTBaseN is duplicated at CNTPL0BaseN
  with CNTVOFF and CNTPL0ACR (which controls PL0 accesses) inaccessible.

I can see that we might have frames/registers we can't access (if we were
booted on the non-secure side), but I can't see anything limiting whether we
use a frame for kernel/hyp/user beyond that. Have I missed something?

Could we not have something like the following for each frame:

frame0 {
	frame-id = <0>;
	status = "disabled"; /* booted NS, secure firmware has not enabled access */
	reg = <0x... 0x1000>, /* CNTBase0 */
	      <0x... 0x1000>; /* CNTPL0Base0 */
};

> 
> Also to get the frame number, I was thinking maybe we should expand the
> reg property to have two address cells. Then we could have reg = <0
> 0xf0001000 0x1000>.

We could do that, but then you definitely need a more complex ranges property,
and additional parsing code to handle grabbing it out of the reg property. I
can't see what it buys us.

Mark.

Re: [PATCH 1/4] Documentation: Add memory mapped ARM architected timer binding

[Stephen Boyd]

On 04/10/13 03:13, Mark Rutland wrote:
>>>> +
>>>> +- #size-cells : Must be 1.
>>>> +
>>>> +- ranges : Indicates parent and child bus address space are the same.
>>>> +
>>> Similarly, what if someone wants to write a more complex mapping for some
>>> reason?
>>>
>>> We should be able to handle it if we use the standard accessors.
>> Maybe I should just leave this part out? They are standard DT properties
>> so I could assume DT writers know what to do.
> I'd be happy with that. It may be worth describing them as "as necessary" or
> something to that effect.

Ok. I added this and removed the property descriptions:

Note that #address-cells, #size-cells, and ranges shall be present to ensure
the CPU can address a frame's registers.

> I can see why we need to specify secure/non-secure, but I'm not sure why we
> need to specify hyp/user/kernel usage. Could we not leave this up to the kernel
> to figure out?
>
> A basic overveiew for those that don't know about the memory mapped timers:
>
> * There's one control frame CNTCTLBase. Some registers in this frame are only
>   available for secure accesses, including CNTNSAR which sets whether the
>   counter frames are accessible from the non-secure side.
>
> * There are up to 8 timer frames, which have their own CNTVOFF and
>   physical/virtual timers. Each frame CNTBaseN is duplicated at CNTPL0BaseN
>   with CNTVOFF and CNTPL0ACR (which controls PL0 accesses) inaccessible.
>
> I can see that we might have frames/registers we can't access (if we were
> booted on the non-secure side), but I can't see anything limiting whether we
> use a frame for kernel/hyp/user beyond that. Have I missed something?
>
> Could we not have something like the following for each frame:
>
> frame0 {
> 	frame-id = <0>;
> 	status = "disabled"; /* booted NS, secure firmware has not enabled access */
> 	reg = <0x... 0x1000>, /* CNTBase0 */
> 	      <0x... 0x1000>; /* CNTPL0Base0 */
> };
>

I don't think you're missing anything. Technically the second view is
not always implemented though. Using the status property should be
sufficient I think.

>> Also to get the frame number, I was thinking maybe we should expand the
>> reg property to have two address cells. Then we could have reg = <0
>> 0xf0001000 0x1000>.
> We could do that, but then you definitely need a more complex ranges property,
> and additional parsing code to handle grabbing it out of the reg property. I
> can't see what it buys us.

Ok. It would mandate node names like "frame@0", "frame@1", but I'll drop
the idea unless someone else finds it useful.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 1/4] Documentation: Add memory mapped ARM architected timer binding

[Mark Rutland]

On Thu, Apr 11, 2013 at 03:52:52AM +0100, Stephen Boyd wrote:
> On 04/10/13 03:13, Mark Rutland wrote:
> >>>> +
> >>>> +- #size-cells : Must be 1.
> >>>> +
> >>>> +- ranges : Indicates parent and child bus address space are the same.
> >>>> +
> >>> Similarly, what if someone wants to write a more complex mapping for some
> >>> reason?
> >>>
> >>> We should be able to handle it if we use the standard accessors.
> >> Maybe I should just leave this part out? They are standard DT properties
> >> so I could assume DT writers know what to do.
> > I'd be happy with that. It may be worth describing them as "as necessary" or
> > something to that effect.
> 
> Ok. I added this and removed the property descriptions:
> 
> Note that #address-cells, #size-cells, and ranges shall be present to ensure
> the CPU can address a frame's registers.

Sounds good to me.

> 
> > I can see why we need to specify secure/non-secure, but I'm not sure why we
> > need to specify hyp/user/kernel usage. Could we not leave this up to the kernel
> > to figure out?
> >
> > A basic overveiew for those that don't know about the memory mapped timers:
> >
> > * There's one control frame CNTCTLBase. Some registers in this frame are only
> >   available for secure accesses, including CNTNSAR which sets whether the
> >   counter frames are accessible from the non-secure side.
> >
> > * There are up to 8 timer frames, which have their own CNTVOFF and
> >   physical/virtual timers. Each frame CNTBaseN is duplicated at CNTPL0BaseN
> >   with CNTVOFF and CNTPL0ACR (which controls PL0 accesses) inaccessible.
> >
> > I can see that we might have frames/registers we can't access (if we were
> > booted on the non-secure side), but I can't see anything limiting whether we
> > use a frame for kernel/hyp/user beyond that. Have I missed something?
> >
> > Could we not have something like the following for each frame:
> >
> > frame0 {
> > 	frame-id = <0>;
> > 	status = "disabled"; /* booted NS, secure firmware has not enabled access */
> > 	reg = <0x... 0x1000>, /* CNTBase0 */
> > 	      <0x... 0x1000>; /* CNTPL0Base0 */
> > };
> >
> 
> I don't think you're missing anything. Technically the second view is
> not always implemented though. Using the status property should be
> sufficient I think.

Could we say the reg for the second view is optional?

Might we have a hardware / firmware configuration where the kernel can only access
the secondary view?

> 
> >> Also to get the frame number, I was thinking maybe we should expand the
> >> reg property to have two address cells. Then we could have reg = <0
> >> 0xf0001000 0x1000>.
> > We could do that, but then you definitely need a more complex ranges property,
> > and additional parsing code to handle grabbing it out of the reg property. I
> > can't see what it buys us.
> 
> Ok. It would mandate node names like "frame@0", "frame@1", but I'll drop
> the idea unless someone else finds it useful.

I see. I'd prefer to use a separate property for the id. Placing it in the reg
and then requiring a mapping sounds like it's going to cause a lot of pain.

Cheers,
Mark.

Re: [PATCH 1/4] Documentation: Add memory mapped ARM architected timer binding

[Stephen Boyd]

On 04/11/13 04:24, Mark Rutland wrote:
> Could we say the reg for the second view is optional?
>

Yes, that's already covered in the binding.

> Might we have a hardware / firmware configuration where the kernel can only access
> the secondary view?
>

I don't see how this is possible. The CNTACRn register controls secure
vs. non-secure access for particular registers in a frame regardless of
which view is used. The CNTPL0ACRn is not a security restricted register
and it can only restrict access to certain registers in the second view.
No combination of settings in these registers can restrict access to
only the second view in a frame with two views.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation