diff mbox series

[RFT,v3,06/27] dt-bindings: timer: arm,arch_timer: Add interrupt-names support

Message ID 20210304213902.83903-7-marcan@marcan.st
State New
Headers show
Series Apple M1 SoC platform bring-up | expand

Commit Message

Hector Martin March 4, 2021, 9:38 p.m. UTC
Not all platforms provide the same set of timers/interrupts, and Linux
only needs one (plus kvm/guest ones); some platforms are working around
this by using dummy fake interrupts. Implementing interrupt-names allows
the devicetree to specify an arbitrary set of available interrupts, so
the timer code can pick the right one.

This also adds the hyp-virt timer/interrupt, which was previously not
expressed in the fixed 4-interrupt form.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../devicetree/bindings/timer/arm,arch_timer.yaml  | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Linus Walleij March 5, 2021, 10:18 a.m. UTC | #1
On Thu, Mar 4, 2021 at 10:40 PM Hector Martin <marcan@marcan.st> wrote:

> Not all platforms provide the same set of timers/interrupts, and Linux
> only needs one (plus kvm/guest ones); some platforms are working around
> this by using dummy fake interrupts. Implementing interrupt-names allows
> the devicetree to specify an arbitrary set of available interrupts, so
> the timer code can pick the right one.
>
> This also adds the hyp-virt timer/interrupt, which was previously not
> expressed in the fixed 4-interrupt form.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>

This looks good to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Marc Zyngier March 8, 2021, 11:12 a.m. UTC | #2
On Thu, 04 Mar 2021 21:38:41 +0000,
Hector Martin <marcan@marcan.st> wrote:
> 
> Not all platforms provide the same set of timers/interrupts, and Linux
> only needs one (plus kvm/guest ones); some platforms are working around
> this by using dummy fake interrupts. Implementing interrupt-names allows
> the devicetree to specify an arbitrary set of available interrupts, so
> the timer code can pick the right one.
> 
> This also adds the hyp-virt timer/interrupt, which was previously not
> expressed in the fixed 4-interrupt form.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

Acked-by: Marc Zyngier <maz@kernel.org>

Thanks,

	M.
Tony Lindgren March 8, 2021, 5:14 p.m. UTC | #3
* Hector Martin <marcan@marcan.st> [210304 21:40]:
> Not all platforms provide the same set of timers/interrupts, and Linux
> only needs one (plus kvm/guest ones); some platforms are working around
> this by using dummy fake interrupts. Implementing interrupt-names allows
> the devicetree to specify an arbitrary set of available interrupts, so
> the timer code can pick the right one.
> 
> This also adds the hyp-virt timer/interrupt, which was previously not
> expressed in the fixed 4-interrupt form.

I like this one too:

Reviewed-by: Tony Lindgren <tony@atomide.com>
Rob Herring (Arm) March 8, 2021, 8:38 p.m. UTC | #4
On Fri, Mar 05, 2021 at 06:38:41AM +0900, Hector Martin wrote:
> Not all platforms provide the same set of timers/interrupts, and Linux
> only needs one (plus kvm/guest ones); some platforms are working around
> this by using dummy fake interrupts. Implementing interrupt-names allows
> the devicetree to specify an arbitrary set of available interrupts, so
> the timer code can pick the right one.
> 
> This also adds the hyp-virt timer/interrupt, which was previously not
> expressed in the fixed 4-interrupt form.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  .../devicetree/bindings/timer/arm,arch_timer.yaml  | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> index 2c75105c1398..ebe9b0bebe41 100644
> --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> @@ -34,11 +34,25 @@ properties:
>                - arm,armv8-timer
>  
>    interrupts:
> +    minItems: 1
> +    maxItems: 5
>      items:
>        - description: secure timer irq
>        - description: non-secure timer irq
>        - description: virtual timer irq
>        - description: hypervisor timer irq
> +      - description: hypervisor virtual timer irq
> +
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 5
> +    items:
> +      enum:
> +        - phys-secure
> +        - phys
> +        - virt
> +        - hyp-phys
> +        - hyp-virt

phys-secure and hyp-phys is not very consistent. secure-phys or sec-phys 
instead?

