diff mbox

[v3,2/2] ata: ahci_platform: allow disabling of hotplug to save power

Message ID 20180529100612.3512-2-0v3rdr0n3@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

0v3rdr0n3@gmail.com May 29, 2018, 10:06 a.m. UTC 
From: Samuel Morris <samorris@lexmark.com>

A number of resources remain powered to support hotplug. On platforms
I've worked with, allowing the ahci_platform to suspend saves about
150mW. This patch enables rpm and allows the device to be auto-suspended
through sysfs.

Signed-off-by: Samuel Morris <samorris@lexmark.com>
---
v3 changes:
-Removed the global ahci_disable_hotplug config switch in favor of just
using sysfs to change runtime behavior per device.

v2 changes:
-Added missing CONFIG_PM_SLEEP ifdefs
---
 drivers/ata/ahci_platform.c    | 11 ++++-
 drivers/ata/libahci_platform.c | 82 +++++++++++++++++++++++++++-------
 include/linux/ahci_platform.h  |  2 +
 3 files changed, 76 insertions(+), 19 deletions(-)

Comments

Samuel Morris June 14, 2018, 5:33 p.m. UTC | #1 
On Tue, May 29, 2018 at 6:06 AM,  <0v3rdr0n3@gmail.com> wrote:
> From: Samuel Morris <samorris@lexmark.com>
>
> A number of resources remain powered to support hotplug. On platforms
> I've worked with, allowing the ahci_platform to suspend saves about
> 150mW. This patch enables rpm and allows the device to be auto-suspended
> through sysfs.
>
> Signed-off-by: Samuel Morris <samorris@lexmark.com>
> ---
> v3 changes:
> -Removed the global ahci_disable_hotplug config switch in favor of just
> using sysfs to change runtime behavior per device.
>
> v2 changes:
> -Added missing CONFIG_PM_SLEEP ifdefs
> ---
>  drivers/ata/ahci_platform.c    | 11 ++++-
>  drivers/ata/libahci_platform.c | 82 +++++++++++++++++++++++++++-------
>  include/linux/ahci_platform.h  |  2 +
>  3 files changed, 76 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index 99f9a895a459..757729376eda 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -68,8 +68,13 @@ static int ahci_probe(struct platform_device *pdev)
>         return rc;
>  }
>
> -static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend,
> -                        ahci_platform_resume);
> +#ifdef CONFIG_PM_SLEEP
> +static const struct dev_pm_ops ahci_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(ahci_platform_suspend, ahci_platform_resume)
> +       SET_RUNTIME_PM_OPS(ahci_platform_runtime_suspend,
> +                       ahci_platform_runtime_resume, NULL)
> +};
> +#endif
>
>  static const struct of_device_id ahci_of_match[] = {
>         { .compatible = "generic-ahci", },
> @@ -98,7 +103,9 @@ static struct platform_driver ahci_driver = {
>                 .name = DRV_NAME,
>                 .of_match_table = ahci_of_match,
>                 .acpi_match_table = ahci_acpi_match,
> +#ifdef CONFIG_PM_SLEEP
>                 .pm = &ahci_pm_ops,
> +#endif
>         },
>  };
>  module_platform_driver(ahci_driver);
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 30cc8f1a31e1..feee2e11fb33 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -257,7 +257,7 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
>         int c;
>
>         if (hpriv->got_runtime_pm) {
> -               pm_runtime_put_sync(dev);
> +               pm_runtime_allow(dev);
>                 pm_runtime_disable(dev);
>         }
>
> @@ -475,8 +475,10 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
>                 if (rc == -EPROBE_DEFER)
>                         goto err_out;
>         }
> +
> +       pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
> -       pm_runtime_get_sync(dev);
> +       pm_runtime_forbid(dev);
>         hpriv->got_runtime_pm = true;
>
>         devres_remove_group(dev, NULL);
> @@ -705,6 +707,21 @@ int ahci_platform_resume_host(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(ahci_platform_resume_host);
>
> +static int _ahci_platform_suspend(struct device *dev)
> +{
> +       struct ata_host *host = dev_get_drvdata(dev);
> +       struct ahci_host_priv *hpriv = host->private_data;
> +       int rc;
> +
> +       rc = ahci_platform_suspend_host(dev);
> +       if (rc)
> +               return rc;
> +
> +       ahci_platform_disable_resources(hpriv);
> +
> +       return 0;
> +}
> +
>  /**
>   * ahci_platform_suspend - Suspend an ahci-platform device
>   * @dev: the platform device to suspend
> @@ -716,20 +733,45 @@ EXPORT_SYMBOL_GPL(ahci_platform_resume_host);
>   * 0 on success otherwise a negative error code
>   */
>  int ahci_platform_suspend(struct device *dev)
> +{
> +       return _ahci_platform_suspend(dev);
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_suspend);
> +
> +/**
> + * ahci_platform_runtime_suspend - Runtime suspend an ahci-platform device
> + * @dev: the platform device to suspend
> + *
> + * This function suspends the host associated with the device, followed by
> + * disabling all the resources of the device.
> + *
> + * RETURNS:
> + * 0 on success otherwise a negative error code
> + */
> +int ahci_platform_runtime_suspend(struct device *dev)
> +{
> +       return _ahci_platform_suspend(dev);
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_runtime_suspend);
> +
> +static int _ahci_platform_resume(struct device *dev)
>  {
>         struct ata_host *host = dev_get_drvdata(dev);
>         struct ahci_host_priv *hpriv = host->private_data;
>         int rc;
>
> -       rc = ahci_platform_suspend_host(dev);
> +       rc = ahci_platform_enable_resources(hpriv);
>         if (rc)
>                 return rc;
>
> -       ahci_platform_disable_resources(hpriv);
> +       rc = ahci_platform_resume_host(dev);
> +       if (rc) {
> +               ahci_platform_disable_resources(hpriv);
> +               return rc;
> +       }
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(ahci_platform_suspend);
>
>  /**
>   * ahci_platform_resume - Resume an ahci-platform device
> @@ -743,31 +785,37 @@ EXPORT_SYMBOL_GPL(ahci_platform_suspend);
>   */
>  int ahci_platform_resume(struct device *dev)
>  {
> -       struct ata_host *host = dev_get_drvdata(dev);
> -       struct ahci_host_priv *hpriv = host->private_data;
>         int rc;
>
> -       rc = ahci_platform_enable_resources(hpriv);
> +       rc = _ahci_platform_resume(dev);
>         if (rc)
>                 return rc;
>
> -       rc = ahci_platform_resume_host(dev);
> -       if (rc)
> -               goto disable_resources;
> -
>         /* We resumed so update PM runtime state */
>         pm_runtime_disable(dev);
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>
>         return 0;
> -
> -disable_resources:
> -       ahci_platform_disable_resources(hpriv);
> -
> -       return rc;
>  }
>  EXPORT_SYMBOL_GPL(ahci_platform_resume);
> +
> +/**
> + * ahci_platform_runtime_resume - Runtime resume an ahci-platform device
> + * @dev: the platform device to resume
> + *
> + * This function enables all the resources of the device followed by
> + * resuming the host associated with the device.
> + *
> + * RETURNS:
> + * 0 on success otherwise a negative error code
> + */
> +int ahci_platform_runtime_resume(struct device *dev)
> +{
> +       return _ahci_platform_resume(dev);
> +}
> +EXPORT_SYMBOL_GPL(ahci_platform_runtime_resume);
> +
>  #endif
>
>  MODULE_DESCRIPTION("AHCI SATA platform library");
> diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> index 1b0a17b22cd3..6396e6982103 100644
> --- a/include/linux/ahci_platform.h
> +++ b/include/linux/ahci_platform.h
> @@ -42,5 +42,7 @@ int ahci_platform_suspend_host(struct device *dev);
>  int ahci_platform_resume_host(struct device *dev);
>  int ahci_platform_suspend(struct device *dev);
>  int ahci_platform_resume(struct device *dev);
> +int ahci_platform_runtime_suspend(struct device *dev);
> +int ahci_platform_runtime_resume(struct device *dev);
>
>  #endif /* _AHCI_PLATFORM_H */
> --
> 2.17.0
>

Has anyone had a chance to look at this yet? Please let me know if
there's anything I can improve or explain further.
Tejun Heo June 18, 2018, 6:02 p.m. UTC | #2 
On Tue, May 29, 2018 at 10:06:12AM +0000, 0v3rdr0n3@gmail.com wrote:
> From: Samuel Morris <samorris@lexmark.com>
> 
> A number of resources remain powered to support hotplug. On platforms
> I've worked with, allowing the ahci_platform to suspend saves about
> 150mW. This patch enables rpm and allows the device to be auto-suspended
> through sysfs.
> 
> Signed-off-by: Samuel Morris <samorris@lexmark.com>

Was waiting for the merge window to close.  Applied to
libata/for-4.19.

Thanks.
diff mbox

Patch

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 99f9a895a459..757729376eda 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -68,8 +68,13 @@  static int ahci_probe(struct platform_device *pdev)
 	return rc;
 }
 
-static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend,
-			 ahci_platform_resume);
+#ifdef CONFIG_PM_SLEEP
+static const struct dev_pm_ops ahci_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ahci_platform_suspend, ahci_platform_resume)
+	SET_RUNTIME_PM_OPS(ahci_platform_runtime_suspend,
+			ahci_platform_runtime_resume, NULL)
+};
+#endif
 
 static const struct of_device_id ahci_of_match[] = {
 	{ .compatible = "generic-ahci", },
@@ -98,7 +103,9 @@  static struct platform_driver ahci_driver = {
 		.name = DRV_NAME,
 		.of_match_table = ahci_of_match,
 		.acpi_match_table = ahci_acpi_match,
+#ifdef CONFIG_PM_SLEEP
 		.pm = &ahci_pm_ops,
+#endif
 	},
 };
 module_platform_driver(ahci_driver);
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 30cc8f1a31e1..feee2e11fb33 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -257,7 +257,7 @@  static void ahci_platform_put_resources(struct device *dev, void *res)
 	int c;
 
 	if (hpriv->got_runtime_pm) {
-		pm_runtime_put_sync(dev);
+		pm_runtime_allow(dev);
 		pm_runtime_disable(dev);
 	}
 
