ACPI power management support for I2C and SPI devices

PATCH Superseded Mika Westerberg 4 participants 14 messages Oct 9, 2013 – Oct 10, 2013

Subsystems (5)

ACPI SPI I2C subsystem host drivers ACPI component architecture (acpica) +1 more subsystems

lore

Share & tools

lore

Mika Westerberg

author

2013-10-09 14:04 UTC #0 lore raw

cover ACPI power management support for I2C and SPI devices

Hi,

We originally tried to add runtime PM support for I2C adapter devices in order to have runtime PM core to power on the I2C host controller device automatically whenever any of its children I2C device is powered on (See
[1] for the last patch series).

The rationale was that it is what the ACPI 5.0 specification requires: the parent device cannot be lower power states than its children.

However, it looks like Windows can put the I2C controller device to low power states independently of its children power states, just like Linux does.

This patch series only adds I2C and SPI devices to the ACPI power domain and make sure that the parent device power state is ignored when transitioning child device to a different power states.

Since patches [2-3/3] depend on patch [1/3] it would be good if these all could be merged together.

[1] http://lwn.net/Articles/568961/

Lv Zheng (1):
i2c: attach/detach I2C client device to the ACPI power domain

Mika Westerberg (2):
ACPI / PM: allow child devices to ignore parent power state
spi: attach/detach SPI device to the ACPI power domain

 drivers/acpi/device_pm.c |  8 +++++---
 drivers/i2c/i2c-core.c   | 10 ++++++++++
 drivers/spi/spi.c        | 20 ++++++++++++++++++--
 include/acpi/acpi_bus.h  |  3 ++-
 4 files changed, 35 insertions(+), 6 deletions(-)

Signature · 1 line

1.8.4.rc3

Mika Westerberg

author

2013-10-09 14:04 UTC #1 lore raw

PATCH 1/3 ACPI / PM: allow child devices to ignore parent power state

Some serial buses like I2C and SPI don't require that the parent device is in D0 before any of its children transitions to D0, but instead the parent device can control its own power independently from the children.

This does not follow the ACPI specification as it requires the parent to be powered on before its children. However, Windows seems to ignore this requirement so I think we can do the same in Linux.

Implement this by adding a new power flag 'ignore_parent' to struct acpi_device. If this flag is set the ACPI core ignores checking of the parent device power state when the device is powered on/off.

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/acpi/device_pm.c | 8 +++++---
 include/acpi/acpi_bus.h  | 3 ++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/acpi/device_pm.c +5 -3

index 59d3202..ca723a4 100644
@@ -118,9 +118,10 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
 	/*
 	 * If we were unsure about the device parent's power state up to this
 	 * point, the fact that the device is in D0 implies that the parent has
-	 * to be in D0 too.
+	 * to be in D0 too, except if ignore_parent is set.
 	 */
-	if (device->parent && device->parent->power.state == ACPI_STATE_UNKNOWN
+	if (!device->power.flags.ignore_parent && device->parent
+	    && device->parent->power.state == ACPI_STATE_UNKNOWN
 	    && result == ACPI_STATE_D0)
 		device->parent->power.state = ACPI_STATE_D0;

@@ -177,7 +178,8 @@ int acpi_device_set_power(struct acpi_device *device, int state)
 			 acpi_power_state_string(state));
 		return -ENODEV;
 	}
