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

[Chen Wang]

On 2025/1/19 19:44, Manivannan Sadhasivam wrote:
> On Wed, Jan 15, 2025 at 03:06:37PM +0800, Chen Wang wrote:
> > From: Chen Wang <unicorn_wang@xxxxxxxxxxx>
> >
> > Add binding for Sophgo SG2042 PCIe host controller.
> >
> > Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>
> > ---
> >   .../bindings/pci/sophgo,sg2042-pcie-host.yaml | 147 ++++++++++++++++++
> >   1 file changed, 147 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/pci/sophgo,sg2042-pcie-host.yaml

[......]

> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    pcie@62000000 {
> > +      compatible = "sophgo,sg2042-pcie-host";
> > +      device_type = "pci";
> > +      reg = <0x62000000  0x00800000>,
> > +            <0x48000000  0x00001000>;
> Use single space between address and size.
ok, thanks.
> > +      reg-names = "reg", "cfg";
> > +      #address-cells = <3>;
> > +      #size-cells = <2>;
> > +      ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000>,
> > +               <0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
> For sure you don't need to set 'relocatable' flag for both regions.
ok, I will correct this in next version.
> > +      bus-range = <0x00 0xff>;
> > +      vendor-id = <0x1f1c>;
> > +      device-id = <0x2042>;
> As Bjorn explained in v2, these properties need to be moved to PCI root port
> node. Your argument of a single root port node for a host bridge doesn't add as
> we have found that describing the root port properties in host bridge only
> creates issues.

Got it. I will try to change this in next version.

> Btw, we are migrating the existing single RP platforms too to root port node.
>
> > +      cdns,no-bar-match-nbits = <48>;
> > +      sophgo,link-id = <0>;
> > +      sophgo,syscon-pcie-ctrl = <&cdns_pcie1_ctrl>;
> Where is the num-lanes property?
Is this num-lanes a must-have property? The lane number of each link on 
the SG2042 is hard-coded in the firmware, so it seems meaningless to 
configure it.
> > +      msi-parent = <&msi_pcie>;
> > +      msi_pcie: msi {
> 'msi' is not a standard node name. 'interrupt-controller' is what usually used
> to describe the MSI node.
OK. I will corret this.
> Btw, is the MSI controller a separate IP inside the host bridge? If not, there
> would no need to add a separate node. Most of the host bridge IPs implementing
> MSI controller, do not use a separate node.

Yes, it's a separated IP inside the host bridge.

> - Mani

Thanks,

Chen