From patchwork Mon Mar 13 10:44:28 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 9620449 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4836060244 for ; Mon, 13 Mar 2017 10:44:48 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3BBC4283F9 for ; Mon, 13 Mar 2017 10:44:48 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3050828456; Mon, 13 Mar 2017 10:44:48 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BD277283F9 for ; Mon, 13 Mar 2017 10:44:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751738AbdCMKor (ORCPT ); Mon, 13 Mar 2017 06:44:47 -0400 Received: from foss.arm.com ([217.140.101.70]:47542 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751521AbdCMKoq (ORCPT ); Mon, 13 Mar 2017 06:44:46 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0794A2B; Mon, 13 Mar 2017 03:44:45 -0700 (PDT) Received: from leverpostej (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B678A3F3E1; Mon, 13 Mar 2017 03:44:43 -0700 (PDT) Date: Mon, 13 Mar 2017 10:44:28 +0000 From: Mark Rutland To: Gabriele Paoloni Cc: "liudongdong (C)" , Bjorn Helgaas , "Wangzhou (B)" , "devicetree@vger.kernel.org" , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: Bad DT binding (hisi-pcie-almost-ecam) Message-ID: <20170313104356.GA23239@leverpostej> References: <20170310174045.GB24571@leverpostej> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, On Mon, Mar 13, 2017 at 08:14:19AM +0000, Gabriele Paoloni wrote: > > -----Original Message----- > > From: Mark Rutland [mailto:mark.rutland@arm.com] > > The commit adds the "hisilicon,pcie-almost-ecam", which goes against > > the > > usual DT conventions, and is non-sensical in that it describes the IP > > based on what it isn't. > > > > This binding shouldn't have gone in as-is, and we should fix it before > > v4.11. > > > > The binding states that this IP is found in Hip06 and Hip07. For these > > cases we'd usually take the name of the first implementation, e.g. > > something like "hisilicon,hip06-pcie", which can be used as a fallback > > in the compatible list if reused in subsequent SoC generations. > > > > I also see that "hisilicon,hip06-pcie" already exists, so I'm even more > > suspicious. > > For Hip06 the IP is the same but in we have a different BIOS configuration > that allows the controller be ECAM compliant for all the devices of the > hierarchy except the RC. > > > What exactly is the "hisilicon,pcie-almost-ecam" binding trying to > > describe? Is it a different IP also found on Hip06, or is it a new > > binding for the same IP? > > The reason why we need this new BIOS is to support the recent PCIe quirks > for the ACPI Root Port driver (commit 5b69b85ba1ddd36be01f5c57830b37a3c8256009 > "PCI/ACPI: Check for platform-specific MCFG quirks"). So using one BIOS we > support both DT and ACPI. > This is the reason why you see Hip-06 being already there... Ok. Thanks for clarifying that. What I think we should do is: a) Update the binding document to clarify when these strings are used. b) Use an SoC prefix in the string. I'm happy to have both hip06 and hip07 strings. c) Drop the "almost", since we don't use that elsewhere. (e.g. for cavium,pci-host-thunder-ecam" Would you be happy with the below? Thanks, Mark. ---->8---- diff --git a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt index b7fa3b9..535426d 100644 --- a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt +++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt @@ -44,13 +44,19 @@ Hip05 Example (note that Hip06 is the same except compatible): }; HiSilicon Hip06/Hip07 PCIe host bridge DT (almost-ECAM) description. + +Some BIOSes place the host controller in a mode where it is ECAM compliant for +all devices other than the root complex. In such cases, the host controller +should be described as below. + The properties and their meanings are identical to those described in host-generic-pci.txt except as listed below. Properties of the host controller node that differ from host-generic-pci.txt: -- compatible : Must be "hisilicon,pcie-almost-ecam" +- compatible : Must be "hisilicon,hip06-pcie-ecam", or + "hisilicon,hip07-pcie-ecam" - reg : Two entries: First the ECAM configuration space for any other bus underneath the root bus. Second, the base @@ -59,7 +65,7 @@ host-generic-pci.txt: Example: pcie0: pcie@a0090000 { - compatible = "hisilicon,pcie-almost-ecam"; + compatible = "hisilicon,hip06-pcie-ecam"; reg = <0 0xb0000000 0 0x2000000>, /* ECAM configuration space */ <0 0xa0090000 0 0x10000>; /* host bridge registers */ bus-range = <0 31>; diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c index fd66a31..bd5b1b4 100644 --- a/drivers/pci/dwc/pcie-hisi.c +++ b/drivers/pci/dwc/pcie-hisi.c @@ -380,7 +380,11 @@ struct pci_ecam_ops hisi_pcie_platform_ops = { static const struct of_device_id hisi_pcie_almost_ecam_of_match[] = { { - .compatible = "hisilicon,pcie-almost-ecam", + .compatible = "hisilicon,hip06-pcie-ecam", + .data = (void *) &hisi_pcie_platform_ops, + }, + { + .compatible = "hisilicon,hip07-pcie-ecam", .data = (void *) &hisi_pcie_platform_ops, }, {},