applespi: Document ACPI methods

[l1k]

Regarding power consumption, you might be looking in the wrong place if you're searching for the interrupt in /proc/interrupts. The DSDT shows that GPE 0x1C sends a device notification, so you need to look at /sys/firmware/acpi/interrupts/gpe1C and see under which conditions it increases. Is the GPE only used for wake from system sleep or is it used for power management? If the latter, perhaps you need to stop polling after a period of inactivity and power down the device with SIEN(0), then wait for the device to send a notification and power it up with SIEN(1). You can call SIEN using acpi_execute_simple_method(), see l1k/linux@65f56e6 for an example. To make use of the GPE, call acpi_install_notify_handler() and acpi_enable_gpe(), see drivers/platform/x86/apple-gmux.c for an example.

As to patching the DSDT, I think this should be avoided if possible. I believe the macOS driver retrieves the required configuration data from the I/O Kit registry, which in turn is filled with values from the _DSM in ACPI. I've recently implemented retrieval of Apple device properties from EFI, see https://github.com/l1k/linux/commits/apple_properties_v1 . Could you test that branch on your MacBook8,1, boot with "dump_apple_properties" and attach the dmesg output to bugzilla? Perhaps the SPI EFI driver sends some device properties that might be of use.

If it doesn't, we could make the data in the _DSM available as device properties as well so that they can be accessed using the API in <linux/property.h> (see l1k/linux@8743c01 for an example). That would require some changes to the ACPI subsystem to fetch the properties from Apple's nonstandard _DSM instead of the standard _DSD. Would the properties in the _DSM enable you to avoid patching the DSDT?

[l1k]

