Bug 240339 - [ig4] I2C2 (touchpad bus) broken on the Google Pixelbook (Sunrise Point PCH)
Summary: [ig4] I2C2 (touchpad bus) broken on the Google Pixelbook (Sunrise Point PCH)
Status: Closed Not A Bug
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-04 22:57 UTC by Greg V
Modified: 2019-09-15 19:17 UTC (History)
1 user (show)

See Also:


Attachments
ig4.patch (38.55 KB, patch)
2019-09-10 20:02 UTC, Vladimir Kondratyev
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg V 2019-09-04 22:57:37 UTC
The Pixelbook (google/eve) has four ig4 controllers enabled.

I2C0 is used for the touchscreen, and it works fine (using https://github.com/wulf7/iichid for the HID).

I2C2 is used for the touchpad, and it doesn't:

- out of the box nothing can be detected on the bus, all commands time out;
- commenting out the code that asserts the reset (IG4_REG_RESETS_SKL), i.e. NOT doing the LPSS reset (0x204), allows the touchpad's devices (HID and EC) to be seen on that bus;
- but they still quickly "disappear" from the bus (so something goes wrong in the controller) when I try to listen to hid events using libinput.

Setting slow polling in hid allows it to last for a bit, but eventually it still breaks.

This is not related to speed settings, the firmware sets them correctly and I commented out the code that replaces them — no difference.

Everything works fine in Linux (I tested SystemRescueCD) and is at least fully detected in NetBSD (I only booted the installer, can't do anything in there really). btw NetBSD doesn't assert the LPSS reset, only deasserts it (writes 0x3). though Linux does both.
Comment 1 Vladimir Kondratyev freebsd_committer 2019-09-09 21:13:00 UTC
(In reply to Greg V from comment #0)
Skylake's initialization currently broken due to SDA_HOLD register set too low. Try following patch (power off is needed after install):

--- a/sys/dev/ichiic/ig4_iic.c
+++ b/sys/dev/ichiic/ig4_iic.c
@@ -590,6 +590,7 @@ ig4iic_attach(ig4iic_softc_t *sc)
        reg_write(sc, IG4_REG_SS_SCL_LCNT, 125);
        reg_write(sc, IG4_REG_FS_SCL_HCNT, 100);
        reg_write(sc, IG4_REG_FS_SCL_LCNT, 125);
+       reg_write(sc, IG4_REG_SDA_HOLD, 28);
 
        /*
         * Use a threshold of 1 so we get interrupted on each character,
Comment 2 Greg V 2019-09-10 00:29:27 UTC
(In reply to Vladimir Kondratyev from comment #1)

It was set to 24. Looks like 28 did allow the reset, but did not fix the controller locking up or whatever is happening after a couple seconds of use.

Interestingly, with the touchpad, unlike the touchscreen, ig4 interrupts are happening constantly, without any touches and without anyone opening the evdev.
Comment 3 Vladimir Kondratyev freebsd_committer 2019-09-10 05:48:28 UTC
(In reply to Greg V from comment #2)
> did allow the reset
What does it mean? You must not reset controller after timings have been set
Comment 4 Greg V 2019-09-10 08:41:45 UTC
(In reply to Vladimir Kondratyev from comment #3)
I mean it's no longer necessary to comment out the Skylake lpss reset (the line that writes zero to 0x204)
Comment 5 Vladimir Kondratyev freebsd_committer 2019-09-10 08:45:49 UTC
Try to set SDA_RX_HOLD to 1. That is what Linux does.
reg_write(sc, IG4_REG_SDA_HOLD, 28); -> reg_write(sc, IG4_REG_SDA_HOLD, 0x10000 + 28);
Comment 6 Greg V 2019-09-10 18:04:01 UTC
(In reply to Vladimir Kondratyev from comment #5)
That did not help..

I looked at the Linux code, it sets (1 << 16) actually. (Also did not help.) Zircon (Fuchsia) does not do this. The datasheet for Skylake does not list any such field in that register, 15:0 are the hold time and 31:16 are reserved.
Comment 7 Vladimir Kondratyev freebsd_committer 2019-09-10 20:02:12 UTC
Created attachment 207356 [details]
ig4.patch

You can try my ig4 driver version. It borrows timings from LPSS driver and have some other improvements like polling mode, suspend/resume, some error handling and so on.
Comment 8 Vladimir Kondratyev freebsd_committer 2019-09-10 20:12:28 UTC
(In reply to Greg V from comment #6)
> The datasheet for Skylake does not list any such field in that register, 15:0 are the hold time and 31:16 are reserved.
This field, like some other slave-mode related parameters, is not exposed in desktop-class chipset specs, but exposed in SOCs specs e.g. Arria SOCs and still has an effect as these devices shares common design.
Comment 9 Greg V 2019-09-10 23:08:53 UTC
(In reply to Vladimir Kondratyev from comment #7)

oh cool, you already wrote the ACPI lookup of parameters, I was thinking about doing that haha. Though the values that it ends up applying seem really high (564, 480 on i2c2 for SCL). I commented that out again

Unfortunately the problem is still there.

Some more testing:

- after the lockup in FreeBSD, Linux cannot recover that i2c bus either — rebooting (not shutting down and powering up) into Chrome OS results in 'i2c_designware.2: controller timed out' and the touchpad not working;

- the EC interface on the same bus allows reading the console of the touchpad's firmware, but that's impossible when the controller is locked up.. I'm waiting for a debug cable to ship, but the documentation doesn't say that it's possible to access the touchpad console through that;

- I added some logging to the error handling, on i2c0 (working touchscreen) after reading the touchscreen's descriptor for setup there's an RX underflow, but here (touchpad) that did not happen, none of these errors happened;

- with HAVE_IG4_POLLING in iichid, there's no RX underflow there (obviously), but it does not fix the lockup.

I wonder if it might actually be iichid/imt doing something that causes the touchpad to do something wrong with the bus…

log: https://gist.github.com/myfreeweb/67b3ca690c22b31f502079b11263f0f4
Comment 10 Greg V 2019-09-10 23:11:21 UTC
(In reply to Greg V from comment #9)

* the parameters are fine, I was expecting fast mode numbers and printing slow mode ones
Comment 11 Vladimir Kondratyev freebsd_committer 2019-09-11 00:09:03 UTC
(In reply to Greg V from comment #9)
> there's an RX underflow, but here (touchpad) that did not happen, none of these errors happened;
RX underflow has happened on attempt to read THQA certificate which is discarded right after it has been read. So failure to read it is not fatal.
By my experience "RX underflow" is caused by ig4 bugs usually, but I am not 100% sure that it is a law.

> - with HAVE_IG4_POLLING in iichid, there's no RX underflow there (obviously), but it does not fix the lockup.
> I wonder if it might actually be iichid/imt doing something that causes the touchpad to do something wrong with the bus…
Really, I have never tested imt with touchpads. It still does not support button state reporting and clickpad detection. So any bugs are possible.
Comment 12 Greg V 2019-09-11 13:18:15 UTC
I think I found the culprit, it's the length retrieval in get_input_report. I changed it to just trust the given length — touchscreen works, touchpad doesn't receive evdev events yet (hmm) but doesn't crash the controller and I can see the reports changing with a debug print, and reload the module multiple times.

Seems like ig4 is innocent and it's the touchpad firmware reacting to iichid that's causing the bus to get screwed up. Closing the bug here.
Comment 13 Greg V 2019-09-11 14:21:48 UTC
(In reply to Greg V from comment #12)
err, actually, it's the thing where it writes the wInputRegister, which I don't think is supposed to happen.

// and it's currently reading mouse reports.. I'll try to fix this.
Comment 14 Vladimir Kondratyev freebsd_committer 2019-09-11 22:19:25 UTC
(In reply to Greg V from comment #12)
> Seems like ig4 is innocent
Not so innocent. You are at least 5-th man who has tried iichid and has bitten by SDA_HOLD issue. Can you check and post here the value of this register right after IG4_REG_RESETS_SKL is written. In my case it is 0x00000001 which is too low to allow normal I2C operation.
I think setting SDA_HOLD to 28 is a good candidate to be committed just now with the aim to be MFC-ed before 12.1 branch is forked.
Comment 15 Vladimir Kondratyev freebsd_committer 2019-09-11 22:35:47 UTC
(In reply to Greg V from comment #12)
> I think I found the culprit, it's the length retrieval in get_input_report. I changed it to just trust the given length — touchscreen works,
That sounds strange. Length retrieval breaks read pipelining, but I dont think it can result in such an errors.

> err, actually, it's the thing where it writes the wInputRegister, which I don't think is supposed to happen.
Write into wInputRegister can't result in any immediate errors as it is just a write into controller's FIFO.

// and it's currently reading mouse reports.. I'll try to fix this.
imt_set_input_mode() is responsible for converting mouse to touchpad
Comment 16 Greg V 2019-09-12 01:19:26 UTC
(In reply to Vladimir Kondratyev from comment #14)
I meant innocent in the lockup case, that was indeed the touchpad firmware blowing up from the write.

Yes, the reset changes the hold value to 1. Interesting how the other buses are fine with this, only the touchpad one was broken.
Comment 17 Vladimir Kondratyev freebsd_committer 2019-09-12 11:39:24 UTC
(In reply to Greg V from comment #16)
> Interesting how the other buses are fine with this, only the touchpad one was broken.
That is related to buses and gates capacitance and resistance. Here you can find what happens if SDA HOLD is set too low: 
https://www.cypress.com/file/364871/download
Comment 18 commit-hook freebsd_committer 2019-09-12 12:33:55 UTC
A commit references this bug:

Author: wulf
Date: Thu Sep 12 12:33:09 UTC 2019
New revision: 352243
URL: https://svnweb.freebsd.org/changeset/base/352243

Log:
  ig4(4): Fix SDA HOLD time set too low on Skylake controllers

  Execution of "Soft reset" command (IG4_REG_RESETS_SKL) at controller init
  stage sets SDA_HOLD register value to 0x0001 which is often too low for
  normal operation.

  Set SDA_HOLD back to 28 after reset to restore controller functionality.

  PR:		240339
  Reported by:	imp, GregV, et al.
  MFC after:	3 days

Changes:
  head/sys/dev/ichiic/ig4_iic.c
Comment 19 commit-hook freebsd_committer 2019-09-15 19:17:19 UTC
A commit references this bug:

Author: wulf
Date: Sun Sep 15 19:16:42 UTC 2019
New revision: 352362
URL: https://svnweb.freebsd.org/changeset/base/352362

Log:
  ig4(4): Fix SDA HOLD time set too low on Skylake controllers

  Execution of "Soft reset" command (IG4_REG_RESETS_SKL) at controller init
  stage sets SDA_HOLD register value to 0x0001 which is often too low for
  normal operation.

  Set SDA_HOLD back to 28 after reset to restore controller functionality.

  PR:		240339
  Reported by:	imp, GregV, et al.

Changes:
_U  stable/12/
  stable/12/sys/dev/ichiic/ig4_iic.c