| Message ID | 1365474623-29181-2-git-send-email-sboyd@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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>.
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.
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.
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.
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.
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>; + }; + };
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(-)