All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rob Herring <robh+dt@kernel.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Mark Kettenis <mark.kettenis@xs4all.nl>,
	devicetree@vger.kernel.org,
	Alyssa Rosenzweig <alyssa@rosenzweig.io>,
	Mark Kettenis <kettenis@openbsd.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Hector Martin <marcan@marcan.st>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Nicolas Saenz Julienne <nsaenz@kernel.org>,
	Jim Quinlan <jim2101024@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
	<bcm-kernel-feedback-list@broadcom.com>,
	Daire McNamara <daire.mcnamara@microchip.com>,
	Saenz Julienne <nsaenzjulienne@suse.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	PCI <linux-pci@vger.kernel.org>,
	"moderated list:BROADCOM BCM2835 ARM ARCHITECTURE" 
	<linux-rpi-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
Date: Mon, 30 Aug 2021 10:57:59 -0500	[thread overview]
Message-ID: <CAL_JsqJC+FxiynFkkcB0amp3s4agsio5ggCrYiPbqoXroAJV4Q@mail.gmail.com> (raw)
In-Reply-To: <87pmtvcgec.wl-maz@kernel.org>

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

WARNING: multiple messages have this Message-ID
From: Rob Herring <robh+dt@kernel.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Mark Kettenis <mark.kettenis@xs4all.nl>,
	devicetree@vger.kernel.org,
	 Alyssa Rosenzweig <alyssa@rosenzweig.io>,
	Mark Kettenis <kettenis@openbsd.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Hector Martin <marcan@marcan.st>,
	 Bjorn Helgaas <bhelgaas@google.com>,
	Nicolas Saenz Julienne <nsaenz@kernel.org>,
	Jim Quinlan <jim2101024@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	 "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
	<bcm-kernel-feedback-list@broadcom.com>,
	 Daire McNamara <daire.mcnamara@microchip.com>,
	Saenz Julienne <nsaenzjulienne@suse.de>,
	 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	 linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	PCI <linux-pci@vger.kernel.org>,
	 "moderated list:BROADCOM BCM2835 ARM ARCHITECTURE"
	<linux-rpi-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 4/4] arm64: apple: Add PCIe node
Date: Mon, 30 Aug 2021 10:57:59 -0500	[thread overview]
Message-ID: <CAL_JsqJC+FxiynFkkcB0amp3s4agsio5ggCrYiPbqoXroAJV4Q@mail.gmail.com> (raw)
In-Reply-To: <87pmtvcgec.wl-maz@kernel.org>

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

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

  parent reply	other threads:[~2021-08-30 15:58 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 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-27 19:15   ` Mark Kettenis
2021-08-27 19:15     ` Mark Kettenis
2021-08-31 21:04     ` Rob Herring
2021-08-31 21:04       ` Rob Herring
2021-08-31 20:57   ` Rob Herring
2021-08-31 20:57     ` Rob Herring
2021-09-01 10:56     ` Mark Kettenis
2021-09-01 10:56       ` Mark Kettenis
2021-08-31 20:58   ` Rob Herring
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-27 17:15   ` Mark Kettenis
2021-08-31 21:16   ` Rob Herring
2021-08-31 21:16     ` Rob Herring
2021-09-21 17:52     ` Mark Kettenis
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:15   ` Mark Kettenis
2021-08-27 17:58   ` Alyssa Rosenzweig
2021-08-27 17:58     ` Alyssa Rosenzweig
2021-08-27 18:22     ` Mark Kettenis
2021-08-27 18:22       ` Mark Kettenis
2021-08-31 21:21   ` Rob Herring
2021-08-31 21:21     ` Rob Herring
2021-09-01 11:29     ` Mark Kettenis
2021-09-01 11:29       ` Mark Kettenis
2021-09-12 20:13       ` Marc Zyngier
2021-09-12 20:13         ` Marc Zyngier
2021-09-13 20:55         ` Rob Herring
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:15   ` Mark Kettenis
2021-08-27 17:59   ` Alyssa Rosenzweig
2021-08-27 17:59     ` Alyssa Rosenzweig
2021-08-27 18:24     ` Mark Kettenis
2021-08-27 18:24       ` Mark Kettenis
2021-08-27 20:09       ` Alyssa Rosenzweig
2021-08-27 20:09         ` Alyssa Rosenzweig
2021-08-30 11:37   ` Marc Zyngier
2021-08-30 11:37     ` Marc Zyngier
2021-08-30 14:57     ` Mark Kettenis
2021-08-30 14:57       ` Mark Kettenis
2021-08-30 20:40       ` Marc Zyngier
2021-08-30 20:40         ` Marc Zyngier
2021-08-30 15:57     ` Rob Herring [this message]
2021-08-30 15:57       ` Rob Herring
2021-08-30 20:20       ` Marc Zyngier
2021-08-30 20:20         ` Marc Zyngier
2021-09-12 21:30   ` Marc Zyngier
2021-09-12 21:30     ` Marc Zyngier
2021-09-13 18:35     ` Mark Kettenis
2021-09-13 18:35       ` Mark Kettenis
2021-09-21 11:01 ` [PATCH v4 0/4] Apple M1 PCIe DT bindings Marc Zyngier
2021-09-21 11:01   ` Marc Zyngier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAL_JsqJC+FxiynFkkcB0amp3s4agsio5ggCrYiPbqoXroAJV4Q@mail.gmail.com \
    --to=robh+dt@kernel.org \
    --cc=alyssa@rosenzweig.io \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=bhelgaas@google.com \
    --cc=daire.mcnamara@microchip.com \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=jim2101024@gmail.com \
    --cc=kettenis@openbsd.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=marcan@marcan.st \
    --cc=mark.kettenis@xs4all.nl \
    --cc=maz@kernel.org \
    --cc=nsaenz@kernel.org \
    --cc=nsaenzjulienne@suse.de \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH v4 4/4] arm64: apple: Add PCIe node' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.