| Message ID | 20180717210815.245639-1-mka@chromium.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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!
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;
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(-)