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

[Rob Herring <robh@kernel.org>]

On Wed, Dec 11, 2024 at 01:20:14PM -0600, Bjorn Helgaas wrote:
> [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@62000000 {
> > >      compatible = "sophgo,sg2042-pcie-host";
> > >      port0: pci@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@7060000000 {
> >     compatible = "sophgo,sg2042-pcie-host";
> >     ...... // other properties
> >     pci@0,0 {
> >       vendor-id = <0x1f1c>;
> >       device-id = <0x2042>;
> >     };
> > }
> > 
> > pcie@7062000000 {
> >     compatible = "sophgo,sg2042-pcie-host";
> >     ...... // other properties
> >     pci@0,0 {
> >       vendor-id = <0x1f1c>;
> >       device-id = <0x2042>;
> >     };
> > }
> > 
> > pcie@7062800000 {
> >     compatible = "sophgo,sg2042-pcie-host";
> >     ...... // other properties
> >     pci@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.

Agreed.


> 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@3000 {
>      compatible = "nvidia,tegra30-pcie";
>      pci@1,0 {
>        reg = <0x000800 0 0 0 0>;
>      };
> 
> where the "0x000800 0 0 0 0" means the "pci@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.

I don't think device 1 would be the correct address. The RP is almost 
always device 0.

I think instead the 'syscon-pcie-ctrl' should perhaps be modeled as a 
phy with the phy binding. Then the host bridge node can have 1 or 2 phy 
entries depending on if the host uses 1 or 2 links. And the 2nd host 
should have 'status = "disabled";' when it is not used.

Or perhaps just 'num-lanes' would be enough.

Rob