-	if (device->parent && (state < device->parent->power.state)) {
+	if (!device->power.flags.ignore_parent &&
+	    device->parent && (state < device->parent->power.state)) {
 		dev_warn(&device->dev,
 			 "Cannot transition to power state %s for parent in %s\n",
 			 acpi_power_state_string(state),

include/acpi/acpi_bus.h +2 -1

index 02e113b..e830ab0 100644
@@ -222,7 +222,8 @@ struct acpi_device_power_flags {
 	u32 power_resources:1;	/* Power resources */
 	u32 inrush_current:1;	/* Serialize Dx->D0 */
 	u32 power_removed:1;	/* Optimize Dx->D0 */
-	u32 reserved:28;
+	u32 ignore_parent:1;	/* Power is independent of parent power state */
+	u32 reserved:27;
 };
 
 struct acpi_device_power_state {

Signature · 1 line

1.8.4.rc3

Mika Westerberg

author

2013-10-09 14:04 UTC #2 lore raw

PATCH 2/3 i2c: attach/detach I2C client device to the ACPI power domain

In reply to Mika Westerberg

From: Lv Zheng <[email protected]>

If the I2C client device is enumerated from ACPI namespace it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx).

Implement this for I2C client devices by checking if the device has an ACPI handle and if that's the case, attach it to the ACPI power domain. In addition we make sure that the device is fully powered when its ->probe() function gets called.

For non-ACPI devices this patch is a no-op.

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/i2c/i2c-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/i2c/i2c-core.c +10 -0

index 29d3f04..f25dee3 100644
@@ -254,10 +254,16 @@ static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
+	if (ACPI_HANDLE(&client->dev))
+		acpi_dev_pm_attach(&client->dev, true);
+
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
 	if (status) {
 		client->driver = NULL;
 		i2c_set_clientdata(client, NULL);
+
+		if (ACPI_HANDLE(&client->dev))
+			acpi_dev_pm_detach(&client->dev, true);
 	}
 	return status;
 }

@@ -283,6 +289,8 @@ static int i2c_device_remove(struct device *dev)
 		client->driver = NULL;
 		i2c_set_clientdata(client, NULL);
 	}
+	if (ACPI_HANDLE(&client->dev))
+		acpi_dev_pm_detach(&client->dev, true);
 	return status;
 }

@@ -1111,8 +1119,10 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	if (ret < 0 || !info.addr)
 		return AE_OK;
 
+	adev->power.flags.ignore_parent = true;
 	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
 	if (!i2c_new_device(adapter, &info)) {
+		adev->power.flags.ignore_parent = false;
 		dev_err(&adapter->dev,
 			"failed to add I2C device %s from ACPI\n",
 			dev_name(&adev->dev));

Signature · 1 line

1.8.4.rc3

Wolfram Sang

Acked-by on v1 2/3

2013-10-10 08:09 UTC #3 lore raw

Re: [PATCH 2/3] i2c: attach/detach I2C client device to the ACPI power domain

Quoted 24 lines · Mika Westerberg

From: Lv Zheng <[email protected]>
If the I2C client device is enumerated from ACPI namespace it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx).
Implement this for I2C client devices by checking if the device has an ACPI handle and if that's the case, attach it to the ACPI power domain. In addition we make sure that the device is fully powered when its ->probe() function gets called.
For non-ACPI devices this patch is a no-op.
Signed-off-by Lv Zheng <[email protected]>
Signed-off-by Mika Westerberg <[email protected]>
---
 drivers/i2c/i2c-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/i2c/i2c-core.c +0 -0
index 29d3f04..f25dee3 100644

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 258

Quoted 5 lines

drivers/i2c/i2c-core.c +2 -0

@@ -254,10 +254,16 @@ static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
+	if (ACPI_HANDLE(&client->dev))
+		acpi_dev_pm_attach(&client->dev, true);

I'd prefer to drop the 'if's in case they are checked inside the acpi_* calls anyway. Not a show-stopper, though, so:

Acked-by Wolfram Sang <[email protected]>

Mika Westerberg

author

2013-10-10 08:21 UTC #4 lore raw

Re: [PATCH 2/3] i2c: attach/detach I2C client device to the ACPI power domain

Quoted 28 lines · Wolfram Sang

Quoted 27 lines · Mika Westerberg
From: Lv Zheng <[email protected]>
If the I2C client device is enumerated from ACPI namespace it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx).
Implement this for I2C client devices by checking if the device has an ACPI handle and if that's the case, attach it to the ACPI power domain. In addition we make sure that the device is fully powered when its ->probe() function gets called.
For non-ACPI devices this patch is a no-op.
Signed-off-by Lv Zheng <[email protected]>
Signed-off-by Mika Westerberg <[email protected]>
---
 drivers/i2c/i2c-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/i2c/i2c-core.c +0 -0
index 29d3f04..f25dee3 100644
@@ -254,10 +254,16 @@ static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 

Quoted 5 lines

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 258
Quoted 2 lines
drivers/i2c/i2c-core.c +2 -0
+	if (ACPI_HANDLE(&client->dev))
+		acpi_dev_pm_attach(&client->dev, true);
I'd prefer to drop the 'if's in case they are checked inside the acpi_* calls anyway. Not a show-stopper, though, so:

OK, I'll drop them then from the final version.

Quoted 2 lines

Acked-by Wolfram Sang <[email protected]>

