Summary: | ig4: Add Cannon Lake LP and H I2C Controller IDs | ||
---|---|---|---|
Product: | Base System | Reporter: | Neel Chauhan <nc> |
Component: | kern | Assignee: | freebsd-bugs (Nobody) <bugs> |
Status: | Closed FIXED | ||
Severity: | Affects Some People | CC: | cem, driesm, grembo, nc, rowens, wulf |
Priority: | --- | Keywords: | feature, needs-qa, patch |
Version: | CURRENT | Flags: | koobs:
mfc-stable12+
koobs: mfc-stable11- |
Hardware: | Any | ||
OS: | Any | ||
See Also: | https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=245654 | ||
Attachments: |
Thank you for the report and patch Neel According to intel-lpss driver, Cannon Lake has a different IC clock rate as compared to Sky Lake, 216MHz vs 120MHz. That means new enum value should be assigned to it. Created attachment 207680 [details]
Patch (Revision 2)
Thanks for the feedback!
I have added an enum for Cannon Lake. However, it does the same exact thing as Skylake.
However, I do not see any mention of 120 MHz frequency for Skylake I2C on FreeBSD and couldn't include it here. If this code exists in FreeBSD, where is it?
(In reply to Neel Chauhan from comment #3) > If this code exists in FreeBSD, where is it? Not in vanilla sources yet. Clock registers in FreeBSD ig4 driver are still tuned to support Mathew Dillon's Haswell? chromebook only but they should be tuned for each controller model separately based on its IC clock rate. Thanks to separate clock line, I2C bus is not very sensitive to timing deviations and Haswell clock values still works sometimes. Only sometimes, so this part must be improved. Created attachment 207704 [details]
ig4 patchset
Now the patch looks good to me, so I included it in my ig4 patchset that I am going to put on phabricator in next few days to further review.
Could you test it, Neel? It requires very recent CURRENT to be applied cleanly but all rejects can be trivially fixed on old CURRENT or 12-STABLE.
(In reply to Vladimir Kondratyev from comment #5) Sorry for the delay, I was busy. Your ig4 patchset works well for me. Created attachment 207864 [details] dmesg log of the HP Spectre x360 13-p0043dx wulf's iichid patch (In reply to Neel Chauhan from comment #6) Never mind this comment, I forgot to reboot when I commented. The touchpad does not work with this patch. dmesg log is attached. (In reply to Neel Chauhan from comment #7) > The touchpad does not work with this patch. That is strange. I see no attempts to read HID descriptor by your I2C device in dmesg. Such attempts should look like: iichid0: HID command I2C_HID_CMD_DESCR at 0xXX Do you observe such a lines in dmesg? They precedes first communication with HID device through I2C so they do not depend on state of the ig4 driver. Created attachment 207905 [details]
ig4 patchset rev.2
Previous patch required patched iicbus driver
I fixed it and now it works with stock iicbus. Neel, could you try it again?
(In reply to Vladimir Kondratyev from comment #9) Good news: your new patch works! Thanks! (In reply to Neel Chauhan from comment #10) Would you be willing to add your current dmesg? I'm trying to get the touchpad working on a Dell Latitude 5500 (*not* E5500); I've applied the patchset rev.2 (attachment 207905 [details]) over 13 CURRENT r352911, and I'm trying to figure out what's missing. Vladimir, I think the ig4 patchset should include newer Intel platforms as well: * Ice Lake: https://github.com/torvalds/linux/commit/a13c93b3a5db87a173b2cdc6c2f2122d9d677808#diff-0cb73ca5571efd62ddd5a5fc79d3af04 * Cannon Lake: https://github.com/torvalds/linux/commit/dd6629073a97e5ee125eacbd22eea62281891c67#diff-0cb73ca5571efd62ddd5a5fc79d3af04 * Elkhart Lake: https://github.com/torvalds/linux/commit/01e4ecee03aa81ec3565d70c80cd1282088fc5a6#diff-0cb73ca5571efd62ddd5a5fc79d3af04 * Tiger Lake: https://github.com/torvalds/linux/commit/ec65b56046d27a21a5ae02eb7fcb321e1942a541#diff-0cb73ca5571efd62ddd5a5fc79d3af04 Keep in mind that I lack the hardware to test these newer platforms, some of these haven't even been released. Created attachment 208154 [details]
Dell Latitude Dell Latitude 5500 running FBSD 13 with ig4 patchset 2: dmesg+pciconf+/boot/loader.conf
I'm adding my dmesg and pciconf output in hopes they're useful. Touchpad is not currently functional on this computer with the ig4 patchset v. 2
(In reply to Rick Owens from comment #13) > Touchpad is not currently functional on this computer with the ig4 patchset v. 2 It is not a driver for touchpad but for I2C controller. You can find 2 different I2C touchpad drivers (with follow-up discussion) here: https://reviews.freebsd.org/D16698 and try them Anyway, thanks for testing! (In reply to Neel Chauhan from comment #12) > some of these haven't even been released. I am not sure if we should include support for non-released platforms. But adding support of released ones is a good idea. Thanks! (In reply to Vladimir Kondratyev from comment #14) Thank you, I'll check it out! A commit references this bug: Author: wulf Date: Sun Nov 3 21:16:07 UTC 2019 New revision: 354320 URL: https://svnweb.freebsd.org/changeset/base/354320 Log: [ig4] Add support for CannonLake controllers They are clocked at 216MHz rate, much higher than previous models. PR: 240485 Submitted by: Neel Chauhan <neel@neelc.org> Changes: head/sys/dev/ichiic/ig4_iic.c head/sys/dev/ichiic/ig4_pci.c head/sys/dev/ichiic/ig4_reg.h head/sys/dev/ichiic/ig4_var.h A commit references this bug: Author: wulf Date: Sun Dec 22 00:46:08 UTC 2019 New revision: 355994 URL: https://svnweb.freebsd.org/changeset/base/355994 Log: MFC r354291 - r354322, r354327, r355596 r354291: [ig4] Give common name to PCI and ACPI device drivers r354292: [ig4] Handle controller startup errors Obtained from: DragonflyBSD (509820b) r354293: [ig4] Only enable interrupts when we want them. Otherwise keep mask at 0. Obtained from: DragonflyBSD (d7c8555) r354294: [ig4] Drop driver's internal RX FIFO r354295: [ig4] Do not wait for interrupts in set_controller() routine r354296: [ig4] Reduce scope of io_lock r354297: [ig4] Ignore stray interrupts r354298: [ig4] We actually need to set the Rx threshold register one smaller. Obtained from: DragonflyBSD (02f0bf2) r354299: [ig4] Stop I2C controller after checking that it's kind of functional. Obtained from: DragonfliBSD (0b3eedb) r354300: [ig4] disable controller before initialization of clock counters r354301: [ig4] Add support for polled mode r354302: [ig4] Allow enabling of polled mode from iicbus allocation callback r354303: [ig4] Do not wait until interrupts are enabled at attach stage r354304: [cyapa] Postpone start of the polling thread until sleep is available r354305: [ig4] dump IG4_REG_COMP_PARAM1 and IG4_REG_COMP_VER registers unconditionally r354306: [ig4] Set clock registers based on controller model r354307: [ig4] Implement burst mode for data reads r354308: [ig4] Add suspend/resume support PR: 238037 r354309: [ig4] Remove dead code inherited from DragonflyBSD r354310: [ig4] Rewrite ig4iic_write routine to use TX_EMPTY status flag r354311: [ig4] Convert last remaining usage of TX_NOTFULL status to TX_EMPTY r354312: [ig4] Use interrupts for waiting for empty TX FIFO r354313: [ig4] Convert polling loop from status-based to interrupt-based r354314: [ig4] Improve error detection r354315: [ig4] Set STOP condition and flush TX/RX FIFOs on error r354316: [ig4] On SkyLake controllers issue reset on attach unconditionally. r354317: [ig4] wait for bus stop condition after stop command issued r354318: [ig4] Minor improvement of write pipelining r354319: [ig4] Add generic resource methods to bus interface r354320: [ig4] Add support for CannonLake controllers PR: 240485 Submitted by: Neel Chauhan <neel@neelc.org> r354321: [ig4] Enable additional registers support on Appolo Lake controllers r354322: [ig4] Convert ithread interrupt handler to filter based one. r354327: [ig4] Try to workaround MIPS namespace pollution issue r355596: [ig4] Remove unused methods from bus interface Suggested by: jhb Reviewed by: imp (previous version) Differential Revision: https://reviews.freebsd.org/D22016 Changes: _U stable/12/ stable/12/sys/dev/chromebook_platform/chromebook_platform.c stable/12/sys/dev/cyapa/cyapa.c stable/12/sys/dev/ichiic/ig4_acpi.c stable/12/sys/dev/ichiic/ig4_iic.c stable/12/sys/dev/ichiic/ig4_pci.c stable/12/sys/dev/ichiic/ig4_reg.h stable/12/sys/dev/ichiic/ig4_var.h stable/12/sys/dev/iicbus/iicbus.c Committed, thank you! ^Triage: Track merges to stable/* |
Created attachment 207365 [details] Patch (Revision 1) Add PCI IDs for I2C controller for Intel Cannon Lake PCH models. Cannon Lake is used on systems based on platforms like Intel Coffee Lake and Whiskey Lake. Inspired by Linux commit b418bbff36dd25dc49ed6abbb1e1deedd0d7cff5 Tested on the HP Spectre x360 13-ap0043dx running 13-CURRENT.