| Message ID | 20240130203714.3020464-6-aren@peacevolution.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: axp20x_usb_power: cleanup input current limit handling | expand |
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 <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. > > (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 <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. > > > > (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.
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);
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 <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. (no changes since v1) drivers/power/supply/axp20x_usb_power.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)