Re: [PATCH V226/7] dt-bindings: PCI: host-generic-pci: Add snps,dw-pcie-ecam-msi binding

[Krzysztof Kozlowski <krzk@kernel.org>]

On 17/07/2024 00:09, Mayank Rana wrote:
> Hi Krzysztof
> 
> On 7/16/2024 12:28 AM, Krzysztof Kozlowski wrote:
>> On 15/07/2024 20:13, Mayank Rana wrote:
>>> To support MSI functionality using Synopsys DesignWare PCIe controller
>>> based MSI controller with ECAM driver, add "snps,dw-pcie-ecam-msi
>>> compatible binding which uses provided SPIs to support MSI functionality.
>>
>> To support MSI, you add MSI support... That's a tautology. Describe
>> hardware instead.
> Ok. let me repharse it to provide more useful information.
>>>
>>> Signed-off-by: Mayank Rana <quic_mrana@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/pci/host-generic-pci.yaml  | 57 ++++++++++++++++++++++
>>>   1 file changed, 57 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
>>> index 9c714fa..9e860d5 100644
>>> --- a/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.yaml
>>> @@ -81,6 +81,12 @@ properties:
>>>                 - marvell,armada8k-pcie-ecam
>>>                 - socionext,synquacer-pcie-ecam
>>>             - const: snps,dw-pcie-ecam
>>> +      - description: |
>>> +         Firmware is configuring Synopsys DesignWare PCIe controller in RC mode with
>>> +         ECAM compatible fashion. To use MSI controller of Synopsys DesignWare PCIe
>>> +         controller for MSI functionality, this compatible is used.
>>> +        items:
>>> +          - const: snps,dw-pcie-ecam-msi
>>
>> MSI is already present in the binding, isn't it? 
> It is mentioning as msi-parent usage which could be different MSI 
> controller (GIC or vendor specific one) not related to designware PCIe 
> controller based MSI controller.
> 
>> Anyway, aren't you
>> forgetting specific compatible? Please open your internal (quite
>> comprehensive) guideline on bindings and DTS.
> Here I am trying to define Designware based PCIe ECAM controller 
> supporting MSIcontroller based device. Hence I am not mentioning vendor 
> specific compatible usage
> and keeping generic compatible binding for such device.

I know what you try, yet it feels simply wrong. Read your guideline.
Are you sure you work on Designware core itself, not on one used in
Qualcomm? I would expect people from Designware to design Designware
cores and people from Qualcomm only to design licensed cores.

>>
>>>         - description:
>>>             CAM or ECAM compliant PCI host controllers without any quirks
>>>           enum:
>>> @@ -116,6 +122,20 @@ properties:
>>>         A phandle to the node that controls power or/and system resource or interface to firmware
>>>         to enable ECAM compliant PCIe root complex.
>>>   
>>> +  interrupts:
>>> +    description:
>>> +      DWC PCIe Root Port/Complex specific MSI interrupt/IRQs.
>>> +    minItems: 1
>>> +    maxItems: 8
>>> +
>>> +  interrupt-names:
>>> +    description:
>>> +      MSI interrupt names
>>> +    minItems: 1
>>> +    maxItems: 8
>>> +    items:
>>> +        pattern: '^msi[0-9]+$'
>>
>> Why the same devices have variable numbers?
> Max supported MSI with designware PCIe controller is 8 Only, and it 
> depends if those all are
> used or some of used on specific vendor based device. Hence I have kept 
> it here variable names. Although here it should be [0 - 7] instead of
> [0 - 9].

Wait, you just said there is no specific vendor device.

Sorry, bring some sanity to this.

Best regards,
Krzysztof