Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace
@ 2019-05-30 11:16 Ard Biesheuvel
  2019-06-03 11:08 ` Mika Westerberg
  2019-06-09 23:44 ` Life is hard, and then you die
  0 siblings, 2 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2019-05-30 11:16 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Ard Biesheuvel, Mika Westerberg, linux-spi, broonie,
	andy.shevchenko, masahisa.kojima, Rafael J. Wysocki,
	Jarkko Nikula, linux-acpi, Lukas Wunner

Currently, the ACPI enumeration that takes place when registering a
SPI master only considers immediate child devices in the ACPI namespace,
rather than checking the ResourceSource field in the SpiSerialBus()
resource descriptor.

This is incorrect: SPI slaves could reside anywhere in the ACPI
namespace, and so we should enumerate the entire namespace and look for
any device that refers to the newly registered SPI master in its
resource descriptor.

So refactor the existing code and use a lookup structure so that
allocating the SPI device structure is deferred until we have identified
the device as an actual child of the controller. This approach is
loosely based on the way the I2C subsystem handles ACPI enumeration.

Note that Apple x86 hardware does not rely on SpiSerialBus() resources
in _CRS but uses nested devices below the controller's device node in
the ACPI namespace, with a special set of device properties. This means
we have to take care to only parse those properties for device nodes
that are direct children of the controller node.

Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: linux-spi@vger.kernel.org
Cc: broonie@kernel.org
Cc: andy.shevchenko@gmail.com
Cc: masahisa.kojima@linaro.org
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: linux-acpi@vger.kernel.org
Cc: Lukas Wunner <lukas@wunner.de>

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/spi/spi.c | 103 ++++++++++++++------
 1 file changed, 72 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5e75944ad5d1..4661b219a7e7 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1799,9 +1799,18 @@ 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_spi_lookup {
+	struct spi_controller 	*ctlr;
+	u32			max_speed_hz;
+	u32			mode;
+	int			irq;
+	u8			bits_per_word;
+	u8			chip_select;
+};
+
+static void acpi_spi_parse_apple_properties(struct acpi_device *dev,
+					    struct acpi_spi_lookup *lookup)
 {
-	struct acpi_device *dev = ACPI_COMPANION(&spi->dev);
 	const union acpi_object *obj;
 
 	if (!x86_apple_machine)
@@ -1809,35 +1818,46 @@ static void acpi_spi_parse_apple_properties(struct spi_device *spi)
 
 	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;
+		lookup->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;
+		lookup->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;
+		lookup->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;
+		lookup->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;
+		lookup->mode |= SPI_CPHA;
 }
 
 static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 {
-	struct spi_device *spi = data;
-	struct spi_controller *ctlr = spi->controller;
+	struct acpi_spi_lookup *lookup = data;
+	struct spi_controller *ctlr = lookup->ctlr;
 
 	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
 		struct acpi_resource_spi_serialbus *sb;
+		acpi_handle parent_handle;
+		acpi_status status;
 
 		sb = &ares->data.spi_serial_bus;
 		if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
+
+			status = acpi_get_handle(NULL,
+						 sb->resource_source.string_ptr,
+						 &parent_handle);
+
+			if (!status ||
+			    ACPI_HANDLE(ctlr->dev.parent) != parent_handle)
+				return -ENODEV;
+
 			/*
 			 * ACPI DeviceSelection numbering is handled by the
 			 * host controller driver in Windows and can vary
@@ -1850,25 +1870,25 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 						sb->device_selection);
 				if (cs < 0)
 					return cs;
-				spi->chip_select = cs;
+				lookup->chip_select = cs;
 			} else {
-				spi->chip_select = sb->device_selection;
+				lookup->chip_select = sb->device_selection;
 			}
 
-			spi->max_speed_hz = sb->connection_speed;
+			lookup->max_speed_hz = sb->connection_speed;
 
 			if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
-				spi->mode |= SPI_CPHA;
+				lookup->mode |= SPI_CPHA;
 			if (sb->clock_polarity == ACPI_SPI_START_HIGH)
-				spi->mode |= SPI_CPOL;
+				lookup->mode |= SPI_CPOL;
 			if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
-				spi->mode |= SPI_CS_HIGH;
+				lookup->mode |= SPI_CS_HIGH;
 		}
-	} else if (spi->irq < 0) {
+	} else if (lookup->irq < 0) {
 		struct resource r;
 
 		if (acpi_dev_resource_interrupt(ares, 0, &r))
-			spi->irq = r.start;
+			lookup->irq = r.start;
 	}
 
 	/* Always tell the ACPI core to skip this resource */
@@ -1878,7 +1898,9 @@ static int acpi_spi_add_resource(struct acpi_resource *ares, void *data)
 static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,
 					    struct acpi_device *adev)
 {
+	acpi_handle parent_handle = NULL;
 	struct list_head resource_list;
+	struct acpi_spi_lookup lookup;
 	struct spi_device *spi;
 	int ret;
 
@@ -1886,28 +1908,44 @@ static acpi_status acpi_register_spi_device(struct spi_controller *ctlr,
 	    acpi_device_enumerated(adev))
 		return AE_OK;
 
-	spi = spi_alloc_device(ctlr);
-	if (!spi) {
-		dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n",
-			dev_name(&adev->dev));
-		return AE_NO_MEMORY;
-	}
-
-	ACPI_COMPANION_SET(&spi->dev, adev);
-	spi->irq = -1;
+	lookup.ctlr		= ctlr;
+	lookup.mode		= 0;
+	lookup.bits_per_word	= 0;
+	lookup.irq		= -1;
 
 	INIT_LIST_HEAD(&resource_list);
 	ret = acpi_dev_get_resources(adev, &resource_list,
-				     acpi_spi_add_resource, spi);
+				     acpi_spi_add_resource, &lookup);
 	acpi_dev_free_resource_list(&resource_list);
 
