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

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

[Matthias Kaehlcke]

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
 
 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";
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
@@ -37,7 +37,9 @@
 #define STATUS_GEN2_STATE_MASK		GENMASK(6, 4)
 #define STATUS_GEN2_STATE_SHIFT		4
 
-#define SHUTDOWN_CTRL1_OVERRIDE_MASK	GENMASK(7, 6)
+#define SHUTDOWN_CTRL1_OVERRIDE_S2	BIT(6)
+#define SHUTDOWN_CTRL1_OVERRIDE_S2_MASK	GENMASK(6, 6)
+#define SHUTDOWN_CTRL1_OVERRIDE_S3_MASK	GENMASK(7, 7)
 #define SHUTDOWN_CTRL1_THRESHOLD_MASK	GENMASK(1, 0)
 
 #define ALARM_CTRL_FORCE_ENABLE		BIT(7)
@@ -198,7 +200,8 @@ static irqreturn_t qpnp_tm_isr(int irq, void *data)
  * current thermal stage and threshold. Setup threshold control and
  * disable shutdown override.
  */
-static int qpnp_tm_init(struct qpnp_tm_chip *chip)
+static int qpnp_tm_init(struct qpnp_tm_chip *chip,
+			bool disable_stage2_shutdown)
 {
 	unsigned int stage;
 	int ret;
@@ -224,13 +227,18 @@ static int qpnp_tm_init(struct qpnp_tm_chip *chip)
 			     (stage - 1) * TEMP_STAGE_STEP +
 			     TEMP_THRESH_MIN;
 
-	/*
-	 * Set threshold and disable software override of stage 2 and 3
-	 * shutdowns.
-	 */
+	/* Set threshold and disable software override of stage 3 shutdown. */
 	chip->thresh = THRESH_MIN;
-	reg &= ~(SHUTDOWN_CTRL1_OVERRIDE_MASK | SHUTDOWN_CTRL1_THRESHOLD_MASK);
+	reg &= ~(SHUTDOWN_CTRL1_OVERRIDE_S3_MASK |
+		 SHUTDOWN_CTRL1_THRESHOLD_MASK);
 	reg |= chip->thresh & SHUTDOWN_CTRL1_THRESHOLD_MASK;
+
+	/* Disable stage 2 shutdown if requested */
+	if (disable_stage2_shutdown)
+		reg |= SHUTDOWN_CTRL1_OVERRIDE_S2;
+	else
+		reg &= ~SHUTDOWN_CTRL1_OVERRIDE_S2_MASK;
+
 	ret = qpnp_tm_write(chip, QPNP_TM_REG_SHUTDOWN_CTRL1, reg);
 	if (ret < 0)
 		return ret;
@@ -248,6 +256,7 @@ static int qpnp_tm_probe(struct platform_device *pdev)
 	struct device_node *node;
 	u8 type, subtype;
 	u32 res;
+	bool stage2_shutdown_disabled;
 	int ret, irq;
 
 	node = pdev->dev.of_node;
@@ -302,7 +311,10 @@ static int qpnp_tm_probe(struct platform_device *pdev)
 
 	chip->subtype = subtype;
 
-	ret = qpnp_tm_init(chip);
+	stage2_shutdown_disabled = of_property_read_bool(node,
+						"stage2-shutdown-disabled");
+
+	ret = qpnp_tm_init(chip, stage2_shutdown_disabled);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "init failed\n");
 		return ret;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH v4 2/3] arm64: dts: qcom: pm8998: Add spmi-temp-alarm node

[Matthias Kaehlcke]

This adds the spmi-temp-alarm node to pm8998 based on the examples in the
bindings.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- none

Changes in v3:
- changed node name from 'qcom,temp-alarm@2400' to 'temp-alarm@2400'
- removed controller register length value from 'reg'

Changes in v2:
- none
---
 arch/arm64/boot/dts/qcom/pm8998.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 92bed1e7d4bb..7eea94701b23 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -11,6 +11,13 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
+		pm8998_temp: temp-alarm@2400 {
+			compatible = "qcom,spmi-temp-alarm";
+			reg = <0x2400>;
+			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_RISING>;
+			#thermal-sensor-cells = <0>;
+		};
+
 		pm8998_gpio: gpios@c000 {
 			compatible = "qcom,pm8998-gpio", "qcom,spmi-gpio";
 			reg = <0xc000>;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH v4 3/3] arm64: dts: qcom: pm8998: Add pm8998 thermal zone

[Matthias Kaehlcke]

The thermal zone uses spmi-temp-alarm as sensor. The trip points
correspond to stage 1 and 2 temperatures. Stage 2 shutdown is disabled,
instead the stage 2 temperature is configured as critical trip point to
allow for an ordely shutdown.

Without an IIO input the sensor only reports a limited number of
temperatures:

- 37°C for temperatures below 105°C
- 107°C for temperatures >= 105°C and < 125°C
- 127°C for temperatures >= 125°C

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v4:
- updated trip point temperatures to match stage 1 and 2 ones
- disabled stage 2 shutdown
- updated commit message

Changes in v3:
- moved 'thermal-zones' node to the beginning of the .dtsi

Changes in v2:
- defined 'thermal-zones' node in pm8998.dtsi instead of using a label
  to refer to it
- use 105°C hardware trip point as critical trip point
- reduced number of trip points to 2
- lowered temperature of passive trip point
- updated trip point names and added labels
- updated commit message
---
 arch/arm64/boot/dts/qcom/pm8998.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 7eea94701b23..3bb179b85a44 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -3,6 +3,31 @@
 
 #include <dt-bindings/spmi/spmi.h>
 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/thermal/thermal.h>
+
+/ {
+	thermal-zones {
+		pm8998 {
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+
+			thermal-sensors = <&pm8998_temp>;
+
+			trips {
+				pm8998_alert0: pm8998-alert0 {
+					temperature = <105000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				pm8998_crit: pm8998-crit {
+					temperature = <125000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+	};
+};
 
 &spmi_bus {
 	pm8998_lsid0: pmic@0 {
@@ -15,6 +40,7 @@
 			compatible = "qcom,spmi-temp-alarm";
 			reg = <0x2400>;
 			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_RISING>;
+			stage2-shutdown-disabled;
 			#thermal-sensor-cells = <0>;
 		};
 
-- 
2.18.0.203.gfac676dfb9-goog

Re: [PATCH v4 2/3] arm64: dts: qcom: pm8998: Add spmi-temp-alarm node

[Matthias Kaehlcke]

On Tue, Jul 17, 2018 at 02:08:14PM -0700, Matthias Kaehlcke wrote:
> This adds the spmi-temp-alarm node to pm8998 based on the examples in the
> bindings.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

I should have added:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

> ---
> Changes in v4:
> - none
> 
> Changes in v3:
> - changed node name from 'qcom,temp-alarm@2400' to 'temp-alarm@2400'
> - removed controller register length value from 'reg'
> 
> Changes in v2:
> - none
> ---
>  arch/arm64/boot/dts/qcom/pm8998.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> index 92bed1e7d4bb..7eea94701b23 100644
> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> @@ -11,6 +11,13 @@
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> +		pm8998_temp: temp-alarm@2400 {
> +			compatible = "qcom,spmi-temp-alarm";
> +			reg = <0x2400>;
> +			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_RISING>;
> +			#thermal-sensor-cells = <0>;
> +		};
> +
>  		pm8998_gpio: gpios@c000 {
>  			compatible = "qcom,pm8998-gpio", "qcom,spmi-gpio";
>  			reg = <0xc000>;

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

=====

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

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

[Matthias Kaehlcke]

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!