[PATCH v3 0/6] Apple SPI properties

[PATCH v3 0/6] Apple SPI properties

[Lukas Wunner]

Retrieve device properties on Macs with an Apple-specific _DSM and
use them in lieu of _CRS data upon SPI slave initialization, v3.

Please refer to the cover letters of v1 and v2 for further details:
http://www.spinics.net/lists/linux-acpi/msg75537.html
http://www.spinics.net/lists/linux-acpi/msg75735.html

Special thanks to Ronald Tschalär for patient testing/debugging
and Andy Shevchenko for equally patient reviewing on GitHub.

Changes v2 -> v3:

Patch 1 + 2:
- Newly inserted patches in v3 to avoid repeated DMI checks for Apple
  hardware:  The result of the first DMI check in osi.c is cached.
  Two other existing DMI checks are converted to use the result.
  Because one of them is in a module (sbs.ko), the bool is_apple_system
  needs to be exported.  On non-x86, the DMI checks and Apple-specific
  code are omitted altogether. (Andy, Rafael)

Patch 4:
- Use bitmap to keep track of valid properties.  Move to x86/apple.c
  to avoid cluttering up generic ACPI code. (Andy, Rafael)

Patch 5:
- Use fwnode_property_present() instead of device_property_present(),
  the latter doesn't work as the fwnode pointer of the struct device
  embedded in a struct acpi_device is always NULL. (Ronald)

Patch 6:
- Check buffer length for extra safety.  Rename "o" to "obj",
  use 32 bit division to calculate max_speed_hz. (Andy)

Thanks,

Lukas


Lukas Wunner (6):
  ACPI / osi: Exclude x86 DMI quirks on other arches
  ACPI / x86: Consolidate Apple DMI checks
  ACPI / property: Don't evaluate objects for devices w/o handle
  ACPI / property: Support Apple _DSM properties
  ACPI / scan: Recognize Apple SPI and I2C slaves
  spi: Use Apple device properties in absence of ACPI resources

 drivers/acpi/Makefile    |   1 +
 drivers/acpi/internal.h  |   6 +++
 drivers/acpi/osi.c       |   8 +++
 drivers/acpi/pci_root.c  |   3 +-
 drivers/acpi/property.c  |   6 +++
 drivers/acpi/sbs.c       |  24 +--------
 drivers/acpi/scan.c      |   6 +++
 drivers/acpi/x86/apple.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/spi/spi.c        |  31 +++++++++++
 include/linux/acpi.h     |   6 +++
 10 files changed, 203 insertions(+), 25 deletions(-)
 create mode 100644 drivers/acpi/x86/apple.c

-- 
2.11.0

[PATCH v3 6/6] spi: Use Apple device properties in absence of ACPI resources

[Lukas Wunner]

MacBooks and MacBook Pros introduced since 2015 return empty _CRS data
for SPI slaves, causing device initialization to fail.  Most of the
information that would normally be conveyed via _CRS is available
through ACPI device properties instead, so take advantage of them.

The meaning and appropriate usage of the device properties was reverse
engineered by Ronald Tschalär and carried over from these commits
authored by him:

https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1

According to Ronald, the device properties have the following meaning:

spiSclkPeriod   /* period in ns */
spiWordSize     /* in number of bits */
spiBitOrder     /* 1 = MSB_FIRST, 0 = LSB_FIRST */
spiSPO          /* clock polarity: 0 = low, 1 = high */
spiSPH          /* clock phase: 0 = first, 1 = second */
spiCSDelay      /* delay between cs and receive on reads in 10 us */
resetA2RUsec    /* active-to-receive delay? */
resetRecUsec    /* receive delay? */

Cc: Federico Lorenzi <florenzi@gmail.com>
Reported-by: Leif Liddy <leif.liddy@gmail.com>
Tested-by: Ronald Tschalär <ronald@innovation.ch>
Acked-by: Mark Brown <broonie@kernel.org>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes v2 -> v3:
- Check buffer length for extra safety.  Rename "o" to "obj",
  use 32 bit division to calculate max_speed_hz. (Andy)

 drivers/spi/spi.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 4fcbb0aa71d3..cce133057700 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1693,6 +1693,35 @@ static void of_register_spi_devices(struct spi_controller *ctlr) { }
 #endif
 
 #ifdef CONFIG_ACPI
+static void acpi_spi_parse_apple_properties(struct spi_device *spi)
+{
+	struct acpi_device *dev = ACPI_COMPANION(&spi->dev);
+	const union acpi_object *obj;
+
+	if (!is_apple_system)
+		return;
+
+	if (!acpi_dev_get_property(dev, "spiSclkPeriod", ACPI_TYPE_BUFFER, &obj)
+	    && obj->buffer.length >= 4)
+		spi->max_speed_hz  = NSEC_PER_SEC / *(u32 *)obj->buffer.pointer;
+
+	if (!acpi_dev_get_property(dev, "spiWordSize", ACPI_TYPE_BUFFER, &obj)
+	    && obj->buffer.length == 8)
+		spi->bits_per_word = *(u64 *)obj->buffer.pointer;
+
+	if (!acpi_dev_get_property(dev, "spiBitOrder", ACPI_TYPE_BUFFER, &obj)
+	    && obj->buffer.length == 8 && !*(u64 *)obj->buffer.pointer)
+		spi->mode |= SPI_LSB_FIRST;
+
+	if (!acpi_dev_get_property(dev, "spiSPO", ACPI_TYPE_BUFFER, &obj)
+	    && obj->buffer.length == 8 &&  *(u64 *)obj->buffer.pointer)
+		spi->mode |= SPI_CPOL;
+
+	if (!acpi_dev_get_property(dev, "spiSPH", ACPI_TYPE_BUFFER, &obj)
+	    && obj->buffer.length == 8 &&  *(u64 *)obj->buffer.pointer)
+		spi->mode |= SPI_CPHA;
+}
+
 static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 {
 	struct spi_device *spi = data;
@@ -1766,6 +1795,8 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,
 				     acpi_spi_add_resource, spi);
 	acpi_dev_free_resource_list(&resource_list);
 
