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

[Hans Zhang]

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