Re: [PATCH v5 10/14] dt-bindings: PCI: Add CIX Sky1 PCIe Root Complex bindings

[Hans Zhang <hans.zhang@cixtech.com>]

On 2025/6/30 15:26, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On Mon, Jun 30, 2025 at 12:15:57PM +0800, hans.zhang@cixtech.com wrote:
>> From: Hans Zhang <hans.zhang@cixtech.com>
>>
>> Document the bindings for CIX Sky1 PCIe Controller configured in
>> root complex mode with five root port.
>>
>> Supports 4 INTx, MSI and MSI-x interrupts from the ARM GICv3 controller.
>>
>> Signed-off-by: Hans Zhang <hans.zhang@cixtech.com>
>> Reviewed-by: Peter Chen <peter.chen@cixtech.com>
>> Reviewed-by: Manikandan K Pillai <mpillai@cadence.com>
>> ---
>>   .../bindings/pci/cix,sky1-pcie-host.yaml      | 133 ++++++++++++++++++
>>   1 file changed, 133 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.yaml b/Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.yaml
>> new file mode 100644
>> index 000000000000..b4395bc06f2f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.yaml
>> @@ -0,0 +1,133 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/cix,sky1-pcie-host.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: CIX Sky1 PCIe Root Complex
>> +
>> +maintainers:
>> +  - Hans Zhang <hans.zhang@cixtech.com>
>> +
>> +description:
>> +  PCIe root complex controller based on the Cadence PCIe core.
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-host-bridge.yaml#
>> +  - $ref: /schemas/pci/cdns-pcie.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - const: cix,sky1-pcie-host
>> +
>> +  reg:
>> +    items:
>> +      - description: PCIe controller registers.
>> +      - description: Remote CIX System Unit registers.
>> +      - description: ECAM registers.
>> +      - description: Region for sending messages registers.
>> +
>> +  reg-names:
>> +    items:
>> +      - const: reg
>> +      - const: rcsu
>> +      - const: cfg
> 
> cfg is the second, look at cdns bindings.
> 

Dear Krzysztof,

Thank you very much for your reply. Will delete it.

>> +      - const: msg
>> +
>> +  "#interrupt-cells":
>> +    const: 1
>> +
>> +  interrupt-map-mask:
>> +    items:
>> +      - const: 0
>> +      - const: 0
>> +      - const: 0
>> +      - const: 7
>> +
>> +  interrupt-map:
>> +    maxItems: 4
>> +
>> +  max-link-speed:
>> +    maximum: 4
> 
> Why are you redefining core properties?
I see. Just add it in "required". Will delete.

> 
>> +
>> +  num-lanes:
>> +    maximum: 8
>> +
>> +  ranges:
>> +    maxItems: 3
>> +
>> +  msi-map:
>> +    maxItems: 1
>> +
>> +  vendor-id:
>> +    const: 0x1f6c
> 
> Why? This is implied by compatible.

Because when we designed the SOC RTL, it was not set to the vendor id 
and device id of our company. We are members of PCI-SIG. So we need to 
set the vendor id and device id in the Root Port driver. Otherwise, the 
output of lspci will be displayed incorrectly.

> 
>> +
>> +  device-id:
>> +    enum:
>> +      - 0x0001
> 
> Why? This is implied by compatible.

The reason is the same as above.

> 
>> +
>> +  cdns,no-inbound-bar:
> 
> That's not a cdns binding, so wrong prefix.

It will be added to Cadence's Doc. I will add a separate patch. What do 
you think?

> 
>> +    description: |
> 
> Do not need '|' unless you need to preserve formatting.

Will delete '|'.

> 
>> +      Indicates the PCIe controller does not require an inbound BAR region.
> 
> And anyway this is implied by compatible, drop.
> 

Because Cadence core driver has this judgment, the latest code of the 
current linux master all has this process. As follows:
int cdns_pcie_host_init(struct cdns_pcie_rc *rc)
     cdns_pcie_host_init_address_translation(rc);
	cdns_pcie_host_map_dma_ranges(rc);
	   cdns_pcie_host_bar_ib_config

So this attribute has been added here, or is there a better way?

>> +    type: boolean
>> +
>> +  sky1,pcie-ctrl-id:
>> +    description: |
>> +      Specifies the PCIe controller instance identifier (0-4).
> 
> No, you don't get an instance ID. Drop the property and look how other
> bindings encoded it (not sure about the purpose and you did not explain
> it, so cannot advise).
> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 4
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - "#interrupt-cells"
>> +  - interrupt-map-mask
>> +  - interrupt-map
>> +  - max-link-speed
>> +  - num-lanes
>> +  - bus-range
>> +  - device_type
>> +  - ranges
>> +  - msi-map
>> +  - vendor-id
>> +  - device-id
>> +  - cdns,no-inbound-bar
>> +  - sky1,pcie-ctrl-id
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    pcie_x8_rc: pcie@a010000 {
> 
> Drop unused label.

Will delete pcie_x8_rc.

Best regards,
Hans

> 
> 
> Best regards,
> Krzysztof
>