+	acpi_spi_parse_apple_properties(spi);
+
 	if (ret < 0 || !spi->max_speed_hz) {
 		spi_dev_put(spi);
 		return AE_OK;
-- 
2.11.0

Re: [PATCH v3 6/6] spi: Use Apple device properties in absence of ACPI resources

[Andy Shevchenko]

On Fri, 2017-07-14 at 00:36 +0200, Lukas Wunner wrote:
> MacBooks and MacBook Pros introduced since 2015 return empty _CRS data
> for SPI slaves, causing device initialization to fail.  Most of the
> information that would normally be conveyed via _CRS is available
> through ACPI device properties instead, so take advantage of them.
> 
> The meaning and appropriate usage of the device properties was reverse
> engineered by Ronald Tschalär and carried over from these commits
> authored by him:
> 
> https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
> https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1
> 
> According to Ronald, the device properties have the following meaning:
> 
> spiSclkPeriod   /* period in ns */
> spiWordSize     /* in number of bits */
> spiBitOrder     /* 1 = MSB_FIRST, 0 = LSB_FIRST */
> spiSPO          /* clock polarity: 0 = low, 1 = high */
> spiSPH          /* clock phase: 0 = first, 1 = second */
> spiCSDelay      /* delay between cs and receive on reads in 10 us */
> resetA2RUsec    /* active-to-receive delay? */
> resetRecUsec    /* receive delay? */
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

(obj is better than o)

> Cc: Federico Lorenzi <florenzi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reported-by: Leif Liddy <leif.liddy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Tested-by: Ronald Tschalär <ronald-Cgq6lnktLNMTaf21n8AfGQ@public.gmane.org>
> Acked-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Acked-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> ---
> Changes v2 -> v3:
> - Check buffer length for extra safety.  Rename "o" to "obj",
>   use 32 bit division to calculate max_speed_hz. (Andy)
> 
>  drivers/spi/spi.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 4fcbb0aa71d3..cce133057700 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1693,6 +1693,35 @@ static void of_register_spi_devices(struct
> spi_controller *ctlr) { }
>  #endif
>  
>  #ifdef CONFIG_ACPI
> +static void acpi_spi_parse_apple_properties(struct spi_device *spi)
> +{
> +	struct acpi_device *dev = ACPI_COMPANION(&spi->dev);
> +	const union acpi_object *obj;
> +
> +	if (!is_apple_system)
> +		return;
> +
> +	if (!acpi_dev_get_property(dev, "spiSclkPeriod",
> ACPI_TYPE_BUFFER, &obj)
> +	    && obj->buffer.length >= 4)
> +		spi->max_speed_hz  = NSEC_PER_SEC / *(u32 *)obj-
> >buffer.pointer;
> +
> +	if (!acpi_dev_get_property(dev, "spiWordSize",
> ACPI_TYPE_BUFFER, &obj)
> +	    && obj->buffer.length == 8)
> +		spi->bits_per_word = *(u64 *)obj->buffer.pointer;
> +
> +	if (!acpi_dev_get_property(dev, "spiBitOrder",
> ACPI_TYPE_BUFFER, &obj)
> +	    && obj->buffer.length == 8 && !*(u64 *)obj-
> >buffer.pointer)
> +		spi->mode |= SPI_LSB_FIRST;
> +
> +	if (!acpi_dev_get_property(dev, "spiSPO", ACPI_TYPE_BUFFER,
> &obj)
> +	    && obj->buffer.length == 8 &&  *(u64 *)obj-
> >buffer.pointer)
> +		spi->mode |= SPI_CPOL;
> +
> +	if (!acpi_dev_get_property(dev, "spiSPH", ACPI_TYPE_BUFFER,
> &obj)
> +	    && obj->buffer.length == 8 &&  *(u64 *)obj-
> >buffer.pointer)
> +		spi->mode |= SPI_CPHA;
> +}
> +
>  static int acpi_spi_add_resource(struct acpi_resource *ares, void
> *data)
>  {
>  	struct spi_device *spi = data;
> @@ -1766,6 +1795,8 @@ static acpi_status
> acpi_register_spi_device(struct spi_controller *ctlr,
>  				     acpi_spi_add_resource, spi);
>  	acpi_dev_free_resource_list(&resource_list);
>  
> +	acpi_spi_parse_apple_properties(spi);
> +
>  	if (ret < 0 || !spi->max_speed_hz) {
>  		spi_dev_put(spi);
>  		return AE_OK;

-- 
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/6] ACPI / property: Support Apple _DSM properties

[Lukas Wunner]

While the rest of the world has standardized on _DSD as the way to store
device properties in AML (introduced with ACPI 5.1 in 2014), Apple has
been using a custom _DSM to achieve the same for much longer (ever since
they switched from DeviceTree-based PowerPC to Intel in 2005, verified
with MacOS X 10.4.11).

The theory of operation on macOS is as follows:  AppleACPIPlatform.kext
invokes mergeEFIproperties() and mergeDSMproperties() for each device to
merge properties conveyed by EFI drivers as well as properties stored in
AML into the I/O Kit registry from which they can be retrieved by
drivers.  We've been supporting EFI properties since commit 58c5475aba67
("x86/efi: Retrieve and assign Apple device properties").  The present
commit adds support for _DSM properties, thereby completing our support
for Apple device properties.  The _DSM properties are made available
under the primary fwnode, the EFI properties under the secondary fwnode.
So for devices which possess both property types, they can all be
elegantly accessed with the uniform API in <linux/property.h>.

Until recently we had no need to support _DSM properties, they contained
only uninteresting garbage.  The situation has changed with MacBooks and
MacBook Pros introduced since 2015:  Their keyboard is attached with SPI
instead of USB and the _CRS data which is necessary to initialize the
spi driver only contains valid information if OSPM responds "false" to
_OSI("Darwin").  If OSPM responds "true", _CRS is empty and the spi
driver fails to initialize.  The rationale is very simple, Apple only
cares about macOS and Windows:  On Windows, _CRS contains valid data,
whereas on macOS it is empty.  Instead, macOS gleans the necessary data
from the _DSM properties.

Since Linux deliberately defaults to responding "true" to _OSI("Darwin"),
we need to emulate macOS' behaviour by initializing the spi driver with
data returned by the _DSM.

An out-of-tree driver for the SPI keyboard exists which currently binds
to the ACPI device, invokes the _DSM, parses the returned package and
instantiates an SPI device with the data gleaned from the _DSM:
https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1

By adding support for Apple's _DSM properties in generic ACPI code, the
out-of-tree driver will be able to register as a regular SPI device,
significantly reducing its amount of code and improving its chances to
be mainlined.

The SPI keyboard will not be the only user of this commit:  E.g. on the
MacBook8,1, the UART-attached Bluetooth device likewise returns empty
_CRS data if OSPM returns "true" to _OSI("Darwin").

The _DSM returns a Package whose format unfortunately deviates slightly
from the _DSD spec:  The properties are marshalled up in a single Package
as alternating key/value elements, unlike _DSD which stores them as a
Package of 2-element Packages.  The present commit therefore converts
the Package to _DSD format and the ACPI core can then treat the data as
if Apple would follow the standard.

Well, except for one small annoyance:  The properties returned by the
_DSM only ever have one of two types, Integer or Buffer.  The former is
retrievable as usual with device_property_read_u64(), but the latter is
not part of the _DSD spec and it is not possible to retrieve Buffer
properties with the device_property_read_*() functions due to the type
checking performed in drivers/acpi/property.c.  It is however possible
to retrieve them with acpi_dev_get_property().  Apple is using the
Buffer type somewhat sloppily to store null-terminated strings but also
integers.  The real data type is not distinguishable by the ACPI core
and the onus is on the caller to use the contents of the Buffer in an
appropriate way.

In case Apple moves to _DSD in the future, this commit first checks for
_DSD and falls back to _DSM only if _DSD is not found.

Cc: Federico Lorenzi <florenzi@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Ronald Tschalär <ronald@innovation.ch>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes v2 -> v3:
- Use bitmap to keep track of valid properties.  Move to x86/apple.c
  to avoid cluttering up generic ACPI code. (Andy, Rafael)

 drivers/acpi/Makefile    |   1 +
 drivers/acpi/internal.h  |   6 +++
 drivers/acpi/property.c  |   3 ++
 drivers/acpi/x86/apple.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 147 insertions(+)
 create mode 100644 drivers/acpi/x86/apple.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index b1aacfc62b1f..90265ab4437a 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -50,6 +50,7 @@ acpi-$(CONFIG_ACPI_REDUCED_HARDWARE_ONLY) += evged.o
 acpi-y				+= sysfs.o
 acpi-y				+= property.o
 acpi-$(CONFIG_X86)		+= acpi_cmos_rtc.o
+acpi-$(CONFIG_X86)		+= x86/apple.o
 acpi-$(CONFIG_X86)		+= x86/utils.o
 acpi-$(CONFIG_DEBUG_FS)		+= debugfs.o
 acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index be79f7db1850..79ee29777909 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -229,6 +229,12 @@ static inline void suspend_nvs_restore(void) {}
 void acpi_init_properties(struct acpi_device *adev);
 void acpi_free_properties(struct acpi_device *adev);
 
+#ifdef CONFIG_X86
+void acpi_extract_apple_properties(struct acpi_device *adev);
+#else
+static inline void acpi_extract_apple_properties(struct acpi_device *adev) {}
+#endif
+
 /*--------------------------------------------------------------------------
 				Watchdog
   -------------------------------------------------------------------------- */
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 27a9294c843c..d05f4f97c963 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -375,6 +375,9 @@ void acpi_init_properties(struct acpi_device *adev)
 	if (acpi_of && !adev->flags.of_compatible_ok)
 		acpi_handle_info(adev->handle,
 			 ACPI_DT_NAMESPACE_HID " requires 'compatible' property\n");
+
+	if (is_apple_system && !adev->data.pointer)
+		acpi_extract_apple_properties(adev);
 }
 
 static void acpi_destroy_nondev_subnodes(struct list_head *list)
diff --git a/drivers/acpi/x86/apple.c b/drivers/acpi/x86/apple.c
new file mode 100644
index 000000000000..bffa38f7460e
--- /dev/null
+++ b/drivers/acpi/x86/apple.c
@@ -0,0 +1,137 @@
+/*
+ * apple.c - Apple ACPI quirks
+ * Copyright (C) 2017 Lukas Wunner <lukas@wunner.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2) as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitmap.h>
+#include <linux/uuid.h>
+
+/* Apple _DSM device properties GUID */
+static const guid_t apple_prp_guid =
+	GUID_INIT(0xA0B5B7C6, 0x1318, 0x441C,
+		  0xB0, 0xC9, 0xFE, 0x69, 0x5E, 0xAF, 0x94, 0x9B);
+
+/**
+ * acpi_extract_apple_properties - retrieve and convert Apple _DSM properties
+ * @adev: ACPI device for which to retrieve the properties
+ *
+ * Invoke Apple's custom _DSM once to check the protocol version and once more
+ * to retrieve the properties.  They are marshalled up in a single package as
+ * alternating key/value elements, unlike _DSD which stores them as a package
+ * of 2-element packages.  Convert to _DSD format and make them available under
+ * the primary fwnode.
+ */
+void acpi_extract_apple_properties(struct acpi_device *adev)
+{
+	unsigned int i, j = 0, newsize = 0, numprops, numvalid;
+	union acpi_object *props, *newprops;
+	unsigned long *valid = NULL;
+	void *free_space;
+
+	props = acpi_evaluate_dsm_typed(adev->handle, &apple_prp_guid, 1, 0,
+					NULL, ACPI_TYPE_BUFFER);
+	if (!props)
+		return;
+
+	if (!props->buffer.length)
+		goto out_free;
+
+	if (props->buffer.pointer[0] != 3) {
+		acpi_handle_info(adev->handle, FW_INFO
+				 "unsupported properties version %*ph\n",
+				 props->buffer.length, props->buffer.pointer);
+		goto out_free;
+	}
+
+	ACPI_FREE(props);
+	props = acpi_evaluate_dsm_typed(adev->handle, &apple_prp_guid, 1, 1,
+					NULL, ACPI_TYPE_PACKAGE);
+	if (!props)
+		return;
+
+	numprops = props->package.count / 2;
+	if (!numprops)
+		goto out_free;
+
+	valid = kcalloc(BITS_TO_LONGS(numprops), sizeof(long), GFP_KERNEL);
+	if (!valid)
+		goto out_free;
+
+	/* newsize = key length + value length of each tuple */
+	for (i = 0; i < numprops; i++) {
+		union acpi_object *key = &props->package.elements[i * 2];
+		union acpi_object *val = &props->package.elements[i * 2 + 1];
+
+		if ( key->type != ACPI_TYPE_STRING ||
+		    (val->type != ACPI_TYPE_INTEGER &&
+		     val->type != ACPI_TYPE_BUFFER))
+			continue; /* skip invalid properties */
+
+		__set_bit(i, valid);
+		newsize += key->string.length + 1;
+		if ( val->type == ACPI_TYPE_BUFFER)
+			newsize += val->buffer.length;
+	}
+
+	numvalid = bitmap_weight(valid, numprops);
+	if (numprops > numvalid)
+		acpi_handle_info(adev->handle, FW_INFO
+				 "skipped %u properties: wrong type\n",
+				 numprops - numvalid);
+	if (numvalid == 0)
+		goto out_free;
+
+	/* newsize += top-level package + 3 objects for each key/value tuple */
+	newsize	+= (1 + 3 * numvalid) * sizeof(union acpi_object);
+	newprops = ACPI_ALLOCATE_ZEROED(newsize);
+	if (!newprops)
+		goto out_free;
+
+	/* layout: top-level package | packages | key/value tuples | strings */
+	newprops->type = ACPI_TYPE_PACKAGE;
+	newprops->package.count = numvalid;
+	newprops->package.elements = &newprops[1];
+	free_space = &newprops[1 + 3 * numvalid];
+
+	for_each_set_bit(i, valid, numprops) {
+		union acpi_object *key = &props->package.elements[i * 2];
+		union acpi_object *val = &props->package.elements[i * 2 + 1];
+		unsigned int k = 1 + numvalid + j * 2; /* index into newprops */
+		unsigned int v = k + 1;
+
+		newprops[1 + j].type = ACPI_TYPE_PACKAGE;
+		newprops[1 + j].package.count = 2;
+		newprops[1 + j].package.elements = &newprops[k];
+
+		newprops[k].type = ACPI_TYPE_STRING;
+		newprops[k].string.length = key->string.length;
+		newprops[k].string.pointer = free_space;
+		memcpy(free_space, key->string.pointer, key->string.length);
+		free_space += key->string.length + 1;
+
+		newprops[v].type = val->type;
+		if (val->type == ACPI_TYPE_INTEGER) {
+			newprops[v].integer.value = val->integer.value;
+		} else {
+			newprops[v].buffer.length = val->buffer.length;
+			newprops[v].buffer.pointer = free_space;
+			memcpy(free_space, val->buffer.pointer,
+			       val->buffer.length);
+			free_space += val->buffer.length;
+		}
+		j++; /* count valid properties */
+	}
+	WARN_ON(free_space != (void *)newprops + newsize);
+
+	adev->data.properties = newprops;
+	adev->data.pointer = newprops;
+
+out_free:
+	ACPI_FREE(props);
+	kfree(valid);
+}
-- 
2.11.0

Re: [PATCH v3 4/6] ACPI / property: Support Apple _DSM properties

[Andy Shevchenko]

On Fri, 2017-07-14 at 00:36 +0200, Lukas Wunner wrote:
> While the rest of the world has standardized on _DSD as the way to
> store
> device properties in AML (introduced with ACPI 5.1 in 2014), Apple has
> been using a custom _DSM to achieve the same for much longer (ever
> since
> they switched from DeviceTree-based PowerPC to Intel in 2005, verified
> with MacOS X 10.4.11).
> 
> The theory of operation on macOS is as
> follows:  AppleACPIPlatform.kext
> invokes mergeEFIproperties() and mergeDSMproperties() for each device
> to
> merge properties conveyed by EFI drivers as well as properties stored
> in
> AML into the I/O Kit registry from which they can be retrieved by
> drivers.  We've been supporting EFI properties since commit
> 58c5475aba67
> ("x86/efi: Retrieve and assign Apple device properties").  The present
> commit adds support for _DSM properties, thereby completing our
> support
> for Apple device properties.  The _DSM properties are made available
> under the primary fwnode, the EFI properties under the secondary
> fwnode.
> So for devices which possess both property types, they can all be
> elegantly accessed with the uniform API in <linux/property.h>.
> 
> Until recently we had no need to support _DSM properties, they
> contained
> only uninteresting garbage.  The situation has changed with MacBooks
> and
> MacBook Pros introduced since 2015:  Their keyboard is attached with
> SPI
> instead of USB and the _CRS data which is necessary to initialize the
> spi driver only contains valid information if OSPM responds "false" to
> _OSI("Darwin").  If OSPM responds "true", _CRS is empty and the spi
> driver fails to initialize.  The rationale is very simple, Apple only
> cares about macOS and Windows:  On Windows, _CRS contains valid data,
> whereas on macOS it is empty.  Instead, macOS gleans the necessary
> data
> from the _DSM properties.
> 
> Since Linux deliberately defaults to responding "true" to
> _OSI("Darwin"),
> we need to emulate macOS' behaviour by initializing the spi driver
> with
> data returned by the _DSM.
> 
> An out-of-tree driver for the SPI keyboard exists which currently
> binds
> to the ACPI device, invokes the _DSM, parses the returned package and
> instantiates an SPI device with the data gleaned from the _DSM:
> https://github.com/cb22/macbook12-spi-driver/commit/9a416d699ef4
> https://github.com/cb22/macbook12-spi-driver/commit/0c34936ed9a1
> 
> By adding support for Apple's _DSM properties in generic ACPI code,
> the
> out-of-tree driver will be able to register as a regular SPI device,
> significantly reducing its amount of code and improving its chances to
> be mainlined.
> 
> The SPI keyboard will not be the only user of this commit:  E.g. on
> the
> MacBook8,1, the UART-attached Bluetooth device likewise returns empty
> _CRS data if OSPM returns "true" to _OSI("Darwin").
> 
> The _DSM returns a Package whose format unfortunately deviates
> slightly
> from the _DSD spec:  The properties are marshalled up in a single
> Package
> as alternating key/value elements, unlike _DSD which stores them as a
> Package of 2-element Packages.  The present commit therefore converts
> the Package to _DSD format and the ACPI core can then treat the data
> as
> if Apple would follow the standard.
> 
> Well, except for one small annoyance:  The properties returned by the
> _DSM only ever have one of two types, Integer or Buffer.  The former
> is
> retrievable as usual with device_property_read_u64(), but the latter
> is
> not part of the _DSD spec and it is not possible to retrieve Buffer
> properties with the device_property_read_*() functions due to the type
> checking performed in drivers/acpi/property.c.  It is however possible
> to retrieve them with acpi_dev_get_property().  Apple is using the
> Buffer type somewhat sloppily to store null-terminated strings but
> also
> integers.  The real data type is not distinguishable by the ACPI core
> and the onus is on the caller to use the contents of the Buffer in an
> appropriate way.
> 
> In case Apple moves to _DSD in the future, this commit first checks
> for
> _DSD and falls back to _DSM only if _DSD is not found.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

(though most of the comments were given on Github page)

> Cc: Federico Lorenzi <florenzi@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Ronald Tschalär <ronald@innovation.ch>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> Changes v2 -> v3:
> - Use bitmap to keep track of valid properties.  Move to x86/apple.c
>   to avoid cluttering up generic ACPI code. (Andy, Rafael)
> 
>  drivers/acpi/Makefile    |   1 +
>  drivers/acpi/internal.h  |   6 +++
>  drivers/acpi/property.c  |   3 ++
>  drivers/acpi/x86/apple.c | 137
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 147 insertions(+)
>  create mode 100644 drivers/acpi/x86/apple.c
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index b1aacfc62b1f..90265ab4437a 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -50,6 +50,7 @@ acpi-$(CONFIG_ACPI_REDUCED_HARDWARE_ONLY) += evged.o
>  acpi-y				+= sysfs.o
>  acpi-y				+= property.o
>  acpi-$(CONFIG_X86)		+= acpi_cmos_rtc.o
> +acpi-$(CONFIG_X86)		+= x86/apple.o
>  acpi-$(CONFIG_X86)		+= x86/utils.o
>  acpi-$(CONFIG_DEBUG_FS)		+= debugfs.o
>  acpi-$(CONFIG_ACPI_NUMA)	+= numa.o
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index be79f7db1850..79ee29777909 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -229,6 +229,12 @@ static inline void suspend_nvs_restore(void) {}
>  void acpi_init_properties(struct acpi_device *adev);
>  void acpi_free_properties(struct acpi_device *adev);
>  
> +#ifdef CONFIG_X86
> +void acpi_extract_apple_properties(struct acpi_device *adev);
> +#else
> +static inline void acpi_extract_apple_properties(struct acpi_device
> *adev) {}
> +#endif
> +
>  /*-------------------------------------------------------------------
> -------
>  				Watchdog
>    -------------------------------------------------------------------
> ------- */
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 27a9294c843c..d05f4f97c963 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -375,6 +375,9 @@ void acpi_init_properties(struct acpi_device
> *adev)
>  	if (acpi_of && !adev->flags.of_compatible_ok)
>  		acpi_handle_info(adev->handle,
>  			 ACPI_DT_NAMESPACE_HID " requires
> 'compatible' property\n");
> +
> +	if (is_apple_system && !adev->data.pointer)
> +		acpi_extract_apple_properties(adev);
>  }
>  
>  static void acpi_destroy_nondev_subnodes(struct list_head *list)
> diff --git a/drivers/acpi/x86/apple.c b/drivers/acpi/x86/apple.c
> new file mode 100644
> index 000000000000..bffa38f7460e
> --- /dev/null
> +++ b/drivers/acpi/x86/apple.c
> @@ -0,0 +1,137 @@
> +/*
> + * apple.c - Apple ACPI quirks
> + * Copyright (C) 2017 Lukas Wunner <lukas@wunner.de>
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License (version 2)
> as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bitmap.h>
> +#include <linux/uuid.h>
> +
> +/* Apple _DSM device properties GUID */
> +static const guid_t apple_prp_guid =
> +	GUID_INIT(0xA0B5B7C6, 0x1318, 0x441C,
> +		  0xB0, 0xC9, 0xFE, 0x69, 0x5E, 0xAF, 0x94, 0x9B);
> +
> +/**
> + * acpi_extract_apple_properties - retrieve and convert Apple _DSM
> properties
> + * @adev: ACPI device for which to retrieve the properties
> + *
> + * Invoke Apple's custom _DSM once to check the protocol version and
> once more
> + * to retrieve the properties.  They are marshalled up in a single
> package as
> + * alternating key/value elements, unlike _DSD which stores them as a
> package
> + * of 2-element packages.  Convert to _DSD format and make them
> available under
> + * the primary fwnode.
> + */
> +void acpi_extract_apple_properties(struct acpi_device *adev)
> +{
> +	unsigned int i, j = 0, newsize = 0, numprops, numvalid;
> +	union acpi_object *props, *newprops;
> +	unsigned long *valid = NULL;
> +	void *free_space;
> +
> +	props = acpi_evaluate_dsm_typed(adev->handle,
> &apple_prp_guid, 1, 0,
> +					NULL, ACPI_TYPE_BUFFER);
> +	if (!props)
> +		return;
> +
> +	if (!props->buffer.length)
> +		goto out_free;
> +
> +	if (props->buffer.pointer[0] != 3) {
> +		acpi_handle_info(adev->handle, FW_INFO
> +				 "unsupported properties version
> %*ph\n",
> +				 props->buffer.length, props-
> >buffer.pointer);
> +		goto out_free;
> +	}
> +
> +	ACPI_FREE(props);
> +	props = acpi_evaluate_dsm_typed(adev->handle,
> &apple_prp_guid, 1, 1,
> +					NULL, ACPI_TYPE_PACKAGE);
> +	if (!props)
> +		return;
> +
> +	numprops = props->package.count / 2;
> +	if (!numprops)
> +		goto out_free;
> +
> +	valid = kcalloc(BITS_TO_LONGS(numprops), sizeof(long),
> GFP_KERNEL);
> +	if (!valid)
> +		goto out_free;
> +
> +	/* newsize = key length + value length of each tuple */
> +	for (i = 0; i < numprops; i++) {
> +		union acpi_object *key = &props->package.elements[i *
> 2];
> +		union acpi_object *val = &props->package.elements[i *
> 2 + 1];
> +
> +		if ( key->type != ACPI_TYPE_STRING ||
> +		    (val->type != ACPI_TYPE_INTEGER &&
> +		     val->type != ACPI_TYPE_BUFFER))
> +			continue; /* skip invalid properties */
> +
> +		__set_bit(i, valid);
> +		newsize += key->string.length + 1;
> +		if ( val->type == ACPI_TYPE_BUFFER)
> +			newsize += val->buffer.length;
> +	}
> +
> +	numvalid = bitmap_weight(valid, numprops);
> +	if (numprops > numvalid)
> +		acpi_handle_info(adev->handle, FW_INFO
> +				 "skipped %u properties: wrong
> type\n",
> +				 numprops - numvalid);
> +	if (numvalid == 0)
> +		goto out_free;
> +
> +	/* newsize += top-level package + 3 objects for each
> key/value tuple */
> +	newsize	+= (1 + 3 * numvalid) * sizeof(union
> acpi_object);
> +	newprops = ACPI_ALLOCATE_ZEROED(newsize);
> +	if (!newprops)
> +		goto out_free;
> +
> +	/* layout: top-level package | packages | key/value tuples |
> strings */
> +	newprops->type = ACPI_TYPE_PACKAGE;
> +	newprops->package.count = numvalid;
> +	newprops->package.elements = &newprops[1];
> +	free_space = &newprops[1 + 3 * numvalid];
> +
> +	for_each_set_bit(i, valid, numprops) {
> +		union acpi_object *key = &props->package.elements[i *
> 2];
> +		union acpi_object *val = &props->package.elements[i *
> 2 + 1];
> +		unsigned int k = 1 + numvalid + j * 2; /* index into
> newprops */
> +		unsigned int v = k + 1;
> +
> +		newprops[1 + j].type = ACPI_TYPE_PACKAGE;
> +		newprops[1 + j].package.count = 2;
> +		newprops[1 + j].package.elements = &newprops[k];
> +
> +		newprops[k].type = ACPI_TYPE_STRING;
> +		newprops[k].string.length = key->string.length;
> +		newprops[k].string.pointer = free_space;
> +		memcpy(free_space, key->string.pointer, key-
> >string.length);
> +		free_space += key->string.length + 1;
> +
> +		newprops[v].type = val->type;
> +		if (val->type == ACPI_TYPE_INTEGER) {
> +			newprops[v].integer.value = val-
> >integer.value;
> +		} else {
> +			newprops[v].buffer.length = val-
> >buffer.length;
> +			newprops[v].buffer.pointer = free_space;
> +			memcpy(free_space, val->buffer.pointer,
> +			       val->buffer.length);
> +			free_space += val->buffer.length;
> +		}
> +		j++; /* count valid properties */
> +	}
> +	WARN_ON(free_space != (void *)newprops + newsize);
> +
> +	adev->data.properties = newprops;
> +	adev->data.pointer = newprops;
> +
> +out_free:
> +	ACPI_FREE(props);
> +	kfree(valid);
> +}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 5/6] ACPI / scan: Recognize Apple SPI and I2C slaves

