Re: [PATCH 3/3] dt-bindings: PCI: Convert generic host binding to DT schema

[Rob Herring]

On Fri, Dec 13, 2019 at 2:28 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Fri, Nov 15, 2019 at 06:52:40PM -0600, Rob Herring wrote:
> > Convert the generic PCI host binding to DT schema. The derivative Juno,
> > PLDA XpressRICH3-AXI, and Designware ECAM bindings all just vary in
> > their compatible strings. The simplest way to convert those to
> > schema is just add them into the common generic PCI host schema.
> >
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Cc: Andrew Murray <andrew.murray@xxxxxxx>
> > Cc: Zhou Wang <wangzhou1@xxxxxxxxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > Cc: David Daney <david.daney@xxxxxxxxxx>
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > ---
> >  .../bindings/pci/arm,juno-r1-pcie.txt         |  10 --
> >  .../bindings/pci/designware-pcie-ecam.txt     |  42 -----
> >  .../bindings/pci/hisilicon-pcie.txt           |   4 +-
> >  .../bindings/pci/host-generic-pci.txt         | 101 ------------
> >  .../bindings/pci/host-generic-pci.yaml        | 150 ++++++++++++++++++
> >  .../bindings/pci/pci-thunder-ecam.txt         |  30 ----
> >  .../bindings/pci/pci-thunder-pem.txt          |   7 +-
> >  .../bindings/pci/plda,xpressrich3-axi.txt     |  12 --
> >  MAINTAINERS                                   |   2 +-
> >  9 files changed, 155 insertions(+), 203 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/pci/arm,juno-r1-pcie.txt
> >  delete mode 100644 Documentation/devicetree/bindings/pci/designware-pcie-ecam.txt
> >  delete mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.txt
> >  create mode 100644 Documentation/devicetree/bindings/pci/host-generic-pci.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/pci/pci-thunder-ecam.txt
> >  delete mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich3-axi.txt
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
> > ...
>
> > +  Interrupt mapping is exactly as described in `Open Firmware Recommended
> > +
>
> I think there's some text missing here.

Removed now. The schemas capture in constraints what the missing text
did in free-form.

> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    description: Depends on the layout of configuration space (CAM vs ECAM
> > +      respectively). May also have more specific compatibles.
> > +    anyOf:
> > +      - description:
> > +          PCIe host controller in Arm Juno based on PLDA XpressRICH3-AXI IP
> > +        items:
> > +          - const: arm,juno-r1-pcie
> > +          - const: plda,xpressrich3-axi
> > +          - const: pci-host-ecam-generic
> > +      - description: |
> > +          ThunderX PCI host controller for pass-1.x silicon
> > +
> > +          Firmware-initialized PCI host controller to on-chip devices found on
> > +          some Cavium ThunderX processors.  These devices have ECAM-based config
> > +          access, but the BARs are all at fixed addresses.  We handle the fixed
> > +          addresses by synthesizing Enhanced Allocation (EA) capabilities for
> > +          these devices.
> > +        const: cavium,pci-host-thunder-ecam
> > +      - description: |
> > +          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, PHYs or device registers, nor
> > +          is there any reason for the driver to reconfigure ATU windows for
> > +          config and/or IO space accesses at runtime.
> > +
> > +          In cases where the IP was synthesized with a minimum ATU window size
> > +          of 64 KB, it cannot be supported by the generic ECAM driver, because
> > +          it requires special config space accessors that filter accesses to
> > +          device #1 and beyond on the first bus.
> > +        items:
> > +          - enum:
> > +              - marvell,armada8k-pcie-ecam
> > +              - socionext,synquacer-pcie-ecam
> > +          - const: snps,dw-pcie-ecam
> > +      - contains:
> > +          enum:
> > +            - pci-host-cam-generic
> > +            - pci-host-ecam-generic
>
> I assume the description that talks about "Synopsys DesignWare" goes
> with "pci-host-cam-generic" and "pci-host-ecam-generic"?

No, it's a catch all for all other cases.

I'll add a description to make the separation more clear. Using
'contains' here was leftover from when I initially kept the same
separate file structure. With it all combined to 1 schema, there's
really no need for that and it should be 'items' list instead. The
difference is we'll fail on 'compatible = "foo,bar-pci",
"pci-host-ecam-generic";' whereas that is valid for 'contains'.

> I hope there
> can be generic controllers using non-Synopsys IP, but I don't know
> quite how the description/items/contains parts are related.

The '-' are important. They separate each entry under the 'anyOf'.

There are a few besides the ones listed with quirks:

arch/arm/boot/dts/alpine.dtsi
arch/arm64/boot/dts/al/alpine-v2.dtsi
arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi
arch/arm64/boot/dts/arm/fvp-base-revc.dts
arch/arm64/boot/dts/cavium/thunder2-99xx.dtsi
arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
arch/xtensa/boot/dts/virt.dts

Some of these might actually be Synopsys. The entry for Synopsys is
only for the not quite compliant configured IP.

Note that 'pci-host-cam-generic' is unused at least by anything upstream.

>
> > +  reg:
> > +    description:
> > +      The Configuration Space base address and size, as accessed from the parent
> > +      bus. The base address corresponds to the first bus in the "bus-range"
> > +      property. If no "bus-range" is specified, this will be bus 0 (the
> > +      default).
> > +    maxItems: 1
> > +
> > +  ranges:
> > +    description:
> > +      As described in IEEE Std 1275-1994, but must provide at least a
> > +      definition of non-prefetchable memory. One or both of prefetchable Memory
> > +      and IO Space may also be provided.
> > +    minItems: 1
> > +    maxItems: 3
> > +
> > +  dma-coherent:
> > +    description: The host controller bridges the AXI transactions into PCIe bus
> > +      in a manner that makes the DMA operations to appear coherent to the CPUs.
>
> The "host-generic-pci.yaml" name sounds very generic, so I'm not quite
> sure how to read "AXI" -- that sounds like a feature of a specific
> platform? I think "dma-coherent" itself is not platform-specific.

Indeed. On second thought, just 'true' here is enough as we don't need
individual bindings to describe common properties over and over.

Rob