[PATCH 0/3] power: supply: axp20x_usb_power: prepare for external power delivery negotiation

[PATCH 0/3] power: supply: axp20x_usb_power: prepare for external power delivery negotiation

[Aren Moynihan]

Being able to set the input current limit will be necessary to implement
usb power delivery on the Pine64 PinePhone. The PinePhone has a separate
chip (anx7688) that handles usb power delivery (among other things), so
we will need a way to apply the current limit which that negotiates.

The first two patches were originally based on code Ondřej Jirman
wrote[1], but I have almost completely rewritten them.

While working on this, I also discovered that the axp803 pmic sets a
current limit of 3A on the usb port without any negotiation if it
doesn't detect a battery.

1: https://xff.cz/git/linux/commit/?h=axp-6.7&id=3dcd33dfd1ae58db159427365dcb6d0d2b12f06d


Aren Moynihan (3):
  power: supply: axp20x_usb_power: add input current limit
  power: supply: axp20x_usb_power: enable usb_type reporting
  power: supply: axp20x_usb_power: set input current limit in probe

 drivers/power/supply/axp20x_usb_power.c | 192 +++++++++++++++++++++++-
 1 file changed, 190 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH 1/3] power: supply: axp20x_usb_power: add input current limit

[Aren Moynihan]

Add properties for setting the maximum current that will be drawn from
the usb connection.

These changes don't apply to all axp20x chips, so we need to add new
power_desc and power supply property objects for axp813 specifically.
These are copied from the axp22x variants that were used before, with
extra fields added.

Also add a dev field to the axp20x_usb_power struct, so we can use
dev_dbg and dev_err in more places.

Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

 drivers/power/supply/axp20x_usb_power.c | 127 +++++++++++++++++++++++-
 1 file changed, 125 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index e23308ad4cc7..8c0c2c25565f 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -50,7 +50,10 @@ struct axp_data {
 	const char * const		*irq_names;
 	unsigned int			num_irq_names;
 	const int			*curr_lim_table;
+	int				input_curr_lim_table_size;
+	const int			*input_curr_lim_table;
 	struct reg_field		curr_lim_fld;
+	struct reg_field		input_curr_lim_fld;
 	struct reg_field		vbus_valid_bit;
 	struct reg_field		vbus_mon_bit;
 	struct reg_field		usb_bc_en_bit;
@@ -59,7 +62,9 @@ struct axp_data {
 };
 
 struct axp20x_usb_power {
+	struct device *dev;
 	struct regmap *regmap;
+	struct regmap_field *input_curr_lim_fld;
 	struct regmap_field *curr_lim_fld;
 	struct regmap_field *vbus_valid_bit;
 	struct regmap_field *vbus_mon_bit;
@@ -115,6 +120,15 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
 	if (val != power->old_status)
 		power_supply_changed(power->supply);
 
+	if (power->usb_bc_en_bit && (val & AXP20X_PWR_STATUS_VBUS_PRESENT) !=
+		(power->old_status & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
+		dev_dbg(power->dev, "Cable status changed, re-enabling USB BC");
+		ret = regmap_field_write(power->usb_bc_en_bit, 1);
+		if (ret)
+			dev_err(power->dev, "failed to enable USB BC: errno %d",
+				ret);
+	}
+
 	power->old_status = val;
 	power->online = val & AXP20X_PWR_STATUS_VBUS_USED;
 
@@ -123,6 +137,66 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
 		mod_delayed_work(system_power_efficient_wq, &power->vbus_detect, DEBOUNCE_TIME);
 }
 
+static int
+axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *power,
+					 int limit)
+{
+	int ret;
+	unsigned int reg;
+
+	if (!power->axp_data->input_curr_lim_table)
+		return -EINVAL;
+
+	if (limit < power->axp_data->input_curr_lim_table[0])
+		return -EINVAL;
+
+	/*
+	 * BC1.2 detection can cause a race condition if we try to set a current
+	 * limit while it's in progress. When it finishes it will overwrite the
+	 * current limit we just set.
+	 */
+	if (power->usb_bc_en_bit) {
+		dev_dbg(power->dev,
+			"disabling BC1.2 detection because current limit was set");
+		ret = regmap_field_write(power->usb_bc_en_bit, 0);
+		if (ret)
+			return ret;
+	}
+
+	for (reg = power->axp_data->input_curr_lim_table_size - 1; reg > 0; reg--) {
+		if (limit >= power->axp_data->input_curr_lim_table[reg])
+			break;
+	}
+
+	dev_dbg(power->dev, "setting input current limit reg to %d (%d uA), requested %d uA",
+		reg, power->axp_data->input_curr_lim_table[reg], limit);
+
+	return regmap_field_write(power->input_curr_lim_fld, reg);
+}
+
+static int
+axp20x_usb_power_get_input_current_limit(struct axp20x_usb_power *power,
+					 int *intval)
+{
+	int ret;
+	unsigned int v;
+
+	if (!power->axp_data->input_curr_lim_table)
+		return -EINVAL;
+
+	ret = regmap_field_read(power->input_curr_lim_fld, &v);
+	if (ret)
+		return ret;
+
+	if (v < power->axp_data->input_curr_lim_table_size)
+		*intval = power->axp_data->input_curr_lim_table[v];
+	else
+		*intval = power->axp_data->input_curr_lim_table[
+			  power->axp_data->input_curr_lim_table_size - 1];
+
+	return 0;
+}
+
 static int axp20x_usb_power_get_property(struct power_supply *psy,
 	enum power_supply_property psp, union power_supply_propval *val)
 {
@@ -189,6 +263,9 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 
 		val->intval = ret * 375; /* 1 step = 0.375 mA */
 		return 0;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return axp20x_usb_power_get_input_current_limit(power,
+								&val->intval);
 	default:
 		break;
 	}
@@ -290,6 +367,10 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CURRENT_MAX:
 		return axp20x_usb_power_set_current_max(power, val->intval);
 
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		return axp20x_usb_power_set_input_current_limit(power,
+								val->intval);
+
 	default:
 		return -EINVAL;
 	}
@@ -313,7 +394,8 @@ static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
 		return power->vbus_disable_bit != NULL;
 
 	return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
-	       psp == POWER_SUPPLY_PROP_CURRENT_MAX;
+	       psp == POWER_SUPPLY_PROP_CURRENT_MAX ||
+	       psp == POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT;
 }
 
 static enum power_supply_property axp20x_usb_power_properties[] = {
@@ -334,6 +416,15 @@ static enum power_supply_property axp22x_usb_power_properties[] = {
 	POWER_SUPPLY_PROP_CURRENT_MAX,
 };
 
+static enum power_supply_property axp813_usb_power_properties[] = {
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+};
+
 static const struct power_supply_desc axp20x_usb_power_desc = {
 	.name = "axp20x-usb",
 	.type = POWER_SUPPLY_TYPE_USB,
@@ -354,6 +445,16 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
 	.set_property = axp20x_usb_power_set_property,
 };
 
+static const struct power_supply_desc axp813_usb_power_desc = {
+	.name = "axp20x-usb",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.properties = axp813_usb_power_properties,
+	.num_properties = ARRAY_SIZE(axp813_usb_power_properties),
+	.property_is_writeable = axp20x_usb_power_prop_writeable,
+	.get_property = axp20x_usb_power_get_property,
+	.set_property = axp20x_usb_power_set_property,
+};
+
 static const char * const axp20x_irq_names[] = {
 	"VBUS_PLUGIN",
 	"VBUS_REMOVAL",
@@ -394,6 +495,18 @@ static int axp813_usb_curr_lim_table[] = {
 	2500000,
 };
 
+static int axp_813_usb_input_curr_lim_table[] = {
+	100000,
+	500000,
+	900000,
+	1500000,
+	2000000,
+	2500000,
+	3000000,
+	3500000,
+	4000000,
+};
+
 static const struct axp_data axp192_data = {
 	.power_desc	= &axp20x_usb_power_desc,
 	.irq_names	= axp20x_irq_names,
@@ -433,11 +546,14 @@ static const struct axp_data axp223_data = {
 };
 
 static const struct axp_data axp813_data = {
-	.power_desc	= &axp22x_usb_power_desc,
+	.power_desc	= &axp813_usb_power_desc,
 	.irq_names	= axp22x_irq_names,
 	.num_irq_names	= ARRAY_SIZE(axp22x_irq_names),
 	.curr_lim_table = axp813_usb_curr_lim_table,
 	.curr_lim_fld   = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 0, 1),
+	.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),
 	.usb_bc_en_bit	= REG_FIELD(AXP288_BC_GLOBAL, 0, 0),
 	.vbus_disable_bit = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 7, 7),
 	.vbus_needs_polling = true,
@@ -558,6 +674,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, power);
 
+	power->dev = &pdev->dev;
 	power->axp_data = axp_data;
 	power->regmap = axp20x->regmap;
 	power->num_irqs = axp_data->num_irq_names;
@@ -567,6 +684,12 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	if (IS_ERR(power->curr_lim_fld))
 		return PTR_ERR(power->curr_lim_fld);
 
+	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
+						 axp_data->input_curr_lim_fld,
+						 &power->input_curr_lim_fld);
+	if (ret)
+		return ret;
+
 	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
 						 axp_data->vbus_valid_bit,
 						 &power->vbus_valid_bit);
-- 
2.43.0

[PATCH 2/3] power: supply: axp20x_usb_power: enable usb_type reporting

[Aren Moynihan]

Axp803 and axp813 can report the detected usb bc mode. SDP, CDP, and DCP
are supported.

Signed-off-by: Aren Moynihan <aren@peacevolution.org>
---

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

diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 8c0c2c25565f..5656f6e57d54 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -57,6 +57,7 @@ struct axp_data {
 	struct reg_field		vbus_valid_bit;
 	struct reg_field		vbus_mon_bit;
 	struct reg_field		usb_bc_en_bit;
+	struct reg_field		usb_bc_det_fld;
 	struct reg_field		vbus_disable_bit;
 	bool				vbus_needs_polling: 1;
 };