Thanks!

Mika Westerberg

author

2013-10-09 14:04 UTC #5 lore raw

PATCH 3/3 spi: attach/detach SPI device to the ACPI power domain

In reply to Mika Westerberg

If the SPI device is enumerated from ACPI namespace (it has an ACPI handle) it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx).

We follow what has been done for platform and I2C buses here and attach the SPI device to the ACPI power domain if the device has an ACPI handle. This makes sure that the device is powered on when its ->probe() is called.

For non-ACPI devices this patch is a no-op.

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/spi/spi.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

drivers/spi/spi.c +18 -2

index 9e039c6..c522aa1 100644
@@ -240,15 +240,29 @@ EXPORT_SYMBOL_GPL(spi_bus_type);
 static int spi_drv_probe(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
+	struct spi_device		*spi = to_spi_device(dev);
+	int ret;
+
+	if (ACPI_HANDLE(&spi->dev))
+		acpi_dev_pm_attach(&spi->dev, true);
+	ret = sdrv->probe(spi);
+	if (ret && ACPI_HANDLE(&spi->dev))
+		acpi_dev_pm_detach(&spi->dev, true);
 
-	return sdrv->probe(to_spi_device(dev));
+	return ret;
 }
 
 static int spi_drv_remove(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
+	struct spi_device		*spi = to_spi_device(dev);
+	int ret;
+
+	ret = sdrv->remove(spi);
+	if (ACPI_HANDLE(&spi->dev))
+		acpi_dev_pm_detach(&spi->dev, true);
 
-	return sdrv->remove(to_spi_device(dev));
+	return ret;
 }
 
 static void spi_drv_shutdown(struct device *dev)

@@ -1025,8 +1039,10 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 		return AE_OK;
 	}
 
+	adev->power.flags.ignore_parent = true;
 	strlcpy(spi->modalias, dev_name(&adev->dev), sizeof(spi->modalias));
 	if (spi_add_device(spi)) {
+		adev->power.flags.ignore_parent = false;
 		dev_err(&master->dev, "failed to add SPI device %s from ACPI\n",
 			dev_name(&adev->dev));
 		spi_dev_put(spi);

Signature · 1 line

1.8.4.rc3

Mark Brown

maintainer Acked-by on v1 3/3

2013-10-09 17:55 UTC #6 lore raw

Re: [PATCH 3/3] spi: attach/detach SPI device to the ACPI power domain

Quoted 3 lines · Mika Westerberg

If the SPI device is enumerated from ACPI namespace (it has an ACPI handle) it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx).

Acked-by Mark Brown <[email protected]>

drivers/spi/spi.c EXPORT_SYMBOL_GPL(spi_bus_type); · line 247

Quoted 2 lines

drivers/spi/spi.c +2 -0

+	if (ACPI_HANDLE(&spi->dev))
+		acpi_dev_pm_attach(&spi->dev, true);

Though I do wonder if it wouldn't be sensible to push the if () here inside acpi_dev_pm_attach() and similarly for _detach(). Terribly trivial either way.

Mika Westerberg

author

2013-10-10 06:07 UTC #7 lore raw

Re: [PATCH 3/3] spi: attach/detach SPI device to the ACPI power domain

Quoted 6 lines · Mark Brown

Quoted 3 lines · Mika Westerberg
If the SPI device is enumerated from ACPI namespace (it has an ACPI handle) it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx).
Acked-by Mark Brown <[email protected]>

Thanks!

Quoted 6 lines

drivers/spi/spi.c EXPORT_SYMBOL_GPL(spi_bus_type); · line 247
Quoted 2 lines
drivers/spi/spi.c +2 -0
+	if (ACPI_HANDLE(&spi->dev))
+		acpi_dev_pm_attach(&spi->dev, true);
Though I do wonder if it wouldn't be sensible to push the if () here inside acpi_dev_pm_attach() and similarly for _detach(). Terribly trivial either way.

Actually, the check is already there in acpi_dev_pm_attach()/detach(). The above code follows what Rafael did for platform bus previously. I think the idea is to have visual hint that this is only for ACPI enumerated devices.

If preferred, I can drop the if() checks, though.

Mark Brown

maintainer

2013-10-10 09:38 UTC #8 lore raw

Re: [PATCH 3/3] spi: attach/detach SPI device to the ACPI power domain

