Bug 240485

Summary: ig4: Add Cannon Lake LP and H I2C Controller IDs
Product: Base System Reporter: Neel Chauhan <nc>
Component: kernAssignee: 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: CURRENTFlags: koobs: mfc-stable12+
koobs: mfc-stable11-
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=245654
Attachments:
Description Flags
Patch (Revision 1)
none
Patch (Revision 2)
none
ig4 patchset
none
dmesg log of the HP Spectre x360 13-p0043dx wulf's iichid patch
none
ig4 patchset rev.2
none
Dell Latitude Dell Latitude 5500 running FBSD 13 with ig4 patchset 2: dmesg+pciconf+/boot/loader.conf none

Description Neel Chauhan freebsd_committer freebsd_triage 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 freebsd_triage 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 freebsd_committer freebsd_triage 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 freebsd_triage 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 freebsd_triage 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 freebsd_committer freebsd_triage 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 freebsd_committer freebsd_triage 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 freebsd_triage 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 freebsd_triage 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 freebsd_committer freebsd_triage 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 freebsd_triage 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 freebsd_triage 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 freebsd_triage 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
Comment 18 commit-hook freebsd_committer freebsd_triage 2019-12-22 00:47:20 UTC
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
Comment 19 Vladimir Kondratyev freebsd_committer freebsd_triage 2019-12-22 00:59:54 UTC
Committed, thank you!
Comment 20 Kubilay Kocak freebsd_committer freebsd_triage 2020-04-16 00:59:35 UTC
^Triage: Track merges to stable/*