All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rob Herring <robh@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>,
	Jim Quinlan <jim2101024@gmail.com>,
	Nicolas Saenz Julienne <nsaenz@kernel.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" 
	<bcm-kernel-feedback-list@broadcom.com>,
	Daire McNamara <daire.mcnamara@microchip.com>,
	Nicolas 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 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
Date: Mon, 13 Sep 2021 15:55:59 -0500	[thread overview]
Message-ID: <CAL_JsqJYhrJxGCdUM8Fz50KN-AEARYzvfi3CcmyCVTkr8VCkjQ@mail.gmail.com> (raw)
In-Reply-To: <8735q9d02q.wl-maz@kernel.org>

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

WARNING: multiple messages have this Message-ID
From: Rob Herring <robh@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>,
	Jim Quinlan <jim2101024@gmail.com>,
	 Nicolas Saenz Julienne <nsaenz@kernel.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	 "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE"
	<bcm-kernel-feedback-list@broadcom.com>,
	 Daire McNamara <daire.mcnamara@microchip.com>,
	 Nicolas 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 3/4] dt-bindings: pci: Add DT bindings for apple,pcie
Date: Mon, 13 Sep 2021 15:55:59 -0500	[thread overview]
Message-ID: <CAL_JsqJYhrJxGCdUM8Fz50KN-AEARYzvfi3CcmyCVTkr8VCkjQ@mail.gmail.com> (raw)
In-Reply-To: <8735q9d02q.wl-maz@kernel.org>

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

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

  reply	other threads:[~2021-09-13 20:56 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 [this message]
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
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_JsqJYhrJxGCdUM8Fz50KN-AEARYzvfi3CcmyCVTkr8VCkjQ@mail.gmail.com \
    --to=robh@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 3/4] dt-bindings: pci: Add DT bindings for apple,pcie' \
    /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.