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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/07/2024 01:19, Mayank Rana wrote:
> 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@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.

Your binding already defines some specifics for specific device, but you
still claim all of them are identical.

Sorry, I am confused. Read carefully writing bindings, consult internal
guideline (go/upstream - it is really detailed!) and then come with a
solution.

Best regards,
Krzysztof