| Message ID | 20210304213902.83903-7-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| 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.
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(+)