@@ -475,8 +475,10 @@  struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		if (rc == -EPROBE_DEFER)
 			goto err_out;
 	}
+
+	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-	pm_runtime_get_sync(dev);
+	pm_runtime_forbid(dev);
 	hpriv->got_runtime_pm = true;
 
 	devres_remove_group(dev, NULL);
@@ -705,6 +707,21 @@  int ahci_platform_resume_host(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(ahci_platform_resume_host);
 
+static int _ahci_platform_suspend(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	int rc;
+
+	rc = ahci_platform_suspend_host(dev);
+	if (rc)
+		return rc;
+
+	ahci_platform_disable_resources(hpriv);
+
+	return 0;
+}
+
 /**
  * ahci_platform_suspend - Suspend an ahci-platform device
  * @dev: the platform device to suspend
@@ -716,20 +733,45 @@  EXPORT_SYMBOL_GPL(ahci_platform_resume_host);
  * 0 on success otherwise a negative error code
  */
 int ahci_platform_suspend(struct device *dev)
+{
+	return _ahci_platform_suspend(dev);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_suspend);
+
+/**
+ * ahci_platform_runtime_suspend - Runtime suspend an ahci-platform device
+ * @dev: the platform device to suspend
+ *
+ * This function suspends the host associated with the device, followed by
+ * disabling all the resources of the device.
+ *
+ * RETURNS:
+ * 0 on success otherwise a negative error code
+ */
+int ahci_platform_runtime_suspend(struct device *dev)
+{
+	return _ahci_platform_suspend(dev);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_runtime_suspend);
+
+static int _ahci_platform_resume(struct device *dev)
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
 
-	rc = ahci_platform_suspend_host(dev);
+	rc = ahci_platform_enable_resources(hpriv);
 	if (rc)
 		return rc;
 
-	ahci_platform_disable_resources(hpriv);
+	rc = ahci_platform_resume_host(dev);
+	if (rc) {
+		ahci_platform_disable_resources(hpriv);
+		return rc;
+	}
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(ahci_platform_suspend);
 
 /**
  * ahci_platform_resume - Resume an ahci-platform device
@@ -743,31 +785,37 @@  EXPORT_SYMBOL_GPL(ahci_platform_suspend);
  */
 int ahci_platform_resume(struct device *dev)
 {
-	struct ata_host *host = dev_get_drvdata(dev);
-	struct ahci_host_priv *hpriv = host->private_data;
 	int rc;
 
-	rc = ahci_platform_enable_resources(hpriv);
+	rc = _ahci_platform_resume(dev);
 	if (rc)
 		return rc;
 
-	rc = ahci_platform_resume_host(dev);
-	if (rc)
-		goto disable_resources;
-
 	/* We resumed so update PM runtime state */
 	pm_runtime_disable(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 
 	return 0;
-
-disable_resources:
-	ahci_platform_disable_resources(hpriv);
-
-	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_platform_resume);
+
+/**
+ * ahci_platform_runtime_resume - Runtime resume an ahci-platform device
+ * @dev: the platform device to resume
+ *
+ * This function enables all the resources of the device followed by
+ * resuming the host associated with the device.
+ *
+ * RETURNS:
+ * 0 on success otherwise a negative error code
+ */
+int ahci_platform_runtime_resume(struct device *dev)
+{
+	return _ahci_platform_resume(dev);
+}
+EXPORT_SYMBOL_GPL(ahci_platform_runtime_resume);
+
 #endif
 
 MODULE_DESCRIPTION("AHCI SATA platform library");
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 1b0a17b22cd3..6396e6982103 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -42,5 +42,7 @@  int ahci_platform_suspend_host(struct device *dev);
 int ahci_platform_resume_host(struct device *dev);
 int ahci_platform_suspend(struct device *dev);
 int ahci_platform_resume(struct device *dev);
+int ahci_platform_runtime_suspend(struct device *dev);
+int ahci_platform_runtime_resume(struct device *dev);
 
 #endif /* _AHCI_PLATFORM_H */