applespi: Remove the need to modify the DSDT.

[roadrunner2]

This creates the spi device based on information taken from the DSM method. While a more complete fix should probably be done in the core, this at least avoids the need for DSDT modifications right now - most of this should be removed again when the core has the desired support. Partly for this reason all the added code uses the `appleacpiprefix instead of theapplespi_` prefix used everywhere else in this module, thereby making it clear that it A) is related to acpi not spi per se, and B) is a piece of functionality that can be removed together. But feedback/comments welcome.

This resolves #21 for now.

[tudorbarascu]

@roadrunner2 just tried it and I can't seem to get it to work.

I'm getting the applespi: No spi-master device found for bus-number 2 - waiting for it to be registered when doing dmesg

[roadrunner2]

@tudorbarascu Thanks for trying this out. There should be a second message "Got spi-master device for bus-number 2" after that, but I'm guessing not since you don't mention it. So, some issue with detecting the spi-master device then. What does ls -l /sys/devices/pci0000:00/0000:00:1e.3/pxa2xx-spi.5/spi_master/spi2 show? If that doesn't exist, which parent of that exists? Also, what does lspci | grep SPI show?

[tudorbarascu]

lspci | grep SPI outputs:

00:1e.3 Signal processing controller: Intel Corporation Sunrise Point-LP Serial IO SPI Controller #1 (rev 21)

in /sys/devices/pci0000:00/0000:00:1e.3/pxa2xx-spi.3 I have:

-rw-r--r-- 1 root root 4096 May 28 21:09 driver_override
lrwxrwxrwx 1 root root    0 May 28 21:09 firmware_node -> ../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:8b
-r--r--r-- 1 root root 4096 May 28 21:09 modalias
drwxr-xr-x 2 root root    0 May 28 21:09 power
lrwxrwxrwx 1 root root    0 May 28 21:09 subsystem -> ../../../../bus/platform
-rw-r--r-- 1 root root 4096 May 28 21:09 uevent

[roadrunner2]

Thanks for the info. So your model has one SPI controller - interesting (the 13,3 model has 2, but one seems unused), and possibly the reason for the device being called pxa2xx-spi.3 instead of pxa2xx-spi.5, but not the issue here.

Instead, I see that the pxa2xx-spi.3 device has no driver associated with it - is the 'spi_pxa2xx_platform' module loaded on your system? If not, make sure to load it.

Oh, and can you run 'lspci -nn | grep SPI' for me? Based on the fact that the SPI controller on your machine is the LP controller, I'm guessing the pci-id is '8086:9d29' or '8086:9d2a'.

[tudorbarascu]