[Lukas Wunner]

SPI and I2C slaves are enumerated by their respective parents rather
than the ACPI core.  They are recognized by presence of _CRS resources,
which however are missing on Macs.  Check for presence of device
properties instead.

Cc: Federico Lorenzi <florenzi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Reported-and-tested-by: Ronald Tschalär <ronald-Cgq6lnktLNMTaf21n8AfGQ@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
Changes v2 -> v3:
- Use fwnode_property_present() instead of device_property_present(),
  the latter doesn't work as the fwnode pointer of the struct device
  embedded in a struct acpi_device is always NULL. (Ronald)

 drivers/acpi/scan.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 59ebbd5f7b83..e84e5eb23222 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1452,6 +1452,12 @@ 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 */
+	if (is_apple_system &&
+	    (fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
+	     fwnode_property_present(&device->fwnode, "i2cAddress")))
+		return true;
+
 	INIT_LIST_HEAD(&resource_list);
 	acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
 			       &is_spi_i2c_slave);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Lukas Wunner]

We're about to amend device scan with multiple checks whether we're
running on a Mac.  Speed that up by performing the DMI match only once
and caching the result.

Switch over existing Apple DMI checks, thereby fixing two deficiencies:

* They only match "Apple Inc." but not "Apple Computer, Inc.", which is
  used by BIOSes released between January 2006 (when the first x86 Macs
  started shipping) and January 2007 (when the company name changed upon
  introduction of the iPhone).

