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

[Mayank Rana]

Hi Krzysztof

On 7/17/2024 11:05 PM, Krzysztof Kozlowski wrote:
> On 17/07/2024 19:20, Mayank Rana wrote:
>> Hi Krzysztof
>>
>> On 7/16/2024 11:47 PM, Krzysztof Kozlowski wrote:
>>> 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 at 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.
>> Ok. let me not make generic comment here. I refereed how it is done with
>> other
>> snps based IP usage for example USB, and would follow same.
> 
> Well, it is not. If you read their bindings or any reviews related to
> such cores, you would see that single Designware compatible is always
> never appropriate. Such cores always have customization per user.
> 
> You can also look at the binding you are changing. Do you see Designware
> alone? No.
I found reference in this binding as below:
  79         items:
  80           - enum:
  81               - marvell,armada8k-pcie-ecam
  82               - socionext,synquacer-pcie-ecam
  83           - const: snps,dw-pcie-ecam

And as you mentioned in previous emails about how to add such usage, I 
ACKed it but let me put reason why I tried to add differently to start 
with it.

This specific driver under-discussion is really not vendor specific 
driver. It can work with any PCIe controller which is already configured 
in ECAM mode by firmware (i.e. PCIe controller from SNPS or any other 
vendor). There are few quirks only added to get specific vendor based 
SOC configuration for SNPS PCIe controller by different SOC vendors.

  90       - description:
  91           CAM or ECAM compliant PCI host controllers without any quirks
  92         enum:
  93           - pci-host-cam-generic
  94           - pci-host-ecam-generic

Above enum based usage works for SA8775P platform which is having SNPS 
PCIe controller which doesn't need any quirks, and firmware is able to 
configure PCIe controller into ECAM mode. Here I tried adding PCIe SNPS 
controller based MSI support as SA8775P doesn't support LPI/ITS for MSI. 
I need to differentiate it hence added generic enum as MSI controller is 
part of SNPS PCIe controller, and not separate MSI IP here. Although how 
many MSI can be supported it depends on how many SPIs are available/used 
with MSI controller depend on particular SOC. Hence put as 
snps,dw-pcie-ecam-msi as usage and variable number of MSI. hopefully I 
am able to convey why this driver binding modified differently.

Regards,
Mayank