Re: [PATCH 4/4] arm64: dts: qcom: qcm6490-fairphone-fp5: Add PM7325 thermals

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 10/20/23 13:31, Luca Weiss wrote:
> On Wed Oct 18, 2023 at 10:28 PM CEST, Konrad Dybcio wrote:
>>
>>
>> On 10/14/23 19:52, Luca Weiss wrote:
>>> On Samstag, 14. Oktober 2023 01:13:29 CEST Konrad Dybcio wrote:
>>>> On 13.10.2023 10:09, Luca Weiss wrote:
>>>>> Configure the thermals for the QUIET_THERM, CAM_FLASH_THERM, MSM_THERM
>>>>> and RFC_CAM_THERM thermistors connected to PM7325.
>>>>>
>>>>> With this PMIC the software communication to the ADC is going through
>>>>> PMK7325 (= PMK8350).
>>>>>
>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>> ---
>>>>>
>>>>>    arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts | 117
>>>>>    +++++++++++++++++++++ 1 file changed, 117 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
>>>>> b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts index
>>>>> 2c01f799a6b2..d0b1e4e507ff 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
>>>>> @@ -9,6 +9,7 @@
>>>>>
>>>>>    #define PM7250B_SID 8
>>>>>    #define PM7250B_SID1 9
>>>>>
>>>>> +#include <dt-bindings/iio/qcom,spmi-adc7-pm7325.h>
>>>>>
>>>>>    #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h>
>>>>>    #include <dt-bindings/leds/common.h>
>>>>>    #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
>>>>>
>>>>> @@ -137,6 +138,20 @@ afvdd_2p8: regulator-afvdd-2p8 {
>>>>>
>>>>>    	};
>>>>>    	
>>>>>    	thermal-zones {
>>>>>
>>>>> +		camera-thermal {
>>>>> +			polling-delay-passive = <0>;
>>>>> +			polling-delay = <0>;
>>>>> +			thermal-sensors = <&pmk8350_adc_tm 2>;
>>>>> +
>>>>> +			trips {
>>>>> +				active-config0 {
>>>>> +					temperature = <125000>;
>>>>
>>>> are
>>>>
>>>>> +		rear-cam-thermal {
>>>>>
>>>>> +					temperature = <125000>;
>>>>
>>>> you
>>>>
>>>>> +		sdm-skin-thermal {
>>>>>
>>>>> +					temperature = <125000>;
>>>>
>>>> sure
>>>>
>>>> about these temps?
>>>
>>> (email from my other address, quicker right now)
>>>
>>> Well yes and no.
>>>
>>> Yes as in those are the temps specified in downstream dtb.
>>> No as in I'm 99% sure there's user space with definitely lower threshold that
>>> actually does something in response to the temps.
>>>
>>> I didn't look too much into this but does the kernel even do something when it
>>> hits one of these trip points? I assume when there's a cooling device thing
>>> specified then it can actually tell the driver to do something, but without
>>> (and most drivers don't support this?) I'm assuming the kernel can't do much
>>> anyways?
>>>
>>> So e.g. when the temperature for the flash led is reached I'm assuming
>>> downstream (+Android) either dims the led or turns it off? But I'd have to dig
>>> quite a bit into the thermal setup there to check what it's really doing.
>> I think reaching "critical" shuts down the platform, unless something
>> registering the thermal zone explicitly overrides the behavior.
> 
> Should probably be easy to test, especially the camera flash thermal
> zone heats up *very* quickly when the flash is on, so should be trivial
> to set the trip point down from 125degC to e.g. 45degC and see what
> happens.
> 
> So I did this and... nothing happened.
> I watched /sys/class/thermal/thermal_zone34/temp climb above 45degC and
> nothing happened.
> 
> I guess trip type being "passive" and no cooling-device makes it not do
> anything.
> 
>    ==> /sys/class/thermal/thermal_zone34/trip_point_0_hyst <==
>    1000
>    ==> /sys/class/thermal/thermal_zone34/trip_point_0_temp <==
>    45000
>    ==> /sys/class/thermal/thermal_zone34/trip_point_0_type <==
>    passive
> 
>  From Documentation/devicetree/bindings/thermal/thermal-zones.yaml:
> 
>    - active   # enable active cooling e.g. fans
>    - passive  # enable passive cooling e.g. throttling cpu
>    - hot      # send notification to driver
>    - critical # send notification to driver, trigger shutdown
> 
> So unless we want to just shut down the system (with "critical"), I
> don't think thermal can't really do anything else right now, since e.g.
> leds-qcom-flash.c driver doesn't have any cooling support to lower the
> brightness or turn off the LED.
> 
> So.. in essence not much we can do right now.
Yeah.. crashing the phone because the LED is too hot is sorta
suboptimal! Though I mainly had the skin temp in mind..

> 
> But seems we also cannot remove this (kinda useless) trip since we need
> at least one trip point in the dts if I read the bindings yaml
> correctly.
Right

> 
>>
>>>
>>> But for now I think it's okay to put this current thermal config into dts and
>>> we'll improve it later when 1. I understand more and 2. maybe some useful
>>> drivers support the cooling bits?
>> Yeah it's better than nothing, but ultimately we should probably move
>> the values that userspace daemon operates on here in the dt..
> 
> For sure.. I spent a bit of time looking into the proprietary Qualcomm
> thermal-daemon sources but didn't really see much interesting things
> there for this platform, maybe some of this thermal handling is
> somewhere else - or half of these thermal zones aren't even used with
> Android.
> 
> So.. good to get the current patch upstream or not? :)
Yep, just having the ability to read out thing is always good ;)

Konrad