Quoted 10 lines · Mika Westerberg

Quoted 8 lines · Mark Brown
drivers/spi/spi.c EXPORT_SYMBOL_GPL(spi_bus_type); · line 247
Quoted 2 lines · Mika Westerberg
drivers/spi/spi.c +2 -0
+	if (ACPI_HANDLE(&spi->dev))
+		acpi_dev_pm_attach(&spi->dev, true);
Though I do wonder if it wouldn't be sensible to push the if () here inside acpi_dev_pm_attach() and similarly for _detach(). Terribly trivial either way.

Quoted 5 lines

Actually, the check is already there in acpi_dev_pm_attach()/detach(). The above code follows what Rafael did for platform bus previously. I think the idea is to have visual hint that this is only for ACPI enumerated devices.
If preferred, I can drop the if() checks, though.

It'd seem neater - the fact that the function is acpi_ ought to be enough of a hint.

Mika Westerberg

author

2013-10-10 10:28 UTC #9 lore raw

v2 cover ACPI power management support for I2C and SPI devices

In reply to Mika Westerberg

Here is an updated version. I added acks from Mark and Wolfram and dropped if (ACPI_HANDLE()) checks as requested. This is already handled by acpi_dev_pm_attach()/detach().

Rafael,

If you are OK with these, could you merge the series to your ACPI tree?

Thanks!

Lv Zheng (1):
i2c: attach/detach I2C client device to the ACPI power domain

Mika Westerberg (2):
ACPI / PM: allow child devices to ignore parent power state
spi: attach/detach SPI device to the ACPI power domain

 drivers/acpi/device_pm.c |  8 +++++---
 drivers/i2c/i2c-core.c   |  5 +++++
 drivers/spi/spi.c        | 18 ++++++++++++++++--
 include/acpi/acpi_bus.h  |  3 ++-
 4 files changed, 28 insertions(+), 6 deletions(-)

Signature · 1 line

1.8.4.rc3

Mika Westerberg

author

2013-10-10 10:28 UTC #10 lore raw

PATCH v2 1/3 ACPI / PM: allow child devices to ignore parent power state

This does not follow the ACPI specification as it requires the parent to be powered on before its children. However, Windows seems to ignore this requirement so I think we can do the same in Linux.

Signed-off-by Mika Westerberg <[email protected]>

---
 drivers/acpi/device_pm.c | 8 +++++---
 include/acpi/acpi_bus.h  | 3 ++-
 2 files changed, 7 insertions(+), 4 deletions(-)

drivers/acpi/device_pm.c +5 -3

index 59d3202..ca723a4 100644
@@ -118,9 +118,10 @@ int acpi_device_get_power(struct acpi_device *device, int *state)
 	/*
 	 * If we were unsure about the device parent's power state up to this
 	 * point, the fact that the device is in D0 implies that the parent has
-	 * to be in D0 too.
+	 * to be in D0 too, except if ignore_parent is set.
 	 */
-	if (device->parent && device->parent->power.state == ACPI_STATE_UNKNOWN
+	if (!device->power.flags.ignore_parent && device->parent
+	    && device->parent->power.state == ACPI_STATE_UNKNOWN
 	    && result == ACPI_STATE_D0)
 		device->parent->power.state = ACPI_STATE_D0;

@@ -177,7 +178,8 @@ int acpi_device_set_power(struct acpi_device *device, int state)
 			 acpi_power_state_string(state));
 		return -ENODEV;
 	}