* They are now #defined to false on non-x86 arches and can thus be
  optimized away by the compiler.

Suggested-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
Changes v2 -> v3:
- Newly inserted patch in v3 to avoid repeated DMI checks for Apple
  hardware:  The result of the first DMI check in osi.c is cached.
  Two other existing DMI checks are converted to use the result.
  Because one of them is in a module (sbs.ko), the bool is_apple_system
  needs to be exported.  On non-x86, the DMI checks and Apple-specific
  code are omitted altogether. (Andy, Rafael)

 drivers/acpi/osi.c      |  4 ++++
 drivers/acpi/pci_root.c |  3 +--
 drivers/acpi/sbs.c      | 24 +-----------------------
 include/linux/acpi.h    |  6 ++++++
 4 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
index cd953ae10238..6c253d4006b4 100644
--- a/drivers/acpi/osi.c
+++ b/drivers/acpi/osi.c
@@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void)
 EXPORT_SYMBOL(acpi_osi_is_win8);
 
 #ifdef CONFIG_X86
+bool is_apple_system;
+EXPORT_SYMBOL(is_apple_system);
+
 static void __init acpi_osi_dmi_darwin(bool enable,
 				       const struct dmi_system_id *d)
 {
 	pr_notice("DMI detected to setup _OSI(\"Darwin\"): %s\n", d->ident);
 	osi_config.darwin_dmi = 1;
 	__acpi_osi_setup_darwin(enable);
+	is_apple_system = true;
 }
 
 static void __init acpi_osi_dmi_linux(bool enable,
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 9eec3095e6c3..1b6341717820 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -431,8 +431,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm)
 	 * been called successfully. We know the feature set supported by the
 	 * platform, so avoid calling _OSC at all
 	 */
