Surface Book 3 battery support

[qzed]

The second battery in the Book 3 seems to use a separate event system, likely KIP. This has yet to be confirmed.

[qzed]

Preliminary support for this has been implemented in f2339db.

@steveniemitz Can you test that this works as expected. Can you also check how this behaves when the clipboard is detached. Likely we'll have to remove the battery device, similar to the HID devices (touchpad/keyboard).

[qzed]

According to the log in linux-surface/linux-surface#201 (comment), the events should be enabled via KIP and IID=0. @steveniemitz or @NickyTope, can you try to confirm this by commenting-in this line, building the module with debug output (or change

surface-aggregator-module/module/surface_sam_sid_power.c

Lines 581 to 582 in b1bd428

	dev_dbg(&bat->pdev->dev, "power event (cid = 0x%02x, iid = %d, chn = %d)\n",
	event->command_id, event->instance_id, event->channel);

to dev_info or something like that) and trying to trigger battery events on the second channel (e.g. disconnect AC and wait until the first events arrive, could take a bit; maybe detaching also works, but I have no clue how the second battery behaves with that yet)?

With debug output enabled or the line changed, you can grep dmesg for power event and look for lines with chn = 2. Once you've found a way to reliably trigger those events, run (from the feature/sb3-v3 branch):

./scripts/evreg.py kip-disable 0x02 0x01 0x00

and confirm that the events are disabled. After that, enable events again with

./scripts/evreg.py kip-enable 0x02 0x01 0x00

and confirm they're working again.

[xanf]

@qzed I've tried to follow your guides (I also own SB3), but it seems I'm missing something - device is missing:

FileNotFoundError: [Errno 2] No such file or directory: '/sys/bus/serial/devices/serial0-0/rqst'

I've built module from sb3-v3 branch, patched that line, I can see messages with both chn but device is missing
I'm running stock arch kernel with this module installed via dkms

[qzed]

Oh, right. I forgot to mention: You'll have to comment-in this line (and re-build the module) for the script to work.

[xanf]

@qzed seems to be working as expected!

boot

[    2.012305] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 1)
[    2.012309] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 1)
[    2.013959] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[    2.013962] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)

disconnecting AC

[   38.940423] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 2)
[   38.940426] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 2)
[   39.006385] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 1)
[   39.006387] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 1)
[   39.008351] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[   39.008352] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)

discharging...

[  179.093977] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  179.093979] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)
[  244.113441] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 2)
[  244.113443] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 2)
[  264.121893] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x18, iid = 1, chn = 2)
[  264.121896] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x18, iid = 1, chn = 2)
[  318.190310] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  318.190312] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)

AC back

[  364.017567] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 2)
[  364.017569] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 2)
[  364.095955] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 1)
[  364.095957] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 1)
[  364.097610] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  364.097611] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)
[  365.066878] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 2)
[  365.066881] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 2)
[  367.070028] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 2)
[  367.070030] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 2)

Executing ./scripts/evreg.py kip-disable 0x02 0x01 0x00

disconnecting AC

[  410.469492] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 1)
[  410.469494] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 1)
[  410.471268] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  410.471270] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)

discharging...

[  461.332990] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  461.332993] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)
[  550.516444] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  550.516447] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)
[  593.106862] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 1)
[  593.109838] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 1)
[  593.985346] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 1)
[  593.988606] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 1)
[  613.055327] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 1)
[  613.059608] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 1)
[  613.932640] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 1)
[  613.936806] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 1)
[  690.593874] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  690.593876] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)

AC back

[  791.361829] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 1)
[  791.361831] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 1)
[  791.363760] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  791.363841] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)

Executing ./scripts/evreg.py kip-enable 0x02 0x01 0x00

disconnecting AC

[  869.543938] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 2)
[  869.543941] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 2)
[  869.621583] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x17, iid = 1, chn = 1)
[  869.621585] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x17, iid = 1, chn = 1)
[  869.623464] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x53, iid = 1, chn = 1)
[  869.623466] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x53, iid = 1, chn = 1)
[  870.593215] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 2)
[  870.593218] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 2)
[  871.595211] surface_sam_sid_battery surface_sam_sid_battery.1: power event (cid = 0x16, iid = 1, chn = 2)
[  871.595214] surface_sam_sid_battery surface_sam_sid_battery.2: power event (cid = 0x16, iid = 1, chn = 2)

[qzed]

@xanf Perfect, thanks! I'll merge the current battery driver into master.

The last thing we need to figure out is how the battery behaves when detaching. Could you try to detach the clipboard and check how upower/your desktop environment shows the second battery when detached (on Gnome, displays for both batteries individually can be found in the settings app). Specifically, does it show the second battery as present/having some charge/0% charge or does it (correctly) show the battery as disconnected/not present?

[xanf]

@qzed on detaching it takes up to two minutes for extra battery to disappear.
On attaching extra battery immediately appears in system.

Offtopic: it seems HDMI is not restored on attaching device (and sometimes simply on resume from suspend) :(

[qzed]

Neat, thanks for testing! So I think we should remove the battery device on detaching. Edit: I think the 2 minute timeout might come from upower, polling the device. So there doesn't seem to be any event/notification sent when the base is disconnected, meaning the disconnect is only noticed when upower actively polls the device the next time.

I've merged the current state into the linux-surface repo, so there should now be basic battery, keyboard, and touchpad support for the SB3 built into the kernel packages.

No clue what's wrong with the HDMI thing. AFAIK that's over USB? Could be that some PM quirk or something has to be set.

[xanf]

"over USB" - kind of
According to https://eu.mouser.com/applications/usb-type-c-adds-hdmi/ it's just "hdmi lines" inside usb-c

[qzed]

Ah, thanks for the link!

[xanf]

@qzed just a quick check - should built-in keyboard / touchpad be non-functional on detach/reattach with latest master of this repo?

[qzed]

IIRC @steveniemitz indicated that the keyboard works and touchpad does too, however only in single-touch mode. There could also be other problems (such as them not working at all). Essentially what we should do is disconnect/remove all of those devices (battery, keyboard, touchpad, ...) in-kernel when the base is detached and re-attach/re-create them when the base is re-attached. That's still on my todo-list and I'm probably going to open up a separate issue for that soon-ish.

So TLDR: There currently isn't any handling at all for detach/re-attach in these driver (apart from DTX on feature/sb3-v3, which is another thing entirely), so behavior will depend on how the subsystem (and potentially user-space) handles that.

[steveniemitz]

yeah I think something might have regressed at some point, it looks like the keyboard/touchpad don't get re-enabled on re-attach anymore.

I think this is also tracked in #42, but also agree it might be better to open a new issue for the device remove/add feature.

[qzed]

In the mean time, you should be able to work around this issue by re-loading the surface_sam_sid_vhf module.

[qzed]

As the main support for the battery has been implemented, I'm going to close this issue. The detach/attach issues will be handled via #46.