Message-ID: <ef51aa03-4478-42f4-88a3-510457c4b917@oss.qualcomm.com>
Date: Mon, 22 Dec 2025 23:08:03 +0530
From: Odelu Kukatla <odelu.kukatla@....qualcomm.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Georgi Djakov <djakov@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>
Cc: Raviteja Laggyshetty <raviteja.laggyshetty@....qualcomm.com>,
        Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mike Tipton <mike.tipton@....qualcomm.com>
Subject: Re: [PATCH 1/3] dt-bindings: interconnect: add clocks property to
 enable QoS on qcs8300



On 11/29/2025 3:03 PM, Krzysztof Kozlowski wrote:
> On 28/11/2025 16:01, Odelu Kukatla wrote:
>> Add 'clocks' property to enable QoS configuration. This property
>> enables the necessary clocks for QoS configuration.
>>
>> QoS configuration is essential for ensuring that latency sensitive
>> components such as CPUs and multimedia engines receive prioritized
>> access to memory and interconnect resources. This helps to manage
>> bandwidth and latency across subsystems, improving system responsiveness
>> and performance in concurrent workloads.
> 
> I don't see how clocks property help here at all. Are you getting clock
> rates in the driver of some other clocks to make QoS decisions?
> 

We don't need to get clock rate/frequency, just need to enable the
clock(s) for QoS register access for which we need to get the clock
handle in driver.

>>
>> Both 'reg' and 'clocks' properties are optional. If either is missing,
> 
> No! They are not. How they can be optional in the hardware? How SoC can
> have for ONE GIVEN device optional reg, meaning one board with the same
> Soc has the IO address space but other board with the same SoC does not
> have it.
> 

I agree, I will drop the “optional” wording and rework the schema so
that `reg` and clocks’ are required for interconnects that implement
these resources, and not allowed for the NOCs without MMIO/clocks and
the virtual providers.

>> QoS configuration will be skipped. This behavior is controlled by the
>> 'qos_requires_clocks' flag in the driver, which ensures that QoS
>> configuration is bypassed when required clocks are not defined.
> 
> This suggests that - driver is not helping. Please describe the
> hardware, not your drivers.
> 

I will remove the driver related description from the commit message and
ensure the binding text stays strictly about the hardware.

>>
>> Signed-off-by: Odelu Kukatla <odelu.kukatla@....qualcomm.com>
>> ---
>>  .../interconnect/qcom,qcs8300-rpmh.yaml       | 53 ++++++++++++++++---
>>  1 file changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,qcs8300-rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,qcs8300-rpmh.yaml
>> index e9f528d6d9a8..594e835d1845 100644
>> --- a/Documentation/devicetree/bindings/interconnect/qcom,qcs8300-rpmh.yaml
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,qcs8300-rpmh.yaml
>> @@ -35,6 +35,10 @@ properties:
>>    reg:
>>      maxItems: 1
>>  
>> +  clocks:
>> +    minItems: 1
>> +    maxItems: 4
>> +
>>  required:
>>    - compatible
>>  
>> @@ -45,14 +49,39 @@ allOf:
>>          compatible:
>>            contains:
>>              enum:
>> -              - qcom,qcs8300-clk-virt
>> -              - qcom,qcs8300-mc-virt
>> +              - qcom,qcs8300-aggre1-noc
>>      then:
>>        properties:
>> -        reg: false
>> -    else:
>> -      required:
>> -        - reg
> 
> Why do you remove this? You cannot make random changes.
> 

My goal was to add clocks for the interconnects that need them, not to
relax the `reg` requirements or change the behavior for
`clk-virt`/`mc-virt`. I will restore the previous `reg` handling and
only add additional constraints for `clocks` where they are actually
required by the hardware.

>> +        clocks:
>> +          items:
>> +            - description: aggre UFS PHY AXI clock
>> +            - description: aggre QUP PRIM AXI clock
>> +            - description: aggre USB2 PRIM AXI clock
>> +            - description: aggre USB3 PRIM AXI clock
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,qcs8300-aggre2-noc
>> +    then:
>> +      properties:
>> +        clocks:
>> +          items:
>> +            - description: RPMH CC IPA clock
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,qcs8300-gem-noc
>> +    then:
>> +      properties:
>> +        clocks:
>> +          items:
>> +            - description: GCC DDRSS GPU AXI clock
> 
> So all devices have clocks now? You made the schema less strict now,
> removed pieces of it and you add more relaxed code telling every device
> has a clock.
> 

Not all interconnects have clocks/MMIO, I will update the bindings as
you suggested in the fix:
https://lore.kernel.org/all/20251129094612.16838-2-krzysztof.kozlowski@oss.qualcomm.com/


> And none of this is explained in the commit msg.
> 
>>  
>>  unevaluatedProperties: false
>>  
>> @@ -63,6 +92,7 @@ examples:
>>          reg = <0x9100000 0xf7080>;
>>          #interconnect-cells = <2>;
>>          qcom,bcm-voters = <&apps_bcm_voter>;
>> +        clocks = <&gcc_ddrss_gpu_axi_clk>;
>>      };
>>  
>>      clk_virt: interconnect-0 {
>> @@ -70,3 +100,14 @@ examples:
>>          #interconnect-cells = <2>;
>>          qcom,bcm-voters = <&apps_bcm_voter>;
>>      };
>> +
>> +    aggre1_noc: interconnect@...0000 {
> 
> No need for new example, it is the same as previous.
> 


Understood. I will drop the additional example instead of duplicating
essentially the same content.
I’ll rework the patch accordingly and send a v2 that keeps the schema
strict.

>> +        compatible = "qcom,qcs8300-aggre1-noc";
>> +        reg = <0x016c0000 0x17080>;
>> +        #interconnect-cells = <2>;
>> +        qcom,bcm-voters = <&apps_bcm_voter>;
>> +        clocks = <&gcc_aggre_ufs_phy_axi_clk>,
>> +                 <&gcc_aggre_noc_qupv3_axi_clk>,
>> +                 <&gcc_aggre_usb2_prim_axi_clk>,
>> +                 <&gcc_aggre_usb3_prim_axi_clk>;
>> +    };
> 
> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists