Proper handling of 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 new logic to get/set the input current limit, and get the usb type
is originally based on code Ondřej Jirman wrote[1], but I have
significantly refactored it.
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.
v1 of this patch can be found at:
https://lore.kernel.org/lkml/[email protected]/
1: https://xff.cz/git/linux/commit/?h=axp-6.7&id=3dcd33dfd1ae58db159427365dcb6d0d2b12f06d
Changes in v2:
- Values less than the lowest supported limit are rounded up instead
of returning -EINVAL when setting the input current limit.
- Rename existing current_max logic to input_current_limit. This also
makes it possible to reuse some of that logic.
- Split current limit register and bc race condition fixes into
different commits.
Aren Moynihan (5):
power: supply: axp20x_usb_power: replace current_max with
input_current_limit
power: supply: axp20x_usb_power: use correct register for input
current limit
power: supply: axp20x_usb_power: fix race condition with usb bc
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 | 160 +++++++++++++++++++++---
1 file changed, 144 insertions(+), 16 deletions(-)
--
2.43.0
The current_max property is supposed to be read-only, and represent the
maximum current the supply can provide. input_current_limit is the
limit that is currently set, which is what we have here.
When determining what value to write to the register, we need to pick a
reasonable value if the requested limit doesn't exactly match one
supported by the hardware. If the requested limit is less than the
lowest value we can set, round up to the lowest value. Otherwise round
down to the nearest value supported by hardware.
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 <[email protected]>
---
Changes in v2:
- Values less than the lowest supported limit are rounded up instead
of returning -EINVAL when setting the input current limit.
drivers/power/supply/axp20x_usb_power.c | 29 +++++++++++++++----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index e23308ad4cc7..f7f2ac2b7dae 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -59,6 +59,7 @@ struct axp_data {
};
struct axp20x_usb_power {
+ struct device *dev;
struct regmap *regmap;
struct regmap_field *curr_lim_fld;
struct regmap_field *vbus_valid_bit;
@@ -160,7 +161,7 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
val->intval = ret * 1700; /* 1 step = 1.7 mV */
return 0;
- case POWER_SUPPLY_PROP_CURRENT_MAX:
+ case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
ret = regmap_field_read(power->curr_lim_fld, &v);
if (ret)
return ret;
@@ -256,19 +257,24 @@ static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
return -EINVAL;
}
-static int axp20x_usb_power_set_current_max(struct axp20x_usb_power *power, int intval)
+static int axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *power,
+ int intval)
{
+ unsigned int reg;
const unsigned int max = GENMASK(power->axp_data->curr_lim_fld.msb,
power->axp_data->curr_lim_fld.lsb);
if (intval == -1)
return -EINVAL;
- for (unsigned int i = 0; i <= max; ++i)
- if (power->axp_data->curr_lim_table[i] == intval)
- return regmap_field_write(power->curr_lim_fld, i);
+ for (reg = max - 1; reg > 0; reg--)
+ if (power->axp_data->curr_lim_table[reg] <= intval)
+ break;
- return -EINVAL;
+ dev_dbg(power->dev, "setting input current limit reg to %d (%d uA), requested %d uA",
+ reg, power->axp_data->curr_lim_table[reg], intval);
+
+ return regmap_field_write(power->curr_lim_fld, reg);
}
static int axp20x_usb_power_set_property(struct power_supply *psy,
@@ -287,8 +293,8 @@ static int axp20x_usb_power_set_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_VOLTAGE_MIN:
return axp20x_usb_power_set_voltage_min(power, val->intval);
- 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 +319,7 @@ 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_INPUT_CURRENT_LIMIT;
}
static enum power_supply_property axp20x_usb_power_properties[] = {
@@ -322,7 +328,7 @@ static enum power_supply_property axp20x_usb_power_properties[] = {
POWER_SUPPLY_PROP_ONLINE,
POWER_SUPPLY_PROP_VOLTAGE_MIN,
POWER_SUPPLY_PROP_VOLTAGE_NOW,
- POWER_SUPPLY_PROP_CURRENT_MAX,
+ POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
POWER_SUPPLY_PROP_CURRENT_NOW,
};
@@ -331,7 +337,7 @@ static enum power_supply_property axp22x_usb_power_properties[] = {
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 = {
@@ -558,6 +564,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;
--
2.43.0
On the axp803 and axp813 chips register 0x30 bits 0-1 is the default
current limit that gets applied after the pmic detects a CDP or DCP
port. The correct field to set is 0x35 bits 4-7.
This field only has nine values (out of the 16 possible if it used all
the bits), so introduce a field size variable to take that into account.
Signed-off-by: Aren Moynihan <[email protected]>
---
Changes in v2:
- Inline get input current logic. It's not that complicated and this
helps to illustrate what changed more clearly.
- Split into separate commit, it was part of adding the input current
limit before.
drivers/power/supply/axp20x_usb_power.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index f7f2ac2b7dae..923121b23d5f 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -50,6 +50,7 @@ struct axp_data {
const char * const *irq_names;
unsigned int num_irq_names;
const int *curr_lim_table;
+ int curr_lim_table_size;
struct reg_field curr_lim_fld;
struct reg_field vbus_valid_bit;
struct reg_field vbus_mon_bit;
@@ -166,7 +167,11 @@ static int axp20x_usb_power_get_property(struct power_supply *psy,
if (ret)
return ret;
- val->intval = power->axp_data->curr_lim_table[v];
+ if (v < power->axp_data->curr_lim_table_size)
+ val->intval = power->axp_data->curr_lim_table[v];
+ else
+ val->intval = power->axp_data->curr_lim_table[
+ power->axp_data->curr_lim_table_size - 1];
return 0;
case POWER_SUPPLY_PROP_CURRENT_NOW:
if (IS_ENABLED(CONFIG_AXP20X_ADC)) {
@@ -261,8 +266,7 @@ static int axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *pow
int intval)
{
unsigned int reg;
- const unsigned int max = GENMASK(power->axp_data->curr_lim_fld.msb,
- power->axp_data->curr_lim_fld.lsb);
+ const unsigned int max = power->axp_data->curr_lim_table_size;
if (intval == -1)
return -EINVAL;
@@ -394,10 +398,15 @@ static int axp221_usb_curr_lim_table[] = {
};
static int axp813_usb_curr_lim_table[] = {
+ 100000,
+ 500000,
900000,
1500000,
2000000,
2500000,
+ 3000000,
+ 3500000,
+ 4000000,
};
static const struct axp_data axp192_data = {
@@ -405,6 +414,7 @@ static const struct axp_data axp192_data = {
.irq_names = axp20x_irq_names,
.num_irq_names = ARRAY_SIZE(axp20x_irq_names),
.curr_lim_table = axp192_usb_curr_lim_table,
+ .curr_lim_table_size = ARRAY_SIZE(axp192_usb_curr_lim_table),
.curr_lim_fld = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 0, 1),
.vbus_valid_bit = REG_FIELD(AXP192_USB_OTG_STATUS, 2, 2),
.vbus_mon_bit = REG_FIELD(AXP20X_VBUS_MON, 3, 3),
@@ -415,6 +425,7 @@ static const struct axp_data axp202_data = {
.irq_names = axp20x_irq_names,
.num_irq_names = ARRAY_SIZE(axp20x_irq_names),
.curr_lim_table = axp20x_usb_curr_lim_table,
+ .curr_lim_table_size = ARRAY_SIZE(axp20x_usb_curr_lim_table),
.curr_lim_fld = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 0, 1),
.vbus_valid_bit = REG_FIELD(AXP20X_USB_OTG_STATUS, 2, 2),
.vbus_mon_bit = REG_FIELD(AXP20X_VBUS_MON, 3, 3),
@@ -425,6 +436,7 @@ static const struct axp_data axp221_data = {
.irq_names = axp22x_irq_names,
.num_irq_names = ARRAY_SIZE(axp22x_irq_names),
.curr_lim_table = axp221_usb_curr_lim_table,
+ .curr_lim_table_size = ARRAY_SIZE(axp221_usb_curr_lim_table),
.curr_lim_fld = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 0, 1),
.vbus_needs_polling = true,
};
@@ -434,6 +446,7 @@ static const struct axp_data axp223_data = {
.irq_names = axp22x_irq_names,
.num_irq_names = ARRAY_SIZE(axp22x_irq_names),
.curr_lim_table = axp20x_usb_curr_lim_table,
+ .curr_lim_table_size = ARRAY_SIZE(axp20x_usb_curr_lim_table),
.curr_lim_fld = REG_FIELD(AXP20X_VBUS_IPSOUT_MGMT, 0, 1),
.vbus_needs_polling = true,
};
@@ -443,7 +456,8 @@ static const struct axp_data axp813_data = {
.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),
+ .curr_lim_table_size = ARRAY_SIZE(axp813_usb_curr_lim_table),
+ .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,
--
2.43.0
When input_current_limit is set while USB BC is in progress, the BC
module will overwrite the value that was set when it finishes detection.
Signed-off-by: Aren Moynihan <[email protected]>
---
Changes in v2:
- Split into sepereate commit, it was part of addig the input current
limit before.
drivers/power/supply/axp20x_usb_power.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index 923121b23d5f..ac5a3f126df6 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -117,6 +117,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;
@@ -265,12 +274,26 @@ static int axp20x_usb_power_set_voltage_min(struct axp20x_usb_power *power,
static int axp20x_usb_power_set_input_current_limit(struct axp20x_usb_power *power,
int intval)
{
+ int ret;
unsigned int reg;
const unsigned int max = power->axp_data->curr_lim_table_size;
if (intval == -1)
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 = max - 1; reg > 0; reg--)
if (power->axp_data->curr_lim_table[reg] <= intval)
break;
--
2.43.0
The axp803 and axp813 chips can report the detected USB BC mode. SDP,
CDP, and DCP are supported.
Signed-off-by: Aren Moynihan <[email protected]>
---
(no changes since v1)
drivers/power/supply/axp20x_usb_power.c | 73 ++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 1 deletion(-)
diff --git a/drivers/power/supply/axp20x_usb_power.c b/drivers/power/supply/axp20x_usb_power.c
index ac5a3f126df6..dae7e5cfc54e 100644
--- a/drivers/power/supply/axp20x_usb_power.c
+++ b/drivers/power/supply/axp20x_usb_power.c
@@ -55,6 +55,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;
};
@@ -66,6 +67,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;
@@ -134,6 +136,37 @@ 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_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, ®);
+ 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)
{
@@ -204,6 +237,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_USB_TYPE:
+ return axp20x_get_usb_type(power, val);
default:
break;
}
@@ -367,6 +403,22 @@ static enum power_supply_property axp22x_usb_power_properties[] = {
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
};
+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_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 = {
.name = "axp20x-usb",
.type = POWER_SUPPLY_TYPE_USB,
@@ -387,6 +439,18 @@ 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,
+ .usb_types = axp813_usb_types,
+ .num_usb_types = ARRAY_SIZE(axp813_usb_types),
+};
+
static const char * const axp20x_irq_names[] = {
"VBUS_PLUGIN",
"VBUS_REMOVAL",
@@ -475,13 +539,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_table_size = ARRAY_SIZE(axp813_usb_curr_lim_table),
.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,
};
@@ -629,6 +694,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
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")
Signed-off-by: Aren Moynihan <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
On Tue, Jan 30, 2024 at 10:13:06PM +0100, Ondřej Jirman wrote:
> 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).
Oh you're right, I overlooked the case where the battery is very low, in
which case bc detection should still be performed (I think, I haven't
tested it). This issue this patch is trying to fix doesn't apply in that
case, so it should be simple enough to check if the pmic has detected a
battery and skip setting the current limit if it has.
> 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.
Perhaps it would be better to read the limit from the device tree, that
way it could be set higher for a specific board if it needs to draw that
much current and be able to boot without a battery? It seems sketchy to
default to a current limit significantly higher than what the usb power
supply is required to support.
> 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.
Adding a dev_warn is a good idea, I'll do that.
Thanks for the review
- Aren
> As for me, this patch should not be applied at all.
>
> Kind regards,
> o.
>
> > Signed-off-by: Aren Moynihan <[email protected]>
> > ---
> > 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
> >
On Tue, Jan 30, 2024 at 11:20:29PM -0500, Aren wrote:
> On Tue, Jan 30, 2024 at 10:13:06PM +0100, Ondřej Jirman wrote:
> > On Tue, Jan 30, 2024 at 03:28:01PM -0500, Aren Moynihan 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).
>
> Oh you're right, I overlooked the case where the battery is very low, in
> which case bc detection should still be performed (I think, I haven't
> tested it). This issue this patch is trying to fix doesn't apply in that
> case, so it should be simple enough to check if the pmic has detected a
> battery and skip setting the current limit if it has.
>
> > 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.
>
> Perhaps it would be better to read the limit from the device tree, that
> way it could be set higher for a specific board if it needs to draw that
> much current and be able to boot without a battery? It seems sketchy to
> default to a current limit significantly higher than what the usb power
> supply is required to support.
But is there really an issue? The board may not be using USB power supply.
It may simply have a barrel jack, like Quartz64-A does. And it will still
create an issue if you make the new behavior "opt-out" via DT. You can make
it opt-in if you like.
Also in Pinephone case, you'll not really have a case where the battery has
< 2V not loaded. That's not going to happen. PMIC will shut off at 3V battery
voltage when loaded. It will not discharge further, and after shutoff battery
voltage will jump to 3.4V or so, and it will not drop below 2V after that, ever.
So the battery will pretty much always be detected as long as it's present.
What actual problem have you seen that this patch is trying to solve?
Thank you and kind regards,
o.
On Sun, Feb 11, 2024 at 12:27:12AM +0100, Ondřej Jirman wrote:
> On Tue, Jan 30, 2024 at 11:20:29PM -0500, Aren wrote:
> > On Tue, Jan 30, 2024 at 10:13:06PM +0100, Ondřej Jirman wrote:
> > > On Tue, Jan 30, 2024 at 03:28:01PM -0500, Aren Moynihan 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).
> >
> > Oh you're right, I overlooked the case where the battery is very low, in
> > which case bc detection should still be performed (I think, I haven't
> > tested it). This issue this patch is trying to fix doesn't apply in that
> > case, so it should be simple enough to check if the pmic has detected a
> > battery and skip setting the current limit if it has.
> >
> > > 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.
> >
> > Perhaps it would be better to read the limit from the device tree, that
> > way it could be set higher for a specific board if it needs to draw that
> > much current and be able to boot without a battery? It seems sketchy to
> > default to a current limit significantly higher than what the usb power
> > supply is required to support.
>
> But is there really an issue? The board may not be using USB power supply.
> It may simply have a barrel jack, like Quartz64-A does. And it will still
> create an issue if you make the new behavior "opt-out" via DT. You can make
> it opt-in if you like.
The axp20x_ac_power driver is used for the barrel jack, so this patch
shouldn't change how those devices behave. There are only a few boards
that are expected to boot/operate with only usb power, which should be
the only ones that need special attention.
> Also in Pinephone case, you'll not really have a case where the battery has
> < 2V not loaded. That's not going to happen. PMIC will shut off at 3V battery
> voltage when loaded. It will not discharge further, and after shutoff battery
> voltage will jump to 3.4V or so, and it will not drop below 2V after that, ever.
> So the battery will pretty much always be detected as long as it's present.
The most likely case I can think of is if someone intentionally tries to
boot the device without the battery. I suspect it's also possible for a
battery to degrade to the point where it won't hold a charge.
> What actual problem have you seen that this patch is trying to solve?
The problem, in theory, is that the pmic ignores the USB BC
specification and sets the current limit to 3A instead of 500mA. In
practice (as long as the power supply is implemented properly) if this
is too much power, it should just cause the power supply to shut off.
I'm not sure how likely / what the risks of a power supply cutting
corners are.
I find it surprising that the hardware/driver takes a lot of care to
figure out what the proper current is and stick to that, except when
there isn't a battery.
The point of this patch (after a revision) should be to make it explicit
when and why this driver ignores the USB BC specification. And to reduce
the cases where it does, if possible.
With the goal of making it explicit what cases ignore the spec, I would
prefer to have an opt-out mechanism. I compiled what I believe to be a
full list of devices that use this driver with usb bc enabled (detailed
notes below), and there's only a handful of them. It shouldn't be too
difficult to out-out the boards that need it.
>
> Thank you and kind regards,
> o.
Sorry it took me a while to respond, I haven't had much time to work on
this in the past few weeks.
Regards
- Aren
p.s. the notes on what devices use this functionality:
These devices include the axp803 or axp81x dtsi:
$ rg -l 'include "axp(803|81x).dtsi"'
- sun50i-a100-allwinner-perf1.dts
- sun50i-a64-amarula-relic.dts
- sun50i-a64-bananapi-m64.dts
- sun50i-a64-nanopi-a64.dts
- sun50i-a64-olinuxino.dts
- sun50i-a64-orangepi-win.dts
- sun50i-a64-pine64.dts
- sun50i-a64-pinebook.dts
- sun50i-a64-pinephone.dtsi
- sun50i-a64-pinetab.dts
- sun50i-a64-sopine.dtsi
- sun50i-a64-teres-i.dts
- sun8i-a83t-allwinner-h8homlet-v2.dts
- sun8i-a83t-bananapi-m3.dts
- sun8i-a83t-cubietruck-plus.dts
- sun8i-a83t-tbs-a711.dts
Out of those only these enable usb_power_supply:
$ rg -l 'include "axp(803|81x).dtsi"' | xargs rg -l 'usb_power_supply'
- sun50i-a64-bananapi-m64.dts
- sun50i-a64-pinetab.dts
- sun50i-a64-pinephone.dtsi
- sun8i-a83t-tbs-a711.dts
- sun8i-a83t-cubietruck-plus.dts
- sun8i-a83t-bananapi-m3.dts
sun50i-a64-bananapi-m64.dts: The barrel jack is connected to acin, so
will be unaffected. Banannapi docs say it's not possible to power over
usb, but schematic suggests it should work. Probably needs to opt-out of
the lower current limit.
sun50i-a64-pinetab.dts: unclear if charging is supported via usb, vbus
is connected through a component listed as "NC/0R". Regardless device
has barrel jack and battery for power, shouldn't need to run exclusively
from usb.
sun50i-a64-pinephone.dtsi: is typically booted with a battery connected,
shouldn't need to run exclusively from usb.
sun8i-a83t-tbs-a711.dts: has an internal battery, shouldn't need to run
exclusively from usb.
sun8i-a83t-cubietruck-plus.dts and sun8i-a83t-bananapi-m3.dts: Both
appear to support being powered over usb and a barrel jack. These will
need to opt-out to be able to run from usb.
Hi Aren,
On Thu, Mar 14, 2024 at 06:39:52PM -0400, Aren wrote:
> > Also in Pinephone case, you'll not really have a case where the battery has
> > < 2V not loaded. That's not going to happen. PMIC will shut off at 3V battery
> > voltage when loaded. It will not discharge further, and after shutoff battery
> > voltage will jump to 3.4V or so, and it will not drop below 2V after that, ever.
> > So the battery will pretty much always be detected as long as it's present.
>
> The most likely case I can think of is if someone intentionally tries to
> boot the device without the battery. I suspect it's also possible for a
> battery to degrade to the point where it won't hold a charge.
Yes, that's my usecase that I'd like to preserve. Pinephone has removable
battery and using it without battery is quite reasonable. It works fine
currently for me and this patch will break this if there's no opt out. And
there's no opt out other than patching and re-building the kernel.
> > What actual problem have you seen that this patch is trying to solve?
>
> The problem, in theory, is that the pmic ignores the USB BC
> specification and sets the current limit to 3A instead of 500mA. In
> practice (as long as the power supply is implemented properly) if this
> is too much power, it should just cause the power supply to shut off.
> I'm not sure how likely / what the risks of a power supply cutting
> corners are.
Pinephone with no battery takes between 0.5-1A from VBUS. Even under full
load, it's not enough to damage even a USB 3.0 SDP port. It's only a
slight problem for unprotected USB 2.0 SDP ports. On protected 2.0 ports
or ports with overdesigned output power, it will either shut down due
to brownout, or just work.
It's not enough to overload any actual USB charger.
In any case, people wanting to run Pinephone without a battery probably
will not do so from a USB 2.0 computer port. Maybe for FEL USB mode, for
development or flashing, but at that stage the power consumption is still
very low, well below 2.5W.
> I find it surprising that the hardware/driver takes a lot of care to
> figure out what the proper current is and stick to that, except when
> there isn't a battery.
It seems to have apparent purpose documented in the datasheet:
https://megous.com/dl/tmp/78d4c0771fc6d2c8.png
"If Battery not present, and this bit is 0,the VBUS current limit set to 3A,
for the F/W update in factory"
You can also get rid of the issue by writing 1 to:
"REG 2DH: BC Module VBUS Control and Status Register" bit 6
in the bootloader very early on before enabling BC. That should fix the issue,
too in a more proper way than forcing 500mA limit halfway during boot, when
BC1.2 detection might have detected something higher earlier on and the boot
success depends on the higher value.
> 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.
Actually it does (I made a quick test with Pinephone with no battery being
plugged to the PC's USB port and executing a test program over FEL that talks to
the PMIC):
PMIC registers: (initial values post-powerup with no battery)
2c: 0 - BC disabled by default, something has to enable it
2d: b0 - bit 6 not set (*not* first boot bit)
2e: 40
2f: 0
30: 1
31: 3
32: 43
33: c5
34: 45
35: 68 - initially 3A limit
36: 59
37: 0
38: a5
39: 1f
3a: 80
changed values
2c: 5 - test program enables BC
2e: 0 - disable DB detection (otherwise with no battery DB detection will
prolong BC detection result by 45minutes or whatever is the timeout)
see DBP_Timeout_CTL(DBP Hardware Timeout Control)
2f: 10
30: 2a
35: 38 - test program sets 1.5A limit
36: 8
.. about 400 ms later
2c: 1 - BC complete
2f: 30 - BC result = SDP (matches reality)
35: 68 - 3A VBUS limit set by PMIC itself
Another run with 2d.6 bit set:
(initial values omitted, same as above)
changed values
2c: 5 - enable BC
2d: f0 - bit 6 set (*not* first boot)
2e: 0
2f: 10
30: 2a
35: 38 - test program sets 1.5A limit
36: 8
.. after about 400 ms
2c: 1 - BC complete
2f: 30 - BC result = SDP
35: 18 - 500mA VBUS limit set by PMIC itself
So the detection works with no battery inserted. PMIC's BC correcly detects
regular USB 2.0 data port and configures a correct limit in about 400ms
after cable plug in. So I don't see a problem with the HW, that you're
describing in the commit message.
The proper way to handle this issue is to fix whichever component is configuring
the BC detection initially (it's disabled by default, apparently), to properly
set the first boot bit before enabling BC. IMO, that place should be the
platform firmware. Then the detection will work from the get go and proper limit
will be always set correctly and will match the USB charger, and there will be
no need for any kernel hacks.
Pretty much what platform FW should do is to:
- set "not first boot bit" in 0x2d register
- check if battery is present
- if not clear 0x2e register
- otherwise configure DBD in 0x2e
- configure DCP/CDP current limit to 1.5A or 2A (1.5A maybe safer)
- configure VBUS Vhold to 4.5V (Pinephone needs this for powered USB dock to
work with in general with arbitrary chargers, and it will overload weaker
USB PSU's less)
- configure BC detection and start it
My usecase of using Pinephone without battery will still work, too, and will
not be broken by this patch.
> The point of this patch (after a revision) should be to make it explicit
> when and why this driver ignores the USB BC specification. And to reduce
> the cases where it does, if possible.
The kernel has no business forcing the limit to some fixed low value that
has no relationship to the last BC detection result and breaks boot in
the exact scenario this patch is targetting (no battery -> too high current
limit).
This doesn't make any sense to me.
kind regards,
o.
> With the goal of making it explicit what cases ignore the spec, I would
> prefer to have an opt-out mechanism. I compiled what I believe to be a
> full list of devices that use this driver with usb bc enabled (detailed
> notes below), and there's only a handful of them. It shouldn't be too
> difficult to out-out the boards that need it.
>
> >
> > Thank you and kind regards,
> > o.
>
> Sorry it took me a while to respond, I haven't had much time to work on
> this in the past few weeks.
>
> Regards
> - Aren
>
> p.s. the notes on what devices use this functionality:
>
> These devices include the axp803 or axp81x dtsi:
> $ rg -l 'include "axp(803|81x).dtsi"'
> - sun50i-a100-allwinner-perf1.dts
> - sun50i-a64-amarula-relic.dts
> - sun50i-a64-bananapi-m64.dts
> - sun50i-a64-nanopi-a64.dts
> - sun50i-a64-olinuxino.dts
> - sun50i-a64-orangepi-win.dts
> - sun50i-a64-pine64.dts
> - sun50i-a64-pinebook.dts
> - sun50i-a64-pinephone.dtsi
> - sun50i-a64-pinetab.dts
> - sun50i-a64-sopine.dtsi
> - sun50i-a64-teres-i.dts
> - sun8i-a83t-allwinner-h8homlet-v2.dts
> - sun8i-a83t-bananapi-m3.dts
> - sun8i-a83t-cubietruck-plus.dts
> - sun8i-a83t-tbs-a711.dts
>
> Out of those only these enable usb_power_supply:
> $ rg -l 'include "axp(803|81x).dtsi"' | xargs rg -l 'usb_power_supply'
> - sun50i-a64-bananapi-m64.dts
> - sun50i-a64-pinetab.dts
> - sun50i-a64-pinephone.dtsi
> - sun8i-a83t-tbs-a711.dts
> - sun8i-a83t-cubietruck-plus.dts
> - sun8i-a83t-bananapi-m3.dts
>
> sun50i-a64-bananapi-m64.dts: The barrel jack is connected to acin, so
> will be unaffected. Banannapi docs say it's not possible to power over
> usb, but schematic suggests it should work. Probably needs to opt-out of
> the lower current limit.
>
> sun50i-a64-pinetab.dts: unclear if charging is supported via usb, vbus
> is connected through a component listed as "NC/0R". Regardless device
> has barrel jack and battery for power, shouldn't need to run exclusively
> from usb.
>
> sun50i-a64-pinephone.dtsi: is typically booted with a battery connected,
> shouldn't need to run exclusively from usb.
>
> sun8i-a83t-tbs-a711.dts: has an internal battery, shouldn't need to run
> exclusively from usb.
>
> sun8i-a83t-cubietruck-plus.dts and sun8i-a83t-bananapi-m3.dts: Both
> appear to support being powered over usb and a barrel jack. These will
> need to opt-out to be able to run from usb.
Hi Ondřej,
On Wed, Mar 20, 2024 at 01:12:31AM +0100, Ondřej Jirman wrote:
> Hi Aren,
>
> On Thu, Mar 14, 2024 at 06:39:52PM -0400, Aren wrote:
> > > Also in Pinephone case, you'll not really have a case where the battery has
> > > < 2V not loaded. That's not going to happen. PMIC will shut off at 3V battery
> > > voltage when loaded. It will not discharge further, and after shutoff battery
> > > voltage will jump to 3.4V or so, and it will not drop below 2V after that, ever.
> > > So the battery will pretty much always be detected as long as it's present.
> >
> > The most likely case I can think of is if someone intentionally tries to
> > boot the device without the battery. I suspect it's also possible for a
> > battery to degrade to the point where it won't hold a charge.
>
> Yes, that's my usecase that I'd like to preserve. Pinephone has removable
> battery and using it without battery is quite reasonable. It works fine
> currently for me and this patch will break this if there's no opt out. And
> there's no opt out other than patching and re-building the kernel.
>
> > > What actual problem have you seen that this patch is trying to solve?
> >
> > The problem, in theory, is that the pmic ignores the USB BC
> > specification and sets the current limit to 3A instead of 500mA. In
> > practice (as long as the power supply is implemented properly) if this
> > is too much power, it should just cause the power supply to shut off.
> > I'm not sure how likely / what the risks of a power supply cutting
> > corners are.
>
> Pinephone with no battery takes between 0.5-1A from VBUS. Even under full
> load, it's not enough to damage even a USB 3.0 SDP port. It's only a
> slight problem for unprotected USB 2.0 SDP ports. On protected 2.0 ports
> or ports with overdesigned output power, it will either shut down due
> to brownout, or just work.
>
> It's not enough to overload any actual USB charger.
>
> In any case, people wanting to run Pinephone without a battery probably
> will not do so from a USB 2.0 computer port. Maybe for FEL USB mode, for
> development or flashing, but at that stage the power consumption is still
> very low, well below 2.5W.
>
> > I find it surprising that the hardware/driver takes a lot of care to
> > figure out what the proper current is and stick to that, except when
> > there isn't a battery.
>
> It seems to have apparent purpose documented in the datasheet:
>
> https://megous.com/dl/tmp/78d4c0771fc6d2c8.png
>
> "If Battery not present, and this bit is 0,the VBUS current limit set to 3A,
> for the F/W update in factory"
>
> You can also get rid of the issue by writing 1 to:
>
> "REG 2DH: BC Module VBUS Control and Status Register" bit 6
>
> in the bootloader very early on before enabling BC. That should fix the issue,
> too in a more proper way than forcing 500mA limit halfway during boot, when
> BC1.2 detection might have detected something higher earlier on and the boot
> success depends on the higher value.
I agree this is a better way of handling this, I made an attempt
previously to get USB BC working without a battery connected, but I was
has having problems with USB BC hanging. Based on what you describe
below, I now suspect that had to do with leaving dead battery detection
enabled (0x2E bit 6). I'll take a look through the code again to see if
I can get it working.
Thanks
- Aren
> > 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.
>
> Actually it does (I made a quick test with Pinephone with no battery being
> plugged to the PC's USB port and executing a test program over FEL that talks to
> the PMIC):
>
> PMIC registers: (initial values post-powerup with no battery)
>
> 2c: 0 - BC disabled by default, something has to enable it
> 2d: b0 - bit 6 not set (*not* first boot bit)
> 2e: 40
> 2f: 0
> 30: 1
> 31: 3
> 32: 43
> 33: c5
> 34: 45
> 35: 68 - initially 3A limit
> 36: 59
> 37: 0
> 38: a5
> 39: 1f
> 3a: 80
>
> changed values
>
> 2c: 5 - test program enables BC
> 2e: 0 - disable DB detection (otherwise with no battery DB detection will
> prolong BC detection result by 45minutes or whatever is the timeout)
> see DBP_Timeout_CTL(DBP Hardware Timeout Control)
> 2f: 10
> 30: 2a
> 35: 38 - test program sets 1.5A limit
> 36: 8
>
> ... about 400 ms later
>
> 2c: 1 - BC complete
> 2f: 30 - BC result = SDP (matches reality)
> 35: 68 - 3A VBUS limit set by PMIC itself
>
>
> Another run with 2d.6 bit set:
>
> (initial values omitted, same as above)
>
>
> changed values
>
> 2c: 5 - enable BC
> 2d: f0 - bit 6 set (*not* first boot)
> 2e: 0
> 2f: 10
> 30: 2a
> 35: 38 - test program sets 1.5A limit
> 36: 8
>
> ... after about 400 ms
>
> 2c: 1 - BC complete
> 2f: 30 - BC result = SDP
> 35: 18 - 500mA VBUS limit set by PMIC itself
>
>
> So the detection works with no battery inserted. PMIC's BC correcly detects
> regular USB 2.0 data port and configures a correct limit in about 400ms
> after cable plug in. So I don't see a problem with the HW, that you're
> describing in the commit message.
>
> The proper way to handle this issue is to fix whichever component is configuring
> the BC detection initially (it's disabled by default, apparently), to properly
> set the first boot bit before enabling BC. IMO, that place should be the
> platform firmware. Then the detection will work from the get go and proper limit
> will be always set correctly and will match the USB charger, and there will be
> no need for any kernel hacks.
>
> Pretty much what platform FW should do is to:
>
> - set "not first boot bit" in 0x2d register
> - check if battery is present
> - if not clear 0x2e register
> - otherwise configure DBD in 0x2e
> - configure DCP/CDP current limit to 1.5A or 2A (1.5A maybe safer)
> - configure VBUS Vhold to 4.5V (Pinephone needs this for powered USB dock to
> work with in general with arbitrary chargers, and it will overload weaker
> USB PSU's less)
> - configure BC detection and start it
>
> My usecase of using Pinephone without battery will still work, too, and will
> not be broken by this patch.
>
> > The point of this patch (after a revision) should be to make it explicit
> > when and why this driver ignores the USB BC specification. And to reduce
> > the cases where it does, if possible.
>
> The kernel has no business forcing the limit to some fixed low value that
> has no relationship to the last BC detection result and breaks boot in
> the exact scenario this patch is targetting (no battery -> too high current
> limit).
>
> This doesn't make any sense to me.
>
> kind regards,
> o.
>
> > With the goal of making it explicit what cases ignore the spec, I would
> > prefer to have an opt-out mechanism. I compiled what I believe to be a
> > full list of devices that use this driver with usb bc enabled (detailed
> > notes below), and there's only a handful of them. It shouldn't be too
> > difficult to out-out the boards that need it.
> >
> > >
> > > Thank you and kind regards,
> > > o.
> >
> > Sorry it took me a while to respond, I haven't had much time to work on
> > this in the past few weeks.
> >
> > Regards
> > - Aren
> >
> > p.s. the notes on what devices use this functionality:
> >
> > These devices include the axp803 or axp81x dtsi:
> > $ rg -l 'include "axp(803|81x).dtsi"'
> > - sun50i-a100-allwinner-perf1.dts
> > - sun50i-a64-amarula-relic.dts
> > - sun50i-a64-bananapi-m64.dts
> > - sun50i-a64-nanopi-a64.dts
> > - sun50i-a64-olinuxino.dts
> > - sun50i-a64-orangepi-win.dts
> > - sun50i-a64-pine64.dts
> > - sun50i-a64-pinebook.dts
> > - sun50i-a64-pinephone.dtsi
> > - sun50i-a64-pinetab.dts
> > - sun50i-a64-sopine.dtsi
> > - sun50i-a64-teres-i.dts
> > - sun8i-a83t-allwinner-h8homlet-v2.dts
> > - sun8i-a83t-bananapi-m3.dts
> > - sun8i-a83t-cubietruck-plus.dts
> > - sun8i-a83t-tbs-a711.dts
> >
> > Out of those only these enable usb_power_supply:
> > $ rg -l 'include "axp(803|81x).dtsi"' | xargs rg -l 'usb_power_supply'
> > - sun50i-a64-bananapi-m64.dts
> > - sun50i-a64-pinetab.dts
> > - sun50i-a64-pinephone.dtsi
> > - sun8i-a83t-tbs-a711.dts
> > - sun8i-a83t-cubietruck-plus.dts
> > - sun8i-a83t-bananapi-m3.dts
> >
> > sun50i-a64-bananapi-m64.dts: The barrel jack is connected to acin, so
> > will be unaffected. Banannapi docs say it's not possible to power over
> > usb, but schematic suggests it should work. Probably needs to opt-out of
> > the lower current limit.
> >
> > sun50i-a64-pinetab.dts: unclear if charging is supported via usb, vbus
> > is connected through a component listed as "NC/0R". Regardless device
> > has barrel jack and battery for power, shouldn't need to run exclusively
> > from usb.
> >
> > sun50i-a64-pinephone.dtsi: is typically booted with a battery connected,
> > shouldn't need to run exclusively from usb.
> >
> > sun8i-a83t-tbs-a711.dts: has an internal battery, shouldn't need to run
> > exclusively from usb.
> >
> > sun8i-a83t-cubietruck-plus.dts and sun8i-a83t-bananapi-m3.dts: Both
> > appear to support being powered over usb and a barrel jack. These will
> > need to opt-out to be able to run from usb.
