[PATCH v2 0/4] Apple SPI properties

[PATCH v2 0/4] 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, v2.

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

The series has now been tested successfully by Ronald.

Changes v1 -> v2:

Patch 2:
- Rebase on uuid-types branch. (Andy, Rafael)
- Don't align assignments. (Mika)
- Move IS_ENABLED() and dmi_match() conditionals into callee. (Mika)
- Tone down messages indicating an unsupported properties version or
  illegal property type to KERN_INFO to avoid scaring or annoying users
  who may see it on every boot despite "quiet", but still allowing
  diagnosis of issues that may ensue. (Mika)

Patch 3:
- Newly inserted patch in v2 to fix an enumeration issue. (Ronald)

Patch 4 (was patch 3 in v1):
- Move IS_ENABLED() and dmi_match() conditionals into callee. (Mika)
- Allow acpi_spi_parse_apple_properties() to augment or override _CRS
  by calling it unconditionally rather than only if _CRS is empty.

Thanks,

Lukas


Lukas Wunner (4):
  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/property.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c     |   6 +++
 drivers/spi/spi.c       |  30 ++++++++++++
 3 files changed, 162 insertions(+)

-- 
2.11.0

[PATCH v2 1/4] 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: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Andy Shevchenko <andriy.shevchenko-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 917c789f953d..116bfc1937b5 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -339,6 +339,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

[PATCH v2 2/4] 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: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Federico Lorenzi <florenzi@gmail.com>
Tested-by: Ronald Tschalär <ronald@innovation.ch>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes v1 -> v2:
- Rebase on uuid-types branch. (Andy, Rafael)
- Don't align assignments. (Mika)
- Move IS_ENABLED() and dmi_match() conditionals into callee. (Mika)
- Tone down messages indicating an unsupported properties version or
  illegal property type to KERN_INFO to avoid scaring or annoying users
  who may see it on every boot despite "quiet", but still allowing
  diagnosis of issues that may ensue. (Mika)

 drivers/acpi/property.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 116bfc1937b5..6a56f3a85f18 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -15,7 +15,9 @@
 
 #include <linux/acpi.h>
 #include <linux/device.h>
+#include <linux/dmi.h>
 #include <linux/export.h>
+#include <linux/uuid.h>
 
 #include "internal.h"
 
@@ -34,6 +36,124 @@ static const u8 ads_uuid[16] = {
 	0xe6, 0xe3, 0xb8, 0xdb, 0x86, 0x58, 0xa6, 0x4b,
 	0x87, 0x95, 0x13, 0x19, 0xf5, 0x2a, 0x96, 0x6b
 };
+/* 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_retrieve_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.
+ */
+static void acpi_retrieve_apple_properties(struct acpi_device *adev)
+{
+	unsigned int i, j, newsize = 0, numprops, skipped = 0;
+	union acpi_object *props, *newprops;
+	void *free_space;
+
+	if (!IS_ENABLED(CONFIG_X86) || !dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
+		return;
+
+	props = acpi_evaluate_dsm_typed(adev->handle, &apple_prp_guid, 1, 0,
+					NULL, ACPI_TYPE_BUFFER);
+	if (!props)
+		return;
+
+	if (!props->buffer.length || 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;
+
+	/* newsize = key length + value length of each tuple */
+	numprops = props->package.count / 2;
+	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)) {
+			key->type = ACPI_TYPE_ANY; /* mark as to be skipped */
+			skipped++;
+			continue;
+		}
+		newsize += key->string.length + 1;
+		if ( val->type == ACPI_TYPE_BUFFER)
+			newsize += val->buffer.length;
+	}
+
+	if (skipped)
+		acpi_handle_info(adev->handle, FW_INFO
+				 "skipped %u properties: wrong type\n",
+				 skipped);
+	if (skipped == numprops)
+		goto out_free;
+
+	/* newsize += top-level package + 3 objects for each key/value tuple */
+	newsize	+= (1 + 3 * (numprops - skipped)) * 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 = numprops - skipped;
+	newprops->package.elements = &newprops[1];
+	free_space = &newprops[1 + 3 * (numprops - skipped)];
+
+	for (i = 0, j = 0; i < numprops; i++) {
+		union acpi_object *key = &props->package.elements[i * 2];
+		union acpi_object *val = &props->package.elements[i * 2 + 1];
+		unsigned int k = (1 + numprops - skipped) + j * 2;
+		unsigned int v = k + 1; /* index into newprops */
+
+		if (key->type == ACPI_TYPE_ANY)
+			continue; /* skipped */
+
+		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++; /* not incremented for skipped properties */
+	}
+	WARN_ON(free_space != (void *)newprops + newsize);
+
+	adev->data.properties = newprops;
+	adev->data.pointer = newprops;
+
+out_free:
+	ACPI_FREE(props);
+}
 
 static bool acpi_enumerate_nondev_subnodes(acpi_handle scope,
 					   const union acpi_object *desc,
@@ -376,6 +496,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 (!adev->data.pointer)
+		acpi_retrieve_apple_properties(adev);
 }
 
 static void acpi_destroy_nondev_subnodes(struct list_head *list)
