[PATCH v2 1/5] dt-bindings: pci: Add Sophgo SG2042 PCIe host

[Bjorn Helgaas]

[cc->to: Rob, Krzysztof, Conor because I'm not a DT expert and I'd
like their thoughts on this idea of describing Root Ports as separate
children]

On Wed, Dec 11, 2024 at 05:00:44PM +0800, Chen Wang wrote:
> On 2024/12/11 1:33, Bjorn Helgaas wrote:
> > On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote:

> > > +      The Cadence IP has two modes of operation, selected by a strap pin.
> > > +
> > > +      In the single-link mode, the Cadence PCIe core instance associated
> > > +      with Link0 is connected to all the lanes and the Cadence PCIe core
> > > +      instance associated with Link1 is inactive.
> > > +
> > > +      In the dual-link mode, the Cadence PCIe core instance associated
> > > +      with Link0 is connected to the lower half of the lanes and the
> > > +      Cadence PCIe core instance associated with Link1 is connected to
> > > +      the upper half of the lanes.

> > > +      SG2042 contains 2 Cadence IPs and configures the Cores as below:
> > > +
> > > +                     +-- Core(Link0) <---> pcie_rc0   +-----------------+
> > > +                     |                                |                 |
> > > +      Cadence IP 1 --+                                | cdns_pcie0_ctrl |
> > > +                     |                                |                 |
> > > +                     +-- Core(Link1) <---> disabled   +-----------------+
> > > +
> > > +                     +-- Core(Link0) <---> pcie_rc1   +-----------------+
> > > +                     |                                |                 |
> > > +      Cadence IP 2 --+                                | cdns_pcie1_ctrl |
> > > +                     |                                |                 |
> > > +                     +-- Core(Link1) <---> pcie_rc2   +-----------------+
> > > +
> > > +      pcie_rcX is pcie node ("sophgo,sg2042-pcie-host") defined in DTS.
> > > +      cdns_pcie0_ctrl is syscon node ("sophgo,sg2042-pcie-ctrl") defined in DTS
> > > +
> > > +      cdns_pcieX_ctrl contains some registers shared by pcie_rcX, even two
> > > +      RC(Link)s may share different bits of the same register. For example,
> > > +      cdns_pcie1_ctrl contains registers shared by link0 & link1 for Cadence IP 2.

> > > +      "sophgo,pcie-port" is defined to flag which core(link) the rc maps to, with
> > > +      this we can know what registers(bits) we should use.

> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - reg-names
> > > +  - vendor-id
> > > +  - device-id
> > > +  - sophgo,syscon-pcie-ctrl
> > > +  - sophgo,pcie-port
> >
> > It looks like vendor-id and device-id apply to PCI devices, i.e.,
> > things that will show up in lspci, I assume Root Ports in this case.
> > Can we make this explicit in the DT, e.g., something like this?
> > 
> >    pcie at 62000000 {
> >      compatible = "sophgo,sg2042-pcie-host";
> >      port0: pci at 0,0 {
> >        vendor-id = <0x1f1c>;
> >        device-id = <0x2042>;
> >      };
> 
> Sorry, I don't understand your meaning very well.  Referring to the topology
> diagram I drew above, is it okay to write DTS as follows?
> 
> pcie at 7060000000 {
>     compatible = "sophgo,sg2042-pcie-host";
>     ...... // other properties
>     pci at 0,0 {
>       vendor-id = <0x1f1c>;
>       device-id = <0x2042>;
>     };
> }
> 
> pcie at 7062000000 {
>     compatible = "sophgo,sg2042-pcie-host";
>     ...... // other properties
>     pci at 0,0 {
>       vendor-id = <0x1f1c>;
>       device-id = <0x2042>;
>     };
> }
> 
> pcie at 7062800000 {
>     compatible = "sophgo,sg2042-pcie-host";
>     ...... // other properties
>     pci at 1,0 {
>       vendor-id = <0x1f1c>;
>       device-id = <0x2042>;
>     };
> 
> }

Generally makes sense to me.  I'm suggesting that we should start
describing Root Ports as children of the host bridge node instead of 
mixing their properties into the host bridge itself.

Some properties apply to the host bridge, e.g., "bus-range" describes
the bus number aperture, and "ranges" describes the address
translation between the upstream CPU address space and the PCI address
space.

Others apply specifically to a Root Port, e.g., "num-lanes",
"max-link-speed", "phys", "vendor-id", "device-id".  I think it will
help if we can describe these in separate children, especially when
there are multiple Root Ports.

Documentation/devicetree/bindings/pci/pci.txt says a Root Port
should include a reg property that contains the bus/device/function
number of the RP, e.g.,
Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt has
this:

  pcie-controller at 3000 {
     compatible = "nvidia,tegra30-pcie";
     pci at 1,0 {
       reg = <0x000800 0 0 0 0>;
     };

where the "0x000800 0 0 0 0" means the "pci at 1,0" Root Port is at
00:01.0 (bus 00, device 01, function 0).  I don't know what the "@1,0"
part means.

> And with this change, I can drop the “pcie-port”property and use the port
> name to figure out the port number, right?

Seems likely to me.

> > > +additionalProperties: true
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    pcie at 62000000 {
> > > +      compatible = "sophgo,sg2042-pcie-host";
> > > +      device_type = "pci";
> > > +      reg = <0x62000000  0x00800000>,
> > > +            <0x48000000  0x00001000>;
> > > +      reg-names = "reg", "cfg";
> > > +      #address-cells = <3>;
> > > +      #size-cells = <2>;
> > > +      ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>,
> > > +               <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
> > > +      bus-range = <0x80 0xbf>;
> > > +      vendor-id = <0x1f1c>;
> > > +      device-id = <0x2042>;
> > > +      cdns,no-bar-match-nbits = <48>;
> > > +      sophgo,pcie-port = <0>;
> > > +      sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
> > > +      msi-parent = <&msi_pcie>;
> > > +      msi_pcie: msi {
> > > +        compatible = "sophgo,sg2042-pcie-msi";
> > > +        msi-controller;
> > > +        interrupt-parent = <&intc>;
> > > +        interrupts = <123 IRQ_TYPE_LEVEL_HIGH>;
> > > +        interrupt-names = "msi";
> > > +      };
> > > +    };