-
-	if (dmi_match(DMI_SYS_VENDOR, "Apple Inc.")) {
+	if (is_apple_system) {
 		root->osc_control_set = ~OSC_PCI_EXPRESS_PME_CONTROL;
 		decode_osc_control(root, "OS assumes control of",
 				   root->osc_control_set);
diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index ad0b13ad4bbb..9b945ae0037e 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -31,7 +31,6 @@
 #include <linux/jiffies.h>
 #include <linux/delay.h>
 #include <linux/power_supply.h>
-#include <linux/dmi.h>
 
 #include "sbshc.h"
 #include "battery.h"
@@ -58,8 +57,6 @@ static unsigned int cache_time = 1000;
 module_param(cache_time, uint, 0644);
 MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
 
-static bool sbs_manager_broken;
-
 #define MAX_SBS_BAT			4
 #define ACPI_SBS_BLOCK_MAX		32
 
@@ -632,31 +629,12 @@ static void acpi_sbs_callback(void *context)
 	}
 }
 
-static int disable_sbs_manager(const struct dmi_system_id *d)
-{
-	sbs_manager_broken = true;
-	return 0;
-}
-
-static struct dmi_system_id acpi_sbs_dmi_table[] = {
-	{
-		.callback = disable_sbs_manager,
-		.ident = "Apple",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc.")
-		},
-	},
-	{ },
-};
-
 static int acpi_sbs_add(struct acpi_device *device)
 {
 	struct acpi_sbs *sbs;
 	int result = 0;
 	int id;
 
-	dmi_check_system(acpi_sbs_dmi_table);
-
 	sbs = kzalloc(sizeof(struct acpi_sbs), GFP_KERNEL);
 	if (!sbs) {
 		result = -ENOMEM;
@@ -677,7 +655,7 @@ static int acpi_sbs_add(struct acpi_device *device)
 
 	result = 0;
 
-	if (!sbs_manager_broken) {
+	if (!is_apple_system) {
 		result = acpi_manager_get_info(sbs);
 		if (!result) {
 			sbs->manager_present = 1;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index cafdfb84ca28..d068cee890ee 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -389,6 +389,12 @@ extern int acpi_blacklisted(void);
 extern void acpi_osi_setup(char *str);
 extern bool acpi_osi_is_win8(void);
 
+#ifdef CONFIG_X86
+extern bool is_apple_system;
+#else
+#define is_apple_system false
+#endif
+
 #ifdef CONFIG_ACPI_NUMA
 int acpi_map_pxm_to_online_node(int pxm);
 int acpi_get_node(acpi_handle handle);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Rafael J. Wysocki]

On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:
> We're about to amend device scan with multiple checks whether we're
> running on a Mac.  Speed that up by performing the DMI match only once
> and caching the result.
> 
> Switch over existing Apple DMI checks, thereby fixing two deficiencies:
> 
> * They only match "Apple Inc." but not "Apple Computer, Inc.", which is
>   used by BIOSes released between January 2006 (when the first x86 Macs
>   started shipping) and January 2007 (when the company name changed upon
>   introduction of the iPhone).
> 
> * They are now #defined to false on non-x86 arches and can thus be
>   optimized away by the compiler.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> Changes v2 -> v3:
> - Newly inserted patch in v3 to avoid repeated DMI checks for Apple
>   hardware:  The result of the first DMI check in osi.c is cached.
>   Two other existing DMI checks are converted to use the result.
>   Because one of them is in a module (sbs.ko), the bool is_apple_system
>   needs to be exported.  On non-x86, the DMI checks and Apple-specific
>   code are omitted altogether. (Andy, Rafael)
> 
>  drivers/acpi/osi.c      |  4 ++++
>  drivers/acpi/pci_root.c |  3 +--
>  drivers/acpi/sbs.c      | 24 +-----------------------
>  include/linux/acpi.h    |  6 ++++++
>  4 files changed, 12 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
> index cd953ae10238..6c253d4006b4 100644
> --- a/drivers/acpi/osi.c
> +++ b/drivers/acpi/osi.c
> @@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void)
>  EXPORT_SYMBOL(acpi_osi_is_win8);
>  
>  #ifdef CONFIG_X86
> +bool is_apple_system;
> +EXPORT_SYMBOL(is_apple_system);

Maybe prepend the name of this variable with acpi_ to indicate that this is
ACPI-specific.

Other than that, I'm basically fine with the changes in this series, but
I need to have a closer look at them.

Thanks,
Rafael

Re: [PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Lukas Wunner]

On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote:
> On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:
> > --- a/drivers/acpi/osi.c
> > +++ b/drivers/acpi/osi.c
> > @@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void)
> >  EXPORT_SYMBOL(acpi_osi_is_win8);
> >  
> >  #ifdef CONFIG_X86
> > +bool is_apple_system;
> > +EXPORT_SYMBOL(is_apple_system);
> 
> Maybe prepend the name of this variable with acpi_ to indicate that this is
> ACPI-specific.

It's not really ACPI-specific, osi.c just happens to be the best place
to set the variable because the acpi_osi_dmi_table[] is checked very
early during boot.  So early in fact, that I could even replace the
existing Apple DMI check in arch/x86/kernel/early-quirks.c with
"is_apple_system".

These non-ACPI files currently contain an Apple DMI check:
arch/x86/kernel/early-quirks.c
drivers/pci/quirks.c (2x)
drivers/firmware/efi/apple-properties.c
drivers/thunderbolt/tb.c
drivers/thunderbolt/icm.c

The latter three do not #include <linux/acpi.h> yet.  Somehow it feels
odd to include that header to check for presence of an Apple system
because that's not really ACPI-related.

I guess I could introduce a new <linux/apple.h> but I hate the insane
proliferation of additional files in include/linux/.  I could merge
the contents of apple_bl.h and apple-gmux.h into that new header to
reduce the number of files a bit.

Struggling to find a solution that's nice and clean.  Any ideas?

Thanks,

Lukas

Re: [PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Rafael J. Wysocki]

On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote:
> On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote:
>> On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:
>> > --- a/drivers/acpi/osi.c
>> > +++ b/drivers/acpi/osi.c
>> > @@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void)
>> >  EXPORT_SYMBOL(acpi_osi_is_win8);
>> >
>> >  #ifdef CONFIG_X86
>> > +bool is_apple_system;
>> > +EXPORT_SYMBOL(is_apple_system);
>>
>> Maybe prepend the name of this variable with acpi_ to indicate that this is
>> ACPI-specific.
>
> It's not really ACPI-specific, osi.c just happens to be the best place
> to set the variable because the acpi_osi_dmi_table[] is checked very
> early during boot.  So early in fact, that I could even replace the
> existing Apple DMI check in arch/x86/kernel/early-quirks.c with
> "is_apple_system".

OK

> These non-ACPI files currently contain an Apple DMI check:
> arch/x86/kernel/early-quirks.c
> drivers/pci/quirks.c (2x)
> drivers/firmware/efi/apple-properties.c
> drivers/thunderbolt/tb.c
> drivers/thunderbolt/icm.c
>
> The latter three do not #include <linux/acpi.h> yet.  Somehow it feels
> odd to include that header to check for presence of an Apple system
> because that's not really ACPI-related.
>
> I guess I could introduce a new <linux/apple.h> but I hate the insane
> proliferation of additional files in include/linux/.

There is include/linux/platform_data/x86/ so maybe put it in there?

>  I could merge the contents of apple_bl.h and apple-gmux.h into that new header to
> reduce the number of files a bit.
>
> Struggling to find a solution that's nice and clean.  Any ideas?

I guess you still want it to work if someone configures the kernel
without CONFIG_ACPI, although that's slightly debatable, so the
variable should be defined somewhere in the arch code I suppose.

I also guess you could add something like arch/x86/platform/apple/ and
put the checks and the variable in there (in which case I'd call it
x86_apple_machine or similar).

Then invoke the check from acpi_osi_dmi_table[] code and the
early-quirks.c one is only needed for !CONFIG_ACPI.

Thanks,
Rafael

Re: [PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Andy Shevchenko]

On Thu, 2017-07-20 at 16:27 +0200, Rafael J. Wysocki wrote:
> On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote:
> > > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:
> > > > 


> > I guess I could introduce a new <linux/apple.h> but I hate the
> > insane
> > proliferation of additional files in include/linux/.
> 
> There is include/linux/platform_data/x86/ so maybe put it in there?

Just suggested the same :-)