This allows any order which is not ideal (unfortunately json-schema 
doesn't have a way to define order with optional entries in the middle). 
How many possible combinations are there which make sense? If that's a 
reasonable number, I'd rather see them listed out.

Rob
Marc Zyngier March 8, 2021, 10:42 p.m. UTC | #5
On Mon, 08 Mar 2021 20:38:41 +0000,
Rob Herring <robh@kernel.org> wrote:
> 
> On Fri, Mar 05, 2021 at 06:38:41AM +0900, Hector Martin wrote:
> > Not all platforms provide the same set of timers/interrupts, and Linux
> > only needs one (plus kvm/guest ones); some platforms are working around
> > this by using dummy fake interrupts. Implementing interrupt-names allows
> > the devicetree to specify an arbitrary set of available interrupts, so
> > the timer code can pick the right one.
> > 
> > This also adds the hyp-virt timer/interrupt, which was previously not
> > expressed in the fixed 4-interrupt form.
> > 
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > ---
> >  .../devicetree/bindings/timer/arm,arch_timer.yaml  | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> > index 2c75105c1398..ebe9b0bebe41 100644
> > --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> > +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
> > @@ -34,11 +34,25 @@ properties:
> >                - arm,armv8-timer
> >  
> >    interrupts:
> > +    minItems: 1
> > +    maxItems: 5
> >      items:
> >        - description: secure timer irq
> >        - description: non-secure timer irq
> >        - description: virtual timer irq
> >        - description: hypervisor timer irq
> > +      - description: hypervisor virtual timer irq
> > +
> > +  interrupt-names:
> > +    minItems: 1
> > +    maxItems: 5
> > +    items:
> > +      enum:
> > +        - phys-secure
> > +        - phys
> > +        - virt
> > +        - hyp-phys
> > +        - hyp-virt
> 
> phys-secure and hyp-phys is not very consistent. secure-phys or sec-phys 
> instead?
> 
> This allows any order which is not ideal (unfortunately json-schema 
> doesn't have a way to define order with optional entries in the middle). 
> How many possible combinations are there which make sense? If that's a 
> reasonable number, I'd rather see them listed out.

The available of interrupts are a function of the number of security
states, privileged exception levels and architecture revisions, as
described in D11.1.1:

<quote>
- An EL1 physical timer.
- A Non-secure EL2 physical timer.
- An EL3 physical timer.
- An EL1 virtual timer.
- A Non-secure EL2 virtual timer.
- A Secure EL2 virtual timer.
- A Secure EL2 physical timer.
</quote>

* Single security state, EL1 only, ARMv7 & ARMv8.0+ (assumed NS):
  - physical, virtual

* Single security state, EL1 + EL2, ARMv7 & ARMv8.0 (assumed NS)
  - physical, virtual, hyp physical

* Single security state, EL1 + EL2, ARMv8.1+ (assumed NS)
  - physical, virtual, hyp physical, hyp virtual

* Two security states, EL1 + EL3, ARMv7 & ARMv8.0+:
  - secure physical, physical, virtual

* Two security states, EL1 + EL2 + EL3, ARMv7 & ARMv8.0
  - secure physical, physical, virtual, hyp physical

* Two security states, EL1 + EL2 + EL3, ARMv8.1+
  - secure physical, physical, virtual, hyp physical, hyp virtual

* Two security states, EL1 + EL2 + S-EL2 + EL3, ARMv8.4+
  - secure physical, physical, virtual, hyp physical, hyp virtual,
    secure hyp physical, secure hyp virtual

Nobody has seen the last combination in the wild (that is, outside of
a SW model).

I'm really not convinced we want to express this kind of complexity in
the binding (each of the 7 cases), specially given that we don't
encode the underlying HW architecture level or number of exception
levels anywhere, and have ho way to validate such information.

Thanks,

	M.
Rob Herring (Arm) March 9, 2021, 4:11 p.m. UTC | #6
On Mon, Mar 8, 2021 at 3:42 PM Marc Zyngier <maz@kernel.org> wrote:
>

> On Mon, 08 Mar 2021 20:38:41 +0000,

> Rob Herring <robh@kernel.org> wrote:

> >

> > On Fri, Mar 05, 2021 at 06:38:41AM +0900, Hector Martin wrote:

> > > Not all platforms provide the same set of timers/interrupts, and Linux

> > > only needs one (plus kvm/guest ones); some platforms are working around

> > > this by using dummy fake interrupts. Implementing interrupt-names allows

> > > the devicetree to specify an arbitrary set of available interrupts, so

> > > the timer code can pick the right one.

> > >

> > > This also adds the hyp-virt timer/interrupt, which was previously not

> > > expressed in the fixed 4-interrupt form.

> > >

> > > Signed-off-by: Hector Martin <marcan@marcan.st>

> > > ---

> > >  .../devicetree/bindings/timer/arm,arch_timer.yaml  | 14 ++++++++++++++

> > >  1 file changed, 14 insertions(+)

> > >

> > > diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

> > > index 2c75105c1398..ebe9b0bebe41 100644

> > > --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

> > > +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

> > > @@ -34,11 +34,25 @@ properties:

> > >                - arm,armv8-timer

> > >

> > >    interrupts:

> > > +    minItems: 1

> > > +    maxItems: 5

> > >      items:

> > >        - description: secure timer irq

> > >        - description: non-secure timer irq

> > >        - description: virtual timer irq

> > >        - description: hypervisor timer irq

> > > +      - description: hypervisor virtual timer irq

> > > +

> > > +  interrupt-names:

> > > +    minItems: 1

> > > +    maxItems: 5

> > > +    items:

> > > +      enum:

> > > +        - phys-secure

> > > +        - phys

> > > +        - virt

> > > +        - hyp-phys

> > > +        - hyp-virt

> >

> > phys-secure and hyp-phys is not very consistent. secure-phys or sec-phys

> > instead?

> >

> > This allows any order which is not ideal (unfortunately json-schema

> > doesn't have a way to define order with optional entries in the middle).

> > How many possible combinations are there which make sense? If that's a

> > reasonable number, I'd rather see them listed out.

>

> The available of interrupts are a function of the number of security

> states, privileged exception levels and architecture revisions, as

> described in D11.1.1:

>

> <quote>

> - An EL1 physical timer.

> - A Non-secure EL2 physical timer.

> - An EL3 physical timer.

> - An EL1 virtual timer.

> - A Non-secure EL2 virtual timer.

> - A Secure EL2 virtual timer.

> - A Secure EL2 physical timer.

> </quote>

>

> * Single security state, EL1 only, ARMv7 & ARMv8.0+ (assumed NS):

>   - physical, virtual

>

> * Single security state, EL1 + EL2, ARMv7 & ARMv8.0 (assumed NS)

>   - physical, virtual, hyp physical

>

> * Single security state, EL1 + EL2, ARMv8.1+ (assumed NS)

>   - physical, virtual, hyp physical, hyp virtual

>

> * Two security states, EL1 + EL3, ARMv7 & ARMv8.0+:

>   - secure physical, physical, virtual

>

> * Two security states, EL1 + EL2 + EL3, ARMv7 & ARMv8.0

>   - secure physical, physical, virtual, hyp physical

>

> * Two security states, EL1 + EL2 + EL3, ARMv8.1+

>   - secure physical, physical, virtual, hyp physical, hyp virtual

>

> * Two security states, EL1 + EL2 + S-EL2 + EL3, ARMv8.4+

>   - secure physical, physical, virtual, hyp physical, hyp virtual,

>     secure hyp physical, secure hyp virtual

>

> Nobody has seen the last combination in the wild (that is, outside of

> a SW model).

>

> I'm really not convinced we want to express this kind of complexity in

> the binding (each of the 7 cases), specially given that we don't

> encode the underlying HW architecture level or number of exception

> levels anywhere, and have ho way to validate such information.


Actually, we can simplify this down to 2 cases:

oneOf:
  - minItems: 2
    items:
      - const: phys
      - const: virt
      - const: hyp-phys
      - const: hyp-virt
  - minItems: 3
    items:
      - const: sec-phys
      - const: phys
      - const: virt
      - const: hyp-phys
      - const: hyp-virt
      - const: sec-hyp-phy
      - const: sec-hyp-virt

And that's below my threshold for not worth the complexity.

Rob
Hector Martin March 9, 2021, 8:28 p.m. UTC | #7
On 10/03/2021 01.11, Rob Herring wrote:
> On Mon, Mar 8, 2021 at 3:42 PM Marc Zyngier <maz@kernel.org> wrote:

>>

>> On Mon, 08 Mar 2021 20:38:41 +0000,

>> Rob Herring <robh@kernel.org> wrote:

>>>

>>> On Fri, Mar 05, 2021 at 06:38:41AM +0900, Hector Martin wrote:

>>>> Not all platforms provide the same set of timers/interrupts, and Linux

>>>> only needs one (plus kvm/guest ones); some platforms are working around

>>>> this by using dummy fake interrupts. Implementing interrupt-names allows

>>>> the devicetree to specify an arbitrary set of available interrupts, so

>>>> the timer code can pick the right one.

>>>>

>>>> This also adds the hyp-virt timer/interrupt, which was previously not

>>>> expressed in the fixed 4-interrupt form.

>>>>

>>>> Signed-off-by: Hector Martin <marcan@marcan.st>

>>>> ---

>>>>   .../devicetree/bindings/timer/arm,arch_timer.yaml  | 14 ++++++++++++++

>>>>   1 file changed, 14 insertions(+)

>>>>

>>>> diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

>>>> index 2c75105c1398..ebe9b0bebe41 100644

>>>> --- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

>>>> +++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml

>>>> @@ -34,11 +34,25 @@ properties:

>>>>                 - arm,armv8-timer

>>>>

>>>>     interrupts:

>>>> +    minItems: 1

>>>> +    maxItems: 5

>>>>       items:

>>>>         - description: secure timer irq

>>>>         - description: non-secure timer irq

>>>>         - description: virtual timer irq

>>>>         - description: hypervisor timer irq

>>>> +      - description: hypervisor virtual timer irq

>>>> +

>>>> +  interrupt-names:

>>>> +    minItems: 1

>>>> +    maxItems: 5

>>>> +    items:

>>>> +      enum:

>>>> +        - phys-secure

>>>> +        - phys

>>>> +        - virt

>>>> +        - hyp-phys

>>>> +        - hyp-virt

>>>

>>> phys-secure and hyp-phys is not very consistent. secure-phys or sec-phys

>>> instead?

>>>

>>> This allows any order which is not ideal (unfortunately json-schema

>>> doesn't have a way to define order with optional entries in the middle).

>>> How many possible combinations are there which make sense? If that's a

>>> reasonable number, I'd rather see them listed out.

>>

>> The available of interrupts are a function of the number of security

>> states, privileged exception levels and architecture revisions, as

>> described in D11.1.1:

>>

>> <quote>

>> - An EL1 physical timer.

>> - A Non-secure EL2 physical timer.

>> - An EL3 physical timer.

>> - An EL1 virtual timer.

>> - A Non-secure EL2 virtual timer.

>> - A Secure EL2 virtual timer.

>> - A Secure EL2 physical timer.

>> </quote>

>>

>> * Single security state, EL1 only, ARMv7 & ARMv8.0+ (assumed NS):

>>    - physical, virtual

>>

>> * Single security state, EL1 + EL2, ARMv7 & ARMv8.0 (assumed NS)

>>    - physical, virtual, hyp physical

>>

>> * Single security state, EL1 + EL2, ARMv8.1+ (assumed NS)

>>    - physical, virtual, hyp physical, hyp virtual

>>

>> * Two security states, EL1 + EL3, ARMv7 & ARMv8.0+:

>>    - secure physical, physical, virtual

>>

>> * Two security states, EL1 + EL2 + EL3, ARMv7 & ARMv8.0

>>    - secure physical, physical, virtual, hyp physical

>>

>> * Two security states, EL1 + EL2 + EL3, ARMv8.1+

>>    - secure physical, physical, virtual, hyp physical, hyp virtual

>>

>> * Two security states, EL1 + EL2 + S-EL2 + EL3, ARMv8.4+

>>    - secure physical, physical, virtual, hyp physical, hyp virtual,

>>      secure hyp physical, secure hyp virtual

>>

>> Nobody has seen the last combination in the wild (that is, outside of

>> a SW model).

>>

>> I'm really not convinced we want to express this kind of complexity in

>> the binding (each of the 7 cases), specially given that we don't

>> encode the underlying HW architecture level or number of exception

>> levels anywhere, and have ho way to validate such information.

> 

> Actually, we can simplify this down to 2 cases:

> 

> oneOf:

>    - minItems: 2

>      items:

>        - const: phys

>        - const: virt

>        - const: hyp-phys

>        - const: hyp-virt

>    - minItems: 3

>      items:

>        - const: sec-phys

>        - const: phys

>        - const: virt

>        - const: hyp-phys

>        - const: hyp-virt

>        - const: sec-hyp-phy

>        - const: sec-hyp-virt

> 

> And that's below my threshold for not worth the complexity.


This makes sense. Since we aren't using the sec-hyp stuff here, and 
those go at the end of the list, we can omit them from this patch for 
now and add them whenever they're needed for a platform. Does that sound OK?

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
index 2c75105c1398..ebe9b0bebe41 100644
--- a/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
+++ b/Documentation/devicetree/bindings/timer/arm,arch_timer.yaml
@@ -34,11 +34,25 @@  properties:
               - arm,armv8-timer
 
   interrupts:
+    minItems: 1
+    maxItems: 5
     items:
       - description: secure timer irq
       - description: non-secure timer irq
       - description: virtual timer irq
       - description: hypervisor timer irq
+      - description: hypervisor virtual timer irq
+
+  interrupt-names:
+    minItems: 1
+    maxItems: 5
+    items:
+      enum:
+        - phys-secure
+        - phys
+        - virt
+        - hyp-phys
+        - hyp-virt
 
   clock-frequency:
     description: The frequency of the main counter, in Hz. Should be present