linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/4] Apple M1 PCIe DT bindings
@ 2021-08-27 17:15 Mark Kettenis
  2021-08-27 17:15 ` [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema Mark Kettenis
                   ` (4 more replies)
  0 siblings, 5 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 17:15 UTC (permalink / raw)
  To: devicetree
  Cc: alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Rob Herring, Hector Martin, Bjorn Helgaas, Florian Fainelli,
	bcm-kernel-feedback-list, Nicolas Saenz Julienne, Jim Quinlan,
	Daire McNamara, Saenz Julienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

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.

This now adds an MSI controller binding schema and uses the generic
msi-ranges property to specify how the MSIs are mapped to interrupts
on the AIC.  I copied some of the description text in the MSI
controller binding schema from msi.txt but it may need some further
tweaks to make sense.

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:

v4: - Convert MSI controller binding to YAML
    - Add generic msi-ranges property to MSI controller binding
    - Fix typos/formatting in apple,pcie binding
    - Use generic MSI controller binding in apple,pcie

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 (4):
  dt-bindings: interrupt-controller: Convert MSI controller to
    json-schema
  dt-bindings: interrupt-controller: msi: Add msi-ranges property
  dt-bindings: pci: Add DT bindings for apple,pcie
  arm64: apple: Add PCIe node

 .../interrupt-controller/msi-controller.yaml  |  42 +++++
 .../devicetree/bindings/pci/apple,pcie.yaml   | 165 ++++++++++++++++++
 .../bindings/pci/brcm,stb-pcie.yaml           |   1 +
 .../bindings/pci/microchip,pcie-host.yaml     |   1 +
 MAINTAINERS                                   |   1 +
 arch/arm64/boot/dts/apple/t8103.dtsi          |  63 +++++++
 6 files changed, 273 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
 create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml

-- 
2.32.0


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema
  2021-08-27 17:15 [PATCH v4 0/4] Apple M1 PCIe DT bindings Mark Kettenis
@ 2021-08-27 17:15 ` Mark Kettenis
  2021-08-27 19:15   ` Mark Kettenis
                     ` (2 more replies)
  2021-08-27 17:15 ` [PATCH v4 2/4] dt-bindings: interrupt-controller: msi: Add msi-ranges property Mark Kettenis
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 17:15 UTC (permalink / raw)
  To: devicetree
  Cc: alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Rob Herring, Hector Martin, Bjorn Helgaas,
	Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Jim Quinlan, Daire McNamara,
	Saenz Julienne, linux-kernel, linux-arm-kernel, linux-pci,
	linux-rpi-kernel

From: Mark Kettenis <kettenis@openbsd.org>

Split the MSI controller bindings from the MSI binding document
into DT schema format using json-schema.

Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
---
 .../interrupt-controller/msi-controller.yaml  | 34 +++++++++++++++++++
 .../bindings/pci/brcm,stb-pcie.yaml           |  1 +
 .../bindings/pci/microchip,pcie-host.yaml     |  1 +
 3 files changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
new file mode 100644
index 000000000000..5ed6cd46e2e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/msi-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MSI controller
+
+maintainers:
+  - Marc Zyngier <marc.zyngier@arm.com>
+
+description: |
+  An MSI controller signals interrupts to a CPU when a write is made
+  to an MMIO address by some master. An MSI controller may feature a
+  number of doorbells.
+
+properties:
+  "#msi-cells":
+    description: |
+      The number of cells in an msi-specifier, required if not zero.
+
+      Typically this will encode information related to sideband data,
+      and will not encode doorbells or payloads as these can be
+      configured dynamically.
+
+      The meaning of the msi-specifier is defined by the device tree
+      binding of the specific MSI controller.
+
+  msi-controller:
+    description:
+      Identifies the node as an MSI controller.
+    $ref: /schemas/types.yaml#/definitions/flag
+
+additionalProperties: true
diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index b9589a0daa5c..5c67976a8dc2 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -88,6 +88,7 @@ required:
 
 allOf:
   - $ref: /schemas/pci/pci-bus.yaml#
+  - $ref: ../interrupt-controller/msi-controller.yaml#
   - if:
       properties:
         compatible:
diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
index fb95c276a986..684d9d036f48 100644
--- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
+++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
@@ -11,6 +11,7 @@ maintainers:
 
 allOf:
   - $ref: /schemas/pci/pci-bus.yaml#
