[PATCH V2 0/2] power: twl4030_charger: cleanup to handle various battery handling error conditions

[PATCH V2 0/2] power: twl4030_charger: cleanup to handle various battery handling error conditions

[Nishanth Menon]

Hi,

This issue was originally reported by Russell on OMAP3-LDP platform,
and original attempt to solve this by Felipe[1] did not quiet work,
follow on attempt[2] seems to be effective, but in this resend, I have
added an shutdown attempt in case things dont quiet look right.

Based on: v3.15-rc7

Tests:
ldp boot with battery: http://slexy.org/raw/s20StiV4xr
ldp boot with a/c charger, no battery: http://slexy.org/raw/s2UfjIkZKG
ldp boot with a/c charger+battery, remove battery: http://slexy.org/raw/s2yFTVlrWJ
	^^ <- this is not exactly a safe test to perform :)..

Nishanth Menon (2):
  power: twl4030_charger: detect battery presence prior to enabling    
    charger
  power: twl4030_charger: attempt to power off in case of critical
    events

 drivers/power/twl4030_charger.c |   70 +++++++++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 6 deletions(-)

[1] https://patchwork.kernel.org/patch/4002371/
[2] https://patchwork.kernel.org/patch/4124751/
-- 
1.7.9.5

[PATCH V2 1/2] power: twl4030_charger: detect battery presence prior to enabling charger

[Nishanth Menon]

TWL4030's Battery Charger seems to be designed for non-hotpluggable
batteries.

If battery is not present in the system, BATSTS is always set with the
expectation that software will take actions to move to a required safe
state (could be power down or disable various charger paths).

It does not seem possible even by manipulating the edge detection
of the event (using BCIEDR2 register) to have a consistent hotplug
handling. This seems to be the result of BATSTS interrupt generated
when the thermistor of the battery pack is disconnected from the
dedicated ADIN1 pin. Clearing the status just results in the status
being regenerated by the monitoring ADC(MADC) and disabling the
edges of event just makes hotplug no longer function. The only
other option is to disable the detection of the MADC by disabling
BCIMFEN4::BATSTSMCHGEN (battery presence detector) - but then, we can
never again detect battery reconnection.

So, detect battery presence based on precharge(which is hardware
automatic state) or default main charger configuration at the time of
probe and enable charger logic only if battery was present.

Reported-by: Russell King <linux@arm.linux.org.uk>
Tested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
V2: just a rebase, picked up tony's tested-by, minor formatting fix
V1: https://patchwork.kernel.org/patch/4124751/
 drivers/power/twl4030_charger.c |   44 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index f141088..2598c58 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -28,10 +28,13 @@
 #define TWL4030_BCIICHG		0x08
 #define TWL4030_BCIVAC		0x0a
 #define TWL4030_BCIVBUS		0x0c
+#define TWL4030_BCIMFSTS3	0x0F
 #define TWL4030_BCIMFSTS4	0x10
 #define TWL4030_BCICTL1		0x23
 #define TWL4030_BB_CFG		0x12
 
+#define TWL4030_BCIMFSTS1	0x01
+
 #define TWL4030_BCIAUTOWEN	BIT(5)
 #define TWL4030_CONFIG_DONE	BIT(4)
 #define TWL4030_BCIAUTOUSB	BIT(1)
@@ -52,6 +55,9 @@
 #define TWL4030_BBISEL_500uA	0x02
 #define TWL4030_BBISEL_1000uA	0x03
 
+#define TWL4030_BATSTSPCHG	BIT(2)
+#define TWL4030_BATSTSMCHG	BIT(6)
+
 /* BCI interrupts */
 #define TWL4030_WOVF		BIT(0) /* Watchdog overflow */
 #define TWL4030_TMOVF		BIT(1) /* Timer overflow */
@@ -145,6 +151,35 @@ static int twl4030bci_read_adc_val(u8 reg)
 }
 
 /*
+ * Check if Battery Pack was present
+ */
+static int twl4030_is_battery_present(struct twl4030_bci *bci)
+{
+	int ret;
+	u8 val = 0;
+
+	/* Battery presence in Main charge? */
+	ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE, &val, TWL4030_BCIMFSTS3);
+	if (ret)
+		return ret;
+	if (val & TWL4030_BATSTSMCHG)
+		return 0;
+
+	/*
+	 * OK, It could be that bootloader did not enable main charger,
+	 * pre-charge is h/w auto. So, Battery presence in Pre-charge?
+	 */
+	ret = twl_i2c_read_u8(TWL4030_MODULE_PRECHARGE, &val,
+			      TWL4030_BCIMFSTS1);
+	if (ret)
+		return ret;
+	if (val & TWL4030_BATSTSPCHG)
+		return 0;
+
+	return -ENODEV;
+}
+
+/*
  * Check if VBUS power is present
  */
 static int twl4030_bci_have_vbus(struct twl4030_bci *bci)
@@ -541,8 +576,14 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 	bci->irq_chg = platform_get_irq(pdev, 0);
 	bci->irq_bci = platform_get_irq(pdev, 1);
 
-	platform_set_drvdata(pdev, bci);
+	/* Only proceed further *IF* battery is physically present */
+	ret = twl4030_is_battery_present(bci);
+	if  (ret) {
+		dev_crit(&pdev->dev, "Battery was not detected:%d\n", ret);
+		goto fail_no_battery;
+	}
 
+	platform_set_drvdata(pdev, bci);
 	bci->ac.name = "twl4030_ac";
 	bci->ac.type = POWER_SUPPLY_TYPE_MAINS;
 	bci->ac.properties = twl4030_charger_props;
@@ -633,6 +674,7 @@ fail_chg_irq:
 fail_register_usb:
 	power_supply_unregister(&bci->ac);
 fail_register_ac:
+fail_no_battery:
 	kfree(bci);
 
 	return ret;
-- 
1.7.9.5

[PATCH V2 1/2] power: twl4030_charger: detect battery presence prior to enabling charger

[Tony Lindgren]

* Nishanth Menon <nm@ti.com> [140528 14:48]:
> TWL4030's Battery Charger seems to be designed for non-hotpluggable
> batteries.
> 
> If battery is not present in the system, BATSTS is always set with the
> expectation that software will take actions to move to a required safe
> state (could be power down or disable various charger paths).
> 
> It does not seem possible even by manipulating the edge detection
> of the event (using BCIEDR2 register) to have a consistent hotplug
> handling. This seems to be the result of BATSTS interrupt generated
> when the thermistor of the battery pack is disconnected from the
> dedicated ADIN1 pin. Clearing the status just results in the status
> being regenerated by the monitoring ADC(MADC) and disabling the
> edges of event just makes hotplug no longer function. The only
> other option is to disable the detection of the MADC by disabling
> BCIMFEN4::BATSTSMCHGEN (battery presence detector) - but then, we can
> never again detect battery reconnection.
> 
> So, detect battery presence based on precharge(which is hardware
> automatic state) or default main charger configuration at the time of
> probe and enable charger logic only if battery was present.
> 
> Reported-by: Russell King <linux@arm.linux.org.uk>
> Tested-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>

Dmitry, can we please get this first patch merged? This is needed
on some omap3 platforms for DT based booting when no battery is
present.

Only the second patch in this series is still being discussed AFAIK.

Regards,

Tony

> ---
> V2: just a rebase, picked up tony's tested-by, minor formatting fix
> V1: https://patchwork.kernel.org/patch/4124751/
>  drivers/power/twl4030_charger.c |   44 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
> index f141088..2598c58 100644
> --- a/drivers/power/twl4030_charger.c
> +++ b/drivers/power/twl4030_charger.c
> @@ -28,10 +28,13 @@
>  #define TWL4030_BCIICHG		0x08
>  #define TWL4030_BCIVAC		0x0a
>  #define TWL4030_BCIVBUS		0x0c
> +#define TWL4030_BCIMFSTS3	0x0F
>  #define TWL4030_BCIMFSTS4	0x10
>  #define TWL4030_BCICTL1		0x23
>  #define TWL4030_BB_CFG		0x12
>  
> +#define TWL4030_BCIMFSTS1	0x01
> +
>  #define TWL4030_BCIAUTOWEN	BIT(5)
>  #define TWL4030_CONFIG_DONE	BIT(4)
>  #define TWL4030_BCIAUTOUSB	BIT(1)
> @@ -52,6 +55,9 @@
>  #define TWL4030_BBISEL_500uA	0x02
>  #define TWL4030_BBISEL_1000uA	0x03
>  
> +#define TWL4030_BATSTSPCHG	BIT(2)
> +#define TWL4030_BATSTSMCHG	BIT(6)
> +
>  /* BCI interrupts */
>  #define TWL4030_WOVF		BIT(0) /* Watchdog overflow */
>  #define TWL4030_TMOVF		BIT(1) /* Timer overflow */
> @@ -145,6 +151,35 @@ static int twl4030bci_read_adc_val(u8 reg)
>  }
>  
>  /*
> + * Check if Battery Pack was present
> + */
> +static int twl4030_is_battery_present(struct twl4030_bci *bci)
> +{
> +	int ret;
> +	u8 val = 0;
> +
> +	/* Battery presence in Main charge? */
> +	ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE, &val, TWL4030_BCIMFSTS3);
> +	if (ret)
> +		return ret;
> +	if (val & TWL4030_BATSTSMCHG)
> +		return 0;
> +
> +	/*
> +	 * OK, It could be that bootloader did not enable main charger,
> +	 * pre-charge is h/w auto. So, Battery presence in Pre-charge?
> +	 */
> +	ret = twl_i2c_read_u8(TWL4030_MODULE_PRECHARGE, &val,
> +			      TWL4030_BCIMFSTS1);
> +	if (ret)
> +		return ret;
> +	if (val & TWL4030_BATSTSPCHG)
> +		return 0;
> +
> +	return -ENODEV;
> +}
> +
> +/*
>   * Check if VBUS power is present
>   */
>  static int twl4030_bci_have_vbus(struct twl4030_bci *bci)
> @@ -541,8 +576,14 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
>  	bci->irq_chg = platform_get_irq(pdev, 0);
>  	bci->irq_bci = platform_get_irq(pdev, 1);
>  
> -	platform_set_drvdata(pdev, bci);
> +	/* Only proceed further *IF* battery is physically present */
> +	ret = twl4030_is_battery_present(bci);
> +	if  (ret) {
> +		dev_crit(&pdev->dev, "Battery was not detected:%d\n", ret);
> +		goto fail_no_battery;
> +	}
>  
> +	platform_set_drvdata(pdev, bci);
>  	bci->ac.name = "twl4030_ac";
>  	bci->ac.type = POWER_SUPPLY_TYPE_MAINS;
>  	bci->ac.properties = twl4030_charger_props;
> @@ -633,6 +674,7 @@ fail_chg_irq:
>  fail_register_usb:
>  	power_supply_unregister(&bci->ac);
>  fail_register_ac:
> +fail_no_battery:
>  	kfree(bci);
>  
>  	return ret;
> -- 
> 1.7.9.5
> 

[PATCH V2 1/2] power: twl4030_charger: detect battery presence prior to enabling charger

[Sebastian Reichel]

On Wed, Jul 23, 2014 at 02:24:20AM -0700, Tony Lindgren wrote:
> * Nishanth Menon <nm@ti.com> [140528 14:48]:
> > TWL4030's Battery Charger seems to be designed for non-hotpluggable
> > batteries.
> > 
> > If battery is not present in the system, BATSTS is always set with the
> > expectation that software will take actions to move to a required safe
> > state (could be power down or disable various charger paths).
> > 
> > It does not seem possible even by manipulating the edge detection
> > of the event (using BCIEDR2 register) to have a consistent hotplug
> > handling. This seems to be the result of BATSTS interrupt generated
> > when the thermistor of the battery pack is disconnected from the
> > dedicated ADIN1 pin. Clearing the status just results in the status
> > being regenerated by the monitoring ADC(MADC) and disabling the
> > edges of event just makes hotplug no longer function. The only
> > other option is to disable the detection of the MADC by disabling
> > BCIMFEN4::BATSTSMCHGEN (battery presence detector) - but then, we can
> > never again detect battery reconnection.
> > 
> > So, detect battery presence based on precharge(which is hardware
> > automatic state) or default main charger configuration at the time of
> > probe and enable charger logic only if battery was present.
> > 
> > Reported-by: Russell King <linux@arm.linux.org.uk>
> > Tested-by: Tony Lindgren <tony@atomide.com>
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> 
> Dmitry, can we please get this first patch merged? This is needed
> on some omap3 platforms for DT based booting when no battery is
> present.
> 
> Only the second patch in this series is still being discussed AFAIK.

applied to battery-2.6.git:

http://git.infradead.org/battery-2.6.git/commit/61a7784efd3c89ffb6242f29bcee170dd7f55e6b

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140723/66e293c7/attachment.sig>

[PATCH V2 1/2] power: twl4030_charger: detect battery presence prior to enabling charger

[Tony Lindgren]

* Sebastian Reichel <sre@kernel.org> [140723 05:05]:
> On Wed, Jul 23, 2014 at 02:24:20AM -0700, Tony Lindgren wrote:
> > * Nishanth Menon <nm@ti.com> [140528 14:48]:
> > > TWL4030's Battery Charger seems to be designed for non-hotpluggable
> > > batteries.
> > > 
> > > If battery is not present in the system, BATSTS is always set with the
> > > expectation that software will take actions to move to a required safe
> > > state (could be power down or disable various charger paths).
> > > 
> > > It does not seem possible even by manipulating the edge detection
> > > of the event (using BCIEDR2 register) to have a consistent hotplug
> > > handling. This seems to be the result of BATSTS interrupt generated
> > > when the thermistor of the battery pack is disconnected from the
> > > dedicated ADIN1 pin. Clearing the status just results in the status
> > > being regenerated by the monitoring ADC(MADC) and disabling the
> > > edges of event just makes hotplug no longer function. The only
> > > other option is to disable the detection of the MADC by disabling
> > > BCIMFEN4::BATSTSMCHGEN (battery presence detector) - but then, we can
> > > never again detect battery reconnection.
> > > 
> > > So, detect battery presence based on precharge(which is hardware
> > > automatic state) or default main charger configuration at the time of
> > > probe and enable charger logic only if battery was present.
> > > 
> > > Reported-by: Russell King <linux@arm.linux.org.uk>
> > > Tested-by: Tony Lindgren <tony@atomide.com>
> > > Signed-off-by: Nishanth Menon <nm@ti.com>
> > 
> > Dmitry, can we please get this first patch merged? This is needed
> > on some omap3 platforms for DT based booting when no battery is
> > present.
> > 
> > Only the second patch in this series is still being discussed AFAIK.
> 
> applied to battery-2.6.git:
> 
> http://git.infradead.org/battery-2.6.git/commit/61a7784efd3c89ffb6242f29bcee170dd7f55e6b

Thanks!

Tony

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Nishanth Menon]

Attempt to power off in case of critical events such as battery removal,
over voltage events.

There is no guarentee that we'd be in a safe scenario here, but the very
least we can try to do is to power off the device to prevent damage to
the system instead of just printing a message and hoping for the best.

NOTE: twl4030 should attempt some form of recovery, but just depending
on that is never a safe alternative.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
new patch. original attempt was: https://patchwork.kernel.org/patch/4002371/

NOTE: we dont have poweroff support yet enabled on LDP, but it just needs
relevant dts entry.

 drivers/power/twl4030_charger.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 2598c58..ed0dbd2 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -22,6 +22,7 @@
 #include <linux/power_supply.h>
 #include <linux/notifier.h>
 #include <linux/usb/otg.h>
+#include <linux/reboot.h>
 #include <linux/regulator/machine.h>
 
 #define TWL4030_BCIMSTATEC	0x02
@@ -332,6 +333,7 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
 	struct twl4030_bci *bci = arg;
 	u8 irqs1, irqs2;
 	int ret;
+	bool power_off = false;
 
 	ret = twl_i2c_read_u8(TWL4030_MODULE_INTERRUPTS, &irqs1,
 			      TWL4030_INTERRUPTS_BCIISR1A);
@@ -352,20 +354,34 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
 	}
 
 	/* various monitoring events, for now we just log them here */
-	if (irqs1 & (TWL4030_TBATOR2 | TWL4030_TBATOR1))
+	if (irqs1 & (TWL4030_TBATOR2 | TWL4030_TBATOR1)) {
 		dev_warn(bci->dev, "battery temperature out of range\n");
+		power_off = true;
+	}
 
-	if (irqs1 & TWL4030_BATSTS)
+	if (irqs1 & TWL4030_BATSTS) {
 		dev_crit(bci->dev, "battery disconnected\n");
+		power_off = true;
+	}
 
-	if (irqs2 & TWL4030_VBATOV)
+	if (irqs2 & TWL4030_VBATOV) {
 		dev_crit(bci->dev, "VBAT overvoltage\n");
+		power_off = true;
+	}
 
-	if (irqs2 & TWL4030_VBUSOV)
+	if (irqs2 & TWL4030_VBUSOV) {
 		dev_crit(bci->dev, "VBUS overvoltage\n");
+		power_off = true;
+	}
 
-	if (irqs2 & TWL4030_ACCHGOV)
+	if (irqs2 & TWL4030_ACCHGOV) {
 		dev_crit(bci->dev, "Ac charger overvoltage\n");
+		power_off = true;
+	}
+
+	/* Try to shutdown the system */
+	if (power_off)
+		orderly_poweroff(true);
 
 	return IRQ_HANDLED;
 }
-- 
1.7.9.5

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Grazvydas Ignotas]

On Thu, May 29, 2014 at 12:46 AM, Nishanth Menon <nm@ti.com> wrote:
> Attempt to power off in case of critical events such as battery removal,
> over voltage events.
>
> There is no guarentee that we'd be in a safe scenario here, but the very
> least we can try to do is to power off the device to prevent damage to
> the system instead of just printing a message and hoping for the best.

At least "battery temperature out of range" does seem to happen quite
often while charging on hot summer day. I'd prefer my pandora to not
shutdown in such case, it could just stop charging instead.

>
> NOTE: twl4030 should attempt some form of recovery, but just depending
> on that is never a safe alternative.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> new patch. original attempt was: https://patchwork.kernel.org/patch/4002371/
>
> NOTE: we dont have poweroff support yet enabled on LDP, but it just needs
> relevant dts entry.
>
>  drivers/power/twl4030_charger.c |   26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
> index 2598c58..ed0dbd2 100644
> --- a/drivers/power/twl4030_charger.c
> +++ b/drivers/power/twl4030_charger.c
> @@ -22,6 +22,7 @@
>  #include <linux/power_supply.h>
>  #include <linux/notifier.h>
>  #include <linux/usb/otg.h>
> +#include <linux/reboot.h>
>  #include <linux/regulator/machine.h>
>
>  #define TWL4030_BCIMSTATEC     0x02
> @@ -332,6 +333,7 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
>         struct twl4030_bci *bci = arg;
>         u8 irqs1, irqs2;
>         int ret;
> +       bool power_off = false;
>
>         ret = twl_i2c_read_u8(TWL4030_MODULE_INTERRUPTS, &irqs1,
>                               TWL4030_INTERRUPTS_BCIISR1A);
> @@ -352,20 +354,34 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
>         }
>
>         /* various monitoring events, for now we just log them here */
> -       if (irqs1 & (TWL4030_TBATOR2 | TWL4030_TBATOR1))
> +       if (irqs1 & (TWL4030_TBATOR2 | TWL4030_TBATOR1)) {
>                 dev_warn(bci->dev, "battery temperature out of range\n");
> +               power_off = true;
> +       }
>
> -       if (irqs1 & TWL4030_BATSTS)
> +       if (irqs1 & TWL4030_BATSTS) {
>                 dev_crit(bci->dev, "battery disconnected\n");
> +               power_off = true;
> +       }
>
> -       if (irqs2 & TWL4030_VBATOV)
> +       if (irqs2 & TWL4030_VBATOV) {
>                 dev_crit(bci->dev, "VBAT overvoltage\n");
> +               power_off = true;
> +       }
>
> -       if (irqs2 & TWL4030_VBUSOV)
> +       if (irqs2 & TWL4030_VBUSOV) {
>                 dev_crit(bci->dev, "VBUS overvoltage\n");
> +               power_off = true;
> +       }
>
> -       if (irqs2 & TWL4030_ACCHGOV)
> +       if (irqs2 & TWL4030_ACCHGOV) {
>                 dev_crit(bci->dev, "Ac charger overvoltage\n");
> +               power_off = true;
> +       }
> +
> +       /* Try to shutdown the system */
> +       if (power_off)
> +               orderly_poweroff(true);
>
>         return IRQ_HANDLED;
>  }
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Gra?vydas

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Nishanth Menon]

On 06/04/2014 05:04 AM, Grazvydas Ignotas wrote:
> On Thu, May 29, 2014 at 12:46 AM, Nishanth Menon <nm@ti.com> wrote:
>> Attempt to power off in case of critical events such as battery removal,
>> over voltage events.
>>
>> There is no guarentee that we'd be in a safe scenario here, but the very
>> least we can try to do is to power off the device to prevent damage to
>> the system instead of just printing a message and hoping for the best.
> 
> At least "battery temperature out of range" does seem to happen quite
> often while charging on hot summer day. I'd prefer my pandora to not
> shutdown in such case, it could just stop charging instead.
Yeah, We could call
  twl4030_charger_enable_ac(false);
  twl4030_charger_enable_usb(bci, false);

But then, is that sufficient?
>From the TRM:
7.5.8 Battery Temperature Out-of-Range Detection
Battery temperature out-of-range detection detects whether the battery
temperature is within a specific
range. Detection is possible for two temperature ranges. When the
battery temperature is not in the
2?50?C range or is in the 3?43?C range, the TBATOR1 and TBATOR2 status
bits rise and an interrupt is
generated.
This MADC monitoring function can be enabled by writing to the
TBATOR1EN (BCIMFEN2[3]) and
TBATOR2EN (BCIMFEN2[1]) fields.

Battery pack at high temperature is a risk, no? and it may not be just
charger that might be causing such a condition. Is'nt it safer to shut
the device down in such a case?

-- 
Regards,
Nishanth Menon

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Grazvydas Ignotas]

On Wed, Jun 4, 2014 at 4:01 PM, Nishanth Menon <nm@ti.com> wrote:
> On 06/04/2014 05:04 AM, Grazvydas Ignotas wrote:
>> On Thu, May 29, 2014 at 12:46 AM, Nishanth Menon <nm@ti.com> wrote:
>>> Attempt to power off in case of critical events such as battery removal,
>>> over voltage events.
>>>
>>> There is no guarentee that we'd be in a safe scenario here, but the very
>>> least we can try to do is to power off the device to prevent damage to
>>> the system instead of just printing a message and hoping for the best.
>>
>> At least "battery temperature out of range" does seem to happen quite
>> often while charging on hot summer day. I'd prefer my pandora to not
>> shutdown in such case, it could just stop charging instead.
> Yeah, We could call
>   twl4030_charger_enable_ac(false);
>   twl4030_charger_enable_usb(bci, false);
>
> But then, is that sufficient?
> From the TRM:
> 7.5.8 Battery Temperature Out-of-Range Detection
> Battery temperature out-of-range detection detects whether the battery
> temperature is within a specific
> range. Detection is possible for two temperature ranges. When the
> battery temperature is not in the
> 2?50?C range or is in the 3?43?C range, the TBATOR1 and TBATOR2 status
> bits rise and an interrupt is
> generated.
> This MADC monitoring function can be enabled by writing to the
> TBATOR1EN (BCIMFEN2[3]) and
> TBATOR2EN (BCIMFEN2[1]) fields.
>
> Battery pack at high temperature is a risk, no? and it may not be just
> charger that might be causing such a condition. Is'nt it safer to shut
> the device down in such a case?

I don't know, so far nobody has complained about the battery exploding
and anybody getting hurt, but it would make the device unusable for
people in hot climates. From what I remember the automatic charge is
stopped automatically on this condition, as some people complained
they couldn't charge their device and saw these messages in dmesg. I
guess mainline could choose the safer option and shutdown, no strong
opinion about this.

-- 
Gra?vydas

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Nishanth Menon]

On 01:30-20140605, Grazvydas Ignotas wrote:
> On Wed, Jun 4, 2014 at 4:01 PM, Nishanth Menon <nm@ti.com> wrote:
> > On 06/04/2014 05:04 AM, Grazvydas Ignotas wrote:
> >> On Thu, May 29, 2014 at 12:46 AM, Nishanth Menon <nm@ti.com> wrote:
> >>> Attempt to power off in case of critical events such as battery removal,
> >>> over voltage events.
> >>>
> >>> There is no guarentee that we'd be in a safe scenario here, but the very
> >>> least we can try to do is to power off the device to prevent damage to
> >>> the system instead of just printing a message and hoping for the best.
> >>
> >> At least "battery temperature out of range" does seem to happen quite
> >> often while charging on hot summer day. I'd prefer my pandora to not
> >> shutdown in such case, it could just stop charging instead.
> > Yeah, We could call
> >   twl4030_charger_enable_ac(false);
> >   twl4030_charger_enable_usb(bci, false);
> >
> > But then, is that sufficient?
> > From the TRM:
> > 7.5.8 Battery Temperature Out-of-Range Detection
> > Battery temperature out-of-range detection detects whether the battery
> > temperature is within a specific
> > range. Detection is possible for two temperature ranges. When the
> > battery temperature is not in the
> > 2?50?C range or is in the 3?43?C range, the TBATOR1 and TBATOR2 status
> > bits rise and an interrupt is
> > generated.
> > This MADC monitoring function can be enabled by writing to the
> > TBATOR1EN (BCIMFEN2[3]) and
> > TBATOR2EN (BCIMFEN2[1]) fields.
> >
> > Battery pack at high temperature is a risk, no? and it may not be just
> > charger that might be causing such a condition. Is'nt it safer to shut
> > the device down in such a case?
> 
> I don't know, so far nobody has complained about the battery exploding
> and anybody getting hurt, but it would make the device unusable for

Yeah - but this is not something we'd like to see, ever! Out side of
2-50C+ range, I'd think "cold climates" too and wonder about[1] style
experiences.. there are too many PR stories on the net which does not
let me do anything else in case of error notifications like this. As
you can guess, I'd want to be the last person responsible for a patch
that does not react to hardware event telling me "unsafe temperature"
and not taking drastic action that kernel lets me(not interested in
appearing in some court).

> people in hot climates. From what I remember the automatic charge is
> stopped automatically on this condition, as some people complained
> they couldn't charge their device and saw these messages in dmesg. I
> guess mainline could choose the safer option and shutdown, no strong
> opinion about this.

OK. Then, lets just stick to reasonable temperature ranges here and go
a little more conservative[shut down the chargers as well] as in the
following patch. Do ack if you are ok with the following.

[1] http://en.wikipedia.org/wiki/Lithium-ion_battery#Safety

8<--------------
>From daeac775473061c5f59d307a9500c688d9b6e87f Mon Sep 17 00:00:00 2001
From: Nishanth Menon <nm@ti.com>
Date: Wed, 28 May 2014 16:01:16 -0500
Subject: [PATCH V2] power: twl4030_charger: attempt to power off in case of
 critical events

Attempt to power off in case of critical events such as battery removal,
over voltage events.

There is no guarentee that we'd be in a safe scenario here, but the
very least we can try to do is to shut down the chargers and power off
the device to prevent damage to the system instead of just printing a
message and hoping for the best.

NOTE: twl4030 should attempt some form of recovery, but just depending
on that is never a safe alternative.

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 drivers/power/twl4030_charger.c |   30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 2598c58..6d92893 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -22,6 +22,7 @@
 #include <linux/power_supply.h>
 #include <linux/notifier.h>
 #include <linux/usb/otg.h>
+#include <linux/reboot.h>
 #include <linux/regulator/machine.h>
 
 #define TWL4030_BCIMSTATEC	0x02
@@ -332,6 +333,7 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
 	struct twl4030_bci *bci = arg;
 	u8 irqs1, irqs2;
 	int ret;
+	bool power_off = false;
 
 	ret = twl_i2c_read_u8(TWL4030_MODULE_INTERRUPTS, &irqs1,
 			      TWL4030_INTERRUPTS_BCIISR1A);
@@ -352,20 +354,38 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
 	}
 
 	/* various monitoring events, for now we just log them here */
-	if (irqs1 & (TWL4030_TBATOR2 | TWL4030_TBATOR1))
+	if (irqs1 & (TWL4030_TBATOR2 | TWL4030_TBATOR1)) {
 		dev_warn(bci->dev, "battery temperature out of range\n");
+		power_off = true;
+	}
 
-	if (irqs1 & TWL4030_BATSTS)
+	if (irqs1 & TWL4030_BATSTS) {
 		dev_crit(bci->dev, "battery disconnected\n");
+		power_off = true;
+	}
 
-	if (irqs2 & TWL4030_VBATOV)
+	if (irqs2 & TWL4030_VBATOV) {
 		dev_crit(bci->dev, "VBAT overvoltage\n");
+		power_off = true;
+	}
 
-	if (irqs2 & TWL4030_VBUSOV)
+	if (irqs2 & TWL4030_VBUSOV) {
 		dev_crit(bci->dev, "VBUS overvoltage\n");
+		power_off = true;
+	}
 
-	if (irqs2 & TWL4030_ACCHGOV)
+	if (irqs2 & TWL4030_ACCHGOV) {
 		dev_crit(bci->dev, "Ac charger overvoltage\n");
+		power_off = true;
+	}
+
+	/* Try to shutdown the system */
+	if (power_off) {
+		/* Disable chargers before shutting off */
+		twl4030_charger_enable_ac(false);
+		twl4030_charger_enable_usb(bci, false);
+		orderly_poweroff(true);
+	}
 
 	return IRQ_HANDLED;
 }
-- 
1.7.9.5

-- 
Regards,
Nishanth Menon

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Pavel Machek]

On Thu 2014-06-05 01:30:40, Grazvydas Ignotas wrote:
> On Wed, Jun 4, 2014 at 4:01 PM, Nishanth Menon <nm@ti.com> wrote:
> > On 06/04/2014 05:04 AM, Grazvydas Ignotas wrote:
> >> On Thu, May 29, 2014 at 12:46 AM, Nishanth Menon <nm@ti.com> wrote:
> >> often while charging on hot summer day. I'd prefer my pandora to not
> >> shutdown in such case, it could just stop charging instead.
> > Yeah, We could call
> >   twl4030_charger_enable_ac(false);
> >   twl4030_charger_enable_usb(bci, false);
> >
> > But then, is that sufficient?
> > From the TRM:
> > 7.5.8 Battery Temperature Out-of-Range Detection
> > Battery temperature out-of-range detection detects whether the battery
> > temperature is within a specific
> > range. Detection is possible for two temperature ranges. When the
> > battery temperature is not in the
> > 2???50?C range or is in the 3???43?C range, the TBATOR1 and TBATOR2 status
> > bits rise and an interrupt is
> > generated.
> > This MADC monitoring function can be enabled by writing to the
> > TBATOR1EN (BCIMFEN2[3]) and
> > TBATOR2EN (BCIMFEN2[1]) fields.
> >
> > Battery pack at high temperature is a risk, no? and it may not be just
> > charger that might be causing such a condition. Is'nt it safer to shut
> > the device down in such a case?
> 
> I don't know, so far nobody has complained about the battery exploding
> and anybody getting hurt, but it would make the device unusable for
> people in hot climates. From what I remember the automatic charge is
> stopped automatically on this condition, as some people complained
> they couldn't charge their device and saw these messages in dmesg. I
> guess mainline could choose the safer option and shutdown, no strong
> opinion about this.

Li-ions have strong casing -- battery will not explode, just will grow a bit. And
50C will probably not make it explode immediately, just shorten its life a lot.

If your battery is close enough to CPU (for example), it should be something else
that heats it up. I think that shutdown by default is a good idea.

If particular hardware triggers this a lot _and_ there are no components (CPU, hdd) 
heating the battery, you may want to just stop charging... but do it per-machine
...

Actually, you probably want to stop charging at 37C or so, to stop triggering this...

										Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Pavel Machek]

Hi!

> +	if (irqs2 & TWL4030_ACCHGOV) {
>  		dev_crit(bci->dev, "Ac charger overvoltage\n");
> +		power_off = true;
> +	}
> +
> +	/* Try to shutdown the system */
> +	if (power_off)
> +		orderly_poweroff(true);
>  
>  	return IRQ_HANDLED;
>  }

Userland can make orderly_poweroff take long time, or even forever. (Think disconnected
network with nfsroot).

Should we do something more forceful here? Userland has to handle sudden poweroffs, anyway...

(You could invent new function "give userland 5 seconds to shut system down" if you
really wanted...)

										Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH V2 2/2] power: twl4030_charger: attempt to power off in case of critical events

[Nishanth Menon]

On Sat, Jun 14, 2014 at 5:21 PM, Pavel Machek <pavel@ucw.cz> wrote:
>
>
> Userland can make orderly_poweroff take long time, or even forever. (Think disconnected
> network with nfsroot).
>
> Should we do something more forceful here? Userland has to handle sudden poweroffs, anyway...
>
> (You could invent new function "give userland 5 seconds to shut system down" if you
> really wanted...)


Or how about:

 /* If we have Power OFF ability, use it, else try restarting */
 if (pm_power_off) {
         kernel_power_off();
 } else {
         WARN(1, "FIXME: NO pm_power_off!!! trying restart\n");
         kernel_restart("Charger triggered emergency restart");
 }


-- 
---
Regards,
Nishanth Menon