From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip Date: Thu, 23 Mar 2017 12:48:55 +0100 Message-ID: <14857b8e-8adf-2f07-64cd-8923c12eb030@redhat.com> References: <20170322145536.30570-1-hdegoede@redhat.com> <20170322145536.30570-5-hdegoede@redhat.com> <20170322154313.GU8575@atomide.com> <2ab0b7d5-61b0-2a1f-7c45-07d7f3f180b6@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:58486 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751567AbdCWLtJ (ORCPT ); Thu, 23 Mar 2017 07:49:09 -0400 In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Liam Breck Cc: Tony Lindgren , Sebastian Reichel , Takashi Iwai , linux-pm@vger.kernel.org, Liam Breck Hi, On 23-03-17 12:47, Liam Breck wrote: > On Thu, Mar 23, 2017 at 4:44 AM, Hans de Goede wrote: >> Hi, >> >> >> On 23-03-17 12:36, Liam Breck wrote: >>> >>> On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede >>> wrote: >>>> >>>> Hi, >>>> >>>> >>>> On 23-03-17 08:16, Liam Breck wrote: >>>>> >>>>> >>>>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede >>>>> wrote: >>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> On 22-03-17 16:43, Tony Lindgren wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> * Hans de Goede [170322 07:57]: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> 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. >>>>>> >>>>>> >>>>>>>> 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." >>>> >>>>> 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 ? >>>> >>>>> 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. > > I'll try it. > > Why would you not want set_mode_host()? Doing set_mode_host() on resume is redundant if we do not reset the chip on suspend/resume. Note we still want the set_mode_host on boot / probe and my patch preservers that set_mode_host call. Regards, Hans