+  - $ref: ../interrupt-controller/msi-controller.yaml#
 
 properties:
   compatible:
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 2/4] dt-bindings: interrupt-controller: msi: Add msi-ranges property
  2021-08-27 17:15 [PATCH v4 0/4] Apple M1 PCIe DT bindings Mark Kettenis
  2021-08-27 17:15 ` [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema Mark Kettenis
@ 2021-08-27 17:15 ` Mark Kettenis
  2021-08-31 21:16   ` Rob Herring
  2021-08-27 17:15 ` [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie Mark Kettenis
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 17:15 UTC (permalink / raw)
  To: devicetree
  Cc: alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Rob Herring, Hector Martin, Bjorn Helgaas, Jim Quinlan,
	Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Daire McNamara, Saenz Julienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

From: Mark Kettenis <kettenis@openbsd.org>

Update the MSI controller binding to add an msi-ranges property
that specifies how MSIs map onto regular interrupts on some other
interrupt controller.

Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
---
 .../bindings/interrupt-controller/msi-controller.yaml     | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
index 5ed6cd46e2e0..bf8b8a7dba09 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
@@ -31,4 +31,12 @@ properties:
       Identifies the node as an MSI controller.
     $ref: /schemas/types.yaml#/definitions/flag
 
+  msi-ranges:
+    description:
+      A list of pairs <intid span>, where "intid" is the specification
+      of the first interrupt (including the phandle for the interrupt
+      controller) that can be used as an MSI, and "span" the size of
+      that range. Multiple ranges can be provided.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+
 additionalProperties: true
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
  2021-08-27 17:15 [PATCH v4 0/4] Apple M1 PCIe DT bindings Mark Kettenis
  2021-08-27 17:15 ` [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema Mark Kettenis
  2021-08-27 17:15 ` [PATCH v4 2/4] dt-bindings: interrupt-controller: msi: Add msi-ranges property Mark Kettenis
@ 2021-08-27 17:15 ` Mark Kettenis
  2021-08-27 17:58   ` Alyssa Rosenzweig
  2021-08-31 21:21   ` Rob Herring
  2021-08-27 17:15 ` [PATCH v4 4/4] arm64: apple: Add PCIe node Mark Kettenis
  2021-09-21 11:01 ` [PATCH v4 0/4] Apple M1 PCIe DT bindings Marc Zyngier
  4 siblings, 2 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 17:15 UTC (permalink / raw)
  To: devicetree
  Cc: alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Rob Herring, Hector Martin, Bjorn Helgaas, Jim Quinlan,
	Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Daire McNamara, Saenz Julienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

From: Mark Kettenis <kettenis@openbsd.org>

The Apple PCIe host controller is a PCIe host controller with
multiple root ports present in Apple ARM SoC platforms, including
various iPhone and iPad devices and the "Apple Silicon" Macs.

Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
---
 .../devicetree/bindings/pci/apple,pcie.yaml   | 165 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 166 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
new file mode 100644
index 000000000000..97a126db935a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
@@ -0,0 +1,165 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/apple,pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple PCIe host controller
+
+maintainers:
+  - Mark Kettenis <kettenis@openbsd.org>
+
+description: |
+  The Apple PCIe host controller is a PCIe host controller with
+  multiple root ports present in Apple ARM SoC platforms, including
+  various iPhone and iPad devices and the "Apple Silicon" Macs.
+  The controller incorporates Synopsys DesigWare PCIe logic to
+  implements its root ports.  But the ATU found on most DesignWare
+  PCIe host bridges is absent.
+
+  All root ports share a single ECAM space, but separate GPIOs are
+  used to take the PCI devices on those ports out of reset.  Therefore
+  the standard "reset-gpios" and "max-link-speed" properties appear on
+  the child nodes that represent the PCI bridges that correspond to
+  the individual root ports.
+
+  MSIs are handled by the PCIe controller and translated into regular
+  interrupts.  A range of 32 MSIs is provided.  These 32 MSIs can be
+  distributed over the root ports as the OS sees fit by programming
+  the PCIe controller's port registers.
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+  - $ref: ../interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: apple,t8103-pcie
+      - const: apple,pcie
+
+  reg:
+    minItems: 3
+    maxItems: 5
+
+  reg-names:
+    minItems: 3
+    maxItems: 5
+    items:
+      - const: config
+      - const: rc
+      - const: port0
+      - const: port1
+      - const: port2
+
+  ranges:
+    minItems: 2
+    maxItems: 2
+
+  interrupts:
+    description:
+      Interrupt specifiers, one for each root port.
+    minItems: 1
+    maxItems: 3
+
+  msi-parent: true
+
+#  msi-ranges:
+#    description:
+#      A list of pairs <intid span>, where "intid" is the first
+#      interrupt number that can be used as an MSI, and "span" the size
+#      of that range.
+#    $ref: /schemas/types.yaml#/definitions/phandle-array
+
+  iommu-map: true
+  iommu-map-mask: true
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - bus-range
+  - interrupts
+  - msi-controller
+  - msi-parent
+  - msi-ranges
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/apple-aic.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      pcie0: pcie@690000000 {
+        compatible = "apple,t8103-pcie", "apple,pcie";
+        device_type = "pci";
+
+        reg = <0x6 0x90000000 0x0 0x1000000>,
+              <0x6 0x80000000 0x0 0x4000>,
+              <0x6 0x81000000 0x0 0x8000>,
+              <0x6 0x82000000 0x0 0x8000>,
+              <0x6 0x83000000 0x0 0x8000>;
+        reg-names = "config", "rc", "port0", "port1", "port2";
+
+        interrupt-parent = <&aic>;
+        interrupts = <AIC_IRQ 695 IRQ_TYPE_LEVEL_HIGH>,
+                     <AIC_IRQ 698 IRQ_TYPE_LEVEL_HIGH>,
+                     <AIC_IRQ 701 IRQ_TYPE_LEVEL_HIGH>;
+
+        msi-controller;
+        msi-parent = <&pcie0>;
+        msi-ranges = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>;
+
+        iommu-map = <0x100 &dart0 1 1>,
+                    <0x200 &dart1 1 1>,
+                    <0x300 &dart2 1 1>;
+        iommu-map-mask = <0xff00>;
+
+        bus-range = <0 3>;
+        #address-cells = <3>;
+        #size-cells = <2>;
+        ranges = <0x43000000 0x6 0xa0000000 0x6 0xa0000000 0x0 0x20000000>,
+                 <0x02000000 0x0 0xc0000000 0x6 0xc0000000 0x0 0x40000000>;
+
+        clocks = <&pcie_core_clk>, <&pcie_aux_clk>, <&pcie_ref_clk>;
+        pinctrl-0 = <&pcie_pins>;
+        pinctrl-names = "default";
+
+        pci@0,0 {
+          device_type = "pci";
+          reg = <0x0 0x0 0x0 0x0 0x0>;
+          reset-gpios = <&pinctrl_ap 152 0>;
+          max-link-speed = <2>;
+
+          #address-cells = <3>;
+          #size-cells = <2>;
+          ranges;
+        };
+
+        pci@1,0 {
+          device_type = "pci";
+          reg = <0x800 0x0 0x0 0x0 0x0>;
+          reset-gpios = <&pinctrl_ap 153 0>;
+          max-link-speed = <2>;
+
+          #address-cells = <3>;
+          #size-cells = <2>;
+          ranges;
+        };
+
+        pci@2,0 {
+          device_type = "pci";
+          reg = <0x1000 0x0 0x0 0x0 0x0>;
+          reset-gpios = <&pinctrl_ap 33 0>;
+          max-link-speed = <1>;
+
+          #address-cells = <3>;
+          #size-cells = <2>;
+          ranges;
+        };
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index c6b8a720c0bc..30bea4042e7e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1694,6 +1694,7 @@ C:	irc://chat.freenode.net/asahi-dev
 T:	git https://github.com/AsahiLinux/linux.git
 F:	Documentation/devicetree/bindings/arm/apple.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
+F:	Documentation/devicetree/bindings/pci/apple,pcie.yaml
 F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
 F:	arch/arm64/boot/dts/apple/
 F:	drivers/irqchip/irq-apple-aic.c
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-27 17:15 [PATCH v4 0/4] Apple M1 PCIe DT bindings Mark Kettenis
                   ` (2 preceding siblings ...)
  2021-08-27 17:15 ` [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie Mark Kettenis
@ 2021-08-27 17:15 ` Mark Kettenis
  2021-08-27 17:59   ` Alyssa Rosenzweig
                     ` (2 more replies)
  2021-09-21 11:01 ` [PATCH v4 0/4] Apple M1 PCIe DT bindings Marc Zyngier
  4 siblings, 3 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 17:15 UTC (permalink / raw)
  To: devicetree
  Cc: alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Rob Herring, Hector Martin, Bjorn Helgaas,
	Nicolas Saenz Julienne, Jim Quinlan, Florian Fainelli,
	bcm-kernel-feedback-list, Daire McNamara, Saenz Julienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

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..6e4677bdef44 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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
  2021-08-27 17:15 ` [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie Mark Kettenis
@ 2021-08-27 17:58   ` Alyssa Rosenzweig
  2021-08-27 18:22     ` Mark Kettenis
  2021-08-31 21:21   ` Rob Herring
  1 sibling, 1 reply; 29+ messages in thread
From: Alyssa Rosenzweig @ 2021-08-27 17:58 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Rob Herring, Hector Martin, Bjorn Helgaas, Jim Quinlan,
	Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Daire McNamara, Saenz Julienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

> +#  msi-ranges:
> +#    description:
> +#      A list of pairs <intid span>, where "intid" is the first
> +#      interrupt number that can be used as an MSI, and "span" the size
> +#      of that range.
> +#    $ref: /schemas/types.yaml#/definitions/phandle-array

Comment intended?

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-27 17:15 ` [PATCH v4 4/4] arm64: apple: Add PCIe node Mark Kettenis
@ 2021-08-27 17:59   ` Alyssa Rosenzweig
  2021-08-27 18:24     ` Mark Kettenis
  2021-08-30 11:37   ` Marc Zyngier
  2021-09-12 21:30   ` Marc Zyngier
  2 siblings, 1 reply; 29+ messages in thread
From: Alyssa Rosenzweig @ 2021-08-27 17:59 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Rob Herring, Hector Martin, Bjorn Helgaas,
	Nicolas Saenz Julienne, Jim Quinlan, Florian Fainelli,
	bcm-kernel-feedback-list, Daire McNamara, Saenz Julienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

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

DART is in mainline .... is there a PCIe specific issue?

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
  2021-08-27 17:58   ` Alyssa Rosenzweig
@ 2021-08-27 18:22     ` Mark Kettenis
  0 siblings, 0 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 18:22 UTC (permalink / raw)
  To: Alyssa Rosenzweig
  Cc: devicetree, kettenis, tglx, maz, robh+dt, marcan, bhelgaas,
	jim2101024, nsaenz, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

> Date: Fri, 27 Aug 2021 17:58:45 +0000
> From: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> 
> > +#  msi-ranges:
> > +#    description:
> > +#      A list of pairs <intid span>, where "intid" is the first
> > +#      interrupt number that can be used as an MSI, and "span" the size
> > +#      of that range.
> > +#    $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Comment intended?

Hmm, no.  I commented it out here in the process of moving it to the
new MSI controller binding schema.  And then forgot about it.  I
suspect I'll need a v5 of the series anyway so I'll fix it there.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-27 17:59   ` Alyssa Rosenzweig
@ 2021-08-27 18:24     ` Mark Kettenis
  2021-08-27 20:09       ` Alyssa Rosenzweig
  0 siblings, 1 reply; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 18:24 UTC (permalink / raw)
  To: Alyssa Rosenzweig
  Cc: devicetree, kettenis, tglx, maz, robh+dt, marcan, bhelgaas,
	nsaenz, jim2101024, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

> Date: Fri, 27 Aug 2021 17:59:59 +0000
> From: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> 
> > Clock references and DART (IOMMU) references are left out at the
> > moment and will be added once the appropriate bindings have been
> > settled upon.
> > 
> 
> DART is in mainline .... is there a PCIe specific issue?

True.  I don't expect 4/4 to be merged as part of this series though
as it will need to go through marcan's tree.  It is mostly there to
show what the device tree will look like.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema
  2021-08-27 17:15 ` [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema Mark Kettenis
@ 2021-08-27 19:15   ` Mark Kettenis
  2021-08-31 21:04     ` Rob Herring
  2021-08-31 20:57   ` Rob Herring
  2021-08-31 20:58   ` Rob Herring
  2 siblings, 1 reply; 29+ messages in thread
From: Mark Kettenis @ 2021-08-27 19:15 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, kettenis, tglx, maz, robh+dt, marcan,
	bhelgaas, nsaenz, f.fainelli, bcm-kernel-feedback-list,
	jim2101024, daire.mcnamara, nsaenzjulienne, linux-kernel,
	linux-arm-kernel, linux-pci, linux-rpi-kernel

> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> Date: Fri, 27 Aug 2021 19:15:26 +0200
> 
> From: Mark Kettenis <kettenis@openbsd.org>
> 
> Split the MSI controller bindings from the MSI binding document
> into DT schema format using json-schema.
> 
> Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> ---
>  .../interrupt-controller/msi-controller.yaml  | 34 +++++++++++++++++++
>  .../bindings/pci/brcm,stb-pcie.yaml           |  1 +
>  .../bindings/pci/microchip,pcie-host.yaml     |  1 +
>  3 files changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> new file mode 100644
> index 000000000000..5ed6cd46e2e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: BSD-2-Clause

Noticed that checkpatch complains that the preferred license for new
binding schemas is (GPL-2.0-only OR BSD-2-Clause) so I'll fix that in
the next version.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/msi-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MSI controller
> +
> +maintainers:
> +  - Marc Zyngier <marc.zyngier@arm.com>
> +
> +description: |
> +  An MSI controller signals interrupts to a CPU when a write is made
> +  to an MMIO address by some master. An MSI controller may feature a
> +  number of doorbells.
> +
> +properties:
> +  "#msi-cells":
> +    description: |
> +      The number of cells in an msi-specifier, required if not zero.
> +
> +      Typically this will encode information related to sideband data,
> +      and will not encode doorbells or payloads as these can be
> +      configured dynamically.
> +
> +      The meaning of the msi-specifier is defined by the device tree
> +      binding of the specific MSI controller.
> +
> +  msi-controller:
> +    description:
> +      Identifies the node as an MSI controller.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index b9589a0daa5c..5c67976a8dc2 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -88,6 +88,7 @@ required:
>  
>  allOf:
>    - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: ../interrupt-controller/msi-controller.yaml#
>    - if:
>        properties:
>          compatible:
> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> index fb95c276a986..684d9d036f48 100644
> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> @@ -11,6 +11,7 @@ maintainers:
>  
>  allOf:
>    - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: ../interrupt-controller/msi-controller.yaml#
>  
>  properties:
>    compatible:
> -- 
> 2.32.0
> 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-27 18:24     ` Mark Kettenis
@ 2021-08-27 20:09       ` Alyssa Rosenzweig
  0 siblings, 0 replies; 29+ messages in thread
From: Alyssa Rosenzweig @ 2021-08-27 20:09 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, kettenis, tglx, maz, robh+dt, marcan, bhelgaas,
	nsaenz, jim2101024, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

> > > Clock references and DART (IOMMU) references are left out at the
> > > moment and will be added once the appropriate bindings have been
> > > settled upon.
> > > 
> > 
> > DART is in mainline .... is there a PCIe specific issue?
> 
> True.  I don't expect 4/4 to be merged as part of this series though
> as it will need to go through marcan's tree.  It is mostly there to
> show what the device tree will look like.

Fair enough. When it does get merged it'll need the updates from my my
PCIe series (v2). It looks like the only change needed for the commit
proposed there is updating the msi-ranges format. Hopefully the solution
proposed here is acceptable to both robh and maz so we can move forward
with this :-)

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-27 17:15 ` [PATCH v4 4/4] arm64: apple: Add PCIe node Mark Kettenis
  2021-08-27 17:59   ` Alyssa Rosenzweig
@ 2021-08-30 11:37   ` Marc Zyngier
  2021-08-30 14:57     ` Mark Kettenis
  2021-08-30 15:57     ` Rob Herring
  2021-09-12 21:30   ` Marc Zyngier
  2 siblings, 2 replies; 29+ messages in thread
From: Marc Zyngier @ 2021-08-30 11:37 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, Mark Kettenis, Thomas Gleixner, Rob Herring,
	Hector Martin, Bjorn Helgaas, Nicolas Saenz Julienne,
	Jim Quinlan, Florian Fainelli, bcm-kernel-feedback-list,
	Daire McNamara, Saenz Julienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

Hi Mark,

On Fri, 27 Aug 2021 18:15:29 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> 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..6e4677bdef44 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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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;
> +			};
> +		};
>  	};
>  };

