Bug 240485 - ig4: Add Cannon Lake LP and H I2C Controller IDs
Summary: ig4: Add Cannon Lake LP and H I2C Controller IDs
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-bugs mailing list
URL:
Keywords: feature, needs-qa, patch
Depends on:
Blocks:
 
Reported: 2019-09-11 01:43 UTC by Neel Chauhan
Modified: 2019-11-03 21:16 UTC (History)
5 users (show)

See Also:
koobs: mfc-stable11?
koobs: mfc-stable12?


Attachments
Patch (Revision 1) (2.27 KB, patch)
2019-09-11 01:43 UTC, Neel Chauhan
no flags Details | Diff
Patch (Revision 2) (5.02 KB, patch)
2019-09-21 14:37 UTC, Neel Chauhan
no flags Details | Diff
ig4 patchset (44.80 KB, patch)
2019-09-21 23:03 UTC, Vladimir Kondratyev
no flags Details | Diff
dmesg log of the HP Spectre x360 13-p0043dx wulf's iichid patch (49.92 KB, text/plain)
2019-09-26 23:57 UTC, Neel Chauhan
no flags Details
ig4 patchset rev.2 (47.55 KB, patch)
2019-09-28 08:10 UTC, Vladimir Kondratyev
no flags Details | Diff
Dell Latitude Dell Latitude 5500 running FBSD 13 with ig4 patchset 2: dmesg+pciconf+/boot/loader.conf (120.51 KB, text/plain)
2019-10-07 15:53 UTC, Rick Owens
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Neel Chauhan 2019-09-11 01:43:09 UTC
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.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-09-11 04:04:27 UTC
Thank you for the report and patch Neel
Comment 2 Vladimir Kondratyev freebsd_committer 2019-09-21 10:33:44 UTC
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.
Comment 3 Neel Chauhan 2019-09-21 14:37:44 UTC
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?
Comment 4 Vladimir Kondratyev freebsd_committer 2019-09-21 22:35:27 UTC
(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.
Comment 5 Vladimir Kondratyev freebsd_committer 2019-09-21 23:03:06 UTC
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.
Comment 6 Neel Chauhan 2019-09-26 22:13:02 UTC
(In reply to Vladimir Kondratyev from comment #5)

Sorry for the delay, I was busy.

Your ig4 patchset works well for me.
Comment 7 Neel Chauhan 2019-09-26 23:57:29 UTC
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.
Comment 8 Vladimir Kondratyev freebsd_committer 2019-09-27 21:51:07 UTC
(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.
Comment 9 Vladimir Kondratyev freebsd_committer 2019-09-28 08:10:25 UTC
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?
Comment 10 Neel Chauhan 2019-09-29 14:28:19 UTC
(In reply to Vladimir Kondratyev from comment #9)
Good news: your new patch works!

Thanks!
Comment 11 Rick Owens 2019-09-30 19:47:42 UTC
(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.
Comment 13 Rick Owens 2019-10-07 15:53:08 UTC
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
Comment 14 Vladimir Kondratyev freebsd_committer 2019-10-07 19:32:38 UTC
(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!
Comment 15 Vladimir Kondratyev freebsd_committer 2019-10-07 19:59:20 UTC
(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!
Comment 16 Rick Owens 2019-10-07 23:03:41 UTC
(In reply to Vladimir Kondratyev from comment #14)
Thank you, I'll check it out!
Comment 17 commit-hook freebsd_committer 2019-11-03 21:16:49 UTC
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