> >  I could merge the contents of apple_bl.h and apple-gmux.h into that
> > new header to
> > reduce the number of files a bit.
> > 
> > Struggling to find a solution that's nice and clean.  Any ideas?
> 
> I guess you still want it to work if someone configures the kernel
> without CONFIG_ACPI, although that's slightly debatable, so the
> variable should be defined somewhere in the arch code I suppose.
> 
> I also guess you could add something like arch/x86/platform/apple/ and
> put the checks and the variable in there (in which case I'd call it
> x86_apple_machine or similar).

I'm not sure if we can use drivers/platform/x86 for this, either agreed
way is fine to me.

Darren?

> 
> Then invoke the check from acpi_osi_dmi_table[] code and the
> early-quirks.c one is only needed for !CONFIG_ACPI.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Rafael J. Wysocki]

On Thursday, July 20, 2017 05:33:43 PM Andy Shevchenko wrote:
> On Thu, 2017-07-20 at 16:27 +0200, Rafael J. Wysocki wrote:
> > On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote:
> > > > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:
> > > > > 
> 
> 
> > > I guess I could introduce a new <linux/apple.h> but I hate the
> > > insane
> > > proliferation of additional files in include/linux/.
> > 
> > There is include/linux/platform_data/x86/ so maybe put it in there?
> 
> Just suggested the same :-)
> 
> > >  I could merge the contents of apple_bl.h and apple-gmux.h into that
> > > new header to
> > > reduce the number of files a bit.
> > > 
> > > Struggling to find a solution that's nice and clean.  Any ideas?
> > 
> > I guess you still want it to work if someone configures the kernel
> > without CONFIG_ACPI, although that's slightly debatable, so the
> > variable should be defined somewhere in the arch code I suppose.
> > 
> > I also guess you could add something like arch/x86/platform/apple/ and
> > put the checks and the variable in there (in which case I'd call it
> > x86_apple_machine or similar).
> 
> I'm not sure if we can use drivers/platform/x86 for this, either agreed
> way is fine to me.

