Received-SPF: pass (google.com: domain of linux-kernel+bounces-32022-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sun, 21 Jan 2024 12:08:37 +0100
From: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>
To: Aren Moynihan <aren@peacevolution.org>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Aidan MacDonald <aidanmacdonald.0x0@gmail.com>, Chen-Yu Tsai <wens@csie.org>, 
	Quentin Schulz <quentin.schulz@bootlin.com>, Sebastian Reichel <sre@kernel.org>
Subject: Re: [PATCH 3/3] power: supply: axp20x_usb_power: set input current
 limit in probe
Message-ID: <tgejuevbkneg5vmo7lnxf7ml3z2g3jcjweii7lxqp5lrxs6srd@la3rdfiiqbun>
Mail-Followup-To: =?utf-8?Q?Ond=C5=99ej?= Jirman <megi@xff.cz>, 
	Aren Moynihan <aren@peacevolution.org>, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Aidan MacDonald <aidanmacdonald.0x0@gmail.com>, Chen-Yu Tsai <wens@csie.org>, 
	Quentin Schulz <quentin.schulz@bootlin.com>, Sebastian Reichel <sre@kernel.org>
References: <20240121014057.1042466-1-aren@peacevolution.org>
 <20240121014057.1042466-7-aren@peacevolution.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240121014057.1042466-7-aren@peacevolution.org>

Hello Aren,

On Sat, Jan 20, 2024 at 08:40:04PM -0500, Aren Moynihan wrote:
> axp803 sets the current limit to 3A by default if it doesn't detect a
> battery. The datasheet says that reg 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.
> 
> 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.
> 
>  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 5656f6e57d54..95b136ee20f2 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -52,6 +52,7 @@ struct axp_data {
>  	const int			*curr_lim_table;
>  	int				input_curr_lim_table_size;
>  	const int			*input_curr_lim_table;
> +	int				force_input_curr_lim;
>  	struct reg_field		curr_lim_fld;
>  	struct reg_field		input_curr_lim_fld;
>  	struct reg_field		vbus_valid_bit;
> @@ -599,6 +600,7 @@ static const struct axp_data axp813_data = {
>  	.input_curr_lim_table_size = ARRAY_SIZE(axp_813_usb_input_curr_lim_table),
>  	.input_curr_lim_table = axp_813_usb_input_curr_lim_table,
>  	.input_curr_lim_fld = REG_FIELD(AXP22X_CHRG_CTRL3, 4, 7),
> +	.force_input_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),
> @@ -786,6 +788,17 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> +	if (power->axp_data->force_input_curr_lim) {
> +		/*
> +		 * Certain 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_input_curr_lim);
> +		if (ret)
> +			return ret;
> +	}

This will break the usecase of powering Pinephone from USB without a battery
present (or fully discharged).

The phone can require more than 500mA before USB power supply detection manages
to run and sets the higher limit. Linux should keep the limit set by the
bootloader until it figures out something better based on BC 1.2 or USB-PD.

You should only change the limit as the result of negotiation.

Kind regards,
	o.

>  	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
>