-- 
2.11.0

[PATCH v2 3/4] 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: Rafael J. Wysocki <rafael.j.wysocki-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Federico Lorenzi <florenzi-Re5JQEeQqe8AvxtiuMwx3w@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 v1 -> v2:
- Newly inserted patch in v2 to fix an enumeration issue. (Ronald)

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

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index d4ff2cd1f738..c565e74bd2dd 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1444,6 +1444,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_ENABLED(CONFIG_X86) && dmi_match(DMI_SYS_VENDOR, "Apple Inc.") &&
+	    (device_property_present(&device->dev, "spiSclkPeriod") ||
+	     device_property_present(&device->dev, "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 v2 4/4] 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: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
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>
Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
---
Changes v1 -> v2:
- Move IS_ENABLED() and dmi_match() conditionals into callee. (Mika)
- Allow acpi_spi_parse_apple_properties() to augment or override _CRS
  by calling it unconditionally rather than only if _CRS is empty.

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

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 89254a55eb2e..51b7bdf841e4 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -21,6 +21,8 @@
 #include <linux/cache.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
+#include <linux/dmi.h>
+#include <linux/math64.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
@@ -1685,6 +1687,32 @@ static void of_register_spi_devices(struct spi_master *master) { }
 #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 *o;
+
+	if (!IS_ENABLED(CONFIG_X86) || !dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
+		return;
+
+	if (!acpi_dev_get_property(dev, "spiSclkPeriod", ACPI_TYPE_BUFFER, &o))
+		spi->max_speed_hz  = div64_u64(NSEC_PER_SEC,
+					       *(u64 *)o->buffer.pointer);
+
+	if (!acpi_dev_get_property(dev, "spiWordSize", ACPI_TYPE_BUFFER, &o))
+		spi->bits_per_word = *(u64 *)o->buffer.pointer;
+
+	if (!acpi_dev_get_property(dev, "spiBitOrder", ACPI_TYPE_BUFFER, &o) &&
+	    !*(u64 *)o->buffer.pointer)
+		spi->mode |= SPI_LSB_FIRST;
+	if (!acpi_dev_get_property(dev, "spiSPO", ACPI_TYPE_BUFFER, &o) &&
+	     *(u64 *)o->buffer.pointer)
+		spi->mode |= SPI_CPOL;
+	if (!acpi_dev_get_property(dev, "spiSPH", ACPI_TYPE_BUFFER, &o) &&
+	     *(u64 *)o->buffer.pointer)
+		spi->mode |= SPI_CPHA;
+}
+
 static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 {
 	struct spi_device *spi = data;
@@ -1758,6 +1786,8 @@ static acpi_status acpi_register_spi_device(struct spi_master *master,
 				     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

--
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 v2 4/4] spi: Use Apple device properties in absence of ACPI resources

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

On Wed, Jun 28, 2017 at 07:20:19PM +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.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] ACPI / property: Support Apple _DSM properties

[Andy Shevchenko]

On Wed, Jun 28, 2017 at 8:20 PM, Lukas Wunner <lukas@wunner.de> 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).

> +/**
> + * acpi_retrieve_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.
> + */
> +static void acpi_retrieve_apple_properties(struct acpi_device *adev)
> +{
> +       unsigned int i, j, newsize = 0, numprops, skipped = 0;
> +       union acpi_object *props, *newprops;
> +       void *free_space;
> +

> +       if (!IS_ENABLED(CONFIG_X86) || !dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
> +               return;

You are using this in few places, perhaps it makes sense to do in
(maybe) header like
drivers/acpi/apple.h

static inline bool is_apple_system(void)
{
   return IS_ENABLED(CONFIG_X86) && dmi_match(DMI_SYS_VENDOR, "Apple Inc.");
}

I know this makes more LOCs, the rationale is to keep such
deterministic things in one place (basically maintenance).

Rafael, Mika?

> +       /* newsize = key length + value length of each tuple */
> +       numprops = props->package.count / 2;
> +       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)) {
> +                       key->type = ACPI_TYPE_ANY; /* mark as to be skipped */
> +                       skipped++;
> +                       continue;
> +               }
> +               newsize += key->string.length + 1;
> +               if ( val->type == ACPI_TYPE_BUFFER)
> +                       newsize += val->buffer.length;
> +       }
> +
> +       if (skipped)
> +               acpi_handle_info(adev->handle, FW_INFO
> +                                "skipped %u properties: wrong type\n",
> +                                skipped);
> +       if (skipped == numprops)
> +               goto out_free;
> +
> +       /* newsize += top-level package + 3 objects for each key/value tuple */
> +       newsize += (1 + 3 * (numprops - skipped)) * 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 = numprops - skipped;
> +       newprops->package.elements = &newprops[1];
> +       free_space = &newprops[1 + 3 * (numprops - skipped)];
> +