No, because of the ACPI involvement.  That needs to go under arch/ IMO.

Thanks,
Rafael

Re: [PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Darren Hart]

On Thu, Jul 20, 2017 at 04:49:48PM +0200, Rafael Wysocki wrote:
> On Thursday, July 20, 2017 05:33:43 PM Andy Shevchenko wrote:
> > On Thu, 2017-07-20 at 16:27 +0200, Rafael J. Wysocki wrote:
> > > On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote:
> > > > > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:
> > > > > > 
> > 
> > 
> > > > I guess I could introduce a new <linux/apple.h> but I hate the
> > > > insane
> > > > proliferation of additional files in include/linux/.
> > > 
> > > There is include/linux/platform_data/x86/ so maybe put it in there?
> > 
> > Just suggested the same :-)

Yes please, this is precisely why we created this directory.

> > 
> > > >  I could merge the contents of apple_bl.h and apple-gmux.h into that
> > > > new header to
> > > > reduce the number of files a bit.
> > > > 
> > > > Struggling to find a solution that's nice and clean.  Any ideas?
> > > 
> > > I guess you still want it to work if someone configures the kernel
> > > without CONFIG_ACPI, although that's slightly debatable, so the
> > > variable should be defined somewhere in the arch code I suppose.
> > > 
> > > I also guess you could add something like arch/x86/platform/apple/ and
> > > put the checks and the variable in there (in which case I'd call it
> > > x86_apple_machine or similar).
> > 
> > I'm not sure if we can use drivers/platform/x86 for this, either agreed
> > way is fine to me.
> 
> No, because of the ACPI involvement.  That needs to go under arch/ IMO.

