| Message ID | 20170313104356.GA23239@leverpostej (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Mark > -----Original Message----- > From: Mark Rutland [mailto:mark.rutland@arm.com] > Sent: 13 March 2017 10:44 > 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) > > 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? Yes indeed it would work for us. We could discuss about the appropriateness of ecam vs almost-ecam, but I don't think it is of much value as long as we make the meaning clear in the Documentation We'll send the fix below to lists in a separate patch... Many Thanks Gab > > 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, > }, > {},
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, }, {},