RE: [PATCH 4/4] dt-bindings: pci: layerscape-pci: define aer/pme interrupts

-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Tuesday, November 30, 2021 7:47 AM
To: Leo Li <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Z.Q. Hou
[off-list ref]
Subject: Re: [PATCH 4/4] dt-bindings: pci: layerscape-pci: define aer/pme
interrupts

On Mon, Nov 29, 2021 at 9:35 PM Leo Li [off-list ref] wrote:

quoted

quoted

-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Monday, November 29, 2021 8:02 PM
To: Leo Li <redacted>
Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Z.Q. Hou
[off-list ref]
Subject: Re: [PATCH 4/4] dt-bindings: pci: layerscape-pci: define
aer/pme interrupts

On Fri, Nov 19, 2021 at 06:16:21PM -0600, Li Yang wrote:

quoted

Some platforms using this controller have separated interrupt
lines for aer or pme events instead of having a single interrupt
line for miscellaneous events.  Define interrupts in the binding
for these interrupt lines.

Signed-off-by: Li Yang <redacted>
---
 .../devicetree/bindings/pci/layerscape-pci.txt     | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git
a/Documentation/devicetree/bindings/pci/layerscape-pci.txt
b/Documentation/devicetree/bindings/pci/layerscape-pci.txt
index 8fd6039a826b..bcf11bfc4bab 100644

--- a/Documentation/devicetree/bindings/pci/layerscape-pci.txt
+++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt

@@ -31,8 +31,13 @@ Required properties:
 - reg: base addresses and lengths of the PCIe controller register blocks.
 - interrupts: A list of interrupt outputs of the controller. Must contain

an

quoted

quoted

quoted

   entry for each entry in the interrupt-names property.
-- interrupt-names: Must include the following entries:
-  "intr": The interrupt that is asserted for controller
interrupts
+- interrupt-names: It could include the following entries:
+  "aer": For interrupt line reporting aer events when non
+MSI/MSI-X/INTx

mode

quoted

+           is used
+  "pme": For interrupt line reporting pme events when non
+ MSI/MSI-

X/INTx mode

quoted

+           is used
+  "intr": For interrupt line reporting miscellaneous controller
+events
+  ......
 - fsl,pcie-scfg: Must include two entries.
   The first entry must be a link to the SCFG device node
   The second entry is the physical PCIe controller index starting from '0'.

@@ -52,8 +57,9 @@ Example:
            reg = <0x00 0x03400000 0x0 0x00010000   /* controller

registers */

quoted

                   0x40 0x00000000 0x0 0x00002000>; /*
configuration space

*/

quoted

            reg-names = "regs", "config";
-           interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /*

controller interrupt */

quoted

-           interrupt-names = "intr";
+           interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>, /* aer

interrupt */

quoted

+                   <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>; /* pme

interrupt */

quoted

+           interrupt-names = "aer", "pme";

This isn't a compatible change. The h/w suddenly has no 'intr'
interrupt?

The original 'intr' was just a place holder for a HW interrupt signal without a

clear definition of events associated.  Some later SoC has more interrupt
signals to associate with more specific events.

'Later SoC' means new compatible, but you're not changing the compatible. If
it was just wrong for all SoCs, then state that in the commit message. Please
define all the interrupts on all SoCs, so it is not changing again.

Different SoCs could have different number of interrupt lines and the events routing could also be different among SoCs.  It is really hard to name the interrupt lines properly that works for all SoCs.  That is probably why we chose to name key events instead of interrupt lines.

The HW documentation is also not very clear on this part.  But I will try to summarize the situation better in next version, for example:

"aer": Used for interrupt line which reports AER events when non MSI/MSI-X/INTx mode is used
"pme": Used for interrupt line which reports PME events when non MSI/MSI-X/INTx mode is used
"intr": Used for SoC(like ls2080a, lx2160, ls2080, ls2088, ls1088) which has a single interrupt line for miscellaneous controller events(which could include AER and PME events).

quoted

If needed, we can keep the "intr" interrupt-name there just for backward

compatibility although it was never used in Linux.

What about other OSs?

Rob