Re: [PATCH v4 1/3] thermal: qcom-spmi: Allow to disable stage 2 shutdown

[Matthias Kaehlcke <mka@chromium.org>]

On Fri, Jul 20, 2018 at 11:42:40AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Jul 17, 2018 at 2:08 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > When the temperature reaches stage 2 the PMIC performs by default a
> > 'partial shutdown', unless software override is enabled. It is not well
> > defined which peripherals are affected by a 'partial shutdown'. Drivers
> > might be unhappy when their devices suddenly disappear and prevent an
> > orderly shutdown.
> >
> > Add an optional device tree property that allows to disable stage 2
> > shutdown.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v4:
> > - patch added to the series
> > ---
> >  .../bindings/thermal/qcom-spmi-temp-alarm.txt |  3 ++
> >  drivers/thermal/qcom-spmi-temp-alarm.c        | 28 +++++++++++++------
> >  2 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> > index 290ec06fa33a..377c94fa1821 100644
> > --- a/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> > +++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt
> > @@ -15,6 +15,8 @@ Optional properties:
> >  - io-channels:     Should contain IIO channel specifier for the ADC channel,
> >                     which report chip die temperature.
> >  - io-channel-names: Should contain "thermal".
> > +- stage2-shutdown-disabled: boolean to disable a partial shutdown of the PMIC
> > +                            when the temperature reaches stage 2
> 
> With my understanding of device tree I believe that this should be
> "qcom,stage2-shutdown-disabled".  If something isn't a generic
> property affecting a whole group of bindings I believe it's supposed
> to have a company prefix.
> 
> From the device tree specification (wow, this exists now?  ...and is public?)
> 
> https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2
> 
> I find:
> 
> > Nonstandard property names should specify a unique string prefix, such
> > as a stock ticker symbol, identifying the name of the company or
> > organization that defined the property. Examples:
>   fsl,channel-fifo-len
>   ibm,ppc-interrupt-server#s
>   linux,network-index

Ok, I'll update the property name in case we keep using one.

> =====
> 
> I would also perhaps add a bit more verbose justification to your
> description of this property.  Specifically there should be some
> indication in the bindings doc of when you'd want this property.  From
> what David Collins said, I'd maybe write something like:
> 
> ---
> 
> You want to disable the partial "stage 2" shutdown any time you've
> properly defined thermal zones in such a way that the OS will try to
> shut down at that stage anyway.  Said another way, there are three
> thermal stages defined in the PMIC:
> 
> stage 1: warning
> stage 2: system should shut down
> stage 3: emergency shut down
> 
> By default the PMIC assumes that the OS isn't doing anything and thus
> at stage 2 it does the partial PMIC shutdown and at stage 3 it kills
> all power.  If we have thermal zones defined such that the OS will
> initiate the shutdown at stage 2 then we want to disable the PMIC's
> automatic mode for stage 2.  That still leaves us with the emergency
> at stage 3.

Thanks, I'll add something along these lines (or bluntly copy them ;-)

> ===
> 
> Actually, after writing all the above I wonder if perhaps there's a
> way to do this all automatically.  AKA: if someone has specified a
> "critical" level can we automatically disable the partial PMIC
> shutdown and we don't need the extra property?  Bonus points if we can
> check to see if the thermal zones defined actually match reality and
> even more bonus points if you can automatically adjust the settings in
> the PMIC based on the thermal zones...
> 
> I did find that in "struct thermal_zone_of_device_ops" there appears
> to be a "set_trip_temp" function you can fill in.  When I tried that
> quickly I saw that I got called when I echoed into
> "/sys/class/thermal/thermal_zone0/trip_point_1_temp" but not at
> bootup, but I didn't debug more than that...
> 
> 
> >  Example:
> >
> > @@ -23,6 +25,7 @@ Example:
> >                 reg = <0x2400 0x100>;
> >                 interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>;
> >                 #thermal-sensor-cells = <0>;
> > +               stage2-shutdown-disabled;
> >
> >                 io-channels = <&pm8941_vadc VADC_DIE_TEMP>;
> >                 io-channel-names = "thermal";
> 
> BTW: can you update the "example" in this file?  Based on the PMIC
> docs I saw it's likely that it should be:
> 
> stage1 {
>   temperature = <1050000>;
>   hysteresis = <2000>;
>   type = "passive";
> };
> stage2 {
>   temperature = <125000>;
>   hysteresis = <2000>;
>   type = "critical";
> };
> 
> ...and I guess we don't specify the final one because by that time
> Linux is kaput.
> 
> 
> > diff --git a/drivers/thermal/qcom-spmi-temp-alarm.c b/drivers/thermal/qcom-spmi-temp-alarm.c
> > index ad4f3a8d6560..acbb0dbec79e 100644
> > --- a/drivers/thermal/qcom-spmi-temp-alarm.c
> > +++ b/drivers/thermal/qcom-spmi-temp-alarm.c
> 
> Sometimes people like bindings and code change to be separate patches.
> Maintainer can specify what's desired in this case, but I find it
> generally pleases more people to make them separate.
> 
> ...though I guess if we can figure out how to do this automatically we
> will have no bindings change...

Thanks for the suggestions, I'll look into them for the next version!