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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2b608302-c4a6-404d-9cc5-d1ab9a6712bd@cixtech.com>
Date: Mon, 30 Jun 2025 23:30:54 +0800
From: Hans Zhang <hans.zhang@...tech.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
 mani@...nel.org, robh@...nel.org, kwilczynski@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, mpillai@...ence.com,
 fugang.duan@...tech.com, guoyin.chen@...tech.com, peter.chen@...tech.com,
 cix-kernel-upstream@...tech.com, linux-pci@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 10/14] dt-bindings: PCI: Add CIX Sky1 PCIe Root Complex
 bindings



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