Re: [PATCH 1/7] dt-bindings: pci: cadence: Extend compatible for new platform configurations

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/03/2025 06:07, Manikandan Karunakaran Pillai wrote:
>> EXTERNAL MAIL
>>
>>
>> On 27/03/2025 12:19, Manikandan Karunakaran Pillai wrote:
>>> Document the compatible property for the newly added values for PCIe EP
>> and
>>> RP configurations. Fix the compilation issues that came up for the existing
>>> Cadence bindings
>>
>> These are two different commits.
> 
> Ok
> 
>>
>>>
>>> Signed-off-by: Manikandan K Pillai <mpillai@cadence.com>
>>> ---
>>>  .../bindings/pci/cdns,cdns-pcie-ep.yaml       |  12 +-
>>>  .../bindings/pci/cdns,cdns-pcie-host.yaml     | 119 +++++++++++++++---
>>>  2 files changed, 110 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
>> b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
>>> index 98651ab22103..aa4ad69a9b71 100644
>>> --- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-ep.yaml
>>> @@ -7,14 +7,22 @@ $schema:
>> https://urldefense.com/v3/__http://devicetree.org/meta-
>> schemas/core.yaml*__;Iw!!EHscmS1ygiU1lA!CB5lvkvRUKSEDPSjpW7GJoPNyXZ
>> xMge5SyndD4Z-VVLCZvzLIPDP-BMRjhKZ2UTxi6a18vaodaU$
>>>  title: Cadence PCIe EP Controller
>>>
>>>  maintainers:
>>> -  - Tom Joseph <tjoseph@cadence.com>
>>> +  - Manikandan K Pillai <mpillai@cadence.com>
>>>
>>>  allOf:
>>>    - $ref: cdns-pcie-ep.yaml#
>>>
>>>  properties:
>>>    compatible:
>>> -    const: cdns,cdns-pcie-ep
>>> +    oneOf:
>>> +      - const: cdns,cdns-pcie-ep
>>> +      - const: cdns,cdns-pcie-hpa-ep
>>
>> What is hpa? Which soc is that?
>>
>> I don't think this should keep growing, but instead use SoC based
>> compatibles.
>>
>> Anyway, that's enum.
>>
> 
> HPA is high performance architecture based controllers. The major difference here in PCIe controllers is that
> the address map changes. Each of the compatibles defined here have different address maps that allow the driver
> to support them from the driver using compable property that provides the info from related data "struct of_device_id" in the driver.

Just switch to SoC specific compatibles.

> 
>>> +      - const: cdns,cdns-cix-pcie-hpa-ep
>>
>> What is cix? If you want to stuff here soc in the middle, then no, no
>> no. Please read devicetree spec and writing bindings how the compatibles
>> are created.
>>
> 
> As mentioned in the earlier sections, cix is another implementation of the PCIe controller where 
> the address map is changed by our customer

So a SoC. Use SoC compatibles and follow every other recent binding.

> 
>>> +      - description: PCIe EP controller from cadence
>>> +        items:
>>> +          - const: cdns,cdns-pcie-ep
>>> +          - const: cdns,cdns-pcie-hpa-ep
>>> +          - const: cdns,cdns-cix-pcie-hpa-ep
>>
>> This makes no sense.
>>
> Only one of the above compatible is valid for PCIe controllers, which will be defined in the SoC related binding.

That's not how lists are working. Don't explain me what it does, because
I know that it does nothing good: it's broken code. You can explain me
what you wanted to achieve, but still this part is just wrong and makes
no sense. Drop.




> 
>>>
>>>    reg:
>>> diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>> b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>> index a8190d9b100f..bb7ffb9ddaf9 100644
>>> --- a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.yaml
>>>
>>>  maintainers:
>>> -  - Tom Joseph <tjoseph@cadence.com>
>>> +  - Manikandan K Pillai <mpillai@cadence.com>
>>>
>>>  allOf:
>>> -  - $ref: cdns-pcie-host.yaml#
>>> +  - $ref: cdns-pcie.yaml#
>>
>> Why?
>>
> 
> The existing yaml files were throwing out errors and the changes in these files are for fixing them.

Then rather investigate the errors instead of doing random changes.

> 
>>>
>>>  properties:
>>> +  "#size-cells":
>>> +    const: 2
>>> +  "#address-cells":
>>> +    const: 3
>>
>> Huh? Why? Nothing here makes sense.
>>
>>
> Compilation error related fixes.

NAK, no point at all.

Best regards,
Krzysztof