> +       for (i = 0, j = 0; i < numprops; i++) {

Instead I would rather to create a bitmask above and do here something like

for_each_bit_set()

(Note, we have a lot of helpers to work with bitmaps)

> +               union acpi_object *key = &props->package.elements[i * 2];
> +               union acpi_object *val = &props->package.elements[i * 2 + 1];

> +               unsigned int k = (1 + numprops - skipped) + j * 2;
> +               unsigned int v = k + 1; /* index into newprops */

...and this rather complex arithmetics will go away.

> +
> +               if (key->type == ACPI_TYPE_ANY)
> +                       continue; /* skipped */

...and this.

> +
> +               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 {

checkpatch.pl ?

> +                       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++; /* not incremented for skipped properties */

> +       }

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jun 28, 2017 at 8:20 PM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> 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.

> spiSclkPeriod   /* period in ns */

> +#include <linux/math64.h>

> +       if (!acpi_dev_get_property(dev, "spiSclkPeriod", ACPI_TYPE_BUFFER, &o))
> +               spi->max_speed_hz  = div64_u64(NSEC_PER_SEC,
> +                                              *(u64 *)o->buffer.pointer);

Why do you need 64 bit division here? It's obviously 32. It makes
nonsense to have variable more than NSEC_PER_SEC (if one paranoid
enough it could be checked).

> +       if (!acpi_dev_get_property(dev, "spiWordSize", ACPI_TYPE_BUFFER, &o))
> +               spi->bits_per_word = *(u64 *)o->buffer.pointer;
> +

> +       if (!acpi_dev_get_property(dev, "spiBitOrder", ACPI_TYPE_BUFFER, &o) &&
> +           !*(u64 *)o->buffer.pointer)

In such cases I would prefer the notation like
ret = func();
if (!ret && ...)
 ...

Also we usually name object variable as obj, though it's minor here.

> +               spi->mode |= SPI_LSB_FIRST;

-- 
With Best Regards,
Andy Shevchenko
--
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 v2 1/4] ACPI / property: Don't evaluate objects for devices w/o handle

[Mika Westerberg]