I have now implemented the MSI change on the Linux driver side, and it
works nicely. So thumbs up from me on this front.

I am now looking at the interrupts provided by each port:
(1) a bunch of port-private interrupts (link up/down...)
(2) INTx interrupts

Given that the programming is per-port, I've implemented this as a
per-port interrupt controller.

(1) is dead easy to implement, and doesn't require any DT description.
(2) is unfortunately exposing the limits of my DT knowledge, and I'm
not clear how to model it. I came up with the following:

	port00: 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;

		interrupt-controller;
		#interrupt-cells = <1>;
		interrupt-parent = <&port00>;
		interrupt-map-mask = <0 0 0 7>;
		interrupt-map = <0 0 0 1 &port00 0>,
				<0 0 0 2 &port00 1>,
				<0 0 0 3 &port00 2>,
				<0 0 0 4 &port00 3>;
	};

which vaguely seem to do the right thing for the devices behind root
ports, but doesn't seem to work for INTx generated by the root ports
themselves. Any clue? Alternatively, I could move it to something
global to the whole PCIe controller, but that doesn't seem completely
right.

It also begs the question whether the per-port interrupt to the AIC
should be moved into each root port, should my per-port approach hold
any water.

Thanks,

	M.

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-30 11:37   ` Marc Zyngier
@ 2021-08-30 14:57     ` Mark Kettenis
  2021-08-30 20:40       ` Marc Zyngier
  2021-08-30 15:57     ` Rob Herring
  1 sibling, 1 reply; 29+ messages in thread
From: Mark Kettenis @ 2021-08-30 14:57 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: devicetree, alyssa, kettenis, tglx, robh+dt, marcan, bhelgaas,
	nsaenz, jim2101024, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

> Date: Mon, 30 Aug 2021 12:37:31 +0100
> From: Marc Zyngier <maz@kernel.org>
> 
> Hi Mark,

Hi Marc,

> On Fri, 27 Aug 2021 18:15:29 +0100,
> Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > 
> > 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..6e4677bdef44 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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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;
> > +			};
> > +		};
> >  	};
> >  };
> 
> I have now implemented the MSI change on the Linux driver side, and it
> works nicely. So thumbs up from me on this front.
> 
> I am now looking at the interrupts provided by each port:
> (1) a bunch of port-private interrupts (link up/down...)
> (2) INTx interrupts
> 
> Given that the programming is per-port, I've implemented this as a
> per-port interrupt controller.
> 
> (1) is dead easy to implement, and doesn't require any DT description.
> (2) is unfortunately exposing the limits of my DT knowledge, and I'm
> not clear how to model it. I came up with the following:
> 
> 	port00: 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;
> 
> 		interrupt-controller;
> 		#interrupt-cells = <1>;
> 		interrupt-parent = <&port00>;
> 		interrupt-map-mask = <0 0 0 7>;
> 		interrupt-map = <0 0 0 1 &port00 0>,
> 				<0 0 0 2 &port00 1>,
> 				<0 0 0 3 &port00 2>,
> 				<0 0 0 4 &port00 3>;
> 	};
> 
> which vaguely seem to do the right thing for the devices behind root
> ports, but doesn't seem to work for INTx generated by the root ports
> themselves. Any clue? Alternatively, I could move it to something
> global to the whole PCIe controller, but that doesn't seem completely
> right.
> 
> It also begs the question whether the per-port interrupt to the AIC
> should be moved into each root port, should my per-port approach hold
> any water.

Must admit that I didn't entirely thinkthrough this aspect fo the
hardware.  MSIs work just fine for the built-in hardware of the
current generation of M1 Macs so I ignored INTx for now.

It isn't entirely clear to me what properties are "allowed" on the
individual pci device child nodes that correspond to the ports.  But
"interrupt-map" and "interrupt-map-mask" are certainly among the
allowed properties, so this approach makes sense to me.  I must say I
don't see what the issue with the INTx generated by the root ports
themselves would be.  

I don't think we can move the interrupt property for the AIC to the
ports though, since that property would actually represent the
interrupt of the PCI bridge device according to the standard PCI
bindings and that isn't the case here.

So this makes sense to me and might not even need changing to the
binding for the Apple PCIe controller itself.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-30 11:37   ` Marc Zyngier
  2021-08-30 14:57     ` Mark Kettenis
@ 2021-08-30 15:57     ` Rob Herring
  2021-08-30 20:20       ` Marc Zyngier
  1 sibling, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-08-30 15:57 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Mark Kettenis, devicetree, Alyssa Rosenzweig, Mark Kettenis,
	Thomas Gleixner, Hector Martin, Bjorn Helgaas,
	Nicolas Saenz Julienne, Jim Quinlan, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Daire McNamara,
	Saenz Julienne, linux-kernel, linux-arm-kernel, PCI,
	moderated list:BROADCOM BCM2835 ARM ARCHITECTURE

On Mon, Aug 30, 2021 at 6:37 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Mark,
>
> On Fri, 27 Aug 2021 18:15:29 +0100,
> Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > 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..6e4677bdef44 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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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;
> > +                     };
> > +             };
> >       };
> >  };
>
> I have now implemented the MSI change on the Linux driver side, and it
> works nicely. So thumbs up from me on this front.
>
> I am now looking at the interrupts provided by each port:
> (1) a bunch of port-private interrupts (link up/down...)
> (2) INTx interrupts

So each port has an independent INTx space? Is that even something PCI
defines or comprehends?

> Given that the programming is per-port, I've implemented this as a
> per-port interrupt controller.
>
> (1) is dead easy to implement, and doesn't require any DT description.
> (2) is unfortunately exposing the limits of my DT knowledge, and I'm
> not clear how to model it. I came up with the following:
>
>         port00: 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;
>
>                 interrupt-controller;
>                 #interrupt-cells = <1>;
>                 interrupt-parent = <&port00>;
>                 interrupt-map-mask = <0 0 0 7>;
>                 interrupt-map = <0 0 0 1 &port00 0>,
>                                 <0 0 0 2 &port00 1>,
>                                 <0 0 0 3 &port00 2>,
>                                 <0 0 0 4 &port00 3>;

IIRC, I don't think the DT IRQ code handles a node having both
'interrupt-controller' and 'interrupt-map' properties. I think that's
why some PCI host bridge nodes have child interrupt-controller nodes.
I don't really like that work-around, so if the above can be made to
work, I'd be happy to see it. But the DT IRQ code is some ancient code
for ancient platforms (PowerMacs being one of them).

>         };
>
> which vaguely seem to do the right thing for the devices behind root
> ports, but doesn't seem to work for INTx generated by the root ports
> themselves. Any clue? Alternatively, I could move it to something
> global to the whole PCIe controller, but that doesn't seem completely
> right.
>
> It also begs the question whether the per-port interrupt to the AIC
> should be moved into each root port, should my per-port approach hold
> any water.

I tend to think per-port is the right thing to do. However, the child
nodes are PCI devices, so that creates some restrictions. Such as the
per port registers are in the host address space, not the PCI address
space, so we can't move the registers into the child nodes. The
interrupts may be okay. Certainly, being an 'interrupt-controller'
without having an 'interrupts' property for an non root interrupt
controller is odd.

Rob

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-30 15:57     ` Rob Herring
@ 2021-08-30 20:20       ` Marc Zyngier
  0 siblings, 0 replies; 29+ messages in thread
From: Marc Zyngier @ 2021-08-30 20:20 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Kettenis, devicetree, Alyssa Rosenzweig, Mark Kettenis,
	Thomas Gleixner, Hector Martin, Bjorn Helgaas,
	Nicolas Saenz Julienne, Jim Quinlan, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Daire McNamara,
	Saenz Julienne, linux-kernel, linux-arm-kernel, PCI,
	moderated list:BROADCOM BCM2835 ARM ARCHITECTURE

On Mon, 30 Aug 2021 16:57:59 +0100,
Rob Herring <robh+dt@kernel.org> wrote:
> 
> On Mon, Aug 30, 2021 at 6:37 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > I have now implemented the MSI change on the Linux driver side, and it
> > works nicely. So thumbs up from me on this front.
> >
> > I am now looking at the interrupts provided by each port:
> > (1) a bunch of port-private interrupts (link up/down...)
> > (2) INTx interrupts
> 
> So each port has an independent INTx space?

Yes.

> Is that even something PCI defines or comprehends?

Can't see why not. That's no different from having several PCI busses.
I don't think anything enforces that INTx interrupts have to be
unique across the system. As long as they are unique across a PCI
hierarchy, we should be OK.

> 
> > Given that the programming is per-port, I've implemented this as a
> > per-port interrupt controller.
> >
> > (1) is dead easy to implement, and doesn't require any DT description.
> > (2) is unfortunately exposing the limits of my DT knowledge, and I'm
> > not clear how to model it. I came up with the following:
> >
> >         port00: 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;
> >
> >                 interrupt-controller;
> >                 #interrupt-cells = <1>;
> >                 interrupt-parent = <&port00>;
> >                 interrupt-map-mask = <0 0 0 7>;
> >                 interrupt-map = <0 0 0 1 &port00 0>,
> >                                 <0 0 0 2 &port00 1>,
> >                                 <0 0 0 3 &port00 2>,
> >                                 <0 0 0 4 &port00 3>;
> 
> IIRC, I don't think the DT IRQ code handles a node having both
> 'interrupt-controller' and 'interrupt-map' properties.

