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

[PATCH v3 1/2] 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 v3:
- changed node name from 'qcom,temp-alarm at 2400' to 'temp-alarm at 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 at 2400 {
+			compatible = "qcom,spmi-temp-alarm";
+			reg = <0x2400>;
+			interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_RISING>;
+			#thermal-sensor-cells = <0>;
+		};
+
 		pm8998_gpio: gpios at c000 {
 			compatible = "qcom,pm8998-gpio", "qcom,spmi-gpio";
 			reg = <0xc000>;
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH v3 2/2] arm64: dts: qcom: pm8998: Add pm8998 thermal zone

[Matthias Kaehlcke]

The thermal zone uses spmi-temp-alarm as sensor. If the sensor is
configured without an IIO input it always reports 37?C for temperatures
below the first hardware trip point at 105?C. This hardware trip point
is configured as critical trip point, to initiate a system shutdown
before the temperature reaches the next hardware trip point at 125?C,
where the PMIC performs a partial shutdown.

The temperature of the critical trip point can be increased after
adding the die temperature ADC as IIO input for spmi-temp-alarm, which
significantly increases the precision of the temperature measurements.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
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 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 7eea94701b23..52c5e797aab2 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 = <95000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				pm8998_crit: pm8998-crit {
+					temperature = <105000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+	};
+};
 
 &spmi_bus {
 	pm8998_lsid0: pmic at 0 {
-- 
2.18.0.203.gfac676dfb9-goog

[PATCH v3 2/2] arm64: dts: qcom: pm8998: Add pm8998 thermal zone

[Doug Anderson]

Hi,

On Mon, Jul 9, 2018 at 2:25 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> The thermal zone uses spmi-temp-alarm as sensor. If the sensor is
> configured without an IIO input it always reports 37?C for temperatures
> below the first hardware trip point at 105?C. This hardware trip point
> is configured as critical trip point, to initiate a system shutdown
> before the temperature reaches the next hardware trip point at 125?C,
> where the PMIC performs a partial shutdown.
>
> The temperature of the critical trip point can be increased after
> adding the die temperature ADC as IIO input for spmi-temp-alarm, which
> significantly increases the precision of the temperature measurements.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 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 | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> index 7eea94701b23..52c5e797aab2 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 = <95000>;
> +                                       hysteresis = <2000>;
> +                                       type = "passive";
> +                               };
> +                               pm8998_crit: pm8998-crit {
> +                                       temperature = <105000>;
> +                                       hysteresis = <2000>;
> +                                       type = "critical";
> +                               };

IMHO since the 95000 won't trigger until we get the ADC, it's a bit
too extreme to go straight to shutdown when the PMIC reaches 105.  I
think we should take David Collins's advice and disable the "partial
PMIC shutdown" and then set the two stages to 105 (passive) / 125
(critical).  In theory we could add a 115 (hot) stage in there that
would only get triggered later when the ADC gets hooked up.

Personally I wouldn't land this patch until we do that.

-Doug

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

[Doug Anderson]

Hi,

On Mon, Jul 9, 2018 at 2:25 PM, Matthias Kaehlcke <mka@chromium.org> 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>
> ---
> Changes in v3:
> - changed node name from 'qcom,temp-alarm at 2400' to 'temp-alarm at 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(+)

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

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

[Stephen Boyd]

Quoting Matthias Kaehlcke (2018-07-09 14:25:21)
> 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 at 2400 {

Are we sorting this file based on node name or unit address? Otherwise
patch looks good.

> +                       compatible = "qcom,spmi-temp-alarm";
> +                       reg = <0x2400>;
> +                       interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_RISING>;
> +                       #thermal-sensor-cells = <0>;
> +               };
> +
>                 pm8998_gpio: gpios at c000 {
>                         compatible = "qcom,pm8998-gpio", "qcom,spmi-gpio";
>                         reg = <0xc000>;

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

[Doug Anderson]

Hi,

On Fri, Jul 13, 2018 at 9:00 AM, Stephen Boyd <swboyd@chromium.org> wrote:
> Quoting Matthias Kaehlcke (2018-07-09 14:25:21)
>> 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 at 2400 {
>
> Are we sorting this file based on node name or unit address? Otherwise
> patch looks good.

My preference would be:

* Anything where we're defining a new node with an address: sort by the address.

* If we are defining nodes without addresses: sort by node name (not by label!)

* In general I'd prefer to see addressless things before things with addresses.

* If we are overriding nodes that were defined in a parent makefile:
sort by the name of the parent label we refer to.


...so I think Matthias's sort order in this case is correct / consistent.


-Doug

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

[Matthias Kaehlcke]

On Fri, Jul 13, 2018 at 09:00:19AM -0700, Stephen Boyd wrote:
> Quoting Matthias Kaehlcke (2018-07-09 14:25:21)
> > 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 at 2400 {
> 
> Are we sorting this file based on node name or unit address? Otherwise
> patch looks good.

The idea was to sort by address, following the 'convention' of
sdm845.dtsi (https://patchwork.kernel.org/patch/10381745/).