Message-ID: <6nf7h3nc4q7fwrnm4spmgv2sdkczowkfpietcv2tyv4mixkq3b@svxgzkdqnzlq>
Date: Tue, 30 Jan 2024 22:13:06 +0100
From: Ondřej Jirman <megi@....cz>
To: Aren Moynihan <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 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@...cevolution.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
> 

Powered by blists - more mailing lists