Fix VoodooI2C intermittency issue with I2C polling

[Qonfused]

Using VoodooI2C, I2C devices can stop responding intermittently until sleep (even when polling mode is forced).

Regarding interface devices like the trackpad and touchscreen displays, this also appears to be an issue affecting I2C devices with GPIO interrupts (see VoodooI2C/VoodooI2C#435). It also appears to happen with the same controllers (i.e. same pad group) on another comet lake laptop in that issue: VoodooI2C/VoodooI2C#435 (comment).

GPIO pinning may also be silently failing, falling back to I2C polling. They can still report the expected GPIO pin and report GPIO mode when instead using I2C polling. Should have been resolved as per VoodooI2C/VoodooI2C#487 but may be worth checking for regression.

Update: GPIO pinning is failing for all 3 input devices and falling back to polling mode.
voodooI2C.log

[Qonfused]

VoodooI2C should only check the return values for the _CRS and _DSM methods, which made investigating this issue quite strange. The declarations for SSCN and FMCN values are present at the root level for each I2C controller, which controls bus timings for each. These declarations check if a USTP variable is defined; manually overriding the value to equal one should force these declarations to always be applied. This may fix the 'slave address not acknowledged messages' issue observed before (cf. VoodooI2C/VoodooI2C#171) as it fixes bus speeds that may cause the I2C controller to eventually stop responding to a slave address when overloaded with incoming messages.

I've included this fix in 6f50e2b. It's not clear what behavior this may cause in windows, so it's possible for this to create a regression if enabled on windows. For now, the override will only be applied for macOS.

---

Update: This does not appear to resolve this issue, though this may fix some behavior since the default bus speed values are taken from Broadwell and aren't generally compatible with CFL/CML.

The same eventual intermittency issue (devices eventually disabling themselves) persists, though this can result in neither touch screen working. I had figured that other I2C controllers may be interfering, though the only other controller in IOReg would be I2C@2 (pci8086,2ea), which doesn't have any devices attached to it.

Reverting this change in development@4f2947d in favor of a more direct override of USTP.

[Qonfused]

Replacing GPIO's _STA method to return 0x0F might be a better fix than overriding the GPHD value. With this change, the _HID value should by default fall back to PNP0C02; the other _HID values match specific controllers (INT3450 and INT34BB). As noted before, INT34BB is used in windows for GPIO (i.e. 'Intel (R) Serial IO GPIO Host Controller - INT34BB').

I don't know what behavior this changes, as VoodooI2C/VoodooI2CHID doesn't appear to depend on a specific _HID value, although a lot of reference working OEM GPIO implementations fall back to this _HID value; ASUS commonly has buggy GPIO that is setup like this.

Update: This does not appear to have an effect on GPIO behavior in macOS or with VoodooGPIO.

[Qonfused]

Voodoo seems unable to retrieve anything from _DSM/XDSM methods, which is problematic. Noticed that when checking the trackpad's _DSM method, VoodooI2CHID emits this error message:
VoodooI2CHIDDevice::ELAN1207 HID descriptor address invalid

This is invoked by this function:

VoodooI2CHIDDevice::getHIDDescriptorAddress()

// VoodooI2CHID/VoodooI2CHIDDevice.cpp#L100-120
100 | IOReturn VoodooI2CHIDDevice::getHIDDescriptorAddress() {
101 |     IOReturn ret;
102 |     OSObject *obj = nullptr;
103 | 
104 |     ret = api->evaluateDSM(I2C_DSM_HIDG, HIDG_DESC_INDEX, &obj);
105 |     if (ret == kIOReturnSuccess) {
106 |         OSNumber *number = OSDynamicCast(OSNumber, obj);
107 |         if (number != nullptr) {
108 |             hid_descriptor_register = number->unsigned16BitValue();
109 |             setProperty("HIDDescriptorAddress", hid_descriptor_register, 16);
110 |         } else {
111 |             IOLog("%s::%s HID descriptor address invalid\n", getName(), name);
112 |             ret = kIOReturnInvalid;
113 |         }
114 |     } else {
115 |         IOLog("%s::%s unable to parse HID descriptor address\n", getName(), name);
116 |         ret = kIOReturnNotFound;
117 |     }
118 |     if (obj) obj->release();
119 |     return ret;
120 | }

These constants used in the evaluateDSM() call:

// VoodooI2C/VoodooI2CDevice/VoodooI2CDeviceNub.hpp#L24
24 | #define I2C_DSM_HIDG "3cdff6f7-4267-4555-ad05-b30a3d8938de"
...
// VoodooI2C/VoodooI2CDevice/VoodooI2CDeviceNub.hpp#L28
28 | #define HIDG_DESC_INDEX 1

This evaluates the _DSM method, with Arg0 as I2C_DSM_HIDG, Arg1 as One, and Arg2 as HIDG_DESC_INDEX:

ETPD _DSM

59886 | Method (_DSM, 4, NotSerialized)
59887 | {
59888 |     If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de")))
59889 |     {
59890 |         If ((Arg2 == Zero))
59891 |         {
59892 |             If ((Arg1 == One))
59893 |             {
59894 |                 Return (Buffer (One)
59895 |                 {
59896 |                       0x03
59897 |                 })
59898 |             }
59899 |             Else
59900 |             {
59901 |                 Return (Buffer (One)
59902 |                  {
59903 |                      0x00
59904 |                  })
59905 |              }
59906 |         }
59907 | 
59908 |         If ((Arg2 == One))
59909 |         {
59910 |              Return (One) /* Returns here */
59911 |         }
59912 |     }
59913 |     Else
59914 |     {
59915 |         Return (Buffer (One)
59916 |         {
59917 |               0x00
59918 |         })
59919 |     }
59920 | }

This is a fallback when VoodooI2C can't retrieve APIC or GPIO from the _CRS method (VoodooI2C/VoodooI2C#365). This doesn't appear to be a problem at current but it is relevant for #22, as this is also the case for the _DSM methods of the touchscreen inputs (TPL1, TPL0).

[Qonfused]

GPIO pinning doesn't look to be a viable workaround here. Using 0x6B or 0x41 for the trackpad's GPIO pin are reported to successfully register with VoodooGPIOCannonLakeLP, but results in no response from the trackpad. I also noticed that using 0x17 or 0x34 (unchained GPIO pins) successfully registers with VoodooGPIOCannonLakeLP, but common pins like 0x1B and 0x55 failed to register. No case resulted in a response from the trackpad (unless falling back to polling mode upon error), though it did however provide the following observations:

• (trackpad GPIO):
  • hardware pin 0x51 is mapped to GPIO IRQ 0x6D
  • hardware pin 0x36 is mapped to GPIO IRQ 0x41

I used the below table to calculate the GPIO IRQ values (as well as the CannonLake table from CoreBoot, mentioned in #19 (comment)):

GPIO Calculation Table - (Intel 8 Gen) CoffeeLake-LF and Whiskylake CPUs

APICPIN Range	GPIO1	GPIO2
47 < APICPIN <= 71	= APICPIN - 16	= APICPIN + 80
71 < APICPIN <= 95	= APICPIN + 184	= APICPIN + 88
95 < APICPIN <= 119	= APICPIN
108 < APICPIN <=115		= APICPIN - 44

The GPIO device is correctly enabled, and the trackpad is correctly enabled and configured for GPIO pinning. It seems that these pins (including the common pins I mentioned earlier) are locked somehow. There doesn't appear to be a mechanism to reset pin ownership if this is can be resolved in macOS, and neither does there appear to be a legacy GPIO setting in BIOS.

I found a comment from gvtk that seems to encounter the same issue regarding firmware support for legacy GPIO (for the UX582LV):

After much experimenting with recompiled Linux kernel, it appears that we are out of luck with the BIOS/firmware on our machines.

The correct Interrupt to get hardware events is 0x5f. This works on both Windows and Linux OOB as they use their IOAPIC handlers. It corresponds to GPIO pin 74. But the firmware appears to use direct IRQ bypassing the legacy GPIO on these newer machines.

The fact that is pin is locked is not a problem because it is set to operate in GPIO mode and so being locked becomes a non-issue. Because the owner is set as APIC, both Voodoo and Linux pin control refuse to use pin 74 for IRQ. But this check can be bypassed in the code which I did. There have been some machines where bypassing this has enabled GPIO interrupts (for example some Razor Blade and even some Asus laptops). But those had the GPIO and APIC interrupts chained and therefore the interrupt status bit for GPIO was set. I suspect in sqlsec‘s laptop which is a different model line, ownership in that BIOS was not set and GPIO was chained and hence worked OOB with the correct pin specifier. No such luck in our case.

I doubt ASUS will ever update the software to enable legacy GPIO interrupts for these touch devices and this might be a problem for a lot of newer laptops if they use interrupts beyond the MacOS limit.

[Qonfused]

Moving on to addressing this issue under I2C polling, I also noticed that each of the 'slave address not acknowledged' errors do mention two specific I2C controllers (unique to each touch input):

• pci8086,2e8: primary touchscreen
• pci8086,2eb: screenpad touchscreen

^^ These however use the same serial address SADR value of 0x10/0x0010:

TPL1: primary touchscreen

// \_SB.PCI0.I2C0.TPL1
60151 | Method (_CRS, 0, NotSerialized)
60152 | {
60153 |     Name (SBFI, ResourceTemplate ()
60154 |     {
60155 |         I2cSerialBusV2 (0x0010, ControllerInitiated, 0x00061A80,
60156 |             AddressingMode7Bit, "\\_SB.PCI0.I2C0",
60157 |             0x00, ResourceConsumer, _Y39, Exclusive,
60158 |             )
60159 |         Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, )
60160 |         {
60161 |             0x00000072,
60162 |         }
60163 |     })
60164 |     CreateWordField (SBFI, \_SB.PCI0.I2C0.TPL1._CRS._Y39._ADR, ADR1)
60165 |     ADR1 = DerefOf (SADR [MTLI])
60166 |     Return (SBFI) /* \_SB_.PCI0.I2C0.TPL1._CRS.SBFI */
60167 | }

TPL0: screenpad touchscreen

// \_SB.PCI0.I2C3.TPL0
60061 | Method (_CRS, 0, NotSerialized)
60062 | {
60063 |     Name (SBFI, ResourceTemplate ()
60064 |     {
60065 |         I2cSerialBusV2 (0x0010, ControllerInitiated, 0x00061A80,
60066 |             AddressingMode7Bit, "\\_SB.PCI0.I2C3",
60067 |             0x00, ResourceConsumer, _Y38, Exclusive,
60068 |             )
60069 |         Interrupt (ResourceConsumer, Level, ActiveLow, Exclusive, ,, )
60070 |         {
60071 |             0x00000042,
60072 |         }
60073 |     })
60074 |     CreateWordField (SBFI, \_SB.PCI0.I2C3.TPL0._CRS._Y38._ADR, ADR1)
60075 |     ADR1 = DerefOf (SADR [TPLI])
60076 |     Return (SBFI) /* \_SB_.PCI0.I2C3.TPL0._CRS.SBFI */
60077 | }

Explainer for I2cSerialBusV2() args

(page 909): https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf

An interesting quirk of this issue is that it usually happens to one of the touch inputs initially but inevitably disables both, which happens shortly after the device is initially reset by VoodooI2C upon loading the driver. There's no particular significance to that latter point beyond being a helpful marker for debugging, though these devices do properly function again after going through the same init sequence after waking from sleep.

This issue may be addressable by adding an _INI method that refactors this assignment in _CRS using a different address for each device during initialization.

[Qonfused]

I've done some pre-emptive work to address the duplicate SADR values for TPL1/TPL0 by fixing duplicate _UID values in d39b6f5. This detail is likely cosmetic (and will probably be reverted), but it is at least setup for testing different addresses in case they're already too crowded (on the same controller).

To investigate a possible I2C/GPIO resource conflict, I've compiled a list of occupied serial addresses in DSDT order:

ACPI Path	HID	SADR
_SB.PCI0.I2C0.PA01	MAX34407	0x0010
_SB.PCI0.I2C0.IICB	?	0x0000
_SB.PCI0.I2C2.CAM0	INT3471	0x0010 0x000E 0x0050 0x0051 0x0052 0x0053
_SB.PCI0.I2C4.CAM1	INT3474	0x0036
_SB.PCI0.I2C2.PMIC	INT346F	0x004C
_SB.PCI0.I2C1.ETPD	ELAN1207	0x0015
_SB.PCI0.I2C3.TPL0	ELAN9009	0x0010
_SB.PCI0.I2C0.TPL1	ELAN9008	0x0010

Important notes:

• The PA01 device (MAX34407) is a current/voltage monitor for calculating I2C/SMBus power consumption.
• CAM0/CAM1 appear to be IR camera sensors, with PMIC (INT346F) being a dedicated power management IC for them.

[Qonfused]

... No case resulted in a response from the trackpad (unless falling back to polling mode upon error), though it did however provide the following observations:

• (trackpad GPIO):
  • hardware pin 0x51 is mapped to GPIO IRQ 0x6D
  • hardware pin 0x36 is mapped to GPIO IRQ 0x41

After revisiting the GPIO pinning guide, there is more to this; there is a mismatch between the hardware pin and GPIO pin number (which is expected). This can be corrected based on values from this table.

E.g. in the case of the trackpad:

CNL_GPP(/* GPP_D */    // Match based on pad group (e.g. GPP_D13 is group D)
    0,  // num
    68, // base        // then subtract the base from the GPIO pin
    92, // end
    96, // gpio_base   // now add the gpio_base to this value:
),                     // (81) => 81 -68 +96 = 109 (0x6d)

Now for the trackpad + touchscreen inputs:

APIC Pin	GPIO IRQ	(Hardware) GPIO Pin	GPIO Pin
0x6d (ETPD)	GPP_D13_IRQ	81 (GPP_D13)	0x6d ((81) -68 +96 = 109)
0x72 (TPL1)	GPP_D18_IRQ	86 (GPP_D18)	0x72 ((86) -68 +96 = 114)
0x42 (TPL0)	GPP_B18_IRQ	43 (GPP_B18)	0x32 ((43) -25 +32 = 50 )

This doesn't resolve the GPIO pinning issue mentioned in the previous comment, but these would be the correct pins.

[Qonfused]

Although the device schematic may suggest different interrupts are being used (GPP_XYY):

[shiecldk]

In VoodooI2C/VoodooI2C#321 (comment), @jjsmith92 addressed the fix for Linux for the issue of TrackPad stopping working after a period of use; however, the file he uploaded was expired. Someone needs to re-porting the Linux fix to VoodooI2C.

[Qonfused]

I believe these are all the related patches that he must have implemented: https://bugzilla.kernel.org/show_bug.cgi?id=200663

--- (comment #16) ---
For the ASUS GL703GE Model it seems that you need to manually add the lines 1016, 1021 and 1022 from https://bugzilla.kernel.org/attachment.cgi?id=277507&action=diff to /drivers/pinctrl/intel/pinctrl-intel.c from kernel 4.19-rc2

--- (comment #29) ---
...
Andrey, the only 'valuable' change I had in my 'for-vivien' branch compared to upstream was bentiss/hid-multitouch@878681f This added a magic sequence found by reverse engineering, so it might or not help. It is still a mystery about what this is however. The rest of the driver should be upstream. (Note: next time clone my repo to keep the history, it will solve the issue of not knowing what is in your repo).

--- (comment #58) ---
Here is a correct patch for the issue:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/intel/pinctrl-cannonlake.c?h=v4.20&id=e50d95e2ad1266f8d3fcdf0724f03dbdffd400aa

--- (comment #73) ---
This bug has been fixed by this commit torvalds/linux@6cb0880

And now the touchpad works even after resuming from suspend on my ASUS FX504GE, no more disconnection like described in earlier kernels. Thanks to everyone for working on this. For anyone's reference I am running Ubuntu 19.10 with kernel version :5.3.0-999-generic(Ubuntu mainline kernel with a daily update cycle from upstream)

--- (comment #77) ---
(In reply to Ryan Reich from comment #74)
...
Can you test this patch: https://lore.kernel.org/linux-gpio/CAHp75VekvqHX_eUm88RQJQiU59hUoxUY=pP4MWsp6xn3os9bPg@mail.gmail.com/T/#t ?

[shiecldk]

@Qonfused Would it be possible to port the numpad driver from Linux?

[Qonfused]

It would need to be refactored, though all it does is convert x/y positions on the trackpad to the corresponding digits; you could do this in linux/macOS without polling the I2C driver either.

There may be a way through some sort of VoodooI2CHID interface class (e.g. multitouch) that lets you get the finger position on the trackpad. I'm not too familiar with how VoodooI2C's interface drivers/etc work on these trackpads but I imagine that'd be the best way to do so.

[shiecldk]

VoodooI2C/VoodooI2C#321 (comment)

Can you report this back to that VoodooI2C thread? Thanks!

[Qonfused]

Still unresolved for the ELAN1200 trackpad. That issue is a better fit for tracking in VoodooI2C/VoodooI2C#321 or in a dedicated issue.

I found a workaround for the touchscreen displays on the UX481 using APIC interrupts instead. Unfortunately, the UX481 isn't as lucky to use APIC interrupts below 0x2F out of the box, though the same APIC interrupts used on the UX582 model are available (0x1B and 0x1C).