diff mbox series

[v2,10/20] power: supply: bq25890: Add bq25890_set_otg_cfg() helper

Message ID 20211114170335.66994-11-hdegoede@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show
Series power-suppy/i2c/extcon: Fix charger setup on Xiaomi Mi Pad 2 and Lenovo Yogabook | expand

Commit Message

Hans de Goede Nov. 14, 2021, 5:03 p.m. UTC
Add a bq25890_set_otg_cfg() helper function, this is a preparation
patch for adding regulator support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
 1 file changed, 15 insertions(+), 13 deletions(-)

Comments

Yauhen Kharuzhy Nov. 15, 2021, 10:11 a.m. UTC | #1
On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
> Add a bq25890_set_otg_cfg() helper function, this is a preparation
> patch for adding regulator support.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> index 2bdfb58cda75..3c41fe86b3d3 100644
> --- a/drivers/power/supply/bq25890_charger.c
> +++ b/drivers/power/supply/bq25890_charger.c
> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>  	return PTR_ERR_OR_ZERO(bq->charger);
>  }
>  
> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
> +{
> +	int ret;
> +
> +	ret = bq25890_field_write(bq, F_OTG_CFG, val);
> +	if (ret < 0)
> +		dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);

Just a note: if a connected USB device has relative big capacitor
at power wires inside, then a starting current pulse may be enough to
overload the boost reguator and VBUS will not be powered. I met this
at Yoga Book: the firmware set boost current limit to 1.4 A (default
value for bq25892) but when USB hub connected, the BOOST_FAULT event
appeared.

To avoid this, Lenovo uses following trick in its kernel: set a boost
current limit to big value (2.1 A), wait some time (500 ms) and set
the current limit to right value (1.4A). This provides enough current to
charge capacitors in the connected device but saves desired long-time limit
to prevent overloading if the device consumes too much power itself.
Hans de Goede Nov. 16, 2021, 9:33 a.m. UTC | #2
Hi Yauhen,

On 11/15/21 11:11, Yauhen Kharuzhy wrote:
> On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
>> Add a bq25890_set_otg_cfg() helper function, this is a preparation
>> patch for adding regulator support.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
>>  1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>> index 2bdfb58cda75..3c41fe86b3d3 100644
>> --- a/drivers/power/supply/bq25890_charger.c
>> +++ b/drivers/power/supply/bq25890_charger.c
>> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>>  	return PTR_ERR_OR_ZERO(bq->charger);
>>  }
>>  
>> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
>> +{
>> +	int ret;
>> +
>> +	ret = bq25890_field_write(bq, F_OTG_CFG, val);
>> +	if (ret < 0)
>> +		dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
> 
> Just a note: if a connected USB device has relative big capacitor
> at power wires inside, then a starting current pulse may be enough to
> overload the boost reguator and VBUS will not be powered. I met this
> at Yoga Book: the firmware set boost current limit to 1.4 A (default
> value for bq25892) but when USB hub connected, the BOOST_FAULT event
> appeared.
> 
> To avoid this, Lenovo uses following trick in its kernel: set a boost
> current limit to big value (2.1 A), wait some time (500 ms) and set
> the current limit to right value (1.4A). This provides enough current to
> charge capacitors in the connected device but saves desired long-time limit
> to prevent overloading if the device consumes too much power itself.

Right I saw this in your git repo, but I cannot reproduce the issue (1)
I was hoping that since you can reproduce this, that you can rebase
your fix on top of my patch-set ?

Also I'm wondering if this behavior should be the default, I believe
that the max. boost current may also be dependent on some external
factors, so maybe we should make this behavior conditional on a
new device-property ?

Regards,

Hans



1) I must admit I did not try really hard, I guess I could try an
USB powered hdd enclosure with a spinning disk

What device are you seeing this with?
Hans de Goede Nov. 28, 2021, 3:02 p.m. UTC | #3
Hi,

