diff mbox series

[v2,15/21] dt-bindings: i2c: document support for SA8255p

Message ID 20240903220240.2594102-16-quic_nkela@quicinc.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series [v2,01/21] dt-bindings: arm: qcom: add the SoC ID for SA8255P | expand

Commit Message

Nikunj Kela Sept. 3, 2024, 10:02 p.m. UTC
Add compatible representing i2c support on SA8255p.

Clocks and interconnects are being configured in Firmware VM
on SA8255p, therefore making them optional.

CC: Praveen Talari <quic_ptalari@quicinc.com>
Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
---
 .../bindings/i2c/qcom,i2c-geni-qcom.yaml      | 33 +++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski Sept. 4, 2024, 6:31 a.m. UTC | #1
On Tue, Sep 03, 2024 at 03:02:34PM -0700, Nikunj Kela wrote:
> Add compatible representing i2c support on SA8255p.
> 
> Clocks and interconnects are being configured in Firmware VM
> on SA8255p, therefore making them optional.
> 
> CC: Praveen Talari <quic_ptalari@quicinc.com>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
>  .../bindings/i2c/qcom,i2c-geni-qcom.yaml      | 33 +++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 

I don't know what to do with this patch. Using specific compatibles next
to generic compatible is just wrong, although mistake was probably
allowing generic compatible. The patch does not explain the differences
in interface which would explain why devices are not compatible. In the
same time my advice of separate binding was not followed, because maybe
these devices are compatible? But then it should be expressed...

You have entire commit msg to explain what and why.

> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> index 9f66a3bb1f80..b477fae734b6 100644
> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
> @@ -15,6 +15,7 @@ properties:
>      enum:
>        - qcom,geni-i2c
>        - qcom,geni-i2c-master-hub
> +      - qcom,sa8255p-geni-i2c
>  
>    clocks:
>      minItems: 1
> @@ -69,8 +70,6 @@ properties:
>  required:
>    - compatible
>    - interrupts
> -  - clocks
> -  - clock-names
>    - reg
>  
>  allOf:
> @@ -81,6 +80,10 @@ allOf:
>            contains:
>              const: qcom,geni-i2c-master-hub
>      then:
> +      required:
> +        - clocks
> +        - clock-names


So it is required here?

> +
>        properties:
>          clocks:
>            minItems: 2
> @@ -100,7 +103,21 @@ allOf:
>            items:
>              - const: qup-core
>              - const: qup-config
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,sa8255p-geni-i2c
> +    then:
> +      required:
> +        - power-domains
> +

And possible here? I assume with the same clocks? The same for
interconnects - same values are valid?

>      else:
> +      required:
> +        - clocks
> +        - clock-names

And clocks are required again?

> +
>        properties:
>          clocks:
>            maxItems: 1

Eeee? So now all other variants have max 1 clock?

Nope, this wasn't ever tested on real DTS.

Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 4, 2024, 7:49 a.m. UTC | #2
On 04/09/2024 00:02, Nikunj Kela wrote:
> Add compatible representing i2c support on SA8255p.
> 
> Clocks and interconnects are being configured in Firmware VM
> on SA8255p, therefore making them optional.
> 
> CC: Praveen Talari <quic_ptalari@quicinc.com>
> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> ---
>  .../bindings/i2c/qcom,i2c-geni-qcom.yaml      | 33 +++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 

Just to clarify to I2C maintainers:
This is incomplete. Missing driver changes.

Best regards,
Krzysztof
Wolfram Sang Sept. 4, 2024, 7:55 a.m. UTC | #3
> Just to clarify to I2C maintainers:
> This is incomplete. Missing driver changes.

Thanks, Krzysztof!
Nikunj Kela Sept. 4, 2024, 12:41 p.m. UTC | #4
On 9/3/2024 11:31 PM, Krzysztof Kozlowski wrote:
> On Tue, Sep 03, 2024 at 03:02:34PM -0700, Nikunj Kela wrote:
>> Add compatible representing i2c support on SA8255p.
>>
>> Clocks and interconnects are being configured in Firmware VM
>> on SA8255p, therefore making them optional.
>>
>> CC: Praveen Talari <quic_ptalari@quicinc.com>
>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>> ---
>>  .../bindings/i2c/qcom,i2c-geni-qcom.yaml      | 33 +++++++++++++++++--
>>  1 file changed, 31 insertions(+), 2 deletions(-)
>>
> I don't know what to do with this patch. Using specific compatibles next
> to generic compatible is just wrong, although mistake was probably
> allowing generic compatible. The patch does not explain the differences
> in interface which would explain why devices are not compatible.

I mentioned in the description that clocks and interconnects on this
platform are configured in Firmware VM(over SCMI using power and perf
domains) therefore this is not compatible with existing generic compatible.


>  In the
> same time my advice of separate binding was not followed, because maybe
> these devices are compatible? But then it should be expressed...

Sorry, I missed that. You want me to use 'oneOf' expression with this
compatible?


>
> You have entire commit msg to explain what and why.

