diff mbox series

[v4,3/4] dt-bindings: pci: Add DT bindings for apple,pcie

Message ID 20210827171534.62380-4-mark.kettenis@xs4all.nl (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series Apple M1 PCIe DT bindings | expand

Commit Message

Mark Kettenis Aug. 27, 2021, 5:15 p.m. UTC
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   | 165 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 166 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml

Comments

Alyssa Rosenzweig Aug. 27, 2021, 5:58 p.m. UTC | #1
> +#  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/phandle-array

Comment intended?
Mark Kettenis Aug. 27, 2021, 6:22 p.m. UTC | #2
> Date: Fri, 27 Aug 2021 17:58:45 +0000
> From: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> 
> > +#  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/phandle-array
> 
> Comment intended?

Hmm, no.  I commented it out here in the process of moving it to the
new MSI controller binding schema.  And then forgot about it.  I
suspect I'll need a v5 of the series anyway so I'll fix it there.
Rob Herring (Arm) Aug. 31, 2021, 9:21 p.m. UTC | #3
On Fri, Aug 27, 2021 at 07:15:28PM +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   | 165 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 166 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..97a126db935a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> @@ -0,0 +1,165 @@
> +# 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-gpios" 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#
> +  - $ref: ../interrupt-controller/msi-controller.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-parent: true

I still think this should be dropped as it is meaningless with 
'msi-controller' present.

> +
> +#  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/phandle-array

Here, you'll want just 'maxItems: 1' as there's only 1 entry.

> +
> +  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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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 c6b8a720c0bc..30bea4042e7e 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
> -- 
> 2.32.0
> 
>
Mark Kettenis Sept. 1, 2021, 11:29 a.m. UTC | #4
> Date: Tue, 31 Aug 2021 16:21:28 -0500
> From: Rob Herring <robh@kernel.org>
> 
> On Fri, Aug 27, 2021 at 07:15:28PM +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   | 165 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 166 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..97a126db935a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > @@ -0,0 +1,165 @@
> > +# 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-gpios" 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#
> > +  - $ref: ../interrupt-controller/msi-controller.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-parent: true
> 
> I still think this should be dropped as it is meaningless with 
> 'msi-controller' present.

Hmm.  As far as I can tell all current arm64 device trees that
describe hardware with an MSI controller integrated on the PCI host
bridge have both the 'msi-controller' and 'msi-parent' properties.
See arch/arm64/boot/dts/marvell/aramada-37xx.dtsi and
arch/arm64/boot/dts/xilinx/zynqmp.dtsi.

The current OpenBSD code will fail to map the MSIs if 'msi-parent'
isn't there, although Linux seems to fall back on an MSI domain that's
directly attached to the host bridge if the 'msi-parent' property is
missing.  I think it makes sense to be explicit here, but if both you
and Marc think it shouldn't be there, I probably can change the
OpenBSD to do a similar fallback.

> > +
> > +#  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/phandle-array
> 
> Here, you'll want just 'maxItems: 1' as there's only 1 entry.

Right.  As far as I can tell the Apple hardware only supports a single
range.

> > +
> > +  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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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 c6b8a720c0bc..30bea4042e7e 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
> > -- 
> > 2.32.0
> > 
> > 
>
Marc Zyngier Sept. 12, 2021, 8:13 p.m. UTC | #5
On Wed, 01 Sep 2021 12:29:22 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > Date: Tue, 31 Aug 2021 16:21:28 -0500
> > From: Rob Herring <robh@kernel.org>
> > 
> > On Fri, Aug 27, 2021 at 07:15:28PM +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   | 165 ++++++++++++++++++
> > >  MAINTAINERS                                   |   1 +
> > >  2 files changed, 166 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..97a126db935a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > @@ -0,0 +1,165 @@
> > > +# 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-gpios" 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#
> > > +  - $ref: ../interrupt-controller/msi-controller.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-parent: true
> > 
> > I still think this should be dropped as it is meaningless with 
> > 'msi-controller' present.
> 
> Hmm.  As far as I can tell all current arm64 device trees that
> describe hardware with an MSI controller integrated on the PCI host
> bridge have both the 'msi-controller' and 'msi-parent' properties.
> See arch/arm64/boot/dts/marvell/aramada-37xx.dtsi and
> arch/arm64/boot/dts/xilinx/zynqmp.dtsi.
> 
> The current OpenBSD code will fail to map the MSIs if 'msi-parent'
> isn't there, although Linux seems to fall back on an MSI domain that's
> directly attached to the host bridge if the 'msi-parent' property is
> missing.  I think it makes sense to be explicit here, but if both you
> and Marc think it shouldn't be there, I probably can change the
> OpenBSD to do a similar fallback.

I think this matches the behaviour we have for interrupt-controller vs
interrupt-parent. I fail to see why msi-controller/msi-parent should
behave differently. And since there is an established OS that actually
requires this, I don't see how we can today make it illegal.

	M.
Rob Herring (Arm) Sept. 13, 2021, 8:55 p.m. UTC | #6
msi-On Sun, Sep 12, 2021 at 3:13 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 01 Sep 2021 12:29:22 +0100,
> Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > Date: Tue, 31 Aug 2021 16:21:28 -0500
> > > From: Rob Herring <robh@kernel.org>
> > >
> > > On Fri, Aug 27, 2021 at 07:15:28PM +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   | 165 ++++++++++++++++++
> > > >  MAINTAINERS                                   |   1 +
> > > >  2 files changed, 166 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..97a126db935a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > > @@ -0,0 +1,165 @@
> > > > +# 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-gpios" 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#
> > > > +  - $ref: ../interrupt-controller/msi-controller.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-parent: true
> > >
> > > I still think this should be dropped as it is meaningless with
> > > 'msi-controller' present.
> >
> > Hmm.  As far as I can tell all current arm64 device trees that
> > describe hardware with an MSI controller integrated on the PCI host
> > bridge have both the 'msi-controller' and 'msi-parent' properties.
> > See arch/arm64/boot/dts/marvell/aramada-37xx.dtsi and
> > arch/arm64/boot/dts/xilinx/zynqmp.dtsi.

Humm, maybe it is the DWC based bindings and driver that are wrong
here. All the ones with an 'msi' interrupt (i.e. the DWC built-in MSI
controller) have neither 'msi-parent' nor 'msi-controller' property.

> > The current OpenBSD code will fail to map the MSIs if 'msi-parent'
> > isn't there, although Linux seems to fall back on an MSI domain that's
> > directly attached to the host bridge if the 'msi-parent' property is
> > missing.  I think it makes sense to be explicit here, but if both you
> > and Marc think it shouldn't be there, I probably can change the
> > OpenBSD to do a similar fallback.
>
> I think this matches the behaviour we have for interrupt-controller vs
> interrupt-parent. I fail to see why msi-controller/msi-parent should
> behave differently. And since there is an established OS that actually
> requires this, I don't see how we can today make it illegal.

Which behavior exactly? An interrupt-controller node with
interrupt-parent pointing to itself? That makes little sense. As far
as finding the parent, the behavior for interrupts is if
'interrupt-controller' is found in a parent node, then that is the
interrupt parent unless 'interrupt-parent' is found. That is the same
behavior I'm suggesting here.

But yes, if this is already needed, then we need to keep it. However,
then my concern is other platforms working on OpenBSD if Linux allows
something that OpenBSD does not.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
new file mode 100644
index 000000000000..97a126db935a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
@@ -0,0 +1,165 @@ 
+# 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-gpios" 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#
+  - $ref: ../interrupt-controller/msi-controller.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-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/phandle-array
+
+  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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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 c6b8a720c0bc..30bea4042e7e 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