-	acpi_spi_parse_apple_properties(spi);
+	if (ret < 0)
+		/* found SPI in _CRS but it points to another controller */
+		return AE_OK;
 
-	if (ret < 0 || !spi->max_speed_hz) {
-		spi_dev_put(spi);
+	if (!lookup.max_speed_hz &&
+	    !ACPI_FAILURE(acpi_get_parent(adev->handle, &parent_handle)) &&
+	    ACPI_HANDLE(ctlr->dev.parent) == parent_handle) {
+		/* Apple does not use _CRS but nested devices for SPI slaves */
+		acpi_spi_parse_apple_properties(adev, &lookup);
+	}
+
+	if (!lookup.max_speed_hz)
 		return AE_OK;
+
+	spi = spi_alloc_device(ctlr);
+	if (!spi) {
+		dev_err(&ctlr->dev, "failed to allocate SPI device for %s\n",
+			dev_name(&adev->dev));
+		return AE_NO_MEMORY;
 	}
 
+	ACPI_COMPANION_SET(&spi->dev, adev);
+	spi->max_speed_hz	= lookup.max_speed_hz;
+	spi->mode		= lookup.mode;
+	spi->irq		= lookup.irq;
+	spi->bits_per_word	= lookup.bits_per_word;
+	spi->chip_select	= lookup.chip_select;
+
 	acpi_set_modalias(adev, acpi_device_hid(adev), spi->modalias,
 			  sizeof(spi->modalias));
 
@@ -1939,6 +1977,8 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 	return acpi_register_spi_device(ctlr, adev);
 }
 
