Message-ID: <uktr265th6h4btay765p33zgihuzgafu25rz7npwfm3ojhq2tm@wvrymmf3xtxy>
Date: Sun, 11 Feb 2024 00:27:12 +0100
From: Ondřej Jirman <megi@....cz>
To: Aren <aren@...cevolution.org>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Hans de Goede <j.w.r.degoede@...il.com>, Aidan MacDonald <aidanmacdonald.0x0@...il.com>, 
	Chen-Yu Tsai <wens@...e.org>, Quentin Schulz <quentin.schulz@...tlin.com>, 
	Sebastian Reichel <sre@...nel.org>
Subject: Re: [PATCH v2 5/5] power: supply: axp20x_usb_power: set input
 current limit in probe

On Tue, Jan 30, 2024 at 11:20:29PM -0500, Aren wrote:
> On Tue, Jan 30, 2024 at 10:13:06PM +0100, Ondřej Jirman wrote:
> > On Tue, Jan 30, 2024 at 03:28:01PM -0500, Aren Moynihan wrote:
> > > Unfortunately BC 1.2 detection doesn't seem to be performed without a
> > > battery, at least I wasn't able to trigger it.
> > >
> > > This will be worth revising once we have a driver that can provide a
> > > signal that USB-PD is in progress and/or finished, but until then I'd
> > > prefer not take on that complexity.
> > 
> > This patch adds complexity and will lead to hard to debug issues for some
> > people. It certainly did cause issues for me, when I had similar patch in
> > my tree a while ago, forcing me to drop it.
> > 
> > There are other situations you're not considering. Like battery being
> > very discharged and unable to provide power, while still being detected
> > and BC1.2 running correctly and successfully when the device is powered
> > up by being plugged into DCP port (only option of powerup in such a 
> > scenario).
> 
> Oh you're right, I overlooked the case where the battery is very low, in
> which case bc detection should still be performed (I think, I haven't
> tested it). This issue this patch is trying to fix doesn't apply in that
> case, so it should be simple enough to check if the pmic has detected a
> battery and skip setting the current limit if it has.
>
> > Battery is detected at 2.2V and certainly it will not provide any power
> > if OCV of the battery is anywhere below 3V. See "9.4.5 Battery detection"
> > in AXP803 datasheet. So you have about 1V range of possible battery voltage
> > where BC1.2 will work, but you'll force lower the correctly detected current
> > limit and break boot, because 2.5W is too low for the boot time power surge.
> > 
> > The phone will just randomly die halfthrough boot for apparently no reason,
> > despite being connected to DCP.
> > 
> > And also forget Pinephone, there may also be batteryless SBCs using this PMIC
> > with battery as an option (similar to Quartz64-A - Rockchip SBC, but exactly
> > this setup with battery capable PMIC in the power path on a normal SBC, with
> > battery being optional), where this patch will break boot on them, too. I'm
> > quite confident PMIC relaxing the limit without a battery is meant for such use
> > cases.
> 
> Perhaps it would be better to read the limit from the device tree, that
> way it could be set higher for a specific board if it needs to draw that
> much current and be able to boot without a battery? It seems sketchy to
> default to a current limit significantly higher than what the usb power
> supply is required to support.

But is there really an issue? The board may not be using USB power supply.
It may simply have a barrel jack, like Quartz64-A does. And it will still
create an issue if you make the new behavior "opt-out" via DT. You can make
it opt-in if you like.

Also in Pinephone case, you'll not really have a case where the battery has
< 2V not loaded. That's not going to happen. PMIC will shut off at 3V battery
voltage when loaded. It will not discharge further, and after shutoff battery
voltage will jump to 3.4V or so, and it will not drop below 2V after that, ever.
So the battery will pretty much always be detected as long as it's present.

What actual problem have you seen that this patch is trying to solve?

Thank you and kind regards,
	o.

Powered by blists - more mailing lists