On 11/16/21 12:07, Yauhen Kharuzhy wrote:
> 
> 
> аў, 16 ліс 2021, 12:33 карыстальнік Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> напісаў:
> 
>     Hi Yauhen,
> 
>     On 11/15/21 11:11, Yauhen Kharuzhy wrote:
>     > On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
>     >> Add a bq25890_set_otg_cfg() helper function, this is a preparation
>     >> patch for adding regulator support.
>     >>
>     >> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
>     >> ---
>     >>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
>     >>  1 file changed, 15 insertions(+), 13 deletions(-)
>     >>
>     >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
>     >> index 2bdfb58cda75..3c41fe86b3d3 100644
>     >> --- a/drivers/power/supply/bq25890_charger.c
>     >> +++ b/drivers/power/supply/bq25890_charger.c
>     >> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
>     >>      return PTR_ERR_OR_ZERO(bq->charger);
>     >>  }
>     >> 
>     >> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
>     >> +{
>     >> +    int ret;
>     >> +
>     >> +    ret = bq25890_field_write(bq, F_OTG_CFG, val);
>     >> +    if (ret < 0)
>     >> +            dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
>     >
>     > Just a note: if a connected USB device has relative big capacitor
>     > at power wires inside, then a starting current pulse may be enough to
>     > overload the boost reguator and VBUS will not be powered. I met this
>     > at Yoga Book: the firmware set boost current limit to 1.4 A (default
>     > value for bq25892) but when USB hub connected, the BOOST_FAULT event
>     > appeared.
>     >
>     > To avoid this, Lenovo uses following trick in its kernel: set a boost
>     > current limit to big value (2.1 A), wait some time (500 ms) and set
>     > the current limit to right value (1.4A). This provides enough current to
>     > charge capacitors in the connected device but saves desired long-time limit
>     > to prevent overloading if the device consumes too much power itself.
> 
>     Right I saw this in your git repo, but I cannot reproduce the issue (1)
>     I was hoping that since you can reproduce this, that you can rebase
>     your fix on top of my patch-set ?
> 
>     Also I'm wondering if this behavior should be the default, I believe
>     that the max. boost current may also be dependent on some external
>     factors, so maybe we should make this behavior conditional on a
>     new device-property ?
> 
> Yes, defining of max VBUS current may be a good idea. Another possible approach may be to use some empirical multiplier, like 150% of max 'long time' current limit setting. I almost sure that all hardware will work with short impulse of such current, its usual condition at device connection.
> 
> 
>     Regards,
> 
>     Hans
> 
> 
> 
>     1) I must admit I did not try really hard, I guess I could try an
>     USB powered hdd enclosure with a spinning disk
> 
>     What device are you seeing this with?
> 
> I cannot remember exactly device but this was a USB hub, possible with keyboard, mouse receiver and USB dongle inserted. I can recheck this issue but one week after, when will return home.

So as I mentioned before I've just tried to reproduce this problem, but
I cannot reproduce it with an 2.5" USB disk enclosure with a spinning
disk, which typically will cause a nice current-peak when spinning up.

I think this might also require an almost empty battery to reproduce ?

Regards,

Hans
Yauhen Kharuzhy Nov. 28, 2021, 7:46 p.m. UTC | #4
On Sun, Nov 28, 2021 at 04:02:06PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/16/21 12:07, Yauhen Kharuzhy wrote:
> > 
> > 
> > аў, 16 ліс 2021, 12:33 карыстальнік Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> напісаў:
> > 
> >     Hi Yauhen,
> > 
> >     On 11/15/21 11:11, Yauhen Kharuzhy wrote:
> >     > On Sun, Nov 14, 2021 at 06:03:25PM +0100, Hans de Goede wrote:
> >     >> Add a bq25890_set_otg_cfg() helper function, this is a preparation
> >     >> patch for adding regulator support.
> >     >>
> >     >> Signed-off-by: Hans de Goede <hdegoede@redhat.com <mailto:hdegoede@redhat.com>>
> >     >> ---
> >     >>  drivers/power/supply/bq25890_charger.c | 28 ++++++++++++++------------
> >     >>  1 file changed, 15 insertions(+), 13 deletions(-)
> >     >>
> >     >> diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
> >     >> index 2bdfb58cda75..3c41fe86b3d3 100644
> >     >> --- a/drivers/power/supply/bq25890_charger.c
> >     >> +++ b/drivers/power/supply/bq25890_charger.c
> >     >> @@ -801,6 +801,17 @@ static int bq25890_power_supply_init(struct bq25890_device *bq)
> >     >>      return PTR_ERR_OR_ZERO(bq->charger);
> >     >>  }
> >     >> 
> >     >> +static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
> >     >> +{
> >     >> +    int ret;
> >     >> +
> >     >> +    ret = bq25890_field_write(bq, F_OTG_CFG, val);
> >     >> +    if (ret < 0)
> >     >> +            dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
> >     >
> >     > Just a note: if a connected USB device has relative big capacitor
> >     > at power wires inside, then a starting current pulse may be enough to
> >     > overload the boost reguator and VBUS will not be powered. I met this
> >     > at Yoga Book: the firmware set boost current limit to 1.4 A (default
> >     > value for bq25892) but when USB hub connected, the BOOST_FAULT event
> >     > appeared.
> >     >
> >     > To avoid this, Lenovo uses following trick in its kernel: set a boost
> >     > current limit to big value (2.1 A), wait some time (500 ms) and set
> >     > the current limit to right value (1.4A). This provides enough current to
> >     > charge capacitors in the connected device but saves desired long-time limit
> >     > to prevent overloading if the device consumes too much power itself.
> > 
> >     Right I saw this in your git repo, but I cannot reproduce the issue (1)
> >     I was hoping that since you can reproduce this, that you can rebase
> >     your fix on top of my patch-set ?
> > 
> >     Also I'm wondering if this behavior should be the default, I believe
> >     that the max. boost current may also be dependent on some external
> >     factors, so maybe we should make this behavior conditional on a
> >     new device-property ?
> > 
> > Yes, defining of max VBUS current may be a good idea. Another possible approach may be to use some empirical multiplier, like 150% of max 'long time' current limit setting. I almost sure that all hardware will work with short impulse of such current, its usual condition at device connection.
> > 
> > 
> >     Regards,
> > 
> >     Hans
> > 
> > 
> > 
> >     1) I must admit I did not try really hard, I guess I could try an
> >     USB powered hdd enclosure with a spinning disk
> > 
> >     What device are you seeing this with?
> > 
> > I cannot remember exactly device but this was a USB hub, possible with keyboard, mouse receiver and USB dongle inserted. I can recheck this issue but one week after, when will return home.
> 
> So as I mentioned before I've just tried to reproduce this problem, but
> I cannot reproduce it with an 2.5" USB disk enclosure with a spinning
> disk, which typically will cause a nice current-peak when spinning up.
> 
> I think this might also require an almost empty battery to reproduce ?