Indeed, and that actually explains why the damned INTx interrupts
insist on being 1-based instead of 0-based as the above mapping
attempts to describe it. Turns out I can rip the interrupt-map out and
it isn't worse.

> I think that's why some PCI host bridge nodes have child
> interrupt-controller nodes.  I don't really like that work-around,
> so if the above can be made to work, I'd be happy to see it. But the
> DT IRQ code is some ancient code for ancient platforms (PowerMacs
> being one of them).

That'd probably need some massaging. I'll have a look. I checked that
if I add something like:

		interrupts-extended = <&port02 2>;

to each port, I get the PME interrupt correctly assigned should I pass
pcie_pme=nomsi. Given that this IP is pretty limited in terms of MSIs,
every bit that can free a MSI is welcome.

I guess that it would make sense to expand this support to also match
for an interrupt-map.

> 
> >         };
> >
> > which vaguely seem to do the right thing for the devices behind root
> > ports, but doesn't seem to work for INTx generated by the root ports
> > themselves. Any clue? Alternatively, I could move it to something
> > global to the whole PCIe controller, but that doesn't seem completely
> > right.

I've investigated this one further, and it looks like the DT IRQ code
insists on trying to find the interrupt in the main pcie node instead
of in the root port itself. But of course it doesn't want to parse an
interrupt-map at that level either.

I guess that's related to the above.

> >
> > It also begs the question whether the per-port interrupt to the AIC
> > should be moved into each root port, should my per-port approach hold
> > any water.
> 
> I tend to think per-port is the right thing to do. However, the child
> nodes are PCI devices, so that creates some restrictions. Such as the
> per port registers are in the host address space, not the PCI address
> space, so we can't move the registers into the child nodes. The
> interrupts may be okay. Certainly, being an 'interrupt-controller'
> without having an 'interrupts' property for an non root interrupt
> controller is odd.

That was my own impression as well.

I guess there is no real canonical way to handle this particular
system and to fully support it, we'll have to amend the current
infrastructure. The question is: what is the least ugly way to express
this that will work reasonably across implementations (OpenBSD, Linux,
u-boot)?

	M.

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-30 14:57     ` Mark Kettenis
@ 2021-08-30 20:40       ` Marc Zyngier
  0 siblings, 0 replies; 29+ messages in thread
From: Marc Zyngier @ 2021-08-30 20:40 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, kettenis, tglx, robh+dt, marcan, bhelgaas,
	nsaenz, jim2101024, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

On Mon, 30 Aug 2021 15:57:02 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > Date: Mon, 30 Aug 2021 12:37:31 +0100
> > From: Marc Zyngier <maz@kernel.org>
> > 
> > Hi Mark,
> 
> Hi Marc,
> 
> > On Fri, 27 Aug 2021 18:15:29 +0100,
> > Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > 
> > > 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..6e4677bdef44 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 = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 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;
> > > +			};
> > > +		};
> > >  	};
> > >  };
> > 
> > I have now implemented the MSI change on the Linux driver side, and it
> > works nicely. So thumbs up from me on this front.
> > 
> > I am now looking at the interrupts provided by each port:
> > (1) a bunch of port-private interrupts (link up/down...)
> > (2) INTx interrupts
> > 
> > Given that the programming is per-port, I've implemented this as a
> > per-port interrupt controller.
> > 
> > (1) is dead easy to implement, and doesn't require any DT description.
> > (2) is unfortunately exposing the limits of my DT knowledge, and I'm
> > not clear how to model it. I came up with the following:
> > 
> > 	port00: 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;
> > 
> > 		interrupt-controller;
> > 		#interrupt-cells = <1>;
> > 		interrupt-parent = <&port00>;
> > 		interrupt-map-mask = <0 0 0 7>;
> > 		interrupt-map = <0 0 0 1 &port00 0>,
> > 				<0 0 0 2 &port00 1>,
> > 				<0 0 0 3 &port00 2>,
> > 				<0 0 0 4 &port00 3>;
> > 	};
> > 
> > which vaguely seem to do the right thing for the devices behind root
> > ports, but doesn't seem to work for INTx generated by the root ports
> > themselves. Any clue? Alternatively, I could move it to something
> > global to the whole PCIe controller, but that doesn't seem completely
> > right.
> > 
> > It also begs the question whether the per-port interrupt to the AIC
> > should be moved into each root port, should my per-port approach hold
> > any water.
> 
> Must admit that I didn't entirely thinkthrough this aspect fo the
> hardware.  MSIs work just fine for the built-in hardware of the
> current generation of M1 Macs so I ignored INTx for now.

It isn't a big deal right now, but I sense that with only 32 MSIs per
PCIe block, keeping the PCIe management interrupts (PME, AER...) as
MSIs is going to waste valuable space once we have Thunderbolt up and
running (one of these days). I certainly intent to stick a PCIe/PCIe
bridge behind each of the ports, and each port is going to eat into
that as well.

> It isn't entirely clear to me what properties are "allowed" on the
> individual pci device child nodes that correspond to the ports.  But
> "interrupt-map" and "interrupt-map-mask" are certainly among the
> allowed properties, so this approach makes sense to me.  I must say I
> don't see what the issue with the INTx generated by the root ports
> themselves would be.

I think this is just a Linux shortcoming (Rob pointed out something in
his email).

> I don't think we can move the interrupt property for the AIC to the
> ports though, since that property would actually represent the
> interrupt of the PCI bridge device according to the standard PCI
> bindings and that isn't the case here.
> 
> So this makes sense to me and might not even need changing to the
> binding for the Apple PCIe controller itself.

I guess there is a great deal of ambiguity when it comes to what
interrupt maps to what, to be honest. At this stage, I don't think
that warrant a change in the current shape of the binding though.

FWIW,

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

The current state of the Linux support is at [1]. I'll sync-up with
Alyssa later this week to have a new posting after the current merge
window.

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=hack/m1-pcie-v3

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema
  2021-08-27 17:15 ` [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema Mark Kettenis
  2021-08-27 19:15   ` Mark Kettenis