Indeed, I retested and it works. Thanks a lot! Sorry for the noise. I noticed that the touchpad speed is quite lower (shouldn't be related to this, right?).

I've setup the touchpad to maximum speed and passing over the touchpad with the finger from left to right only does 3/4 of the screen.

[roadrunner2]

Glad to hear that fixed it.

Regarding the touchpad speed, did you add the hwdb entries I specify in the "Keyboard/Touchpad/Touchbar" section in my gist? Also note that I fixed a syntax bug in the attached 61-evdev-local.hwdb file there yesterday, in case you got an older version.

[tudorbarascu]

Regarding the touchpad speed, did you add the hwdb entries I specify in the "Keyboard/Touchpad/Touchbar" section in my gist? Doh... no.. I managed to forget that step, I must be day-dreaming.

Thanks a lot! Do you still plan on pushing those upstream?

[roadrunner2]

Yes, I definitely want to upstream that (as with pretty much everything else). Before doing so I was hoping somebody could give me the proper dpi values for the MacBook touchpad's, though.

[cb22]

@roadrunner2 this works well on my MacBook9,1; after removing the DSDT modification everything is still detected. Agreed that it should be temporary until there's a better solution in the kernel, although might end up in a bit of a chicken and egg problem.

If you're happy with it, I'll merge it in.

[roadrunner2]

I'm reasonably happy with it, as far as things go: it's been stable for me in various scenarios, and nobody so far has reported issues. The implicit driver dependency that @tudorbarascu ran into above is unfortunate, but since that spi_pxa2xx_platform module exports no symbols I don't know of any way to set up an explicit dependency on it, i.e. this is a matter of documentation.

One of the reasons I created this patch is because I noticed that the modified DSDT was a huge area of problems for folks (various distros apparently make it quite hard to load a custom DSDT short of building your own modified kernel, which is definitely more work and maintenance than just building just a module or two out of tree). So I really think it'll significantly lower the barrier to using this module (and generally running Linux on these machines).

[choelzl]

Have been running it for some days on two different macbook9.1, no errors or problems here 👍

[cb22]

@roadrunner2 do you perhaps want to add a note about spi_pxa2xx_platform to the README, then I'll merge this?

[roadrunner2]

@cb22 Done.

[andy-shev]

@roadrunner2 Thanks for pointing to this. So, the problem has legal aspect as well. _DSM methods are owned by companies. I'm pretty sure that

• _DSM for Mac is owned by Apple (hello, Cap!)
• Apple will not allow to use it in open source

That said, you can't upstream this (part) at all.

[roadrunner2]

Regarding the legal comments, I'm having some trouble understanding how invoking _DSM is any different from invoking any other method in the DSDT - in both cases the implementation is copyrighted by whoever wrote it, but having bought the machine you also have a license to use them (I have not seen anything anywhere that indicates any restriction on what software may call these methods). So I'm going to need a bit more solid evidence to be convinced this is an actual issue. But anyway, that's a discussion for the patches to the acpi core and for the lawyers.

[andy-shev]

I'm not a lawyer, so I'm speculating on a common sense of the reading of ACPI specification. So, the methods that are nicely defined in ACPI are free to be called under ACPI terms (via ACPICA or maybe any other custom implementation), while vendor specific methods are completely specific to vendors and specification doesn't clarify this legal aspect. So, if I would be you, I would contact Apple to clarify use of this method out of Mac OS X. To the examples, we exactly stumbled once over one of the case (w/ Microsoft though) where we got a permission to use it in Linux kernel.

[l1k]

I've coded up two commits to the ACPI core and SPI core today, essentially what I proposed last year (see link in #21). Need to do a bit of testing and polishing, please stand by.

[andy-shev]

@l1k Lukas, whatever you come with please Cc me and Rafael (maybe Mika as well).

[l1k]

@andy-shev: Sure, of course.

@roadrunner2: I've just pushed a new spi_properties_v1 branch containing these two commits:

• l1k/linux@f09b3e7a66e66bab44f621b4d2883bca5d5d0564 ("ACPI / property: Support Apple _DSM properties")
• l1k/linux@b87b36370a88e2a39c284fa35a595f419cac82c9 ("spi: Use Apple device properties in absence of ACPI resources")

This is an implementation of what I proposed last year (linked by @Dunedan in #21).

I do not have a machine with SPI-attached keyboard, so I cannot test this. I did test retrieval of _DSM Buffer properties with a u32 stored for the Firewire device on my machine and it worked fine. Could you try rebasing the SPI driver on top of these two commits. (Preferably on a separate branch so that people can continue using the existing solution for now.) It would be good to have a Tested-by from you at least for the second commit when posting to the mailing lists.

The two commits may still contain bugs and there's also some debugging code left in which hexdumps the _DSM properties. I will have to review the commits a couple of times with a fresh pair of eyeballs before I can post them to the mailing lists. I will force-push the branch and let you know if I make modifications.

Thanks!

[l1k]

Okay, 0-day notified me of two bugs. Fixed those and force-pushed to the branch, the commit IDs referenced above are thus already outdated.

[l1k]

@roadrunner2: I've fixed a bunch of bugs and pushed the spi_properties_v1 branch once more. Still need to do some testing, but getting closer.

Fun fact: I've downloaded the MacOS X 10.4.11 Combo updater to verify that indeed they had support for both, _DSM and EFI properties built in from the very beginning. :-)

[roadrunner2]

@l1k Thanks a bunch for this! I had a quick look at the commits, and at a high level they look good. I'll pull your commits, adjust the driver, and test things out later this week.

[l1k]

@roadrunner2: Great, thanks. In the meantime I've also tested various edge cases successfully (properties with wrong type, empty property name). Confidence is increasing that at least the ACPI changes are good.

[l1k]

@roadrunner2: I've fully qualified the ACPI changes now and sent out the patches to get some more feedback. That doesn't mean you need to hurry, just respond once you've found time to test. In theory it should be sufficient to checkout a commit before 0c34936ed9a1c01c18365475db86170443c6d918, apply my ACPI and SPI patches to the kernel and use an unpatched DSDT. The SPI slave should then be initialized correctly. Thanks!

[roadrunner2]

@l1k I pulled the code from the spi_properties_v2 branch, built, and debugged this now. Unfortunately things don't quite work yet out of the box. In particular, there are two places that are checking for acpi-resources and doing the wrong thing because none are found on the APP000D device:

1. drivers/acpi/scan.c line 1765 checks for the spi_i2c_slave flag and sets the device to enumerated if false (which then causes drivers/spi/spi.c line 1771 to bail). The flag is set by acpi_is_spi_i2c_slave(), which doesn't return true because it doesn't find any serial-bus resources on the device.
2. drivers/spi/spi.c line 1791 checks for a successful enumeration of resources in line 1785, but again since this device has no resources that returns an error.

I'm not sure what the best solution is here; for now I've hacked in some explicit checks for the APP000D device in the above mentioned places and with that it works. The applespi driver needed a bit more work than just reverting commit 0c34936ed9a1c01c18365475db86170443c6d918, since we still need to get a few of the other properties provided by the _DSM method; I've pushed those changes to my spi-properties branch.

[l1k]

@roadrunner2: Amazing, thanks a lot!

As to (1), I think it should be sufficient to check for presence of one representative device property in acpi_is_spi_i2c_slave(), e.g. spiSclkPeriod, which is also checked in acpi_register_spi_device() (as max_speed_hz), and immediately return true if found. I'll amend the series accordingly. Sorry for missing that.

As to (2), I'm not sure how that check could be a problem since ret < 0 should be false (acpi_dev_get_resources() should return 0 if _CRS returns empty data or cannot find _CRS at all) and max_speed_hz should have been set to an appropriate value by acpi_spi_parse_apple_properties(). Could you double-check which of the two conditions return true? Thanks!

[roadrunner2]

@l1k The value of ret was -5 (EIO).

[l1k]

Hm, then acpi_walk_resources() returned an error:
http://elixir.free-electrons.com/linux/latest/source/drivers/acpi/resource.c#L622

While we could add another Apple-specific quirk here to ignore -EIO, it doesn't appear to be a proper solution. I'm wondering if this is a bug in the ACPICA.

The AML contains an empty resource template:

    Name (ABUF, ResourceTemplate ()
    {
    })

    Return (ABUF) /* \_SB_.PCI0.SPI1.SPIT._CRS.ABUF */

And the ACPI 6.1 spec, page 800 explicitly allows empty resource templates:

ResourceTemplateTerm :=
    ResourceTemplate () {ResourceMacroList} => Buffer
ResourceMacroList :=
    Nothing | <ResourceMacroTerm ResourceMacroList>

Could you insert a couple of printk()s in acpi_walk_resources() and the functions it calls to find out where the ACPI_FAILURE() status is coming from?

[roadrunner2]

@l1k Ok, narrowed it down to drivers/acpi/acpica/utresrc.c line 191: aml_length is 2, as is the sizeof(struct). The call stack is: drivers/acpi/acpica/utresrc.c line 191 drivers/acpi/acpica/rscreate.c line 181 drivers/acpi/acpica/rsutils.c line 698 drivers/acpi/acpica/rsxface.c line 651 drivers/acpi/resource.c line 618

[l1k]

@roadrunner2: If you revert commit l1k/linux@a83019eb9f1f ("ACPICA: Update resource descriptor handling"), does the issue go away? That commit is brand new (June 5) and may have introduced a regression for empty ResourceTemplates.

[roadrunner2]

@l1k Bingo! Good catch. That indeed makes acpi_dev_get_resources() return a sucess now.

[l1k]

@roadrunner2: Okay. Getting stuff fixed in the ACPICA is somewhat complicated because it's an OS-independent layer that is merely included verbatim in the kernel and called from functions outside of the drivers/acpi/acpica directory. Usually patches have to go through ACPICA upstream and then flow down to the kernel, so the proper folks to contact are the ACPICA maintainers, which I've just done and cc'ed you. Hopefully this gets fixed quickly. Thanks a lot for taking the time to track this down.

[l1k]

@roadrunner2: I've pushed a new version of the spi_properties_v2 branch which contains a revert of the offending ACPICA commit plus a new commit to work around the enumeration issue.

However it has now occurred to me that a better approach might be to modify acpi_dev_get_resources() so that on Macs it fabricates a struct acpi_resource for each applicable device property and invokes the supplied preproc callback on it. This would avoid having to modify the SPI core and it would also avoid the additional commit which modifies acpi_is_spi_i2c_slave(). Please stand by while I hack together an implementation of this idea.

[l1k]

@roadrunner2: Actually... maybe it's a dumb idea. There's no bijection between Apple's properties and _CRS, e.g. spiBitOrder can't be represented with _CRS. Perhaps that was their reason in the first place to come up with their own scheme. So the solution that's now on the spi_properties_v2 branch may be the right thing to do. Could you give it a spin and see if it works for you? Thanks!

[andy-shev]

@l1k, Perhaps you may convert Apple's _DSM to _DSD somewhere, though the best case for doing that is still near to SPI core. ACPI core should be free of such mappings / conversions.

[roadrunner2]

@l1k I can confirm things work nicely now with your latest version of the branch and my modified driver. Many thanks.

Regarding the properties vs _CRS, yes, the driver stills need to get those delay values from the properties as they can't be represented in the acpi spi resource.

[l1k]

@andy-shev: The device properties are not only queried by the SPI core: As @roadrunner2 has pointed out above, the HID driver for the SPI-attached keyboard likewise has the need to query certain device properties. In addition, the DSDT on recent MacBooks and MacBook Pros contains device properties (instead of _CRS data) for various UART- and I2C-attached devices and I anticipate we'll have to query those as well, e.g. in the I2C core. It seems inappropriate to me to place the DSM-to-DSD converter in the SPI subsystem even though other subsystems depend on the device properties as well. The ACPI core appears to be the most logical place as the properties are stored in AML. I could however place the code in a separate file if consensus is that it clutters up drivers/acpi/property.c.

[l1k]

@roadrunner2: Sorry for the delay, I've finished v3 of the SPI properties patches and pushed them to the spi_properties_v3 branch. They pass all my tests and I'm relatively confident that they should still work on MacBooks with SPI keyboards, nevertheless I'd be grateful if you could give them another spin. Thanks!

[roadrunner2]

@l1k I've built and debugged v3 now. I needed to make the following change:

@@ -1452,10 +1452,15 @@ static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
        struct list_head resource_list;
        bool is_spi_i2c_slave = false;

-       /* Macs use device properties in lieu of _CRS resources */
+       /*
+        * Macs use device properties in lieu of _CRS resources.
+        *
+        * Note: can't use device_property_present() because fwnode is not set
+        * yet.
+        */
        if (is_apple_system &&
-           (device_property_present(&device->dev, "spiSclkPeriod") ||
-            device_property_present(&device->dev, "i2cAddress")))
+           (!acpi_dev_get_property(device, "spiSclkPeriod", ACPI_TYPE_ANY, NULL) ||
+            !acpi_dev_get_property(device, "i2cAddress", ACPI_TYPE_ANY, NULL)))
                return true;

        INIT_LIST_HEAD(&resource_list);

The problem is that device_property_present() looks at the acpi device's fwnode (and then from there gets back to the acpi device), but fwnode is not set (yet) at this point. Now, your v2 branch contained commit 3708184afc which (presumably inadvertently) changed the semantics of fwnode_property_present() to return true (or really -EINVAL) if fwnode was null. But v3 does not contain this commit, and hence device_property_present() always returns false here.

[l1k]

@roadrunner2: My apologies for missing that issue and thanks so much for tracking it down. The fwnode attribute is only ever set for the struct device embedded in physical devices (e.g. struct spi_device) or platform devices. It is never set and thus always NULL for the struct device embedded in struct acpi_device.

On one hand that makes sense because the struct acpi_device is the firmware node / companion to the physical device. On the other hand it could also be considered a bug because there are drivers which bind directly to the struct acpi_device (e.g. anything that declares a module_acpi_driver()), and those drivers will never be able to use the device_property_*() functions due to fwnode being NULL for the struct device embedded in the struct acpi_device they're binding to.

I'll ask Rafael what he thinks about it, meanwhile I've gone with fwnode_property_present(), which is minimally shorter than acpi_dev_get_property(). I'm pushing the revised patches to the spi_properties_v3 branch right now, could you double-check if they're working for you? Thanks!

[andy-shev]

It's not a bug, pure ACPI devices in Linux kernel module is a legacy.

[roadrunner2]

@l1k Ah, right, that explanation makes sense. Thanks. In any case, your latest v3 branch is working like a charm now.

[l1k]

@roadrunner2 @andy-shev: Just a quick heads-up, I've cooked up an spi_properties_v4 series over the weekend and am planning to post it sometime this week.

The retrieval of the ACPI properties and the SPI initialization is unchanged from v3, only the recognition of Apple machines was amended as discussed on the mailing list with Rafael, Andy & Darren:

New commit l1k/linux@20f8b74b83ed45171583fe501182c93e5c6eb4d9 ("treewide: Consolidate Apple DMI checks") performs the Apple DMI check once early on in the boot sequence and the cached value is subsequently used everywhere else. Commit l1k/linux@6dad5b18be68f1b7abb243ec04383bb7011f1ca3 ("ACPI / osi: Exclude x86 DMI quirks on other arches") is no longer needed and thus dropped.

[roadrunner2]

@l1k FYI, I'm still debugging this, but something's busted: in acpi_is_spi_i2c_slave() the fwnode_property_present(...) checks are returning false. It'll be another day or so before I can debug this further.

OTOH, the new x86_apple_machine flag/check is fine.

[l1k]

@roadrunner2: Ugh. :-/ Is this with an unpatched DSDT? It's certainly odd since it worked before and the patches should be the same. However they're on top of Rafael's bleeding-edge branch as of last Friday wheras spi_properties_v3 was based on an earlier date of that branch, it's possible something has changed in the meantime. But that would seem to be unrelated to this series and could be addressed by a separate fix.

[roadrunner2]

@l1k Ok, chased this one down now. Sorry, fwnode_property_present(...) is fine, as is the rest of your patches. Instead, it's our old friend acpi_ut_walk_aml_resources() that we had a talk about with the acpica guys - I guess Rafael still has that change in his tree, though it's not in v4.13-rc3 nor Rafael's acpi-4.13-rc4.

In short, all clear on your patches. Many thanks!

[l1k]

@roadrunner2: Ah, right, dammit. Rafael said that he'd drop the offending commit from the 4.13 pull that he'd send to Linus, which he did. But he intentionally left the commit on his bleeding-edge branch as the ACPICA material for 4.14 was going to be based on it. Indeed, the ACPICA folks reverted the commit as well with acpica/acpica@f3300640 on June 27 (one day after I forwarded your report), posted the Linuxized version to the mailing list yesterday and Rafael applied it to his bleeding-edge branch 10 hours ago.

I had based the series on Rafael's bleeding-edge branch to ensure that it applies cleanly once he picks it up. I should have remembered the offending commit and revert it on the spi_properties_v4 branch. (I just did that.) Truly sorry for the extra work this has caused you! :-(

[l1k]

Rafael kindly merged the spi_properties_v4 series into his bleeding-edge branch yesterday and it will now get some testing in linux-next. If there are no regressions, it should land in Linus' tree during the upcoming 4.14 merge window (about a month from now).

[roadrunner2]

@l1k Excellent news! Thanks for creating these patches and pushing them through.

[l1k]

Okay the SPI properties support landed in Linus' tree half an hour ago with commit torvalds/linux@53ac64aac9af8cd0e5456c8a9bb68c47b571b0a9.