Forgot to mention: The iFixit teardown names 3 chips on the touchpad logic board: BCM5976, 32F103 and LT3954 (see https://www.ifixit.com/Teardown/Retina+Macbook+2015+Teardown/39841#s89866).

Assuming that the BCM5976 is only used for the touchpad and the LT3954 is only used for keyboard backlight, that would leave the 32F103 as the controller you're talking to. According to the spec, the 32F103 supports USB, CAN and SDIO (see http://www.st.com/content/st_com/en/products/microcontrollers/stm32-32-bit-arm-cortex-mcus/stm32f1-series.html?sc=stm32f1&querycriteria=productId=SS1031).

No mention of SPI, but SDIO can be used in an SPI mode (see https://www.sdcard.org/developers/overview/sdio/sdio_spec/Simplified_SDIO_Card_Spec.pdf).

I'm wondering if some of the protocol oddities you're seeing can simply be explained by the SPI-over-SDIO mode? Perhaps looking at the SDIO spec might ease reverse engineering the protocol.

[cb22]

@l1k, thanks for the info! It turned out to be exactly that; in my case on the Macbook9,1 the GPE is 0x17. It seems like the GPE is triggered whenever there is data to be read, so it functions more like a normal interrupt. It only works when SIST = 1; if you disable the SPI interface no GPEs come through.

What really threw me off were the entries in the DSDT describing IRQ 14, as well as Windows showing IRQ 14 in use by the device manager. I wonder if this is just a complete red herring, or if it is actually used for something?

I've hacked together some code that uses acpi_install_notify_handler and it works! Probably a few bugs still, but it's now fully interrupt driven which is great. One thing I struggled with is clearing the GPE; in my notify handler, I schedule_work and then call acpi_set_gpe(NULL, 23, ACPI_GPE_DISABLE);. The corresponding function that gets called by the workqueue then does a acpi_set_gpe(NULL, 23, ACPI_GPE_ENABLE); I'll try and push that here soon.

Regarding the DSDT, I'll have a shot at looking at those values. My driver doesn't really need much, and most of it is hardcoded for now. I'm quite new to kernel development, so patching the DSDT seemed like the easiest way to get up and running.

Great insight with the names; I've been wondering what they do. Just a note with ISOL, I find the opposite behavior to your comment (setting to 1 gives all 00s, setting to 0 returns to normal data). There's also a CLRB, that gets called by the Windows driver, but I can't figure out what effect it has.

[cb22]

I'm not too sure that we're looking at SPI over SDIO; the 32F103 definitely has a few SPI controllers (might be listed as USARTs).

I had a quick glance at the spec and it doesn't seem to match up, for example that document seems to mention always having a start bit of 0, but a sample of data from the device is "80 11 00 01 75 02 33 06 00"

[l1k]

Glad to hear it's working with the GPE.

It seems like the GPE is triggered whenever there is data to be read, so it functions more like a normal interrupt. It only works when SIST = 1; if you disable the SPI interface no GPEs come through.

Ah okay so the SPI interface is intended to stay enabled all the time.

What really threw me off were the entries in the DSDT describing IRQ 14, as well as Windows showing IRQ 14 in use by the device manager. I wonder if this is just a complete red herring, or if it is actually used for something?

Hm, on the MacBook8,1 it's IRQ 0x1E (decimal 30). If indeed there are no interrupts coming through, then one theory would be that Windows' SPI HID driver requires an Interrupt Resource Descriptor Macro, so they put a dummy entry in there to make it happy.

One thing I struggled with is clearing the GPE; in my notify handler, I schedule_work and then call acpi_set_gpe(NULL, 23, ACPI_GPE_DISABLE);. The corresponding function that gets called by the workqueue then does a acpi_set_gpe(NULL, 23, ACPI_GPE_ENABLE); I'll try and push that here soon.

Hm, perhaps it makes sense to handle the key strokes and mouse movements in the notify handler itself, without deferring to a workqueue? Or the notify handler could append the events to a list of outstanding events (e.g. protected by a spinlock) and schedule a work item which reacts on them? Then you wouldn't have to deal with disabling/enabling the GPE. I'm not familiar with the patterns used by the input subsystem, @jwrdegoede might be able to help.

Regarding the DSDT, I'll have a shot at looking at those values. My driver doesn't really need much, and most of it is hardcoded for now. I'm quite new to kernel development, so patching the DSDT seemed like the easiest way to get up and running.

They put the values used by the Windows driver in the UBUF ResourceTemplate, which gets returned on all non-Darwin OSes. Linux masquerades as Darwin so we do not have direct access to the information, hence the need for DSDT patching. On Darwin, the information in UBUF is apparently not needed, the driver instead looks at the information in the _DSM. We currently do not have access to that on Linux either but I could come up with a patch for that. However for that patch to get upstreamed there needs to be a user. Thus my question if the information in the _DSM would allow us to forgo DSDT patching. It would still be good if you could test the https://github.com/l1k/linux/commits/apple_properties_v1 branch, perhaps SPI configuration data is coming from EFI. But take your time, no stress.

Great insight with the names; I've been wondering what they do. Just a note with ISOL, I find the opposite behavior to your comment (setting to 1 gives all 00s, setting to 0 returns to normal data).

Ugh, then I mixed it up, please rectify that. You're right, I had looked at the disassembled macOS driver and now I remember that it inverted the bool value given to the function.

There's also a CLRB, that gets called by the Windows driver, but I can't figure out what effect it has.

I only have an ACPI dump of a MacBook8,1 (posted by Leif Liddy). If they added CLRB on the MacBook9,1, could you attach an ACPI dump to bugzilla so I can have a look?

I'm not too sure that we're looking at SPI over SDIO; the 32F103 definitely has a few SPI controllers (might be listed as USARTs).

Alright, it was just a theory. :)

As for the copy-pasted code from the bcm5974 driver, I'm wondering if your driver could instantiate a virtual bcm5974 usb device to which the bcm5974 driver binds, then just pass the mouse movements through? An alternative approach would be to split the bcm5974 driver.

[cb22]

Hm, on the MacBook8,1 it's IRQ 0x1E (decimal 30). If indeed there are no interrupts coming through, then one theory would be that Windows' SPI HID driver requires an Interrupt Resource Descriptor Macro, so they put a dummy entry in there to make it happy.

It does seem like that's a possibility. I had originally thought that the APIC wasn't configuring the interrupt correctly (see here), but I don't know how plausible that is.

Hm, perhaps it makes sense to handle the key strokes and mouse movements in the notify handler itself, without deferring to a workqueue? Or the notify handler could append the events to a list of outstanding events (e.g. protected by a spinlock) and schedule a work item which reacts on them? Then you wouldn't have to deal with disabling/enabling the GPE. I'm not familiar with the patterns used by the input subsystem, @jwrdegoede might be able to help.

Well, I have to do an SPI call to read from the device, and since we're in an interrupt context we can't sleep or use spi_sync (to the best of my understanding). I've actually modified it to call spi_async with a callback rather than delegating to a workqueue, but the original problem still remains.

It actually seems like calling acpi_set_gpe(NULL, 23, ACPI_GPE_DISABLE); has no effect whatsover. Which is odd, since a patch relating to acpi_set_gpe here explicitly mentions temporarily disabling the IRQ and reenabling it later to prevent IRQ storms.

I only have an ACPI dump of a MacBook8,1 (posted by Leif Liddy). If they added CLRB on the MacBook9,1, could you attach an ACPI dump to bugzilla so I can have a look?

I've attached a full DSDT dump to bugzilla.

[l1k]

Just some quick review comments:

The pr_info() for error messages should be pr_err() in applespi_probe().

The GPE number is different for the MB8,1, so instead of hardcoding 23, you need to call acpi_evaluate_integer(applespi->handle, "_GPE", NULL, &applespi->gpe), like the apple-gmux driver does.

If acpi_install_notify_handler() and acpi_enable_gpe() fail, the function should return since the driver needs the GPE to work properly AFAIUI.

[l1k]

Oh and I think the applespi_init() needs to be moved before the acpi_install_notify_handler() because the handler might be called as soon as it's installed.

[cb22]

Ah yes, I'll sort those issues out. I've had some luck; using acpi_install_gpe_handler instead of acpi_install_notify_handler. Now, the notify handler can just return ACPI_INTERRUPT_HANDLED and the SPI async completion handler calls acpi_finish_gpe(NULL, 23) which reenables the GPE.

Seems to work pretty well, and there are no longer any issues with GPE storms. (well, occasionally the device gets into a weird state after a few driver unloads / reloads, where it keeps the GPE asserted and sends a different packet to the usual one where there is no data).

[l1k]

Hm, perhaps these issues go away if you use ISOL and/or SIEN on driver load/unload?

I'd just declare the gpe member in struct applespi_data as unsigned long long and pass that member directly to acpi_evaluate_integer(). The reason apple-gmux uses an int there is because it tries to limp on if it can't find a GPE, and this is remembered by storing -1 in gpe. However since your driver needs the gpe to function and bails out if it can't find it, you don't need the ability to store negative values.

I've looked at the DSDT, unfortunately they've obfuscated things considerably on your machine and it's no longer possible to see directly which GPIO pin is modified. CLRB writes the 32 bit value 0x00800000 to memory address 0x0100 + _((_0x00AF0000) + *(0x8AF9EC98 + 104)). It would require access to such a machine and some fiddling to find out what this means, an easier method to determine the meaning of CLRB is probably to look where the Apple driver calls that. Basically grep -rl CLRB /System/Library/Extensions, then open that in a disassembler. On my older OS X release, CLRB isn't called by any driver.

Did you create the DSDT dump after modifying the DSDT? Because it does return UBUF even in the OSDW case, suggesting that patching the DSDT isn't necessary on your machine.

[cb22]

Hm, perhaps these issues go away if you use ISOL and/or SIEN on driver load/unload?

I'll give that a shot, thanks!

I'd just declare the gpe member in struct applespi_data as unsigned long long and pass that member directly to acpi_evaluate_integer(). The reason apple-gmux uses an int there is because it tries to limp on if it can't find a GPE, and this is remembered by storing -1 in gpe. However since your driver needs the gpe to function and bails out if it can't find it, you don't need the ability to store negative values.

Ah, OK. I've changed that. One reason I did it being that I was too lazy to look up the printk format code for unsigned long long (which in retrospect being llu is pretty self evident) :)

I've looked at the DSDT, unfortunately they've obfuscated things considerably on your machine and it's no longer possible to see directly which GPIO pin is modified. CLRB writes the 32 bit value 0x00800000 to memory address 0x0100 + ((0x00AF0000) + *(0x8AF9EC98 + 104)). It would require access to such a machine and some fiddling to find out what this means, an easier method to determine the meaning of CLRB is probably to look where the Apple driver calls that. Basically grep -rl CLRB /System/Library/Extensions, then open that in a disassembler. On my older OS X release, CLRB isn't called by any driver.

I just did a quick grep on my OS X, and it doesn't seem to be used at all either. It seems like only the Windows driver uses it. When I was playing around with calling it, I didn't find it to have much of an effect on anything, so it's quite mysterious.

Did you create the DSDT dump after modifying the DSDT? Because it does return UBUF even in the OSDW case, suggesting that patching the DSDT isn't necessary on your machine.

D'oh, that's exactly what I did. My mistake - that's the only change in there. Now that I've got the basics of interrupts working fairly well, I'll have a look at running your apple_properties_v1 branch. It seems for some reason on Arch's 4.7 kernel the DSDT patch no longer works (the driver doesn't attach and APP000D isn't found). Downgrading back to 4.6.5 makes it work again.

[l1k]

I just did a quick grep on my OS X, and it doesn't seem to be used at all either. It seems like only the Windows driver uses it. When I was playing around with calling it, I didn't find it to have much of an effect on anything, so it's quite mysterious.

Oh in that case I'll download Bootcamp one of these days to take a look at the Windows driver. On Apple's support site I find Bootcamp 5.1.5769, seems to be the latest, or is there a newer one? I don't use Windows but hopefully I'll be able to unpack the installer to disassemble the driver.

Now that I've got the basics of interrupts working fairly well, I'll have a look at running your apple_properties_v1 branch. It seems for some reason on Arch's 4.7 kernel the DSDT patch no longer works (the driver doesn't attach and APP000D isn't found). Downgrading back to 4.6.5 makes it work again.

I believe 4.7 is able to patch the DSDT at runtime, or is that in 4.8? Can't remember, but I saw patches for this on the linux-acpi mailing list. As for the EFI properties, I totally forgot to mention, this only works if the kernel is booted with the efistub. So if you use rEFInd or gummiboot, it'll just work. grub with "linuxefi" directive should also work, but grub with "linux" directive will not work. (Because then grub calls ExitBootServices before starting the kernel, and retrieval of the EFI device properties requires boot services.)

[cgutman]

@cb22 Try adding 'acpi_osi=!Darwin' to your kernel command-line on 4.7. With that, it falls into the !OSDW() case in the DSDT so the APP000D device correctly enumerates.

I tried this on 4.7 (with !Darwin rather than the DSDT patch) and saw no GPEs being signaled on my MacBook9,1 and thus no functionality for either trackpad or keyboard. If I checkout aa14cda the polling method does result in working keyboard and trackpad.

I wonder whether !Darwin breaks something else that causes the GPEs to not fire or 4.7 has changes that caused it. !Darwin doesn't work correctly on 4.6 so it's hard to confirm.

[l1k]

@cgutman Why are you using that? IIUC the kernel will respond negatively to any OSI string with that option, so you also need to add an OSI string such as acpi_osi="Windows 2013". It's probably not reasonable to expect that the driver should work with no OSI string enabled.

[l1k]

Also, disabling the OSI string "Darwin" has unwanted side effects, e.g. on the MB8,1 the _PS0 and _PS3 methods to power USB down are OSDW-only (in SSDT5).

[cgutman]

If I'm reading [1] correctly, 'acpi_osi=!Darwin' should just remove Darwin from the list of strings that the kernel will respond to in an _OSI query. If I had specified, 'acpi_osi=!' or 'acpi_osi=', that would disable them all.

I'll do the DSDT patch and see if GPEs start working.

[1] https://www.kernel.org/doc/Documentation/kernel-parameters.txt

[l1k]

Apple's AML code is such that OSDW is only set if the OS responds "yay" to Darwin and "nay" to everything else, see mjg59's blog and torvalds/linux@7bc5a2bad0b. Thus, if you use acpi_osi=!Darwin, my expectation would be that the OS responds "nay" to all OSI strings.

The OSI code was rewritten for 4.7 by Lv Zheng and Chen Yu in collaboration with yours truly, see bugzilla and torvalds/linux@e10cfdc33a0f.