battery charger driver

[networkimprov]

Manual: http://www.ti.com/lit/ds/symlink/bq24190.pdf PMIC TRM: http://www.droid-developers.org/images/2/21/Tps65950_TRM.pdf Source: https://github.com/networkimprov/linux/blob/anvl-v4.7/drivers/power/bq24190_charger.c Discussion of USB charging: https://lwn.net/Articles/694062/ Charger Manager API: https://github.com/networkimprov/linux/blob/anvl-v4.7/include/linux/power/charger-manager.h See also: https://github.com/networkimprov/usb-manager/issues/3

1) Review commits to mainline source since it landed. Code was originally commissioned for anvl project. https://github.com/torvalds/linux/commits/master/drivers/power/bq24190_charger.c

2) Check these variations of the driver for stuff we might need... bq2419x from android 3.10 Tegra kernel: https://android.googlesource.com/kernel/tegra/+/android-tegra-3.10/drivers/power/bq2419x-charger.c patches for it: https://github.com/CaptainThrowback/kernel_android-tegra-flounder-3.10/commits/BeyondStock-M/drivers/power/bq2419x-charger.c another variation, pt 1: https://github.com/CaptainThrowback/kernel_android-tegra-flounder-3.10/blob/BeyondStock-M/drivers/power/bq2419x-charger-htc.c pt 2: https://github.com/CaptainThrowback/kernel_android-tegra-flounder-3.10/blob/BeyondStock-M/drivers/power/htc_battery_bq2419x.c charge-related patches in that repo: https://github.com/CaptainThrowback/kernel_android-tegra-flounder-3.10/commits/BeyondStock-M

3) Attempt to enable charging after a disconnect on error. This can happen if the USB cable is not plugged into a hub in a straight angle and the USB hub port is loose enough to allow slight sideways movement.

4) Handle OTG pin if its gpio is specified in the dts. See https://github.com/networkimprov/arch-packages/blob/master/archlinux-anvl/anvl-util/anvl-otg.sh

5) power_supply_changed() uevents don't indicate what changed, so we can't write a udev rule that triggers only when vbus_stat changes. Possible fixes: a) un/register bq24190-charger on vbus-up/down to issue add/remove uevents. Move the sysfs group to bq24190-battery. Enables this: http://unix.stackexchange.com/questions/63232/what-is-the-correct-way-to-write-a-udev-rule-to-stop-a-service-under-systemd b) un/register a virtual device on vbus-up/down to issue add/remove uevents. See http://stackoverflow.com/questions/5970595/create-a-device-node-in-code c) register a 3rd power_supply to call power_supply_changed on vbus up/down. See https://github.com/networkimprov/linux/commits/anvl-v4.7-hack_bq24190 (note: crashes on boot if usb connected) d) add user_stat field to let the user maintain a state value (e.g. previously reported vbus_stat value). This cannot be done with udev env{x} variables; see [1] below. e) implement sysfs_notify, so user apps can poll() /sys...charger/*

6) uevent for disconnect often appears long after the plug is pulled. musb-hdrc sees it immediately. If we can't get a prompt event from the charger, we'll need one from usb subsystem.

7) Issue bq24190-battery changed uevents only if f_reg or ss_reg change is relevant to battery.

8) bq24190_charger_supplied_to gives "main-battery". Is there such an entity, if not should it be "bq24190-battery", and in either case would it actually receive charger events since no external_power_changed callback is implemented? https://www.kernel.org/doc/Documentation/power/power_supply_class.txt

9) Call power_supply_changed with good reason. Clear bdi->ss_reg/f_reg in probe() and in resume() before interrupts enabled. Drop bdi->first_time. Do set_mode_host() in resume. Drop power_supply_changed() in resume(). See https://github.com/networkimprov/linux/commits/anvl-v4.7-lb-bq24190-a/drivers/power/bq24190_charger.c

