applespi: Re-enable SPI on resume #2
Conversation
|
Hi Cameron - thanks for the pull request! Quick question, what hardware / configuration have you been testing on? The reason I ask is that I've been struggling a bit with suspend / resume in general. I'm on the MacBook9,1 and booting off the internal NVMe drive. Suspending goes OK, but the laptop doesn't resume properly. After much digging, it turns out that the _PS0 method for the NVMe drive is returning Of course, if you're running from an external USB drive, this doesn't really apply; but perhaps there's a simple cmdline flag I'm missing. |
|
I've been running from a USB disk, so I haven't hit the NVMe resume issue. I'm on 4.8-rc5 where I did hit a deadlock on suspend in brcmfmac which someone already found and fixed here: https://patchwork.kernel.org/patch/9280681/ I still do see the message complaining about a device not transitioning from D3 to D0 on resume, so I don't think it's been fixed in mainline yet. Since I wasn't using the NVMe drive, I never noticed it dropping off the bus after resume. Edit: My machine is a MacBook9,1 |
|
@cgutman: You're calling SIEN on However on the MBP12,1, if the controller was switched to USB mode by EFI and the USB driver has already loaded, things will break because you're taking the interface away from it. You need to test presence of UIST, if it exists call it and if the result is 1, bail out. And only afterwards call SIEN. Please store the handle to the SIEN method in The 50 ms delay is quite long, is this really necessary? The macOS driver doesn't seem to delay at all AFAICS, but maybe I've missed something. With that addressed, |
|
@cb22: That's not the _PS0 method of the NVMe drive but of the root port it's attached to. It checks the Data Link Active bit in the PCIe Link Status register and bails out if the bit doesn't revert to 1. What does
|
|
The behaviour of _PS0 changes depending on a byte named NVME in the GNVS region. The byte should be at address 0x8AF9C918 + offset 0xB2 + 42 bytes = 0x8AF9C9F4 (decimal 2331625972). Which value do you get if you type: As a shot in the dark, EFI configures the machine in a special BootCamp mode for Windows unless the OS identifies as macOS using a proprietary EFI protocol before ExitBootServices is called. Perhaps the SSD is treated differently on suspend/resume depending on macOS vs. BootCamp mode. You could try one of the methods described at https://github.com/0xbb/gpu-switch#macbook-pro-113-and-115-notes to enable macOS mode and see if this fixes suspend/resume for the SSD. |
I did that, and got: Which makes sense to me - considering the
Gave that a shot, unfortunately no difference. I tried both with the EFI application method, and by patching the kernel - just to make sure it was indeed being called.
Ah, yes indeed. Here are the and after resume: |
|
This looks like the SSD isn't powered on after resume, PCIe link is down and fails to link-train. I'll have to do some research on this first, stand by. |
|
One odd thing in the lspci output is that the Link Capabilities Register of the Root Port lists support for ASPM L0s after resume but not before. But it seems more likely to me that the SSD wasn't powered up. The DSDT for the MacBook8,1 contains this for _PS3: And this for _PS0: GPIO pin 38 resets the SSD, GPIO pin 33 shuts down several power rails for suspend or brings them up for resume. On the MacBook9,1, the GPIO pins are apparently no longer mapped into SystemIO space but into SystemMemory space, hence it looks a little different there but likely does the same thing. _PS3: _PS0: Notably, if NVME is set to a different value than 1, these GPIO pins are not toggled. You could try this with: Perhaps Apple used this for debugging, I don't know. Please do not hold me liable if this damages your hardware or erases your data. (It's probably safe.) Power consumption may of course be higher during sleep if the SSD wasn't powered down. Are you sure that _PS0 is executed at all after resume? You may want to add some debug messages to |
Hmm, so that's strange, toggling that seems to have no effect.
I thought it might be something strange in the PCI resume code, so I've also tested by just using So Also interesting, calling I'll add some debug stores to the DSDT and see if I can figure out what's going on |
|
Another idea would be to look at the macOS and Windows drivers, perhaps they do something special to resume the SSD. The DSDT sets device properties for the SSD called If you |
cgutman
force-pushed
the
workingresume
branch
from
38330b5
to
d2153b8
Compare
The firmware calls the SIEN ACPI method on boot to enable the keyboard and trackpad. However, after resuming from sleep, the SPI interface is disabled and we must call SIEN(1) to enable it. Signed-off-by: Cameron Gutman <aicommander@gmail.com>
cgutman
force-pushed
the
workingresume
branch
from
d2153b8
to
2b99468
Compare
Yep, exactly why I added it there.
Good catch, thanks. I've changed it to just return -ENODEV in probe() if the USB interface is enabled. I'm not sure of the real pros and cons (maybe power consumption?) of driving it via USB vs. SPI but for now, the USB HID implementation is more feature complete.
Done.
I did find the delay was necessary. I added a call to SIST to skip the SIEN call and the delay if SPI is already enabled. What I saw without it (or a smaller delay), the trackpad would not reliably be put into multitouch mode successfully via applespi_init(). Keyboard and mouse clicks would still work, but pointer movement would not. This would happen randomly coming out of sleep or stressing the probe path with "rmmod applespi && insmod ./applespi.ko" over and over (before I added the SIST check). By my rough count, I'd say it failed 10-20% of the time the system resumed or the module loaded without the delay in place. I didn't dig too deeply, but maybe the response from the device in applespi_init() may tell us that we need to retry or something. |
|
Thanks @cgutman, it's good to see this driver move forward and gain features. Who knows, maybe the upcoming MacBook Pros and Airs will move to SPI as well, it's important to be prepared and have this in as much a mainline-able form as possible. Reviewed the changes again, the only thing that I spotted is that it might make sense to disable the SPI interface in the ->suspend hook. That way it would be off if the user invokes suspend-to-idle (which powers down devices but doesn't ask the platform to go to S3), but it would only be of value of it actually saves power. On the other hand, maybe it's necessary to keep the SPI interface enabled to be able to wake up from suspend-to-idle. So I'm not sure about it. Another thing to keep in mind is that |
I'm sure it's something like that; the macOS driver does other things like CRC checking and validating the responses. I did record the correct response to the modeswitch command, so that can be used to make the init process much more robust. There are also a ton of other commands that I haven't fully decoded that do things like get the version, name, etc from the touchpad. I'm happy to merge this in as-is, and we can revisit the
Perhaps we should try and profile it; I know that the touchpad still responds to clicks when suspended normally in macOS, so I think even with the SPI interface disabled the touchpad is still running - at least in some form. |
I've tried replacing the _PS3 and _PS0 methods for RP01 with a single line that just returns
There are indeed matches. I've been reversing the macOS driver, and so far from what I can see it basically just shuts down the NVMe side of things before calling the _PS3 method. This led me to try unload the driver - and this is actually broken. Doing a simple |
|
I'm fine merging it as is here. As an aside, we should probably come up with some better way of communicating to work through these issues with the new MacBooks. I don't care whether it's via some mailing list, (slightly) abusing this repo's bug tracker, or creating a separate repo to file issues against. PRs can be a bit difficult to find after the fact, especially if you don't know what you're looking for. |
I've tried replacing the _PS3 and _PS0 methods for RP01 with a single line that just returns 0x02; calling this with acpi_call yields the correct value, indicating that the modified DSDT is being used, but on suspend / resume the exact same behavior as before seems to happen. Is the modified DSDT table perhaps ignored when suspending / resuming? Don't think so, but you'd have to ask the experts on linux-acpi@ to be sure. When _PS3 or _PS0 is called from The problem is that _PS0 does not or cannot complete all steps because it returns early. And it does so because the PCIe link to the SSD doesn't come up. Either the SSD isn't powered up at all or it's somehow stuck or not initialized. There are indeed matches. I've been reversing the macOS driver, and so far from what I can see it basically just shuts down the NVMe side of things before calling the _PS3 method. This led me to try unload the driver - and this is actually broken. Doing a simple rmmod nvme (with no suspend / resume involved) takes a while and leads to a message about shutdown failing. Perhaps the NVMe drive can only be woken up if it has been shutdown properly? Does the SSD driver call _PS3 directly, or via On Linux, the device hierarchy is walked bottom-up during the suspend phase and top-down during the resume phase. If _PS0 and _PS3 is present, the PCI core should automatically regard the device as With Thunderbolt, we did have an issue with the Cactus Ridge (2012) generation of controllers which have to be transitioned into a suspend state ("Go2Sx") by calling a number of custom ACPI methods in a specific order, this is in |
Great, merging.
I agree, I've been (ab)using these PRs a bit to discuss matters and ask questions that really have nothing to do with the trackpad / keyboard. Perhaps there's already a suitable mailing list? Otherwise I'm open to suggestions. |
Oh, sure. I was just returning a value to verify manually (via acpi_call) that my custom DSDT was being called and not the original one.
I believe it is called directly. I'm quite new when it comes to disassembly, so I could be mistaken.
One thing I did notice, after skimming through the NVMe spec and toying with the NVMe driver, was that attempting to shutdown the controller by removing queues and setting the appropriate register (which is what the Linux driver does on suspend / removal of device; page 182 of the spec) actually leads to the controller indicating a fatal status condition (page 217 / page 45). Interestingly, the controller won't work after this when trying to reload the driver (ie, just doing a simple |
|
@cb22: Interesting find, thanks! The SSD is supposed to be reset by the |
|
@l1k I think I've found something worth mentioning; it seems like the macOS driver executes a vendor specific NVMe admin command with the opcode According to the NVMe spec, the The chain goes something like this: My guess is whatever |
|
Nice job, looks like you're fairly close, @cb22. Most of the kernel extensions are written in C++, or rather the subset of C++ supported by the runtime in the xnu kernel. The way to decipher these vtable calls is as follows: The |
|
Sorry to add noise and resurrect this thread, but I'm wondering if anyone is still working on overall suspend/resume (I assume the NVMe issue is still the blocker)? I have a 2016 MacBookPro13,1, and it would be great if suspend would work. I'd be happy to pitch in if it's possible to help on this. |
The firmware calls the SIEN ACPI method on boot to enable the
keyboard and trackpad. However, after resuming from sleep, the SPI
interface is disabled and we must call SIEN(1) to enable it.
Signed-off-by: Cameron Gutman aicommander@gmail.com