On Mon, Jul 02, 2018 at 12:53:44PM -0700, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 2, 2018 at 11:10 AM, Matthias Kaehlcke <m...@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 raised 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 <m...@chromium.org>
> > ---
> > 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
>
> I'm not sure this was right. I guess you're trying to avoid
> Temperature Stage 2?
Indeed
> From Davi'd email in response to v1:
>
> > The PMIC TEMP_ALARM hardware peripheral will perform an automatic partial
> > PMIC shutdown upon hitting over-temperature stage 2 (125 C). This turns
> > off peripherals within the PMIC that are expected to draw significant
> > current. The set of peripherals included varies between PMICs. This
> > partial shutdown will occur simultaneously with the triggering of an
> > interrupt to the APPS processor that informs the qcom-spmi-temp-alarm
> > driver that an over-temperature threshold has been crossed.
>
> I think it's actually OK to use Temperature Stage 2 as the "critical"
> point, which is why it still interrupts the CPU. At "critical" the
> system will shut down, right? ...so presumably it's OK if the drivers
> can't recover from having the power yanked out from underneath them as
> long as they don't hang/crash the system in this case. If I had to
> guess the whole point of this stage is to give the system shutdown a
> better chance of succeeding without getting to stage 3.
That was my starting point, however in my tests the system reset
several times when the temperature got close to 125°C, not allowing
for a proper shutdown. Apparently the partial shutdown of the PMIC can
result in a full reset at least on some systems.
> I do agree, however, that removing the "145" from the device tree was
> the right thing to do since software will never see that. The system
> will just shut down.
>
>
> > - reduced number of trip points to 2
> > - lowered temperature of passive trip point
>
> This won't actually do anything until the ADC gets hooked up, right?
Correct
> I guess I would have expected:
> - 105: passive
> - 125: critical
>
> ...and then we could add (if we wanted) a "hot" between passive and
> critical once we have the ADC hooked up?
Exactly, once we have more granularity we could add (an)other trip point(s).
> > - 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 2f4989e7ef68..e7caa334e6c7 100644
> > --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > @@ -3,6 +3,7 @@
> >
> > #include <dt-bindings/spmi/spmi.h>
> > #include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/thermal/thermal.h>
> >
> > &spmi_bus {
> > pm8998_lsid0: pmic@0 {
> > @@ -60,3 +61,27 @@
> > #size-cells = <0>;
> > };
> > };
> > +
> > +/ {
> > + 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";
> > + };
> > + };
> > + };
> > + };
> > +};
>
> A nit, but I think convention is to actually put additions straight to
> the root node before reference to phandles, so I would have put this
> above the "&spmi_bus" part.
Ok, thanks, will do in v3