@@ -69,6 +70,7 @@ struct axp20x_usb_power {
 	struct regmap_field *vbus_valid_bit;
 	struct regmap_field *vbus_mon_bit;
 	struct regmap_field *usb_bc_en_bit;
+	struct regmap_field *usb_bc_det_fld;
 	struct regmap_field *vbus_disable_bit;
 	struct power_supply *supply;
 	const struct axp_data *axp_data;
@@ -197,6 +199,37 @@ axp20x_usb_power_get_input_current_limit(struct axp20x_usb_power *power,
 	return 0;
 }
 
+static int axp20x_get_usb_type(struct axp20x_usb_power *power,
+			       union power_supply_propval *val)
+{
+	unsigned int reg;
+	int ret;
+
+	if (!power->usb_bc_det_fld)
+		return -EINVAL;
+
+	ret = regmap_field_read(power->usb_bc_det_fld, &reg);
+	if (ret)
+		return ret;
+
+	switch (reg) {
+	case 1:
+		val->intval = POWER_SUPPLY_USB_TYPE_SDP;
+		break;
+	case 2:
+		val->intval = POWER_SUPPLY_USB_TYPE_CDP;
+		break;
+	case 3:
+		val->intval = POWER_SUPPLY_USB_TYPE_DCP;
+		break;
+	default:
+		val->intval = POWER_SUPPLY_USB_TYPE_UNKNOWN;
+		break;
+	}
+
+	return 0;
+}
+
 static int axp20x_usb_power_get_property(struct power_supply *psy,
 	enum power_supply_property psp, union power_supply_propval *val)
 {
@@ -266,6 +299,8 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		return axp20x_usb_power_get_input_current_limit(power,
 								&val->intval);
+	case POWER_SUPPLY_PROP_USB_TYPE:
+		return axp20x_get_usb_type(power, val);
 	default:
 		break;
 	}
@@ -423,6 +458,14 @@ static enum power_supply_property axp813_usb_power_properties[] = {
 	POWER_SUPPLY_PROP_VOLTAGE_MIN,
 	POWER_SUPPLY_PROP_CURRENT_MAX,
 	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+	POWER_SUPPLY_PROP_USB_TYPE,
+};
+
+static enum power_supply_usb_type axp813_usb_types[] = {
+	POWER_SUPPLY_USB_TYPE_SDP,
+	POWER_SUPPLY_USB_TYPE_DCP,
+	POWER_SUPPLY_USB_TYPE_CDP,
+	POWER_SUPPLY_USB_TYPE_UNKNOWN,
 };
 
 static const struct power_supply_desc axp20x_usb_power_desc = {
@@ -453,6 +496,8 @@ static const struct power_supply_desc axp813_usb_power_desc = {
 	.property_is_writeable = axp20x_usb_power_prop_writeable,
 	.get_property = axp20x_usb_power_get_property,
 	.set_property = axp20x_usb_power_set_property,
+	.usb_types = axp813_usb_types,
+	.num_usb_types = ARRAY_SIZE(axp813_usb_types),
 };
 
 static const char * const axp20x_irq_names[] = {
@@ -555,6 +600,7 @@ static const struct axp_data axp813_data = {
 	.input_curr_lim_table = axp_813_usb_input_curr_lim_table,
 	.input_curr_lim_fld = REG_FIELD(AXP22X_CHRG_CTRL3, 4, 7),
 	.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),
 	.vbus_needs_polling = true,
 };
@@ -708,6 +754,12 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
+						 axp_data->usb_bc_det_fld,
+						 &power->usb_bc_det_fld);
+	if (ret)
+		return ret;
+
 	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
 						 axp_data->vbus_disable_bit,
 						 &power->vbus_disable_bit);
-- 
2.43.0

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

[Aren Moynihan]

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;
+	}
+
 	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

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

[Ondřej Jirman]

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
> 

Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input current limit

[Ondřej Jirman]

Hello Aren,

