diff mbox series

[3/3] power: supply: axp20x_usb_power: set input current limit in probe

Message ID 20240121014057.1042466-7-aren@peacevolution.org
State New
Headers
Series power: supply: axp20x_usb_power: prepare for external power delivery negotiation |

Commit Message

Aren Jan. 21, 2024, 1:40 a.m. UTC 
  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(+)

Comments

Ondřej Jirman Jan. 21, 2024, 11:08 a.m. UTC | #1 
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
>
Aren Jan. 23, 2024, 1:40 a.m. UTC | #2 
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@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.

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
> >
diff mbox series

Patch

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;
+	}
+
 	if (power->usb_bc_en_bit) {
 		/* Enable USB Battery Charging specification detection */
 		ret = regmap_field_write(power->usb_bc_en_bit, 1);