Re: [PATCH] arm64: dts: qcom: pm6150: Add thermal zone for PMIC on-die temperature

From: Matthias Kaehlcke <mka@chromium.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>, Stephen Boyd <swboyd@chromium.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH] arm64: dts: qcom: pm6150: Add thermal zone for PMIC on-die temperature
Date: Wed, 2 Jun 2021 09:44:38 -0700	[thread overview]
Message-ID: <YLe1dmEWq9+MjfTR@google.com> (raw)
In-Reply-To: <CAD=FV=XcoLLCn+H6h9cX+qPjoueVFgK8BssvUXQ6WjVAapRyMg@mail.gmail.com>

Hi Doug,

On Wed, Jun 02, 2021 at 09:36:07AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 2, 2021 at 9:06 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Add a thermal zone for the pm6150 on-die temperature. The system should
> > try to shut down orderly when the temperature reaches 95degC, otherwise
> > the PMIC will power off at 115degC.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >
> >  arch/arm64/boot/dts/qcom/pm6150.dtsi | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/pm6150.dtsi b/arch/arm64/boot/dts/qcom/pm6150.dtsi
> > index 8ab4f1f78bbf..de7fb129f739 100644
> > --- a/arch/arm64/boot/dts/qcom/pm6150.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/pm6150.dtsi
> > @@ -7,6 +7,30 @@
> >  #include <dt-bindings/spmi/spmi.h>
> >  #include <dt-bindings/thermal/thermal.h>
> >
> > +/ {
> > +       thermal-zones {
> > +               pm6150_thermal: pm6150-thermal {
> > +                       polling-delay-passive = <100>;
> > +                       polling-delay = <0>;
> > +                       thermal-sensors = <&pm6150_temp>;
> > +
> > +                       trips {
> > +                               pm6150_trip0: trip0 {
> > +                                       temperature = <95000>;
> > +                                       hysteresis = <0>;
> > +                                       type = "passive";
> > +                               };
> > +
> > +                               pm6150_crit: pm6150-crit {
> 
> Nit that the node names are not symmetric. One is "trip0" and the
> other is "pm6510-crit". Seems like you can remove the "pm6150-" prefix
> from this one (but keep it in the label?)

Sounds good.

> > +                                       temperature = <115000>;
> > +                                       hysteresis = <0>;
> > +                                       type = "critical";
> > +                               };
> > +                       };
> > +               };
> 
> Correct me if I'm misunderstanding, but I don't think that the
> description of this patch matches the contents. You're saying that the
> PMIC will power itself off at 115C and that we want to do an orderly
> shutdown _before_ the PMIC powers off. Doesn't that mean that the
> "critical" trip needs to be at some temperature _lower_ than 115C? As
> I remember it the system performs an orderly shutdown starting when it
> sees the critical temperature.

Yeah, you are right. Polling starts at 95degC, the system should try to
shutdown at 115degC and based on that configuration the emergency
shutdown would happen at 145degC:

  static const long temp_map_gen2_v1[THRESH_COUNT][STAGE_COUNT] = {
        {  90000, 110000, 140000 },
  	{  95000, 115000, 145000 },
  	{ 100000, 120000, 150000 },
  	{ 105000, 125000, 155000 },
  };

I'll fix the commit message in the v2.