On 24 August 2017 at 23:12, Rob Herring <robh@xxxxxxxxxx> wrote: > On Thu, Aug 24, 2017 at 3:12 PM, Ard Biesheuvel > <ard.biesheuvel@xxxxxxxxxx> wrote: >> On 24 August 2017 at 21:02, Rob Herring <robh@xxxxxxxxxx> wrote: >>> Cc the DT list for bindings please. >>> >>> On Thu, Aug 24, 2017 at 1:43 PM, Ard Biesheuvel >>> <ard.biesheuvel@xxxxxxxxxx> wrote: >>>> Describe the binding for firmware-configured instances of the Synopsys >>>> Designware PCIe controller in RC mode. >>>> >>>> Cc: Rob Herring <robh@xxxxxxxxxx> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> >>>> --- >>>> 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. >