10) Fault mgmt looks wrong. irq_handler_thread() should read fault reg 2x on fault; if 1st read!=0, cache 2nd read, else cache 0. Attribute getters use cache. Use RW spinlocks http://www.makelinux.net/ldd3/chp-5-sect-5

11) Do we get vbus_stat & chrg_stat interrupts after hw_init() if USB connected before boot?

12) What causes periodic 3-4s bursts of uevents when connected to usb charging port (usb phy modules blacklisted, no gadget config)?

13) Verify that charger draws 1.5A from charging port given suitable cable.

14) For USB host mode, we need to add regulator framework support to the charger driver so the USB PHY driver can just request the regulator for 5V VBUS as needed.

15) Disable omap off_mode while charging (while usb connected?) as this triggers a bug which loses interrupts.

[1] These rules give multiple on and zero off results because env{vbus} gets erased KERNEL=="bq24190-charger", ATTR{f_vbus_stat}!="0", ENV{VBUS}!="1", ENV{VBUS}="1", RUN+="/bin/bash -c 'echo vbus_on > /dev/console'" KERNEL=="bq24190-charger", ATTR{f_vbus_stat}=="0", ENV{VBUS}=="1", ENV{VBUS}="0", RUN+="/bin/bash -c 'echo vbus_off > /dev/console'"

[networkimprov]

Regarding competition between PMIC & BQ24190 for D+ line on cable insertion. Markku's (Haltian EE) comments from email:

1) No USB cable inserted -> disable USB block of PMIC. This puts PMIC resistors to high impedance state. Set BQ_OTG_CTRL to “0” state. Wait USB VBUS. [pg_stat?] 2) VBUS detected -> A charger detection by a charger chip. Keep PMIC in disable mode during it (delay to PMIC). 3) Detection done -> Charging current is set to right level by the charging chip (100mA or 1,47A). 4) After delay try to enumerate [enable PMIC USB]. If a wall charger is detected earlier, please do not touch to current settings. If not, set BQ_OTG_CTRL to right state depending of HUB (100mA or 500mA). 5) VBUS disappears [or pg_stat down] -> set PMIC to disable mode and BQ_OTG_CTRL to “0” state.

Re PMIC disabling, resistors are inside PMIC. It sets 1,5kohm pull up to D+ line to indicate it is device (not host). Issue is that pull up timing not value. We have made this kind of delay by SW example to our Thingsee device.

[networkimprov]

Just found this API, wondering if is applicable to us: https://github.com/networkimprov/linux/blob/anvl-v4.7/include/linux/power/charger-manager.h

[networkimprov]

Edited this a few times; reposting... Re GPIO patch: https://github.com/networkimprov/linux/commit/0ad6c3c50ae280c456b212ae4fe6283f67c78331

It's not clear why pg_stat determines gpio state vs vbus_stat. If correct, maybe comment it.

No need to read reg if !pg_state. Try...

#define BQ24190_REG_ISC_IINLIM_500M BIT(1)
...
u8 inlim;
if (pg_state) {
  ret = bq24190_read_mask(bdi, BQ24190_REG_ISC,
    BQ24190_REG_ISC_IINLIM_MASK,
    BQ24190_REG_ISC_IINLIM_SHIFT, &inlim);
  if (ret < 0)
    return ret;
  pg_state = inlim == BQ24190_REG_ISC_IINLIM_500MA;
}
gpio_set_value_cansleep(bdi->gpio_otg, pg_state);

[mranostay]

@networkimprov @tmlind v2 is ready for review.. hasn't been tested till I have a battery cable.

https://github.com/networkimprov/linux/commit/7cc291515c1e28e3ef56a9cda6fd25a439f757cd

[networkimprov]

int ret is empty if !pg_state; try...

