diff mbox series

[v4,4/4] arm64: apple: Add PCIe node

Message ID 20210827171534.62380-5-mark.kettenis@xs4all.nl
State New
Headers show
Series Apple M1 PCIe DT bindings | expand

Commit Message

Mark Kettenis Aug. 27, 2021, 5:15 p.m. UTC
From: Mark Kettenis <kettenis@openbsd.org>

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(+)

Comments

Alyssa Rosenzweig Aug. 27, 2021, 5:59 p.m. UTC | #1
> 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?
Mark Kettenis Aug. 27, 2021, 6:24 p.m. UTC | #2
> 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.
Marc Zyngier Aug. 30, 2021, 11:37 a.m. UTC | #3
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.
Mark Kettenis Aug. 30, 2021, 2:57 p.m. UTC | #4
> 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.
Rob Herring Aug. 30, 2021, 3:57 p.m. UTC | #5
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
Marc Zyngier Aug. 30, 2021, 8:20 p.m. UTC | #6
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.
Marc Zyngier Aug. 30, 2021, 8:40 p.m. UTC | #7
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.
Marc Zyngier Sept. 12, 2021, 9:30 p.m. UTC | #8
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.
Mark Kettenis Sept. 13, 2021, 6:35 p.m. UTC | #9
> 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
diff mbox series

Patch

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;
+			};
+		};
 	};
 };