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

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Dec 09, 2024 at 03:19:38PM +0800, Chen Wang wrote:
> Add binding for Sophgo SG2042 PCIe host controller.

> +  sophgo,pcie-port:

This is just an index, isn't it?  I don't see why it should include
"sophgo" unless it encodes something sophgo-specific.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      SG2042 uses Cadence IP, every IP is composed of 2 cores(called link0

Add space before "(".  More instances below.

> +      & link1 as Cadence's term). "sophgo,pcie-port" is used to identify which
> +      core/link the pcie host controller node corresponds to.

s/pcie/PCIe/ for consistency in the text.  More instances below.

> +      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.

I assume this means there are two separate Root Ports, one for Link0
and a second for Link1?

> +      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.

An RC doesn't have a Link.  A Root Port does.

> +      "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.
> +
> +  sophgo,syscon-pcie-ctrl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the PCIe System Controller DT node. It's required to
> +      access some MSI operation registers shared by PCIe RCs.

I think this probably means "shared by PCIe Root Ports", not RCs.
It's unlikely that this hardware has multiple Root Complexes.

> +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>;
    };

> +additionalProperties: true
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    pcie@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";
> +      };
> +    };
> -- 
> 2.34.1
>