[PATCH 0/2] More thermal configuration for Fairphone 4

[PATCH 0/2] More thermal configuration for Fairphone 4

[Luca Weiss]

Add the thermal configuration for the thermistors connected to PMK8003
and PM6150L. With that all the external thermistors on the phone should
be present in the dts.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
Luca Weiss (2):
      arm64: dts: qcom: sm7225-fairphone-fp4: Add PMK8003 thermals
      arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

 arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 216 ++++++++++++++++++++++
 1 file changed, 216 insertions(+)
---
base-commit: 50c107ee601a3d57e4fb13a9ecd9cf6c8df187f2
change-id: 20240105-fp4-thermals-f3c8e0417b70

Best regards,
-- 
Luca Weiss <luca.weiss@fairphone.com>

[PATCH 1/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PMK8003 thermals

[Luca Weiss]

Configure the thermals for the XO_THERM thermistor connected to the
PMK8003 (which is called PMK8350 in software).

The ADC configuration for PMK8350_ADC7_AMUX_THM1_100K_PU has already
been added in the past.

The trip points can really only be considered as placeholders, more
configuration with cooling etc. can be added later.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 25 +++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
index ade619805519..b7ccfe4011bb 100644
--- a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
+++ b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
@@ -112,6 +112,20 @@ active-config0 {
 				};
 			};
 		};
+
+		xo-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&pmk8350_adc_tm 0>;
+
+			trips {
+				active-config0 {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+		};
 	};
 };
 
@@ -490,6 +504,17 @@ conn-therm@1 {
 	};
 };
 
+&pmk8350_adc_tm {
+	status = "okay";
+
+	xo-therm@0 {
+		reg = <0>;
+		io-channels = <&pmk8350_vadc PMK8350_ADC7_AMUX_THM1_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time-us = <200>;
+	};
+};
+
 &pmk8350_rtc {
 	status = "okay";
 };

-- 
2.43.0

