linux-kernel - Re: [PATCH 3/3] power: supply: axp20x_usb_power: set input current limit in probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <wavexoz34i5qq53iyb33k4ned37lpwzkr7lstddddjc6cmdkpe@nbd4fymlnrzp>
Date: Mon, 22 Jan 2024 20:40:55 -0500
From: Aren <aren@...cevolution.org>
To: Ondřej Jirman <megi@....cz>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 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 3/3] power: supply: axp20x_usb_power: set input current
 limit in probe

On Sun, Jan 21, 2024 at 12:08:37PM +0100, Ondřej Jirman wrote:
> 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@...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.
> > 
> >  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.

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.

 - Aren

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