[v2,5/5] power: supply: axp20x_usb_power: set input current limit in probe

  axp803 sets the current limit to 3A by default if it doesn't detect a
battery. The datasheet says that register 0x2D bit 6 is used to indicate
first power on status. According to it, if that bit is 0 and the battery
is not detected, it will set the input current limit to 3A, however
setting that bit to 1 doesn't to prevent the pmic from setting the
current limit to 3A.

Waiting for USB BC detection doesn't work either, because (as far as I
can tell) USB BC detection isn't performed when there isn't a battery
detected.

Fixes: c279adafe6ab ("power: supply: axp20x_usb_power: add support for AXP813")

Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---
I'm not sure if the pmic simply ignores the first power on field, or if
it needs to be set in a specific way / at a specific time. I tried
setting it in arm-trusted-firmware, and the pmic still set the input
current limit to 3A.

The datasheet for axp813 says it has the same first power on bit, but I
don't have hardware to test if it behaves the same way. This driver uses
the same platform data for axp803 and axp813.

(no changes since v1)

 drivers/power/supply/axp20x_usb_power.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
  

On Tue, Jan 30, 2024 at 03:28:01PM -0500, Aren Moynihan wrote:
> axp803 sets the current limit to 3A by default if it doesn't detect a
> battery. The datasheet says that register 0x2D bit 6 is used to indicate
> first power on status. According to it, if that bit is 0 and the battery
> is not detected, it will set the input current limit to 3A, however
> setting that bit to 1 doesn't to prevent the pmic from setting the
> current limit to 3A.
> 
> Waiting for USB BC detection doesn't work either, because (as far as I
> can tell) USB BC detection isn't performed when there isn't a battery
> detected.
> 
> Fixes: c279adafe6ab ("power: supply: axp20x_usb_power: add support for AXP813")

Breaks: ;)

Last time you 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).

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.

If you insist on adding this, at least add dev_warn() about forcing lower
limit than detected by the PMIC, so that people who'll do cursory debugging
via serial console will know why's their device failing to boot on a strong
enough power supply, or why their SBC is suddenly failing when used without
battery.

As for me, this patch should not be applied at all.

Kind regards,
        o.

> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> I'm not sure if the pmic simply ignores the first power on field, or if
> it needs to be set in a specific way / at a specific time. I tried
> setting it in arm-trusted-firmware, and the pmic still set the input
> current limit to 3A.
> 
> The datasheet for axp813 says it has the same first power on bit, but I
> don't have hardware to test if it behaves the same way. This driver uses
> the same platform data for axp803 and axp813.
> 
> (no changes since v1)
> 
>  drivers/power/supply/axp20x_usb_power.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index dae7e5cfc54e..751b9f02d36f 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -51,6 +51,7 @@ struct axp_data {
>  	unsigned int			num_irq_names;
>  	const int			*curr_lim_table;
>  	int				curr_lim_table_size;
> +	int				force_curr_lim;
>  	struct reg_field		curr_lim_fld;
>  	struct reg_field		vbus_valid_bit;
>  	struct reg_field		vbus_mon_bit;
> @@ -545,6 +546,7 @@ static const struct axp_data axp813_data = {
>  	.curr_lim_table = axp813_usb_curr_lim_table,
>  	.curr_lim_table_size = ARRAY_SIZE(axp813_usb_curr_lim_table),
>  	.curr_lim_fld	= REG_FIELD(AXP22X_CHRG_CTRL3, 4, 7),
> +	.force_curr_lim = 500000,
>  	.usb_bc_en_bit	= REG_FIELD(AXP288_BC_GLOBAL, 0, 0),
>  	.usb_bc_det_fld = REG_FIELD(AXP288_BC_DET_STAT, 5, 7),
>  	.vbus_disable_bit = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 7, 7),
> @@ -726,6 +728,17 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> +	if (power->axp_data->force_curr_lim) {
> +		/*
> +		 * Some chips set the input current limit to 3A when there is no
> +		 * battery connected. Normally the default is 500mA.
> +		 */
> +		ret = axp20x_usb_power_set_input_current_limit(power,
> +				power->axp_data->force_curr_lim);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (power->usb_bc_en_bit) {
>  		/* Enable USB Battery Charging specification detection */
>  		ret = regmap_field_write(power->usb_bc_en_bit, 1);
> -- 
> 2.43.0
>
  

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:
> > axp803 sets the current limit to 3A by default if it doesn't detect a
> > battery. The datasheet says that register 0x2D bit 6 is used to indicate
> > first power on status. According to it, if that bit is 0 and the battery
> > is not detected, it will set the input current limit to 3A, however
> > setting that bit to 1 doesn't to prevent the pmic from setting the
> > current limit to 3A.
> > 
> > Waiting for USB BC detection doesn't work either, because (as far as I
> > can tell) USB BC detection isn't performed when there isn't a battery
> > detected.
> > 
> > Fixes: c279adafe6ab ("power: supply: axp20x_usb_power: add support for AXP813")
> 
> Breaks: ;)
> 
> Last time you 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.

> If you insist on adding this, at least add dev_warn() about forcing lower
> limit than detected by the PMIC, so that people who'll do cursory debugging
> via serial console will know why's their device failing to boot on a strong
> enough power supply, or why their SBC is suddenly failing when used without
> battery.

Adding a dev_warn is a good idea, I'll do that.

Thanks for the review
 - Aren

> As for me, this patch should not be applied at all.
> 
> Kind regards,
>         o.
> 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> > ---
> > I'm not sure if the pmic simply ignores the first power on field, or if
> > it needs to be set in a specific way / at a specific time. I tried
> > setting it in arm-trusted-firmware, and the pmic still set the input
> > current limit to 3A.
> > 
> > The datasheet for axp813 says it has the same first power on bit, but I
> > don't have hardware to test if it behaves the same way. This driver uses
> > the same platform data for axp803 and axp813.
> > 
> > (no changes since v1)
> > 
> >  drivers/power/supply/axp20x_usb_power.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> > index dae7e5cfc54e..751b9f02d36f 100644
> > --- a/drivers/power/supply/axp20x_usb_power.c
> > +++ b/drivers/power/supply/axp20x_usb_power.c
> > @@ -51,6 +51,7 @@ struct axp_data {
> >  	unsigned int			num_irq_names;
> >  	const int			*curr_lim_table;
> >  	int				curr_lim_table_size;
> > +	int				force_curr_lim;
> >  	struct reg_field		curr_lim_fld;
> >  	struct reg_field		vbus_valid_bit;
> >  	struct reg_field		vbus_mon_bit;
> > @@ -545,6 +546,7 @@ static const struct axp_data axp813_data = {
> >  	.curr_lim_table = axp813_usb_curr_lim_table,
> >  	.curr_lim_table_size = ARRAY_SIZE(axp813_usb_curr_lim_table),
> >  	.curr_lim_fld	= REG_FIELD(AXP22X_CHRG_CTRL3, 4, 7),
> > +	.force_curr_lim = 500000,
> >  	.usb_bc_en_bit	= REG_FIELD(AXP288_BC_GLOBAL, 0, 0),
> >  	.usb_bc_det_fld = REG_FIELD(AXP288_BC_DET_STAT, 5, 7),
> >  	.vbus_disable_bit = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 7, 7),
> > @@ -726,6 +728,17 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
> >  			return ret;
> >  	}
> >  
> > +	if (power->axp_data->force_curr_lim) {
> > +		/*
> > +		 * Some chips set the input current limit to 3A when there is no
> > +		 * battery connected. Normally the default is 500mA.
> > +		 */
> > +		ret = axp20x_usb_power_set_input_current_limit(power,
> > +				power->axp_data->force_curr_lim);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >  	if (power->usb_bc_en_bit) {
> >  		/* Enable USB Battery Charging specification detection */
> >  		ret = regmap_field_write(power->usb_bc_en_bit, 1);
> > -- 
> > 2.43.0
> >
  

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.