On Wed, Jun 28, 2017 at 07:20:19PM +0200, 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: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 3/4] ACPI / scan: Recognize Apple SPI and I2C slaves

[Mika Westerberg]

On Wed, Jun 28, 2017 at 07:20:19PM +0200, Lukas Wunner wrote:
> 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: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Federico Lorenzi <florenzi@gmail.com>
> Reported-and-tested-by: Ronald Tschalär <ronald@innovation.ch>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> Changes v1 -> v2:
> - Newly inserted patch in v2 to fix an enumeration issue. (Ronald)
> 
>  drivers/acpi/scan.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index d4ff2cd1f738..c565e74bd2dd 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1444,6 +1444,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_ENABLED(CONFIG_X86) && dmi_match(DMI_SYS_VENDOR, "Apple Inc.") &&

Do we really need these checks?

> +	    (device_property_present(&device->dev, "spiSclkPeriod") ||
> +	     device_property_present(&device->dev, "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

Re: [PATCH v2 2/4] ACPI / property: Support Apple _DSM properties

[Mika Westerberg]

On Wed, Jun 28, 2017 at 07:20:19PM +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.
> 
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Andy had some comments which I think you should consider. Regardless of
that this looks good to me. Also I have one of those new Macs with SPI
connected keyboard so happy to see the support getting into mainline ;-)

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

[Mika Westerberg]

On Wed, Jun 28, 2017 at 07:20:19PM +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? */
> 
> Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 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>
> Signed-off-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>

Acked-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
--
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 v2 3/4] ACPI / scan: Recognize Apple SPI and I2C slaves

[Lukas Wunner]

On Thu, Jun 29, 2017 at 10:34:20AM +0300, Mika Westerberg wrote:
> On Wed, Jun 28, 2017 at 07:20:19PM +0200, Lukas Wunner wrote:
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -1444,6 +1444,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_ENABLED(CONFIG_X86) && dmi_match(DMI_SYS_VENDOR, "Apple Inc.") &&
> 
> Do we really need these checks?

With these checks present, searching for the properties can be optimized
away on ARM and skipped on x86 non-Macs.  (Each property query requires
decoding the _DSD Package and performing an O(n) search of the properties.)

So I'd say yes?

Thanks,

Lukas

> 
> > +	    (device_property_present(&device->dev, "spiSclkPeriod") ||
> > +	     device_property_present(&device->dev, "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

Re: [PATCH v2 3/4] ACPI / scan: Recognize Apple SPI and I2C slaves

[Mika Westerberg]

On Thu, Jun 29, 2017 at 10:46:04AM +0200, Lukas Wunner wrote:
> On Thu, Jun 29, 2017 at 10:34:20AM +0300, Mika Westerberg wrote:
> > On Wed, Jun 28, 2017 at 07:20:19PM +0200, Lukas Wunner wrote:
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -1444,6 +1444,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_ENABLED(CONFIG_X86) && dmi_match(DMI_SYS_VENDOR, "Apple Inc.") &&
> > 
> > Do we really need these checks?
> 
> With these checks present, searching for the properties can be optimized
> away on ARM and skipped on x86 non-Macs.  (Each property query requires
> decoding the _DSD Package and performing an O(n) search of the properties.)

Well, you add dmi_match() that gets called every time and I was under
the impression that we already performed _DSD decode when the property
set was initially parsed.

Those checks just uglify the code IMHO.

No strong feelings though, so up to Rafael to decide :-)
--
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 v2 2/4] ACPI / property: Support Apple _DSM properties

[Lukas Wunner]

