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
+ - 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
+
+ num-lanes:
+ maximum: 8
+
+ ranges:
+ maxItems: 3
+
+ msi-map:
+ maxItems: 1
+
+ vendor-id:
+ const: 0x1f6c
+
+ device-id:
+ enum:
+ - 0x0001
+
+ cdns,no-inbound-bar:
+ description: |
+ Indicates the PCIe controller does not require an inbound BAR region.
+ type: boolean
+
+ sky1,pcie-ctrl-id:
+ description: |
+ Specifies the PCIe controller instance identifier (0-4).
+ $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 {
+ compatible = "cix,sky1-pcie-host";
+ reg = <0x00 0x0a010000 0x00 0x10000>,
+ <0x00 0x0a000000 0x00 0x10000>,
+ <0x00 0x2c000000 0x00 0x4000000>,
+ <0x00 0x60000000 0x00 0x00100000>;
+ reg-names = "reg", "rcsu", "cfg", "msg";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 &gic 0 0 GIC_SPI 407 IRQ_TYPE_LEVEL_HIGH 0>,
+ <0 0 0 2 &gic 0 0 GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH 0>,
+ <0 0 0 3 &gic 0 0 GIC_SPI 409 IRQ_TYPE_LEVEL_HIGH 0>,
+ <0 0 0 4 &gic 0 0 GIC_SPI 410 IRQ_TYPE_LEVEL_HIGH 0>;
+ max-link-speed = <4>;
+ num-lanes = <8>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ bus-range = <0xc0 0xff>;
+ device_type = "pci";
+ ranges = <0x01000000 0x00 0x60100000 0x00 0x60100000 0x00 0x00100000>,
+ <0x02000000 0x00 0x60200000 0x00 0x60200000 0x00 0x1fe00000>,
+ <0x43000000 0x18 0x00000000 0x18 0x00000000 0x04 0x00000000>;
+ msi-map = <0xc000 &gic_its 0xc000 0x4000>;
+ vendor-id = <0x1f6c>;
+ device-id = <0x0001>;
+ sky1,pcie-ctrl-id = <0x0>;
+ cdns,no-inbound-bar;
+ };
--
2.49.0On 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.
> + - 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?
> +
> + num-lanes:
> + maximum: 8
> +
> + ranges:
> + maxItems: 3
> +
> + msi-map:
> + maxItems: 1
> +
> + vendor-id:
> + const: 0x1f6c
Why? This is implied by compatible.
> +
> + device-id:
> + enum:
> + - 0x0001
Why? This is implied by compatible.
> +
> + cdns,no-inbound-bar:
That's not a cdns binding, so wrong prefix.
> + description: |
Do not need '|' unless you need to preserve formatting.
> + Indicates the PCIe controller does not require an inbound BAR region.
And anyway this is implied by compatible, drop.
> + 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.
Best regards,
Krzysztof
On 2025/6/30 15:26, Krzysztof Kozlowski wrote:
>> + 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).
Dear Krzysztof,
Sorry, I missed your reply to this in the previous email.
Because our Root Port driver needs to support 5 PCIe ports, and the
register configuration and offset of each port are different, it is
necessary to know which port it is currently. Perhaps I can use the
following method and then delete this attribute.
aliases {
......
pcie_rc0 = &pcie_x8_rc;
pcie_rc1 = &pcie_x4_rc;
pcie_rc2 = &pcie_x2_rc;
pcie_rc3 = &pcie_x1_0_rc;
pcie_rc4 = &pcie_x1_1_rc;
id = of_alias_get_id(dev->of_node, "pcie_rc");
Best regards,
Hans
On 30/06/2025 17:54, Hans Zhang wrote:
>
>
> On 2025/6/30 15:26, Krzysztof Kozlowski wrote:
>>> + 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).
>
>
> Dear Krzysztof,
>
> Sorry, I missed your reply to this in the previous email.
>
> Because our Root Port driver needs to support 5 PCIe ports, and the
> register configuration and offset of each port are different, it is
> necessary to know which port it is currently. Perhaps I can use the
> following method and then delete this attribute.
>
> aliases {
> ......
> pcie_rc0 = &pcie_x8_rc;
> pcie_rc1 = &pcie_x4_rc;
> pcie_rc2 = &pcie_x2_rc;
> pcie_rc3 = &pcie_x1_0_rc;
> pcie_rc4 = &pcie_x1_1_rc;
>
> id = of_alias_get_id(dev->of_node, "pcie_rc");
I think Rob commented pretty strongly about aliases in this thread... or
was it other one? Maybe it was about Tesla FSD PCI PHY... huh, same pattern.
So no, you do not get your own aliases.
Explain the differences in the hardware. If the hardware is different,
then it warrants different compatibles or other properties. But you need
to explain these differences. What is there? Different number of lanes?
Different phy? We have properties for that, use these. Different speed?
All of them have their own properties already, so use them. Maybe
something else... Do the homework and look at schemas and dtschema (yes,
I know that I said other poor solutions are not excuse to copy them, but
look for good solutions).
Best regards,
Krzysztof
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
>
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. > >> >>> + >>> + 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
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
On 30/06/2025 17:30, Hans Zhang wrote: > > > 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 I would need to spend time to investigate that and I choose to do other things instead. I am recently very grumpy on arguments "I found this somewhere else". I found bugs somewhere else, so am I okay to introduce them? > > > 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? Don't know. If this is ABI, then not. Best regards, Krzysztof
On 2025/7/3 04:23, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL > > On 30/06/2025 17:30, Hans Zhang wrote: >> >> >> 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 > > I would need to spend time to investigate that and I choose to do other > things instead. I am recently very grumpy on arguments "I found this > somewhere else". I found bugs somewhere else, so am I okay to introduce > them? > Dear Krzysztof, Thank you very much for your reply. No, no, no. You misunderstood me. I didn't mean to say this because we don't study dt-binding doc every day. So we can only refer to the practices of other SOC manufacturers. If it's incorrect, we will definitely listen to your opinion. Here, I'm just explaining the origin of what I did. Anyway, I have solved this problem by following your method and using compatible. >> >> >> 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? > > Don't know. If this is ABI, then not. > According to my understanding, this is not ABI. Best regards, Hans > > Best regards, > Krzysztof
On 03/07/2025 03:47, Hans Zhang wrote: >>> >>> 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? >> >> Don't know. If this is ABI, then not. >> > > According to my understanding, this is not ABI. Huh? Then what is ABI, by your understanding? Best regards, Krzysztof
On 2025/7/14 15:43, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL > > On 03/07/2025 03:47, Hans Zhang wrote: >>>> >>>> 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? >>> >>> Don't know. If this is ABI, then not. >>> >> >> According to my understanding, this is not ABI. > > Huh? Then what is ABI, by your understanding? > Dear Krzysztof, I understand kernel ABI primarily refers to the stable binary contract between the kernel and userspace (e.g., syscalls, /sys/proc interfaces). Device tree properties are part of the boot-time hardware description consumed by drivers during initialization. They are not directly exposed to userspace as ABI interfaces. If I understand wrongly, please correct me. It was about half a year ago when I submitted the patch that could view LTSSM link status in dwc that I learned about the ABI. There are not many studies on this. https://lore.kernel.org/linux-pci/20250123164944.GA1223935@bhelgaas/ Best regards, Hans
On 14/07/2025 10:03, Hans Zhang wrote: > > > On 2025/7/14 15:43, Krzysztof Kozlowski wrote: >> EXTERNAL EMAIL >> >> On 03/07/2025 03:47, Hans Zhang wrote: >>>>> >>>>> 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? >>>> >>>> Don't know. If this is ABI, then not. >>>> >>> >>> According to my understanding, this is not ABI. >> >> Huh? Then what is ABI, by your understanding? >> > > Dear Krzysztof, > > I understand kernel ABI primarily refers to the stable binary contract > between the kernel and userspace (e.g., syscalls, /sys/proc interfaces). > Device tree properties are part of the boot-time hardware description > consumed by drivers during initialization. They are not directly exposed > to userspace as ABI interfaces. > > If I understand wrongly, please correct me. Then that's wrong understanding. The DT interface, documented explicitly and one implied by kernel drivers in case it differs, is the ABI, as explained in docs in the kernel and what we said on the lists thousands of times. Best regards, Krzysztof
On 2025/7/15 14:40, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL > > On 14/07/2025 10:03, Hans Zhang wrote: >> >> >> On 2025/7/14 15:43, Krzysztof Kozlowski wrote: >>> EXTERNAL EMAIL >>> >>> On 03/07/2025 03:47, Hans Zhang wrote: >>>>>> >>>>>> 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? >>>>> >>>>> Don't know. If this is ABI, then not. >>>>> >>>> >>>> According to my understanding, this is not ABI. >>> >>> Huh? Then what is ABI, by your understanding? >>> >> >> Dear Krzysztof, >> >> I understand kernel ABI primarily refers to the stable binary contract >> between the kernel and userspace (e.g., syscalls, /sys/proc interfaces). >> Device tree properties are part of the boot-time hardware description >> consumed by drivers during initialization. They are not directly exposed >> to userspace as ABI interfaces. >> >> If I understand wrongly, please correct me. > > > Then that's wrong understanding. > > The DT interface, documented explicitly and one implied by kernel > drivers in case it differs, is the ABI, as explained in docs in the > kernel and what we said on the lists thousands of times. > Dear Krzysztof, Thank you very much for your reply and explanation. Now I understand that I have been discussing issues in the linux community for about half a year. I didn't pay attention to it before. Thank you again. Best regards, Hans
On Mon, 30 Jun 2025 12:15:57 +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
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.yaml: properties:compatible:oneOf: [{'const': 'cix,sky1-pcie-host'}] should not be valid under {'items': {'propertyNames': {'const': 'const'}, 'required': ['const']}}
hint: Use 'enum' rather than 'oneOf' + 'const' entries
from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
Error: Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.example.dts:29.47-48 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1525: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250630041601.399921-11-hans.zhang@cixtech.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
On 2025/6/30 13:36, Rob Herring (Arm) wrote:
> [Some people who received this message don't often get email from robh@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL
>
> On Mon, 30 Jun 2025 12:15:57 +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
>>
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.yaml: properties:compatible:oneOf: [{'const': 'cix,sky1-pcie-host'}] should not be valid under {'items': {'propertyNames': {'const': 'const'}, 'required': ['const']}}
> hint: Use 'enum' rather than 'oneOf' + 'const' entries
> from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
> Error: Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.example.dts:29.47-48 syntax error
> FATAL ERROR: Unable to parse input tree
> make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/pci/cix,sky1-pcie-host.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1525: dt_binding_check] Error 2
> make: *** [Makefile:248: __sub-make] Error 2
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250630041601.399921-11-hans.zhang@cixtech.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>
Dear Rob,
Thank you very much for your reply and reminder. The next version will fix.
The modification is as follows: make dt_binding_check. There are no errors.
s/sky1,pcie-ctrl-id/cix,pcie-ctrl-id/
Best regards,
Hans
© 2016 - 2025 Red Hat, Inc.