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

[Matthias Kaehlcke <mka@chromium.org>]

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.