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

[Doug Anderson]

Hi,

On Tue, Jul 17, 2018 at 2:08 PM, Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 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

=====

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.

===

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...


-Doug