Will put more details in description.


>> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> index 9f66a3bb1f80..b477fae734b6 100644
>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>> @@ -15,6 +15,7 @@ properties:
>>      enum:
>>        - qcom,geni-i2c
>>        - qcom,geni-i2c-master-hub
>> +      - qcom,sa8255p-geni-i2c
>>  
>>    clocks:
>>      minItems: 1
>> @@ -69,8 +70,6 @@ properties:
>>  required:
>>    - compatible
>>    - interrupts
>> -  - clocks
>> -  - clock-names
>>    - reg
>>  
>>  allOf:
>> @@ -81,6 +80,10 @@ allOf:
>>            contains:
>>              const: qcom,geni-i2c-master-hub
>>      then:
>> +      required:
>> +        - clocks
>> +        - clock-names
>
> So it is required here?

We are removing clocks from generic required list and enforcing rules
for all compatibles other than sa8255p.


>> +
>>        properties:
>>          clocks:
>>            minItems: 2
>> @@ -100,7 +103,21 @@ allOf:
>>            items:
>>              - const: qup-core
>>              - const: qup-config
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: qcom,sa8255p-geni-i2c
>> +    then:
>> +      required:
>> +        - power-domains
>> +
> And possible here? I assume with the same clocks? The same for
> interconnects - same values are valid?

I guess I need to put here the same description as in the cover letter
to make it more clear. We are not using clocks and interconnects in this
platform in Linux. Instead, sending request to Firmware VM over
SCMI(using power and perf protocols)


>
>>      else:
>> +      required:
>> +        - clocks
>> +        - clock-names
> And clocks are required again?
Explained above.
>> +
>>        properties:
>>          clocks:
>>            maxItems: 1
> Eeee? So now all other variants have max 1 clock?

I will make if block for sa8255p up so else is not applied to rest of
the platforms.


>
> Nope, this wasn't ever tested on real DTS.

This is tested on SA8255p DTS and I ran DT schema check on SA8775p DT as
well.


>
> Best regards,
> Krzysztof
>
Nikunj Kela Sept. 4, 2024, 12:45 p.m. UTC | #5
On 9/4/2024 12:55 AM, Wolfram Sang wrote:
>> Just to clarify to I2C maintainers:
>> This is incomplete. Missing driver changes.
> Thanks, Krzysztof!

Driver changes are going through internal review and will soon be
posted. For your reference, we have pushed driver changes in CodeLinaro
git branch(nkela/sa8255p_v6_11_rc2)  in kernel-qcom repo [1]. You can
take a look at the changes that are in pipeline and will follow soon.

[1]:
https://git.codelinaro.org/clo/linux-kernel/kernel-qcom/-/tree/nkela/sa8255p_v6_11_rc2?ref_type=heads
Krzysztof Kozlowski Sept. 4, 2024, 1:20 p.m. UTC | #6
On 04/09/2024 14:41, Nikunj Kela wrote:
> 
> On 9/3/2024 11:31 PM, Krzysztof Kozlowski wrote:
>> On Tue, Sep 03, 2024 at 03:02:34PM -0700, Nikunj Kela wrote:
>>> Add compatible representing i2c support on SA8255p.
>>>
>>> Clocks and interconnects are being configured in Firmware VM
>>> on SA8255p, therefore making them optional.
>>>
>>> CC: Praveen Talari <quic_ptalari@quicinc.com>
>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
>>> ---
>>>  .../bindings/i2c/qcom,i2c-geni-qcom.yaml      | 33 +++++++++++++++++--
>>>  1 file changed, 31 insertions(+), 2 deletions(-)
>>>
>> I don't know what to do with this patch. Using specific compatibles next
>> to generic compatible is just wrong, although mistake was probably
>> allowing generic compatible. The patch does not explain the differences
>> in interface which would explain why devices are not compatible.
> 
> I mentioned in the description that clocks and interconnects on this
> platform are configured in Firmware VM(over SCMI using power and perf
> domains) therefore this is not compatible with existing generic compatible.

It is not obvious to me. I doubt it is obvious to others. Commit msg
does not say they are compatible and usually difference in
clocks/interconnects is not reason of incompatibility. So why suddenly
here we would understand it differently?


> 
> 
>>  In the
>> same time my advice of separate binding was not followed, because maybe
>> these devices are compatible? But then it should be expressed...
> 
> Sorry, I missed that. You want me to use 'oneOf' expression with this
> compatible?

I proposed separate binding file. But your commit msg suggested these
are compatible. Lack of driver change is also proof of that.

I don't want to keep discussing this because it does not lead to
anywhere. We keep repeating the same.

