| Message ID | 20170824184321.19432-4-ard.biesheuvel@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Cc the DT list for bindings please. On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Describe the binding for firmware-configured instances of the Synopsys > Designware PCIe controller in RC mode. > > Cc: Rob Herring <robh@kernel.org> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt > new file mode 100644 > index 000000000000..b8127b19c220 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt > @@ -0,0 +1,56 @@ > +* Synopsys Designware PCIe root complex in ECAM mode > + > +In some cases, firmware may already have configured the Synopsys Designware > +PCIe controller in RC mode with static ATU window mappings that cover all > +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion. > +In this case, there is no need for the OS to perform any low level setup > +of clocks or device registers, nor is there any reason for the driver to > +reconfigure ATU windows for config and/or IO space accesses at runtime. > + > +Such hardware configurations should be described as "pci-host-ecam-generic" > +if they are truly ECAM compatible. Configurations that require no low-level > +setup by the OS nor any ATU window reconfiguration at runtime, but do > +require special handling for type 0 config TLPs may instead be described as > +"snps,dw-pcie-ecam". Humm, what happens when we have the next exception that's SoC specific or another vendor? Seems like perhaps "firmware initialized" should have been a separate property flag for bootloaders to add rather than a compatible string. I'd rather see this done in a way that does not require DT updates if quirks have to be handled/added later. Rob
On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote: > Cc the DT list for bindings please. > > On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> Describe the binding for firmware-configured instances of the Synopsys >> Designware PCIe controller in RC mode. >> >> Cc: Rob Herring <robh@kernel.org> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >> new file mode 100644 >> index 000000000000..b8127b19c220 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >> @@ -0,0 +1,56 @@ >> +* Synopsys Designware PCIe root complex in ECAM mode >> + >> +In some cases, firmware may already have configured the Synopsys Designware >> +PCIe controller in RC mode with static ATU window mappings that cover all >> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion. >> +In this case, there is no need for the OS to perform any low level setup >> +of clocks or device registers, nor is there any reason for the driver to >> +reconfigure ATU windows for config and/or IO space accesses at runtime. >> + >> +Such hardware configurations should be described as "pci-host-ecam-generic" >> +if they are truly ECAM compatible. Configurations that require no low-level >> +setup by the OS nor any ATU window reconfiguration at runtime, but do >> +require special handling for type 0 config TLPs may instead be described as >> +"snps,dw-pcie-ecam". > > Humm, what happens when we have the next exception that's SoC specific > or another vendor? This is not SoC specific, but IP specific. We are working with two different SoCs from completely different vendors that both synthesized this IP with a 64 KB ATU window size, not expecting this to break ECAM compatibility. > Seems like perhaps "firmware initialized" should > have been a separate property flag for bootloaders to add rather than > a compatible string. > Yes, but then you still have 10 different drivers that all retain the low-level bits that are all different between SoCs. That is exactly what I want to get rid of, and usually we can do that with existing bindings, because we simply expose it as pci-host-ecam-generic. Only in this particular case, that doesn't fly due to the quirk. > I'd rather see this done in a way that does not require DT updates if > quirks have to be handled/added later. > Do you see a way that still allows us to keep the abstraction? I don't want a flag, I simply don't want to expose any low-level specifics about the device to the OS, beyond what it needs to use it in its configured state.
On Thu, Aug 24, 2017 at 3:12 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote: >> Cc the DT list for bindings please. >> >> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> Describe the binding for firmware-configured instances of the Synopsys >>> Designware PCIe controller in RC mode. >>> >>> Cc: Rob Herring <robh@kernel.org> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- >>> Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++ >>> 1 file changed, 56 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >>> new file mode 100644 >>> index 000000000000..b8127b19c220 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >>> @@ -0,0 +1,56 @@ >>> +* Synopsys Designware PCIe root complex in ECAM mode >>> + >>> +In some cases, firmware may already have configured the Synopsys Designware >>> +PCIe controller in RC mode with static ATU window mappings that cover all >>> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion. >>> +In this case, there is no need for the OS to perform any low level setup >>> +of clocks or device registers, nor is there any reason for the driver to >>> +reconfigure ATU windows for config and/or IO space accesses at runtime. >>> + >>> +Such hardware configurations should be described as "pci-host-ecam-generic" >>> +if they are truly ECAM compatible. Configurations that require no low-level >>> +setup by the OS nor any ATU window reconfiguration at runtime, but do >>> +require special handling for type 0 config TLPs may instead be described as >>> +"snps,dw-pcie-ecam". >> >> Humm, what happens when we have the next exception that's SoC specific >> or another vendor? > > This is not SoC specific, but IP specific. We are working with two > different SoCs from completely different vendors that both synthesized > this IP with a 64 KB ATU window size, not expecting this to break ECAM > compatibility. This case is not SoC specific, but quirks/bugs show up in both places. Plus "dw-pcie" is fairly meaningless because the IP is both configurable and has multiple releases. >> Seems like perhaps "firmware initialized" should >> have been a separate property flag for bootloaders to add rather than >> a compatible string. >> > > Yes, but then you still have 10 different drivers that all retain the > low-level bits that are all different between SoCs. That is exactly > what I want to get rid of, and usually we can do that with existing > bindings, because we simply expose it as pci-host-ecam-generic. Only > in this particular case, that doesn't fly due to the quirk. You can expose it as "vendor,soc-pcie", "pci-host-ecam-generic" and then the kernel can decide. Then you can use: the default ecam driver, the ecam driver but match on "vendor,soc-pcie" for quirks, or a vendor specific driver. The only thing that wouldn't work would be having both quirks in the ecam driver and a vendor driver which IMO we just shouldn't support anyway. Now if you have 10 SoCs all needing this same quirk, then maybe adding "snps,dw-pcie-ecam" addtionally to the above makes sense. But that only saves you match strings. >> I'd rather see this done in a way that does not require DT updates if >> quirks have to be handled/added later. >> > > Do you see a way that still allows us to keep the abstraction? I don't > want a flag, I simply don't want to expose any low-level specifics > about the device to the OS, beyond what it needs to use it in its > configured state. That's not how DT works. Detailed information is exposed to the OS and the OS decides if it can handle things generically or not because that decision can change over time. Whenever we don't make things specific enough, we've gotten burned. Rob
On 24 August 2017 at 23:12, Rob Herring <robh@kernel.org> wrote: > On Thu, Aug 24, 2017 at 3:12 PM, Ard Biesheuvel > <ard.biesheuvel@linaro.org> wrote: >> On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote: >>> Cc the DT list for bindings please. >>> >>> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel >>> <ard.biesheuvel@linaro.org> wrote: >>>> Describe the binding for firmware-configured instances of the Synopsys >>>> Designware PCIe controller in RC mode. >>>> >>>> Cc: Rob Herring <robh@kernel.org> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>> --- >>>> Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++ >>>> 1 file changed, 56 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >>>> new file mode 100644 >>>> index 000000000000..b8127b19c220 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >>>> @@ -0,0 +1,56 @@ >>>> +* Synopsys Designware PCIe root complex in ECAM mode >>>> + >>>> +In some cases, firmware may already have configured the Synopsys Designware >>>> +PCIe controller in RC mode with static ATU window mappings that cover all >>>> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion. >>>> +In this case, there is no need for the OS to perform any low level setup >>>> +of clocks or device registers, nor is there any reason for the driver to >>>> +reconfigure ATU windows for config and/or IO space accesses at runtime. >>>> + >>>> +Such hardware configurations should be described as "pci-host-ecam-generic" >>>> +if they are truly ECAM compatible. Configurations that require no low-level >>>> +setup by the OS nor any ATU window reconfiguration at runtime, but do >>>> +require special handling for type 0 config TLPs may instead be described as >>>> +"snps,dw-pcie-ecam". >>> >>> Humm, what happens when we have the next exception that's SoC specific >>> or another vendor? >> >> This is not SoC specific, but IP specific. We are working with two >> different SoCs from completely different vendors that both synthesized >> this IP with a 64 KB ATU window size, not expecting this to break ECAM >> compatibility. > > This case is not SoC specific, but quirks/bugs show up in both places. > Plus "dw-pcie" is fairly meaningless because the IP is both > configurable and has multiple releases. > >>> Seems like perhaps "firmware initialized" should >>> have been a separate property flag for bootloaders to add rather than >>> a compatible string. >>> >> >> Yes, but then you still have 10 different drivers that all retain the >> low-level bits that are all different between SoCs. That is exactly >> what I want to get rid of, and usually we can do that with existing >> bindings, because we simply expose it as pci-host-ecam-generic. Only >> in this particular case, that doesn't fly due to the quirk. > > You can expose it as "vendor,soc-pcie", "pci-host-ecam-generic" and > then the kernel can decide. Then you can use: the default ecam driver, > the ecam driver but match on "vendor,soc-pcie" for quirks, or a vendor > specific driver. The only thing that wouldn't work would be having > both quirks in the ecam driver and a vendor driver which IMO we just > shouldn't support anyway. > > Now if you have 10 SoCs all needing this same quirk, then maybe adding > "snps,dw-pcie-ecam" addtionally to the above makes sense. But that > only saves you match strings. > Exposing this as "pci-host-ecam-generic" doesn't work, or we wouldn't be having this discussion. So if I understand you correctly, you would rather have marvell,armada8k-pcie-ecam socionext,synquacer-pcie-ecam and add more of those to the list if needed, and update the binding+driver to use those and drop the generic identifier? I take it that applies to the description of the MSI frame as well? >>> I'd rather see this done in a way that does not require DT updates if >>> quirks have to be handled/added later. >>> >> >> Do you see a way that still allows us to keep the abstraction? I don't >> want a flag, I simply don't want to expose any low-level specifics >> about the device to the OS, beyond what it needs to use it in its >> configured state. > > That's not how DT works. Detailed information is exposed to the OS and > the OS decides if it can handle things generically or not because that > decision can change over time. Whenever we don't make things specific > enough, we've gotten burned. >
On Thu, Aug 24, 2017 at 5:37 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 24 August 2017 at 23:12, Rob Herring <robh@kernel.org> wrote: >> On Thu, Aug 24, 2017 at 3:12 PM, Ard Biesheuvel >> <ard.biesheuvel@linaro.org> wrote: >>> On 24 August 2017 at 21:02, Rob Herring <robh@kernel.org> wrote: >>>> Cc the DT list for bindings please. >>>> >>>> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel >>>> <ard.biesheuvel@linaro.org> wrote: >>>>> Describe the binding for firmware-configured instances of the Synopsys >>>>> Designware PCIe controller in RC mode. >>>>> >>>>> Cc: Rob Herring <robh@kernel.org> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> --- >>>>> Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++ >>>>> 1 file changed, 56 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >>>>> new file mode 100644 >>>>> index 000000000000..b8127b19c220 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt >>>>> @@ -0,0 +1,56 @@ >>>>> +* Synopsys Designware PCIe root complex in ECAM mode >>>>> + >>>>> +In some cases, firmware may already have configured the Synopsys Designware >>>>> +PCIe controller in RC mode with static ATU window mappings that cover all >>>>> +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion. >>>>> +In this case, there is no need for the OS to perform any low level setup >>>>> +of clocks or device registers, nor is there any reason for the driver to >>>>> +reconfigure ATU windows for config and/or IO space accesses at runtime. >>>>> + >>>>> +Such hardware configurations should be described as "pci-host-ecam-generic" >>>>> +if they are truly ECAM compatible. Configurations that require no low-level >>>>> +setup by the OS nor any ATU window reconfiguration at runtime, but do >>>>> +require special handling for type 0 config TLPs may instead be described as >>>>> +"snps,dw-pcie-ecam". >>>> >>>> Humm, what happens when we have the next exception that's SoC specific >>>> or another vendor? >>> >>> This is not SoC specific, but IP specific. We are working with two >>> different SoCs from completely different vendors that both synthesized >>> this IP with a 64 KB ATU window size, not expecting this to break ECAM >>> compatibility. >> >> This case is not SoC specific, but quirks/bugs show up in both places. >> Plus "dw-pcie" is fairly meaningless because the IP is both >> configurable and has multiple releases. >> >>>> Seems like perhaps "firmware initialized" should >>>> have been a separate property flag for bootloaders to add rather than >>>> a compatible string. >>>> >>> >>> Yes, but then you still have 10 different drivers that all retain the >>> low-level bits that are all different between SoCs. That is exactly >>> what I want to get rid of, and usually we can do that with existing >>> bindings, because we simply expose it as pci-host-ecam-generic. Only >>> in this particular case, that doesn't fly due to the quirk. >> >> You can expose it as "vendor,soc-pcie", "pci-host-ecam-generic" and >> then the kernel can decide. Then you can use: the default ecam driver, >> the ecam driver but match on "vendor,soc-pcie" for quirks, or a vendor >> specific driver. The only thing that wouldn't work would be having >> both quirks in the ecam driver and a vendor driver which IMO we just >> shouldn't support anyway. >> >> Now if you have 10 SoCs all needing this same quirk, then maybe adding >> "snps,dw-pcie-ecam" addtionally to the above makes sense. But that >> only saves you match strings. >> > > Exposing this as "pci-host-ecam-generic" doesn't work, or we wouldn't > be having this discussion. Exposing it *alone* won't work, but you could match on it for the driver and then in the probe function have an: if (of_device_is_compatible(np, "marvell,armada8k-pcie-ecam")) // handle device quirk I haven't looked at what exactly you need to handle though. I don't have the rest of the series. But since you already know the problem, you could just drop "pci-host-ecam-generic" in this case. > > So if I understand you correctly, you would rather have > > marvell,armada8k-pcie-ecam > socionext,synquacer-pcie-ecam > > and add more of those to the list if needed, and update the > binding+driver to use those and drop the generic identifier? Yes, but dropping the generic identifier depends if you know you have some problem up front. You do here, but that's not always the case. The other part is that "pci-host-ecam-generic" alone should not be considered valid other than for VM guests. But I can't really enforce that. > I take it > that applies to the description of the MSI frame as well? Probably so. Is there something special about it, too? Rob
diff --git a/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt new file mode 100644 index 000000000000..b8127b19c220 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt @@ -0,0 +1,56 @@ +* Synopsys Designware PCIe root complex in ECAM mode + +In some cases, firmware may already have configured the Synopsys Designware +PCIe controller in RC mode with static ATU window mappings that cover all +config, MMIO and I/O spaces in a [mostly] ECAM compatible fashion. +In this case, there is no need for the OS to perform any low level setup +of clocks or device registers, nor is there any reason for the driver to +reconfigure ATU windows for config and/or IO space accesses at runtime. + +Such hardware configurations should be described as "pci-host-ecam-generic" +if they are truly ECAM compatible. Configurations that require no low-level +setup by the OS nor any ATU window reconfiguration at runtime, but do +require special handling for type 0 config TLPs may instead be described as +"snps,dw-pcie-ecam". + +Required properties: +- compatible: should contain "snps,dw-pcie-ecam". + +Please refer to the binding document of "pci-host-ecam-generic" in the +file host-generic-pci.txt for a description of the remaining required +and optional properties. + + +* MSI support for Synopsys Designware PCIe root complex in ECAM mode + +Platforms that elect to perform all configuration of the RC in firmware +and use the "pci-host-ecam-generic" or "snps,dw-pcie-ecam" binding to +describe it to the OS may include a separate description of the embedded +MSI controller in case no MSI support is available in the core interrupt +controller. + +Required properties: +- compatible: should contain "snps,dw-pcie-msi". +- reg: a single region describing the device registers. +- interrupts: interrupt specifier for the interrupt that is asserted when + an MSI is received by the RC. +- msi-controller: empty property identifying this device as an MSI controller. + +Example for an implementation that routes all legacy INTx interrupts via SPI +#188 and all MSI interrupts via SPI #190: + + pcie@20000000 { + compatible = "snps,dw-pcie-ecam"; + device_type = "pci"; + msi-parent = <&msi0>; + interrupt-map-mask = <0x0 0x0 0x0 0x0>; + interrupt-map = <0x0 0x0 0x0 0x0 &gic GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>; + ... + }; + + msi0: msi@10000000 { + compatible = "snps,dw-pcie-msi"; + reg = <0x0 0x10000000 0x0 0x10000>; + interrupts = <GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>; + msi-controller; + };
Describe the binding for firmware-configured instances of the Synopsys Designware PCIe controller in RC mode. Cc: Rob Herring <robh@kernel.org> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt | 56 ++++++++++++++++++++ 1 file changed, 56 insertions(+)