Re: [PATCH v4 4/4] arm64: apple: Add PCIe node

2 messages, 2 authors, 2021-08-30 · open the first message on its own page

Re: [PATCH v4 4/4] arm64: apple: Add PCIe node

From: Mark Kettenis <hidden>
Date: 2021-08-30 14:57:09

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 [off-list ref] wrote:
quoted
From: Mark Kettenis <redacted>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/4] arm64: apple: Add PCIe node

From: Marc Zyngier <maz@kernel.org>
Date: 2021-08-30 20:40:48

On Mon, 30 Aug 2021 15:57:02 +0100,
Mark Kettenis [off-list ref] wrote:
quoted
Date: Mon, 30 Aug 2021 12:37:31 +0100
From: Marc Zyngier <maz@kernel.org>

Hi Mark,
Hi Marc,
quoted
On Fri, 27 Aug 2021 18:15:29 +0100,
Mark Kettenis [off-list ref] wrote:
quoted
From: Mark Kettenis <redacted>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help