Re: [PATCH 1/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PMK8003 thermals

[Konrad Dybcio]

On 1/5/24 15:54, Luca Weiss wrote:
> Configure the thermals for the XO_THERM thermistor connected to the
> PMK8003 (which is called PMK8350 in software).
> 
> The ADC configuration for PMK8350_ADC7_AMUX_THM1_100K_PU has already
> been added in the past.
> 
> The trip points can really only be considered as placeholders, more
> configuration with cooling etc. can be added later.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>   arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 25 +++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
> index ade619805519..b7ccfe4011bb 100644
> --- a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
> +++ b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
> @@ -112,6 +112,20 @@ active-config0 {
>   				};
>   			};
>   		};
> +
> +		xo-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&pmk8350_adc_tm 0>;
> +
> +			trips {
> +				active-config0 {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "passive";
> +				};
> +			};
> +		};
>   	};
>   };
>   
> @@ -490,6 +504,17 @@ conn-therm@1 {
>   	};
>   };
>   
> +&pmk8350_adc_tm {
> +	status = "okay";
> +
> +	xo-therm@0 {
> +		reg = <0>;
> +		io-channels = <&pmk8350_vadc PMK8350_ADC7_AMUX_THM1_100K_PU>;
> +		qcom,ratiometric;
> +		qcom,hw-settle-time-us = <200>;

My ocd would rather see the boolean property at the end

anyway

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

[PATCH 2/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

[Luca Weiss]

Configure the thermals for the PA_THERM1, MSM_THERM, PA_THERM0,
RFC_CAM_THERM, CAM_FLASH_THERM and QUIET_THERM thermistors connected to
PM6150L.

Due to hardware constraints we can only register 4 zones with
pm6150l_adc_tm, the other 2 we can register via generic-adc-thermal.

The trip points can really only be considered as placeholders, more
configuration with cooling etc. can be added later.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 191 ++++++++++++++++++++++
 1 file changed, 191 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
index b7ccfe4011bb..6f435a7ed855 100644
--- a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
+++ b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
@@ -84,6 +84,20 @@ memory@efe01000 {
 		};
 	};
 
+	msm_therm_sensor: thermal-sensor-msm {
+		compatible = "generic-adc-thermal";
+		#thermal-sensor-cells = <0>;
+		io-channels = <&pm6150l_adc ADC5_AMUX_THM2_100K_PU>;
+		io-channel-names = "sensor-channel";
+	};
+
+	rear_cam_sensor: thermal-sensor-rear-cam {
+		compatible = "generic-adc-thermal";
+		#thermal-sensor-cells = <0>;
+		io-channels = <&pm6150l_adc ADC5_GPIO2_100K_PU>;
+		io-channel-names = "sensor-channel";
+	};
+
 	thermal-zones {
 		chg-skin-thermal {
 			polling-delay-passive = <0>;
@@ -113,6 +127,90 @@ active-config0 {
 			};
 		};
 
+		pa0-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&pm6150l_adc_tm 1>;
+
+			trips {
+				active-config0 {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+		};
+
+		pa1-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&pm6150l_adc_tm 0>;
+
+			trips {
+				active-config0 {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+		};
+
+		quiet-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&pm6150l_adc_tm 3>;
+
+			trips {
+				active-config0 {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+		};
+
+		rear-cam-thermal {
+			polling-delay-passive = <1000>;
+			polling-delay = <5000>;
+			thermal-sensors = <&rear_cam_sensor>;
+
+			trips {
+				active-config0 {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+		};
+
+		rfc-flash-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&pm6150l_adc_tm 2>;
+
+			trips {
+				active-config0 {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+		};
+
+		sdm-skin-thermal {
+			polling-delay-passive = <1000>;
+			polling-delay = <5000>;
+			thermal-sensors = <&msm_therm_sensor>;
+
+			trips {
+				active-config0 {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "passive";
+				};
+			};
+		};
+
 		xo-thermal {
 			polling-delay-passive = <0>;
 			polling-delay = <0>;
@@ -423,6 +521,91 @@ &mpss {
 	status = "okay";
 };
 
+&pm6150l_adc {
+	pinctrl-0 = <&pm6150l_adc_default>;
+	pinctrl-names = "default";
+
+	channel@4d {
+		reg = <ADC5_AMUX_THM1_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time = <200>;
+		qcom,pre-scaling = <1 1>;
+		label = "pa_therm1";
+	};
+
+	channel@4e {
+		reg = <ADC5_AMUX_THM2_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time = <200>;
+		qcom,pre-scaling = <1 1>;
+		label = "msm_therm";
+	};
+
+	channel@4f {
+		reg = <ADC5_AMUX_THM3_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time = <200>;
+		qcom,pre-scaling = <1 1>;
+		label = "pa_therm0";
+	};
+
+	channel@53 {
+		reg = <ADC5_GPIO2_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time = <200>;
+		qcom,pre-scaling = <1 1>;
+		label = "rear_cam_therm";
+	};
+
+	channel@54 {
+		reg = <ADC5_GPIO3_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time = <200>;
+		qcom,pre-scaling = <1 1>;
+		label = "rear_cam_flash_therm";
+	};
+
+	channel@55 {
+		reg = <ADC5_GPIO4_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time = <200>;
+		qcom,pre-scaling = <1 1>;
+		label = "quiet_therm";
+	};
+};
+
+&pm6150l_adc_tm {
+	status = "okay";
+
+	pa-therm1@0 {
+		reg = <0>;
+		io-channels = <&pm6150l_adc ADC5_AMUX_THM1_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time-us = <200>;
+	};
+
+	pa-therm0@1 {
+		reg = <1>;
+		io-channels = <&pm6150l_adc ADC5_AMUX_THM3_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time-us = <200>;
+	};
+
+	rear-cam-flash-therm@2 {
+		reg = <2>;
+		io-channels = <&pm6150l_adc ADC5_GPIO3_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time-us = <200>;
+	};
+
+	quiet-therm@3 {
+		reg = <3>;
+		io-channels = <&pm6150l_adc ADC5_GPIO4_100K_PU>;
+		qcom,ratiometric;
+		qcom,hw-settle-time-us = <200>;
+	};
+};
+
 &pm6150l_flash {
 	status = "okay";
 
@@ -445,6 +628,14 @@ led-1 {
 	};
 };
 
+&pm6150l_gpios {
+	pm6150l_adc_default: adc-default-state {
+		pins = "gpio6", "gpio7", "gpio10";
+		function = PMIC_GPIO_FUNC_NORMAL;
+		bias-high-impedance;
+	};
+};
+
 &pm6150l_wled {
 	qcom,switching-freq = <800>;
 	qcom,current-limit-microamp = <20000>;

-- 
2.43.0

Re: [PATCH 2/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

[Konrad Dybcio]

On 1/5/24 15:54, Luca Weiss wrote:
> Configure the thermals for the PA_THERM1, MSM_THERM, PA_THERM0,
> RFC_CAM_THERM, CAM_FLASH_THERM and QUIET_THERM thermistors connected to
> PM6150L.
> 
> Due to hardware constraints we can only register 4 zones with
> pm6150l_adc_tm, the other 2 we can register via generic-adc-thermal.

Ugh.. so the ADC can support more inputs than the ADC_TM that was
designed to ship alongside it can?

And that's why the "generic-adc-thermal"-provided zones need to
be polled?

> 
> The trip points can really only be considered as placeholders, more
> configuration with cooling etc. can be added later.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
[...]

I've read the sentence above, but..
> +		sdm-skin-thermal {
> +			polling-delay-passive = <1000>;
> +			polling-delay = <5000>;
> +			thermal-sensors = <&msm_therm_sensor>;
> +
> +			trips {
> +				active-config0 {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "passive";

I don't fancy burnt fingers for dinner!

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

[Dmitry Baryshkov]

On Tue, 9 Jan 2024 at 12:10, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 1/5/24 15:54, Luca Weiss wrote:
> > Configure the thermals for the PA_THERM1, MSM_THERM, PA_THERM0,
> > RFC_CAM_THERM, CAM_FLASH_THERM and QUIET_THERM thermistors connected to
> > PM6150L.
> >
> > Due to hardware constraints we can only register 4 zones with
> > pm6150l_adc_tm, the other 2 we can register via generic-adc-thermal.
>
> Ugh.. so the ADC can support more inputs than the ADC_TM that was
> designed to ship alongside it can?

Yes. ADC_TM can support monitoring of 8 channels in total.

>
> And that's why the "generic-adc-thermal"-provided zones need to
> be polled?
>
> >
> > The trip points can really only be considered as placeholders, more
> > configuration with cooling etc. can be added later.
> >
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> [...]
>
> I've read the sentence above, but..
> > +             sdm-skin-thermal {
> > +                     polling-delay-passive = <1000>;
> > +                     polling-delay = <5000>;
> > +                     thermal-sensors = <&msm_therm_sensor>;
> > +
> > +                     trips {
> > +                             active-config0 {
> > +                                     temperature = <125000>;
> > +                                     hysteresis = <1000>;
> > +                                     type = "passive";
>
> I don't fancy burnt fingers for dinner!
>
> Konrad
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

[Luca Weiss]

On Tue Jan 9, 2024 at 11:09 AM CET, Konrad Dybcio wrote:
>
>
> On 1/5/24 15:54, Luca Weiss wrote:
> > Configure the thermals for the PA_THERM1, MSM_THERM, PA_THERM0,
> > RFC_CAM_THERM, CAM_FLASH_THERM and QUIET_THERM thermistors connected to
> > PM6150L.
> > 
> > Due to hardware constraints we can only register 4 zones with
> > pm6150l_adc_tm, the other 2 we can register via generic-adc-thermal.
>
> Ugh.. so the ADC can support more inputs than the ADC_TM that was
> designed to ship alongside it can?
>
> And that's why the "generic-adc-thermal"-provided zones need to
> be polled?

This part of the code from qcom-spmi-adc-tm5.c was trigerring if I
define more than 4 channels, and looking at downstream I can also see
that only 4 zones are registered properly with adc_tm, the rest is
registered with "qcom,adc-tm5-iio" which skips from what I could tell
basically all the HW bits and only registering the thermal zone.


	ret = adc_tm5_read(chip, ADC_TM5_NUM_BTM,
			   &channels_available, sizeof(channels_available));
	if (ret) {
		dev_err(chip->dev, "read failed for BTM channels\n");
		return ret;
	}

	for (i = 0; i < chip->nchannels; i++) {
		if (chip->channels[i].channel >= channels_available) {
			dev_err(chip->dev, "Invalid channel %d\n", chip->channels[i].channel);
			return -EINVAL;
		}
	}


>
> > 
> > The trip points can really only be considered as placeholders, more
> > configuration with cooling etc. can be added later.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> [...]
>
> I've read the sentence above, but..
> > +		sdm-skin-thermal {
> > +			polling-delay-passive = <1000>;
> > +			polling-delay = <5000>;
> > +			thermal-sensors = <&msm_therm_sensor>;
> > +
> > +			trips {
> > +				active-config0 {
> > +					temperature = <125000>;
> > +					hysteresis = <1000>;
> > +					type = "passive";
>
> I don't fancy burnt fingers for dinner!

With passive trip point it wouldn't even do anything now, but at what
temp do you think it should do what? I'd definitely need more time to
understand more of how the thermal setup works in downstream Android,
and then replicate a sane configuration for mainline with proper
temperatures, cooling, etc.

Regards
Luca

>
> Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

[Konrad Dybcio]

On 1/9/24 12:24, Luca Weiss wrote:
> On Tue Jan 9, 2024 at 11:09 AM CET, Konrad Dybcio wrote:
>>
>>
>> On 1/5/24 15:54, Luca Weiss wrote:
>>> Configure the thermals for the PA_THERM1, MSM_THERM, PA_THERM0,
>>> RFC_CAM_THERM, CAM_FLASH_THERM and QUIET_THERM thermistors connected to
>>> PM6150L.
>>>
>>> Due to hardware constraints we can only register 4 zones with
>>> pm6150l_adc_tm, the other 2 we can register via generic-adc-thermal.
>>
>> Ugh.. so the ADC can support more inputs than the ADC_TM that was
>> designed to ship alongside it can?
>>
>> And that's why the "generic-adc-thermal"-provided zones need to
>> be polled?
> 
> This part of the code from qcom-spmi-adc-tm5.c was trigerring if I
> define more than 4 channels, and looking at downstream I can also see
> that only 4 zones are registered properly with adc_tm, the rest is
> registered with "qcom,adc-tm5-iio" which skips from what I could tell
> basically all the HW bits and only registering the thermal zone.
> 
> 
> 	ret = adc_tm5_read(chip, ADC_TM5_NUM_BTM,
> 			   &channels_available, sizeof(channels_available));
> 	if (ret) {
> 		dev_err(chip->dev, "read failed for BTM channels\n");
> 		return ret;
> 	}
> 
> 	for (i = 0; i < chip->nchannels; i++) {
> 		if (chip->channels[i].channel >= channels_available) {
> 			dev_err(chip->dev, "Invalid channel %d\n", chip->channels[i].channel);
> 			return -EINVAL;
> 		}
> 	}
> 
> 
>>
>>>
>>> The trip points can really only be considered as placeholders, more
>>> configuration with cooling etc. can be added later.
>>>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>> [...]
>>
>> I've read the sentence above, but..
>>> +		sdm-skin-thermal {
>>> +			polling-delay-passive = <1000>;
>>> +			polling-delay = <5000>;
>>> +			thermal-sensors = <&msm_therm_sensor>;
>>> +
>>> +			trips {
>>> +				active-config0 {
>>> +					temperature = <125000>;
>>> +					hysteresis = <1000>;
>>> +					type = "passive";
>>
>> I don't fancy burnt fingers for dinner!
> 
> With passive trip point it wouldn't even do anything now, but at what
> temp do you think it should do what? I'd definitely need more time to
> understand more of how the thermal setup works in downstream Android,
> and then replicate a sane configuration for mainline with proper
> temperatures, cooling, etc.
If "skin therm" means "the temperature of some part of the phone's
body that can be felt with a human hand", then definitely some
throttling should happen at 40ish with heavy throttling at 50
and crit at 55 or so..

We should probably make this a broader topic and keep a single
policy for all supported phones.

+ CC AGdR, may be interested in where this leads

Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

[Luca Weiss]

On Wed Jan 10, 2024 at 8:16 PM CET, Konrad Dybcio wrote:
>
>
> On 1/9/24 12:24, Luca Weiss wrote:
> > On Tue Jan 9, 2024 at 11:09 AM CET, Konrad Dybcio wrote:
> >>
> >>
> >> On 1/5/24 15:54, Luca Weiss wrote:
> >>> Configure the thermals for the PA_THERM1, MSM_THERM, PA_THERM0,
> >>> RFC_CAM_THERM, CAM_FLASH_THERM and QUIET_THERM thermistors connected to
> >>> PM6150L.
> >>>
> >>> Due to hardware constraints we can only register 4 zones with
> >>> pm6150l_adc_tm, the other 2 we can register via generic-adc-thermal.
> >>
> >> Ugh.. so the ADC can support more inputs than the ADC_TM that was
> >> designed to ship alongside it can?
> >>
> >> And that's why the "generic-adc-thermal"-provided zones need to
> >> be polled?
> > 
> > This part of the code from qcom-spmi-adc-tm5.c was trigerring if I
> > define more than 4 channels, and looking at downstream I can also see
> > that only 4 zones are registered properly with adc_tm, the rest is
> > registered with "qcom,adc-tm5-iio" which skips from what I could tell
> > basically all the HW bits and only registering the thermal zone.
> > 
> > 
> > 	ret = adc_tm5_read(chip, ADC_TM5_NUM_BTM,
> > 			   &channels_available, sizeof(channels_available));
> > 	if (ret) {
> > 		dev_err(chip->dev, "read failed for BTM channels\n");
> > 		return ret;
> > 	}
> > 
> > 	for (i = 0; i < chip->nchannels; i++) {
> > 		if (chip->channels[i].channel >= channels_available) {
> > 			dev_err(chip->dev, "Invalid channel %d\n", chip->channels[i].channel);
> > 			return -EINVAL;
> > 		}
> > 	}
> > 
> > 
> >>
> >>>
> >>> The trip points can really only be considered as placeholders, more
> >>> configuration with cooling etc. can be added later.
> >>>
> >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> >>> ---
> >> [...]
> >>
> >> I've read the sentence above, but..
> >>> +		sdm-skin-thermal {
> >>> +			polling-delay-passive = <1000>;
> >>> +			polling-delay = <5000>;
> >>> +			thermal-sensors = <&msm_therm_sensor>;
> >>> +
> >>> +			trips {
> >>> +				active-config0 {
> >>> +					temperature = <125000>;
> >>> +					hysteresis = <1000>;
> >>> +					type = "passive";
> >>
> >> I don't fancy burnt fingers for dinner!
> > 
> > With passive trip point it wouldn't even do anything now, but at what
> > temp do you think it should do what? I'd definitely need more time to
> > understand more of how the thermal setup works in downstream Android,
> > and then replicate a sane configuration for mainline with proper
> > temperatures, cooling, etc.
> If "skin therm" means "the temperature of some part of the phone's
> body that can be felt with a human hand", then definitely some
> throttling should happen at 40ish with heavy throttling at 50
> and crit at 55 or so..
>
> We should probably make this a broader topic and keep a single
> policy for all supported phones.

I agree that this shouldn't be implemented differently per device since
it's really more a question "what should Linux do" that's quite a
general question and not device-specific. Of course some device-specific
tweaks could be here and there, like if the phone has metal back or
plastic back but it's only minor.

Based on the config here
https://gerrit-public.fairphone.software/plugins/gitiles/platform/hardware/qcom/thermal/+/refs/heads/odm/dev/target/13/fp5/thermalConfig.cpp#946
it looks like throtteling starts for internal components at 95degC with
a shutdown threshold of 115degC.
The skin sensor here has a throttling threshold of 40degC and shutdown
threshold of 95degC.

But actually I'm not even sure this config gets active for QCM6490 with
socid=497. So yeah I need more time digging into the thermal code to see
what it's actually doing.. Not that it would/should be much different
for socid=497 I guess though.

There's also plenty of thermal code in qcom proprietary.

Regards
Luca

>
> + CC AGdR, may be interested in where this leads
>
> Konrad

Re: [PATCH 2/2] arm64: dts: qcom: sm7225-fairphone-fp4: Add PM6150L thermals

[AngeloGioacchino Del Regno]

Il 10/01/24 20:16, Konrad Dybcio ha scritto:
> 
> 
> On 1/9/24 12:24, Luca Weiss wrote:
>> On Tue Jan 9, 2024 at 11:09 AM CET, Konrad Dybcio wrote:
>>>
>>>
>>> On 1/5/24 15:54, Luca Weiss wrote:
>>>> Configure the thermals for the PA_THERM1, MSM_THERM, PA_THERM0,
>>>> RFC_CAM_THERM, CAM_FLASH_THERM and QUIET_THERM thermistors connected to
>>>> PM6150L.
>>>>
>>>> Due to hardware constraints we can only register 4 zones with
>>>> pm6150l_adc_tm, the other 2 we can register via generic-adc-thermal.
>>>
>>> Ugh.. so the ADC can support more inputs than the ADC_TM that was
>>> designed to ship alongside it can?
>>>
>>> And that's why the "generic-adc-thermal"-provided zones need to
>>> be polled?
>>
>> This part of the code from qcom-spmi-adc-tm5.c was trigerring if I
>> define more than 4 channels, and looking at downstream I can also see
>> that only 4 zones are registered properly with adc_tm, the rest is
>> registered with "qcom,adc-tm5-iio" which skips from what I could tell
>> basically all the HW bits and only registering the thermal zone.
>>
>>
>>     ret = adc_tm5_read(chip, ADC_TM5_NUM_BTM,
>>                &channels_available, sizeof(channels_available));
>>     if (ret) {
>>         dev_err(chip->dev, "read failed for BTM channels\n");
>>         return ret;
>>     }
>>
>>     for (i = 0; i < chip->nchannels; i++) {
>>         if (chip->channels[i].channel >= channels_available) {
>>             dev_err(chip->dev, "Invalid channel %d\n", chip->channels[i].channel);
>>             return -EINVAL;
>>         }
>>     }
>>
>>
>>>
>>>>
>>>> The trip points can really only be considered as placeholders, more
>>>> configuration with cooling etc. can be added later.
>>>>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>> [...]
>>>
>>> I've read the sentence above, but..
>>>> +        sdm-skin-thermal {
>>>> +            polling-delay-passive = <1000>;
>>>> +            polling-delay = <5000>;
>>>> +            thermal-sensors = <&msm_therm_sensor>;
>>>> +
>>>> +            trips {
>>>> +                active-config0 {
>>>> +                    temperature = <125000>;
>>>> +                    hysteresis = <1000>;
>>>> +                    type = "passive";
>>>
>>> I don't fancy burnt fingers for dinner!
>>
>> With passive trip point it wouldn't even do anything now, but at what
>> temp do you think it should do what? I'd definitely need more time to
>> understand more of how the thermal setup works in downstream Android,
>> and then replicate a sane configuration for mainline with proper
>> temperatures, cooling, etc.
> If "skin therm" means "the temperature of some part of the phone's
> body that can be felt with a human hand", then definitely some
> throttling should happen at 40ish with heavy throttling at 50
> and crit at 55 or so..
> 
> We should probably make this a broader topic and keep a single
> policy for all supported phones.
> 
> + CC AGdR, may be interested in where this leads
> 
> Konrad

A thermal trip at 125°C for *skin temperature* is useless... if a device's skin
temperature (be it a smartphone, a SBC, a Chromebook, a non-specially-identified
laptop, a car head unit, or whatever else you can imagine) reaches that kind of
temperature, this means that something inside likely reached something along the
lines of 150°C for a prolonged period of time.

You will definitely agree with me that if something reached that temperature for
a certain period of time, it is *highly unlikely* (not to say impossible) that
Linux is even still running and that the green smoke that is naturally trapped in
any chip didn't get released :-)

Besides, keep in mind that if the SKIN temperature is 55°C, if your device has
a -> lithium <- battery (li-ion/lifepo/others), your hands are "probably" in a
kind of danger that I wouldn't be comfortable with (and neither you I'm sure).

Strictly related to the trip temperature setting for "SKIN", for me this is a
strong NAK.

I'd go for stricter ranges too, something like...
- critical: ~52/53
- heavy throttling: 49/50
- throttle: ~45
NOTE: this would be valid only if there are other throttling points for CPU/GPU/etc


---- Anyway, something else I have in my mind: ----

Konrad: "standardizing" skin temperature is too big of a bet, and will be wrong,
because industrial-grade devices are permitted to reach higher skin temperatures.
Some industrial grade smartphones (example: rugged stuff) may have sensors in
the underside of the ruggedized shell... so in that case you want to set the skin
temperature limit with delta-T ideally...

Though!

Making this easier for everyone: we can somehow dictate (unofficially, because
of more of the many reasons I already explained) an acceptable temperature range
for "skin" temperature, outside of which, reviews should follow manual testing.

As for the concept of "skin temperature", and for some thermal framework work
(sorry for the word game) that I'm bringing on the table, in this case we can
imagine it as:

Thermal zone type: SKIN
Thermal zone name: shell-upper/shell-rightmost/shell....something

Type SKIN would be defined as
"a zone defining the temperature of the outer shell of a device"...

...and for example differently from type PCB, which fits different temperature
probe points.


Feel free to keep me in the loop, btw.

Cheers,
Angelo