| Message ID | 20211209043249.65474-2-marcan@marcan.st |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | irqchip/apple-aic: Add support for AICv2 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success |
On Thu, Dec 09, 2021 at 01:32:44PM +0900, Hector Martin wrote: > This new incompatible revision of the AIC peripheral introduces > multi-die support. To handle that, we introduce an optional > 4-argument interrupt-cells form. > > Also add an apple,event-reg property to specify the offset of the event > register. Inexplicably, the capability registers allow us to compute > other register offsets, but not this one. This allows us to keep > forward-compatibility with future SoCs that will likely implement > different die counts, thus shifting the event register. Apple do the > same thing in their device tree... > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../interrupt-controller/apple,aic.yaml | 62 +++++++++++++++---- > 1 file changed, 49 insertions(+), 13 deletions(-) > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml > index 97359024709a..6a8dd213e59a 100644 > --- a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml > +++ b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml > @@ -18,38 +18,44 @@ description: | > > - Level-triggered hardware IRQs wired to SoC blocks > - Single mask bit per IRQ > - - Per-IRQ affinity setting > + - Per-IRQ affinity setting (AICv1 only) > - Automatic masking on event delivery (auto-ack) > - Software triggering (ORed with hw line) > - 2 per-CPU IPIs (meant as "self" and "other", but they are interchangeable > - if not symmetric) > + if not symmetric) (AICv1 only) > - Automatic prioritization (single event/ack register per CPU, lower IRQs = > higher priority) > - Automatic masking on ack > - - Default "this CPU" register view and explicit per-CPU views > + - Default "this CPU" register view and explicit per-CPU views (AICv1 only) > > This device also represents the FIQ interrupt sources on platforms using AIC, > - which do not go through a discrete interrupt controller. > - > -allOf: > - - $ref: /schemas/interrupt-controller.yaml# > + which do not go through a discrete interrupt controller. It also handles > + FIQ-based Fast IPIs on supported chips. > > properties: > compatible: > - items: > - - const: apple,t8103-aic > - - const: apple,aic > + oneOf: > + - items: > + - const: apple,t8103-aic > + - const: apple,aic > + - items: > + - const: apple,t6000-aic > + - const: apple,aic2 > > interrupt-controller: true > > '#interrupt-cells': > - const: 3 > + minimum: 3 > + maximum: 4 > description: | > The 1st cell contains the interrupt type: > - 0: Hardware IRQ > - 1: FIQ > > - The 2nd cell contains the interrupt number. > + The optional 2nd cell contains the die ID (apple,aic2 only). > + If not present, it defaults to 0. > + > + The next cell contains the interrupt number. > - HW IRQs: interrupt number > - FIQs: > - 0: physical HV timer > @@ -57,7 +63,7 @@ properties: > - 2: physical guest timer > - 3: virtual guest timer > > - The 3rd cell contains the interrupt flags. This is normally > + The last cell contains the interrupt flags. This is normally > IRQ_TYPE_LEVEL_HIGH (4). > > reg: > @@ -68,6 +74,13 @@ properties: > power-domains: > maxItems: 1 > > + apple,event-reg: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Specifies the offset of the event register, which lies after all the > + implemented die register sets, page aligned. This is not computable from > + capability register values, so we have to specify it explicitly. > + > required: > - compatible > - '#interrupt-cells' > @@ -76,6 +89,29 @@ required: > > additionalProperties: false > > +allOf: > + - $ref: /schemas/interrupt-controller.yaml# > + - if: > + properties: > + compatible: > + contains: > + enum: > + - apple,aic > + then: > + properties: > + '#interrupt-cells': > + const: 3 > + > + - if: > + properties: > + compatible: > + contains: > + enum: > + - apple,aic2 > + then: > + required: > + - apple,event-reg Is this property valid for aic1? If not, you need: else: not: required: - apple,event-reg I tend to think you should just make this a separate document. There's not a whole lot of sharing (compared to any other interrupt controller). > + > examples: > - | > soc { > -- > 2.33.0 > >
On 10/12/2021 02.28, Rob Herring wrote: > On Thu, Dec 09, 2021 at 01:32:44PM +0900, Hector Martin wrote: <snip> >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - apple,aic2 >> + then: >> + required: >> + - apple,event-reg > > Is this property valid for aic1? If not, you need: > > else: > not: > required: > - apple,event-reg > Thanks, I wasn't sure how to do this. Took me a second to realize how the logic works here, heh. > > I tend to think you should just make this a separate document. There's > not a whole lot of sharing (compared to any other interrupt controller). Good point. I just kind of defaulted to this way because the driver is the same (and does share a bunch), but indeed the binding doesn't really reflect any of that. I'll split it off into another document for v2. Might as well make the 4-argument interrupt form mandatory then (we use it for all DTs, even the current 1-die machines, on AICv2 SoCs; the driver can handle both but we might as well be stricter with the binding).
On Sat, 11 Dec 2021 12:28:10 +0000, Hector Martin <marcan@marcan.st> wrote: > > On 10/12/2021 02.28, Rob Herring wrote: > > On Thu, Dec 09, 2021 at 01:32:44PM +0900, Hector Martin wrote: > <snip> > >> + - if: > >> + properties: > >> + compatible: > >> + contains: > >> + enum: > >> + - apple,aic2 > >> + then: > >> + required: > >> + - apple,event-reg > > > > Is this property valid for aic1? If not, you need: > > > > else: > > not: > > required: > > - apple,event-reg > > > > Thanks, I wasn't sure how to do this. Took me a second to realize how > the logic works here, heh. > > > > > I tend to think you should just make this a separate document. There's > > not a whole lot of sharing (compared to any other interrupt controller). > > Good point. I just kind of defaulted to this way because the driver is > the same (and does share a bunch), but indeed the binding doesn't > really reflect any of that. I'll split it off into another document > for v2. Might as well make the 4-argument interrupt form mandatory > then (we use it for all DTs, even the current 1-die machines, on AICv2 > SoCs; the driver can handle both but we might as well be stricter with > the binding). Well, I'm about to add this 4th cell for FIQ signalled interrupts so that we can specify an affinity (similarly to what we do with GICv3, 0 meaning no specific affinity and a non-zero phandle indicating a specific affinity). Generalising the 4-cell even on AICv1 systems would be pretty nice, and we can always keep the backward compat as a fallback for old DTs (that'd pretty cheap). M.
> From: Hector Martin <marcan@marcan.st> > Date: Sat, 11 Dec 2021 21:28:10 +0900 > > On 10/12/2021 02.28, Rob Herring wrote: > > On Thu, Dec 09, 2021 at 01:32:44PM +0900, Hector Martin wrote: > <snip> > >> + - if: > >> + properties: > >> + compatible: > >> + contains: > >> + enum: > >> + - apple,aic2 > >> + then: > >> + required: > >> + - apple,event-reg > > > > Is this property valid for aic1? If not, you need: > > > > else: > > not: > > required: > > - apple,event-reg > > > > Thanks, I wasn't sure how to do this. Took me a second to realize how > the logic works here, heh. > > > > > I tend to think you should just make this a separate document. There's > > not a whole lot of sharing (compared to any other interrupt controller). > > Good point. I just kind of defaulted to this way because the driver is > the same (and does share a bunch), but indeed the binding doesn't really > reflect any of that. I'll split it off into another document for v2. > Might as well make the 4-argument interrupt form mandatory then (we use > it for all DTs, even the current 1-die machines, on AICv2 SoCs; the > driver can handle both but we might as well be stricter with the binding). Simpler that way, so I'd support that.
On 11/12/2021 21.44, Marc Zyngier wrote: > On Sat, 11 Dec 2021 12:28:10 +0000, > Hector Martin <marcan@marcan.st> wrote: >> >> On 10/12/2021 02.28, Rob Herring wrote: >>> On Thu, Dec 09, 2021 at 01:32:44PM +0900, Hector Martin wrote: >> <snip> >>>> + - if: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>>> + - apple,aic2 >>>> + then: >>>> + required: >>>> + - apple,event-reg >>> >>> Is this property valid for aic1? If not, you need: >>> >>> else: >>> not: >>> required: >>> - apple,event-reg >>> >> >> Thanks, I wasn't sure how to do this. Took me a second to realize how >> the logic works here, heh. >> >>> >>> I tend to think you should just make this a separate document. There's >>> not a whole lot of sharing (compared to any other interrupt controller). >> >> Good point. I just kind of defaulted to this way because the driver is >> the same (and does share a bunch), but indeed the binding doesn't >> really reflect any of that. I'll split it off into another document >> for v2. Might as well make the 4-argument interrupt form mandatory >> then (we use it for all DTs, even the current 1-die machines, on AICv2 >> SoCs; the driver can handle both but we might as well be stricter with >> the binding). > > Well, I'm about to add this 4th cell for FIQ signalled interrupts so > that we can specify an affinity (similarly to what we do with GICv3, 0 > meaning no specific affinity and a non-zero phandle indicating a > specific affinity). > > Generalising the 4-cell even on AICv1 systems would be pretty nice, > and we can always keep the backward compat as a fallback for old DTs > (that'd pretty cheap). The driver still takes both, so that's not an issue; we can certainly have the AICv1 binding allow both and the AICv2 one require the 4-cell form. That will also make copy/paste between t8103 and t6000 SoCs slightly less error-prone, since both will have the extra cell.
diff --git a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml index 97359024709a..6a8dd213e59a 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml +++ b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml @@ -18,38 +18,44 @@ description: | - Level-triggered hardware IRQs wired to SoC blocks - Single mask bit per IRQ - - Per-IRQ affinity setting + - Per-IRQ affinity setting (AICv1 only) - Automatic masking on event delivery (auto-ack) - Software triggering (ORed with hw line) - 2 per-CPU IPIs (meant as "self" and "other", but they are interchangeable - if not symmetric) + if not symmetric) (AICv1 only) - Automatic prioritization (single event/ack register per CPU, lower IRQs = higher priority) - Automatic masking on ack - - Default "this CPU" register view and explicit per-CPU views + - Default "this CPU" register view and explicit per-CPU views (AICv1 only) This device also represents the FIQ interrupt sources on platforms using AIC, - which do not go through a discrete interrupt controller. - -allOf: - - $ref: /schemas/interrupt-controller.yaml# + which do not go through a discrete interrupt controller. It also handles + FIQ-based Fast IPIs on supported chips. properties: compatible: - items: - - const: apple,t8103-aic - - const: apple,aic + oneOf: + - items: + - const: apple,t8103-aic + - const: apple,aic + - items: + - const: apple,t6000-aic + - const: apple,aic2 interrupt-controller: true '#interrupt-cells': - const: 3 + minimum: 3 + maximum: 4 description: | The 1st cell contains the interrupt type: - 0: Hardware IRQ - 1: FIQ - The 2nd cell contains the interrupt number. + The optional 2nd cell contains the die ID (apple,aic2 only). + If not present, it defaults to 0. + + The next cell contains the interrupt number. - HW IRQs: interrupt number - FIQs: - 0: physical HV timer @@ -57,7 +63,7 @@ properties: - 2: physical guest timer - 3: virtual guest timer - The 3rd cell contains the interrupt flags. This is normally + The last cell contains the interrupt flags. This is normally IRQ_TYPE_LEVEL_HIGH (4). reg: @@ -68,6 +74,13 @@ properties: power-domains: maxItems: 1 + apple,event-reg: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Specifies the offset of the event register, which lies after all the + implemented die register sets, page aligned. This is not computable from + capability register values, so we have to specify it explicitly. + required: - compatible - '#interrupt-cells' @@ -76,6 +89,29 @@ required: additionalProperties: false +allOf: + - $ref: /schemas/interrupt-controller.yaml# + - if: + properties: + compatible: + contains: + enum: + - apple,aic + then: + properties: + '#interrupt-cells': + const: 3 + + - if: + properties: + compatible: + contains: + enum: + - apple,aic2 + then: + required: + - apple,event-reg + examples: - | soc {
This new incompatible revision of the AIC peripheral introduces multi-die support. To handle that, we introduce an optional 4-argument interrupt-cells form. Also add an apple,event-reg property to specify the offset of the event register. Inexplicably, the capability registers allow us to compute other register offsets, but not this one. This allows us to keep forward-compatibility with future SoCs that will likely implement different die counts, thus shifting the event register. Apple do the same thing in their device tree... Signed-off-by: Hector Martin <marcan@marcan.st> --- .../interrupt-controller/apple,aic.yaml | 62 +++++++++++++++---- 1 file changed, 49 insertions(+), 13 deletions(-)