-	if (device->parent && (state < device->parent->power.state)) {
+	if (!device->power.flags.ignore_parent &&
+	    device->parent && (state < device->parent->power.state)) {
 		dev_warn(&device->dev,
 			 "Cannot transition to power state %s for parent in %s\n",
 			 acpi_power_state_string(state),

include/acpi/acpi_bus.h +2 -1

index 02e113b..e830ab0 100644
@@ -222,7 +222,8 @@ struct acpi_device_power_flags {
 	u32 power_resources:1;	/* Power resources */
 	u32 inrush_current:1;	/* Serialize Dx->D0 */
 	u32 power_removed:1;	/* Optimize Dx->D0 */
-	u32 reserved:28;
+	u32 ignore_parent:1;	/* Power is independent of parent power state */
+	u32 reserved:27;
 };
 
 struct acpi_device_power_state {

Signature · 1 line

1.8.4.rc3

Mika Westerberg

author

2013-10-10 10:28 UTC #11 lore raw

PATCH v2 2/3 i2c: attach/detach I2C client device to the ACPI power domain

In reply to Mika Westerberg

From: Lv Zheng <[email protected]>

If the I2C client device is enumerated from ACPI namespace it might have ACPI methods that needs to be called in order to transition the device to different power states (such as _PSx).

For non-ACPI devices this patch is a no-op.

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Mika Westerberg <[email protected]>

Acked-by Wolfram Sang <[email protected]>

---
 drivers/i2c/i2c-core.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/i2c/i2c-core.c +5 -0

index 29d3f04..03a8ae6 100644
@@ -254,10 +254,12 @@ static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
+	acpi_dev_pm_attach(&client->dev, true);
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
 	if (status) {
 		client->driver = NULL;
 		i2c_set_clientdata(client, NULL);
+		acpi_dev_pm_detach(&client->dev, true);
 	}
 	return status;
 }

@@ -283,6 +285,7 @@ static int i2c_device_remove(struct device *dev)
 		client->driver = NULL;
 		i2c_set_clientdata(client, NULL);
 	}
+	acpi_dev_pm_detach(&client->dev, true);
 	return status;
 }

@@ -1111,8 +1114,10 @@ static acpi_status acpi_i2c_add_device(acpi_handle handle, u32 level,
 	if (ret < 0 || !info.addr)
 		return AE_OK;
 
+	adev->power.flags.ignore_parent = true;
 	strlcpy(info.type, dev_name(&adev->dev), sizeof(info.type));
 	if (!i2c_new_device(adapter, &info)) {
+		adev->power.flags.ignore_parent = false;
 		dev_err(&adapter->dev,
 			"failed to add I2C device %s from ACPI\n",
 			dev_name(&adev->dev));

Signature · 1 line

1.8.4.rc3

Mika Westerberg

author

2013-10-10 10:28 UTC #12 lore raw

PATCH v2 3/3 spi: attach/detach SPI device to the ACPI power domain

In reply to Mika Westerberg

For non-ACPI devices this patch is a no-op.

Signed-off-by Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/[email protected]>

Acked-by Mark Brown <[email protected]>

---
 drivers/spi/spi.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

drivers/spi/spi.c +16 -2

index 9e039c6..740f9dd 100644
@@ -240,15 +240,27 @@ EXPORT_SYMBOL_GPL(spi_bus_type);
 static int spi_drv_probe(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
+	struct spi_device		*spi = to_spi_device(dev);
+	int ret;
+
+	acpi_dev_pm_attach(&spi->dev, true);
+	ret = sdrv->probe(spi);
+	if (ret)
+		acpi_dev_pm_detach(&spi->dev, true);
 
-	return sdrv->probe(to_spi_device(dev));
+	return ret;
 }
 
 static int spi_drv_remove(struct device *dev)
 {
 	const struct spi_driver		*sdrv = to_spi_driver(dev->driver);
+	struct spi_device		*spi = to_spi_device(dev);
+	int ret;
+
+	ret = sdrv->remove(spi);
+	acpi_dev_pm_detach(&spi->dev, true);
 
-	return sdrv->remove(to_spi_device(dev));
+	return ret;
 }
 
 static void spi_drv_shutdown(struct device *dev)

@@ -1025,8 +1037,10 @@ static acpi_status acpi_spi_add_device(acpi_handle handle, u32 level,
 		return AE_OK;
 	}
 
+	adev->power.flags.ignore_parent = true;
 	strlcpy(spi->modalias, dev_name(&adev->dev), sizeof(spi->modalias));
 	if (spi_add_device(spi)) {
+		adev->power.flags.ignore_parent = false;
 		dev_err(&master->dev, "failed to add SPI device %s from ACPI\n",
 			dev_name(&adev->dev));
 		spi_dev_put(spi);

Signature · 1 line

1.8.4.rc3

Rafael J. Wysocki

2013-10-10 12:50 UTC #13 lore raw

In reply to Mika Westerberg

Quoted 9 lines · Mika Westerberg

Here is an updated version. I added acks from Mark and Wolfram and dropped if (ACPI_HANDLE()) checks as requested. This is already handled by acpi_dev_pm_attach()/detach().
Rafael,
If you are OK with these, could you merge the series to your ACPI tree?
Thanks!

I will, thanks!

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.