[PATCH v3 0/2] Apple M1 PCIe DT bindings

[PATCH v3 0/2] Apple M1 PCIe DT bindings

[Mark Kettenis]

From: Mark Kettenis <kettenis@openbsd.org>

This small series adds bindings for the PCIe controller found on the
Apple M1 SoC.

At this point, the primary consumer for these bindings is U-Boot.
With these bindings U-Boot can bring up the links for the root ports
of the PCIe root complex.  A simple OS driver can then provide
standard ECAM access and manage MSI interrupts to provide access
to the built-in Ethernet and XHCI controllers of the Mac mini.

The Apple controller incorporates Synopsys Designware PCIe logic
to implement its root port.  But unlike other hardware currently
supported by U-Boot and the Linux kernel the Apple hardware
integrates multiple root ports.  As such the existing bindings
for the DWC PCIe interface can't be used.  There is a single ECAM
space for all root space, but separate GPIOs 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 representing
the PCI devices 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 port registers.

I still hope to hear from Marc Zyngier on the way MSIs are represented
in this binding.

Patch 2/2 of this series depends on the pinctrl series I sent earlier
and will probably go through Hector Martin's Apple M1 SoC tree.


Changelog:

v3: - Remove unneeded include in example

v2: - Adjust name for ECAM in "reg-names"
    - Drop "phy" registers
    - Expand description
    - Add description for "interrupts"
    - Fix incorrect minItems for "interrupts"
    - Fix incorrect MaxItems for "reg-names"
    - Document the use of "msi-controller", "msi-parent", "iommu-map" and
      "iommu-map-mask"
    - Fix "bus-range" and "iommu-map" properties in the example

Mark Kettenis (2):
  dt-bindings: pci: Add DT bindings for apple,pcie
  arm64: apple: Add PCIe node

 .../devicetree/bindings/pci/apple,pcie.yaml   | 166 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 arch/arm64/boot/dts/apple/t8103.dtsi          |  63 +++++++
 3 files changed, 230 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml

-- 
2.32.0

[PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Mark Kettenis]

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.
+  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
-- 
2.32.0

[PATCH v3 2/2] arm64: apple: Add PCIe node

[Mark Kettenis]

From: Mark Kettenis <kettenis@openbsd.org>

Add node corresponding to the apcie,t8103 node in the
Apple device tree for the Mac mini (M1, 2020).

Clock references and DART (IOMMU) references are left out at the
moment and will be added once the appropriate bindings have been
settled upon.

Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 63 ++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 503a76fc30e6..cd3ebb940e86 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -214,5 +214,68 @@ pinctrl_smc: pinctrl@23e820000 {
 				     <AIC_IRQ 396 IRQ_TYPE_LEVEL_HIGH>,
 				     <AIC_IRQ 397 IRQ_TYPE_LEVEL_HIGH>;
 		};
+
+		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>;
+
+			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>;
+
+			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;
+			};
+		};
 	};
 };
-- 
2.32.0

Re: [PATCH v3 0/2] Apple M1 PCIe DT bindings

[Marc Zyngier]

On Mon, 26 Jul 2021 09:31:59 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> From: Mark Kettenis <kettenis@openbsd.org>
> 
> This small series adds bindings for the PCIe controller found on the
> Apple M1 SoC.
> 
> At this point, the primary consumer for these bindings is U-Boot.
> With these bindings U-Boot can bring up the links for the root ports
> of the PCIe root complex.  A simple OS driver can then provide
> standard ECAM access and manage MSI interrupts to provide access
> to the built-in Ethernet and XHCI controllers of the Mac mini.
> 
> The Apple controller incorporates Synopsys Designware PCIe logic
> to implement its root port.  But unlike other hardware currently
> supported by U-Boot and the Linux kernel the Apple hardware
> integrates multiple root ports.  As such the existing bindings
> for the DWC PCIe interface can't be used.  There is a single ECAM
> space for all root space, but separate GPIOs 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 representing
> the PCI devices 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 port registers.
> 
> I still hope to hear from Marc Zyngier on the way MSIs are represented
> in this binding.
> 
> Patch 2/2 of this series depends on the pinctrl series I sent earlier
> and will probably go through Hector Martin's Apple M1 SoC tree.
> 
> 
> Changelog:
> 
> v3: - Remove unneeded include in example
> 
> v2: - Adjust name for ECAM in "reg-names"
>     - Drop "phy" registers
>     - Expand description
>     - Add description for "interrupts"
>     - Fix incorrect minItems for "interrupts"
>     - Fix incorrect MaxItems for "reg-names"
>     - Document the use of "msi-controller", "msi-parent", "iommu-map" and
>       "iommu-map-mask"
>     - Fix "bus-range" and "iommu-map" properties in the example
> 
> Mark Kettenis (2):
>   dt-bindings: pci: Add DT bindings for apple,pcie
>   arm64: apple: Add PCIe node
> 
>  .../devicetree/bindings/pci/apple,pcie.yaml   | 166 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  arch/arm64/boot/dts/apple/t8103.dtsi          |  63 +++++++
>  3 files changed, 230 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml

Thanks a log for doing this! For the whole series:

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Rob Herring]

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.

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.

Rob

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Mark Kettenis]

> 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.

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Marc Zyngier]

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.

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Rob Herring]

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

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Marc Zyngier]

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.

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Rob Herring]

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

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Mark Kettenis]

> 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
> 

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Rob Herring]

,

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

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Mark Kettenis]

> 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

Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie

[Rob Herring]

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