On Thu, Mar 23, 2017 at 3:01 PM, Hans de Goede [off-list ref] wrote:

Hi,


On 23-03-17 21:33, Liam Breck wrote:

quoted

On Thu, Mar 23, 2017 at 4:44 AM, Hans de Goede [off-list ref]
wrote:

quoted

Hi,


On 23-03-17 12:36, Liam Breck wrote:

quoted

On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede [off-list ref]
wrote:

quoted

Hi,


On 23-03-17 08:16, Liam Breck wrote:

quoted

On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede [off-list ref]
wrote:

quoted

Hi,

On 22-03-17 16:43, Tony Lindgren wrote:

quoted

* Hans de Goede [off-list ref] [170322 07:57]:

quoted

Resetting the charger should never be necessary it should always
have
sane values programmed. If it is running with invalid values while
we
are not running (system turned off or suspended) there is a big
problem
as that may lead to overcharging the battery.

And some systems may only configure it in the bootloader with no
configuration passed to the kernel at all.

Right.

quoted

quoted

The reset in suspend() is meant to put the charger back into
default
mode, but this is not necessary and not a good idea. If the charger
has
been programmed with a higher max charge_current / charge_voltage
then
putting it back in default-mode will reset those to the safe
power-on
defaults, leading to slower charging, or charging to a lower
voltage
(and thus not using the full capacity) while suspended which is
undesirable. Reprogramming the max charge_current / charge_voltage
after the reset will not help here as that will put the charger
back
in host mode and start the i2c watchdog if the host then does not
do
anything for 40s (iow if we're suspended for more then 40s) the
watchdog
expires resetting the device to default-mode, including resetting
all
the registers to there safe power-on defaults. So the only way to
keep
using custom charge settings while suspending is to keep the
charger
in
its normal running state with the i2c watchdog disabled. This is
fine
as the charger will still automatically switch from constant
current
to constant voltage and stop charging when the battery is full.

Besides never being necessary resetting the charger also causes
problems
on systems where the charge voltage limit is set higher then the
reset
value, if this is the case and the charger is reset while charging
and
the battery voltage is between the 2 voltages, then about half the
time
the charger gets confused and claims to be charging (REG08 contains
0x64)
but in reality the charger has decoupled itself from VBUS (Q1 off)
and
is drawing 0A from VBUS, leaving the system running from the
battery.

We do have cases where Linux kernel is the bootloader though using
kexec. And in those cases we may need to reset, so I wonder if the
reset should just be optional based on a proper device tree
(or platform_data) configuration?

As described above during my testing I've found that the chip does
not
respond well to reset under certain conditions, so I think that if
we have settings to apply we should just overwrite the settings with
our settings, what does doing a reset before overwriting the
registers
buy us? We still need to do the overwrite anyways.

When driver loads on any conceivable hw, how do we know the chip is in
a state we predicted?

Any non reset values have to come from somewhere, likely firmware,
and unless we've platform data telling us the exact settings we should
use why would we decide the reset values are better then the ones
the charger was *already using* before our driver loads ? As said in
the commit msg: "If it is running with invalid values while we
are not running (system turned off or suspended) there is a big problem
as that may lead to overcharging the battery."

quoted

If it isn't, how can we fix any undesirable
settings?

Why would the chip not be in a sane state? Where would the undesirable
settings come from ?

quoted

Reset + fix a,b,c is simple way to do that.

As my testing using real-world hardware under real-world conditions
has shown resetting the charger IC while it is charging can cause it
to misbehave. So we have this theoretical problem of there somehow
being undesirable settings at boot vs a real-world problem...

A bug in the charger? Or in the platform hw/fw?

I'm pretty sure the fw is not touching the charger, still the problem
might be something specific to my hw. Either way there does not seem
to be a good reason to reset the chip and doing a reset despite it
not being necessary does cause real problems independently of the
root cause (I'm running this on production hardware which many
people have, so no way to modify the hardware).

If you want to test this on your systems you can easily add a
patch on top of your current branch which just comments out
all the reset calls (and the bq24190_set_mode_host call in
resume) to get the same result on top of your current branch.

My mistake... what I need to try is reproducing your failure due to
reset during charge. Can you explain what switches I need to flip and
dials to watch?

The GPDwin mini laptop I'm developing this on /for has a bq24292i
(bq24192i compatible) with a LiHV battery and max charging
voltage set to 4.38V When resetting the charger while the
battery voltage is between 4.1V (reset max volt) and 4.38V
and the charger is charging. Sometimes instead of just
turning of charging as the bat voltage is now above the max
voltage, it turns of Q1 (disconnects itself from Vbus) while
it thinks it is still charging (observed from REG08 as well
as the charging LED.

Thanks. It will take me a few days to get to this.

In the meantime, note that on a DT-configured system, we don't want
any charger settings made by the bootloader, as we're going to change
default settings (e.g. precharge & endcharge from my patchset) based
on DT params.