+#define SPI_ACPI_ENUMERATE_MAX_DEPTH		32
+
 static void acpi_register_spi_devices(struct spi_controller *ctlr)
 {
 	acpi_status status;
@@ -1948,7 +1988,8 @@ static void acpi_register_spi_devices(struct spi_controller *ctlr)
 	if (!handle)
 		return;
 
-	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				     SPI_ACPI_ENUMERATE_MAX_DEPTH,
 				     acpi_spi_add_device, NULL, ctlr, NULL);
 	if (ACPI_FAILURE(status))
 		dev_warn(&ctlr->dev, "failed to enumerate SPI slaves\n");
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace
  2019-05-30 11:16 [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace Ard Biesheuvel
@ 2019-06-03 11:08 ` Mika Westerberg
  2019-06-03 15:56   ` Ard Biesheuvel
  2019-06-09 23:44 ` Life is hard, and then you die
  1 sibling, 1 reply; 5+ messages in thread
From: Mika Westerberg @ 2019-06-03 11:08 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-arm-kernel, linux-spi, broonie, andy.shevchenko,
	masahisa.kojima, Rafael J. Wysocki, Jarkko Nikula, linux-acpi,
	Lukas Wunner

On Thu, May 30, 2019 at 01:16:34PM +0200, Ard Biesheuvel wrote:
> @@ -1948,7 +1988,8 @@ static void acpi_register_spi_devices(struct spi_controller *ctlr)
>  	if (!handle)
>  		return;
>  
> -	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,

Would it be simpler to differentiate here between Apple and non-Apple
systems? Then we don't need all that special code and the above becomes:

	depth = x86_apple_system ? 1 : SPI_ACPI_ENUMERATE_MAX_DEPTH;
	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, depth,
	..

Probably requires a comment explaining why we do it like that, though.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace
  2019-06-03 11:08 ` Mika Westerberg
@ 2019-06-03 15:56   ` Ard Biesheuvel
  2019-06-13 19:14     ` Mark Brown
  0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2019-06-03 15:56 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: linux-arm-kernel, linux-spi, Mark Brown, Andy Shevchenko,
	Masahisa Kojima, Rafael J. Wysocki, Jarkko Nikula,
	ACPI Devel Maling List, Lukas Wunner

On Mon, 3 Jun 2019 at 13:08, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, May 30, 2019 at 01:16:34PM +0200, Ard Biesheuvel wrote:
> > @@ -1948,7 +1988,8 @@ static void acpi_register_spi_devices(struct spi_controller *ctlr)
> >       if (!handle)
> >               return;
> >
> > -     status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> > +     status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
>
> Would it be simpler to differentiate here between Apple and non-Apple
> systems? Then we don't need all that special code and the above becomes:
>
>         depth = x86_apple_system ? 1 : SPI_ACPI_ENUMERATE_MAX_DEPTH;
>         status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, depth,
>         ..
>
> Probably requires a comment explaining why we do it like that, though.

Yes, but note that both the root and the depth are different in this case.

I'll play around with this idea, to see if it simplifies things.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace
  2019-05-30 11:16 [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace Ard Biesheuvel
  2019-06-03 11:08 ` Mika Westerberg
@ 2019-06-09 23:44 ` Life is hard, and then you die
  1 sibling, 0 replies; 5+ messages in thread
From: Life is hard, and then you die @ 2019-06-09 23:44 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: linux-arm-kernel, Mika Westerberg, linux-spi, broonie,
	andy.shevchenko, masahisa.kojima, Rafael J. Wysocki,
	Jarkko Nikula, linux-acpi, Lukas Wunner


On Thu, May 30, 2019 at 01:16:34PM +0200, Ard Biesheuvel wrote:
> Currently, the ACPI enumeration that takes place when registering a
> SPI master only considers immediate child devices in the ACPI namespace,
> rather than checking the ResourceSource field in the SpiSerialBus()
> resource descriptor.
> 
> This is incorrect: SPI slaves could reside anywhere in the ACPI
> namespace, and so we should enumerate the entire namespace and look for
> any device that refers to the newly registered SPI master in its
> resource descriptor.
> 
> So refactor the existing code and use a lookup structure so that
> allocating the SPI device structure is deferred until we have identified
> the device as an actual child of the controller. This approach is
> loosely based on the way the I2C subsystem handles ACPI enumeration.
> 
> Note that Apple x86 hardware does not rely on SpiSerialBus() resources
> in _CRS but uses nested devices below the controller's device node in
> the ACPI namespace, with a special set of device properties. This means
> we have to take care to only parse those properties for device nodes
> that are direct children of the controller node.
> 
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: linux-spi@vger.kernel.org
> Cc: broonie@kernel.org
> Cc: andy.shevchenko@gmail.com
> Cc: masahisa.kojima@linaro.org
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: linux-acpi@vger.kernel.org
> Cc: Lukas Wunner <lukas@wunner.de>
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/spi/spi.c | 103 ++++++++++++++------
>  1 file changed, 72 insertions(+), 31 deletions(-)
[snip]

FYI, I tested this on a MacBook Pro where the (SPI) keyboard driver
depends on those special device properties, and verified this patch
doesn't break anything there.


  Cheers,

  Ronald


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace
  2019-06-03 15:56   ` Ard Biesheuvel
@ 2019-06-13 19:14     ` Mark Brown
  0 siblings, 0 replies; 5+ messages in thread
From: Mark Brown @ 2019-06-13 19:14 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Mika Westerberg, linux-arm-kernel, linux-spi, Andy Shevchenko,
	Masahisa Kojima, Rafael J. Wysocki, Jarkko Nikula,
	ACPI Devel Maling List, Lukas Wunner

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

On Mon, Jun 03, 2019 at 05:56:00PM +0200, Ard Biesheuvel wrote:
> On Mon, 3 Jun 2019 at 13:08, Mika Westerberg
> > On Thu, May 30, 2019 at 01:16:34PM +0200, Ard Biesheuvel wrote:

> > > -     status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> > > +     status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,

> > Would it be simpler to differentiate here between Apple and non-Apple
> > systems? Then we don't need all that special code and the above becomes:

> >         depth = x86_apple_system ? 1 : SPI_ACPI_ENUMERATE_MAX_DEPTH;
> >         status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, depth,
> >         ..

> > Probably requires a comment explaining why we do it like that, though.

> Yes, but note that both the root and the depth are different in this case.

> I'll play around with this idea, to see if it simplifies things.

Given that this works and got some testing I've applied this now, if
there's a simplification it can always be done incrementally.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-30 11:16 [PATCH v2] spi/acpi: enumerate all SPI slaves in the namespace Ard Biesheuvel
2019-06-03 11:08 ` Mika Westerberg
2019-06-03 15:56   ` Ard Biesheuvel
2019-06-13 19:14     ` Mark Brown
2019-06-09 23:44 ` Life is hard, and then you die

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org linux-acpi@archiver.kernel.org
	public-inbox-index linux-acpi


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/ public-inbox