Re: [PATCH v4 RESEND 0/4] power: supply: Add Support for RK817 Charger

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:

quoted

On Donnerstag, 16. September 2021 21:42:04 CET Chris Morgan wrote:

quoted

From: Chris Morgan <redacted>

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 [off-list ref] and
myself Chris Morgan [off-list ref].

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.

quoted

Regards,
Nicolas Frattaroli