We have an example, silead_dmi.c, which extracts properties based on the DMI
match, and adds them to the i2c client device. While I suppose we could do
something like this, the drivers affected are really not "platform drivers", but
rather common drivers with platform specific properties. I concur with Rafael.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 2/6] ACPI / x86: Consolidate Apple DMI checks

[Andy Shevchenko]

On Thu, 2017-07-20 at 16:03 +0200, Lukas Wunner wrote:
> On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote:
> > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:

> I guess I could introduce a new <linux/apple.h> but I hate the insane
> proliferation of additional files in include/linux/.  I could merge
> the contents of apple_bl.h and apple-gmux.h into that new header to
> reduce the number of files a bit.

Just a side comment:

include/platform_data/x86/apple.h in this case

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 3/6] ACPI / property: Don't evaluate objects for devices w/o handle

[Lukas Wunner]

Fabricated devices such as LNXPWRBN lack a handle, causing evaluation
of _CCA and _DSD to always fail with AE_BAD_PARAMETER.  While that is
merely a (negligible) waste of processing power, evaluating a _DSM for
them (such as Apple's device properties _DSM which we're about to add)
results in an ugly error:

    ACPI: \: failed to evaluate _DSM (0x1001)

Avoid by not evaluating _DSD and the upcoming _DSM for devices without
handle.

Cc: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Acked-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
 drivers/acpi/property.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 9364398204e9..27a9294c843c 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -338,6 +338,9 @@ void acpi_init_properties(struct acpi_device *adev)
 
 	INIT_LIST_HEAD(&adev->data.subnodes);
 
+	if (!adev->handle)
+		return;
+
 	/*
 	 * Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
 	 * Device Tree compatible properties for this device.
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] ACPI / property: Don't evaluate objects for devices w/o handle

[Rafael J. Wysocki]

On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote:
> Fabricated devices such as LNXPWRBN lack a handle, causing evaluation
> of _CCA and _DSD to always fail with AE_BAD_PARAMETER.  While that is
> merely a (negligible) waste of processing power, evaluating a _DSM for
> them (such as Apple's device properties _DSM which we're about to add)
> results in an ugly error:
> 
>     ACPI: \: failed to evaluate _DSM (0x1001)
> 
> Avoid by not evaluating _DSD and the upcoming _DSM for devices without
> handle.
> 
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/acpi/property.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 9364398204e9..27a9294c843c 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -338,6 +338,9 @@ void acpi_init_properties(struct acpi_device *adev)
>  
>  	INIT_LIST_HEAD(&adev->data.subnodes);
>  
> +	if (!adev->handle)
> +		return;
> +
>  	/*
>  	 * Check if ACPI_DT_NAMESPACE_HID is present and inthat case we fill in
>  	 * Device Tree compatible properties for this device.
> 

This can go in separately AFAICS.

Thanks,
Rafael

[PATCH v3 1/6] ACPI / osi: Exclude x86 DMI quirks on other arches

[Lukas Wunner]

All existing quirks in acpi_osi_dmi_table[] as well as their callbacks
are specific to x86 so exclude them on other arches.

The simplest approach is to #ifdef the DMI quirks.  An alternative would
be to declare a __weak empty table which can be overridden per arch.

The ability to specify "acpi_osi=Linux" and "acpi_osi=Darwin" on the
command line is retained, though its usefulness on non-x86 arches is
debatable.  (Do we really need to return false to _OSI(Linux) on
non-x86 arches as well?)

Beyond that the ACPI core contains various other x86-specific quirks
(such as video_detect_dmi_table[]) and a bigger effort is needed to
achieve a clean separation of generic and arch-specific code.

Cc: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes v2 -> v3:
- Newly inserted patch in v3 to avoid repeated DMI checks for Apple
  hardware:  The result of the first DMI check in osi.c is cached.
  Two other existing DMI checks are converted to use the result.
  Because one of them is in a module (sbs.ko), the bool is_apple_system
  needs to be exported.  On non-x86, the DMI checks and Apple-specific
  code are omitted altogether. (Andy, Rafael)

 drivers/acpi/osi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c
index 723bee58bbcf..cd953ae10238 100644
--- a/drivers/acpi/osi.c
+++ b/drivers/acpi/osi.c
@@ -257,6 +257,7 @@ bool acpi_osi_is_win8(void)
 }
 EXPORT_SYMBOL(acpi_osi_is_win8);
 
+#ifdef CONFIG_X86
 static void __init acpi_osi_dmi_darwin(bool enable,
 				       const struct dmi_system_id *d)
 {
@@ -312,6 +313,7 @@ static int __init dmi_disable_osi_win8(const struct dmi_system_id *d)
 
 	return 0;
 }
+#endif /* CONFIG_X86 */
 
 /*
  * Linux default _OSI response behavior is determined by this DMI table.
@@ -320,6 +322,7 @@ static int __init dmi_disable_osi_win8(const struct dmi_system_id *d)
  * by acpi_osi=!Linux/acpi_osi=!Darwin command line options.
  */
 static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
+#ifdef CONFIG_X86
 	{
 	.callback = dmi_disable_osi_vista,
 	.ident = "Fujitsu Siemens",
@@ -499,6 +502,7 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
 		     DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."),
 		},
 	},
+#endif /* CONFIG_X86 */
 	{}
 };
 
-- 
2.11.0