> 
> 
>>
>> You have entire commit msg to explain what and why.
> 
> Will put more details in description.
> 
> 
>>> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> index 9f66a3bb1f80..b477fae734b6 100644
>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>> @@ -15,6 +15,7 @@ properties:
>>>      enum:
>>>        - qcom,geni-i2c
>>>        - qcom,geni-i2c-master-hub
>>> +      - qcom,sa8255p-geni-i2c
>>>  
>>>    clocks:
>>>      minItems: 1
>>> @@ -69,8 +70,6 @@ properties:
>>>  required:
>>>    - compatible
>>>    - interrupts
>>> -  - clocks
>>> -  - clock-names
>>>    - reg
>>>  
>>>  allOf:
>>> @@ -81,6 +80,10 @@ allOf:
>>>            contains:
>>>              const: qcom,geni-i2c-master-hub
>>>      then:
>>> +      required:
>>> +        - clocks
>>> +        - clock-names
>>
>> So it is required here?
> 
> We are removing clocks from generic required list and enforcing rules
> for all compatibles other than sa8255p.
> 
> 
>>> +
>>>        properties:
>>>          clocks:
>>>            minItems: 2
>>> @@ -100,7 +103,21 @@ allOf:
>>>            items:
>>>              - const: qup-core
>>>              - const: qup-config
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: qcom,sa8255p-geni-i2c
>>> +    then:
>>> +      required:
>>> +        - power-domains
>>> +
>> And possible here? I assume with the same clocks? The same for
>> interconnects - same values are valid?
> 
> I guess I need to put here the same description as in the cover letter
> to make it more clear. We are not using clocks and interconnects in this
> platform in Linux. Instead, sending request to Firmware VM over
> SCMI(using power and perf protocols)
> 
> 
>>
>>>      else:
>>> +      required:
>>> +        - clocks
>>> +        - clock-names
>> And clocks are required again?
> Explained above.
>>> +
>>>        properties:
>>>          clocks:
>>>            maxItems: 1
>> Eeee? So now all other variants have max 1 clock?
> 
> I will make if block for sa8255p up so else is not applied to rest of
> the platforms.
> 
> 
>>
>> Nope, this wasn't ever tested on real DTS.
> 
> This is tested on SA8255p DTS and I ran DT schema check on SA8775p DT as
> well.

You just affected all the DTS everywhere. It's your task to check all
DTS everywhere. Not ours.

Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 4, 2024, 1:20 p.m. UTC | #7
On 04/09/2024 14:45, Nikunj Kela wrote:
> 
> On 9/4/2024 12:55 AM, Wolfram Sang wrote:
>>> Just to clarify to I2C maintainers:
>>> This is incomplete. Missing driver changes.
>> Thanks, Krzysztof!
> 
> Driver changes are going through internal review and will soon be
> posted. For your reference, we have pushed driver changes in CodeLinaro
> git branch(nkela/sa8255p_v6_11_rc2)  in kernel-qcom repo [1]. You can
> take a look at the changes that are in pipeline and will follow soon.
> 

Sorry, we are not reviewing other repos. Post patches ONLY when they are
ready. Sending one piece without driver is not correct and it does not
make any, absolutely any sense.

Best regards,
Krzysztof
Andi Shyti Sept. 5, 2024, 7:28 p.m. UTC | #8
Hi Nikunj,

On Wed, Sep 04, 2024 at 05:45:05AM GMT, Nikunj Kela wrote:
> 
> On 9/4/2024 12:55 AM, Wolfram Sang wrote:
> >> Just to clarify to I2C maintainers:
> >> This is incomplete. Missing driver changes.
> > Thanks, Krzysztof!
> 
> Driver changes are going through internal review and will soon be
> posted. For your reference, we have pushed driver changes in CodeLinaro
> git branch(nkela/sa8255p_v6_11_rc2)  in kernel-qcom repo [1]. You can
> take a look at the changes that are in pipeline and will follow soon.

Please post here driver changes along with the DTS updates.

Thanks Krzysztof for being active here!

Andi
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
index 9f66a3bb1f80..b477fae734b6 100644
--- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
+++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
@@ -15,6 +15,7 @@  properties:
     enum:
       - qcom,geni-i2c
       - qcom,geni-i2c-master-hub
+      - qcom,sa8255p-geni-i2c
 
   clocks:
     minItems: 1
@@ -69,8 +70,6 @@  properties:
 required:
   - compatible
   - interrupts
-  - clocks
-  - clock-names
   - reg
 
 allOf:
@@ -81,6 +80,10 @@  allOf:
           contains:
             const: qcom,geni-i2c-master-hub
     then:
+      required:
+        - clocks
+        - clock-names
+
       properties:
         clocks:
           minItems: 2
@@ -100,7 +103,21 @@  allOf:
           items:
             - const: qup-core
             - const: qup-config
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,sa8255p-geni-i2c
+    then:
+      required:
+        - power-domains
+
     else:
+      required:
+        - clocks
+        - clock-names
+
       properties:
         clocks:
           maxItems: 1
@@ -143,4 +160,16 @@  examples:
         power-domains = <&rpmhpd SC7180_CX>;
         required-opps = <&rpmhpd_opp_low_svs>;
     };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    i2c@a90000 {
+        compatible = "qcom,sa8255p-geni-i2c";
+        reg = <0xa90000 0x4000>;
+        interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        power-domains = <&scmi9_pd 11>;
+    };
 ...