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

[Chen Wang <unicorn_wang@outlook.com>]

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@outlook.com>
>>
>> Add binding for Sophgo SG2042 PCIe host controller.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   .../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