| Message ID | 20210726083204.93196-2-mark.kettenis@xs4all.nl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Apple M1 PCIe DT bindings | expand |
> Date: Mon, 26 Jul 2021 17:18:48 -0600 > From: Rob Herring <robh@kernel.org> Hi Rob, > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > From: Mark Kettenis <kettenis@openbsd.org> > > > > The Apple PCIe host controller is a PCIe host controller with > > multiple root ports present in Apple ARM SoC platforms, including > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > --- > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 167 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > new file mode 100644 > > index 000000000000..bfcbdee79c64 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > @@ -0,0 +1,166 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Apple PCIe host controller > > + > > +maintainers: > > + - Mark Kettenis <kettenis@openbsd.org> > > + > > +description: | > > + The Apple PCIe host controller is a PCIe host controller with > > + multiple root ports present in Apple ARM SoC platforms, including > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > + The controller incorporates Synopsys DesigWare PCIe logic to > > + implements its root ports. But the ATU found on most DesignWare > > + PCIe host bridges is absent. > > blank line > > > + All root ports share a single ECAM space, but separate GPIOs are > > + used to take the PCI devices on those ports out of reset. Therefore > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > reset-gpios > > > + the child nodes that represent the PCI bridges that correspond to > > + the individual root ports. > > blank line Fixing these for v4. > > + MSIs are handled by the PCIe controller and translated into regular > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > + distributed over the root ports as the OS sees fit by programming > > + the PCIe controller's port registers. > > + > > +allOf: > > + - $ref: /schemas/pci/pci-bus.yaml# > > + > > +properties: > > + compatible: > > + items: > > + - const: apple,t8103-pcie > > + - const: apple,pcie > > + > > + reg: > > + minItems: 3 > > + maxItems: 5 > > + > > + reg-names: > > + minItems: 3 > > + maxItems: 5 > > + items: > > + - const: config > > + - const: rc > > + - const: port0 > > + - const: port1 > > + - const: port2 > > + > > + ranges: > > + minItems: 2 > > + maxItems: 2 > > + > > + interrupts: > > + description: > > + Interrupt specifiers, one for each root port. > > + minItems: 1 > > + maxItems: 3 > > + > > + msi-controller: true > > + msi-parent: true > > + > > + msi-ranges: > > + description: > > + A list of pairs <intid span>, where "intid" is the first > > + interrupt number that can be used as an MSI, and "span" the size > > + of that range. > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > + items: > > + minItems: 2 > > + maxItems: 2 > > I still have issues I raised on v1 with this property. It's genericish > looking, but not generic. 'intid' as a single cell can't specify any > parent interrupt such as a GIC which uses 3 cells. You could put in all > the cells, but you'd still be assuming which cell you can increment. Not sure what you mean with "specify any parent interrupt" here. For the GIC and AIC the three cells encode the interrupt type, interrupt number, and trigger level/polarity. And for MSIs the interrupt type and trigger level/polarity are pretty much implied. It is genericish in the sense that it follows the pattern of the "mbi-ranges" in the arm,gic-v3.yaml binding. > I think you should just list all these under 'interrupts' using > interrupt-names to make your life easier: > > interrupt-names: > items: > - const: port0 > - const: port1 > - const: port2 > - const: msi0 > - const: msi1 > - const: msi2 > - const: msi3 > ... > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > you should list N interrupts. The worst case for the above is N entries > too if not contiguous. Hmm, "msi-ranges" was what Marc Zyngier came up with since he didn't really like the approach you sketch above. And he gave this version of the binding his blessing. Your approach would work fine as well, although doing a string-based lookup for each MSI vector is a bit ugly.
On Tue, 27 Jul 2021 00:18:48 +0100, Rob Herring <robh@kernel.org> wrote: > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > From: Mark Kettenis <kettenis@openbsd.org> > > > > The Apple PCIe host controller is a PCIe host controller with > > multiple root ports present in Apple ARM SoC platforms, including > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > --- > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 167 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > new file mode 100644 > > index 000000000000..bfcbdee79c64 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > @@ -0,0 +1,166 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Apple PCIe host controller > > + > > +maintainers: > > + - Mark Kettenis <kettenis@openbsd.org> > > + > > +description: | > > + The Apple PCIe host controller is a PCIe host controller with > > + multiple root ports present in Apple ARM SoC platforms, including > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > + The controller incorporates Synopsys DesigWare PCIe logic to > > + implements its root ports. But the ATU found on most DesignWare > > + PCIe host bridges is absent. > > blank line > > > + All root ports share a single ECAM space, but separate GPIOs are > > + used to take the PCI devices on those ports out of reset. Therefore > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > reset-gpios > > > + the child nodes that represent the PCI bridges that correspond to > > + the individual root ports. > > blank line > > > + MSIs are handled by the PCIe controller and translated into regular > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > + distributed over the root ports as the OS sees fit by programming > > + the PCIe controller's port registers. > > + > > +allOf: > > + - $ref: /schemas/pci/pci-bus.yaml# > > + > > +properties: > > + compatible: > > + items: > > + - const: apple,t8103-pcie > > + - const: apple,pcie > > + > > + reg: > > + minItems: 3 > > + maxItems: 5 > > + > > + reg-names: > > + minItems: 3 > > + maxItems: 5 > > + items: > > + - const: config > > + - const: rc > > + - const: port0 > > + - const: port1 > > + - const: port2 > > + > > + ranges: > > + minItems: 2 > > + maxItems: 2 > > + > > + interrupts: > > + description: > > + Interrupt specifiers, one for each root port. > > + minItems: 1 > > + maxItems: 3 > > + > > + msi-controller: true > > + msi-parent: true > > + > > + msi-ranges: > > + description: > > + A list of pairs <intid span>, where "intid" is the first > > + interrupt number that can be used as an MSI, and "span" the size > > + of that range. > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > + items: > > + minItems: 2 > > + maxItems: 2 > > I still have issues I raised on v1 with this property. It's genericish > looking, but not generic. 'intid' as a single cell can't specify any > parent interrupt such as a GIC which uses 3 cells. You could put in all > the cells, but you'd still be assuming which cell you can increment. The GIC bindings already use similar abstractions, see what we do for both GICv2m and GICv3 MBIs. Other MSI controllers use similar properties (alpine and loongson, for example). > I think you should just list all these under 'interrupts' using > interrupt-names to make your life easier: > > interrupt-names: > items: > - const: port0 > - const: port1 > - const: port2 > - const: msi0 > - const: msi1 > - const: msi2 > - const: msi3 > ... > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > you should list N interrupts. The worst case for the above is N entries > too if not contiguous. And that's where I beg to differ, again. Specifying interrupts like this gives the false impression that these interrupts are generated by the device that owns them (the RC). Which for MSIs is not the case. This is not only verbose, this is semantically dubious. And what should we do when the number of possible interrupt is ridiculously large, as it is for the GICv3 ITS? I wish we had a standard way to express these constraints. Until we do, I don't think enumerating individual interrupts is a practical thing to do, nor that it actually represents the topology of the system. Thanks, M. -- Without deviation from the norm, progress is not possible.
On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 27 Jul 2021 00:18:48 +0100, > Rob Herring <robh@kernel.org> wrote: > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > The Apple PCIe host controller is a PCIe host controller with > > > multiple root ports present in Apple ARM SoC platforms, including > > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > --- > > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > > MAINTAINERS | 1 + > > > 2 files changed, 167 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > new file mode 100644 > > > index 000000000000..bfcbdee79c64 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > @@ -0,0 +1,166 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Apple PCIe host controller > > > + > > > +maintainers: > > > + - Mark Kettenis <kettenis@openbsd.org> > > > + > > > +description: | > > > + The Apple PCIe host controller is a PCIe host controller with > > > + multiple root ports present in Apple ARM SoC platforms, including > > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > > + The controller incorporates Synopsys DesigWare PCIe logic to > > > + implements its root ports. But the ATU found on most DesignWare > > > + PCIe host bridges is absent. > > > > blank line > > > > > + All root ports share a single ECAM space, but separate GPIOs are > > > + used to take the PCI devices on those ports out of reset. Therefore > > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > > > reset-gpios > > > > > + the child nodes that represent the PCI bridges that correspond to > > > + the individual root ports. > > > > blank line > > > > > + MSIs are handled by the PCIe controller and translated into regular > > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > > + distributed over the root ports as the OS sees fit by programming > > > + the PCIe controller's port registers. > > > + > > > +allOf: > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > + > > > +properties: > > > + compatible: > > > + items: > > > + - const: apple,t8103-pcie > > > + - const: apple,pcie > > > + > > > + reg: > > > + minItems: 3 > > > + maxItems: 5 > > > + > > > + reg-names: > > > + minItems: 3 > > > + maxItems: 5 > > > + items: > > > + - const: config > > > + - const: rc > > > + - const: port0 > > > + - const: port1 > > > + - const: port2 > > > + > > > + ranges: > > > + minItems: 2 > > > + maxItems: 2 > > > + > > > + interrupts: > > > + description: > > > + Interrupt specifiers, one for each root port. > > > + minItems: 1 > > > + maxItems: 3 > > > + > > > + msi-controller: true > > > + msi-parent: true > > > + > > > + msi-ranges: > > > + description: > > > + A list of pairs <intid span>, where "intid" is the first > > > + interrupt number that can be used as an MSI, and "span" the size > > > + of that range. > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > + items: > > > + minItems: 2 > > > + maxItems: 2 > > > > I still have issues I raised on v1 with this property. It's genericish > > looking, but not generic. 'intid' as a single cell can't specify any > > parent interrupt such as a GIC which uses 3 cells. You could put in all > > the cells, but you'd still be assuming which cell you can increment. > > The GIC bindings already use similar abstractions, see what we do for > both GICv2m and GICv3 MBIs. Other MSI controllers use similar > properties (alpine and loongson, for example). That's the problem. Everyone making up their own crap. > > I think you should just list all these under 'interrupts' using > > interrupt-names to make your life easier: > > > > interrupt-names: > > items: > > - const: port0 > > - const: port1 > > - const: port2 > > - const: msi0 > > - const: msi1 > > - const: msi2 > > - const: msi3 > > ... > > > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > > you should list N interrupts. The worst case for the above is N entries > > too if not contiguous. > > And that's where I beg to differ, again. > > Specifying interrupts like this gives the false impression that these > interrupts are generated by the device that owns them (the RC). Which > for MSIs is not the case. It's no different than an interrupt controller node having an interrupts property. The source is downstream and the interrupt controller is combining/translating the interrupts. The physical interrupt signals are connected to and originating in this block. That sounds like perfectly 'describing the h/w' to me. > This is not only verbose, this is > semantically dubious. And what should we do when the number of > possible interrupt is ridiculously large, as it is for the GICv3 ITS? I don't disagree with the verbose part. But that's not really an issue in this case. > I wish we had a standard way to express these constraints. Until we > do, I don't think enumerating individual interrupts is a practical > thing to do, nor that it actually represents the topology of the > system. The only way a standard way will happen is to stop accepting the custom properties. All the custom properties suffer from knowledge of what the parent interrupt controller is. To fix that, I think we need something like this: msi-ranges = <intspec base>, <intspec step>, <intspec end>; 'intspec' is defined by the parent interrupt-controller cells. step is the value to add. And end is what to match on to stop aka the last interrupt in the range. For example, if the GIC is the parent, we'd have something like this: <GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0> Does this apply to cases other than MSI? I think so as don't we have the same type of properties with the low power mode shadow interrupt controllers? So 'interrupt-ranges'? It looks to me like there's an assumption in the kernel that an MSI controller has a linear range of parent interrupts? Is that correct and something that's guaranteed? That assumption leaks into the existing bindings. It's fine for the kernel to assume that until there's a case that's not linear, but a common binding needs to be able handle a non-linear case. Rob
Hi Rob, Apologies for the delay, I somehow misplaced this email... On Mon, 02 Aug 2021 17:10:39 +0100, Rob Herring <robh@kernel.org> wrote: > > On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 27 Jul 2021 00:18:48 +0100, > > Rob Herring <robh@kernel.org> wrote: > > > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > The Apple PCIe host controller is a PCIe host controller with > > > > multiple root ports present in Apple ARM SoC platforms, including > > > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > > --- > > > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > > > MAINTAINERS | 1 + > > > > 2 files changed, 167 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > new file mode 100644 > > > > index 000000000000..bfcbdee79c64 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > @@ -0,0 +1,166 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Apple PCIe host controller > > > > + > > > > +maintainers: > > > > + - Mark Kettenis <kettenis@openbsd.org> > > > > + > > > > +description: | > > > > + The Apple PCIe host controller is a PCIe host controller with > > > > + multiple root ports present in Apple ARM SoC platforms, including > > > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > + The controller incorporates Synopsys DesigWare PCIe logic to > > > > + implements its root ports. But the ATU found on most DesignWare > > > > + PCIe host bridges is absent. > > > > > > blank line > > > > > > > + All root ports share a single ECAM space, but separate GPIOs are > > > > + used to take the PCI devices on those ports out of reset. Therefore > > > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > > > > > reset-gpios > > > > > > > + the child nodes that represent the PCI bridges that correspond to > > > > + the individual root ports. > > > > > > blank line > > > > > > > + MSIs are handled by the PCIe controller and translated into regular > > > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > > > + distributed over the root ports as the OS sees fit by programming > > > > + the PCIe controller's port registers. > > > > + > > > > +allOf: > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > + > > > > +properties: > > > > + compatible: > > > > + items: > > > > + - const: apple,t8103-pcie > > > > + - const: apple,pcie > > > > + > > > > + reg: > > > > + minItems: 3 > > > > + maxItems: 5 > > > > + > > > > + reg-names: > > > > + minItems: 3 > > > > + maxItems: 5 > > > > + items: > > > > + - const: config > > > > + - const: rc > > > > + - const: port0 > > > > + - const: port1 > > > > + - const: port2 > > > > + > > > > + ranges: > > > > + minItems: 2 > > > > + maxItems: 2 > > > > + > > > > + interrupts: > > > > + description: > > > > + Interrupt specifiers, one for each root port. > > > > + minItems: 1 > > > > + maxItems: 3 > > > > + > > > > + msi-controller: true > > > > + msi-parent: true > > > > + > > > > + msi-ranges: > > > > + description: > > > > + A list of pairs <intid span>, where "intid" is the first > > > > + interrupt number that can be used as an MSI, and "span" the size > > > > + of that range. > > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > > + items: > > > > + minItems: 2 > > > > + maxItems: 2 > > > > > > I still have issues I raised on v1 with this property. It's genericish > > > looking, but not generic. 'intid' as a single cell can't specify any > > > parent interrupt such as a GIC which uses 3 cells. You could put in all > > > the cells, but you'd still be assuming which cell you can increment. > > > > The GIC bindings already use similar abstractions, see what we do for > > both GICv2m and GICv3 MBIs. Other MSI controllers use similar > > properties (alpine and loongson, for example). > > That's the problem. Everyone making up their own crap. And that crap gets approved: https://lore.kernel.org/lkml/20200512205704.GA10412@bogus/ I'm not trying to be antagonistic here, but it seems that your position on this very subject has changed recently. > > > I think you should just list all these under 'interrupts' using > > > interrupt-names to make your life easier: > > > > > > interrupt-names: > > > items: > > > - const: port0 > > > - const: port1 > > > - const: port2 > > > - const: msi0 > > > - const: msi1 > > > - const: msi2 > > > - const: msi3 > > > ... > > > > > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > > > you should list N interrupts. The worst case for the above is N entries > > > too if not contiguous. > > > > And that's where I beg to differ, again. > > > > Specifying interrupts like this gives the false impression that these > > interrupts are generated by the device that owns them (the RC). Which > > for MSIs is not the case. > > It's no different than an interrupt controller node having an > interrupts property. The source is downstream and the interrupt > controller is combining/translating the interrupts. > > The physical interrupt signals are connected to and originating in > this block. Oh, I also object to this, for the same reasons. The only case where it makes sense IMHO is when the interrupt controller is a multiplexer. > That sounds like perfectly 'describing the h/w' to me. I guess we have a different view of about these things. At the end of the day, I don't care enough as long as we can expose a range of interrupts one way or another. > > This is not only verbose, this is > > semantically dubious. And what should we do when the number of > > possible interrupt is ridiculously large, as it is for the GICv3 ITS? > > I don't disagree with the verbose part. But that's not really an issue > in this case. > > > I wish we had a standard way to express these constraints. Until we > > do, I don't think enumerating individual interrupts is a practical > > thing to do, nor that it actually represents the topology of the > > system. > > The only way a standard way will happen is to stop accepting the > custom properties. > > All the custom properties suffer from knowledge of what the parent > interrupt controller is. To fix that, I think we need something like > this: > > msi-ranges = <intspec base>, <intspec step>, <intspec end>; > > 'intspec' is defined by the parent interrupt-controller cells. step is > the value to add. And end is what to match on to stop aka the last > interrupt in the range. For example, if the GIC is the parent, we'd > have something like this: > > <GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0> > > Does this apply to cases other than MSI? I think so as don't we have > the same type of properties with the low power mode shadow interrupt > controllers? So 'interrupt-ranges'? This would work, though the increment seems a bit over-engineered. You also may need this property to accept multiple ranges. > It looks to me like there's an assumption in the kernel that an MSI > controller has a linear range of parent interrupts? Is that correct > and something that's guaranteed? That assumption leaks into the > existing bindings. Depends on how the controller works. In general, the range maps to the MultiMSI requirements where the message is an offset from the base of the interrupt range. So you generally end-up with ranges of at least 32 contiguous MSIs. Anything under that is sub-par and probably not worth supporting. Of course, the controller may have some mapping facilities, which makes things more... interesting. > It's fine for the kernel to assume that until there's a case that's > not linear, but a common binding needs to be able handle a > non-linear case. Fair enough. I can probably work with Mark to upgrade the binding and the M1 PCIe code. Could you come up with a more formalised proposal? Thanks, M. -- Without deviation from the norm, progress is not possible.
On Sun, Aug 15, 2021 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote: > > Hi Rob, > > Apologies for the delay, I somehow misplaced this email... > > On Mon, 02 Aug 2021 17:10:39 +0100, > Rob Herring <robh@kernel.org> wrote: > > > > On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Tue, 27 Jul 2021 00:18:48 +0100, > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > > > The Apple PCIe host controller is a PCIe host controller with > > > > > multiple root ports present in Apple ARM SoC platforms, including > > > > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > > > --- > > > > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > > > > MAINTAINERS | 1 + > > > > > 2 files changed, 167 insertions(+) > > > > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > new file mode 100644 > > > > > index 000000000000..bfcbdee79c64 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > @@ -0,0 +1,166 @@ > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > +%YAML 1.2 > > > > > +--- > > > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > + > > > > > +title: Apple PCIe host controller > > > > > + > > > > > +maintainers: > > > > > + - Mark Kettenis <kettenis@openbsd.org> > > > > > + > > > > > +description: | > > > > > + The Apple PCIe host controller is a PCIe host controller with > > > > > + multiple root ports present in Apple ARM SoC platforms, including > > > > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > + The controller incorporates Synopsys DesigWare PCIe logic to > > > > > + implements its root ports. But the ATU found on most DesignWare > > > > > + PCIe host bridges is absent. > > > > > > > > blank line > > > > > > > > > + All root ports share a single ECAM space, but separate GPIOs are > > > > > + used to take the PCI devices on those ports out of reset. Therefore > > > > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > > > > > > > reset-gpios > > > > > > > > > + the child nodes that represent the PCI bridges that correspond to > > > > > + the individual root ports. > > > > > > > > blank line > > > > > > > > > + MSIs are handled by the PCIe controller and translated into regular > > > > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > > > > + distributed over the root ports as the OS sees fit by programming > > > > > + the PCIe controller's port registers. > > > > > + > > > > > +allOf: > > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + items: > > > > > + - const: apple,t8103-pcie > > > > > + - const: apple,pcie > > > > > + > > > > > + reg: > > > > > + minItems: 3 > > > > > + maxItems: 5 > > > > > + > > > > > + reg-names: > > > > > + minItems: 3 > > > > > + maxItems: 5 > > > > > + items: > > > > > + - const: config > > > > > + - const: rc > > > > > + - const: port0 > > > > > + - const: port1 > > > > > + - const: port2 > > > > > + > > > > > + ranges: > > > > > + minItems: 2 > > > > > + maxItems: 2 > > > > > + > > > > > + interrupts: > > > > > + description: > > > > > + Interrupt specifiers, one for each root port. > > > > > + minItems: 1 > > > > > + maxItems: 3 > > > > > + > > > > > + msi-controller: true > > > > > + msi-parent: true > > > > > + > > > > > + msi-ranges: > > > > > + description: > > > > > + A list of pairs <intid span>, where "intid" is the first > > > > > + interrupt number that can be used as an MSI, and "span" the size > > > > > + of that range. > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > > > + items: > > > > > + minItems: 2 > > > > > + maxItems: 2 > > > > > > > > I still have issues I raised on v1 with this property. It's genericish > > > > looking, but not generic. 'intid' as a single cell can't specify any > > > > parent interrupt such as a GIC which uses 3 cells. You could put in all > > > > the cells, but you'd still be assuming which cell you can increment. > > > > > > The GIC bindings already use similar abstractions, see what we do for > > > both GICv2m and GICv3 MBIs. Other MSI controllers use similar > > > properties (alpine and loongson, for example). > > > > That's the problem. Everyone making up their own crap. > > And that crap gets approved: > > https://lore.kernel.org/lkml/20200512205704.GA10412@bogus/ > > I'm not trying to be antagonistic here, but it seems that your > position on this very subject has changed recently. Not really, I think it's not the first time we've discussed this. But as I see things over and over, my tolerance for another instance without solving the problem for everyone diminishes. And what other leverage do I have? Additionally, how long we have to support something comes into play. I have no idea for a Loongson MSI controller. I have a better idea on an Apple product... > > > > I think you should just list all these under 'interrupts' using > > > > interrupt-names to make your life easier: > > > > > > > > interrupt-names: > > > > items: > > > > - const: port0 > > > > - const: port1 > > > > - const: port2 > > > > - const: msi0 > > > > - const: msi1 > > > > - const: msi2 > > > > - const: msi3 > > > > ... > > > > > > > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > > > > you should list N interrupts. The worst case for the above is N entries > > > > too if not contiguous. > > > > > > And that's where I beg to differ, again. > > > > > > Specifying interrupts like this gives the false impression that these > > > interrupts are generated by the device that owns them (the RC). Which > > > for MSIs is not the case. > > > > It's no different than an interrupt controller node having an > > interrupts property. The source is downstream and the interrupt > > controller is combining/translating the interrupts. > > > > The physical interrupt signals are connected to and originating in > > this block. > > Oh, I also object to this, for the same reasons. The only case where > it makes sense IMHO is when the interrupt controller is a multiplexer. So we've had the same kind of property for interrupt multiplexers. I'm fine if you think an 'MSI to interrupts mapping property' should be named something else. > > That sounds like perfectly 'describing the h/w' to me. > > I guess we have a different view of about these things. At the end of > the day, I don't care enough as long as we can expose a range of > interrupts one way or another. I don't really either. I just don't want 10 ways AND another... > > > This is not only verbose, this is > > > semantically dubious. And what should we do when the number of > > > possible interrupt is ridiculously large, as it is for the GICv3 ITS? > > > > I don't disagree with the verbose part. But that's not really an issue > > in this case. > > > > > I wish we had a standard way to express these constraints. Until we > > > do, I don't think enumerating individual interrupts is a practical > > > thing to do, nor that it actually represents the topology of the > > > system. > > > > The only way a standard way will happen is to stop accepting the > > custom properties. > > > > All the custom properties suffer from knowledge of what the parent > > interrupt controller is. To fix that, I think we need something like > > this: > > > > msi-ranges = <intspec base>, <intspec step>, <intspec end>; > > > > 'intspec' is defined by the parent interrupt-controller cells. step is > > the value to add. And end is what to match on to stop aka the last > > interrupt in the range. For example, if the GIC is the parent, we'd > > have something like this: > > > > <GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0> > > > > Does this apply to cases other than MSI? I think so as don't we have > > the same type of properties with the low power mode shadow interrupt > > controllers? So 'interrupt-ranges'? > > This would work, though the increment seems a bit over-engineered. You > also may need this property to accept multiple ranges. Yes, certainly. Worst case is a map. > > It looks to me like there's an assumption in the kernel that an MSI > > controller has a linear range of parent interrupts? Is that correct > > and something that's guaranteed? That assumption leaks into the > > existing bindings. > > Depends on how the controller works. In general, the range maps to the > MultiMSI requirements where the message is an offset from the base of > the interrupt range. So you generally end-up with ranges of at least > 32 contiguous MSIs. Anything under that is sub-par and probably not > worth supporting. Maybe just this is enough: msi-ranges = <intspec base>, <length>, <intspec base>, <length>, ... While I say 'length' here, that's really up to the interrupt parent to interpret the intspec cells. > Of course, the controller may have some mapping facilities, which > makes things more... interesting. > > > It's fine for the kernel to assume that until there's a case that's > > not linear, but a common binding needs to be able handle a > > non-linear case. > > Fair enough. I can probably work with Mark to upgrade the binding and > the M1 PCIe code. Could you come up with a more formalised proposal? Not my itch. Rob
> From: Rob Herring <robh@kernel.org> > Date: Sun, 15 Aug 2021 14:19:57 -0500 > > On Sun, Aug 15, 2021 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote: > > > > Hi Rob, > > > > Apologies for the delay, I somehow misplaced this email... > > > > On Mon, 02 Aug 2021 17:10:39 +0100, > > Rob Herring <robh@kernel.org> wrote: > > > > > > On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Tue, 27 Jul 2021 00:18:48 +0100, > > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > > > > > The Apple PCIe host controller is a PCIe host controller with > > > > > > multiple root ports present in Apple ARM SoC platforms, including > > > > > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > > > > --- > > > > > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > > > > > MAINTAINERS | 1 + > > > > > > 2 files changed, 167 insertions(+) > > > > > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > new file mode 100644 > > > > > > index 000000000000..bfcbdee79c64 > > > > > > --- /dev/null > > > > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > @@ -0,0 +1,166 @@ > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > > +%YAML 1.2 > > > > > > +--- > > > > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > + > > > > > > +title: Apple PCIe host controller > > > > > > + > > > > > > +maintainers: > > > > > > + - Mark Kettenis <kettenis@openbsd.org> > > > > > > + > > > > > > +description: | > > > > > > + The Apple PCIe host controller is a PCIe host controller with > > > > > > + multiple root ports present in Apple ARM SoC platforms, including > > > > > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > + The controller incorporates Synopsys DesigWare PCIe logic to > > > > > > + implements its root ports. But the ATU found on most DesignWare > > > > > > + PCIe host bridges is absent. > > > > > > > > > > blank line > > > > > > > > > > > + All root ports share a single ECAM space, but separate GPIOs are > > > > > > + used to take the PCI devices on those ports out of reset. Therefore > > > > > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > > > > > > > > > reset-gpios > > > > > > > > > > > + the child nodes that represent the PCI bridges that correspond to > > > > > > + the individual root ports. > > > > > > > > > > blank line > > > > > > > > > > > + MSIs are handled by the PCIe controller and translated into regular > > > > > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > > > > > + distributed over the root ports as the OS sees fit by programming > > > > > > + the PCIe controller's port registers. > > > > > > + > > > > > > +allOf: > > > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > > > + > > > > > > +properties: > > > > > > + compatible: > > > > > > + items: > > > > > > + - const: apple,t8103-pcie > > > > > > + - const: apple,pcie > > > > > > + > > > > > > + reg: > > > > > > + minItems: 3 > > > > > > + maxItems: 5 > > > > > > + > > > > > > + reg-names: > > > > > > + minItems: 3 > > > > > > + maxItems: 5 > > > > > > + items: > > > > > > + - const: config > > > > > > + - const: rc > > > > > > + - const: port0 > > > > > > + - const: port1 > > > > > > + - const: port2 > > > > > > + > > > > > > + ranges: > > > > > > + minItems: 2 > > > > > > + maxItems: 2 > > > > > > + > > > > > > + interrupts: > > > > > > + description: > > > > > > + Interrupt specifiers, one for each root port. > > > > > > + minItems: 1 > > > > > > + maxItems: 3 > > > > > > + > > > > > > + msi-controller: true > > > > > > + msi-parent: true > > > > > > + > > > > > > + msi-ranges: > > > > > > + description: > > > > > > + A list of pairs <intid span>, where "intid" is the first > > > > > > + interrupt number that can be used as an MSI, and "span" the size > > > > > > + of that range. > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > > > > + items: > > > > > > + minItems: 2 > > > > > > + maxItems: 2 > > > > > > > > > > I still have issues I raised on v1 with this property. It's genericish > > > > > looking, but not generic. 'intid' as a single cell can't specify any > > > > > parent interrupt such as a GIC which uses 3 cells. You could put in all > > > > > the cells, but you'd still be assuming which cell you can increment. > > > > > > > > The GIC bindings already use similar abstractions, see what we do for > > > > both GICv2m and GICv3 MBIs. Other MSI controllers use similar > > > > properties (alpine and loongson, for example). > > > > > > That's the problem. Everyone making up their own crap. > > > > And that crap gets approved: > > > > https://lore.kernel.org/lkml/20200512205704.GA10412@bogus/ > > > > I'm not trying to be antagonistic here, but it seems that your > > position on this very subject has changed recently. > > Not really, I think it's not the first time we've discussed this. But > as I see things over and over, my tolerance for another instance > without solving the problem for everyone diminishes. And what other > leverage do I have? > > Additionally, how long we have to support something comes into play. I > have no idea for a Loongson MSI controller. I have a better idea on an > Apple product... > > > > > > I think you should just list all these under 'interrupts' using > > > > > interrupt-names to make your life easier: > > > > > > > > > > interrupt-names: > > > > > items: > > > > > - const: port0 > > > > > - const: port1 > > > > > - const: port2 > > > > > - const: msi0 > > > > > - const: msi1 > > > > > - const: msi2 > > > > > - const: msi3 > > > > > ... > > > > > > > > > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > > > > > you should list N interrupts. The worst case for the above is N entries > > > > > too if not contiguous. > > > > > > > > And that's where I beg to differ, again. > > > > > > > > Specifying interrupts like this gives the false impression that these > > > > interrupts are generated by the device that owns them (the RC). Which > > > > for MSIs is not the case. > > > > > > It's no different than an interrupt controller node having an > > > interrupts property. The source is downstream and the interrupt > > > controller is combining/translating the interrupts. > > > > > > The physical interrupt signals are connected to and originating in > > > this block. > > > > Oh, I also object to this, for the same reasons. The only case where > > it makes sense IMHO is when the interrupt controller is a multiplexer. > > So we've had the same kind of property for interrupt multiplexers. I'm > fine if you think an 'MSI to interrupts mapping property' should be > named something else. > > > > That sounds like perfectly 'describing the h/w' to me. > > > > I guess we have a different view of about these things. At the end of > > the day, I don't care enough as long as we can expose a range of > > interrupts one way or another. > > I don't really either. I just don't want 10 ways AND another... > > > > > This is not only verbose, this is > > > > semantically dubious. And what should we do when the number of > > > > possible interrupt is ridiculously large, as it is for the GICv3 ITS? > > > > > > I don't disagree with the verbose part. But that's not really an issue > > > in this case. > > > > > > > I wish we had a standard way to express these constraints. Until we > > > > do, I don't think enumerating individual interrupts is a practical > > > > thing to do, nor that it actually represents the topology of the > > > > system. > > > > > > The only way a standard way will happen is to stop accepting the > > > custom properties. > > > > > > All the custom properties suffer from knowledge of what the parent > > > interrupt controller is. To fix that, I think we need something like > > > this: > > > > > > msi-ranges = <intspec base>, <intspec step>, <intspec end>; > > > > > > 'intspec' is defined by the parent interrupt-controller cells. step is > > > the value to add. And end is what to match on to stop aka the last > > > interrupt in the range. For example, if the GIC is the parent, we'd > > > have something like this: > > > > > > <GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0> > > > > > > Does this apply to cases other than MSI? I think so as don't we have > > > the same type of properties with the low power mode shadow interrupt > > > controllers? So 'interrupt-ranges'? > > > > This would work, though the increment seems a bit over-engineered. You > > also may need this property to accept multiple ranges. > > Yes, certainly. Worst case is a map. > > > > It looks to me like there's an assumption in the kernel that an MSI > > > controller has a linear range of parent interrupts? Is that correct > > > and something that's guaranteed? That assumption leaks into the > > > existing bindings. > > > > Depends on how the controller works. In general, the range maps to the > > MultiMSI requirements where the message is an offset from the base of > > the interrupt range. So you generally end-up with ranges of at least > > 32 contiguous MSIs. Anything under that is sub-par and probably not > > worth supporting. > > Maybe just this is enough: > msi-ranges = <intspec base>, <length>, <intspec base>, <length>, ... > > While I say 'length' here, that's really up to the interrupt parent to > interpret the intspec cells. So for the Apple PCIe controller we'd have: msi-ranges = <AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>; That would work just fine. Should this be documented in the apple,pcie binding, or somewhere more generic? > > Of course, the controller may have some mapping facilities, which > > makes things more... interesting. > > > > > It's fine for the kernel to assume that until there's a case that's > > > not linear, but a common binding needs to be able handle a > > > non-linear case. > > > > Fair enough. I can probably work with Mark to upgrade the binding and > > the M1 PCIe code. Could you come up with a more formalised proposal? > > Not my itch. > > Rob >
, On Wed, Aug 18, 2021 at 2:56 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Rob Herring <robh@kernel.org> > > Date: Sun, 15 Aug 2021 14:19:57 -0500 > > > > On Sun, Aug 15, 2021 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > Hi Rob, > > > > > > Apologies for the delay, I somehow misplaced this email... > > > > > > On Mon, 02 Aug 2021 17:10:39 +0100, > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Tue, 27 Jul 2021 00:18:48 +0100, > > > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > > > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > > > > > > > The Apple PCIe host controller is a PCIe host controller with > > > > > > > multiple root ports present in Apple ARM SoC platforms, including > > > > > > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > > > > > --- > > > > > > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > > > > > > MAINTAINERS | 1 + > > > > > > > 2 files changed, 167 insertions(+) > > > > > > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > new file mode 100644 > > > > > > > index 000000000000..bfcbdee79c64 > > > > > > > --- /dev/null > > > > > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > @@ -0,0 +1,166 @@ > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > > > +%YAML 1.2 > > > > > > > +--- > > > > > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > > + > > > > > > > +title: Apple PCIe host controller > > > > > > > + > > > > > > > +maintainers: > > > > > > > + - Mark Kettenis <kettenis@openbsd.org> > > > > > > > + > > > > > > > +description: | > > > > > > > + The Apple PCIe host controller is a PCIe host controller with > > > > > > > + multiple root ports present in Apple ARM SoC platforms, including > > > > > > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > + The controller incorporates Synopsys DesigWare PCIe logic to > > > > > > > + implements its root ports. But the ATU found on most DesignWare > > > > > > > + PCIe host bridges is absent. > > > > > > > > > > > > blank line > > > > > > > > > > > > > + All root ports share a single ECAM space, but separate GPIOs are > > > > > > > + used to take the PCI devices on those ports out of reset. Therefore > > > > > > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > > > > > > > > > > > reset-gpios > > > > > > > > > > > > > + the child nodes that represent the PCI bridges that correspond to > > > > > > > + the individual root ports. > > > > > > > > > > > > blank line > > > > > > > > > > > > > + MSIs are handled by the PCIe controller and translated into regular > > > > > > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > > > > > > + distributed over the root ports as the OS sees fit by programming > > > > > > > + the PCIe controller's port registers. > > > > > > > + > > > > > > > +allOf: > > > > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > > > > + > > > > > > > +properties: > > > > > > > + compatible: > > > > > > > + items: > > > > > > > + - const: apple,t8103-pcie > > > > > > > + - const: apple,pcie > > > > > > > + > > > > > > > + reg: > > > > > > > + minItems: 3 > > > > > > > + maxItems: 5 > > > > > > > + > > > > > > > + reg-names: > > > > > > > + minItems: 3 > > > > > > > + maxItems: 5 > > > > > > > + items: > > > > > > > + - const: config > > > > > > > + - const: rc > > > > > > > + - const: port0 > > > > > > > + - const: port1 > > > > > > > + - const: port2 > > > > > > > + > > > > > > > + ranges: > > > > > > > + minItems: 2 > > > > > > > + maxItems: 2 > > > > > > > + > > > > > > > + interrupts: > > > > > > > + description: > > > > > > > + Interrupt specifiers, one for each root port. > > > > > > > + minItems: 1 > > > > > > > + maxItems: 3 > > > > > > > + > > > > > > > + msi-controller: true > > > > > > > + msi-parent: true BTW, I don't think msi-parent and msi-controller together is valid? > > > > > > > + > > > > > > > + msi-ranges: > > > > > > > + description: > > > > > > > + A list of pairs <intid span>, where "intid" is the first > > > > > > > + interrupt number that can be used as an MSI, and "span" the size > > > > > > > + of that range. > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > > > > > + items: > > > > > > > + minItems: 2 > > > > > > > + maxItems: 2 > > > > > > > > > > > > I still have issues I raised on v1 with this property. It's genericish > > > > > > looking, but not generic. 'intid' as a single cell can't specify any > > > > > > parent interrupt such as a GIC which uses 3 cells. You could put in all > > > > > > the cells, but you'd still be assuming which cell you can increment. > > > > > > > > > > The GIC bindings already use similar abstractions, see what we do for > > > > > both GICv2m and GICv3 MBIs. Other MSI controllers use similar > > > > > properties (alpine and loongson, for example). > > > > > > > > That's the problem. Everyone making up their own crap. > > > > > > And that crap gets approved: > > > > > > https://lore.kernel.org/lkml/20200512205704.GA10412@bogus/ > > > > > > I'm not trying to be antagonistic here, but it seems that your > > > position on this very subject has changed recently. > > > > Not really, I think it's not the first time we've discussed this. But > > as I see things over and over, my tolerance for another instance > > without solving the problem for everyone diminishes. And what other > > leverage do I have? > > > > Additionally, how long we have to support something comes into play. I > > have no idea for a Loongson MSI controller. I have a better idea on an > > Apple product... > > > > > > > > I think you should just list all these under 'interrupts' using > > > > > > interrupt-names to make your life easier: > > > > > > > > > > > > interrupt-names: > > > > > > items: > > > > > > - const: port0 > > > > > > - const: port1 > > > > > > - const: port2 > > > > > > - const: msi0 > > > > > > - const: msi1 > > > > > > - const: msi2 > > > > > > - const: msi3 > > > > > > ... > > > > > > > > > > > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > > > > > > you should list N interrupts. The worst case for the above is N entries > > > > > > too if not contiguous. > > > > > > > > > > And that's where I beg to differ, again. > > > > > > > > > > Specifying interrupts like this gives the false impression that these > > > > > interrupts are generated by the device that owns them (the RC). Which > > > > > for MSIs is not the case. > > > > > > > > It's no different than an interrupt controller node having an > > > > interrupts property. The source is downstream and the interrupt > > > > controller is combining/translating the interrupts. > > > > > > > > The physical interrupt signals are connected to and originating in > > > > this block. > > > > > > Oh, I also object to this, for the same reasons. The only case where > > > it makes sense IMHO is when the interrupt controller is a multiplexer. > > > > So we've had the same kind of property for interrupt multiplexers. I'm > > fine if you think an 'MSI to interrupts mapping property' should be > > named something else. > > > > > > That sounds like perfectly 'describing the h/w' to me. > > > > > > I guess we have a different view of about these things. At the end of > > > the day, I don't care enough as long as we can expose a range of > > > interrupts one way or another. > > > > I don't really either. I just don't want 10 ways AND another... > > > > > > > This is not only verbose, this is > > > > > semantically dubious. And what should we do when the number of > > > > > possible interrupt is ridiculously large, as it is for the GICv3 ITS? > > > > > > > > I don't disagree with the verbose part. But that's not really an issue > > > > in this case. > > > > > > > > > I wish we had a standard way to express these constraints. Until we > > > > > do, I don't think enumerating individual interrupts is a practical > > > > > thing to do, nor that it actually represents the topology of the > > > > > system. > > > > > > > > The only way a standard way will happen is to stop accepting the > > > > custom properties. > > > > > > > > All the custom properties suffer from knowledge of what the parent > > > > interrupt controller is. To fix that, I think we need something like > > > > this: > > > > > > > > msi-ranges = <intspec base>, <intspec step>, <intspec end>; > > > > > > > > 'intspec' is defined by the parent interrupt-controller cells. step is > > > > the value to add. And end is what to match on to stop aka the last > > > > interrupt in the range. For example, if the GIC is the parent, we'd > > > > have something like this: > > > > > > > > <GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0> > > > > > > > > Does this apply to cases other than MSI? I think so as don't we have > > > > the same type of properties with the low power mode shadow interrupt > > > > controllers? So 'interrupt-ranges'? > > > > > > This would work, though the increment seems a bit over-engineered. You > > > also may need this property to accept multiple ranges. > > > > Yes, certainly. Worst case is a map. > > > > > > It looks to me like there's an assumption in the kernel that an MSI > > > > controller has a linear range of parent interrupts? Is that correct > > > > and something that's guaranteed? That assumption leaks into the > > > > existing bindings. > > > > > > Depends on how the controller works. In general, the range maps to the > > > MultiMSI requirements where the message is an offset from the base of > > > the interrupt range. So you generally end-up with ranges of at least > > > 32 contiguous MSIs. Anything under that is sub-par and probably not > > > worth supporting. > > > > Maybe just this is enough: > > msi-ranges = <intspec base>, <length>, <intspec base>, <length>, ... > > > > While I say 'length' here, that's really up to the interrupt parent to > > interpret the intspec cells. > > So for the Apple PCIe controller we'd have: > > msi-ranges = <AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>; > > That would work just fine. > > Should this be documented in the apple,pcie binding, or somewhere more > generic? It doesn't have an 'apple,' prefix, so somewhere generic. Probably bindings/interrupt-controller/msi.txt. Or start an msi-controller.yaml schema as I'd rather not add things we can't validate, but I don't want to gate this on converting all the MSI bindings. Someone that understands MSI better than me should review too. Another thing I thought of is the above is assuming the interrupt parent is the same as any interrupts for the node and that all MSIs go to 1 interrupt controller. Also, given Marc doesn't think using 'interrupts' is right, then using 'interrupt-parent' isn't either (though many of the examples below do just that). So maybe we need the phandle in there: msi-ranges = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>; Other examples of this type of property include: al,msi-base-spi/al,msi-base-spi arm,msi-base-spi/arm,msi-num-spis mbi-ranges loongson,msi-base-vec/loongson,msi-num-vecs marvell,spi-ranges ti,interrupt-ranges? We should make sure msi-ranges works for all of these cases at least even if we can't change them. Rob
> From: Rob Herring <robh@kernel.org> > Date: Wed, 18 Aug 2021 15:51:11 -0500 Hi Rob, > On Wed, Aug 18, 2021 at 2:56 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > From: Rob Herring <robh@kernel.org> > > > Date: Sun, 15 Aug 2021 14:19:57 -0500 > > > > > > On Sun, Aug 15, 2021 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > Hi Rob, > > > > > > > > Apologies for the delay, I somehow misplaced this email... > > > > > > > > On Mon, 02 Aug 2021 17:10:39 +0100, > > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > On Tue, 27 Jul 2021 00:18:48 +0100, > > > > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > > > > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > > > > > > > > > The Apple PCIe host controller is a PCIe host controller with > > > > > > > > multiple root ports present in Apple ARM SoC platforms, including > > > > > > > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > > > > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > --- > > > > > > > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > > > > > > > MAINTAINERS | 1 + > > > > > > > > 2 files changed, 167 insertions(+) > > > > > > > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > new file mode 100644 > > > > > > > > index 000000000000..bfcbdee79c64 > > > > > > > > --- /dev/null > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > @@ -0,0 +1,166 @@ > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > > > > +%YAML 1.2 > > > > > > > > +--- > > > > > > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > > > + > > > > > > > > +title: Apple PCIe host controller > > > > > > > > + > > > > > > > > +maintainers: > > > > > > > > + - Mark Kettenis <kettenis@openbsd.org> > > > > > > > > + > > > > > > > > +description: | > > > > > > > > + The Apple PCIe host controller is a PCIe host controller with > > > > > > > > + multiple root ports present in Apple ARM SoC platforms, including > > > > > > > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > + The controller incorporates Synopsys DesigWare PCIe logic to > > > > > > > > + implements its root ports. But the ATU found on most DesignWare > > > > > > > > + PCIe host bridges is absent. > > > > > > > > > > > > > > blank line > > > > > > > > > > > > > > > + All root ports share a single ECAM space, but separate GPIOs are > > > > > > > > + used to take the PCI devices on those ports out of reset. Therefore > > > > > > > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > > > > > > > > > > > > > reset-gpios > > > > > > > > > > > > > > > + the child nodes that represent the PCI bridges that correspond to > > > > > > > > + the individual root ports. > > > > > > > > > > > > > > blank line > > > > > > > > > > > > > > > + MSIs are handled by the PCIe controller and translated into regular > > > > > > > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > > > > > > > + distributed over the root ports as the OS sees fit by programming > > > > > > > > + the PCIe controller's port registers. > > > > > > > > + > > > > > > > > +allOf: > > > > > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > > > > > + > > > > > > > > +properties: > > > > > > > > + compatible: > > > > > > > > + items: > > > > > > > > + - const: apple,t8103-pcie > > > > > > > > + - const: apple,pcie > > > > > > > > + > > > > > > > > + reg: > > > > > > > > + minItems: 3 > > > > > > > > + maxItems: 5 > > > > > > > > + > > > > > > > > + reg-names: > > > > > > > > + minItems: 3 > > > > > > > > + maxItems: 5 > > > > > > > > + items: > > > > > > > > + - const: config > > > > > > > > + - const: rc > > > > > > > > + - const: port0 > > > > > > > > + - const: port1 > > > > > > > > + - const: port2 > > > > > > > > + > > > > > > > > + ranges: > > > > > > > > + minItems: 2 > > > > > > > > + maxItems: 2 > > > > > > > > + > > > > > > > > + interrupts: > > > > > > > > + description: > > > > > > > > + Interrupt specifiers, one for each root port. > > > > > > > > + minItems: 1 > > > > > > > > + maxItems: 3 > > > > > > > > + > > > > > > > > + msi-controller: true > > > > > > > > + msi-parent: true > > BTW, I don't think msi-parent and msi-controller together is valid? There is an existing example in: arm64/boot/dts/marvell/armada-37xx.dtsi I think it makes sense as the pcie bridge itself serves as the MSI controller. > > > > > > > > + > > > > > > > > + msi-ranges: > > > > > > > > + description: > > > > > > > > + A list of pairs <intid span>, where "intid" is the first > > > > > > > > + interrupt number that can be used as an MSI, and "span" the size > > > > > > > > + of that range. > > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > > > > > > + items: > > > > > > > > + minItems: 2 > > > > > > > > + maxItems: 2 > > > > > > > > > > > > > > I still have issues I raised on v1 with this property. It's genericish > > > > > > > looking, but not generic. 'intid' as a single cell can't specify any > > > > > > > parent interrupt such as a GIC which uses 3 cells. You could put in all > > > > > > > the cells, but you'd still be assuming which cell you can increment. > > > > > > > > > > > > The GIC bindings already use similar abstractions, see what we do for > > > > > > both GICv2m and GICv3 MBIs. Other MSI controllers use similar > > > > > > properties (alpine and loongson, for example). > > > > > > > > > > That's the problem. Everyone making up their own crap. > > > > > > > > And that crap gets approved: > > > > > > > > https://lore.kernel.org/lkml/20200512205704.GA10412@bogus/ > > > > > > > > I'm not trying to be antagonistic here, but it seems that your > > > > position on this very subject has changed recently. > > > > > > Not really, I think it's not the first time we've discussed this. But > > > as I see things over and over, my tolerance for another instance > > > without solving the problem for everyone diminishes. And what other > > > leverage do I have? > > > > > > Additionally, how long we have to support something comes into play. I > > > have no idea for a Loongson MSI controller. I have a better idea on an > > > Apple product... > > > > > > > > > > I think you should just list all these under 'interrupts' using > > > > > > > interrupt-names to make your life easier: > > > > > > > > > > > > > > interrupt-names: > > > > > > > items: > > > > > > > - const: port0 > > > > > > > - const: port1 > > > > > > > - const: port2 > > > > > > > - const: msi0 > > > > > > > - const: msi1 > > > > > > > - const: msi2 > > > > > > > - const: msi3 > > > > > > > ... > > > > > > > > > > > > > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > > > > > > > you should list N interrupts. The worst case for the above is N entries > > > > > > > too if not contiguous. > > > > > > > > > > > > And that's where I beg to differ, again. > > > > > > > > > > > > Specifying interrupts like this gives the false impression that these > > > > > > interrupts are generated by the device that owns them (the RC). Which > > > > > > for MSIs is not the case. > > > > > > > > > > It's no different than an interrupt controller node having an > > > > > interrupts property. The source is downstream and the interrupt > > > > > controller is combining/translating the interrupts. > > > > > > > > > > The physical interrupt signals are connected to and originating in > > > > > this block. > > > > > > > > Oh, I also object to this, for the same reasons. The only case where > > > > it makes sense IMHO is when the interrupt controller is a multiplexer. > > > > > > So we've had the same kind of property for interrupt multiplexers. I'm > > > fine if you think an 'MSI to interrupts mapping property' should be > > > named something else. > > > > > > > > That sounds like perfectly 'describing the h/w' to me. > > > > > > > > I guess we have a different view of about these things. At the end of > > > > the day, I don't care enough as long as we can expose a range of > > > > interrupts one way or another. > > > > > > I don't really either. I just don't want 10 ways AND another... > > > > > > > > > This is not only verbose, this is > > > > > > semantically dubious. And what should we do when the number of > > > > > > possible interrupt is ridiculously large, as it is for the GICv3 ITS? > > > > > > > > > > I don't disagree with the verbose part. But that's not really an issue > > > > > in this case. > > > > > > > > > > > I wish we had a standard way to express these constraints. Until we > > > > > > do, I don't think enumerating individual interrupts is a practical > > > > > > thing to do, nor that it actually represents the topology of the > > > > > > system. > > > > > > > > > > The only way a standard way will happen is to stop accepting the > > > > > custom properties. > > > > > > > > > > All the custom properties suffer from knowledge of what the parent > > > > > interrupt controller is. To fix that, I think we need something like > > > > > this: > > > > > > > > > > msi-ranges = <intspec base>, <intspec step>, <intspec end>; > > > > > > > > > > 'intspec' is defined by the parent interrupt-controller cells. step is > > > > > the value to add. And end is what to match on to stop aka the last > > > > > interrupt in the range. For example, if the GIC is the parent, we'd > > > > > have something like this: > > > > > > > > > > <GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0> > > > > > > > > > > Does this apply to cases other than MSI? I think so as don't we have > > > > > the same type of properties with the low power mode shadow interrupt > > > > > controllers? So 'interrupt-ranges'? > > > > > > > > This would work, though the increment seems a bit over-engineered. You > > > > also may need this property to accept multiple ranges. > > > > > > Yes, certainly. Worst case is a map. > > > > > > > > It looks to me like there's an assumption in the kernel that an MSI > > > > > controller has a linear range of parent interrupts? Is that correct > > > > > and something that's guaranteed? That assumption leaks into the > > > > > existing bindings. > > > > > > > > Depends on how the controller works. In general, the range maps to the > > > > MultiMSI requirements where the message is an offset from the base of > > > > the interrupt range. So you generally end-up with ranges of at least > > > > 32 contiguous MSIs. Anything under that is sub-par and probably not > > > > worth supporting. > > > > > > Maybe just this is enough: > > > msi-ranges = <intspec base>, <length>, <intspec base>, <length>, ... > > > > > > While I say 'length' here, that's really up to the interrupt parent to > > > interpret the intspec cells. > > > > So for the Apple PCIe controller we'd have: > > > > msi-ranges = <AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>; > > > > That would work just fine. > > > > Should this be documented in the apple,pcie binding, or somewhere more > > generic? > > It doesn't have an 'apple,' prefix, so somewhere generic. Probably > bindings/interrupt-controller/msi.txt. Or start an msi-controller.yaml > schema as I'd rather not add things we can't validate, but I don't > want to gate this on converting all the MSI bindings. Someone that > understands MSI better than me should review too. Yes, I'd like to avoid converting all the MSI bindings, but I could add an msi-controller.yaml file and use it in the appropriate interrupt controller and pci host bridge bindings that have been converted to yaml. > Another thing I thought of is the above is assuming the interrupt > parent is the same as any interrupts for the node and that all MSIs go > to 1 interrupt controller. Also, given Marc doesn't think using > 'interrupts' is right, then using 'interrupt-parent' isn't either > (though many of the examples below do just that). So maybe we need the > phandle in there: > > msi-ranges = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>; That makes sense. > Other examples of this type of property include: > al,msi-base-spi/al,msi-base-spi > arm,msi-base-spi/arm,msi-num-spis > mbi-ranges > loongson,msi-base-vec/loongson,msi-num-vecs > marvell,spi-ranges > ti,interrupt-ranges? > > We should make sure msi-ranges works for all of these cases at least > even if we can't change them. I think this scheme would work for all of these although it isn't entirely clear to me whether ti,interrupt-ranges is about just MSIs or if it also maps some other kind of interrupts. marvell,spi-ranges is an interesting one since it typically specifies two ranges of 64 MSIs. But that's something your proposal addresses just fine. I suppose we will provide a phandle for the parent interrupt controller for each individual range? If Marc agrees, I'll get working on implementing the msi-controller.yaml schema. Thanks, Mark
On Sun, Aug 22, 2021 at 12:44 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Rob Herring <robh@kernel.org> > > Date: Wed, 18 Aug 2021 15:51:11 -0500 > > Hi Rob, > > > On Wed, Aug 18, 2021 at 2:56 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > > > From: Rob Herring <robh@kernel.org> > > > > Date: Sun, 15 Aug 2021 14:19:57 -0500 > > > > > > > > On Sun, Aug 15, 2021 at 11:36 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > Hi Rob, > > > > > > > > > > Apologies for the delay, I somehow misplaced this email... > > > > > > > > > > On Mon, 02 Aug 2021 17:10:39 +0100, > > > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > > > On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > > > > > On Tue, 27 Jul 2021 00:18:48 +0100, > > > > > > > Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > > > > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > > > > > > > From: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > > > > > > > > > > > The Apple PCIe host controller is a PCIe host controller with > > > > > > > > > multiple root ports present in Apple ARM SoC platforms, including > > > > > > > > > various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > > > > > > > > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > > > > > > > > --- > > > > > > > > > .../devicetree/bindings/pci/apple,pcie.yaml | 166 ++++++++++++++++++ > > > > > > > > > MAINTAINERS | 1 + > > > > > > > > > 2 files changed, 167 insertions(+) > > > > > > > > > create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > > new file mode 100644 > > > > > > > > > index 000000000000..bfcbdee79c64 > > > > > > > > > --- /dev/null > > > > > > > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml > > > > > > > > > @@ -0,0 +1,166 @@ > > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > > > > > +%YAML 1.2 > > > > > > > > > +--- > > > > > > > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# > > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > > > > > + > > > > > > > > > +title: Apple PCIe host controller > > > > > > > > > + > > > > > > > > > +maintainers: > > > > > > > > > + - Mark Kettenis <kettenis@openbsd.org> > > > > > > > > > + > > > > > > > > > +description: | > > > > > > > > > + The Apple PCIe host controller is a PCIe host controller with > > > > > > > > > + multiple root ports present in Apple ARM SoC platforms, including > > > > > > > > > + various iPhone and iPad devices and the "Apple Silicon" Macs. > > > > > > > > > + The controller incorporates Synopsys DesigWare PCIe logic to > > > > > > > > > + implements its root ports. But the ATU found on most DesignWare > > > > > > > > > + PCIe host bridges is absent. > > > > > > > > > > > > > > > > blank line > > > > > > > > > > > > > > > > > + All root ports share a single ECAM space, but separate GPIOs are > > > > > > > > > + used to take the PCI devices on those ports out of reset. Therefore > > > > > > > > > + the standard "reset-gpio" and "max-link-speed" properties appear on > > > > > > > > > > > > > > > > reset-gpios > > > > > > > > > > > > > > > > > + the child nodes that represent the PCI bridges that correspond to > > > > > > > > > + the individual root ports. > > > > > > > > > > > > > > > > blank line > > > > > > > > > > > > > > > > > + MSIs are handled by the PCIe controller and translated into regular > > > > > > > > > + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be > > > > > > > > > + distributed over the root ports as the OS sees fit by programming > > > > > > > > > + the PCIe controller's port registers. > > > > > > > > > + > > > > > > > > > +allOf: > > > > > > > > > + - $ref: /schemas/pci/pci-bus.yaml# > > > > > > > > > + > > > > > > > > > +properties: > > > > > > > > > + compatible: > > > > > > > > > + items: > > > > > > > > > + - const: apple,t8103-pcie > > > > > > > > > + - const: apple,pcie > > > > > > > > > + > > > > > > > > > + reg: > > > > > > > > > + minItems: 3 > > > > > > > > > + maxItems: 5 > > > > > > > > > + > > > > > > > > > + reg-names: > > > > > > > > > + minItems: 3 > > > > > > > > > + maxItems: 5 > > > > > > > > > + items: > > > > > > > > > + - const: config > > > > > > > > > + - const: rc > > > > > > > > > + - const: port0 > > > > > > > > > + - const: port1 > > > > > > > > > + - const: port2 > > > > > > > > > + > > > > > > > > > + ranges: > > > > > > > > > + minItems: 2 > > > > > > > > > + maxItems: 2 > > > > > > > > > + > > > > > > > > > + interrupts: > > > > > > > > > + description: > > > > > > > > > + Interrupt specifiers, one for each root port. > > > > > > > > > + minItems: 1 > > > > > > > > > + maxItems: 3 > > > > > > > > > + > > > > > > > > > + msi-controller: true > > > > > > > > > + msi-parent: true > > > > BTW, I don't think msi-parent and msi-controller together is valid? > > There is an existing example in: > > arm64/boot/dts/marvell/armada-37xx.dtsi > > I think it makes sense as the pcie bridge itself serves as the MSI > controller. That's pretty common such as all the DWC implementations using the built-in MSI controller (in that case 'msi-parent' present means no built-in controller). AFAICT, 'msi-parent' is just redundant here because the fallback if msi-parent is not found is checking the bus's DT node for a domain. See pci_host_bridge_of_msi_domain(). > > > > > > > > > + msi-ranges: > > > > > > > > > + description: > > > > > > > > > + A list of pairs <intid span>, where "intid" is the first > > > > > > > > > + interrupt number that can be used as an MSI, and "span" the size > > > > > > > > > + of that range. > > > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > > > > > > > + items: > > > > > > > > > + minItems: 2 > > > > > > > > > + maxItems: 2 > > > > > > > > > > > > > > > > I still have issues I raised on v1 with this property. It's genericish > > > > > > > > looking, but not generic. 'intid' as a single cell can't specify any > > > > > > > > parent interrupt such as a GIC which uses 3 cells. You could put in all > > > > > > > > the cells, but you'd still be assuming which cell you can increment. > > > > > > > > > > > > > > The GIC bindings already use similar abstractions, see what we do for > > > > > > > both GICv2m and GICv3 MBIs. Other MSI controllers use similar > > > > > > > properties (alpine and loongson, for example). > > > > > > > > > > > > That's the problem. Everyone making up their own crap. > > > > > > > > > > And that crap gets approved: > > > > > > > > > > https://lore.kernel.org/lkml/20200512205704.GA10412@bogus/ > > > > > > > > > > I'm not trying to be antagonistic here, but it seems that your > > > > > position on this very subject has changed recently. > > > > > > > > Not really, I think it's not the first time we've discussed this. But > > > > as I see things over and over, my tolerance for another instance > > > > without solving the problem for everyone diminishes. And what other > > > > leverage do I have? > > > > > > > > Additionally, how long we have to support something comes into play. I > > > > have no idea for a Loongson MSI controller. I have a better idea on an > > > > Apple product... > > > > > > > > > > > > I think you should just list all these under 'interrupts' using > > > > > > > > interrupt-names to make your life easier: > > > > > > > > > > > > > > > > interrupt-names: > > > > > > > > items: > > > > > > > > - const: port0 > > > > > > > > - const: port1 > > > > > > > > - const: port2 > > > > > > > > - const: msi0 > > > > > > > > - const: msi1 > > > > > > > > - const: msi2 > > > > > > > > - const: msi3 > > > > > > > > ... > > > > > > > > > > > > > > > > Yeah, it's kind of verbose, but if the h/w block handles N interrupts, > > > > > > > > you should list N interrupts. The worst case for the above is N entries > > > > > > > > too if not contiguous. > > > > > > > > > > > > > > And that's where I beg to differ, again. > > > > > > > > > > > > > > Specifying interrupts like this gives the false impression that these > > > > > > > interrupts are generated by the device that owns them (the RC). Which > > > > > > > for MSIs is not the case. > > > > > > > > > > > > It's no different than an interrupt controller node having an > > > > > > interrupts property. The source is downstream and the interrupt > > > > > > controller is combining/translating the interrupts. > > > > > > > > > > > > The physical interrupt signals are connected to and originating in > > > > > > this block. > > > > > > > > > > Oh, I also object to this, for the same reasons. The only case where > > > > > it makes sense IMHO is when the interrupt controller is a multiplexer. > > > > > > > > So we've had the same kind of property for interrupt multiplexers. I'm > > > > fine if you think an 'MSI to interrupts mapping property' should be > > > > named something else. > > > > > > > > > > That sounds like perfectly 'describing the h/w' to me. > > > > > > > > > > I guess we have a different view of about these things. At the end of > > > > > the day, I don't care enough as long as we can expose a range of > > > > > interrupts one way or another. > > > > > > > > I don't really either. I just don't want 10 ways AND another... > > > > > > > > > > > This is not only verbose, this is > > > > > > > semantically dubious. And what should we do when the number of > > > > > > > possible interrupt is ridiculously large, as it is for the GICv3 ITS? > > > > > > > > > > > > I don't disagree with the verbose part. But that's not really an issue > > > > > > in this case. > > > > > > > > > > > > > I wish we had a standard way to express these constraints. Until we > > > > > > > do, I don't think enumerating individual interrupts is a practical > > > > > > > thing to do, nor that it actually represents the topology of the > > > > > > > system. > > > > > > > > > > > > The only way a standard way will happen is to stop accepting the > > > > > > custom properties. > > > > > > > > > > > > All the custom properties suffer from knowledge of what the parent > > > > > > interrupt controller is. To fix that, I think we need something like > > > > > > this: > > > > > > > > > > > > msi-ranges = <intspec base>, <intspec step>, <intspec end>; > > > > > > > > > > > > 'intspec' is defined by the parent interrupt-controller cells. step is > > > > > > the value to add. And end is what to match on to stop aka the last > > > > > > interrupt in the range. For example, if the GIC is the parent, we'd > > > > > > have something like this: > > > > > > > > > > > > <GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0> > > > > > > > > > > > > Does this apply to cases other than MSI? I think so as don't we have > > > > > > the same type of properties with the low power mode shadow interrupt > > > > > > controllers? So 'interrupt-ranges'? > > > > > > > > > > This would work, though the increment seems a bit over-engineered. You > > > > > also may need this property to accept multiple ranges. > > > > > > > > Yes, certainly. Worst case is a map. > > > > > > > > > > It looks to me like there's an assumption in the kernel that an MSI > > > > > > controller has a linear range of parent interrupts? Is that correct > > > > > > and something that's guaranteed? That assumption leaks into the > > > > > > existing bindings. > > > > > > > > > > Depends on how the controller works. In general, the range maps to the > > > > > MultiMSI requirements where the message is an offset from the base of > > > > > the interrupt range. So you generally end-up with ranges of at least > > > > > 32 contiguous MSIs. Anything under that is sub-par and probably not > > > > > worth supporting. > > > > > > > > Maybe just this is enough: > > > > msi-ranges = <intspec base>, <length>, <intspec base>, <length>, ... > > > > > > > > While I say 'length' here, that's really up to the interrupt parent to > > > > interpret the intspec cells. > > > > > > So for the Apple PCIe controller we'd have: > > > > > > msi-ranges = <AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>; > > > > > > That would work just fine. > > > > > > Should this be documented in the apple,pcie binding, or somewhere more > > > generic? > > > > It doesn't have an 'apple,' prefix, so somewhere generic. Probably > > bindings/interrupt-controller/msi.txt. Or start an msi-controller.yaml > > schema as I'd rather not add things we can't validate, but I don't > > want to gate this on converting all the MSI bindings. Someone that > > understands MSI better than me should review too. > > Yes, I'd like to avoid converting all the MSI bindings, but I could > add an msi-controller.yaml file and use it in the appropriate > interrupt controller and pci host bridge bindings that have been > converted to yaml. I didn't mean converting *all* MSI users, just what's in msi.txt and maybe the PCI MSI stuff. Just starting a msi-controller.yaml schema with the new properties is fine. Not that I wouldn't love for someone to do all of msi.txt. > > Another thing I thought of is the above is assuming the interrupt > > parent is the same as any interrupts for the node and that all MSIs go > > to 1 interrupt controller. Also, given Marc doesn't think using > > 'interrupts' is right, then using 'interrupt-parent' isn't either > > (though many of the examples below do just that). So maybe we need the > > phandle in there: > > > > msi-ranges = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>; > > That makes sense. > > > Other examples of this type of property include: > > al,msi-base-spi/al,msi-base-spi > > arm,msi-base-spi/arm,msi-num-spis > > mbi-ranges > > loongson,msi-base-vec/loongson,msi-num-vecs > > marvell,spi-ranges > > ti,interrupt-ranges? > > > > We should make sure msi-ranges works for all of these cases at least > > even if we can't change them. > > I think this scheme would work for all of these although it isn't > entirely clear to me whether ti,interrupt-ranges is about just MSIs or > if it also maps some other kind of interrupts. > > marvell,spi-ranges is an interesting one since it typically specifies > two ranges of 64 MSIs. But that's something your proposal addresses > just fine. I suppose we will provide a phandle for the parent > interrupt controller for each individual range? Right. Rob
diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml new file mode 100644 index 000000000000..bfcbdee79c64 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml @@ -0,0 +1,166 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple PCIe host controller + +maintainers: + - Mark Kettenis <kettenis@openbsd.org> + +description: | + The Apple PCIe host controller is a PCIe host controller with + multiple root ports present in Apple ARM SoC platforms, including + various iPhone and iPad devices and the "Apple Silicon" Macs. + The controller incorporates Synopsys DesigWare PCIe logic to + implements its root ports. But the ATU found on most DesignWare + PCIe host bridges is absent. + All root ports share a single ECAM space, but separate GPIOs are + used to take the PCI devices on those ports out of reset. Therefore + the standard "reset-gpio" and "max-link-speed" properties appear on + the child nodes that represent the PCI bridges that correspond to + the individual root ports. + MSIs are handled by the PCIe controller and translated into regular + interrupts. A range of 32 MSIs is provided. These 32 MSIs can be + distributed over the root ports as the OS sees fit by programming + the PCIe controller's port registers. + +allOf: + - $ref: /schemas/pci/pci-bus.yaml# + +properties: + compatible: + items: + - const: apple,t8103-pcie + - const: apple,pcie + + reg: + minItems: 3 + maxItems: 5 + + reg-names: + minItems: 3 + maxItems: 5 + items: + - const: config + - const: rc + - const: port0 + - const: port1 + - const: port2 + + ranges: + minItems: 2 + maxItems: 2 + + interrupts: + description: + Interrupt specifiers, one for each root port. + minItems: 1 + maxItems: 3 + + msi-controller: true + msi-parent: true + + msi-ranges: + description: + A list of pairs <intid span>, where "intid" is the first + interrupt number that can be used as an MSI, and "span" the size + of that range. + $ref: /schemas/types.yaml#/definitions/uint32-matrix + items: + minItems: 2 + maxItems: 2 + + iommu-map: true + iommu-map-mask: true + +required: + - compatible + - reg + - reg-names + - bus-range + - interrupts + - msi-controller + - msi-parent + - msi-ranges + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/apple-aic.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + pcie0: pcie@690000000 { + compatible = "apple,t8103-pcie", "apple,pcie"; + device_type = "pci"; + + reg = <0x6 0x90000000 0x0 0x1000000>, + <0x6 0x80000000 0x0 0x4000>, + <0x6 0x81000000 0x0 0x8000>, + <0x6 0x82000000 0x0 0x8000>, + <0x6 0x83000000 0x0 0x8000>; + reg-names = "config", "rc", "port0", "port1", "port2"; + + interrupt-parent = <&aic>; + interrupts = <AIC_IRQ 695 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 698 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 701 IRQ_TYPE_LEVEL_HIGH>; + + msi-controller; + msi-parent = <&pcie0>; + msi-ranges = <704 32>; + + iommu-map = <0x100 &dart0 1 1>, + <0x200 &dart1 1 1>, + <0x300 &dart2 1 1>; + iommu-map-mask = <0xff00>; + + bus-range = <0 3>; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x43000000 0x6 0xa0000000 0x6 0xa0000000 0x0 0x20000000>, + <0x02000000 0x0 0xc0000000 0x6 0xc0000000 0x0 0x40000000>; + + clocks = <&pcie_core_clk>, <&pcie_aux_clk>, <&pcie_ref_clk>; + pinctrl-0 = <&pcie_pins>; + pinctrl-names = "default"; + + pci@0,0 { + device_type = "pci"; + reg = <0x0 0x0 0x0 0x0 0x0>; + reset-gpios = <&pinctrl_ap 152 0>; + max-link-speed = <2>; + + #address-cells = <3>; + #size-cells = <2>; + ranges; + }; + + pci@1,0 { + device_type = "pci"; + reg = <0x800 0x0 0x0 0x0 0x0>; + reset-gpios = <&pinctrl_ap 153 0>; + max-link-speed = <2>; + + #address-cells = <3>; + #size-cells = <2>; + ranges; + }; + + pci@2,0 { + device_type = "pci"; + reg = <0x1000 0x0 0x0 0x0 0x0>; + reset-gpios = <&pinctrl_ap 33 0>; + max-link-speed = <1>; + + #address-cells = <3>; + #size-cells = <2>; + ranges; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 19135a9d778e..f4aa40d3166e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1694,6 +1694,7 @@ C: irc://chat.freenode.net/asahi-dev T: git https://github.com/AsahiLinux/linux.git F: Documentation/devicetree/bindings/arm/apple.yaml F: Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml +F: Documentation/devicetree/bindings/pci/apple,pcie.yaml F: Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml F: arch/arm64/boot/dts/apple/ F: drivers/irqchip/irq-apple-aic.c