int ret = bq24190_read_mask(... // int decl here
...
gpiod_set_value_cansleep(...
return 0; // unless gpiod_set_value returns something we should return

Use vbus mask here or comment on why pg is used: bq24190_update_otg(bdi, !!(ss_reg & BQ24190_REG_SS_PG_STAT_MASK)

[networkimprov]

[deleted discussion of git to this point]

We need to avoid proliferation of public branches.

Get a single patch for a set of commits: $ curl -sLO https://github.com/networkimprov/linux/compare/anvl-v4.7...40d11b3d0b.patch See https://github.com/networkimprov/linux/compare/anvl-v4.7...40d11b3d0b.patch

Then git apply that to master, commit, and push.

[networkimprov]

I committed the above fix to your branch.

Re testing, you can see the current draw on USB meter. If the gpio patch works and battery is partly drained, plugging into a PC should draw 500mA (default is 100). What does connecting a power supply buy you?

[mranostay]

@networkimprov just basically not having to drain the battery. A battery with any decent charge causes the HIZ state on a USB 2.0 100 mA host before negotiation to 500 mA.

[networkimprov]

Pls review these closely. It's a minor cleanup, but you have a lot more exp than I do with kernel drivers.

https://github.com/networkimprov/linux/commit/106e77ae771c09bcb706e4be08007e947da395a6 - resume() omits power_supply_changed() https://github.com/networkimprov/linux/commit/fe89faaf6b06b3ee5b4de259e8b23b0b2e9fd72b - probe() and resume() zero cached state https://github.com/networkimprov/linux/commit/ee5dba3dd0f0bc94b0cb53d560d19ce379984b7f - suspend() omit register_reset() https://github.com/networkimprov/linux/commit/c6f6dcc919e99a6e4b7e334618ca2d960832086a - irq_handler_thread() read chip registers together https://github.com/networkimprov/linux/commit/256361d7d8b25d915e466a0ab8b7a4a60acfeaa6 - update_otg() clear HIZ state

[networkimprov]

...and I think you found a HIZ bug in update_otg() or elsewhere in driver. Hopefully fixed in a commit I added to previous note.

[mranostay]

On Wed, Sep 14, 2016 at 5:28 AM, Liam notifications@github.com wrote:

...and I think you found a HIZ bug in update_otg() or elsewhere in driver. Hopefully fixed in a commit I added to previous note.

Actually not. it is part of the USB Battery Charging Spec 1.2. But I just run cpuburn for about 20 minutes to get down to a battery to state I can test the OTG state

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-246997132, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXObO8T-a2CtThnNxvNSByLdY7eqedks5qp-hhgaJpZM4GveUL .

[networkimprov]

There is nothing in the spec about HIZ mode. The only near-match in the current table is data contact detect. http://composter.com.ua/documents/BC1.2_FINAL.pdf#55

The spec allows drawing 500mA if the USB host can provide it, which we know from iinlim==2. The manual tells us to check for HIZ, and how to turn it off, but not when. So pls try my patch!

[mranostay]

@networkimprov See section 9.3.1.3.1 http://www.ti.com/lit/ds/symlink/bq24190.pdf

Maybe not correct.. but not sure what we can do with a full battery in which 500 mA isn't shown in IINLIMIT

[networkimprov]

That is a temporary condition while VBUS is rising after connect. Didja try patch? You should see 500mA for any PC host.

Re commit comments...

So want a uevent happening at every interrupt?

It's always been this way; I plan to get more selective.

This is going to cause double uevents with the fuel gauge driver I suspect

Possibly but we can ignore them.

No issue with the zero'ing of everything here but devm_kzalloc() already did it Do these ever get referenced without a bq24190_read() beforehand? So zeroing I don't think matters

Yes, they were ref'd before set in first call of irq_handle_thread()

You need to move the mutex_lock as well

Considered that; it's never used to protect reads elsewhere.

[mranostay]

On Wed, Sep 14, 2016 at 1:42 PM, Liam notifications@github.com wrote:

You should see 500mA for any PC host. (btw tried to IM you)

Sorry don't usually keep hangouts open.. I do see f_iinlimit of 2 (500 mA) usually, I'll check the vbus rising status, after I test the configurable fuel gauge polling.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-247146932, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOUmrXiHo0MAiN2-UWHeyfyCXJw97ks5qqFxQgaJpZM4GveUL .

[networkimprov]

I think we need to make a virtual device to emit uevents on vbus on/off. We'd un/register the device in the irq_handler_thread on vbus change (right next to your update_otg call :) Can you tackle this?

See http://stackoverflow.com/questions/5970595/create-a-device-node-in-code

Also, let's add a HIZ-off before inlim read to your -v2 otg branch. The function should maybe be renamed to update_current()

[mranostay]

On Thu, Sep 15, 2016 at 1:31 AM, Liam notifications@github.com wrote:

I think we need to make a virtual device to emit uevents on vbus on/off. We'd un/register the device in the irq_handler_thread on vbus change (right next to your update_otg call :) Can you tackle this?

I'll look into this.

Thanks,

Matt

See http://stackoverflow.com/questions/5970595/create-a- device-node-in-code

Also, let's add a HIZ-off before inlim read to your -v2 otg branch. The function should maybe be renamed to update_current()

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-247268611, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOTY4689fYIuu6pcQ7HbIkCdi_szkks5qqQJbgaJpZM4GveUL .

[networkimprov]

It seems like we won't need to patch musb from Tony's last note re bq24190 interrupts but who knows. Let's investigate whether musb can emit uevents of any sort; maybe extending it to emit vbus-on/off uevents would be simple and upstreamable.

A more upstreamable scheme for BQ driver than "un/register virtual device" might be to make a kobject for "vbus" and use it to issue add/remove uevents. I can always mknod/unlink in the udev rule that catches them, or maybe we can hang a sysfs file on the kobject?

[mranostay]

On Thu, Sep 15, 2016 at 12:23 PM, Liam notifications@github.com wrote:

It seems like we won't need to patch musb from Tony's last note re bq24190 interrupts but who knows. Let's investigate whether musb can emit uevents of any sort; maybe extending it to emit vbus-on/off uevents would be simple and upstreamable.

A more upstreamable scheme for BQ driver than "un/register virtual device" might be to make a kobject for "vbus" and use it to issue add/remove uevents. I can always mknod/unlink in the udev rule that catches them, or maybe we can hang a sysfs file on the kobject?

Yeah I like that idea much better.. since it is maybe upstreamable

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-247426190, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOYabGH346A_7AvrJahdis9p9Xnphks5qqZsWgaJpZM4GveUL .

[tmlind]

Yeh the musb phy does legacy sysfs events, but hopefully we don't need those

[mranostay]

Yep we get VBUS events in omap2430_musb_set_vbus() and omap_musb_set_mailbox()... Probably pretty simple to add a uevent framework. Checking other usb host drivers to see if they do anything similar

Edit: @networkimprov @tmlind Does this seems a sane way to get VBUS events reported to userspace? https://github.com/networkimprov/linux/commit/3bc79026e85b31a6f3e73410cc27c30915f1c8d6

[networkimprov]

I committed a HIZ fix to your -v2 branch. Pls review/test when you have a moment.

[mranostay]

Ok will test.. not sure it will help with the IINLIMIT being reported as 100 mA

@networkimprov Which of the bullet points in the issue still need to be worked on?

[networkimprov]

You've started on 4 (current/otg) & 5 (connect uevents); I'm working on 7 (uevent relevance) & 9 (host mode on resume).

About to start on 10 (fault handling). I need advice on rw-spinlocks for use in irq_handler_thread() and the attribute getters, where mutex has been used. My plan is to read the fault reg 2x in case of fault, and cache second value for use by getters.

Re 5, do you still need an upstreamable spot in musb to emit events from?

3 is important: Attempt to enable charging after a disconnect on error. This can happen if the USB cable is not plugged into a hub in a straight angle and the USB hub port is loose enough to allow slight sideways movement. Tony may have ideas about how to simulate such errors.

[mranostay]

@networkimprov Ok for 5 per @tmlind 's suggestion we should move the events to the phy driver directly. Will check how much work that would be.

Ok yeah for 3 I suspect the musb IP block is to blame... noticed a few comments in the code in the musb driver. Just need to figure a way to to test it... anyone have a suggestions on a USB hub with horrible port tolerences? :)

[networkimprov]

We need the phy uevents in any case, since I can't tell which charger uevent is connect; keep goin on that one.

Hm, it's possible that my new fault-mgmt concept would address 3. It's not related to musb to my knowledge.

[mranostay]

@networkimprov HIZ patchset seems to improve things.. albeit haven't done a lot of testing of it yet

[networkimprov]

Pls review this patch re emitting uevents on stat and fault changes, which replaces the previous patches you reviewed from this branch: https://github.com/networkimprov/linux/commit/280d3a3a5e3cc20281ec4f682fd2f086ecfdc33b

Re git branching, you prob want to start the branch for each component from the anvl-v4.7 branch, unless you need newer commits from a different one.

[mranostay]

Pls review this patch re emitting uevents on stat and fault changes, which replaces the previous patches you reviewed from this branch: https://github.com/networkimprov/linux/commit/280d3a3a5e3cc20281ec4f682fd2f086ecfdc33b

Reviewed

[networkimprov]

Thx for input, revised: https://github.com/networkimprov/linux/compare/5e2a97e42c63a1b29117c9f1f93b1c041b865348...a9d65c6428692fb6ecdd9a8544fa79a7cbf88702

[mranostay]

Looks okay to me. Minus one rogue white space you have at the end.

On Sat, Sep 17, 2016 at 1:19 PM, Liam notifications@github.com wrote:

Thx for input, revised: networkimprov/linux@5e2a97e...a9d65c6 https://github.com/networkimprov/linux/compare/5e2a97e42c63a1b29117c9f1f93b1c041b865348...a9d65c6428692fb6ecdd9a8544fa79a7cbf88702

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-247805128, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOTsNhPjOeX2s8Yz6zEIALX9BxH_3ks5qrEs8gaJpZM4GveUL .

[networkimprov]

Any insight on phy uevents?

[mranostay]

@networkimprov Got sidetracked with the battery gauge Friday. Working on this today.

Also for (3) I noticed a workaround for the in the am335x bits so that is useful..

[networkimprov]

Re (3), we need to replicate/detect the failure to charge after connect and then peer into the charger's registers to understand what state it got stuck in. We would probably fix it in the interrupt handler. I think Tony has detected this with the current meter.

[mranostay]

The interrupt handler I assume you mean is omap2430_musb_interrupt()?

[networkimprov]

(3) is a bq24190_charger issue; not related to USB phy. Connect/disconnect uevents (5) is the only item above related to usb subsystem (out of necessity; power_supply api can't help us).

[mranostay]

Ok makes more sense.. did we conclude that the uevent could only be a KOBJ_ONLINE and KOBJ_OFFLINE or do we need more information about VBUS state via properties? I assume for gadget mode we need to detect MUSB_ID_GROUND for the ID pin

[networkimprov]

KOBJ_ON/OFFLINE prob makes the most sense; could also be _ADD/REMOVE.

Ideally we'd create a "vbus" sysfs file which appears for ONLINE/ADD and disappears for OFFLINE/REMOVE. If that's acceptable, prob no need to attach attributes to the uevent directly. Also systemd units could BindTo that file.

How would checking the ID pin impact uevent calls? We only run in gadget mode, btw.

[mranostay]

Please review. uevent is now generated from phy interrupt

https://github.com/networkimprov/linux/commit/cfb3ba8cd5f39e08f9c459dddb9984507a0ea816

[networkimprov]

And a sysfs file? Or is that not upstreamable?

[mranostay]

@networkimprov sysfs for reporting what? VBUS status? I would suspect that wouldn't be accepted upstream

[networkimprov]

A sysfs file to serve as a virtual device which goes and comes with dis/connect. Tho I can prob do this with mknod/unlink in udev rule

Also waiting for your review of https://github.com/networkimprov/linux/compare/7162ddc4...aabccacd commit message here https://github.com/networkimprov/linux/commit/ac0bfdd4

[mranostay]

On Mon, Sep 19, 2016 at 4:11 PM, Liam notifications@github.com wrote:

A sysfs file to serve as a virtual device which goes and comes with dis/connect. Tho I can prob do this with mknod/unlink in udev rule

Yeah I suspect wouldn't be accepted because sysfs isn't going to be deterministic of the state.. sure sysfs_notify exists

Also waiting for your review of networkimprov/linux@7162ddc...aabccac https://github.com/networkimprov/linux/compare/7162ddc4...aabccacd commit message here networkimprov/linux@ac0bfdd https://github.com/networkimprov/linux/commit/ac0bfdd4

Will review.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-248155695, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOdeGKF8pylGOrA2yLVtvZfVAt5ddks5qrxalgaJpZM4GveUL .

[networkimprov]

Did my reply to your comment here satisfy your concern? https://github.com/networkimprov/linux/commit/ac0bfdd4

[mranostay]

On Mon, Sep 19, 2016 at 8:35 PM, Liam notifications@github.com wrote:

Did my reply to your comment here satisfy your concern? networkimprov/linux@ac0bfdd https://github.com/networkimprov/linux/commit/ac0bfdd4

Ok sorry didn't see that till now. Yeah makes sense

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-248192726, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOZRLm_YcYa3g9i1hxsHveEVfClMxks5qr1SHgaJpZM4GveUL .

[networkimprov]

phy uevent patch can go upstream for review.

Re (3) that might be related to misconfigured interrupt gpio. Now fixed in u-boot but dts still awaiting attn from Tony...

[mranostay]

On Tue, Sep 20, 2016 at 12:11 PM, Liam notifications@github.com wrote:

phy uevent patch can go upstream for review.

Ok will do. Have mixed feelings on if this will get any traction upstream.

Re (3) that might be related to misconfigured interrupt gpio. Now fixed in u-boot but dts still awaiting attn from Tony...

Ok cool I'll hold off on that one... esp after looking how I'd have to really hack this in :)

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-248402119, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOZRx5AEAlYl-aoPhi18XY9SCfAsWks5qsC_ygaJpZM4GveUL .

[networkimprov]

Ok will do. Have mixed feelings on if this will get any traction upstream.

Well cc Tony and if you get any flak ping him privately to request backup. I doubt he'd have said OK if the other phy maintainers would object.

I'd have to really hack this in :)

Are you referring to musb again? (3) is only about making the charger work when the usb connectors are flaky.

[mranostay]

On Tue, Sep 20, 2016 at 12:39 PM, Liam notifications@github.com wrote:

Ok will do. Have mixed feelings on if this will get any traction upstream.

Well cc Tony and if you get any flak ping him privately to request backup. I doubt he'd have said OK if the other phy maintainers would object.

I'd have to really hack this in :)

Are you referring to musb again? (3) is only about making the charger work when the usb connectors are flaky.

No I was referring to the charger. So the flakey usb connection has nothing to with incorrect pinmuxing?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/networkimprov/arch-packages/issues/39#issuecomment-248410484, or mute the thread https://github.com/notifications/unsubscribe-auth/AAwXOdkVz_wJAIuAJ4_k0ouubdto1DjTks5qsDaNgaJpZM4GveUL .

[networkimprov]

Oh, what hacking did you mean? I don't see why the charger would get stuck if usb source comes and goes. Maybe it ends up in HIZ (hopefully fixed now) or a fault state. But we can't test it without correct interrupt gpio config.