On Sat, Jan 20, 2024 at 08:40:00PM -0500, Aren Moynihan wrote:
> Add properties for setting the maximum current that will be drawn from
> the usb connection.
> 
> These changes don't apply to all axp20x chips, so we need to add new
> power_desc and power supply property objects for axp813 specifically.
> These are copied from the axp22x variants that were used before, with
> extra fields added.
> 
> Also add a dev field to the axp20x_usb_power struct, so we can use
> dev_dbg and dev_err in more places.
> 
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> 
>  drivers/power/supply/axp20x_usb_power.c | 127 +++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index e23308ad4cc7..8c0c2c25565f 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -50,7 +50,10 @@ struct axp_data {
>  	const char * const		*irq_names;
>  	unsigned int			num_irq_names;
>  	const int			*curr_lim_table;
> +	int				input_curr_lim_table_size;
> +	const int			*input_curr_lim_table;
>  	struct reg_field		curr_lim_fld;
> +	struct reg_field		input_curr_lim_fld;
>  	struct reg_field		vbus_valid_bit;
>  	struct reg_field		vbus_mon_bit;
>  	struct reg_field		usb_bc_en_bit;
> @@ -59,7 +62,9 @@ struct axp_data {
>  };
>  
>  struct axp20x_usb_power {
> +	struct device *dev;
>  	struct regmap *regmap;
> +	struct regmap_field *input_curr_lim_fld;
>  	struct regmap_field *curr_lim_fld;
>  	struct regmap_field *vbus_valid_bit;
>  	struct regmap_field *vbus_mon_bit;
> @@ -115,6 +120,15 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
>  	if (val != power->old_status)
>  		power_supply_changed(power->supply);
>  
> +	if (power->usb_bc_en_bit && (val & AXP20X_PWR_STATUS_VBUS_PRESENT) !=
> +		(power->old_status & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +		dev_dbg(power->dev, "Cable status changed, re-enabling USB BC");
> +		ret = regmap_field_write(power->usb_bc_en_bit, 1);
> +		if (ret)
> +			dev_err(power->dev, "failed to enable USB BC: errno %d",
> +				ret);
> +	}
> +
>  	power->old_status = val;
>  	power->online = val & AXP20X_PWR_STATUS_VBUS_USED;
>  
> @@ -123,6 +137,66 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
>  		mod_delayed_work(system_power_efficient_wq, &power->vbus_detect, DEBOUNCE_TIME);
>  }
>  
> +static int
> +axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *power,
> +					 int limit)
> +{
> +	int ret;
> +	unsigned int reg;
> +
> +	if (!power->axp_data->input_curr_lim_table)
> +		return -EINVAL;
> +
> +	if (limit < power->axp_data->input_curr_lim_table[0])
> +		return -EINVAL;

I think that you should just set the lowest possible limit. A caller (calling
driver or userspace or user) has no way to identify what is the lowest possible
limit on arbitrary PMIC and I kinda like having an option to
echo 0 > .../axp20x-usb/input_current_limit as a way to limit power consumption from
USB to a minimum without having to look up what actual minimum is in various
PMICs dataheets.

I looked through most of the uses of POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT in
the upstream drivers and both approaches are used. Erroring out when out of range
is less common.

Otherwise,

Reviewed-By: Ondrej Jirman <megi@xff.cz>

Thank you,
	Ondrej

Re: [PATCH 2/3] power: supply: axp20x_usb_power: enable usb_type reporting

[Ondřej Jirman]

Hello Aren,

On Sat, Jan 20, 2024 at 08:40:02PM -0500, Aren Moynihan wrote:
> Axp803 and axp813 can report the detected usb bc mode. SDP, CDP, and DCP
> are supported.
> 
> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
> 
>  drivers/power/supply/axp20x_usb_power.c | 52 +++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index 8c0c2c25565f..5656f6e57d54 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -57,6 +57,7 @@ struct axp_data {
>  	struct reg_field		vbus_valid_bit;
>  	struct reg_field		vbus_mon_bit;
>  	struct reg_field		usb_bc_en_bit;
> +	struct reg_field		usb_bc_det_fld;
>  	struct reg_field		vbus_disable_bit;
>  	bool				vbus_needs_polling: 1;
>  };
> @@ -69,6 +70,7 @@ struct axp20x_usb_power {
>  	struct regmap_field *vbus_valid_bit;
>  	struct regmap_field *vbus_mon_bit;
>  	struct regmap_field *usb_bc_en_bit;
> +	struct regmap_field *usb_bc_det_fld;
>  	struct regmap_field *vbus_disable_bit;
>  	struct power_supply *supply;
>  	const struct axp_data *axp_data;
> @@ -197,6 +199,37 @@ axp20x_usb_power_get_input_current_limit(struct axp20x_usb_power *power,
>  	return 0;
>  }
>  
> +static int axp20x_get_usb_type(struct axp20x_usb_power *power,
> +			       union power_supply_propval *val)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	if (!power->usb_bc_det_fld)
> +		return -EINVAL;

What if detection is disabled becase custom input current limit was set by the
user? Does the HW keep the result of previous detection in the register?

Kind regards,
	Ondrej

> +	ret = regmap_field_read(power->usb_bc_det_fld, &reg);
> +	if (ret)
> +		return ret;
> +
> +	switch (reg) {
> +	case 1:
> +		val->intval = POWER_SUPPLY_USB_TYPE_SDP;
> +		break;
> +	case 2:
> +		val->intval = POWER_SUPPLY_USB_TYPE_CDP;
> +		break;
> +	case 3:
> +		val->intval = POWER_SUPPLY_USB_TYPE_DCP;
> +		break;
> +	default:
> +		val->intval = POWER_SUPPLY_USB_TYPE_UNKNOWN;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static int axp20x_usb_power_get_property(struct power_supply *psy,
>  	enum power_supply_property psp, union power_supply_propval *val)
>  {
> @@ -266,6 +299,8 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>  		return axp20x_usb_power_get_input_current_limit(power,
>  								&val->intval);
> +	case POWER_SUPPLY_PROP_USB_TYPE:
> +		return axp20x_get_usb_type(power, val);
>  	default:
>  		break;
>  	}
> @@ -423,6 +458,14 @@ static enum power_supply_property axp813_usb_power_properties[] = {
>  	POWER_SUPPLY_PROP_VOLTAGE_MIN,
>  	POWER_SUPPLY_PROP_CURRENT_MAX,
>  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +	POWER_SUPPLY_PROP_USB_TYPE,
> +};
> +
> +static enum power_supply_usb_type axp813_usb_types[] = {
> +	POWER_SUPPLY_USB_TYPE_SDP,
> +	POWER_SUPPLY_USB_TYPE_DCP,
> +	POWER_SUPPLY_USB_TYPE_CDP,
> +	POWER_SUPPLY_USB_TYPE_UNKNOWN,
>  };
>  
>  static const struct power_supply_desc axp20x_usb_power_desc = {
> @@ -453,6 +496,8 @@ static const struct power_supply_desc axp813_usb_power_desc = {
>  	.property_is_writeable = axp20x_usb_power_prop_writeable,
>  	.get_property = axp20x_usb_power_get_property,
>  	.set_property = axp20x_usb_power_set_property,
> +	.usb_types = axp813_usb_types,
> +	.num_usb_types = ARRAY_SIZE(axp813_usb_types),
>  };
>  
>  static const char * const axp20x_irq_names[] = {
> @@ -555,6 +600,7 @@ static const struct axp_data axp813_data = {
>  	.input_curr_lim_table = axp_813_usb_input_curr_lim_table,
>  	.input_curr_lim_fld = REG_FIELD(AXP22X_CHRG_CTRL3, 4, 7),
>  	.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),
>  	.vbus_needs_polling = true,
>  };
> @@ -708,6 +754,12 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
> +						 axp_data->usb_bc_det_fld,
> +						 &power->usb_bc_det_fld);
> +	if (ret)
> +		return ret;
> +
>  	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
>  						 axp_data->vbus_disable_bit,
>  						 &power->vbus_disable_bit);
> -- 
> 2.43.0
> 

Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input current limit

[Chen-Yu Tsai]

Hi,

+CC Hans de Goede since the AXP288 is similar to the later AXPs in the USB
power section.

On Sun, Jan 21, 2024 at 9:54 AM Aren Moynihan <aren@peacevolution.org> wrote:
>
> Add properties for setting the maximum current that will be drawn from
> the usb connection.
>
> These changes don't apply to all axp20x chips, so we need to add new
> power_desc and power supply property objects for axp813 specifically.
> These are copied from the axp22x variants that were used before, with
> extra fields added.
>
> Also add a dev field to the axp20x_usb_power struct, so we can use
> dev_dbg and dev_err in more places.

I think this patch highlights some errors in the driver.

1. We are likely misusing POWER_SUPPLY_PROP_CURRENT_MAX. Based on the
   ABI docs, this is supposed to be a read only property. The correct
   way would be to use POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT as you did.

2. For AXP288 and AXP803/AXP813/AXP818, we are setting the current limit
   for when BC detection finishes in register 0x30, not the actual active
   limit in register 0x35. This should be fixed.

3. BC detection race condition

Could you maybe respin the patches to address them separately instead?


Thanks
ChenYu



> Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> ---
>
>  drivers/power/supply/axp20x_usb_power.c | 127 +++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> index e23308ad4cc7..8c0c2c25565f 100644
> --- a/drivers/power/supply/axp20x_usb_power.c
> +++ b/drivers/power/supply/axp20x_usb_power.c
> @@ -50,7 +50,10 @@ struct axp_data {
>         const char * const              *irq_names;
>         unsigned int                    num_irq_names;
>         const int                       *curr_lim_table;
> +       int                             input_curr_lim_table_size;
> +       const int                       *input_curr_lim_table;
>         struct reg_field                curr_lim_fld;
> +       struct reg_field                input_curr_lim_fld;
>         struct reg_field                vbus_valid_bit;
>         struct reg_field                vbus_mon_bit;
>         struct reg_field                usb_bc_en_bit;
> @@ -59,7 +62,9 @@ struct axp_data {
>  };
>
>  struct axp20x_usb_power {
> +       struct device *dev;
>         struct regmap *regmap;
> +       struct regmap_field *input_curr_lim_fld;
>         struct regmap_field *curr_lim_fld;
>         struct regmap_field *vbus_valid_bit;
>         struct regmap_field *vbus_mon_bit;
> @@ -115,6 +120,15 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
>         if (val != power->old_status)
>                 power_supply_changed(power->supply);
>
> +       if (power->usb_bc_en_bit && (val & AXP20X_PWR_STATUS_VBUS_PRESENT) !=
> +               (power->old_status & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> +               dev_dbg(power->dev, "Cable status changed, re-enabling USB BC");
> +               ret = regmap_field_write(power->usb_bc_en_bit, 1);
> +               if (ret)
> +                       dev_err(power->dev, "failed to enable USB BC: errno %d",
> +                               ret);
> +       }
> +
>         power->old_status = val;
>         power->online = val & AXP20X_PWR_STATUS_VBUS_USED;
>
> @@ -123,6 +137,66 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
>                 mod_delayed_work(system_power_efficient_wq, &power->vbus_detect, DEBOUNCE_TIME);
>  }
>
> +static int
> +axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *power,
> +                                        int limit)
> +{
> +       int ret;
> +       unsigned int reg;
> +
> +       if (!power->axp_data->input_curr_lim_table)
> +               return -EINVAL;
> +
> +       if (limit < power->axp_data->input_curr_lim_table[0])
> +               return -EINVAL;
> +
> +       /*
> +        * BC1.2 detection can cause a race condition if we try to set a current
> +        * limit while it's in progress. When it finishes it will overwrite the
> +        * current limit we just set.
> +        */
> +       if (power->usb_bc_en_bit) {
> +               dev_dbg(power->dev,
> +                       "disabling BC1.2 detection because current limit was set");
> +               ret = regmap_field_write(power->usb_bc_en_bit, 0);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       for (reg = power->axp_data->input_curr_lim_table_size - 1; reg > 0; reg--) {
> +               if (limit >= power->axp_data->input_curr_lim_table[reg])
> +                       break;
> +       }
> +
> +       dev_dbg(power->dev, "setting input current limit reg to %d (%d uA), requested %d uA",
> +               reg, power->axp_data->input_curr_lim_table[reg], limit);
> +
> +       return regmap_field_write(power->input_curr_lim_fld, reg);
> +}
> +
> +static int
> +axp20x_usb_power_get_input_current_limit(struct axp20x_usb_power *power,
> +                                        int *intval)
> +{
> +       int ret;
> +       unsigned int v;
> +
> +       if (!power->axp_data->input_curr_lim_table)
> +               return -EINVAL;
> +
> +       ret = regmap_field_read(power->input_curr_lim_fld, &v);
> +       if (ret)
> +               return ret;
> +
> +       if (v < power->axp_data->input_curr_lim_table_size)
> +               *intval = power->axp_data->input_curr_lim_table[v];
> +       else
> +               *intval = power->axp_data->input_curr_lim_table[
> +                         power->axp_data->input_curr_lim_table_size - 1];
> +
> +       return 0;
> +}
> +
>  static int axp20x_usb_power_get_property(struct power_supply *psy,
>         enum power_supply_property psp, union power_supply_propval *val)
>  {
> @@ -189,6 +263,9 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
>
>                 val->intval = ret * 375; /* 1 step = 0.375 mA */
>                 return 0;
> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +               return axp20x_usb_power_get_input_current_limit(power,
> +                                                               &val->intval);
>         default:
>                 break;
>         }
> @@ -290,6 +367,10 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
>         case POWER_SUPPLY_PROP_CURRENT_MAX:
>                 return axp20x_usb_power_set_current_max(power, val->intval);
>
> +       case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +               return axp20x_usb_power_set_input_current_limit(power,
> +                                                               val->intval);
> +
>         default:
>                 return -EINVAL;
>         }
> @@ -313,7 +394,8 @@ static int axp20x_usb_power_prop_writeable(struct power_supply *psy,
>                 return power->vbus_disable_bit != NULL;
>
>         return psp == POWER_SUPPLY_PROP_VOLTAGE_MIN ||
> -              psp == POWER_SUPPLY_PROP_CURRENT_MAX;
> +              psp == POWER_SUPPLY_PROP_CURRENT_MAX ||
> +              psp == POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT;
>  }
>
>  static enum power_supply_property axp20x_usb_power_properties[] = {
> @@ -334,6 +416,15 @@ static enum power_supply_property axp22x_usb_power_properties[] = {
>         POWER_SUPPLY_PROP_CURRENT_MAX,
>  };
>
> +static enum power_supply_property axp813_usb_power_properties[] = {
> +       POWER_SUPPLY_PROP_HEALTH,
> +       POWER_SUPPLY_PROP_PRESENT,
> +       POWER_SUPPLY_PROP_ONLINE,
> +       POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +       POWER_SUPPLY_PROP_CURRENT_MAX,
> +       POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +};
> +
>  static const struct power_supply_desc axp20x_usb_power_desc = {
>         .name = "axp20x-usb",
>         .type = POWER_SUPPLY_TYPE_USB,
> @@ -354,6 +445,16 @@ static const struct power_supply_desc axp22x_usb_power_desc = {
>         .set_property = axp20x_usb_power_set_property,
>  };
>
> +static const struct power_supply_desc axp813_usb_power_desc = {
> +       .name = "axp20x-usb",
> +       .type = POWER_SUPPLY_TYPE_USB,
> +       .properties = axp813_usb_power_properties,
> +       .num_properties = ARRAY_SIZE(axp813_usb_power_properties),
> +       .property_is_writeable = axp20x_usb_power_prop_writeable,
> +       .get_property = axp20x_usb_power_get_property,
> +       .set_property = axp20x_usb_power_set_property,
> +};
> +
>  static const char * const axp20x_irq_names[] = {
>         "VBUS_PLUGIN",
>         "VBUS_REMOVAL",
> @@ -394,6 +495,18 @@ static int axp813_usb_curr_lim_table[] = {
>         2500000,
>  };
>
> +static int axp_813_usb_input_curr_lim_table[] = {
> +       100000,
> +       500000,
> +       900000,
> +       1500000,
> +       2000000,
> +       2500000,
> +       3000000,
> +       3500000,
> +       4000000,
> +};
> +
>  static const struct axp_data axp192_data = {
>         .power_desc     = &axp20x_usb_power_desc,
>         .irq_names      = axp20x_irq_names,
> @@ -433,11 +546,14 @@ static const struct axp_data axp223_data = {
>  };
>
>  static const struct axp_data axp813_data = {
> -       .power_desc     = &axp22x_usb_power_desc,
> +       .power_desc     = &axp813_usb_power_desc,
>         .irq_names      = axp22x_irq_names,
>         .num_irq_names  = ARRAY_SIZE(axp22x_irq_names),
>         .curr_lim_table = axp813_usb_curr_lim_table,
>         .curr_lim_fld   = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 0, 1),
> +       .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),
>         .usb_bc_en_bit  = REG_FIELD(AXP288_BC_GLOBAL, 0, 0),
>         .vbus_disable_bit = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 7, 7),
>         .vbus_needs_polling = true,
> @@ -558,6 +674,7 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, power);
>
> +       power->dev = &pdev->dev;
>         power->axp_data = axp_data;
>         power->regmap = axp20x->regmap;
>         power->num_irqs = axp_data->num_irq_names;
> @@ -567,6 +684,12 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
>         if (IS_ERR(power->curr_lim_fld))
>                 return PTR_ERR(power->curr_lim_fld);
>
> +       ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
> +                                                axp_data->input_curr_lim_fld,
> +                                                &power->input_curr_lim_fld);
> +       if (ret)
> +               return ret;
> +
>         ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
>                                                  axp_data->vbus_valid_bit,
>                                                  &power->vbus_valid_bit);
> --
> 2.43.0
>

Re: Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input current limit

[Aren]

On Sun, Jan 21, 2024 at 11:25:04PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> +CC Hans de Goede since the AXP288 is similar to the later AXPs in the USB
> power section.
> 
> On Sun, Jan 21, 2024 at 9:54 AM Aren Moynihan <aren@peacevolution.org> wrote:
> >
> > Add properties for setting the maximum current that will be drawn from
> > the usb connection.
> >
> > These changes don't apply to all axp20x chips, so we need to add new
> > power_desc and power supply property objects for axp813 specifically.
> > These are copied from the axp22x variants that were used before, with
> > extra fields added.
> >
> > Also add a dev field to the axp20x_usb_power struct, so we can use
> > dev_dbg and dev_err in more places.
> 
> I think this patch highlights some errors in the driver.
> 
> 1. We are likely misusing POWER_SUPPLY_PROP_CURRENT_MAX. Based on the
>    ABI docs, this is supposed to be a read only property. The correct
>    way would be to use POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT as you did.

I think so, yes. I was hesitant to touch that because I wasn't sure if
current_max was relied on anywhere. I'll do some grepping to confirm
that it'll be safe.

> 2. For AXP288 and AXP803/AXP813/AXP818, we are setting the current limit
>    for when BC detection finishes in register 0x30, not the actual active
>    limit in register 0x35. This should be fixed.
> 
> 3. BC detection race condition
> 
> Could you maybe respin the patches to address them separately instead?

Sure, that sounds like a better way to split it up.

 - Aren

> Thanks
> ChenYu

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

[Aren]

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

Re: Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input current limit

[Aren]

On Sun, Jan 21, 2024 at 12:38:54PM +0100, Ondřej Jirman wrote:
> Hello Aren,
> 
> On Sat, Jan 20, 2024 at 08:40:00PM -0500, Aren Moynihan wrote:
> > Add properties for setting the maximum current that will be drawn from
> > the usb connection.
> > 
> > These changes don't apply to all axp20x chips, so we need to add new
> > power_desc and power supply property objects for axp813 specifically.
> > These are copied from the axp22x variants that were used before, with
> > extra fields added.
> > 
> > Also add a dev field to the axp20x_usb_power struct, so we can use
> > dev_dbg and dev_err in more places.
> > 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> > ---
> > 
> >  drivers/power/supply/axp20x_usb_power.c | 127 +++++++++++++++++++++++-
> >  1 file changed, 125 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> > index e23308ad4cc7..8c0c2c25565f 100644
> > --- a/drivers/power/supply/axp20x_usb_power.c
> > +++ b/drivers/power/supply/axp20x_usb_power.c
> > @@ -50,7 +50,10 @@ struct axp_data {
> >  	const char * const		*irq_names;
> >  	unsigned int			num_irq_names;
> >  	const int			*curr_lim_table;
> > +	int				input_curr_lim_table_size;
> > +	const int			*input_curr_lim_table;
> >  	struct reg_field		curr_lim_fld;
> > +	struct reg_field		input_curr_lim_fld;
> >  	struct reg_field		vbus_valid_bit;
> >  	struct reg_field		vbus_mon_bit;
> >  	struct reg_field		usb_bc_en_bit;
> > @@ -59,7 +62,9 @@ struct axp_data {
> >  };
> >  
> >  struct axp20x_usb_power {
> > +	struct device *dev;
> >  	struct regmap *regmap;
> > +	struct regmap_field *input_curr_lim_fld;
> >  	struct regmap_field *curr_lim_fld;
> >  	struct regmap_field *vbus_valid_bit;
> >  	struct regmap_field *vbus_mon_bit;
> > @@ -115,6 +120,15 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
> >  	if (val != power->old_status)
> >  		power_supply_changed(power->supply);
> >  
> > +	if (power->usb_bc_en_bit && (val & AXP20X_PWR_STATUS_VBUS_PRESENT) !=
> > +		(power->old_status & AXP20X_PWR_STATUS_VBUS_PRESENT)) {
> > +		dev_dbg(power->dev, "Cable status changed, re-enabling USB BC");
> > +		ret = regmap_field_write(power->usb_bc_en_bit, 1);
> > +		if (ret)
> > +			dev_err(power->dev, "failed to enable USB BC: errno %d",
> > +				ret);
> > +	}
> > +
> >  	power->old_status = val;
> >  	power->online = val & AXP20X_PWR_STATUS_VBUS_USED;
> >  
> > @@ -123,6 +137,66 @@ static void axp20x_usb_power_poll_vbus(struct work_struct *work)
> >  		mod_delayed_work(system_power_efficient_wq, &power->vbus_detect, DEBOUNCE_TIME);
> >  }
> >  
> > +static int
> > +axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *power,
> > +					 int limit)
> > +{
> > +	int ret;
> > +	unsigned int reg;
> > +
> > +	if (!power->axp_data->input_curr_lim_table)
> > +		return -EINVAL;
> > +
> > +	if (limit < power->axp_data->input_curr_lim_table[0])
> > +		return -EINVAL;
> 
> I think that you should just set the lowest possible limit. A caller (calling
> driver or userspace or user) has no way to identify what is the lowest possible
> limit on arbitrary PMIC and I kinda like having an option to
> echo 0 > .../axp20x-usb/input_current_limit as a way to limit power consumption from
> USB to a minimum without having to look up what actual minimum is in various
> PMICs dataheets.
> 
> I looked through most of the uses of POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT in
> the upstream drivers and both approaches are used. Erroring out when out of range
> is less common.

That should be pretty easy, I love a good reason to delete code :)

Thanks
 - Aren

> Otherwise,
> 
> Reviewed-By: Ondrej Jirman <megi@xff.cz>
> 
> Thank you,
> 	Ondrej

Re: [PATCH 2/3] power: supply: axp20x_usb_power: enable usb_type reporting

[Aren]

On Sun, Jan 21, 2024 at 12:55:43PM +0100, Ondřej Jirman wrote:
> Hello Aren,
> 
> On Sat, Jan 20, 2024 at 08:40:02PM -0500, Aren Moynihan wrote:
> > Axp803 and axp813 can report the detected usb bc mode. SDP, CDP, and DCP
> > are supported.
> > 
> > Signed-off-by: Aren Moynihan <aren@peacevolution.org>
> > ---
> > 
> >  drivers/power/supply/axp20x_usb_power.c | 52 +++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> > 
> > diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
> > index 8c0c2c25565f..5656f6e57d54 100644
> > --- a/drivers/power/supply/axp20x_usb_power.c
> > +++ b/drivers/power/supply/axp20x_usb_power.c
> > @@ -57,6 +57,7 @@ struct axp_data {
> >  	struct reg_field		vbus_valid_bit;
> >  	struct reg_field		vbus_mon_bit;
> >  	struct reg_field		usb_bc_en_bit;
> > +	struct reg_field		usb_bc_det_fld;
> >  	struct reg_field		vbus_disable_bit;
> >  	bool				vbus_needs_polling: 1;
> >  };
> > @@ -69,6 +70,7 @@ struct axp20x_usb_power {
> >  	struct regmap_field *vbus_valid_bit;
> >  	struct regmap_field *vbus_mon_bit;
> >  	struct regmap_field *usb_bc_en_bit;
> > +	struct regmap_field *usb_bc_det_fld;
> >  	struct regmap_field *vbus_disable_bit;
> >  	struct power_supply *supply;
> >  	const struct axp_data *axp_data;
> > @@ -197,6 +199,37 @@ axp20x_usb_power_get_input_current_limit(struct axp20x_usb_power *power,
> >  	return 0;
> >  }
> >  
> > +static int axp20x_get_usb_type(struct axp20x_usb_power *power,
> > +			       union power_supply_propval *val)
> > +{
> > +	unsigned int reg;
> > +	int ret;
> > +
> > +	if (!power->usb_bc_det_fld)
> > +		return -EINVAL;
> 
> What if detection is disabled becase custom input current limit was set by the
> user? Does the HW keep the result of previous detection in the register?

In that case the result appears to be USB_TYPE_UNKNOWN. I don't expect
that would cause problems, but I guess we could cache the value when
disabling usb bc detection if necessary.

> Kind regards,
> 	Ondrej
> 
> > +	ret = regmap_field_read(power->usb_bc_det_fld, &reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (reg) {
> > +	case 1:
> > +		val->intval = POWER_SUPPLY_USB_TYPE_SDP;
> > +		break;
> > +	case 2:
> > +		val->intval = POWER_SUPPLY_USB_TYPE_CDP;
> > +		break;
> > +	case 3:
> > +		val->intval = POWER_SUPPLY_USB_TYPE_DCP;
> > +		break;
> > +	default:
> > +		val->intval = POWER_SUPPLY_USB_TYPE_UNKNOWN;
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int axp20x_usb_power_get_property(struct power_supply *psy,
> >  	enum power_supply_property psp, union power_supply_propval *val)
> >  {
> > @@ -266,6 +299,8 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
> >  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> >  		return axp20x_usb_power_get_input_current_limit(power,
> >  								&val->intval);
> > +	case POWER_SUPPLY_PROP_USB_TYPE:
> > +		return axp20x_get_usb_type(power, val);
> >  	default:
> >  		break;
> >  	}
> > @@ -423,6 +458,14 @@ static enum power_supply_property axp813_usb_power_properties[] = {
> >  	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> >  	POWER_SUPPLY_PROP_CURRENT_MAX,
> >  	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> > +	POWER_SUPPLY_PROP_USB_TYPE,
> > +};
> > +
> > +static enum power_supply_usb_type axp813_usb_types[] = {
> > +	POWER_SUPPLY_USB_TYPE_SDP,
> > +	POWER_SUPPLY_USB_TYPE_DCP,
> > +	POWER_SUPPLY_USB_TYPE_CDP,
> > +	POWER_SUPPLY_USB_TYPE_UNKNOWN,
> >  };
> >  
> >  static const struct power_supply_desc axp20x_usb_power_desc = {
> > @@ -453,6 +496,8 @@ static const struct power_supply_desc axp813_usb_power_desc = {
> >  	.property_is_writeable = axp20x_usb_power_prop_writeable,
> >  	.get_property = axp20x_usb_power_get_property,
> >  	.set_property = axp20x_usb_power_set_property,
> > +	.usb_types = axp813_usb_types,
> > +	.num_usb_types = ARRAY_SIZE(axp813_usb_types),
> >  };
> >  
> >  static const char * const axp20x_irq_names[] = {
> > @@ -555,6 +600,7 @@ static const struct axp_data axp813_data = {
> >  	.input_curr_lim_table = axp_813_usb_input_curr_lim_table,
> >  	.input_curr_lim_fld = REG_FIELD(AXP22X_CHRG_CTRL3, 4, 7),
> >  	.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),
> >  	.vbus_needs_polling = true,
> >  };
> > @@ -708,6 +754,12 @@ static int axp20x_usb_power_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
> > +						 axp_data->usb_bc_det_fld,
> > +						 &power->usb_bc_det_fld);
> > +	if (ret)
> > +		return ret;
> > +
> >  	ret = axp20x_regmap_field_alloc_optional(&pdev->dev, power->regmap,
> >  						 axp_data->vbus_disable_bit,
> >  						 &power->vbus_disable_bit);
> > -- 
> > 2.43.0
> >