[PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[Luca Weiss]

Add temp-alarm device tree node and a default configuration for the
corresponding thermal zone for this PMIC. Temperatures are based on
downstream values.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
With this config I'm getting this in dmesg, not sure if it's a warning
that should be solved or just an informative warning.

[    0.268256] spmi-temp-alarm c440000.spmi:pmic@0:temp-alarm@2400: No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140 C! Configuring stage 2 shutdown at 140 C.

As far as I can tell, based on downstream dts this PMIC doesn't have an
ADC.

 arch/arm64/boot/dts/qcom/pm6350.dtsi | 38 ++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm6350.dtsi b/arch/arm64/boot/dts/qcom/pm6350.dtsi
index c5d85064562b..1d24189680ea 100644
--- a/arch/arm64/boot/dts/qcom/pm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm6350.dtsi
@@ -5,6 +5,37 @@
 
 #include <dt-bindings/spmi/spmi.h>
 
+/ {
+	thermal-zones {
+		pm6350-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+
+			thermal-sensors = <&pm6350_temp>;
+
+			trips {
+				trip0 {
+					temperature = <95000>;
+					hysteresis = <0>;
+					type = "passive";
+				};
+
+				trip1 {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "hot";
+				};
+
+				trip2 {
+					temperature = <145000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+	};
+};
+
 &spmi_bus {
 	pmic@0 {
 		compatible = "qcom,pm6350", "qcom,spmi-pmic";
@@ -35,6 +66,13 @@ pm6350_resin: resin {
 			};
 		};
 
+		pm6350_temp: temp-alarm@2400 {
+			compatible = "qcom,spmi-temp-alarm";
+			reg = <0x2400>;
+			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_BOTH>;
+			#thermal-sensor-cells = <0>;
+		};
+
 		pm6350_gpios: gpios@c000 {
 			compatible = "qcom,pm6350-gpio";
 			reg = <0xc000>;
-- 
2.37.1

Re: [PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[Krzysztof Kozlowski]

On 12/08/2022 14:44, Luca Weiss wrote:
> Add temp-alarm device tree node and a default configuration for the
> corresponding thermal zone for this PMIC. Temperatures are based on
> downstream values.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
> With this config I'm getting this in dmesg, not sure if it's a warning
> that should be solved or just an informative warning.
> 
> [    0.268256] spmi-temp-alarm c440000.spmi:pmic@0:temp-alarm@2400: No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140 C! Configuring stage 2 shutdown at 140 C.
> 
> As far as I can tell, based on downstream dts this PMIC doesn't have an
> ADC.

You configure 145 and driver believes 140 is the limit, so it seems
warning should be addressed.

From where did you get 145 degrees as limit? Downstream DTS?

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[Luca Weiss]

Hi Krzysztof,

+CC Matthias Kaehlcke (author of patch mentioned further below)

On Fri Aug 12, 2022 at 3:36 PM CEST, Krzysztof Kozlowski wrote:
> On 12/08/2022 14:44, Luca Weiss wrote:
> > Add temp-alarm device tree node and a default configuration for the
> > corresponding thermal zone for this PMIC. Temperatures are based on
> > downstream values.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> > With this config I'm getting this in dmesg, not sure if it's a warning
> > that should be solved or just an informative warning.
> > 
> > [    0.268256] spmi-temp-alarm c440000.spmi:pmic@0:temp-alarm@2400: No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140 C! Configuring stage 2 shutdown at 140 C.
> > 
> > As far as I can tell, based on downstream dts this PMIC doesn't have an
> > ADC.
>
> You configure 145 and driver believes 140 is the limit, so it seems
> warning should be addressed.

Hm...

>
> From where did you get 145 degrees as limit? Downstream DTS?

Yes, downstream dts[0].

From what I can see in the downstream driver, it always disabled this
"software override of stage 2 and 3 shutdowns"[1]

In mainline only since f1599f9e4cd6 ("thermal: qcom-spmi: Use PMIC
thermal stage 2 for critical trip points") this check exists, which is
not part of downstream (wasn't in 4.19 yet), where this software
override tries to get enabled so that thermal core can handle this.

Any suggestion what I can do here? Maybe looking at msm-5.4 sources (and
associated dts) might reveal something..? Maybe newer SoCs/PMICs have a
different config?

Regards
Luca

[0] https://android.googlesource.com/kernel/msm-extra/devicetree/+/refs/heads/android-msm-barbet-4.19-android12L/qcom/pm6350.dtsi#126
[1] https://gerrit-public.fairphone.software/plugins/gitiles/kernel/msm-4.19/+/refs/heads/kernel/11/fp4/drivers/thermal/qcom-spmi-temp-alarm.c#251

>
> Best regards,
> Krzysztof

Re: [PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[Matthias Kaehlcke]

On Fri, Aug 12, 2022 at 04:06:47PM +0200, Luca Weiss wrote:
> Hi Krzysztof,
> 
> +CC Matthias Kaehlcke (author of patch mentioned further below)
> 
> On Fri Aug 12, 2022 at 3:36 PM CEST, Krzysztof Kozlowski wrote:
> > On 12/08/2022 14:44, Luca Weiss wrote:
> > > Add temp-alarm device tree node and a default configuration for the
> > > corresponding thermal zone for this PMIC. Temperatures are based on
> > > downstream values.
> > > 
> > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > ---
> > > With this config I'm getting this in dmesg, not sure if it's a warning
> > > that should be solved or just an informative warning.
> > > 
> > > [    0.268256] spmi-temp-alarm c440000.spmi:pmic@0:temp-alarm@2400: No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140 C! Configuring stage 2 shutdown at 140 C.
> > > 
> > > As far as I can tell, based on downstream dts this PMIC doesn't have an
> > > ADC.

I don't seem to have access to the datasheet, in any case that the ADC isn't
configured in the downstream dts doesn't necessarily mean the PMIC doesn't
have one. The PM6150 has one, and it is probably relatively close to the
PM6350.

> > You configure 145 and driver believes 140 is the limit, so it seems
> > warning should be addressed.
> 
> Hm...
> 
> >
> > From where did you get 145 degrees as limit? Downstream DTS?
> 
> Yes, downstream dts[0].
> 
> From what I can see in the downstream driver, it always disabled this
> "software override of stage 2 and 3 shutdowns"[1]
> 
> In mainline only since f1599f9e4cd6 ("thermal: qcom-spmi: Use PMIC
> thermal stage 2 for critical trip points") this check exists, which is
> not part of downstream (wasn't in 4.19 yet), where this software
> override tries to get enabled so that thermal core can handle this.
> 
> Any suggestion what I can do here? Maybe looking at msm-5.4 sources (and
> associated dts) might reveal something..?

I wouldn't necessarily consider QC downstream code as a reliable source of
truth ...

> Maybe newer SoCs/PMICs have a different config?

Commit aa92b3310c55 ("thermal/drivers/qcom-spmi-temp-alarm: Add support
for GEN2 rev 1 PMIC peripherals") added support for gen2 PMICs, which
actually have lower thresholds than gen1. From the log it seems that the
PM6350 is identified as gen1 device (max stage 2 threshold = 140 degC).

It seems setting the limit to 140 degC or one of the other stage 2
thresholds would be a reasonable course of action. stage 2 is the
threshold at which the PMIC is so hot that the system should shut
down, and 140 degC is the highest of the stage 2 thresholds. Even
if it was possible, what would be gained from setting the trip
point 5 degC higher?

Re: [PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[Konrad Dybcio]

On 12.08.2022 18:49, Matthias Kaehlcke wrote:
> On Fri, Aug 12, 2022 at 04:06:47PM +0200, Luca Weiss wrote:
>> Hi Krzysztof,
>>
>> +CC Matthias Kaehlcke (author of patch mentioned further below)
>>
>> On Fri Aug 12, 2022 at 3:36 PM CEST, Krzysztof Kozlowski wrote:
>>> On 12/08/2022 14:44, Luca Weiss wrote:
>>>> Add temp-alarm device tree node and a default configuration for the
>>>> corresponding thermal zone for this PMIC. Temperatures are based on
>>>> downstream values.
>>>>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>> With this config I'm getting this in dmesg, not sure if it's a warning
>>>> that should be solved or just an informative warning.
>>>>
>>>> [    0.268256] spmi-temp-alarm c440000.spmi:pmic@0:temp-alarm@2400: No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140 C! Configuring stage 2 shutdown at 140 C.
>>>>
>>>> As far as I can tell, based on downstream dts this PMIC doesn't have an
>>>> ADC.
> 
> I don't seem to have access to the datasheet, in any case that the ADC isn't
> configured in the downstream dts doesn't necessarily mean the PMIC doesn't
> have one. The PM6150 has one, and it is probably relatively close to the
> PM6350.
> 
>>> You configure 145 and driver believes 140 is the limit, so it seems
>>> warning should be addressed.
>>
>> Hm...
>>
>>>
>>> From where did you get 145 degrees as limit? Downstream DTS?
>>
>> Yes, downstream dts[0].
>>
>> From what I can see in the downstream driver, it always disabled this
>> "software override of stage 2 and 3 shutdowns"[1]
>>
>> In mainline only since f1599f9e4cd6 ("thermal: qcom-spmi: Use PMIC
>> thermal stage 2 for critical trip points") this check exists, which is
>> not part of downstream (wasn't in 4.19 yet), where this software
>> override tries to get enabled so that thermal core can handle this.
>>
>> Any suggestion what I can do here? Maybe looking at msm-5.4 sources (and
>> associated dts) might reveal something..?
> 
> I wouldn't necessarily consider QC downstream code as a reliable source of
> truth ...
> 
>> Maybe newer SoCs/PMICs have a different config?
> 
> Commit aa92b3310c55 ("thermal/drivers/qcom-spmi-temp-alarm: Add support
> for GEN2 rev 1 PMIC peripherals") added support for gen2 PMICs, which
> actually have lower thresholds than gen1. From the log it seems that the
> PM6350 is identified as gen1 device (max stage 2 threshold = 140 degC).
> 
> It seems setting the limit to 140 degC or one of the other stage 2
> thresholds would be a reasonable course of action. stage 2 is the
> threshold at which the PMIC is so hot that the system should shut
> down, and 140 degC is the highest of the stage 2 thresholds. Even
> if it was possible, what would be gained from setting the trip
> point 5 degC higher?
Eeeh, if it ran at anything near that for prolonged time, the heat
would likely spread around the device causing the battery to
eventually combust.. Qualcomm DTs say one thing, but vendor
userspace daemons are far more conservative. Not sure if they're
still around (should be iirc), but I would definitely
object allowing anything inside [mainly] mobile devices to reach
that kind of temperature..

Konrad

Re: [PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[Luca Weiss]

Hi Matthias,

sorry for the delay in getting back to you.

On Fri Aug 12, 2022 at 6:49 PM CEST, Matthias Kaehlcke wrote:
> On Fri, Aug 12, 2022 at 04:06:47PM +0200, Luca Weiss wrote:
> > Hi Krzysztof,
> > 
> > +CC Matthias Kaehlcke (author of patch mentioned further below)
> > 
> > On Fri Aug 12, 2022 at 3:36 PM CEST, Krzysztof Kozlowski wrote:
> > > On 12/08/2022 14:44, Luca Weiss wrote:
> > > > Add temp-alarm device tree node and a default configuration for the
> > > > corresponding thermal zone for this PMIC. Temperatures are based on
> > > > downstream values.
> > > > 
> > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > > ---
> > > > With this config I'm getting this in dmesg, not sure if it's a warning
> > > > that should be solved or just an informative warning.
> > > > 
> > > > [    0.268256] spmi-temp-alarm c440000.spmi:pmic@0:temp-alarm@2400: No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140 C! Configuring stage 2 shutdown at 140 C.
> > > > 
> > > > As far as I can tell, based on downstream dts this PMIC doesn't have an
> > > > ADC.
>
> I don't seem to have access to the datasheet, in any case that the ADC isn't
> configured in the downstream dts doesn't necessarily mean the PMIC doesn't
> have one. The PM6150 has one, and it is probably relatively close to the
> PM6350.

Too bad :(

>
> > > You configure 145 and driver believes 140 is the limit, so it seems
> > > warning should be addressed.
> > 
> > Hm...
> > 
> > >
> > > From where did you get 145 degrees as limit? Downstream DTS?
> > 
> > Yes, downstream dts[0].
> > 
> > From what I can see in the downstream driver, it always disabled this
> > "software override of stage 2 and 3 shutdowns"[1]
> > 
> > In mainline only since f1599f9e4cd6 ("thermal: qcom-spmi: Use PMIC
> > thermal stage 2 for critical trip points") this check exists, which is
> > not part of downstream (wasn't in 4.19 yet), where this software
> > override tries to get enabled so that thermal core can handle this.
> > 
> > Any suggestion what I can do here? Maybe looking at msm-5.4 sources (and
> > associated dts) might reveal something..?
>
> I wouldn't necessarily consider QC downstream code as a reliable source of
> truth ...
>
> > Maybe newer SoCs/PMICs have a different config?
>
> Commit aa92b3310c55 ("thermal/drivers/qcom-spmi-temp-alarm: Add support
> for GEN2 rev 1 PMIC peripherals") added support for gen2 PMICs, which
> actually have lower thresholds than gen1. From the log it seems that the
> PM6350 is identified as gen1 device (max stage 2 threshold = 140 degC).

PM6350 is detected as QPNP_TM_SUBTYPE_GEN2 so gen2 actually. Just the
log message is hardcoded to 140 degC, the if above actually has
stage2_threshold_max = 125000 (125degC) and stage2_threshold_min =
110000 (110degC) so lower than 140 (basically like you said).

>
> It seems setting the limit to 140 degC or one of the other stage 2
> thresholds would be a reasonable course of action. stage 2 is the
> threshold at which the PMIC is so hot that the system should shut
> down, and 140 degC is the highest of the stage 2 thresholds. Even
> if it was possible, what would be gained from setting the trip
> point 5 degC higher?

Based on this, do you think it's reasonable to just set the limit to
125 degC and be done with it? Or some other way to resolve this? I'd of
course mention in the commit message that I've decreased the value from
145 (msm-4.19) to 125.

Regards
Luca

Re: [PATCH] arm64: dts: qcom: pm6350: add temp sensor and thermal zone config

[Matthias Kaehlcke]

Hi Luca,

On Thu, Oct 20, 2022 at 04:28:07PM +0200, Luca Weiss wrote:
> Hi Matthias,
> 
> sorry for the delay in getting back to you.
> 
> On Fri Aug 12, 2022 at 6:49 PM CEST, Matthias Kaehlcke wrote:
> > On Fri, Aug 12, 2022 at 04:06:47PM +0200, Luca Weiss wrote:
> > > Hi Krzysztof,
> > > 
> > > +CC Matthias Kaehlcke (author of patch mentioned further below)
> > > 
> > > On Fri Aug 12, 2022 at 3:36 PM CEST, Krzysztof Kozlowski wrote:
> > > > On 12/08/2022 14:44, Luca Weiss wrote:
> > > > > Add temp-alarm device tree node and a default configuration for the
> > > > > corresponding thermal zone for this PMIC. Temperatures are based on
> > > > > downstream values.
> > > > > 
> > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > > > ---
> > > > > With this config I'm getting this in dmesg, not sure if it's a warning
> > > > > that should be solved or just an informative warning.
> > > > > 
> > > > > [    0.268256] spmi-temp-alarm c440000.spmi:pmic@0:temp-alarm@2400: No ADC is configured and critical temperature is above the maximum stage 2 threshold of 140 C! Configuring stage 2 shutdown at 140 C.
> > > > > 
> > > > > As far as I can tell, based on downstream dts this PMIC doesn't have an
> > > > > ADC.
> >
> > I don't seem to have access to the datasheet, in any case that the ADC isn't
> > configured in the downstream dts doesn't necessarily mean the PMIC doesn't
> > have one. The PM6150 has one, and it is probably relatively close to the
> > PM6350.
> 
> Too bad :(
> 
> >
> > > > You configure 145 and driver believes 140 is the limit, so it seems
> > > > warning should be addressed.
> > > 
> > > Hm...
> > > 
> > > >
> > > > From where did you get 145 degrees as limit? Downstream DTS?
> > > 
> > > Yes, downstream dts[0].
> > > 
> > > From what I can see in the downstream driver, it always disabled this
> > > "software override of stage 2 and 3 shutdowns"[1]
> > > 
> > > In mainline only since f1599f9e4cd6 ("thermal: qcom-spmi: Use PMIC
> > > thermal stage 2 for critical trip points") this check exists, which is
> > > not part of downstream (wasn't in 4.19 yet), where this software
> > > override tries to get enabled so that thermal core can handle this.
> > > 
> > > Any suggestion what I can do here? Maybe looking at msm-5.4 sources (and
> > > associated dts) might reveal something..?
> >
> > I wouldn't necessarily consider QC downstream code as a reliable source of
> > truth ...
> >
> > > Maybe newer SoCs/PMICs have a different config?
> >
> > Commit aa92b3310c55 ("thermal/drivers/qcom-spmi-temp-alarm: Add support
> > for GEN2 rev 1 PMIC peripherals") added support for gen2 PMICs, which
> > actually have lower thresholds than gen1. From the log it seems that the
> > PM6350 is identified as gen1 device (max stage 2 threshold = 140 degC).
> 
> PM6350 is detected as QPNP_TM_SUBTYPE_GEN2 so gen2 actually. Just the
> log message is hardcoded to 140 degC, the if above actually has
> stage2_threshold_max = 125000 (125degC) and stage2_threshold_min =
> 110000 (110degC) so lower than 140 (basically like you said).

Good to know.

> >
> > It seems setting the limit to 140 degC or one of the other stage 2
> > thresholds would be a reasonable course of action. stage 2 is the
> > threshold at which the PMIC is so hot that the system should shut
> > down, and 140 degC is the highest of the stage 2 thresholds. Even
> > if it was possible, what would be gained from setting the trip
> > point 5 degC higher?
> 
> Based on this, do you think it's reasonable to just set the limit to
> 125 degC and be done with it? Or some other way to resolve this? I'd of
> course mention in the commit message that I've decreased the value from
> 145 (msm-4.19) to 125.

Yes, setting it to 125°C or one of the other stage 2 threshold values for
gen2 sounds good to me.