Hi.

I tried to reproduce just now with success.

I have the UNITEK Y-2165B OTG USB hub with cardreader. It has three
100 uF capacitors connected to the VBUS inside.

If the boost current limit is set to 1.4A, boot failure condition is appeared
when the hub connected, there is no USB device detected.

If the limit is set to 2.15A, the device detected and works, there is no
fault condition appeared.

So, I think that you may add 1-3 100uF capacitors to
any USB device or cable and try to reproduce.

Debug I2C dumps are below.

Before connection of the hub:

root@yogabook:/home/jek# i2cdump -f -y 7 0x6b
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 06 00 0a 42 13 80 cc 03 44 73 02 00 8d 58 57    .?.?B????Ds?.?XW
10: 4e 00 00 00 05 ff ff ff ff ff ff ff ff ff ff ff    N...?...........
20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................

Immediately after connection (look to 0x0c register):

root@yogabook:/home/jek# i2cdump -f -y 7 0x6b
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 06 00 2a 42 13 82 cc 03 44 73 e2 40 8d 57 57    .?.*B????Ds?@?WW
10: 4d 00 00 00 05 ff ff ff ff ff ff ff ff ff ff ff    M...?...........
20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................

Increase current limit manually and restart the boost converter:

root@yogabook:/home/jek# i2cset -f -y 7 0x6b 0x0a 0x76
root@yogabook:/home/jek# i2cset -f -y 7 0x6b 0x03 0x0a
root@yogabook:/home/jek# i2cset -f -y 7 0x6b 0x03 0x2a

root@yogabook:/home/jek# i2cdump -f -y 7 0x6b
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 00 06 00 2a 42 13 82 cc 03 44 76 e2 00 8d 57 57    .?.*B????Dv?.?WW
10: 4d 00 00 00 05 ff ff ff ff ff ff ff ff ff ff ff    M...?...........
20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................



> 
> Regards,
> 
> Hans
>
diff mbox series

Patch

diff --git a/drivers/power/supply/bq25890_charger.c b/drivers/power/supply/bq25890_charger.c
index 2bdfb58cda75..3c41fe86b3d3 100644
--- a/drivers/power/supply/bq25890_charger.c
+++ b/drivers/power/supply/bq25890_charger.c
@@ -801,6 +801,17 @@  static int bq25890_power_supply_init(struct bq25890_device *bq)
 	return PTR_ERR_OR_ZERO(bq->charger);
 }
 
+static int bq25890_set_otg_cfg(struct bq25890_device *bq, u8 val)
+{
+	int ret;
+
+	ret = bq25890_field_write(bq, F_OTG_CFG, val);
+	if (ret < 0)
+		dev_err(bq->dev, "Error switching to boost/charger mode: %d\n", ret);
+
+	return ret;
+}
+
 static void bq25890_usb_work(struct work_struct *data)
 {
 	int ret;
@@ -810,25 +821,16 @@  static void bq25890_usb_work(struct work_struct *data)
 	switch (bq->usb_event) {
 	case USB_EVENT_ID:
 		/* Enable boost mode */
-		ret = bq25890_field_write(bq, F_OTG_CFG, 1);
-		if (ret < 0)
-			goto error;
+		bq25890_set_otg_cfg(bq, 1);
 		break;
 
 	case USB_EVENT_NONE:
 		/* Disable boost mode */
-		ret = bq25890_field_write(bq, F_OTG_CFG, 0);
-		if (ret < 0)
-			goto error;
-
-		power_supply_changed(bq->charger);
+		ret = bq25890_set_otg_cfg(bq, 0);
+		if (ret == 0)
+			power_supply_changed(bq->charger);
 		break;
 	}
-
-	return;
-
-error:
-	dev_err(bq->dev, "Error switching to boost/charger mode.\n");
 }
 
 static int bq25890_usb_notifier(struct notifier_block *nb, unsigned long val,