Re: [PATCH 3/6] dt-bindings: phy: spacemit: introduce PCIe root complex

[Alex Elder]

On 9/20/25 12:55 AM, Manivannan Sadhasivam wrote:
> On Fri, Sep 19, 2025 at 03:14:05PM -0500, Alex Elder wrote:
> > On 9/15/25 3:14 AM, Manivannan Sadhasivam wrote:
> > > On Wed, Aug 13, 2025 at 01:46:57PM GMT, Alex Elder wrote:
> > >
> > > Subject should have 'pci' prefix, not 'phy'.
> >
> > OK I'll update that in the next version.

I have a new version of this code ready, but I'll delay sending
it until the end of the merge window.

> > > > Add the Device Tree binding for the PCIe root complex found on the
> > > > SpacemiT K1 SoC.  This device is derived from the Synopsys Designware
> > > > PCIe IP.  It supports up to three PCIe ports operating at PCIe gen 2
> > > > link speeds (5 GT/sec).  One of the ports uses a combo PHY, which is
> > > > typically used to support a USB 3 port.
> > > >
> > > > Signed-off-by: Alex Elder <elder@xxxxxxxxxxxx>
> > > > ---
> > > >    .../bindings/pci/spacemit,k1-pcie-rc.yaml     | 141 ++++++++++++++++++
> > > >    1 file changed, 141 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml
> > > > new file mode 100644
> > > > index 0000000000000..6bcca2f91a6fd
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/pci/spacemit,k1-pcie-rc.yaml
> > > > @@ -0,0 +1,141 @@

. . .

> > > > +  interrupts-extended:
> > > > +    maxItems: 1
> > >
> > > What is the purpose of this property? Is it for MSI or INTx?
> >
> > It is for MSIs, which are translated into this interrupt.
> > I'll add a short description indicating this.
> >
> > Is there a better way to represent this?
>
> For external MSI controllers, it is recommended to use 'msi-map' property as the
> client often need to pass RID, RID length and other sideband data to the MSI
> controller.

This implementation is using the DWC built-in MSI controller, not an
external controller.  It implements 256 MSIs, and they're handled by
dw_handle_msi_irq().  The interrupt will be given name "msi" in the
next version and I hope that at least helps clarify things.

> > > > +
> > > > +  spacemit,syscon-pmu:
> > > > +    description:
> > > > +      PHandle that refers to the APMU system controller, whose
> > > > +      regmap is used in managing resets and link state.
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +
> > > > +  device_type:
> > > > +    const: pci
> > > > +
> > > > +  max-link-speed:
> > > > +    const: 2
> > >
> > > Why do you need to limit it to 5 GT/s always?
> >
> > It's what the hardware overview says is the speed
> > of the ports.
> >      PCIE PortA Gen2x1
> >      PCIE PortB Gen2x2
> >      PCIE PortC Gen2x2
> >
> > But I think what you're asking might be "why do you
> > need to specify in DT that the link speed is limited".
> > And in that case, I realize now that it is not needed.
> > I will specify dw_pcie->max_link_speed to 2 before
> > calling dw_pcie_host_init().
>
> You only need to specify this property if you want to limit the link speed to a
> certain data rate by the controller driver than what is supported by the
> hardware.

Yes, this makes sense.

> Looks like your hardware supports 5 GT/s by default for all ports. So you can
> omit this property and also no need to set 'dw_pcie->max_link_speed'. In the
> absence of this property, 'dw_pcie->max_link_speed' will be populated with the
> hardware default value, which will be 2 in your case.

You are correct, and I verified this.  I won't specify the link
speed "manually," and the hardware default (which is 2) will be
used.

> > If that's not what you meant, please let me know.
> >
> > > > +  num-viewport:
> > > > +    const: 8
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - clocks
> > > > +  - clock-names
> > > > +  - resets
> > > > +  - reset-names
> > > > +  - spacemit,syscon-pmu
> > > > +  - "#address-cells"
> > > > +  - "#size-cells"
> > > > +  - device_type
> > > > +  - max-link-speed
> > >
> > > Same comment as above.
> > >
> > > > +  - bus-range
> > > > +  - num-viewport
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/clock/spacemit,k1-syscon.h>
> > > > +    pcie0: pcie@ca000000 {
> > > > +        compatible = "spacemit,k1-pcie-rc";
> > > > +        reg = <0x0 0xca000000 0x0 0x00001000>,
> > > > +              <0x0 0xca300000 0x0 0x0001ff24>,
> > > > +              <0x0 0x8f000000 0x0 0x00002000>,
> > > > +              <0x0 0xc0b20000 0x0 0x00001000>;
> > > > +        reg-names = "dbi",
> > > > +                    "atu",
> > > > +                    "config",
> > > > +                    "link";
> > > > +
> > > > +        ranges = <0x01000000 0x8f002000 0x0 0x8f002000 0x0 0x100000>,
> > >
> > > I/O port CPU address starts from 0.
> >
> > First, I'm not sure what this comment means.
>
> Sorry, I meant to say 'PCI address' not 'CPU address'. The second element in
> your 'ranges' example corresponds to the PCI start address of the I/O port and
> it should always start from 0. Also, the encoding for both tuples are wrong. It
> should be:
>
> 	ranges = <0x01000000 0x0 0x00000000 0x8f002000 0x0 0x100000>,
> 		 <0x02000000 0x0 0x80000000 0x80000000 0x0 0x0f000000>;

Simply switching to what you show works just fine.  PCI
addresses show up being zero-based, as they should be.
What I don't fully understand is why it worked before.
Maybe because this is simply defining bounds in the
mapping?

In any case, I am using ranges properties like what you show.

Thanks Mani.

					-Alex

> - Mani