Hi Rob,

Apologies for the delay, I somehow misplaced this email...

On Mon, 02 Aug 2021 17:10:39 +0100,
Rob Herring [off-list ref] wrote:

On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier [off-list ref] wrote:

quoted

On Tue, 27 Jul 2021 00:18:48 +0100,
Rob Herring [off-list ref] wrote:

quoted

On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote:

quoted

From: Mark Kettenis <redacted>

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 <redacted>
---
 .../devicetree/bindings/pci/apple,pcie.yaml   | 166 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 167 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..bfcbdee79c64
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/apple,pcie.yaml

@@ -0,0 +1,166 @@
+# 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.

blank line

quoted

+  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-gpio" and "max-link-speed" properties appear on

reset-gpios

quoted

+  the child nodes that represent the PCI bridges that correspond to
+  the individual root ports.

blank line

quoted

+  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#
+
+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-controller: true
+  msi-parent: true
+
+  msi-ranges:
+    description:
+      A list of pairs <intid span>, where "intid" is the first
+      interrupt number that can be used as an MSI, and "span" the size
+      of that range.
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    items:
+      minItems: 2
+      maxItems: 2

I still have issues I raised on v1 with this property. It's genericish
looking, but not generic. 'intid' as a single cell can't specify any
parent interrupt such as a GIC which uses 3 cells. You could put in all
the cells, but you'd still be assuming which cell you can increment.

The GIC bindings already use similar abstractions, see what we do for
both GICv2m and GICv3 MBIs. Other MSI controllers use similar
properties (alpine and loongson, for example).

That's the problem. Everyone making up their own crap.

And that crap gets approved:

https://lore.kernel.org/lkml/20200512205704.GA10412@bogus/ (local)

I'm not trying to be antagonistic here, but it seems that your
position on this very subject has changed recently.

quoted

quoted

I think you should just list all these under 'interrupts' using
interrupt-names to make your life easier:

interrupt-names:
  items:
    - const: port0
    - const: port1
    - const: port2
    - const: msi0
    - const: msi1
    - const: msi2
    - const: msi3
    ...

Yeah, it's kind of verbose, but if the h/w block handles N interrupts,
you should list N interrupts. The worst case for the above is N entries
too if not contiguous.

And that's where I beg to differ, again.

Specifying interrupts like this gives the false impression that these
interrupts are generated by the device that owns them (the RC). Which
for MSIs is not the case.

It's no different than an interrupt controller node having an
interrupts property. The source is downstream and the interrupt
controller is combining/translating the interrupts.

The physical interrupt signals are connected to and originating in
this block.

Oh, I also object to this, for the same reasons. The only case where
it makes sense IMHO is when the interrupt controller is a multiplexer.

That sounds like perfectly 'describing the h/w' to me.

I guess we have a different view of about these things. At the end of
the day, I don't care enough as long as we can expose a range of
interrupts one way or another.

quoted

This is not only verbose, this is
semantically dubious. And what should we do when the number of
possible interrupt is ridiculously large, as it is for the GICv3 ITS?

I don't disagree with the verbose part. But that's not really an issue
in this case.

quoted

I wish we had a standard way to express these constraints. Until we
do, I don't think enumerating individual interrupts is a practical
thing to do, nor that it actually represents the topology of the
system.

The only way a standard way will happen is to stop accepting the
custom properties.

All the custom properties suffer from knowledge of what the parent
interrupt controller is. To fix that, I think we need something like
this:

msi-ranges = <intspec base>, <intspec step>, <intspec end>;

'intspec' is defined by the parent interrupt-controller cells. step is
the value to add. And end is what to match on to stop aka the last
interrupt in the range. For example, if the GIC is the parent, we'd
have something like this:

<GIC_SPI 123 0>, <0 1 0>, <GIC_SPI 124 0>

Does this apply to cases other than MSI? I think so as don't we have
the same type of properties with the low power mode shadow interrupt
controllers?  So 'interrupt-ranges'?

This would work, though the increment seems a bit over-engineered. You
also may need this property to accept multiple ranges.

It looks to me like there's an assumption in the kernel that an MSI
controller has a linear range of parent interrupts? Is that correct
and something that's guaranteed? That assumption leaks into the
existing bindings.

Depends on how the controller works. In general, the range maps to the
MultiMSI requirements where the message is an offset from the base of
the interrupt range. So you generally end-up with ranges of at least
32 contiguous MSIs. Anything under that is sub-par and probably not
worth supporting.

Of course, the controller may have some mapping facilities, which
makes things more... interesting.

It's fine for the kernel to assume that until there's a case that's
not linear, but a common binding needs to be able handle a
non-linear case.

Fair enough. I can probably work with Mark to upgrade the binding and
the M1 PCIe code. Could you come up with a more formalised proposal?

Thanks,

	M.

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