On Wed, Jun 28, 2017 at 09:53:49PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 28, 2017 at 8:20 PM, Lukas Wunner <lukas@wunner.de> 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).
[snip]
> > +static void acpi_retrieve_apple_properties(struct acpi_device *adev)
> > +{
> > +       unsigned int i, j, newsize = 0, numprops, skipped = 0;
> > +       union acpi_object *props, *newprops;
> > +       void *free_space;
> > +
> > +       if (!IS_ENABLED(CONFIG_X86) || !dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
> > +               return;
> 
> You are using this in few places, perhaps it makes sense to do in
> (maybe) header like
> drivers/acpi/apple.h
> 
> static inline bool is_apple_system(void)
> {
>    return IS_ENABLED(CONFIG_X86) && dmi_match(DMI_SYS_VENDOR, "Apple Inc.");
> }
> 
> I know this makes more LOCs, the rationale is to keep such
> deterministic things in one place (basically maintenance).

You mean like is_uv_system() for SGI UltraViolet?

I could add a global bool is_apple_system which is set early during boot,
then the DMI check would only have to be performed once.  The bool would
be #defined to false on non-x86 arches via a header file which would have
to live in include/linux/.  I could add an x86-specific config option
such as CONFIG_X86_APPLE which would allow removal of all Mac-specific
quirks and behaviour by likewise #defining is_apple_system to false.

I'm not sure if the x86 maintainers will approve of this.  Next thing
you know, someone wants to add the same thing for Dell or whatever.
OTOH the amount of code to deal with Apple quirks may justify it.
Adding x86 maintainers to cc.  Ingo, Thomas, Peter, any opinion?

My concern is that this series is bikeshedded / postponed indefinitely
if it is made to depend on such bigger changes.  (The discussion of
which is legitimate of course.)

Thanks,

Lukas

Re: [PATCH v2 2/4] ACPI / property: Support Apple _DSM properties

[Rafael J. Wysocki]

On Sun, Jul 2, 2017 at 1:07 PM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> wrote:
> On Wed, Jun 28, 2017 at 09:53:49PM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 28, 2017 at 8:20 PM, Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org> 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).
> [snip]
>> > +static void acpi_retrieve_apple_properties(struct acpi_device *adev)
>> > +{
>> > +       unsigned int i, j, newsize = 0, numprops, skipped = 0;
>> > +       union acpi_object *props, *newprops;
>> > +       void *free_space;
>> > +
>> > +       if (!IS_ENABLED(CONFIG_X86) || !dmi_match(DMI_SYS_VENDOR, "Apple Inc."))
>> > +               return;
>>
>> You are using this in few places, perhaps it makes sense to do in
>> (maybe) header like
>> drivers/acpi/apple.h
>>
>> static inline bool is_apple_system(void)
>> {
>>    return IS_ENABLED(CONFIG_X86) && dmi_match(DMI_SYS_VENDOR, "Apple Inc.");
>> }
>>
>> I know this makes more LOCs, the rationale is to keep such
>> deterministic things in one place (basically maintenance).
>
> You mean like is_uv_system() for SGI UltraViolet?
>
> I could add a global bool is_apple_system which is set early during boot,
> then the DMI check would only have to be performed once.  The bool would
> be #defined to false on non-x86 arches via a header file which would have
> to live in include/linux/.  I could add an x86-specific config option
> such as CONFIG_X86_APPLE which would allow removal of all Mac-specific
> quirks and behaviour by likewise #defining is_apple_system to false.
>
> I'm not sure if the x86 maintainers will approve of this.  Next thing
> you know, someone wants to add the same thing for Dell or whatever.
> OTOH the amount of code to deal with Apple quirks may justify it.
> Adding x86 maintainers to cc.  Ingo, Thomas, Peter, any opinion?

Well, it doesn't have to go into arch/x86/.

We have drivers/acpi/x86/ now, so you can put it in there, say into
apple.c that will only be built for CONFIG_X86 set.

Then, instead of using a concealed #ifdef in-line, you could do that
openly in a header.

Moreover, having a static bool variable in drivers/acpi/x86/apple.c
would not be super-objectionable IMO.

> My concern is that this series is bikeshedded / postponed indefinitely
> if it is made to depend on such bigger changes.  (The discussion of
> which is legitimate of course.)

Of course, the Apple properties check in scan.c only makes sense if
the system is Apple, but then the dmi_match() adds a bit of overhead
for everything x86 that isn't Apple, which I guess is the majority.
So I'd like that overhead to be reduced if possible.

Thanks,
Rafael
--
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