| Message ID | 20210916194208.10387-1-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | power: supply: Add Support for RK817 Charger | expand |
On Donnerstag, 16. September 2021 21:42:04 CET Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > This series is to add support for the Rockchip rk817 battery charger > which is present in all Rockchip RK817 PMICs. The driver was written > as a joint effort by Maya Matuszczyk <maccraft123mc@gmail.com> and > myself Chris Morgan <macromorgan@hotmail.com>. Hi Chris and Maya, Gave this a whirl on my Quartz64 Model A. I noticed that this will happily let me discharge past voltage_min_design: $ cat /sys/class/power_supply/rk817-battery/voltage_min_design 3625000 $ cat /sys/class/power_supply/rk817-battery/voltage_avg 3381360 Is this normal? It went all the way to under 3V before the board finally locked up. Does the minimum voltage not affect some sort of cutout on the RK817? Does it even have one? Is it the driver's job to do something here or not? Regards, Nicolas Frattaroli
On Tue, Nov 30, 2021 at 03:03:03AM +0100, Nicolas Frattaroli wrote: > On Donnerstag, 16. September 2021 21:42:04 CET Chris Morgan wrote: > > From: Chris Morgan <macromorgan@hotmail.com> > > > > This series is to add support for the Rockchip rk817 battery charger > > which is present in all Rockchip RK817 PMICs. The driver was written > > as a joint effort by Maya Matuszczyk <maccraft123mc@gmail.com> and > > myself Chris Morgan <macromorgan@hotmail.com>. > > Hi Chris and Maya, > > Gave this a whirl on my Quartz64 Model A. I noticed that this will > happily let me discharge past voltage_min_design: > > $ cat /sys/class/power_supply/rk817-battery/voltage_min_design > 3625000 > $ cat /sys/class/power_supply/rk817-battery/voltage_avg > 3381360 > > Is this normal? It went all the way to under 3V before the > board finally locked up. > > Does the minimum voltage not affect some sort of cutout on > the RK817? Does it even have one? Is it the driver's job to > do something here or not? It does not look like I coded that, but I can. The PMIC has a selectable register (RK817_PMIC_SYS_CFG0 = 0xf1) to automatically shut down the system at a certain voltage (between 2.7v and 3.4v in increments of 100mv; bits 6-4), a register to set a low voltage value (between 2.8v and 3.5v in increments of 100mv; bits 2-0), and a register to set a low voltage action (either shut down the machine or trigger an interrupt; bit 3 and then I believe the interrupt is read at bit 7 of RK817_PMIC_INT_STS0 - 0xf8). I guess I just assumed userspace would handle it, but that's probably a bad assumption. What if I set the low voltage value to either 3.5v or the min design voltage, whichever is less; and then set the low voltage trigger to shut down the system? This would prevent your battery from dropping below 3.5v or the min design voltage (whichever is less, sadly 3.5v is as high as I can go for the minimum) in the event userspace doesn't take action first? We could also set the shutdown voltage to be 100mv less than the min design voltage and use the low voltage interrupt to trigger an action, but I'm not sure what action would be appropriate (and if userspace isn't listening it would be moot anyway). Thank you. > > Regards, > Nicolas Frattaroli > >
On Dienstag, 30. November 2021 17:10:21 CET Chris Morgan wrote: > On Tue, Nov 30, 2021 at 03:03:03AM +0100, Nicolas Frattaroli wrote: > > On Donnerstag, 16. September 2021 21:42:04 CET Chris Morgan wrote: > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > This series is to add support for the Rockchip rk817 battery charger > > > which is present in all Rockchip RK817 PMICs. The driver was written > > > as a joint effort by Maya Matuszczyk <maccraft123mc@gmail.com> and > > > myself Chris Morgan <macromorgan@hotmail.com>. > > > > Hi Chris and Maya, > > > > Gave this a whirl on my Quartz64 Model A. I noticed that this will > > happily let me discharge past voltage_min_design: > > > > $ cat /sys/class/power_supply/rk817-battery/voltage_min_design > > 3625000 > > $ cat /sys/class/power_supply/rk817-battery/voltage_avg > > 3381360 > > > > Is this normal? It went all the way to under 3V before the > > board finally locked up. > > > > Does the minimum voltage not affect some sort of cutout on > > the RK817? Does it even have one? Is it the driver's job to > > do something here or not? Hi Chris, > It does not look like I coded that, but I can. The PMIC has a > selectable register (RK817_PMIC_SYS_CFG0 = 0xf1) to automatically shut > down the system at a certain voltage (between 2.7v and 3.4v in > increments of 100mv; bits 6-4), a register to set a low voltage value > (between 2.8v and 3.5v in increments of 100mv; bits 2-0), and a > register to set a low voltage action (either shut down the machine or > trigger an interrupt; bit 3 and then I believe the interrupt is read at > bit 7 of RK817_PMIC_INT_STS0 - 0xf8). Excellent, I think shutting down the system is definitely the way to go here, as by reaching this voltage we can assume userspace has gone missing and we don't know if the kernel is still alive. > I guess I just assumed userspace would handle it, but that's probably > a bad assumption. Yes, I didn't have upower installed, but over discharge protection shouldn't be handled by userspace at all. Had I connected an unprotected cell that didn't cut out at below 3V, it would've been irreversibly damaged due to a simple forgotten package install. Even if unprotected batteries are a bad idea, I think we best assume the worst case situation here. > What if I set the low voltage value to either 3.5v > or the min design voltage, whichever is less; and then set the low > voltage trigger to shut down the system? This would prevent your > battery from dropping below 3.5v or the min design voltage (whichever > is less, sadly 3.5v is as high as I can go for the minimum) in the > event userspace doesn't take action first? We could also set the > shutdown voltage to be 100mv less than the min design voltage and use > the low voltage interrupt to trigger an action, but I'm not sure what > action would be appropriate (and if userspace isn't listening it would > be moot anyway). A very simple solution would be to simply set the shutoff point to the voltage reaching less than 3.1V. The RK817 only charges Lithium/Li-Po batteries, and they should generally not be discharged below 3.0V. Of the protected batteries I own, over-discharge protection generally kicks in at the 3V threshold. This gets rid of a lot of logic that could lead to bugs by simply setting a known good value to begin with. I think I can summarise how I understand this is supposed to work as follows: - if battery reaches 0% and the battery alarm is rang, userspace should power down the system - this prevents unclean shutdowns - if the battery reaches a dangerously low voltage for its chemistry, the PMIC should cut power to the system as instructed by the kernel - last resort measure in case userspace is absent and the battery happens to be unprotected - there may or may not be battery protection which kicks in at 3V, but we should not rely on this working - some 18650 cases infamously aren't sized to fit cells with a protection circuit connected to it. I shan't name names. In a freak accident where the kernel is completely locked up and the battery is unprotected and being drained, I presume that having set this PMIC register will still make it kick in, which lets me sleep a little sounder at night. Regards, Nicolas Frattaroli > > Thank you. > > > > > Regards, > > Nicolas Frattaroli > > > > >
On Tue, Nov 30, 2021 at 07:12:11PM +0100, Nicolas Frattaroli wrote: > On Dienstag, 30. November 2021 17:10:21 CET Chris Morgan wrote: > > On Tue, Nov 30, 2021 at 03:03:03AM +0100, Nicolas Frattaroli wrote: > > > On Donnerstag, 16. September 2021 21:42:04 CET Chris Morgan wrote: > > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > > > This series is to add support for the Rockchip rk817 battery charger > > > > which is present in all Rockchip RK817 PMICs. The driver was written > > > > as a joint effort by Maya Matuszczyk <maccraft123mc@gmail.com> and > > > > myself Chris Morgan <macromorgan@hotmail.com>. > > > > > > Hi Chris and Maya, > > > > > > Gave this a whirl on my Quartz64 Model A. I noticed that this will > > > happily let me discharge past voltage_min_design: > > > > > > $ cat /sys/class/power_supply/rk817-battery/voltage_min_design > > > 3625000 > > > $ cat /sys/class/power_supply/rk817-battery/voltage_avg > > > 3381360 > > > > > > Is this normal? It went all the way to under 3V before the > > > board finally locked up. > > > > > > Does the minimum voltage not affect some sort of cutout on > > > the RK817? Does it even have one? Is it the driver's job to > > > do something here or not? > > Hi Chris, > > > It does not look like I coded that, but I can. The PMIC has a > > selectable register (RK817_PMIC_SYS_CFG0 = 0xf1) to automatically shut > > down the system at a certain voltage (between 2.7v and 3.4v in > > increments of 100mv; bits 6-4), a register to set a low voltage value > > (between 2.8v and 3.5v in increments of 100mv; bits 2-0), and a > > register to set a low voltage action (either shut down the machine or > > trigger an interrupt; bit 3 and then I believe the interrupt is read at > > bit 7 of RK817_PMIC_INT_STS0 - 0xf8). > > Excellent, I think shutting down the system is definitely the way to > go here, as by reaching this voltage we can assume userspace has gone > missing and we don't know if the kernel is still alive. > > > I guess I just assumed userspace would handle it, but that's probably > > a bad assumption. > > Yes, I didn't have upower installed, but over discharge protection > shouldn't be handled by userspace at all. Had I connected an unprotected > cell that didn't cut out at below 3V, it would've been irreversibly > damaged due to a simple forgotten package install. Even if unprotected > batteries are a bad idea, I think we best assume the worst case > situation here. > > > What if I set the low voltage value to either 3.5v > > or the min design voltage, whichever is less; and then set the low > > voltage trigger to shut down the system? This would prevent your > > battery from dropping below 3.5v or the min design voltage (whichever > > is less, sadly 3.5v is as high as I can go for the minimum) in the > > event userspace doesn't take action first? We could also set the > > shutdown voltage to be 100mv less than the min design voltage and use > > the low voltage interrupt to trigger an action, but I'm not sure what > > action would be appropriate (and if userspace isn't listening it would > > be moot anyway). > > A very simple solution would be to simply set the shutoff point to the > voltage reaching less than 3.1V. The RK817 only charges Lithium/Li-Po > batteries, and they should generally not be discharged below 3.0V. Of > the protected batteries I own, over-discharge protection generally > kicks in at the 3V threshold. > > This gets rid of a lot of logic that could lead to bugs by simply setting > a known good value to begin with. > > I think I can summarise how I understand this is supposed to work as > follows: > > - if battery reaches 0% and the battery alarm is rang, userspace should > power down the system > - this prevents unclean shutdowns > - if the battery reaches a dangerously low voltage for its chemistry, > the PMIC should cut power to the system as instructed by the kernel > - last resort measure in case userspace is absent and the battery > happens to be unprotected > - there may or may not be battery protection which kicks in at 3V, but > we should not rely on this working > - some 18650 cases infamously aren't sized to fit cells with a > protection circuit connected to it. I shan't name names. > > In a freak accident where the kernel is completely locked up and the > battery is unprotected and being drained, I presume that having set > this PMIC register will still make it kick in, which lets me sleep > a little sounder at night. > Okay. The default value for automatic shutdown is 2.7v, but that's probably too low. I'm wondering though if this should go in the main MFD driver, rather than the battery driver? The reason why I ask is that without a battery driver to raise the shutdown voltage to 3.1v, the shutdown will not happen until 2.7v which risks damage to the battery. This means devices in production today (such as anyone running the Odroid Go Advance on a mainline kernel) on a battery will experience this behavior. Setting that register value shouldn't do anything harmful if you don't have a battery, so I assume if it's safe to set it 100% of the time for battery users it's safe to set 100% of the time in total. Which also begs the question, if this is such a danger to the hardware, should we even be doing it in Linux? We could be setting it in U-Boot, because if the MFD driver fails to load or the kernel freezes before the MFD driver loads there is STILL the risk you could undervolt the battery, regardless of fixing this in the MFD driver or the battery driver. Thank you. > Regards, > Nicolas Frattaroli > > > > > Thank you. > > > > > > > > Regards, > > > Nicolas Frattaroli > > > > > > > > > > > >
On Dienstag, 30. November 2021 19:33:02 CET you wrote: > On Tue, Nov 30, 2021 at 07:12:11PM +0100, Nicolas Frattaroli wrote: > > On Dienstag, 30. November 2021 17:10:21 CET Chris Morgan wrote: > > > On Tue, Nov 30, 2021 at 03:03:03AM +0100, Nicolas Frattaroli wrote: > > > > On Donnerstag, 16. September 2021 21:42:04 CET Chris Morgan wrote: > > > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > > > > > This series is to add support for the Rockchip rk817 battery charger > > > > > which is present in all Rockchip RK817 PMICs. The driver was written > > > > > as a joint effort by Maya Matuszczyk <maccraft123mc@gmail.com> and > > > > > myself Chris Morgan <macromorgan@hotmail.com>. > > > > > > > > Hi Chris and Maya, > > > > > > > > Gave this a whirl on my Quartz64 Model A. I noticed that this will > > > > happily let me discharge past voltage_min_design: > > > > > > > > $ cat /sys/class/power_supply/rk817-battery/voltage_min_design > > > > 3625000 > > > > $ cat /sys/class/power_supply/rk817-battery/voltage_avg > > > > 3381360 > > > > > > > > Is this normal? It went all the way to under 3V before the > > > > board finally locked up. > > > > > > > > Does the minimum voltage not affect some sort of cutout on > > > > the RK817? Does it even have one? Is it the driver's job to > > > > do something here or not? > > > > Hi Chris, > > > > > It does not look like I coded that, but I can. The PMIC has a > > > selectable register (RK817_PMIC_SYS_CFG0 = 0xf1) to automatically shut > > > down the system at a certain voltage (between 2.7v and 3.4v in > > > increments of 100mv; bits 6-4), a register to set a low voltage value > > > (between 2.8v and 3.5v in increments of 100mv; bits 2-0), and a > > > register to set a low voltage action (either shut down the machine or > > > trigger an interrupt; bit 3 and then I believe the interrupt is read at > > > bit 7 of RK817_PMIC_INT_STS0 - 0xf8). > > > > Excellent, I think shutting down the system is definitely the way to > > go here, as by reaching this voltage we can assume userspace has gone > > missing and we don't know if the kernel is still alive. > > > > > I guess I just assumed userspace would handle it, but that's probably > > > a bad assumption. > > > > Yes, I didn't have upower installed, but over discharge protection > > shouldn't be handled by userspace at all. Had I connected an unprotected > > cell that didn't cut out at below 3V, it would've been irreversibly > > damaged due to a simple forgotten package install. Even if unprotected > > batteries are a bad idea, I think we best assume the worst case > > situation here. > > > > > What if I set the low voltage value to either 3.5v > > > or the min design voltage, whichever is less; and then set the low > > > voltage trigger to shut down the system? This would prevent your > > > battery from dropping below 3.5v or the min design voltage (whichever > > > is less, sadly 3.5v is as high as I can go for the minimum) in the > > > event userspace doesn't take action first? We could also set the > > > shutdown voltage to be 100mv less than the min design voltage and use > > > the low voltage interrupt to trigger an action, but I'm not sure what > > > action would be appropriate (and if userspace isn't listening it would > > > be moot anyway). > > > > A very simple solution would be to simply set the shutoff point to the > > voltage reaching less than 3.1V. The RK817 only charges Lithium/Li-Po > > batteries, and they should generally not be discharged below 3.0V. Of > > the protected batteries I own, over-discharge protection generally > > kicks in at the 3V threshold. > > > > This gets rid of a lot of logic that could lead to bugs by simply setting > > a known good value to begin with. > > > > I think I can summarise how I understand this is supposed to work as > > follows: > > > > - if battery reaches 0% and the battery alarm is rang, userspace should > > power down the system > > - this prevents unclean shutdowns > > - if the battery reaches a dangerously low voltage for its chemistry, > > the PMIC should cut power to the system as instructed by the kernel > > - last resort measure in case userspace is absent and the battery > > happens to be unprotected > > - there may or may not be battery protection which kicks in at 3V, but > > we should not rely on this working > > - some 18650 cases infamously aren't sized to fit cells with a > > protection circuit connected to it. I shan't name names. > > > > In a freak accident where the kernel is completely locked up and the > > battery is unprotected and being drained, I presume that having set > > this PMIC register will still make it kick in, which lets me sleep > > a little sounder at night. > > > > Okay. The default value for automatic shutdown is 2.7v, but that's > probably too low. I'm wondering though if this should go in the main > MFD driver, rather than the battery driver? The reason why I ask is > that without a battery driver to raise the shutdown voltage to 3.1v, > the shutdown will not happen until 2.7v which risks damage to the > battery. This means devices in production today (such as anyone running > the Odroid Go Advance on a mainline kernel) on a battery will > experience this behavior. Setting that register value shouldn't do > anything harmful if you don't have a battery, so I assume if it's safe > to set it 100% of the time for battery users it's safe to set 100% of > the time in total. Which also begs the question, if this is such a > danger to the hardware, should we even be doing it in Linux? We could > be setting it in U-Boot, because if the MFD driver fails to load or the > kernel freezes before the MFD driver loads there is STILL the risk you > could undervolt the battery, regardless of fixing this in the MFD driver > or the battery driver. I believe 2.7V is still fine. The protection circuits may cut off before that because they want to keep the longevity of the battery and there isn't much juice left between 2.7V and 3.0V anyway. If the PMIC always shuts down at 2.7V then there isn't anything to fear really, that's still above the 2.5V that Wikipedia states as being the lower end of the safe voltage range. While that's not the source to end all sources, the PMIC having a lower cutoff probably means that some engineer did think of this and pulled up the correct numbers. Now that I've learned of this, I suspect there isn't really an issue at all here. If we shut off at 2.7V we're good. Userspace should shut down before then, and if there's battery protection that kicks in before that, it makes really no difference to the end result, as long as there is some cutoff somewhere Thanks for your quick responses, my concerns regarding this are alleviated. Devices that are already out there should be fine in this regard. > > Thank you. > > > Regards, > > Nicolas Frattaroli > > > > > > > > Thank you. > > > > > > > > > > > Regards, > > > > Nicolas Frattaroli > > > > > > > > > > > > > > > > > > > >
Hi Chris. Any chance of an updated series based on 5.17-rc1 soon? Keep up the good work. Den 16.09.2021 kl. 21.42 skrev Chris Morgan: > From: Chris Morgan <macromorgan@hotmail.com> > > This series is to add support for the Rockchip rk817 battery charger > which is present in all Rockchip RK817 PMICs. The driver was written > as a joint effort by Maya Matuszczyk <maccraft123mc@gmail.com> and > myself Chris Morgan <macromorgan@hotmail.com>. > > The driver requires some basic parameters be described about the > battery in the devicetree such as the maximum capacity, the minimum > and maximum voltage for the battery, the maximum charge current, the > maximum charge voltage, and the value of sample resistors and sleep > currents. > > The hardware itself contains an ADC capable of measuring the voltage, > current, and temperature of the battery (though my implementation of an > Odroid Go Advance lacks a thermistor for temperature). It also contains > a columb counter, registers for tracking the measured voltage and > current at boot, and a few bytes of nvram for storing data. > > Changes from V3: > > - Corrected issues in device tree documentation. > - Added additional logic to battery to correct for columb counter > drift when the device stays plugged in at a full charge state. > > Changes from V2: > > - Updated devicetree bindings to use common property units. > - Removed unneeded includes. > - Updated rk817_chg_cur_to_reg to make more legible. > - Simplified formula for displaying calibrated voltage. > - Updated power supply type to POWER_SUPPLY_TYPE_USB. > - Implemented get/put_unaligned macros for bulk reads and writes. > - Changed numerous dev_err() to dev_err_probe(). > - Call power_supply_put_battery_info() at end of probe function. > - Removed unneeded whitespace. > > Changes from V1: > > - Fixed a CLANG warning regarding an uninitalized variable. > - Fixed a CLANG warning regarding a pointer as a bool value always > returning as true. > - Added Maya Matuszczyk to the Signed-off-by. > > Chris Morgan (4): > dt-bindings: Add Rockchip rk817 battery charger support > mfd: Add Rockchip rk817 battery charger support > power: supply: Add charger driver for Rockchip RK817 > arm64: dts: rockchip: add rk817 charger to Odroid Go Advance > > .../devicetree/bindings/mfd/rk808.txt | 38 + > .../boot/dts/rockchip/rk3326-odroid-go2.dts | 26 + > drivers/mfd/rk808.c | 16 +- > drivers/power/supply/Kconfig | 6 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/rk817_charger.c | 959 ++++++++++++++++++ > include/linux/mfd/rk808.h | 87 ++ > 7 files changed, 1132 insertions(+), 1 deletion(-) > create mode 100644 drivers/power/supply/rk817_charger.c >
From: Chris Morgan <macromorgan@hotmail.com> This series is to add support for the Rockchip rk817 battery charger which is present in all Rockchip RK817 PMICs. The driver was written as a joint effort by Maya Matuszczyk <maccraft123mc@gmail.com> and myself Chris Morgan <macromorgan@hotmail.com>. The driver requires some basic parameters be described about the battery in the devicetree such as the maximum capacity, the minimum and maximum voltage for the battery, the maximum charge current, the maximum charge voltage, and the value of sample resistors and sleep currents. The hardware itself contains an ADC capable of measuring the voltage, current, and temperature of the battery (though my implementation of an Odroid Go Advance lacks a thermistor for temperature). It also contains a columb counter, registers for tracking the measured voltage and current at boot, and a few bytes of nvram for storing data. Changes from V3: - Corrected issues in device tree documentation. - Added additional logic to battery to correct for columb counter drift when the device stays plugged in at a full charge state. Changes from V2: - Updated devicetree bindings to use common property units. - Removed unneeded includes. - Updated rk817_chg_cur_to_reg to make more legible. - Simplified formula for displaying calibrated voltage. - Updated power supply type to POWER_SUPPLY_TYPE_USB. - Implemented get/put_unaligned macros for bulk reads and writes. - Changed numerous dev_err() to dev_err_probe(). - Call power_supply_put_battery_info() at end of probe function. - Removed unneeded whitespace. Changes from V1: - Fixed a CLANG warning regarding an uninitalized variable. - Fixed a CLANG warning regarding a pointer as a bool value always returning as true. - Added Maya Matuszczyk to the Signed-off-by. Chris Morgan (4): dt-bindings: Add Rockchip rk817 battery charger support mfd: Add Rockchip rk817 battery charger support power: supply: Add charger driver for Rockchip RK817 arm64: dts: rockchip: add rk817 charger to Odroid Go Advance .../devicetree/bindings/mfd/rk808.txt | 38 + .../boot/dts/rockchip/rk3326-odroid-go2.dts | 26 + drivers/mfd/rk808.c | 16 +- drivers/power/supply/Kconfig | 6 + drivers/power/supply/Makefile | 1 + drivers/power/supply/rk817_charger.c | 959 ++++++++++++++++++ include/linux/mfd/rk808.h | 87 ++ 7 files changed, 1132 insertions(+), 1 deletion(-) create mode 100644 drivers/power/supply/rk817_charger.c