@ 2021-08-31 20:57   ` Rob Herring
  2021-09-01 10:56     ` Mark Kettenis
  2021-08-31 20:58   ` Rob Herring
  2 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-08-31 20:57 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Hector Martin, Bjorn Helgaas, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, Jim Quinlan,
	Daire McNamara, Saenz Julienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

On Fri, Aug 27, 2021 at 07:15:26PM +0200, Mark Kettenis wrote:
> From: Mark Kettenis <kettenis@openbsd.org>
> 
> Split the MSI controller bindings from the MSI binding document
> into DT schema format using json-schema.
> 
> Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> ---
>  .../interrupt-controller/msi-controller.yaml  | 34 +++++++++++++++++++
>  .../bindings/pci/brcm,stb-pcie.yaml           |  1 +
>  .../bindings/pci/microchip,pcie-host.yaml     |  1 +
>  3 files changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> new file mode 100644
> index 000000000000..5ed6cd46e2e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/msi-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MSI controller
> +
> +maintainers:
> +  - Marc Zyngier <marc.zyngier@arm.com>
> +
> +description: |
> +  An MSI controller signals interrupts to a CPU when a write is made
> +  to an MMIO address by some master. An MSI controller may feature a
> +  number of doorbells.
> +
> +properties:
> +  "#msi-cells":
> +    description: |
> +      The number of cells in an msi-specifier, required if not zero.
> +
> +      Typically this will encode information related to sideband data,
> +      and will not encode doorbells or payloads as these can be
> +      configured dynamically.
> +
> +      The meaning of the msi-specifier is defined by the device tree
> +      binding of the specific MSI controller.

I'd prefer we limit this to the maximum range. I'd like to know when 
someone needs 2 cells (or 3000).

enum: [ 0, 1 ]

Though no one seems to use 0 (making it optional was probably a 
mistake...)

> +
> +  msi-controller:
> +    description:
> +      Identifies the node as an MSI controller.
> +    $ref: /schemas/types.yaml#/definitions/flag

dependencies:
  "#msi-cells": [ msi-controller ]

> +
> +additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index b9589a0daa5c..5c67976a8dc2 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -88,6 +88,7 @@ required:
>  
>  allOf:
>    - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: ../interrupt-controller/msi-controller.yaml#

/schemas/interrupt-controller/msi-controller.yaml#

>    - if:
>        properties:
>          compatible:
> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> index fb95c276a986..684d9d036f48 100644
> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> @@ -11,6 +11,7 @@ maintainers:
>  
>  allOf:
>    - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: ../interrupt-controller/msi-controller.yaml#
>  
>  properties:
>    compatible:
> -- 
> 2.32.0
> 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema
  2021-08-27 17:15 ` [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema Mark Kettenis
  2021-08-27 19:15   ` Mark Kettenis
  2021-08-31 20:57   ` Rob Herring
@ 2021-08-31 20:58   ` Rob Herring
  2 siblings, 0 replies; 29+ messages in thread
From: Rob Herring @ 2021-08-31 20:58 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Hector Martin, Bjorn Helgaas, Nicolas Saenz Julienne,
	Florian Fainelli, bcm-kernel-feedback-list, Jim Quinlan,
	Daire McNamara, Saenz Julienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

On Fri, Aug 27, 2021 at 07:15:26PM +0200, Mark Kettenis wrote:
> From: Mark Kettenis <kettenis@openbsd.org>
> 
> Split the MSI controller bindings from the MSI binding document
> into DT schema format using json-schema.
> 
> Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> ---
>  .../interrupt-controller/msi-controller.yaml  | 34 +++++++++++++++++++
>  .../bindings/pci/brcm,stb-pcie.yaml           |  1 +
>  .../bindings/pci/microchip,pcie-host.yaml     |  1 +
>  3 files changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> new file mode 100644
> index 000000000000..5ed6cd46e2e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/msi-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MSI controller
> +
> +maintainers:
> +  - Marc Zyngier <marc.zyngier@arm.com>

Not the current email for Marc.

> +
> +description: |
> +  An MSI controller signals interrupts to a CPU when a write is made
> +  to an MMIO address by some master. An MSI controller may feature a
> +  number of doorbells.
> +
> +properties:
> +  "#msi-cells":
> +    description: |
> +      The number of cells in an msi-specifier, required if not zero.
> +
> +      Typically this will encode information related to sideband data,
> +      and will not encode doorbells or payloads as these can be
> +      configured dynamically.
> +
> +      The meaning of the msi-specifier is defined by the device tree
> +      binding of the specific MSI controller.
> +
> +  msi-controller:
> +    description:
> +      Identifies the node as an MSI controller.
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +additionalProperties: true
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index b9589a0daa5c..5c67976a8dc2 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -88,6 +88,7 @@ required:
>  
>  allOf:
>    - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: ../interrupt-controller/msi-controller.yaml#
>    - if:
>        properties:
>          compatible:
> diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> index fb95c276a986..684d9d036f48 100644
> --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> @@ -11,6 +11,7 @@ maintainers:
>  
>  allOf:
>    - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: ../interrupt-controller/msi-controller.yaml#
>  
>  properties:
>    compatible:
> -- 
> 2.32.0
> 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema
  2021-08-27 19:15   ` Mark Kettenis
@ 2021-08-31 21:04     ` Rob Herring
  0 siblings, 0 replies; 29+ messages in thread
From: Rob Herring @ 2021-08-31 21:04 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, kettenis, tglx, maz, marcan, bhelgaas,
	nsaenz, f.fainelli, bcm-kernel-feedback-list, jim2101024,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

On Fri, Aug 27, 2021 at 09:15:11PM +0200, Mark Kettenis wrote:
> > From: Mark Kettenis <mark.kettenis@xs4all.nl>
> > Date: Fri, 27 Aug 2021 19:15:26 +0200
> > 
> > From: Mark Kettenis <kettenis@openbsd.org>
> > 
> > Split the MSI controller bindings from the MSI binding document
> > into DT schema format using json-schema.
> > 
> > Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> > ---
> >  .../interrupt-controller/msi-controller.yaml  | 34 +++++++++++++++++++
> >  .../bindings/pci/brcm,stb-pcie.yaml           |  1 +
> >  .../bindings/pci/microchip,pcie-host.yaml     |  1 +
> >  3 files changed, 36 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > new file mode 100644
> > index 000000000000..5ed6cd46e2e0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > @@ -0,0 +1,34 @@
> > +# SPDX-License-Identifier: BSD-2-Clause
> 
> Noticed that checkpatch complains that the preferred license for new
> binding schemas is (GPL-2.0-only OR BSD-2-Clause) so I'll fix that in
> the next version.

Yes, but the text you copied is default GPL-2.0, so you need the author's 
permission to add BSD license. However, as Mark Rutland wrote all of 
msi.txt for Arm Ltd, I can tell you dual licensing this is fine. Maybe 
it's so little to fall under fair use anyways, but IANAL.

Rob

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 2/4] dt-bindings: interrupt-controller: msi: Add msi-ranges property
  2021-08-27 17:15 ` [PATCH v4 2/4] dt-bindings: interrupt-controller: msi: Add msi-ranges property Mark Kettenis
@ 2021-08-31 21:16   ` Rob Herring
  2021-09-21 17:52     ` Mark Kettenis
  0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-08-31 21:16 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Hector Martin, Bjorn Helgaas, Jim Quinlan,
	Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Daire McNamara, Saenz Julienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

On Fri, Aug 27, 2021 at 07:15:27PM +0200, Mark Kettenis wrote:
> From: Mark Kettenis <kettenis@openbsd.org>
> 
> Update the MSI controller binding to add an msi-ranges property
> that specifies how MSIs map onto regular interrupts on some other
> interrupt controller.
> 
> Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> ---
>  .../bindings/interrupt-controller/msi-controller.yaml     | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> index 5ed6cd46e2e0..bf8b8a7dba09 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> @@ -31,4 +31,12 @@ properties:
>        Identifies the node as an MSI controller.
>      $ref: /schemas/types.yaml#/definitions/flag
>  
> +  msi-ranges:
> +    description:
> +      A list of pairs <intid span>, where "intid" is the specification

It's not really 'pairs' and 'interrupt specifier' is the terminology the 
spec uses. How about:

A list of <phandle intspec span>, where "phandle" is parent interrupt 
controller, "intspec" is the starting/base interrupt specifier, and 
"span" is the size of that range (typically multiples of 32).

The 'multiples of 32' part is what Marc told me.

> +      of the first interrupt (including the phandle for the interrupt
> +      controller) that can be used as an MSI, and "span" the size of
> +      that range. Multiple ranges can be provided.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +
>  additionalProperties: true
> -- 
> 2.32.0
> 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
  2021-08-27 17:15 ` [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie Mark Kettenis
  2021-08-27 17:58   ` Alyssa Rosenzweig
@ 2021-08-31 21:21   ` Rob Herring
  2021-09-01 11:29     ` Mark Kettenis
  1 sibling, 1 reply; 29+ messages in thread
From: Rob Herring @ 2021-08-31 21:21 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, Mark Kettenis, Thomas Gleixner, Marc Zyngier,
	Hector Martin, Bjorn Helgaas, Jim Quinlan,
	Nicolas Saenz Julienne, Florian Fainelli,
	bcm-kernel-feedback-list, Daire McNamara, Saenz Julienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

On Fri, Aug 27, 2021 at 07:15:28PM +0200, Mark Kettenis wrote:
> From: Mark Kettenis <kettenis@openbsd.org>
> 
> The Apple PCIe host controller is a PCIe host controller with
> multiple root ports present in Apple ARM SoC platforms, including
> various iPhone and iPad devices and the "Apple Silicon" Macs.
> 
> Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> ---
>  .../devicetree/bindings/pci/apple,pcie.yaml   | 165 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 166 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> new file mode 100644
> index 000000000000..97a126db935a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> @@ -0,0 +1,165 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple PCIe host controller
> +
> +maintainers:
> +  - Mark Kettenis <kettenis@openbsd.org>
> +
> +description: |
> +  The Apple PCIe host controller is a PCIe host controller with
> +  multiple root ports present in Apple ARM SoC platforms, including
> +  various iPhone and iPad devices and the "Apple Silicon" Macs.
> +  The controller incorporates Synopsys DesigWare PCIe logic to
> +  implements its root ports.  But the ATU found on most DesignWare
> +  PCIe host bridges is absent.
> +
> +  All root ports share a single ECAM space, but separate GPIOs are
> +  used to take the PCI devices on those ports out of reset.  Therefore
> +  the standard "reset-gpios" and "max-link-speed" properties appear on
> +  the child nodes that represent the PCI bridges that correspond to
> +  the individual root ports.
> +
> +  MSIs are handled by the PCIe controller and translated into regular
> +  interrupts.  A range of 32 MSIs is provided.  These 32 MSIs can be
> +  distributed over the root ports as the OS sees fit by programming
> +  the PCIe controller's port registers.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - $ref: ../interrupt-controller/msi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: apple,t8103-pcie
> +      - const: apple,pcie
> +
> +  reg:
> +    minItems: 3
> +    maxItems: 5
> +
> +  reg-names:
> +    minItems: 3
> +    maxItems: 5
> +    items:
> +      - const: config
> +      - const: rc
> +      - const: port0
> +      - const: port1
> +      - const: port2
> +
> +  ranges:
> +    minItems: 2
> +    maxItems: 2
> +
> +  interrupts:
> +    description:
> +      Interrupt specifiers, one for each root port.
> +    minItems: 1
> +    maxItems: 3
> +
> +  msi-parent: true

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

> +
> +#  msi-ranges:
> +#    description:
> +#      A list of pairs <intid span>, where "intid" is the first
> +#      interrupt number that can be used as an MSI, and "span" the size
> +#      of that range.
> +#    $ref: /schemas/types.yaml#/definitions/phandle-array

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

> +
> +  iommu-map: true
> +  iommu-map-mask: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - bus-range
> +  - interrupts
> +  - msi-controller
> +  - msi-parent
> +  - msi-ranges
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/apple-aic.h>
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      pcie0: pcie@690000000 {
> +        compatible = "apple,t8103-pcie", "apple,pcie";
> +        device_type = "pci";
> +
> +        reg = <0x6 0x90000000 0x0 0x1000000>,
> +              <0x6 0x80000000 0x0 0x4000>,
> +              <0x6 0x81000000 0x0 0x8000>,
> +              <0x6 0x82000000 0x0 0x8000>,
> +              <0x6 0x83000000 0x0 0x8000>;
> +        reg-names = "config", "rc", "port0", "port1", "port2";
> +
> +        interrupt-parent = <&aic>;
> +        interrupts = <AIC_IRQ 695 IRQ_TYPE_LEVEL_HIGH>,
> +                     <AIC_IRQ 698 IRQ_TYPE_LEVEL_HIGH>,
> +                     <AIC_IRQ 701 IRQ_TYPE_LEVEL_HIGH>;
> +
> +        msi-controller;
> +        msi-parent = <&pcie0>;
> +        msi-ranges = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>;
> +
> +        iommu-map = <0x100 &dart0 1 1>,
> +                    <0x200 &dart1 1 1>,
> +                    <0x300 &dart2 1 1>;
> +        iommu-map-mask = <0xff00>;
> +
> +        bus-range = <0 3>;
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +        ranges = <0x43000000 0x6 0xa0000000 0x6 0xa0000000 0x0 0x20000000>,
> +                 <0x02000000 0x0 0xc0000000 0x6 0xc0000000 0x0 0x40000000>;
> +
> +        clocks = <&pcie_core_clk>, <&pcie_aux_clk>, <&pcie_ref_clk>;
> +        pinctrl-0 = <&pcie_pins>;
> +        pinctrl-names = "default";
> +
> +        pci@0,0 {
> +          device_type = "pci";
> +          reg = <0x0 0x0 0x0 0x0 0x0>;
> +          reset-gpios = <&pinctrl_ap 152 0>;
> +          max-link-speed = <2>;
> +
> +          #address-cells = <3>;
> +          #size-cells = <2>;
> +          ranges;
> +        };
> +
> +        pci@1,0 {
> +          device_type = "pci";
> +          reg = <0x800 0x0 0x0 0x0 0x0>;
> +          reset-gpios = <&pinctrl_ap 153 0>;
> +          max-link-speed = <2>;
> +
> +          #address-cells = <3>;
> +          #size-cells = <2>;
> +          ranges;
> +        };
> +
> +        pci@2,0 {
> +          device_type = "pci";
> +          reg = <0x1000 0x0 0x0 0x0 0x0>;
> +          reset-gpios = <&pinctrl_ap 33 0>;
> +          max-link-speed = <1>;
> +
> +          #address-cells = <3>;
> +          #size-cells = <2>;
> +          ranges;
> +        };
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c6b8a720c0bc..30bea4042e7e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1694,6 +1694,7 @@ C:	irc://chat.freenode.net/asahi-dev
>  T:	git https://github.com/AsahiLinux/linux.git
>  F:	Documentation/devicetree/bindings/arm/apple.yaml
>  F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
> +F:	Documentation/devicetree/bindings/pci/apple,pcie.yaml
>  F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
>  F:	arch/arm64/boot/dts/apple/
>  F:	drivers/irqchip/irq-apple-aic.c
> -- 
> 2.32.0
> 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema
  2021-08-31 20:57   ` Rob Herring
@ 2021-09-01 10:56     ` Mark Kettenis
  0 siblings, 0 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-09-01 10:56 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, alyssa, kettenis, tglx, maz, marcan, bhelgaas,
	nsaenz, f.fainelli, bcm-kernel-feedback-list, jim2101024,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

> Date: Tue, 31 Aug 2021 15:57:10 -0500
> From: Rob Herring <robh@kernel.org>
> 
> On Fri, Aug 27, 2021 at 07:15:26PM +0200, Mark Kettenis wrote:
> > From: Mark Kettenis <kettenis@openbsd.org>
> > 
> > Split the MSI controller bindings from the MSI binding document
> > into DT schema format using json-schema.
> > 
> > Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> > ---
> >  .../interrupt-controller/msi-controller.yaml  | 34 +++++++++++++++++++
> >  .../bindings/pci/brcm,stb-pcie.yaml           |  1 +
> >  .../bindings/pci/microchip,pcie-host.yaml     |  1 +
> >  3 files changed, 36 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > new file mode 100644
> > index 000000000000..5ed6cd46e2e0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > @@ -0,0 +1,34 @@
> > +# SPDX-License-Identifier: BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/msi-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MSI controller
> > +
> > +maintainers:
> > +  - Marc Zyngier <marc.zyngier@arm.com>
> > +
> > +description: |
> > +  An MSI controller signals interrupts to a CPU when a write is made
> > +  to an MMIO address by some master. An MSI controller may feature a
> > +  number of doorbells.
> > +
> > +properties:
> > +  "#msi-cells":
> > +    description: |
> > +      The number of cells in an msi-specifier, required if not zero.
> > +
> > +      Typically this will encode information related to sideband data,
> > +      and will not encode doorbells or payloads as these can be
> > +      configured dynamically.
> > +
> > +      The meaning of the msi-specifier is defined by the device tree
> > +      binding of the specific MSI controller.
> 
> I'd prefer we limit this to the maximum range. I'd like to know when 
> someone needs 2 cells (or 3000).
> 
> enum: [ 0, 1 ]
> 
> Though no one seems to use 0 (making it optional was probably a 
> mistake...)
> 
> > +
> > +  msi-controller:
> > +    description:
> > +      Identifies the node as an MSI controller.
> > +    $ref: /schemas/types.yaml#/definitions/flag
> 
> dependencies:
>   "#msi-cells": [ msi-controller ]
> 
> > +
> > +additionalProperties: true
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index b9589a0daa5c..5c67976a8dc2 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -88,6 +88,7 @@ required:
> >  
> >  allOf:
> >    - $ref: /schemas/pci/pci-bus.yaml#
> > +  - $ref: ../interrupt-controller/msi-controller.yaml#
> 
> /schemas/interrupt-controller/msi-controller.yaml#
> 
> >    - if:
> >        properties:
> >          compatible:
> > diff --git a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > index fb95c276a986..684d9d036f48 100644
> > --- a/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > +++ b/Documentation/devicetree/bindings/pci/microchip,pcie-host.yaml
> > @@ -11,6 +11,7 @@ maintainers:
> >  
> >  allOf:
> >    - $ref: /schemas/pci/pci-bus.yaml#
> > +  - $ref: ../interrupt-controller/msi-controller.yaml#
> >  
> >  properties:
> >    compatible:

Thanks Rob.  Makes sense to me, so I made these changes (and fixed
Marc's mail address) for v5.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
  2021-08-31 21:21   ` Rob Herring
@ 2021-09-01 11:29     ` Mark Kettenis
  2021-09-12 20:13       ` Marc Zyngier
  0 siblings, 1 reply; 29+ messages in thread
From: Mark Kettenis @ 2021-09-01 11:29 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, alyssa, kettenis, tglx, maz, marcan, bhelgaas,
	jim2101024, nsaenz, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

> Date: Tue, 31 Aug 2021 16:21:28 -0500
> From: Rob Herring <robh@kernel.org>
> 
> On Fri, Aug 27, 2021 at 07:15:28PM +0200, Mark Kettenis wrote:
> > From: Mark Kettenis <kettenis@openbsd.org>
> > 
> > The Apple PCIe host controller is a PCIe host controller with
> > multiple root ports present in Apple ARM SoC platforms, including
> > various iPhone and iPad devices and the "Apple Silicon" Macs.
> > 
> > Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> > ---
> >  .../devicetree/bindings/pci/apple,pcie.yaml   | 165 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 166 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > new file mode 100644
> > index 000000000000..97a126db935a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > @@ -0,0 +1,165 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Apple PCIe host controller
> > +
> > +maintainers:
> > +  - Mark Kettenis <kettenis@openbsd.org>
> > +
> > +description: |
> > +  The Apple PCIe host controller is a PCIe host controller with
> > +  multiple root ports present in Apple ARM SoC platforms, including
> > +  various iPhone and iPad devices and the "Apple Silicon" Macs.
> > +  The controller incorporates Synopsys DesigWare PCIe logic to
> > +  implements its root ports.  But the ATU found on most DesignWare
> > +  PCIe host bridges is absent.
> > +
> > +  All root ports share a single ECAM space, but separate GPIOs are
> > +  used to take the PCI devices on those ports out of reset.  Therefore
> > +  the standard "reset-gpios" and "max-link-speed" properties appear on
> > +  the child nodes that represent the PCI bridges that correspond to
> > +  the individual root ports.
> > +
> > +  MSIs are handled by the PCIe controller and translated into regular
> > +  interrupts.  A range of 32 MSIs is provided.  These 32 MSIs can be
> > +  distributed over the root ports as the OS sees fit by programming
> > +  the PCIe controller's port registers.
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +  - $ref: ../interrupt-controller/msi-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: apple,t8103-pcie
> > +      - const: apple,pcie
> > +
> > +  reg:
> > +    minItems: 3
> > +    maxItems: 5
> > +
> > +  reg-names:
> > +    minItems: 3
> > +    maxItems: 5
> > +    items:
> > +      - const: config
> > +      - const: rc
> > +      - const: port0
> > +      - const: port1
> > +      - const: port2
> > +
> > +  ranges:
> > +    minItems: 2
> > +    maxItems: 2
> > +
> > +  interrupts:
> > +    description:
> > +      Interrupt specifiers, one for each root port.
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  msi-parent: true
> 
> I still think this should be dropped as it is meaningless with 
> 'msi-controller' present.

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

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

> > +
> > +#  msi-ranges:
> > +#    description:
> > +#      A list of pairs <intid span>, where "intid" is the first
> > +#      interrupt number that can be used as an MSI, and "span" the size
> > +#      of that range.
> > +#    $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Here, you'll want just 'maxItems: 1' as there's only 1 entry.

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

> > +
> > +  iommu-map: true
> > +  iommu-map-mask: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - bus-range
> > +  - interrupts
> > +  - msi-controller
> > +  - msi-parent
> > +  - msi-ranges
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/apple-aic.h>
> > +
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      pcie0: pcie@690000000 {
> > +        compatible = "apple,t8103-pcie", "apple,pcie";
> > +        device_type = "pci";
> > +
> > +        reg = <0x6 0x90000000 0x0 0x1000000>,
> > +              <0x6 0x80000000 0x0 0x4000>,
> > +              <0x6 0x81000000 0x0 0x8000>,
> > +              <0x6 0x82000000 0x0 0x8000>,
> > +              <0x6 0x83000000 0x0 0x8000>;
> > +        reg-names = "config", "rc", "port0", "port1", "port2";
> > +
> > +        interrupt-parent = <&aic>;
> > +        interrupts = <AIC_IRQ 695 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <AIC_IRQ 698 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <AIC_IRQ 701 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +        msi-controller;
> > +        msi-parent = <&pcie0>;
> > +        msi-ranges = <&aic AIC_IRQ 704 IRQ_TYPE_EDGE_RISING 32>;
> > +
> > +        iommu-map = <0x100 &dart0 1 1>,
> > +                    <0x200 &dart1 1 1>,
> > +                    <0x300 &dart2 1 1>;
> > +        iommu-map-mask = <0xff00>;
> > +
> > +        bus-range = <0 3>;
> > +        #address-cells = <3>;
> > +        #size-cells = <2>;
> > +        ranges = <0x43000000 0x6 0xa0000000 0x6 0xa0000000 0x0 0x20000000>,
> > +                 <0x02000000 0x0 0xc0000000 0x6 0xc0000000 0x0 0x40000000>;
> > +
> > +        clocks = <&pcie_core_clk>, <&pcie_aux_clk>, <&pcie_ref_clk>;
> > +        pinctrl-0 = <&pcie_pins>;
> > +        pinctrl-names = "default";
> > +
> > +        pci@0,0 {
> > +          device_type = "pci";
> > +          reg = <0x0 0x0 0x0 0x0 0x0>;
> > +          reset-gpios = <&pinctrl_ap 152 0>;
> > +          max-link-speed = <2>;
> > +
> > +          #address-cells = <3>;
> > +          #size-cells = <2>;
> > +          ranges;
> > +        };
> > +
> > +        pci@1,0 {
> > +          device_type = "pci";
> > +          reg = <0x800 0x0 0x0 0x0 0x0>;
> > +          reset-gpios = <&pinctrl_ap 153 0>;
> > +          max-link-speed = <2>;
> > +
> > +          #address-cells = <3>;
> > +          #size-cells = <2>;
> > +          ranges;
> > +        };
> > +
> > +        pci@2,0 {
> > +          device_type = "pci";
> > +          reg = <0x1000 0x0 0x0 0x0 0x0>;
> > +          reset-gpios = <&pinctrl_ap 33 0>;
> > +          max-link-speed = <1>;
> > +
> > +          #address-cells = <3>;
> > +          #size-cells = <2>;
> > +          ranges;
> > +        };
> > +      };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c6b8a720c0bc..30bea4042e7e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1694,6 +1694,7 @@ C:	irc://chat.freenode.net/asahi-dev
> >  T:	git https://github.com/AsahiLinux/linux.git
> >  F:	Documentation/devicetree/bindings/arm/apple.yaml
> >  F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
> > +F:	Documentation/devicetree/bindings/pci/apple,pcie.yaml
> >  F:	Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml
> >  F:	arch/arm64/boot/dts/apple/
> >  F:	drivers/irqchip/irq-apple-aic.c
> > -- 
> > 2.32.0
> > 
> > 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
  2021-09-01 11:29     ` Mark Kettenis
@ 2021-09-12 20:13       ` Marc Zyngier
  2021-09-13 20:55         ` Rob Herring
  0 siblings, 1 reply; 29+ messages in thread
From: Marc Zyngier @ 2021-09-12 20:13 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: Rob Herring, devicetree, alyssa, kettenis, tglx, marcan,
	bhelgaas, jim2101024, nsaenz, f.fainelli,
	bcm-kernel-feedback-list, daire.mcnamara, nsaenzjulienne,
	linux-kernel, linux-arm-kernel, linux-pci, linux-rpi-kernel

On Wed, 01 Sep 2021 12:29:22 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > Date: Tue, 31 Aug 2021 16:21:28 -0500
> > From: Rob Herring <robh@kernel.org>
> > 
> > On Fri, Aug 27, 2021 at 07:15:28PM +0200, Mark Kettenis wrote:
> > > From: Mark Kettenis <kettenis@openbsd.org>
> > > 
> > > The Apple PCIe host controller is a PCIe host controller with
> > > multiple root ports present in Apple ARM SoC platforms, including
> > > various iPhone and iPad devices and the "Apple Silicon" Macs.
> > > 
> > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> > > ---
> > >  .../devicetree/bindings/pci/apple,pcie.yaml   | 165 ++++++++++++++++++
> > >  MAINTAINERS                                   |   1 +
> > >  2 files changed, 166 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > new file mode 100644
> > > index 000000000000..97a126db935a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > @@ -0,0 +1,165 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Apple PCIe host controller
> > > +
> > > +maintainers:
> > > +  - Mark Kettenis <kettenis@openbsd.org>
> > > +
> > > +description: |
> > > +  The Apple PCIe host controller is a PCIe host controller with
> > > +  multiple root ports present in Apple ARM SoC platforms, including
> > > +  various iPhone and iPad devices and the "Apple Silicon" Macs.
> > > +  The controller incorporates Synopsys DesigWare PCIe logic to
> > > +  implements its root ports.  But the ATU found on most DesignWare
> > > +  PCIe host bridges is absent.
> > > +
> > > +  All root ports share a single ECAM space, but separate GPIOs are
> > > +  used to take the PCI devices on those ports out of reset.  Therefore
> > > +  the standard "reset-gpios" and "max-link-speed" properties appear on
> > > +  the child nodes that represent the PCI bridges that correspond to
> > > +  the individual root ports.
> > > +
> > > +  MSIs are handled by the PCIe controller and translated into regular
> > > +  interrupts.  A range of 32 MSIs is provided.  These 32 MSIs can be
> > > +  distributed over the root ports as the OS sees fit by programming
> > > +  the PCIe controller's port registers.
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/pci/pci-bus.yaml#
> > > +  - $ref: ../interrupt-controller/msi-controller.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: apple,t8103-pcie
> > > +      - const: apple,pcie
> > > +
> > > +  reg:
> > > +    minItems: 3
> > > +    maxItems: 5
> > > +
> > > +  reg-names:
> > > +    minItems: 3
> > > +    maxItems: 5
> > > +    items:
> > > +      - const: config
> > > +      - const: rc
> > > +      - const: port0
> > > +      - const: port1
> > > +      - const: port2
> > > +
> > > +  ranges:
> > > +    minItems: 2
> > > +    maxItems: 2
> > > +
> > > +  interrupts:
> > > +    description:
> > > +      Interrupt specifiers, one for each root port.
> > > +    minItems: 1
> > > +    maxItems: 3
> > > +
> > > +  msi-parent: true
> > 
> > I still think this should be dropped as it is meaningless with 
> > 'msi-controller' present.
> 
> Hmm.  As far as I can tell all current arm64 device trees that
> describe hardware with an MSI controller integrated on the PCI host
> bridge have both the 'msi-controller' and 'msi-parent' properties.
> See arch/arm64/boot/dts/marvell/aramada-37xx.dtsi and
> arch/arm64/boot/dts/xilinx/zynqmp.dtsi.
> 
> The current OpenBSD code will fail to map the MSIs if 'msi-parent'
> isn't there, although Linux seems to fall back on an MSI domain that's
> directly attached to the host bridge if the 'msi-parent' property is
> missing.  I think it makes sense to be explicit here, but if both you
> and Marc think it shouldn't be there, I probably can change the
> OpenBSD to do a similar fallback.

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

	M.

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-08-27 17:15 ` [PATCH v4 4/4] arm64: apple: Add PCIe node Mark Kettenis
  2021-08-27 17:59   ` Alyssa Rosenzweig
  2021-08-30 11:37   ` Marc Zyngier
@ 2021-09-12 21:30   ` Marc Zyngier
  2021-09-13 18:35     ` Mark Kettenis
  2 siblings, 1 reply; 29+ messages in thread
From: Marc Zyngier @ 2021-09-12 21:30 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, Mark Kettenis, Thomas Gleixner, Rob Herring,
	Hector Martin, Bjorn Helgaas, Nicolas Saenz Julienne,
	Jim Quinlan, Florian Fainelli, bcm-kernel-feedback-list,
	Daire McNamara, linux-kernel, linux-arm-kernel, linux-pci,
	linux-rpi-kernel

On Fri, 27 Aug 2021 18:15:29 +0100,
Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> 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..6e4677bdef44 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>,

Only exposing 16kB for the 'rc' crashes the Linux driver as it tries
to configure the port ref-clock configurations, which live much
higher:

#define CORE_LANE_CFG(port)		(0x84000 + 0x4000 * (port))

Previous versions of the binding had this region as 1MB, which made
things work.

> +			      <0x6 0x81000000 0x0 0x8000>,
> +			      <0x6 0x82000000 0x0 0x8000>,
> +			      <0x6 0x83000000 0x0 0x8000>;

These used to be 16kB, and are now twice as much. Didn't cause any
issue with the Linux driver, but I wonder what trigger either change.

Thanks,

	M.

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
  2021-09-12 21:30   ` Marc Zyngier
@ 2021-09-13 18:35     ` Mark Kettenis
  0 siblings, 0 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-09-13 18:35 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: devicetree, alyssa, kettenis, tglx, robh+dt, marcan, bhelgaas,
	nsaenz, jim2101024, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, linux-kernel, linux-arm-kernel, linux-pci,
	linux-rpi-kernel

> Date: Sun, 12 Sep 2021 22:30:42 +0100
> From: Marc Zyngier <maz@kernel.org>

Hi Marc,

> On Fri, 27 Aug 2021 18:15:29 +0100,
> Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > 
> > 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..6e4677bdef44 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>,
> 
> Only exposing 16kB for the 'rc' crashes the Linux driver as it tries
> to configure the port ref-clock configurations, which live much
> higher:
> 
> #define CORE_LANE_CFG(port)		(0x84000 + 0x4000 * (port))
> 
> Previous versions of the binding had this region as 1MB, which made
> things work.

Oops.  When I formalized the binding, I looked at the Apple DT and
used the sizes from there.  And didn't notice that this wasn't
sufficient since U-Boot doesn't actually use the size of the region to
create a mapping like an actual OS would do.  It is somewhat unclear
how big the regions really are, but as marcan noted at some point in
the past the sizes in the Apple DT seem to be somewhat inconsistent so
religiously following what is done there may not make sense.  So I'll
fix this in v5 (also in the example in the DT binding).

Corellium uses 1MB, which makes more sense unless we break up the
block into multiple ranges.

> > +			      <0x6 0x81000000 0x0 0x8000>,
> > +			      <0x6 0x82000000 0x0 0x8000>,
> > +			      <0x6 0x83000000 0x0 0x8000>;
> 
> These used to be 16kB, and are now twice as much. Didn't cause any
> issue with the Linux driver, but I wonder what trigger either change.

0x8000 is what the Apple DT uses.

Since we don't have authorative documentation for the chip we have to
make some guesses here.  I suspect we should try to keep the sizes as
small as possible while sticking to sizes of 2^n?  Then it probably
makes sense to use 0x4000 for these ranges.

Cheers,

Mark

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
  2021-09-12 20:13       ` Marc Zyngier
@ 2021-09-13 20:55         ` Rob Herring
  0 siblings, 0 replies; 29+ messages in thread
From: Rob Herring @ 2021-09-13 20:55 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Mark Kettenis, devicetree, Alyssa Rosenzweig, Mark Kettenis,
	Thomas Gleixner, Hector Martin, Bjorn Helgaas, Jim Quinlan,
	Nicolas Saenz Julienne, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE, Daire McNamara,
	Nicolas Saenz Julienne, linux-kernel, linux-arm-kernel, PCI,
	moderated list:BROADCOM BCM2835 ARM ARCHITECTURE

msi-On Sun, Sep 12, 2021 at 3:13 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 01 Sep 2021 12:29:22 +0100,
> Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >
> > > Date: Tue, 31 Aug 2021 16:21:28 -0500
> > > From: Rob Herring <robh@kernel.org>
> > >
> > > On Fri, Aug 27, 2021 at 07:15:28PM +0200, Mark Kettenis wrote:
> > > > From: Mark Kettenis <kettenis@openbsd.org>
> > > >
> > > > The Apple PCIe host controller is a PCIe host controller with
> > > > multiple root ports present in Apple ARM SoC platforms, including
> > > > various iPhone and iPad devices and the "Apple Silicon" Macs.
> > > >
> > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> > > > ---
> > > >  .../devicetree/bindings/pci/apple,pcie.yaml   | 165 ++++++++++++++++++
> > > >  MAINTAINERS                                   |   1 +
> > > >  2 files changed, 166 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/apple,pcie.yaml b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > > new file mode 100644
> > > > index 000000000000..97a126db935a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml
> > > > @@ -0,0 +1,165 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/pci/apple,pcie.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Apple PCIe host controller
> > > > +
> > > > +maintainers:
> > > > +  - Mark Kettenis <kettenis@openbsd.org>
> > > > +
> > > > +description: |
> > > > +  The Apple PCIe host controller is a PCIe host controller with
> > > > +  multiple root ports present in Apple ARM SoC platforms, including
> > > > +  various iPhone and iPad devices and the "Apple Silicon" Macs.
> > > > +  The controller incorporates Synopsys DesigWare PCIe logic to
> > > > +  implements its root ports.  But the ATU found on most DesignWare
> > > > +  PCIe host bridges is absent.
> > > > +
> > > > +  All root ports share a single ECAM space, but separate GPIOs are
> > > > +  used to take the PCI devices on those ports out of reset.  Therefore
> > > > +  the standard "reset-gpios" and "max-link-speed" properties appear on
> > > > +  the child nodes that represent the PCI bridges that correspond to
> > > > +  the individual root ports.
> > > > +
> > > > +  MSIs are handled by the PCIe controller and translated into regular
> > > > +  interrupts.  A range of 32 MSIs is provided.  These 32 MSIs can be
> > > > +  distributed over the root ports as the OS sees fit by programming
> > > > +  the PCIe controller's port registers.
> > > > +
> > > > +allOf:
> > > > +  - $ref: /schemas/pci/pci-bus.yaml#
> > > > +  - $ref: ../interrupt-controller/msi-controller.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - const: apple,t8103-pcie
> > > > +      - const: apple,pcie
> > > > +
> > > > +  reg:
> > > > +    minItems: 3
> > > > +    maxItems: 5
> > > > +
> > > > +  reg-names:
> > > > +    minItems: 3
> > > > +    maxItems: 5
> > > > +    items:
> > > > +      - const: config
> > > > +      - const: rc
> > > > +      - const: port0
> > > > +      - const: port1
> > > > +      - const: port2
> > > > +
> > > > +  ranges:
> > > > +    minItems: 2
> > > > +    maxItems: 2
> > > > +
> > > > +  interrupts:
> > > > +    description:
> > > > +      Interrupt specifiers, one for each root port.
> > > > +    minItems: 1
> > > > +    maxItems: 3
> > > > +
> > > > +  msi-parent: true
> > >
> > > I still think this should be dropped as it is meaningless with
> > > 'msi-controller' present.
> >
> > Hmm.  As far as I can tell all current arm64 device trees that
> > describe hardware with an MSI controller integrated on the PCI host
> > bridge have both the 'msi-controller' and 'msi-parent' properties.
> > See arch/arm64/boot/dts/marvell/aramada-37xx.dtsi and
> > arch/arm64/boot/dts/xilinx/zynqmp.dtsi.

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

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

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

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

Rob

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 0/4] Apple M1 PCIe DT bindings
  2021-08-27 17:15 [PATCH v4 0/4] Apple M1 PCIe DT bindings Mark Kettenis
                   ` (3 preceding siblings ...)
  2021-08-27 17:15 ` [PATCH v4 4/4] arm64: apple: Add PCIe node Mark Kettenis
@ 2021-09-21 11:01 ` Marc Zyngier
  4 siblings, 0 replies; 29+ messages in thread
From: Marc Zyngier @ 2021-09-21 11:01 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: devicetree, alyssa, Mark Kettenis, Thomas Gleixner, Rob Herring,
	Hector Martin, Bjorn Helgaas, Florian Fainelli,
	bcm-kernel-feedback-list, Nicolas Saenz Julienne, Jim Quinlan,
	Daire McNamara, linux-kernel, linux-arm-kernel, linux-pci,
	linux-rpi-kernel

On Fri, 27 Aug 2021 18:15:25 +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.
> 
> This now adds an MSI controller binding schema and uses the generic
> msi-ranges property to specify how the MSIs are mapped to interrupts
> on the AIC.  I copied some of the description text in the MSI
> controller binding schema from msi.txt but it may need some further
> tweaks to make sense.
> 
> 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:
> 
> v4: - Convert MSI controller binding to YAML
>     - Add generic msi-ranges property to MSI controller binding
>     - Fix typos/formatting in apple,pcie binding
>     - Use generic MSI controller binding in apple,pcie
> 
> 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 (4):
>   dt-bindings: interrupt-controller: Convert MSI controller to
>     json-schema
>   dt-bindings: interrupt-controller: msi: Add msi-ranges property
>   dt-bindings: pci: Add DT bindings for apple,pcie
>   arm64: apple: Add PCIe node
> 
>  .../interrupt-controller/msi-controller.yaml  |  42 +++++
>  .../devicetree/bindings/pci/apple,pcie.yaml   | 165 ++++++++++++++++++
>  .../bindings/pci/brcm,stb-pcie.yaml           |   1 +
>  .../bindings/pci/microchip,pcie-host.yaml     |   1 +
>  MAINTAINERS                                   |   1 +
>  arch/arm64/boot/dts/apple/t8103.dtsi          |  63 +++++++
>  6 files changed, 273 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
>  create mode 100644 Documentation/devicetree/bindings/pci/apple,pcie.yaml

With Rob's comments addressed, and the fix on the M1 RC MMIO region,
for the whole series:

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

	M.

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v4 2/4] dt-bindings: interrupt-controller: msi: Add msi-ranges property
  2021-08-31 21:16   ` Rob Herring
@ 2021-09-21 17:52     ` Mark Kettenis
  0 siblings, 0 replies; 29+ messages in thread
From: Mark Kettenis @ 2021-09-21 17:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, alyssa, kettenis, tglx, maz, marcan, bhelgaas,
	jim2101024, nsaenz, f.fainelli, bcm-kernel-feedback-list,
	daire.mcnamara, nsaenzjulienne, linux-kernel, linux-arm-kernel,
	linux-pci, linux-rpi-kernel

> Date: Tue, 31 Aug 2021 16:16:02 -0500
> From: Rob Herring <robh@kernel.org>
> 
> On Fri, Aug 27, 2021 at 07:15:27PM +0200, Mark Kettenis wrote:
> > From: Mark Kettenis <kettenis@openbsd.org>
> > 
> > Update the MSI controller binding to add an msi-ranges property
> > that specifies how MSIs map onto regular interrupts on some other
> > interrupt controller.
> > 
> > Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> > ---
> >  .../bindings/interrupt-controller/msi-controller.yaml     | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > index 5ed6cd46e2e0..bf8b8a7dba09 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/msi-controller.yaml
> > @@ -31,4 +31,12 @@ properties:
> >        Identifies the node as an MSI controller.
> >      $ref: /schemas/types.yaml#/definitions/flag
> >  
> > +  msi-ranges:
> > +    description:
> > +      A list of pairs <intid span>, where "intid" is the specification
> 
> It's not really 'pairs' and 'interrupt specifier' is the terminology the 
> spec uses. How about:
> 
> A list of <phandle intspec span>, where "phandle" is parent interrupt 
> controller, "intspec" is the starting/base interrupt specifier, and 
> "span" is the size of that range (typically multiples of 32).
> 
> The 'multiples of 32' part is what Marc told me.

Thanks Rob!  That sounds good.  But 32 is what's typical for the Apple
hardware, and I expect that different hardware that might use this
property will use a different value, so I left that last bit out.  I
also kept the bit that states that multiple ranges are allowed.

> > +      of the first interrupt (including the phandle for the interrupt
> > +      controller) that can be used as an MSI, and "span" the size of
> > +      that range. Multiple ranges can be provided.
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +
> >  additionalProperties: true
> > -- 
> > 2.32.0
> > 
> > 
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2021-09-21 17:52 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-27 17:15 [PATCH v4 0/4] Apple M1 PCIe DT bindings Mark Kettenis
2021-08-27 17:15 ` [PATCH v4 1/4] dt-bindings: interrupt-controller: Convert MSI controller to json-schema Mark Kettenis
2021-08-27 19:15   ` Mark Kettenis
2021-08-31 21:04     ` Rob Herring
2021-08-31 20:57   ` Rob Herring
2021-09-01 10:56     ` Mark Kettenis
2021-08-31 20:58   ` Rob Herring
2021-08-27 17:15 ` [PATCH v4 2/4] dt-bindings: interrupt-controller: msi: Add msi-ranges property Mark Kettenis
2021-08-31 21:16   ` Rob Herring
2021-09-21 17:52     ` Mark Kettenis
2021-08-27 17:15 ` [PATCH v4 3/4] dt-bindings: pci: Add DT bindings for apple,pcie Mark Kettenis
2021-08-27 17:58   ` Alyssa Rosenzweig
2021-08-27 18:22     ` Mark Kettenis
2021-08-31 21:21   ` Rob Herring
2021-09-01 11:29     ` Mark Kettenis
2021-09-12 20:13       ` Marc Zyngier
2021-09-13 20:55         ` Rob Herring
2021-08-27 17:15 ` [PATCH v4 4/4] arm64: apple: Add PCIe node Mark Kettenis
2021-08-27 17:59   ` Alyssa Rosenzweig
2021-08-27 18:24     ` Mark Kettenis
2021-08-27 20:09       ` Alyssa Rosenzweig
2021-08-30 11:37   ` Marc Zyngier
2021-08-30 14:57     ` Mark Kettenis
2021-08-30 20:40       ` Marc Zyngier
2021-08-30 15:57     ` Rob Herring
2021-08-30 20:20       ` Marc Zyngier
2021-09-12 21:30   ` Marc Zyngier
2021-09-13 18:35     ` Mark Kettenis
2021-09-21 11:01 ` [PATCH v4 0/4] Apple M1 PCIe DT bindings Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).