Re: [PATCH 1/2] dt-bindings: misc: introduce pci1179,0220.yaml

On 8/13/26 8:45 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Low] The example DT nodes map multiple PCI BARs to the exact same parent physical address, and define unused BARs that contradict the hardware diagram.
> - [Low] Typographical error in the compatible string comment.
> --
> 
> --- Patch [1]: [PATCH 1/2] dt-bindings: misc: introduce pci1179,0220.yaml ---
> commit ee5f64080011b047dd81e1bbdc568f784c09b77f
> Author: Alex Elder <[email protected]>
> 
> dt-bindings: misc: introduce pci1179,0220.yaml
> 
> Define the binding for the Toshiba TC9564 PCI endpoint function device.
> The third downstream PCIe switch port within this chip has an embedded
> PCIe controller, and that implements two of these PCIe functions.
> 
>> diff --git a/Documentation/devicetree/bindings/misc/pci1179,0220.yaml b/Documentation/devicetree/bindings/misc/pci1179,0220.yaml
>> new file mode 100644
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/pci1179,0220.yaml
> [ ... ]
>> +properties:
>> +  compatible:
>> +    const: pci1179,0220 # Toshiba TC96564 (a.k.a. Qualcomm QPS615)
> 
> [Severity: Low]
> This isn't a bug, but there's a typo in this comment. It says TC96564 instead
> of TC9564.

I will fix this in the next version.

> [ ... ]
>> +        pci@0,0 {
>> +            compatible = "pci1179,0220";
>> +            reg = <0x0 0x0 0x0 0x0 0x0>;
>> +            #address-cells = <3>;
>> +            #size-cells = <2>;
>> +            /* Ranges will be updated dynamically */
>> +            ranges = <0x0 0x0 0x0  0x83000000 0x0 0x0  0x0 0x4000>,
>> +                     <0x2 0x0 0x0  0x83000000 0x0 0x0  0x0 0x80000>,
>> +                     <0x4 0x0 0x0  0x83000000 0x0 0x0  0x0 0x200000>;
> 
> [Severity: Low]
> Are these example ranges accurate for the hardware? The mappings configure
> BAR 0, BAR 2, and BAR 4 to use the identical parent physical address
> (0x83000000), which would result in overlapping memory regions.

Yes, and I could put something else in there, but this is related
to another issue about the dynamic nature of assigned BAR base
addresses.  I sent out another series to propose a fix for that.

   https://lore.kernel.org/lkml/[email protected]/

I'm interested to know if anyone has another suggestion for how
best to represent this.

> Additionally, the hardware diagram in the binding description indicates
> BAR 2 is not present, but it is configured in this example.

I find it very impressive that Sashiko interprets the ASCII
art diagram this well...

The reality is that--at least when the ranges property is dynamically
created--all BARs will have a range value defined in the ranges
property, even if one or more is not used.

I chose this way of specifying this so that the relationship
between the endpoint ranges property and the pci-ep-bus ranges
properties were clear (indicating the BAR number in the first
cell).

The remaining questions below have the same basic answers as I
have already provided.

					-Alex

> 
> [ ... ]
>> +        pci@0,1 {
>> +            compatible = "pci1179,0220";
>> +            reg = <0x100 0x0 0x0 0x0 0x0>;
>> +            #address-cells = <3>;
>> +            #size-cells = <2>;
>> +            /* Ranges will be updated dynamically */
>> +            ranges = <0x0 0x0 0x0  0x83000100 0x0 0x0  0x0 0x4000>,
>> +                     <0x2 0x0 0x0  0x83000100 0x0 0x0  0x0 0x80000>,
>> +                     <0x4 0x0 0x0  0x83000100 0x0 0x0  0x0 0x200000>;
> 
> [Severity: Low]
> Do these ranges match the hardware design for Function 1? The example maps
> BAR 0 and BAR 2 here, but the hardware diagram indicates Function 1 only
> uses BAR 4.
> 
> These mappings also map to the same parent physical address (0x83000100),
> causing overlapping memory regions similar to the previous node.
>