diff mbox series

[v4,3/4] dt-bindings: interconnect: add clock property to enable QOS on SC7280

Message ID 20240325181628.9407-4-quic_okukatla@quicinc.com (mailing list archive)
State Handled Elsewhere
Headers show
Series Add support for QoS configuration | expand

Commit Message

Odelu Kukatla March 25, 2024, 6:16 p.m. UTC
Added clock property to enable clocks required for accessing
qos registers.

Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
---
 .../bindings/interconnect/qcom,sc7280-rpmh.yaml    | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Krzysztof Kozlowski March 26, 2024, 7:30 a.m. UTC | #1
On 25/03/2024 19:16, Odelu Kukatla wrote:
> Added clock property to enable clocks required for accessing
> qos registers.
> 
> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
> ---
>  .../bindings/interconnect/qcom,sc7280-rpmh.yaml    | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
> index b135597d9489..950ecdd5252e 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
> @@ -35,6 +35,10 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  clocks:
> +    minItems: 1
> +    maxItems: 2

Why is this flexible? Nothing in commit msg explains that. I gave the
same talk twice, gave there examples, yet it is not enough...

https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132

> +
>  required:
>    - compatible
>  
> @@ -57,6 +61,7 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
>      interconnect {
>          compatible = "qcom,sc7280-clk-virt";
>          #interconnect-cells = <2>;
> @@ -69,3 +74,12 @@ examples:
>          #interconnect-cells = <2>;
>          qcom,bcm-voters = <&apps_bcm_voter>;

If all devices have clocks, then you could add them here. It seems not
all of them have clocks...

Best regards,
Krzysztof
Odelu Kukatla March 27, 2024, 11:35 a.m. UTC | #2
On 3/26/2024 1:00 PM, Krzysztof Kozlowski wrote:
> On 25/03/2024 19:16, Odelu Kukatla wrote:
>> Added clock property to enable clocks required for accessing
>> qos registers.
>>
>> Signed-off-by: Odelu Kukatla <quic_okukatla@quicinc.com>
>> ---
>>  .../bindings/interconnect/qcom,sc7280-rpmh.yaml    | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
>> index b135597d9489..950ecdd5252e 100644
>> --- a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
>> @@ -35,6 +35,10 @@ properties:
>>    reg:
>>      maxItems: 1
>>  
>> +  clocks:
>> +    minItems: 1
>> +    maxItems: 2
> 
> Why is this flexible? Nothing in commit msg explains that. I gave the
> same talk twice, gave there examples, yet it is not enough...
> 

Clocks property is optional, and can be either 1 or 2 or none.
I think "minItems: 1" should be removed. If no clock property is mentioned in node that means no clock is required to be enabled for QoS configuration.

I will add back specific number of clocks based on interconnect compatible similar to what i did in v3.

> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132
> 
>> +
>>  required:
>>    - compatible
>>  
>> @@ -57,6 +61,7 @@ unevaluatedProperties: false
>>  
>>  examples:
>>    - |
>> +    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
>>      interconnect {
>>          compatible = "qcom,sc7280-clk-virt";
>>          #interconnect-cells = <2>;
>> @@ -69,3 +74,12 @@ examples:
>>          #interconnect-cells = <2>;
>>          qcom,bcm-voters = <&apps_bcm_voter>;
> 
> If all devices have clocks, then you could add them here. It seems not
> all of them have clocks...
> 
> Best regards,
> Krzysztof
> 

Thanks,
Odelu
Krzysztof Kozlowski March 27, 2024, 12:14 p.m. UTC | #3
On 27/03/2024 12:35, Odelu Kukatla wrote:
>>>      maxItems: 1
>>>  
>>> +  clocks:
>>> +    minItems: 1
>>> +    maxItems: 2
>>
>> Why is this flexible? Nothing in commit msg explains that. I gave the
>> same talk twice, gave there examples, yet it is not enough...
>>
> 
> Clocks property is optional, and can be either 1 or 2 or none.
> I think "minItems: 1" should be removed. If no clock property is mentioned in node that means no clock is required to be enabled for QoS configuration.
> 
> I will add back specific number of clocks based on interconnect compatible similar to what i did in v3.

Just be sure you read the example I gave you.



Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
index b135597d9489..950ecdd5252e 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,sc7280-rpmh.yaml
@@ -35,6 +35,10 @@  properties:
   reg:
     maxItems: 1
 
+  clocks:
+    minItems: 1
+    maxItems: 2
+
 required:
   - compatible
 
@@ -57,6 +61,7 @@  unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
     interconnect {
         compatible = "qcom,sc7280-clk-virt";
         #interconnect-cells = <2>;
@@ -69,3 +74,12 @@  examples:
         #interconnect-cells = <2>;
         qcom,bcm-voters = <&apps_bcm_voter>;
     };
+
+    interconnect@16e0000 {
+        reg = <0x016e0000 0x1c080>;
+        compatible = "qcom,sc7280-aggre1-noc";
+        #interconnect-cells = <2>;
+        qcom,bcm-voters = <&apps_bcm_voter>;
+        clocks = <&gcc GCC_AGGRE_UFS_PHY_AXI_CLK>,
+                 <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>;
+    };