Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Add gpu cooling support

[Akhil P Oommen <akhilpo@codeaurora.org>]

On 10/9/2020 10:27 PM, Matthias Kaehlcke wrote:
> On Fri, Oct 09, 2020 at 08:05:10AM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, Oct 8, 2020 at 10:10 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>>>
>>> Add cooling-cells property and the cooling maps for the gpu tzones
>>> to support GPU cooling.
>>>
>>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sc7180.dtsi | 29 ++++++++++++++++++++++-------
>>>   1 file changed, 22 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>>> index d46b383..40d6a28 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>>> @@ -2,7 +2,7 @@
>>>   /*
>>>    * SC7180 SoC device tree source
>>>    *
>>> - * Copyright (c) 2019, The Linux Foundation. All rights reserved.
>>> + * Copyright (c) 2019-20, The Linux Foundation. All rights reserved.
>>>    */
>>>
>>>   #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
>>> @@ -1885,6 +1885,7 @@
>>>                          iommus = <&adreno_smmu 0>;
>>>                          operating-points-v2 = <&gpu_opp_table>;
>>>                          qcom,gmu = <&gmu>;
>>> +                       #cooling-cells = <2>;
>>
>> Presumably we should add this to the devicetree bindings, too?
Yes, thanks for catching this. Will update in the next patch.

>>
>>
>>>                          interconnects = <&gem_noc MASTER_GFX3D &mc_virt SLAVE_EBI1>;
>>>                          interconnect-names = "gfx-mem";
>>> @@ -3825,16 +3826,16 @@
>>>                  };
>>>
>>>                  gpuss0-thermal {
>>> -                       polling-delay-passive = <0>;
>>> +                       polling-delay-passive = <100>;
>>
>> Why did you make this change?  I'm pretty sure that we _don't_ want
>> this since we're using interrupts for the thermal sensor.  See commit
>> 22337b91022d ("arm64: dts: qcom: sc7180: Changed polling mode in
>> Thermal-zones node").
> 
> I was going to ask the same, this shouldn't be needed.
> 
>>>                          polling-delay = <0>;
>>>
>>>                          thermal-sensors = <&tsens0 13>;
>>>
>>>                          trips {
>>>                                  gpuss0_alert0: trip-point0 {
>>> -                                       temperature = <90000>;
>>> +                                       temperature = <95000>;
>>>                                          hysteresis = <2000>;
>>> -                                       type = "hot";
>>> +                                       type = "passive";
>>
>> Matthias probably knows better, but I wonder if we should be making
>> two passive trip levels like we do with CPU.  IIRC this is important
>> if someone wants to be able to use this with IPA.
> 
> Yes, please introduce a second trip point and make both of them
> 'passive'.
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
Adding Manaf here.

-Akhil.