| Message ID | 20210304213902.83903-7-marcan@marcan.st |
|---|---|
| State | New |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
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
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.
* 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>
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
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. -- Without deviation from the norm, progress is not possible.
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
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?
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
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(+)