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 19:14, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On 30/06/2025 10:29, Hans Zhang wrote:
>>>> +
>>>> +  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.
> 
> Please read carefully. Previous discussions were also pointlessly
> ping-ponging on irrelevant arguments. Did I suggest you do not have to
> set it in root port driver? No. If this is const here, this is implied
> by compatible and completely redundant, because your driver knows this
> value already. It already has all the information to deduce this value
> from the compatible.
> 
> 
Dear Krzysztof,

Thank you very much for your reply.

These two attributes are also in the following document. Is this place 
out of date?
Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml


We initially used the logic of Cadence common driver as follows:
drivers/pci/controller/cadence/pcie-cadence-host.c
of_property_read_u32(np, "vendor-id", &rc->vendor_id);

of_property_read_u32(np, "device-id", &rc->device_id);

So, can the code in Cadence be deleted?


I see. It will be removed in the next version. The vendor id and device 
id are directly assigned by the Root Port driver based on compatible.

Best regards,
Hans

> 
> 
>>
>>>
>>>> +
>>>> +  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
> 
> And you cannot fix or change drivers? How does it matter for discussion
> here?
> 
>>
>> So this attribute has been added here, or is there a better way?
> 
> Of course, like every other driver in Linux kernel. This is FIXED for
> your platform, so set it in your CIX driver.
> 
> 
> 
> Best regards,
> Krzysztof