From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9F6ADC432BE for ; Mon, 2 Aug 2021 16:10:55 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8959D60720 for ; Mon, 2 Aug 2021 16:10:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232481AbhHBQLD (ORCPT ); Mon, 2 Aug 2021 12:11:03 -0400 Received: from mail.kernel.org ([198.145.29.99]:34218 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232069AbhHBQLC (ORCPT ); Mon, 2 Aug 2021 12:11:02 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id CD9B461107; Mon, 2 Aug 2021 16:10:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1627920652; bh=rglWHlzS0hDB02auQNohIp4iijeWGQuf2Ja7Qb4hYUM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=unCZl25uSNAUzvN7qGehQ9rQCneXxegVdslhG2QXGl0Q5ZOqSFALCnPRGxRuvhgP6 DLkenKRkOntptdvXrk3AhtIrOBKknYu5YOap23EHL5/KX4EKqfdv3TEePLzT0yCRQc gi/9xNM0a984FHhLb939V90piXvxzsvKwySaMmeLQ6zjE05U9JYSJEDEstqMNojxDg Yg/eNvfehj0znMejm8C4yb5GInF4Iud/ZFqmOKWO0/e1JUcy/Xo8Zt4ybXDhj8t5sd /ZPyA63H4ACh53ampfTdyR62yHl7DHbSyQJES7sHBBW4OQ486PLYl2zCd+Dtdu9YOx ZI8y8Q904Tc8g== Received: by mail-qv1-f54.google.com with SMTP id cg4so4081351qvb.5; Mon, 02 Aug 2021 09:10:52 -0700 (PDT) X-Gm-Message-State: AOAM531kZ20GD9oIpUCgl0yaYkdzoJjQSvVuAodDvtch1CBvetSFGKs3 qt8fCvRvoSwQikq/sErxrf0osWUjOTt4UWjVVA== X-Google-Smtp-Source: ABdhPJzGQUjalfg03Ji3GXBsBmtGCSK/zTh5PrJJsVuj2U9Uk2JfYU1Debygp3dek7c3OE+8pMR8zpYBMxtmKWE4BYA= X-Received: by 2002:a0c:ff4b:: with SMTP id y11mr3347079qvt.50.1627920651928; Mon, 02 Aug 2021 09:10:51 -0700 (PDT) MIME-Version: 1.0 References: <20210726083204.93196-1-mark.kettenis@xs4all.nl> <20210726083204.93196-2-mark.kettenis@xs4all.nl> <20210726231848.GA1025245@robh.at.kernel.org> <87sfzt1pg9.wl-maz@kernel.org> In-Reply-To: <87sfzt1pg9.wl-maz@kernel.org> From: Rob Herring Date: Mon, 2 Aug 2021 10:10:39 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie To: Marc Zyngier Cc: Mark Kettenis , devicetree@vger.kernel.org, Robin Murphy , Sven Peter , Mark Kettenis , Hector Martin , Bjorn Helgaas , linux-arm-kernel , PCI , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier wrote: > > On Tue, 27 Jul 2021 00:18:48 +0100, > Rob Herring wrote: > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > From: Mark Kettenis > > > > > > 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 > > > --- > > > .../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 > > > + > > > +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 > > > > > + 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 > > > > > + the child nodes that represent the PCI bridges that correspond to > > > + the individual root ports. > > > > blank line > > > > > + 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 , 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. > > 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. That sounds like perfectly 'describing the h/w' to me. > 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. > 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' 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: , <0 1 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'? 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. 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. Rob From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 96DAEC4338F for ; Mon, 2 Aug 2021 16:12:57 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5969C60FA0 for ; Mon, 2 Aug 2021 16:12:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 5969C60FA0 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=nCKYZJx2v4qo4D4xR6W9KDrXTc+5flq59HEA5kTF9fc=; b=gDmHPiPFKuB6R4 qGvRVecuTC9lHtWXSxyRLZw9EBr0n4dXeWTu0Rw1JjTpeWHFdpwikYjwUfpKdW+jhbOhpZQxOvvGm 03qrCW7AuIG7GhyQqzIz5Azaqn9BKtpcfkp62lTDKyIOLonTOHDljwvLW0A+Q6BpcroVUhhbG6kvm t3jxf2TNNSCjq7/0ZiS1arwfnCSYVOxCLxdK9vgPzjQx4Cy7JvctWYiw96hqPbsCjB0aUnybv86GY COuSmIPtMN8GWDIzeCGjuYUvqGh3mlMgaRt0yr9ObvWxX9nnP+cQmYGqw+6M4YlVqOxRSQ5Mme+Cj 5BaDA4V5PHhcoeQ0cQng==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mAaWn-00H74P-QY; Mon, 02 Aug 2021 16:10:57 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mAaWj-00H73R-30 for linux-arm-kernel@lists.infradead.org; Mon, 02 Aug 2021 16:10:55 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id C47A961102 for ; Mon, 2 Aug 2021 16:10:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1627920652; bh=rglWHlzS0hDB02auQNohIp4iijeWGQuf2Ja7Qb4hYUM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=unCZl25uSNAUzvN7qGehQ9rQCneXxegVdslhG2QXGl0Q5ZOqSFALCnPRGxRuvhgP6 DLkenKRkOntptdvXrk3AhtIrOBKknYu5YOap23EHL5/KX4EKqfdv3TEePLzT0yCRQc gi/9xNM0a984FHhLb939V90piXvxzsvKwySaMmeLQ6zjE05U9JYSJEDEstqMNojxDg Yg/eNvfehj0znMejm8C4yb5GInF4Iud/ZFqmOKWO0/e1JUcy/Xo8Zt4ybXDhj8t5sd /ZPyA63H4ACh53ampfTdyR62yHl7DHbSyQJES7sHBBW4OQ486PLYl2zCd+Dtdu9YOx ZI8y8Q904Tc8g== Received: by mail-qv1-f44.google.com with SMTP id 3so9123269qvd.2 for ; Mon, 02 Aug 2021 09:10:52 -0700 (PDT) X-Gm-Message-State: AOAM5335ShezN8IpUGIk4lTF5D+GMYCik5PpWqxP6r+2tdrEyGeeR3EP U5k4MMMKFKcXXY9T+lYGuQVlhoskSlMDfzr20g== X-Google-Smtp-Source: ABdhPJzGQUjalfg03Ji3GXBsBmtGCSK/zTh5PrJJsVuj2U9Uk2JfYU1Debygp3dek7c3OE+8pMR8zpYBMxtmKWE4BYA= X-Received: by 2002:a0c:ff4b:: with SMTP id y11mr3347079qvt.50.1627920651928; Mon, 02 Aug 2021 09:10:51 -0700 (PDT) MIME-Version: 1.0 References: <20210726083204.93196-1-mark.kettenis@xs4all.nl> <20210726083204.93196-2-mark.kettenis@xs4all.nl> <20210726231848.GA1025245@robh.at.kernel.org> <87sfzt1pg9.wl-maz@kernel.org> In-Reply-To: <87sfzt1pg9.wl-maz@kernel.org> From: Rob Herring Date: Mon, 2 Aug 2021 10:10:39 -0600 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 1/2] dt-bindings: pci: Add DT bindings for apple,pcie To: Marc Zyngier Cc: Mark Kettenis , devicetree@vger.kernel.org, Robin Murphy , Sven Peter , Mark Kettenis , Hector Martin , Bjorn Helgaas , linux-arm-kernel , PCI , "linux-kernel@vger.kernel.org" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210802_091053_210917_00780C77 X-CRM114-Status: GOOD ( 50.32 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, Aug 1, 2021 at 3:31 AM Marc Zyngier wrote: > > On Tue, 27 Jul 2021 00:18:48 +0100, > Rob Herring wrote: > > > > On Mon, Jul 26, 2021 at 10:32:00AM +0200, Mark Kettenis wrote: > > > From: Mark Kettenis > > > > > > 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 > > > --- > > > .../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 > > > + > > > +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 > > > > > + 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 > > > > > + the child nodes that represent the PCI bridges that correspond to > > > + the individual root ports. > > > > blank line > > > > > + 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 , 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. > > 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. That sounds like perfectly 'describing the h/w' to me. > 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. > 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' 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: , <0 1 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'? 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. 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. Rob _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel