Hi!
What about something like this? N900 will drain the battery down to
system crash, which is quite uncool.
Pavel
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 0fe278b..8eb2f8f 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -46,6 +46,7 @@
#include <linux/delay.h>
#include <linux/platform_device.h>
#include <linux/power_supply.h>
+#include <linux/reboot.h>
#include <linux/slab.h>
#include <linux/of.h>
@@ -740,6 +741,9 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
}
EXPORT_SYMBOL_GPL(bq27xxx_battery_update);
+static int bq27xxx_battery_protect(struct bq27xxx_device_info *di);
+
+
static void bq27xxx_battery_poll(struct work_struct *work)
{
struct bq27xxx_device_info *di =
@@ -747,6 +751,7 @@ static void bq27xxx_battery_poll(struct work_struct *work)
work.work);
bq27xxx_battery_update(di);
+ bq27xxx_battery_protect(di);
if (poll_interval > 0)
schedule_delayed_work(&di->work, poll_interval * HZ);
@@ -958,6 +963,41 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
return ret;
}
+static int bq27xxx_battery_protect(struct bq27xxx_device_info *di)
+{
+ union power_supply_propval val;
+ int mV, mA, mOhm = 430, mVadj;
+ int res;
+
+ printk(KERN_INFO "Main battery check\n");
+
+ res = bq27xxx_battery_voltage(di, &val);
+ if (res)
+ return res;
+
+ mV = val.intval / 1000;
+
+ if (mV < 2950) {
+ printk(KERN_ALERT "Main battery below 2.95V, forcing shutdown.\n");
+ orderly_poweroff(true);
+ }
+
+ res = bq27xxx_battery_current(di, &val);
+ if (res)
+ return res;
+
+ mA = val.intval / 1000;
+ mVadj = mV + (mA * mOhm) / 1000;
+
+ if (mVadj < 3150) {
+ printk(KERN_ALERT "Main battery internal voltage below 3.15, shutdown.\n");
+ orderly_poweroff(true);
+ }
+ printk(KERN_INFO "Main battery %d mV, internal voltage %d mV\n",
+ mV, mVadj);
+ return 0;
+}
+
static void bq27xxx_external_power_changed(struct power_supply *psy)
{
struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 226b0b4ac..bcdc1f8 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -444,7 +444,7 @@ static void handle_critical_trips(struct thermal_zone_device *tz,
if (trip_type == THERMAL_TRIP_CRITICAL) {
dev_emerg(&tz->device,
- "critical temperature reached(%d C),shutting down\n",
+ "critical temperature reached(%d C), shutting down\n",
tz->temperature / 1000);
orderly_poweroff(true);
}
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
* Pavel Machek <[email protected]> [161024 14:24]:
> Hi!
>
> What about something like this? N900 will drain the battery down to
> system crash, which is quite uncool.
Can't we make that generic and configurable for the voltage somehow?
Also, the shutdown voltage can depend on external devices connected.
It could be for example 3.3V depending on eMMC on some devices while
devices with no eMMC could have it at 3.0V.
Regards,
Tony
On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> * Pavel Machek <[email protected]> [161024 14:24]:
> > Hi!
> >
> > What about something like this? N900 will drain the battery down to
> > system crash, which is quite uncool.
>
> Can't we make that generic and configurable for the voltage somehow?
I was afraid someone would ask :-).
Yes, we probably need to create battery object in the device tree,
then add properties there.
> Also, the shutdown voltage can depend on external devices connected.
> It could be for example 3.3V depending on eMMC on some devices while
> devices with no eMMC could have it at 3.0V.
Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
going below that is pretty mean to the battery. But if I set threshold
too high, GSM activity will push it below that for a very short while,
and I'll shutdown too soon.
Ideas welcome...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Monday 24 October 2016 23:41:52 Pavel Machek wrote:
> On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > Also, the shutdown voltage can depend on external devices
> > connected. It could be for example 3.3V depending on eMMC on some
> > devices while devices with no eMMC could have it at 3.0V.
>
> Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
> going below that is pretty mean to the battery. But if I set
> threshold too high, GSM activity will push it below that for a very
> short while, and I'll shutdown too soon.
>
> Ideas welcome...
bq27x00 has EDVF flag which means that battery is empty. Maemo with
bq27x00 driver is configured to issue system shutdown when EDVF is set.
Maybe kernel should issue emergency shutdown e.g. after minute or two
after EDVF flag is set?
--
Pali Rohár
[email protected]
On Mon 2016-10-24 23:48:47, Pali Roh?r wrote:
> On Monday 24 October 2016 23:41:52 Pavel Machek wrote:
> > On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > > Also, the shutdown voltage can depend on external devices
> > > connected. It could be for example 3.3V depending on eMMC on some
> > > devices while devices with no eMMC could have it at 3.0V.
> >
> > Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
> > going below that is pretty mean to the battery. But if I set
> > threshold too high, GSM activity will push it below that for a very
> > short while, and I'll shutdown too soon.
> >
> > Ideas welcome...
>
> bq27x00 has EDVF flag which means that battery is empty. Maemo with
> bq27x00 driver is configured to issue system shutdown when EDVF is set.
>
> Maybe kernel should issue emergency shutdown e.g. after minute or two
> after EDVF flag is set?
Thanks for pointer.
EDVF seems to be exposed as health. ... but only if battery is
calibrated, AFAICT.
if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
...
cache.health = -ENODATA;
Plus, it prioritizes battery cold over battery dead. IMO we don't need
to shutdown on battery cold (we just may not charge the battery), but
we need to shutdown on battery dead.
So something like this?
Pavel
Signed-off-by: Pavel Machek <[email protected]>
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 8eb2f8f..5ddf6d7 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -680,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
/* Unlikely but important to return first */
if (unlikely(bq27xxx_battery_overtemp(di, flags)))
return POWER_SUPPLY_HEALTH_OVERHEAT;
- if (unlikely(bq27xxx_battery_undertemp(di, flags)))
- return POWER_SUPPLY_HEALTH_COLD;
if (unlikely(bq27xxx_battery_dead(di, flags)))
return POWER_SUPPLY_HEALTH_DEAD;
+ if (unlikely(bq27xxx_battery_undertemp(di, flags)))
+ return POWER_SUPPLY_HEALTH_COLD;
return POWER_SUPPLY_HEALTH_GOOD;
}
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tuesday 25 October 2016 12:24:35 Pavel Machek wrote:
> On Mon 2016-10-24 23:48:47, Pali Rohár wrote:
> > On Monday 24 October 2016 23:41:52 Pavel Machek wrote:
> > > On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > > > Also, the shutdown voltage can depend on external devices
> > > > connected. It could be for example 3.3V depending on eMMC on some
> > > > devices while devices with no eMMC could have it at 3.0V.
> > >
> > > Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
> > > going below that is pretty mean to the battery. But if I set
> > > threshold too high, GSM activity will push it below that for a very
> > > short while, and I'll shutdown too soon.
> > >
> > > Ideas welcome...
> >
> > bq27x00 has EDVF flag which means that battery is empty. Maemo with
> > bq27x00 driver is configured to issue system shutdown when EDVF is set.
> >
> > Maybe kernel should issue emergency shutdown e.g. after minute or two
> > after EDVF flag is set?
>
> Thanks for pointer.
>
> EDVF seems to be exposed as health. ... but only if battery is
> calibrated, AFAICT.
No, EDVF is available also for uncalibrated battery. There are EDV1 and
EDVF flags. Both are set based on battery voltage and some other
parameters from bq EEPROM.
> if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
> dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
Yes, it ignores only capacity values (which needs calibration), not
those raw flags which works also without calibration.
> ...
> cache.health = -ENODATA;
>
> Plus, it prioritizes battery cold over battery dead. IMO we don't need
> to shutdown on battery cold (we just may not charge the battery), but
> we need to shutdown on battery dead.
>
> So something like this?
>
> Pavel
>
> Signed-off-by: Pavel Machek <[email protected]>
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 8eb2f8f..5ddf6d7 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -680,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
> /* Unlikely but important to return first */
> if (unlikely(bq27xxx_battery_overtemp(di, flags)))
> return POWER_SUPPLY_HEALTH_OVERHEAT;
> - if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> - return POWER_SUPPLY_HEALTH_COLD;
> if (unlikely(bq27xxx_battery_dead(di, flags)))
> return POWER_SUPPLY_HEALTH_DEAD;
> + if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> + return POWER_SUPPLY_HEALTH_COLD;
>
> return POWER_SUPPLY_HEALTH_GOOD;
> }
>
>
Looks like this is OK.
--
Pali Rohár
[email protected]
On Tue 2016-10-25 12:53:20, Pali Roh?r wrote:
> On Tuesday 25 October 2016 12:24:35 Pavel Machek wrote:
> > On Mon 2016-10-24 23:48:47, Pali Roh?r wrote:
> > > On Monday 24 October 2016 23:41:52 Pavel Machek wrote:
> > > > On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > > > > Also, the shutdown voltage can depend on external devices
> > > > > connected. It could be for example 3.3V depending on eMMC on some
> > > > > devices while devices with no eMMC could have it at 3.0V.
> > > >
> > > > Actually, I'd like to shutdown at 3.3V or more (like 3.5V), because
> > > > going below that is pretty mean to the battery. But if I set
> > > > threshold too high, GSM activity will push it below that for a very
> > > > short while, and I'll shutdown too soon.
> > > >
> > > > Ideas welcome...
> > >
> > > bq27x00 has EDVF flag which means that battery is empty. Maemo with
> > > bq27x00 driver is configured to issue system shutdown when EDVF is set.
> > >
> > > Maybe kernel should issue emergency shutdown e.g. after minute or two
> > > after EDVF flag is set?
> >
> > Thanks for pointer.
> >
> > EDVF seems to be exposed as health. ... but only if battery is
> > calibrated, AFAICT.
>
> No, EDVF is available also for uncalibrated battery. There are EDV1 and
> EDVF flags. Both are set based on battery voltage and some other
> parameters from bq EEPROM.
>
> > if (has_ci_flag && (cache.flags & BQ27000_FLAG_CI)) {
> > dev_info_once(di->dev, "battery is not calibrated! ignoring capacity values\n");
>
> Yes, it ignores only capacity values (which needs calibration), not
> those raw flags which works also without calibration.
>
> > ...
> > cache.health = -ENODATA;
Take a look at code. Health is not read from hardware unless battery
is calibrated.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tuesday 25 October 2016 12:56:04 Pavel Machek wrote:
> Take a look at code. Health is not read from hardware unless battery
> is calibrated.
Then it is bug. EDVF should be accepted also when battery is not
calibrated.
--
Pali Rohár
[email protected]
On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> * Pavel Machek <[email protected]> [161024 14:24]:
> > Hi!
> >
> > What about something like this? N900 will drain the battery down to
> > system crash, which is quite uncool.
>
> Can't we make that generic and configurable for the voltage somehow?
>
> Also, the shutdown voltage can depend on external devices connected.
> It could be for example 3.3V depending on eMMC on some devices while
> devices with no eMMC could have it at 3.0V.
Actually, do we need to make it configurable? It looks like we should
respect hardware telling us battery is dead, and only use (low)
hardcoded voltages as a fallback.
Currently patch looks like this. generic_protect() should work for
other batteries drivers, too.
Pavel
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 0fe278b..04094ad 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -46,6 +46,7 @@
#include <linux/delay.h>
#include <linux/platform_device.h>
#include <linux/power_supply.h>
+#include <linux/reboot.h>
#include <linux/slab.h>
#include <linux/of.h>
@@ -679,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
/* Unlikely but important to return first */
if (unlikely(bq27xxx_battery_overtemp(di, flags)))
return POWER_SUPPLY_HEALTH_OVERHEAT;
- if (unlikely(bq27xxx_battery_undertemp(di, flags)))
- return POWER_SUPPLY_HEALTH_COLD;
if (unlikely(bq27xxx_battery_dead(di, flags)))
return POWER_SUPPLY_HEALTH_DEAD;
+ if (unlikely(bq27xxx_battery_undertemp(di, flags)))
+ return POWER_SUPPLY_HEALTH_COLD;
return POWER_SUPPLY_HEALTH_GOOD;
}
@@ -740,6 +741,49 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
}
EXPORT_SYMBOL_GPL(bq27xxx_battery_update);
+static void shutdown(char *reason)
+{
+ pr_alert("%s Forcing shutdown\n", reason);
+ orderly_poweroff(true);
+}
+
+static int generic_protect(struct power_supply *psy)
+{
+ union power_supply_propval val;
+ int res;
+ int mV, mA, mOhm = 430, mVadj = 0;
+
+ res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_HEALTH, &val);
+ if (res)
+ return res;
+
+ if (val.intval == POWER_SUPPLY_HEALTH_OVERHEAT)
+ shutdown("Battery overheat.");
+ if (val.intval == POWER_SUPPLY_HEALTH_DEAD)
+ shutdown("Battery dead.");
+
+ res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW, &val);
+ if (res)
+ return res;
+ mV = val.intval / 1000;
+
+ if (mV < 2950)
+ shutdown("Battery below 2.95V.");
+
+ res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW, &val);
+ if (res)
+ return res;
+ mA = val.intval / 1000;
+ mVadj = mV + (mA * mOhm) / 1000;
+
+ if (mVadj < 3150)
+ shutdown("Battery internal voltage below 3.15.");
+
+ printk(KERN_INFO "Main battery %d mV, internal voltage %d mV\n",
+ mV, mVadj);
+ return 0;
+}
+
static void bq27xxx_battery_poll(struct work_struct *work)
{
struct bq27xxx_device_info *di =
@@ -747,6 +791,7 @@ static void bq27xxx_battery_poll(struct work_struct *work)
work.work);
bq27xxx_battery_update(di);
+ generic_protect(di->bat);
if (poll_interval > 0)
schedule_delayed_work(&di->work, poll_interval * HZ);
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tuesday 25 October 2016 13:27:57 Pavel Machek wrote:
> On Mon 2016-10-24 14:29:33, Tony Lindgren wrote:
> > * Pavel Machek <[email protected]> [161024 14:24]:
> > > Hi!
> > >
> > > What about something like this? N900 will drain the battery down to
> > > system crash, which is quite uncool.
> >
> > Can't we make that generic and configurable for the voltage somehow?
> >
> > Also, the shutdown voltage can depend on external devices connected.
> > It could be for example 3.3V depending on eMMC on some devices while
> > devices with no eMMC could have it at 3.0V.
>
> Actually, do we need to make it configurable? It looks like we should
> respect hardware telling us battery is dead, and only use (low)
> hardcoded voltages as a fallback.
>
> Currently patch looks like this. generic_protect() should work for
> other batteries drivers, too.
Now I checked Maemo's BME replacement code and it shutdown device 10
seconds after it read that capacity level is LOW or CRITICAL. bq27xxx
driver reports LOW when EDV1 is set and CRITICAL when EDVF is set. And
bq27xxx reports those LOW/CRITICAL based on EDV1/EDVF flags even if
battery is not calibrated.
So I would be happy if kernel does not issue emergency shutdown before
Maemo's BME replacement issue own "correct" system shutdown. As Maemo is
doing it on EDV1 flag (not EDVF as I thought!) with 10 seconds delay and
check is done every 30 seconds it means that Maemo shutdown process in
worst case is started 40 seconds after EDV1 is set. Shutdown process is
about 60 seconds (probably max.), can we ensure that kernel does not do
its own emergency shutdown earlier then 2 minutes before first see EDV1
flag? Or can test that EDVF flag is set in most cases 2 minutes after
EDV1?
It is really bad idea to start emergency kernel shutdown before even
userspace do it!
> Pavel
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 0fe278b..04094ad 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -46,6 +46,7 @@
> #include <linux/delay.h>
> #include <linux/platform_device.h>
> #include <linux/power_supply.h>
> +#include <linux/reboot.h>
> #include <linux/slab.h>
> #include <linux/of.h>
>
> @@ -679,10 +680,10 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
> /* Unlikely but important to return first */
> if (unlikely(bq27xxx_battery_overtemp(di, flags)))
> return POWER_SUPPLY_HEALTH_OVERHEAT;
> - if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> - return POWER_SUPPLY_HEALTH_COLD;
> if (unlikely(bq27xxx_battery_dead(di, flags)))
> return POWER_SUPPLY_HEALTH_DEAD;
> + if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> + return POWER_SUPPLY_HEALTH_COLD;
>
> return POWER_SUPPLY_HEALTH_GOOD;
> }
> @@ -740,6 +741,49 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
> }
> EXPORT_SYMBOL_GPL(bq27xxx_battery_update);
>
> +static void shutdown(char *reason)
> +{
> + pr_alert("%s Forcing shutdown\n", reason);
> + orderly_poweroff(true);
> +}
> +
> +static int generic_protect(struct power_supply *psy)
> +{
> + union power_supply_propval val;
> + int res;
> + int mV, mA, mOhm = 430, mVadj = 0;
> +
> + res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_HEALTH, &val);
> + if (res)
> + return res;
> +
> + if (val.intval == POWER_SUPPLY_HEALTH_OVERHEAT)
> + shutdown("Battery overheat.");
> + if (val.intval == POWER_SUPPLY_HEALTH_DEAD)
> + shutdown("Battery dead.");
Generally this is not a good idea. On some boards with bq27xxx devices
you can have another battery device or connected power device. You could
have "dead" battery but device supplied e.g. from wallcharger.
N900 cannot be powered from wallcharger by default, but in specific
conditions (turned everything except display) you can do battery
hotswap (when wallcharger is connected; it can power system).
So this patch basically break lot of other self powered devices.
I would propose check for am_i_power_supplied() (function with such name
in power_supply interface exist) and do that only for negative response.
> + res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_VOLTAGE_NOW, &val);
> + if (res)
> + return res;
> + mV = val.intval / 1000;
> +
> + if (mV < 2950)
> + shutdown("Battery below 2.95V.");
> +
> + res = psy->desc->get_property(psy, POWER_SUPPLY_PROP_CURRENT_NOW, &val);
> + if (res)
> + return res;
> + mA = val.intval / 1000;
> + mVadj = mV + (mA * mOhm) / 1000;
> +
> + if (mVadj < 3150)
> + shutdown("Battery internal voltage below 3.15.");
> +
> + printk(KERN_INFO "Main battery %d mV, internal voltage %d mV\n",
> + mV, mVadj);
Please no printk. There is dev_info or how is that function called. And
spamming dmesg for every poll is not good idea. It should be probably
DEBUG not INFO.
> + return 0;
> +}
> +
> static void bq27xxx_battery_poll(struct work_struct *work)
> {
> struct bq27xxx_device_info *di =
> @@ -747,6 +791,7 @@ static void bq27xxx_battery_poll(struct work_struct *work)
> work.work);
>
> bq27xxx_battery_update(di);
> + generic_protect(di->bat);
>
> if (poll_interval > 0)
> schedule_delayed_work(&di->work, poll_interval * HZ);
>
--
Pali Rohár
[email protected]
> + res = bq27xxx_battery_voltage(di, &val);
> + if (res)
> + return res;
> +
> + mV = val.intval / 1000;
Reading that code I stumbled over the comment in
bq27xxx_battery_voltage saying that it returns millivolts. The code
here, the code in bq27xxx_battery_voltage and power_supply.h all
indicate that it in fact returns microvolts. Please double-check and
fix, as it stands now the code looks inconsistent (but not knowing that
device at all I don't feel fit to submit a fix).
regards, Olaf