[PATCH v2 0/3] phy: core: Re-work runtime PM deployment and fix an issue

[PATCH v2 0/3] phy: core: Re-work runtime PM deployment and fix an issue

[Ulf Hansson]

Changes in v2:
	- Address scenario, pointed out by Kishon, that runtime PM may be
	enabled by the phy provider driver, *after* it have called
	devm_phy_create() or phy_create().

The intend of this series is to simplify the runtime PM deployment in the phy
core, but while doing that fixing a related problem for a Renesas SoC making use
of a phy. This issue was raised by Geert Uytterhoeven and Yoshihiro Shimoda, due
to a changed behaviour in the runtime PM core.

The commit in question is f8817f61e821 (PM / runtime: Drop children check from
__pm_runtime_set_status()). However, it turned out that the problem existed
already prior this change, only that the error was printed instead of giving a
WARN splat.

More details about the changes are available in each change log. For additional
background, please have look at the link below.

https://patchwork.kernel.org/patch/10086651/


Ulf Hansson (3):
  phy: core: Move runtime PM reference counting to the parent device
  phy: core: Drop unused runtime PM APIs
  phy: core: Update the runtime PM section in the docs to reflect
    changes

 Documentation/phy.txt   | 29 ++++++++++++----------
 drivers/phy/phy-core.c  | 65 +++++++------------------------------------------
 include/linux/phy/phy.h | 46 +---------------------------------
 3 files changed, 26 insertions(+), 114 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

The runtime PM deployment in the phy core is deployed using the phy core
device, which is created by the phy core and assigned as a child device of
the phy provider device.

The behaviour around the runtime PM deployment cause some issues during
system suspend, in cases when the phy provider device is put into a low
power state via a call to the pm_runtime_force_suspend() helper, as is the
case for a Renesas SoC, which has its phy provider device attached to the
generic PM domain.

In more detail, the problem in this case is that pm_runtime_force_suspend()
expects the child device of the provider device, which is the phy core
device, to be runtime suspended, else a WARN splat will be printed
(correctly) when runtime PM gets re-enabled at system resume.

In the current scenario, even if a call to phy_power_off() triggers it to
invoke pm_runtime_put() during system suspend, the phy core device doesn't
get runtime suspended, because this is prevented in the system suspend
phases by the PM core.

To solve this problem, let's move the runtime PM deployment from the phy
core device to the phy provider device, as this provides the similar
behaviour. Changing this makes it redundant to enable runtime PM for the
phy core device, so let's avoid doing that.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/phy/phy-core.c  | 33 +++++++++++++++------------------
 include/linux/phy/phy.h |  1 +
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index b4964b0..09588ec 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -153,12 +153,12 @@ int phy_pm_runtime_get(struct phy *phy)
 {
 	int ret;
 
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	ret = pm_runtime_get(&phy->dev);
+	ret = pm_runtime_get(phy->dev.parent);
 	if (ret < 0 && ret != -EINPROGRESS)
-		pm_runtime_put_noidle(&phy->dev);
+		pm_runtime_put_noidle(phy->dev.parent);
 
 	return ret;
 }
@@ -168,12 +168,12 @@ int phy_pm_runtime_get_sync(struct phy *phy)
 {
 	int ret;
 
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	ret = pm_runtime_get_sync(&phy->dev);
+	ret = pm_runtime_get_sync(phy->dev.parent);
 	if (ret < 0)
-		pm_runtime_put_sync(&phy->dev);
+		pm_runtime_put_sync(phy->dev.parent);
 
 	return ret;
 }
@@ -181,37 +181,37 @@ EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
 
 int phy_pm_runtime_put(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	return pm_runtime_put(&phy->dev);
+	return pm_runtime_put(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
 
 int phy_pm_runtime_put_sync(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
-	return pm_runtime_put_sync(&phy->dev);
+	return pm_runtime_put_sync(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
 
 void phy_pm_runtime_allow(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return;
 
-	pm_runtime_allow(&phy->dev);
+	pm_runtime_allow(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
 
 void phy_pm_runtime_forbid(struct phy *phy)
 {
-	if (!pm_runtime_enabled(&phy->dev))
+	if (!phy->use_runtime_pm)
 		return;
 
-	pm_runtime_forbid(&phy->dev);
+	pm_runtime_forbid(phy->dev.parent);
 }
 EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
 
@@ -774,10 +774,7 @@ struct phy *phy_create(struct device *dev, struct device_node *node,
 	if (ret)
 		goto put_dev;
 
-	if (pm_runtime_enabled(dev)) {
-		pm_runtime_enable(&phy->dev);
-		pm_runtime_no_callbacks(&phy->dev);
-	}
+	phy->use_runtime_pm = pm_runtime_enabled(dev);
 
 	return phy;
 
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 4f8423a..b4298a1 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -83,6 +83,7 @@ struct phy {
 	int			power_count;
 	struct phy_attrs	attrs;
 	struct regulator	*pwr;
+	bool			use_runtime_pm;
 };
 
 /**
-- 
2.7.4

[PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Ulf Hansson]

The phy core already deploys runtime PM support, so there seems to be no
obvious reason for having dedicated APIs to control runtime PM for phys.

Therefore, let's remove the APIs altogether and instead convert internal
needed functions to be static.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/phy/phy-core.c  | 50 +++----------------------------------------------
 include/linux/phy/phy.h | 45 --------------------------------------------
 2 files changed, 3 insertions(+), 92 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 09588ec..1621625 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -149,22 +149,7 @@ static struct phy_provider *of_phy_provider_lookup(struct device_node *node)
 	return ERR_PTR(-EPROBE_DEFER);
 }
 
-int phy_pm_runtime_get(struct phy *phy)
-{
-	int ret;
-
-	if (!phy->use_runtime_pm)
-		return -ENOTSUPP;
-
-	ret = pm_runtime_get(phy->dev.parent);
-	if (ret < 0 && ret != -EINPROGRESS)
-		pm_runtime_put_noidle(phy->dev.parent);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_get);
-
-int phy_pm_runtime_get_sync(struct phy *phy)
+static int phy_pm_runtime_get_sync(struct phy *phy)
 {
 	int ret;
 
@@ -177,43 +162,14 @@ int phy_pm_runtime_get_sync(struct phy *phy)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(phy_pm_runtime_get_sync);
 
-int phy_pm_runtime_put(struct phy *phy)
+static int phy_pm_runtime_put(struct phy *phy)
 {
 	if (!phy->use_runtime_pm)
 		return -ENOTSUPP;
 
 	return pm_runtime_put(phy->dev.parent);
 }
-EXPORT_SYMBOL_GPL(phy_pm_runtime_put);
-
-int phy_pm_runtime_put_sync(struct phy *phy)
-{
-	if (!phy->use_runtime_pm)
-		return -ENOTSUPP;
-
-	return pm_runtime_put_sync(phy->dev.parent);
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_put_sync);
-
-void phy_pm_runtime_allow(struct phy *phy)
-{
-	if (!phy->use_runtime_pm)
-		return;
-
-	pm_runtime_allow(phy->dev.parent);
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_allow);
-
-void phy_pm_runtime_forbid(struct phy *phy)
-{
-	if (!phy->use_runtime_pm)
-		return;
-
-	pm_runtime_forbid(phy->dev.parent);
-}
-EXPORT_SYMBOL_GPL(phy_pm_runtime_forbid);
 
 int phy_init(struct phy *phy)
 {
@@ -306,7 +262,7 @@ int phy_power_on(struct phy *phy)
 
 err_pwr_on:
 	mutex_unlock(&phy->mutex);
-	phy_pm_runtime_put_sync(phy);
+	phy_pm_runtime_put(phy);
 err_pm_sync:
 	if (phy->pwr)
 		regulator_disable(phy->pwr);
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index b4298a1..050b620 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -17,7 +17,6 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <linux/device.h>
-#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
 struct phy;
@@ -134,12 +133,6 @@ static inline void *phy_get_drvdata(struct phy *phy)
 }
 
 #if IS_ENABLED(CONFIG_GENERIC_PHY)
-int phy_pm_runtime_get(struct phy *phy);
-int phy_pm_runtime_get_sync(struct phy *phy);
-int phy_pm_runtime_put(struct phy *phy);
-int phy_pm_runtime_put_sync(struct phy *phy);
-void phy_pm_runtime_allow(struct phy *phy);
-void phy_pm_runtime_forbid(struct phy *phy);
 int phy_init(struct phy *phy);
 int phy_exit(struct phy *phy);
 int phy_power_on(struct phy *phy);
@@ -188,44 +181,6 @@ void devm_of_phy_provider_unregister(struct device *dev,
 int phy_create_lookup(struct phy *phy, const char *con_id, const char *dev_id);
 void phy_remove_lookup(struct phy *phy, const char *con_id, const char *dev_id);
 #else
-static inline int phy_pm_runtime_get(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline int phy_pm_runtime_get_sync(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline int phy_pm_runtime_put(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline int phy_pm_runtime_put_sync(struct phy *phy)
-{
-	if (!phy)
-		return 0;
-	return -ENOSYS;
-}
-
-static inline void phy_pm_runtime_allow(struct phy *phy)
-{
-	return;
-}
-
-static inline void phy_pm_runtime_forbid(struct phy *phy)
-{
-	return;
-}
-
 static inline int phy_init(struct phy *phy)
 {
 	if (!phy)
-- 
2.7.4

[PATCH v2 3/3] phy: core: Update the runtime PM section in the docs to reflect changes

[Ulf Hansson]

Let's update and clarify he phy documentation, to reflect the latest
changes around the runtime PM deployment in the phy core.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-doc@vger.kernel.org
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/phy.txt | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/Documentation/phy.txt b/Documentation/phy.txt
index 457c3e0..1c2c761 100644
--- a/Documentation/phy.txt
+++ b/Documentation/phy.txt
@@ -160,19 +160,22 @@ associated with this PHY.
 PM Runtime
 ==========
 
-This subsystem is pm runtime enabled. So while creating the PHY,
-pm_runtime_enable of the phy device created by this subsystem is called and
-while destroying the PHY, pm_runtime_disable is called. Note that the phy
-device created by this subsystem will be a child of the device that calls
-phy_create (PHY provider device).
-
-So pm_runtime_get_sync of the phy_device created by this subsystem will invoke
-pm_runtime_get_sync of PHY provider device because of parent-child relationship.
-It should also be noted that phy_power_on and phy_power_off performs
-phy_pm_runtime_get_sync and phy_pm_runtime_put respectively.
-There are exported APIs like phy_pm_runtime_get, phy_pm_runtime_get_sync,
-phy_pm_runtime_put, phy_pm_runtime_put_sync, phy_pm_runtime_allow and
-phy_pm_runtime_forbid for performing PM operations.
+This subsystem deploys runtime PM support. More precisely, calls to
+pm_runtime_get_sync() and to pm_runtime_put() surrounds calls to the phy
+provider callbacks, ->init|exit(), in phy_init|exit(). At phy_power_on(), the
+runtime PM usage count is raised again, via pm_runtime_get_sync(). The usage
+count remain raised, until the internal phy power on count reaches zero in
+phy_power_off(), at which point pm_runtime_put() is called to restore the
+runtime PM usage count. In this way, the device is guranteed to stay runtime
+resumed as long as the phy is powered on.
+
+In regards to the runtime PM deployment in the phy core, it should also be
+noted that it's deployed for the phy provider device, which is the parent of
+the phy child device. In other words, the phy device created by the phy core
+remains runtime PM disabled. Of course, whether runtime PM is really used or
+not, depends on whether the phy provider driver has enabled runtime PM for its
+provider device. More exactly, pm_runtime_enable() needs to be called prior
+calling phy_create() or devm_phy_create().
 
 PHY Mappings
 ============
-- 
2.7.4

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The runtime PM deployment in the phy core is deployed using the phy core
> device, which is created by the phy core and assigned as a child device of
> the phy provider device.
>
> The behaviour around the runtime PM deployment cause some issues during
> system suspend, in cases when the phy provider device is put into a low
> power state via a call to the pm_runtime_force_suspend() helper, as is the
> case for a Renesas SoC, which has its phy provider device attached to the
> generic PM domain.
>
> In more detail, the problem in this case is that pm_runtime_force_suspend()
> expects the child device of the provider device, which is the phy core
> device, to be runtime suspended, else a WARN splat will be printed
> (correctly) when runtime PM gets re-enabled at system resume.

So we are now trying to work around issues with
pm_runtime_force_suspend().  Lovely. :-/

> In the current scenario, even if a call to phy_power_off() triggers it to
> invoke pm_runtime_put() during system suspend, the phy core device doesn't
> get runtime suspended, because this is prevented in the system suspend
> phases by the PM core.
>
> To solve this problem, let's move the runtime PM deployment from the phy
> core device to the phy provider device, as this provides the similar
> behaviour. Changing this makes it redundant to enable runtime PM for the
> phy core device, so let's avoid doing that.

I'm not really convinced that this approach is the best one to be honest.

I'll have a deeper look at this in the next few days, stay tuned.

Thanks,
Rafael

RE: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Yoshihiro Shimoda]

Hi Ulf-san,

> -----Original Message-----
> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
<snip>
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index b4298a1..050b620 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -17,7 +17,6 @@
>  #include <linux/err.h>
>  #include <linux/of.h>
>  #include <linux/device.h>
> -#include <linux/pm_runtime.h>

After I applied this patch, some ata and gpu drivers causes build error [1].
So, we should fix the drivers at first...

Best regards,
Yoshihiro Shimoda
---
[1]
drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_resume':
drivers/ata/ahci_qoriq.c:318:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
drivers/ata/ahci_qoriq.c:319:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
  pm_runtime_set_active(dev);
  ^
drivers/ata/ahci_qoriq.c:320:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/ata/ahci.c: In function 'ahci_init_one':
drivers/ata/ahci.c:1761:2: error: implicit declaration of function 'pm_runtime_put_noidle' [-Werror=implicit-function-declaration]
  pm_runtime_put_noidle(&pdev->dev);
  ^
drivers/ata/ahci.c: In function 'ahci_remove_one':
drivers/ata/ahci.c:1767:2: error: implicit declaration of function 'pm_runtime_get_noresume' [-Werror=implicit-function-declaration]
  pm_runtime_get_noresume(&pdev->dev);
  ^
drivers/ata/libahci.c: In function 'ahci_rpm_get_port':
drivers/ata/libahci.c:239:9: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
  return pm_runtime_get_sync(ap->dev);
         ^
drivers/ata/libahci.c: In function 'ahci_rpm_put_port':
drivers/ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
  pm_runtime_put(ap->dev);
  ^
drivers/ata/ahci_ceva.c: In function 'ceva_ahci_resume':
drivers/ata/ahci_ceva.c:326:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
drivers/ata/ahci_ceva.c:327:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
  pm_runtime_set_active(dev);
  ^
drivers/ata/ahci_ceva.c:328:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/ahci_qoriq.o] Error 1
make[2]: *** Waiting for unfinished jobs....
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/ahci_ceva.o] Error 1
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/ahci.o] Error 1
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_get_modes':
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:949:3: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
   pm_runtime_get_sync(dp->dev);
   ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:951:3: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
   pm_runtime_put(dp->dev);
   ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bridge_disable':
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1120:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
  pm_runtime_put_sync(dp->dev);
  ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bind':
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1387:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1431:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_enable':
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1385:2: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
  pm_runtime_get_sync(dsi->dev);
  ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1392:3: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
   pm_runtime_put_sync(dsi->dev);
   ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_probe':
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1797:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_remove':
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1808:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(&pdev->dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_enable':
drivers/gpu/drm/rockchip/cdn-dp-core.c:111:8: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
  ret = pm_runtime_get_sync(dp->dev);
        ^
drivers/gpu/drm/rockchip/cdn-dp-core.c:137:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
  pm_runtime_put(dp->dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_disable':
drivers/gpu/drm/rockchip/cdn-dp-core.c:148:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
  pm_runtime_put_sync(dp->dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_bind':
drivers/gpu/drm/rockchip/cdn-dp-core.c:1094:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_unbind':
drivers/gpu/drm/rockchip/cdn-dp-core.c:1118:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/libahci.o] Error 1
make[1]: *** [drivers/ata] Error 2
make[1]: *** Waiting for unfinished jobs....
cc1: some warnings being treated as errors
make[5]: *** [drivers/gpu/drm/bridge/analogix/analogix_dp_core.o] Error 1
make[4]: *** [drivers/gpu/drm/bridge/analogix] Error 2
make[3]: *** [drivers/gpu/drm/bridge] Error 2
make[3]: *** Waiting for unfinished jobs....
cc1: some warnings being treated as errors
make[4]: *** [drivers/gpu/drm/exynos/exynos_drm_dsi.o] Error 1
make[3]: *** [drivers/gpu/drm/exynos] Error 2
cc1: some warnings being treated as errors
make[4]: *** [drivers/gpu/drm/rockchip/cdn-dp-core.o] Error 1
make[3]: *** [drivers/gpu/drm/rockchip] Error 2
make[2]: *** [drivers/gpu/drm] Error 2
make[1]: *** [drivers/gpu] Error 2
make: *** [drivers] Error 2

RE: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Yoshihiro Shimoda]

Hi Ulf-san,

> -----Original Message-----
> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
<snip>
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index b4298a1..050b620 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -17,7 +17,6 @@
>  #include <linux/err.h>
>  #include <linux/of.h>
>  #include <linux/device.h>
> -#include <linux/pm_runtime.h>

After I applied this patch, some ata and gpu drivers causes build error [1].
So, we should fix the drivers at first...

Best regards,
Yoshihiro Shimoda
---
[1]
drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_resume':
drivers/ata/ahci_qoriq.c:318:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
drivers/ata/ahci_qoriq.c:319:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
  pm_runtime_set_active(dev);
  ^
drivers/ata/ahci_qoriq.c:320:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/ata/ahci.c: In function 'ahci_init_one':
drivers/ata/ahci.c:1761:2: error: implicit declaration of function 'pm_runtime_put_noidle' [-Werror=implicit-function-declaration]
  pm_runtime_put_noidle(&pdev->dev);
  ^
drivers/ata/ahci.c: In function 'ahci_remove_one':
drivers/ata/ahci.c:1767:2: error: implicit declaration of function 'pm_runtime_get_noresume' [-Werror=implicit-function-declaration]
  pm_runtime_get_noresume(&pdev->dev);
  ^
drivers/ata/libahci.c: In function 'ahci_rpm_get_port':
drivers/ata/libahci.c:239:9: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
  return pm_runtime_get_sync(ap->dev);
         ^
drivers/ata/libahci.c: In function 'ahci_rpm_put_port':
drivers/ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
  pm_runtime_put(ap->dev);
  ^
drivers/ata/ahci_ceva.c: In function 'ceva_ahci_resume':
drivers/ata/ahci_ceva.c:326:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
drivers/ata/ahci_ceva.c:327:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
  pm_runtime_set_active(dev);
  ^
drivers/ata/ahci_ceva.c:328:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/ahci_qoriq.o] Error 1
make[2]: *** Waiting for unfinished jobs....
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/ahci_ceva.o] Error 1
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/ahci.o] Error 1
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_get_modes':
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:949:3: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
   pm_runtime_get_sync(dp->dev);
   ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:951:3: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
   pm_runtime_put(dp->dev);
   ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bridge_disable':
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1120:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
  pm_runtime_put_sync(dp->dev);
  ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bind':
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1387:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1431:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_enable':
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1385:2: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
  pm_runtime_get_sync(dsi->dev);
  ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1392:3: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
   pm_runtime_put_sync(dsi->dev);
   ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_probe':
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1797:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_remove':
drivers/gpu/drm/exynos/exynos_drm_dsi.c:1808:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(&pdev->dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_enable':
drivers/gpu/drm/rockchip/cdn-dp-core.c:111:8: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
  ret = pm_runtime_get_sync(dp->dev);
        ^
drivers/gpu/drm/rockchip/cdn-dp-core.c:137:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
  pm_runtime_put(dp->dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_disable':
drivers/gpu/drm/rockchip/cdn-dp-core.c:148:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
  pm_runtime_put_sync(dp->dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_bind':
drivers/gpu/drm/rockchip/cdn-dp-core.c:1094:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
  pm_runtime_enable(dev);
  ^
drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_unbind':
drivers/gpu/drm/rockchip/cdn-dp-core.c:1118:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
  pm_runtime_disable(dev);
  ^
cc1: some warnings being treated as errors
make[2]: *** [drivers/ata/libahci.o] Error 1
make[1]: *** [drivers/ata] Error 2
make[1]: *** Waiting for unfinished jobs....
cc1: some warnings being treated as errors
make[5]: *** [drivers/gpu/drm/bridge/analogix/analogix_dp_core.o] Error 1
make[4]: *** [drivers/gpu/drm/bridge/analogix] Error 2
make[3]: *** [drivers/gpu/drm/bridge] Error 2
make[3]: *** Waiting for unfinished jobs....
cc1: some warnings being treated as errors
make[4]: *** [drivers/gpu/drm/exynos/exynos_drm_dsi.o] Error 1
make[3]: *** [drivers/gpu/drm/exynos] Error 2
cc1: some warnings being treated as errors
make[4]: *** [drivers/gpu/drm/rockchip/cdn-dp-core.o] Error 1
make[3]: *** [drivers/gpu/drm/rockchip] Error 2
make[2]: *** [drivers/gpu/drm] Error 2
make[1]: *** [drivers/gpu] Error 2
make: *** [drivers] Error 2

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> The runtime PM deployment in the phy core is deployed using the phy core
>> device, which is created by the phy core and assigned as a child device of
>> the phy provider device.
>>
>> The behaviour around the runtime PM deployment cause some issues during
>> system suspend, in cases when the phy provider device is put into a low
>> power state via a call to the pm_runtime_force_suspend() helper, as is the
>> case for a Renesas SoC, which has its phy provider device attached to the
>> generic PM domain.
>>
>> In more detail, the problem in this case is that pm_runtime_force_suspend()
>> expects the child device of the provider device, which is the phy core
>> device, to be runtime suspended, else a WARN splat will be printed
>> (correctly) when runtime PM gets re-enabled at system resume.
>
> So we are now trying to work around issues with
> pm_runtime_force_suspend().  Lovely. :-/

Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or
are you saying we should just ignore all issues related to it?

Of course, if we had something that could replace
pm_runtime_force_suspend(), that would be great, but there isn't.

>
>> In the current scenario, even if a call to phy_power_off() triggers it to
>> invoke pm_runtime_put() during system suspend, the phy core device doesn't
>> get runtime suspended, because this is prevented in the system suspend
>> phases by the PM core.
>>
>> To solve this problem, let's move the runtime PM deployment from the phy
>> core device to the phy provider device, as this provides the similar
>> behaviour. Changing this makes it redundant to enable runtime PM for the
>> phy core device, so let's avoid doing that.
>
> I'm not really convinced that this approach is the best one to be honest.
>
> I'll have a deeper look at this in the next few days, stay tuned.

There is different ways to solve this, for sure. I picked this one,
because I think it's the most trivial thing to do, and it shouldn't
cause any other problems.

I think any other option would involve assigning ->suspend|resume()
callbacks to the phy core device, but that's fine too, if you prefer
that.

Also, I have considered how to deal with wakeup paths for phys,
although I didn't want to post changes as a part of this series, but
maybe I should to give a more complete picture?

Kind regards
Uffe

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Ulf Hansson]

On 21 December 2017 at 11:33, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Hi Ulf-san,
>
>> -----Original Message-----
>> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
> <snip>
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> index b4298a1..050b620 100644
>> --- a/include/linux/phy/phy.h
>> +++ b/include/linux/phy/phy.h
>> @@ -17,7 +17,6 @@
>>  #include <linux/err.h>
>>  #include <linux/of.h>
>>  #include <linux/device.h>
>> -#include <linux/pm_runtime.h>
>
> After I applied this patch, some ata and gpu drivers causes build error [1].
> So, we should fix the drivers at first...

Huh, right, those drivers shouldn't be relying on the phy.h to include
pm_runtime.h.

The easiest way at this point is to just put back "#include
<linux/pm_runtime.h>" in phy.h, then we can deal with these problems
separately. I do that in a re-spin soon.

BTW, I would be great if you could test this on the Renesas SoC to
make sure it still fixes the problems (at least half of them I mean).

Thanks and kind regards
Uffe

>
> Best regards,
> Yoshihiro Shimoda
> ---
> [1]
> drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_resume':
> drivers/ata/ahci_qoriq.c:318:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> drivers/ata/ahci_qoriq.c:319:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
>   pm_runtime_set_active(dev);
>   ^
> drivers/ata/ahci_qoriq.c:320:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/ata/ahci.c: In function 'ahci_init_one':
> drivers/ata/ahci.c:1761:2: error: implicit declaration of function 'pm_runtime_put_noidle' [-Werror=implicit-function-declaration]
>   pm_runtime_put_noidle(&pdev->dev);
>   ^
> drivers/ata/ahci.c: In function 'ahci_remove_one':
> drivers/ata/ahci.c:1767:2: error: implicit declaration of function 'pm_runtime_get_noresume' [-Werror=implicit-function-declaration]
>   pm_runtime_get_noresume(&pdev->dev);
>   ^
> drivers/ata/libahci.c: In function 'ahci_rpm_get_port':
> drivers/ata/libahci.c:239:9: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>   return pm_runtime_get_sync(ap->dev);
>          ^
> drivers/ata/libahci.c: In function 'ahci_rpm_put_port':
> drivers/ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
>   pm_runtime_put(ap->dev);
>   ^
> drivers/ata/ahci_ceva.c: In function 'ceva_ahci_resume':
> drivers/ata/ahci_ceva.c:326:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> drivers/ata/ahci_ceva.c:327:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
>   pm_runtime_set_active(dev);
>   ^
> drivers/ata/ahci_ceva.c:328:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/ahci_qoriq.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/ahci_ceva.o] Error 1
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/ahci.o] Error 1
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_get_modes':
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:949:3: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>    pm_runtime_get_sync(dp->dev);
>    ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:951:3: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
>    pm_runtime_put(dp->dev);
>    ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bridge_disable':
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1120:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
>   pm_runtime_put_sync(dp->dev);
>   ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bind':
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1387:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1431:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_enable':
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1385:2: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>   pm_runtime_get_sync(dsi->dev);
>   ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1392:3: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
>    pm_runtime_put_sync(dsi->dev);
>    ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_probe':
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1797:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_remove':
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1808:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(&pdev->dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_enable':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:111:8: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>   ret = pm_runtime_get_sync(dp->dev);
>         ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c:137:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
>   pm_runtime_put(dp->dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_disable':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:148:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
>   pm_runtime_put_sync(dp->dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_bind':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:1094:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_unbind':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:1118:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/libahci.o] Error 1
> make[1]: *** [drivers/ata] Error 2
> make[1]: *** Waiting for unfinished jobs....
> cc1: some warnings being treated as errors
> make[5]: *** [drivers/gpu/drm/bridge/analogix/analogix_dp_core.o] Error 1
> make[4]: *** [drivers/gpu/drm/bridge/analogix] Error 2
> make[3]: *** [drivers/gpu/drm/bridge] Error 2
> make[3]: *** Waiting for unfinished jobs....
> cc1: some warnings being treated as errors
> make[4]: *** [drivers/gpu/drm/exynos/exynos_drm_dsi.o] Error 1
> make[3]: *** [drivers/gpu/drm/exynos] Error 2
> cc1: some warnings being treated as errors
> make[4]: *** [drivers/gpu/drm/rockchip/cdn-dp-core.o] Error 1
> make[3]: *** [drivers/gpu/drm/rockchip] Error 2
> make[2]: *** [drivers/gpu/drm] Error 2
> make[1]: *** [drivers/gpu] Error 2
> make: *** [drivers] Error 2
>

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Ulf Hansson]

On 21 December 2017 at 11:33, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Hi Ulf-san,
>
>> -----Original Message-----
>> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
> <snip>
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> index b4298a1..050b620 100644
>> --- a/include/linux/phy/phy.h
>> +++ b/include/linux/phy/phy.h
>> @@ -17,7 +17,6 @@
>>  #include <linux/err.h>
>>  #include <linux/of.h>
>>  #include <linux/device.h>
>> -#include <linux/pm_runtime.h>
>
> After I applied this patch, some ata and gpu drivers causes build error [1].
> So, we should fix the drivers at first...

Huh, right, those drivers shouldn't be relying on the phy.h to include
pm_runtime.h.

The easiest way at this point is to just put back "#include
<linux/pm_runtime.h>" in phy.h, then we can deal with these problems
separately. I do that in a re-spin soon.

BTW, I would be great if you could test this on the Renesas SoC to
make sure it still fixes the problems (at least half of them I mean).

Thanks and kind regards
Uffe

>
> Best regards,
> Yoshihiro Shimoda
> ---
> [1]
> drivers/ata/ahci_qoriq.c: In function 'ahci_qoriq_resume':
> drivers/ata/ahci_qoriq.c:318:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> drivers/ata/ahci_qoriq.c:319:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
>   pm_runtime_set_active(dev);
>   ^
> drivers/ata/ahci_qoriq.c:320:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/ata/ahci.c: In function 'ahci_init_one':
> drivers/ata/ahci.c:1761:2: error: implicit declaration of function 'pm_runtime_put_noidle' [-Werror=implicit-function-declaration]
>   pm_runtime_put_noidle(&pdev->dev);
>   ^
> drivers/ata/ahci.c: In function 'ahci_remove_one':
> drivers/ata/ahci.c:1767:2: error: implicit declaration of function 'pm_runtime_get_noresume' [-Werror=implicit-function-declaration]
>   pm_runtime_get_noresume(&pdev->dev);
>   ^
> drivers/ata/libahci.c: In function 'ahci_rpm_get_port':
> drivers/ata/libahci.c:239:9: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>   return pm_runtime_get_sync(ap->dev);
>          ^
> drivers/ata/libahci.c: In function 'ahci_rpm_put_port':
> drivers/ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
>   pm_runtime_put(ap->dev);
>   ^
> drivers/ata/ahci_ceva.c: In function 'ceva_ahci_resume':
> drivers/ata/ahci_ceva.c:326:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> drivers/ata/ahci_ceva.c:327:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
>   pm_runtime_set_active(dev);
>   ^
> drivers/ata/ahci_ceva.c:328:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/ahci_qoriq.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/ahci_ceva.o] Error 1
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/ahci.o] Error 1
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_get_modes':
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:949:3: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>    pm_runtime_get_sync(dp->dev);
>    ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:951:3: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
>    pm_runtime_put(dp->dev);
>    ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bridge_disable':
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1120:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
>   pm_runtime_put_sync(dp->dev);
>   ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c: In function 'analogix_dp_bind':
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1387:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1431:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_enable':
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1385:2: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>   pm_runtime_get_sync(dsi->dev);
>   ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1392:3: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
>    pm_runtime_put_sync(dsi->dev);
>    ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_probe':
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1797:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/gpu/drm/exynos/exynos_drm_dsi.c: In function 'exynos_dsi_remove':
> drivers/gpu/drm/exynos/exynos_drm_dsi.c:1808:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(&pdev->dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_enable':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:111:8: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
>   ret = pm_runtime_get_sync(dp->dev);
>         ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c:137:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
>   pm_runtime_put(dp->dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_clk_disable':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:148:2: error: implicit declaration of function 'pm_runtime_put_sync' [-Werror=implicit-function-declaration]
>   pm_runtime_put_sync(dp->dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_bind':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:1094:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
>   pm_runtime_enable(dev);
>   ^
> drivers/gpu/drm/rockchip/cdn-dp-core.c: In function 'cdn_dp_unbind':
> drivers/gpu/drm/rockchip/cdn-dp-core.c:1118:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
>   pm_runtime_disable(dev);
>   ^
> cc1: some warnings being treated as errors
> make[2]: *** [drivers/ata/libahci.o] Error 1
> make[1]: *** [drivers/ata] Error 2
> make[1]: *** Waiting for unfinished jobs....
> cc1: some warnings being treated as errors
> make[5]: *** [drivers/gpu/drm/bridge/analogix/analogix_dp_core.o] Error 1
> make[4]: *** [drivers/gpu/drm/bridge/analogix] Error 2
> make[3]: *** [drivers/gpu/drm/bridge] Error 2
> make[3]: *** Waiting for unfinished jobs....
> cc1: some warnings being treated as errors
> make[4]: *** [drivers/gpu/drm/exynos/exynos_drm_dsi.o] Error 1
> make[3]: *** [drivers/gpu/drm/exynos] Error 2
> cc1: some warnings being treated as errors
> make[4]: *** [drivers/gpu/drm/rockchip/cdn-dp-core.o] Error 1
> make[3]: *** [drivers/gpu/drm/rockchip] Error 2
> make[2]: *** [drivers/gpu/drm] Error 2
> make[1]: *** [drivers/gpu] Error 2
> make: *** [drivers] Error 2
>

RE: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Yoshihiro Shimoda]

> From: Ulf Hansson, Sent: Thursday, December 21, 2017 7:58 PM
> 
> On 21 December 2017 at 11:33, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Hi Ulf-san,
> >
> >> -----Original Message-----
> >> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
> > <snip>
> >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> >> index b4298a1..050b620 100644
> >> --- a/include/linux/phy/phy.h
> >> +++ b/include/linux/phy/phy.h
> >> @@ -17,7 +17,6 @@
> >>  #include <linux/err.h>
> >>  #include <linux/of.h>
> >>  #include <linux/device.h>
> >> -#include <linux/pm_runtime.h>
> >
> > After I applied this patch, some ata and gpu drivers causes build error [1].
> > So, we should fix the drivers at first...
> 
> Huh, right, those drivers shouldn't be relying on the phy.h to include
> pm_runtime.h.
> 
> The easiest way at this point is to just put back "#include
> <linux/pm_runtime.h>" in phy.h, then we can deal with these problems
> separately. I do that in a re-spin soon.

I got it.

> BTW, I would be great if you could test this on the Renesas SoC to
> make sure it still fixes the problems (at least half of them I mean).

Sure. I put back the "#include <linux/pm_runtime.h>" in the phy.h and tested
the patches on my environment (r8a7795-salvator-x.dts with v4.15-rc4). And then,
the issue [1] disappeared.
[1]:
printed "Enabling runtime PM for inactive device (ee0a0200.usb-phy) with active children"
in resume timing.

So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

RE: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Yoshihiro Shimoda]

> From: Ulf Hansson, Sent: Thursday, December 21, 2017 7:58 PM
> 
> On 21 December 2017 at 11:33, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > Hi Ulf-san,
> >
> >> -----Original Message-----
> >> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
> > <snip>
> >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> >> index b4298a1..050b620 100644
> >> --- a/include/linux/phy/phy.h
> >> +++ b/include/linux/phy/phy.h
> >> @@ -17,7 +17,6 @@
> >>  #include <linux/err.h>
> >>  #include <linux/of.h>
> >>  #include <linux/device.h>
> >> -#include <linux/pm_runtime.h>
> >
> > After I applied this patch, some ata and gpu drivers causes build error [1].
> > So, we should fix the drivers at first...
> 
> Huh, right, those drivers shouldn't be relying on the phy.h to include
> pm_runtime.h.
> 
> The easiest way at this point is to just put back "#include
> <linux/pm_runtime.h>" in phy.h, then we can deal with these problems
> separately. I do that in a re-spin soon.

I got it.

> BTW, I would be great if you could test this on the Renesas SoC to
> make sure it still fixes the problems (at least half of them I mean).

Sure. I put back the "#include <linux/pm_runtime.h>" in the phy.h and tested
the patches on my environment (r8a7795-salvator-x.dts with v4.15-rc4). And then,
the issue [1] disappeared.
[1]:
printed "Enabling runtime PM for inactive device (ee0a0200.usb-phy) with active children"
in resume timing.

So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Ulf Hansson]

On 21 December 2017 at 13:24, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
>> From: Ulf Hansson, Sent: Thursday, December 21, 2017 7:58 PM
>>
>> On 21 December 2017 at 11:33, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> > Hi Ulf-san,
>> >
>> >> -----Original Message-----
>> >> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
>> > <snip>
>> >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> >> index b4298a1..050b620 100644
>> >> --- a/include/linux/phy/phy.h
>> >> +++ b/include/linux/phy/phy.h
>> >> @@ -17,7 +17,6 @@
>> >>  #include <linux/err.h>
>> >>  #include <linux/of.h>
>> >>  #include <linux/device.h>
>> >> -#include <linux/pm_runtime.h>
>> >
>> > After I applied this patch, some ata and gpu drivers causes build error [1].
>> > So, we should fix the drivers at first...
>>
>> Huh, right, those drivers shouldn't be relying on the phy.h to include
>> pm_runtime.h.
>>
>> The easiest way at this point is to just put back "#include
>> <linux/pm_runtime.h>" in phy.h, then we can deal with these problems
>> separately. I do that in a re-spin soon.
>
> I got it.
>
>> BTW, I would be great if you could test this on the Renesas SoC to
>> make sure it still fixes the problems (at least half of them I mean).
>
> Sure. I put back the "#include <linux/pm_runtime.h>" in the phy.h and tested
> the patches on my environment (r8a7795-salvator-x.dts with v4.15-rc4). And then,
> the issue [1] disappeared.
> [1]:
> printed "Enabling runtime PM for inactive device (ee0a0200.usb-phy) with active children"
> in resume timing.
>
> So,
>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Great, thanks!

I add your tag in the next re-vision!

Kind regards
Uffe

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[Ulf Hansson]

On 21 December 2017 at 13:24, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>
>> From: Ulf Hansson, Sent: Thursday, December 21, 2017 7:58 PM
>>
>> On 21 December 2017 at 11:33, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> > Hi Ulf-san,
>> >
>> >> -----Original Message-----
>> >> From: Ulf Hansson, Sent: Wednesday, December 20, 2017 11:09 PM
>> > <snip>
>> >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> >> index b4298a1..050b620 100644
>> >> --- a/include/linux/phy/phy.h
>> >> +++ b/include/linux/phy/phy.h
>> >> @@ -17,7 +17,6 @@
>> >>  #include <linux/err.h>
>> >>  #include <linux/of.h>
>> >>  #include <linux/device.h>
>> >> -#include <linux/pm_runtime.h>
>> >
>> > After I applied this patch, some ata and gpu drivers causes build error [1].
>> > So, we should fix the drivers at first...
>>
>> Huh, right, those drivers shouldn't be relying on the phy.h to include
>> pm_runtime.h.
>>
>> The easiest way at this point is to just put back "#include
>> <linux/pm_runtime.h>" in phy.h, then we can deal with these problems
>> separately. I do that in a re-spin soon.
>
> I got it.
>
>> BTW, I would be great if you could test this on the Renesas SoC to
>> make sure it still fixes the problems (at least half of them I mean).
>
> Sure. I put back the "#include <linux/pm_runtime.h>" in the phy.h and tested
> the patches on my environment (r8a7795-salvator-x.dts with v4.15-rc4). And then,
> the issue [1] disappeared.
> [1]:
> printed "Enabling runtime PM for inactive device (ee0a0200.usb-phy) with active children"
> in resume timing.
>
> So,
>
> Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Great, thanks!

I add your tag in the next re-vision!

Kind regards
Uffe

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> The runtime PM deployment in the phy core is deployed using the phy core
>>> device, which is created by the phy core and assigned as a child device of
>>> the phy provider device.
>>>
>>> The behaviour around the runtime PM deployment cause some issues during
>>> system suspend, in cases when the phy provider device is put into a low
>>> power state via a call to the pm_runtime_force_suspend() helper, as is the
>>> case for a Renesas SoC, which has its phy provider device attached to the
>>> generic PM domain.
>>>
>>> In more detail, the problem in this case is that pm_runtime_force_suspend()
>>> expects the child device of the provider device, which is the phy core
>>> device, to be runtime suspended, else a WARN splat will be printed
>>> (correctly) when runtime PM gets re-enabled at system resume.
>>
>> So we are now trying to work around issues with
>> pm_runtime_force_suspend().  Lovely. :-/
>
> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or
> are you saying we should just ignore all issues related to it?

Or get rid of it?

Seriously, is the resulting pain worth it?

> Of course, if we had something that could replace
> pm_runtime_force_suspend(), that would be great, but there isn't.

I beg to differ.

At least some of it could be replaced with the driver flags.

>>> In the current scenario, even if a call to phy_power_off() triggers it to
>>> invoke pm_runtime_put() during system suspend, the phy core device doesn't
>>> get runtime suspended, because this is prevented in the system suspend
>>> phases by the PM core.
>>>
>>> To solve this problem, let's move the runtime PM deployment from the phy
>>> core device to the phy provider device, as this provides the similar
>>> behaviour. Changing this makes it redundant to enable runtime PM for the
>>> phy core device, so let's avoid doing that.
>>
>> I'm not really convinced that this approach is the best one to be honest.
>>
>> I'll have a deeper look at this in the next few days, stay tuned.
>
> There is different ways to solve this, for sure. I picked this one,
> because I think it's the most trivial thing to do, and it shouldn't
> cause any other problems.
>
> I think any other option would involve assigning ->suspend|resume()
> callbacks to the phy core device, but that's fine too, if you prefer
> that.
>
> Also, I have considered how to deal with wakeup paths for phys,
> although I didn't want to post changes as a part of this series, but
> maybe I should to give a more complete picture?

Yes, you should.

The point is that without genpd using pm_runtime_force_suspend() the
phy code could very well stay the way it is.  And it is logical,
because having a parent with enabled runtime PM without enabling
runtime PM for its children is at least conceptually questionable.

So the conclusion may be that the phy code is OK, but calling
pm_runtime_force_suspend() from genpd isn't.

Thanks,
Rafael

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 2:35 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> The runtime PM deployment in the phy core is deployed using the phy core
>>>> device, which is created by the phy core and assigned as a child device of
>>>> the phy provider device.

[cut]

>>
>> Also, I have considered how to deal with wakeup paths for phys,
>> although I didn't want to post changes as a part of this series, but
>> maybe I should to give a more complete picture?
>
> Yes, you should.
>
> The point is that without genpd using pm_runtime_force_suspend() the
> phy code could very well stay the way it is.  And it is logical,
> because having a parent with enabled runtime PM without enabling
> runtime PM for its children is at least conceptually questionable.

Actually, I sort of agree that the phy's usage of runtime PM is too
convoluted.  For example, it uses pm_runtime_enabled() unnecessarily
at least in some places, but that doesn't seem to be fixed by your
patches.

Thanks,
Rafael

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[kbuild test robot]

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

Hi Ulf,

I love your patch! Yet something to improve:

[auto build test ERROR on phy/next]
[also build test ERROR on v4.15-rc4 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ulf-Hansson/phy-core-Re-work-runtime-PM-deployment-and-fix-an-issue/20171223-170432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next
config: i386-randconfig-a0-201751 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers//ata/ahci.c: In function 'ahci_init_one':
>> drivers//ata/ahci.c:1761:2: error: implicit declaration of function 'pm_runtime_put_noidle' [-Werror=implicit-function-declaration]
     pm_runtime_put_noidle(&pdev->dev);
     ^
   drivers//ata/ahci.c: In function 'ahci_remove_one':
>> drivers//ata/ahci.c:1767:2: error: implicit declaration of function 'pm_runtime_get_noresume' [-Werror=implicit-function-declaration]
     pm_runtime_get_noresume(&pdev->dev);
     ^
   cc1: some warnings being treated as errors
--
   drivers//ata/libahci.c: In function 'ahci_rpm_get_port':
>> drivers//ata/libahci.c:239:2: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
     return pm_runtime_get_sync(ap->dev);
     ^
   drivers//ata/libahci.c: In function 'ahci_rpm_put_port':
>> drivers//ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
     pm_runtime_put(ap->dev);
     ^
   cc1: some warnings being treated as errors
--
   drivers//ata/ahci_ceva.c: In function 'ceva_ahci_resume':
>> drivers//ata/ahci_ceva.c:326:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
     pm_runtime_disable(dev);
     ^
>> drivers//ata/ahci_ceva.c:327:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
     pm_runtime_set_active(dev);
     ^
>> drivers//ata/ahci_ceva.c:328:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
     pm_runtime_enable(dev);
     ^
   cc1: some warnings being treated as errors
--
   drivers//ata/ahci_qoriq.c: In function 'ahci_qoriq_resume':
>> drivers//ata/ahci_qoriq.c:306:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
     pm_runtime_disable(dev);
     ^
>> drivers//ata/ahci_qoriq.c:307:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
     pm_runtime_set_active(dev);
     ^
>> drivers//ata/ahci_qoriq.c:308:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
     pm_runtime_enable(dev);
     ^
   cc1: some warnings being treated as errors

vim +/pm_runtime_put_noidle +1761 drivers//ata/ahci.c

d243bed32f drivers/ata/ahci.c  Tirumalesh Chalamarla   2016-02-16  1642  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1643  	/* save initial config */
394d6e535f drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1644  	ahci_pci_save_initial_config(pdev, hpriv);
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1645  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1646  	/* prepare host */
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1647  	if (hpriv->cap & HOST_CAP_NCQ) {
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1648  		pi.flags |= ATA_FLAG_NCQ;
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1649  		/*
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1650  		 * Auto-activate optimization is supposed to be
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1651  		 * supported on all AHCI controllers indicating NCQ
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1652  		 * capability, but it seems to be broken on some
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1653  		 * chipsets including NVIDIAs.
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1654  		 */
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1655  		if (!(hpriv->flags & AHCI_HFLAG_NO_FPDMA_AA))
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1656  			pi.flags |= ATA_FLAG_FPDMA_AA;
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1657  
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1658  		/*
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1659  		 * All AHCI controllers should be forward-compatible
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1660  		 * with the new auxiliary field. This code should be
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1661  		 * conditionalized if any buggy AHCI controllers are
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1662  		 * encountered.
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1663  		 */
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1664  		pi.flags |= ATA_FLAG_FPDMA_AUX;
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1665  	}
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1666  
7d50b60b5e drivers/ata/ahci.c  Tejun Heo               2007-09-23  1667  	if (hpriv->cap & HOST_CAP_PMP)
7d50b60b5e drivers/ata/ahci.c  Tejun Heo               2007-09-23  1668  		pi.flags |= ATA_FLAG_PMP;
7d50b60b5e drivers/ata/ahci.c  Tejun Heo               2007-09-23  1669  
0cbb0e774b drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1670  	ahci_set_em_messages(hpriv, &pi);
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1671  
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1672  	if (ahci_broken_system_poweroff(pdev)) {
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1673  		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN;
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1674  		dev_info(&pdev->dev,
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1675  			"quirky BIOS, skipping spindown on poweroff\n");
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1676  	}
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1677  
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1678  	if (ahci_broken_suspend(pdev)) {
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1679  		hpriv->flags |= AHCI_HFLAG_NO_SUSPEND;
a44fec1fce drivers/ata/ahci.c  Joe Perches             2011-04-15  1680  		dev_warn(&pdev->dev,
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1681  			 "BIOS update required for suspend/resume\n");
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1682  	}
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1683  
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1684  	if (ahci_broken_online(pdev)) {
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1685  		hpriv->flags |= AHCI_HFLAG_SRST_TOUT_IS_OFFLINE;
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1686  		dev_info(&pdev->dev,
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1687  			 "online status unreliable, applying workaround\n");
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1688  	}
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1689  
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1690  
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1691  	/* Acer SA5-271 workaround modifies private_data */
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1692  	acer_sa5_271_workaround(hpriv, pdev);
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1693  
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1694  	/* CAP.NP sometimes indicate the index of the last enabled
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1695  	 * port, at other times, that of the last possible port, so
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1696  	 * determining the maximum port number requires looking at
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1697  	 * both CAP.NP and port_map.
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1698  	 */
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1699  	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1700  
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1701  	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1702  	if (!host)
24dc5f33ea drivers/ata/ahci.c  Tejun Heo               2007-01-20  1703  		return -ENOMEM;
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1704  	host->private_data = hpriv;
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1705  
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1706  	if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1707  		/* legacy intx interrupts */
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1708  		pci_intx(pdev, 1);
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1709  	}
0ce57f8af1 drivers/ata/ahci.c  Christoph Hellwig       2016-10-25  1710  	hpriv->irq = pci_irq_vector(pdev, 0);
21bfd1aa95 drivers/ata/ahci.c  Robert Richter          2015-05-31  1711  
f3d7f23f87 drivers/ata/ahci.c  Arjan van de Ven        2009-01-26  1712  	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
886ad09fc8 drivers/ata/ahci.c  Arjan van de Ven        2009-01-09  1713  		host->flags |= ATA_HOST_PARALLEL_SCAN;
f3d7f23f87 drivers/ata/ahci.c  Arjan van de Ven        2009-01-26  1714  	else
d2782d96f3 drivers/ata/ahci.c  Jingoo Han              2013-10-05  1715  		dev_info(&pdev->dev, "SSS flag set, parallel bus scan disabled\n");
886ad09fc8 drivers/ata/ahci.c  Arjan van de Ven        2009-01-09  1716  
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1717  	if (pi.flags & ATA_FLAG_EM)
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1718  		ahci_reset_em(host);
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1719  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1720  	for (i = 0; i < host->n_ports; i++) {
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1721  		struct ata_port *ap = host->ports[i];
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1722  
318893e142 drivers/ata/ahci.c  Alessandro Rubini       2012-01-06  1723  		ata_port_pbar_desc(ap, ahci_pci_bar, -1, "abar");
318893e142 drivers/ata/ahci.c  Alessandro Rubini       2012-01-06  1724  		ata_port_pbar_desc(ap, ahci_pci_bar,
cbcdd87593 drivers/ata/ahci.c  Tejun Heo               2007-08-18  1725  				   0x100 + ap->port_no * 0x80, "port");
cbcdd87593 drivers/ata/ahci.c  Tejun Heo               2007-08-18  1726  
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1727  		/* set enclosure management message type */
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1728  		if (ap->flags & ATA_FLAG_EM)
008dbd61eb drivers/ata/ahci.c  Harry Zhang             2010-04-23  1729  			ap->em_message_type = hpriv->em_msg_type;
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1730  
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1731  
dab632e8c4 drivers/ata/ahci.c  Jeff Garzik             2007-05-28  1732  		/* disabled/not-implemented port */
350756f6da drivers/ata/ahci.c  Tejun Heo               2008-04-07  1733  		if (!(hpriv->port_map & (1 << i)))
dab632e8c4 drivers/ata/ahci.c  Jeff Garzik             2007-05-28  1734  			ap->ops = &ata_dummy_port_ops;
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1735  	}
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1736  
edc9305284 drivers/ata/ahci.c  Tejun Heo               2007-10-25  1737  	/* apply workaround for ASUS P5W DH Deluxe mainboard */
edc9305284 drivers/ata/ahci.c  Tejun Heo               2007-10-25  1738  	ahci_p5wdh_workaround(host);
edc9305284 drivers/ata/ahci.c  Tejun Heo               2007-10-25  1739  
f80ae7e45a drivers/ata/ahci.c  Tejun Heo               2009-09-16  1740  	/* apply gtf filter quirk */
f80ae7e45a drivers/ata/ahci.c  Tejun Heo               2009-09-16  1741  	ahci_gtf_filter_workaround(host);
f80ae7e45a drivers/ata/ahci.c  Tejun Heo               2009-09-16  1742  
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1743  	/* initialize adapter */
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1744  	rc = ahci_configure_dma_masks(pdev, hpriv->cap & HOST_CAP_64);
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1745  	if (rc)
24dc5f33ea drivers/ata/ahci.c  Tejun Heo               2007-01-20  1746  		return rc;
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1747  
3303040d8b drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1748  	rc = ahci_pci_reset_controller(host);
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1749  	if (rc)
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1750  		return rc;
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1751  
781d655083 drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1752  	ahci_pci_init_controller(host);
439fcaec10 drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1753  	ahci_pci_print_info(host);
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1754  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1755  	pci_set_master(pdev);
5ca72c4f7c drivers/ata/ahci.c  Alexander Gordeev       2012-11-19  1756  
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1757  	rc = ahci_host_activate(host, &ahci_sht);
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1758  	if (rc)
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1759  		return rc;
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1760  
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18 @1761  	pm_runtime_put_noidle(&pdev->dev);
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1762  	return 0;
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1763  }
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1764  
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1765  static void ahci_remove_one(struct pci_dev *pdev)
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1766  {
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18 @1767  	pm_runtime_get_noresume(&pdev->dev);
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1768  	ata_pci_remove_one(pdev);
907f4678c1 drivers/scsi/ahci.c Jeff Garzik             2005-05-12  1769  }
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1770  

:::::: The code at line 1761 was first introduced by commit
:::::: 02e53293eafcb19b4fabc8a2e7bbfa11b88d0e6c ahci: Add runtime PM support for the host controller

:::::: TO: Mika Westerberg <mika.westerberg@linux.intel.com>
:::::: CC: Tejun Heo <tj@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24293 bytes --]

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[kbuild test robot]

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

Hi Ulf,

I love your patch! Yet something to improve:

[auto build test ERROR on phy/next]
[also build test ERROR on v4.15-rc4 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ulf-Hansson/phy-core-Re-work-runtime-PM-deployment-and-fix-an-issue/20171223-170432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next
config: i386-randconfig-a0-201751 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers//ata/ahci.c: In function 'ahci_init_one':
>> drivers//ata/ahci.c:1761:2: error: implicit declaration of function 'pm_runtime_put_noidle' [-Werror=implicit-function-declaration]
     pm_runtime_put_noidle(&pdev->dev);
     ^
   drivers//ata/ahci.c: In function 'ahci_remove_one':
>> drivers//ata/ahci.c:1767:2: error: implicit declaration of function 'pm_runtime_get_noresume' [-Werror=implicit-function-declaration]
     pm_runtime_get_noresume(&pdev->dev);
     ^
   cc1: some warnings being treated as errors
--
   drivers//ata/libahci.c: In function 'ahci_rpm_get_port':
>> drivers//ata/libahci.c:239:2: error: implicit declaration of function 'pm_runtime_get_sync' [-Werror=implicit-function-declaration]
     return pm_runtime_get_sync(ap->dev);
     ^
   drivers//ata/libahci.c: In function 'ahci_rpm_put_port':
>> drivers//ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put' [-Werror=implicit-function-declaration]
     pm_runtime_put(ap->dev);
     ^
   cc1: some warnings being treated as errors
--
   drivers//ata/ahci_ceva.c: In function 'ceva_ahci_resume':
>> drivers//ata/ahci_ceva.c:326:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
     pm_runtime_disable(dev);
     ^
>> drivers//ata/ahci_ceva.c:327:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
     pm_runtime_set_active(dev);
     ^
>> drivers//ata/ahci_ceva.c:328:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
     pm_runtime_enable(dev);
     ^
   cc1: some warnings being treated as errors
--
   drivers//ata/ahci_qoriq.c: In function 'ahci_qoriq_resume':
>> drivers//ata/ahci_qoriq.c:306:2: error: implicit declaration of function 'pm_runtime_disable' [-Werror=implicit-function-declaration]
     pm_runtime_disable(dev);
     ^
>> drivers//ata/ahci_qoriq.c:307:2: error: implicit declaration of function 'pm_runtime_set_active' [-Werror=implicit-function-declaration]
     pm_runtime_set_active(dev);
     ^
>> drivers//ata/ahci_qoriq.c:308:2: error: implicit declaration of function 'pm_runtime_enable' [-Werror=implicit-function-declaration]
     pm_runtime_enable(dev);
     ^
   cc1: some warnings being treated as errors

vim +/pm_runtime_put_noidle +1761 drivers//ata/ahci.c

d243bed32f drivers/ata/ahci.c  Tirumalesh Chalamarla   2016-02-16  1642  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1643  	/* save initial config */
394d6e535f drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1644  	ahci_pci_save_initial_config(pdev, hpriv);
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1645  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1646  	/* prepare host */
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1647  	if (hpriv->cap & HOST_CAP_NCQ) {
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1648  		pi.flags |= ATA_FLAG_NCQ;
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1649  		/*
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1650  		 * Auto-activate optimization is supposed to be
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1651  		 * supported on all AHCI controllers indicating NCQ
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1652  		 * capability, but it seems to be broken on some
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1653  		 * chipsets including NVIDIAs.
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1654  		 */
83f2b9630c drivers/ata/ahci.c  Tejun Heo               2010-03-30  1655  		if (!(hpriv->flags & AHCI_HFLAG_NO_FPDMA_AA))
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1656  			pi.flags |= ATA_FLAG_FPDMA_AA;
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1657  
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1658  		/*
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1659  		 * All AHCI controllers should be forward-compatible
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1660  		 * with the new auxiliary field. This code should be
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1661  		 * conditionalized if any buggy AHCI controllers are
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1662  		 * encountered.
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1663  		 */
40fb59e75a drivers/ata/ahci.c  Marc Carino             2013-08-24  1664  		pi.flags |= ATA_FLAG_FPDMA_AUX;
453d3131ec drivers/ata/ahci.c  Robert Hancock          2010-01-26  1665  	}
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1666  
7d50b60b5e drivers/ata/ahci.c  Tejun Heo               2007-09-23  1667  	if (hpriv->cap & HOST_CAP_PMP)
7d50b60b5e drivers/ata/ahci.c  Tejun Heo               2007-09-23  1668  		pi.flags |= ATA_FLAG_PMP;
7d50b60b5e drivers/ata/ahci.c  Tejun Heo               2007-09-23  1669  
0cbb0e774b drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1670  	ahci_set_em_messages(hpriv, &pi);
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1671  
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1672  	if (ahci_broken_system_poweroff(pdev)) {
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1673  		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN;
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1674  		dev_info(&pdev->dev,
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1675  			"quirky BIOS, skipping spindown on poweroff\n");
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1676  	}
1fd684346d drivers/ata/ahci.c  Rafael J. Wysocki       2009-01-19  1677  
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1678  	if (ahci_broken_suspend(pdev)) {
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1679  		hpriv->flags |= AHCI_HFLAG_NO_SUSPEND;
a44fec1fce drivers/ata/ahci.c  Joe Perches             2011-04-15  1680  		dev_warn(&pdev->dev,
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1681  			 "BIOS update required for suspend/resume\n");
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1682  	}
9b10ae86d1 drivers/ata/ahci.c  Tejun Heo               2009-05-30  1683  
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1684  	if (ahci_broken_online(pdev)) {
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1685  		hpriv->flags |= AHCI_HFLAG_SRST_TOUT_IS_OFFLINE;
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1686  		dev_info(&pdev->dev,
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1687  			 "online status unreliable, applying workaround\n");
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1688  	}
5594639aab drivers/ata/ahci.c  Tejun Heo               2009-08-04  1689  
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1690  
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1691  	/* Acer SA5-271 workaround modifies private_data */
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1692  	acer_sa5_271_workaround(hpriv, pdev);
8bfd174312 drivers/ata/ahci.c  Sui Chen                2017-05-09  1693  
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1694  	/* CAP.NP sometimes indicate the index of the last enabled
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1695  	 * port, at other times, that of the last possible port, so
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1696  	 * determining the maximum port number requires looking at
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1697  	 * both CAP.NP and port_map.
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1698  	 */
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1699  	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1700  
837f5f8fb9 drivers/ata/ahci.c  Tejun Heo               2008-02-06  1701  	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1702  	if (!host)
24dc5f33ea drivers/ata/ahci.c  Tejun Heo               2007-01-20  1703  		return -ENOMEM;
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1704  	host->private_data = hpriv;
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1705  
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1706  	if (ahci_init_msi(pdev, n_ports, hpriv) < 0) {
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1707  		/* legacy intx interrupts */
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1708  		pci_intx(pdev, 1);
0b9e2988ab drivers/ata/ahci.c  Christoph Hellwig       2016-09-05  1709  	}
0ce57f8af1 drivers/ata/ahci.c  Christoph Hellwig       2016-10-25  1710  	hpriv->irq = pci_irq_vector(pdev, 0);
21bfd1aa95 drivers/ata/ahci.c  Robert Richter          2015-05-31  1711  
f3d7f23f87 drivers/ata/ahci.c  Arjan van de Ven        2009-01-26  1712  	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
886ad09fc8 drivers/ata/ahci.c  Arjan van de Ven        2009-01-09  1713  		host->flags |= ATA_HOST_PARALLEL_SCAN;
f3d7f23f87 drivers/ata/ahci.c  Arjan van de Ven        2009-01-26  1714  	else
d2782d96f3 drivers/ata/ahci.c  Jingoo Han              2013-10-05  1715  		dev_info(&pdev->dev, "SSS flag set, parallel bus scan disabled\n");
886ad09fc8 drivers/ata/ahci.c  Arjan van de Ven        2009-01-09  1716  
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1717  	if (pi.flags & ATA_FLAG_EM)
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1718  		ahci_reset_em(host);
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1719  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1720  	for (i = 0; i < host->n_ports; i++) {
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1721  		struct ata_port *ap = host->ports[i];
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1722  
318893e142 drivers/ata/ahci.c  Alessandro Rubini       2012-01-06  1723  		ata_port_pbar_desc(ap, ahci_pci_bar, -1, "abar");
318893e142 drivers/ata/ahci.c  Alessandro Rubini       2012-01-06  1724  		ata_port_pbar_desc(ap, ahci_pci_bar,
cbcdd87593 drivers/ata/ahci.c  Tejun Heo               2007-08-18  1725  				   0x100 + ap->port_no * 0x80, "port");
cbcdd87593 drivers/ata/ahci.c  Tejun Heo               2007-08-18  1726  
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1727  		/* set enclosure management message type */
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1728  		if (ap->flags & ATA_FLAG_EM)
008dbd61eb drivers/ata/ahci.c  Harry Zhang             2010-04-23  1729  			ap->em_message_type = hpriv->em_msg_type;
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1730  
18f7ba4c2f drivers/ata/ahci.c  Kristen Carlson Accardi 2008-06-03  1731  
dab632e8c4 drivers/ata/ahci.c  Jeff Garzik             2007-05-28  1732  		/* disabled/not-implemented port */
350756f6da drivers/ata/ahci.c  Tejun Heo               2008-04-07  1733  		if (!(hpriv->port_map & (1 << i)))
dab632e8c4 drivers/ata/ahci.c  Jeff Garzik             2007-05-28  1734  			ap->ops = &ata_dummy_port_ops;
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1735  	}
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1736  
edc9305284 drivers/ata/ahci.c  Tejun Heo               2007-10-25  1737  	/* apply workaround for ASUS P5W DH Deluxe mainboard */
edc9305284 drivers/ata/ahci.c  Tejun Heo               2007-10-25  1738  	ahci_p5wdh_workaround(host);
edc9305284 drivers/ata/ahci.c  Tejun Heo               2007-10-25  1739  
f80ae7e45a drivers/ata/ahci.c  Tejun Heo               2009-09-16  1740  	/* apply gtf filter quirk */
f80ae7e45a drivers/ata/ahci.c  Tejun Heo               2009-09-16  1741  	ahci_gtf_filter_workaround(host);
f80ae7e45a drivers/ata/ahci.c  Tejun Heo               2009-09-16  1742  
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1743  	/* initialize adapter */
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1744  	rc = ahci_configure_dma_masks(pdev, hpriv->cap & HOST_CAP_64);
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1745  	if (rc)
24dc5f33ea drivers/ata/ahci.c  Tejun Heo               2007-01-20  1746  		return rc;
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1747  
3303040d8b drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1748  	rc = ahci_pci_reset_controller(host);
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1749  	if (rc)
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1750  		return rc;
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1751  
781d655083 drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1752  	ahci_pci_init_controller(host);
439fcaec10 drivers/ata/ahci.c  Anton Vorontsov         2010-03-03  1753  	ahci_pci_print_info(host);
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1754  
4447d35156 drivers/ata/ahci.c  Tejun Heo               2007-04-17  1755  	pci_set_master(pdev);
5ca72c4f7c drivers/ata/ahci.c  Alexander Gordeev       2012-11-19  1756  
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1757  	rc = ahci_host_activate(host, &ahci_sht);
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1758  	if (rc)
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1759  		return rc;
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1760  
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18 @1761  	pm_runtime_put_noidle(&pdev->dev);
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1762  	return 0;
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1763  }
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1764  
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1765  static void ahci_remove_one(struct pci_dev *pdev)
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1766  {
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18 @1767  	pm_runtime_get_noresume(&pdev->dev);
02e53293ea drivers/ata/ahci.c  Mika Westerberg         2016-02-18  1768  	ata_pci_remove_one(pdev);
907f4678c1 drivers/scsi/ahci.c Jeff Garzik             2005-05-12  1769  }
^1da177e4c drivers/scsi/ahci.c Linus Torvalds          2005-04-16  1770  

:::::: The code at line 1761 was first introduced by commit
:::::: 02e53293eafcb19b4fabc8a2e7bbfa11b88d0e6c ahci: Add runtime PM support for the host controller

:::::: TO: Mika Westerberg <mika.westerberg@linux.intel.com>
:::::: CC: Tejun Heo <tj@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24293 bytes --]

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[kbuild test robot]

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

Hi Ulf,

I love your patch! Yet something to improve:

[auto build test ERROR on phy/next]
[also build test ERROR on v4.15-rc4 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ulf-Hansson/phy-core-Re-work-runtime-PM-deployment-and-fix-an-issue/20171223-170432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//ata/libahci.c: In function 'ahci_rpm_get_port':
>> drivers//ata/libahci.c:239:9: error: implicit declaration of function 'pm_runtime_get_sync'; did you mean 'ktime_get_ns'? [-Werror=implicit-function-declaration]
     return pm_runtime_get_sync(ap->dev);
            ^~~~~~~~~~~~~~~~~~~
            ktime_get_ns
   drivers//ata/libahci.c: In function 'ahci_rpm_put_port':
>> drivers//ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put'; did you mean 'of_node_put'? [-Werror=implicit-function-declaration]
     pm_runtime_put(ap->dev);
     ^~~~~~~~~~~~~~
     of_node_put
   cc1: some warnings being treated as errors

vim +251 drivers//ata/libahci.c

365cfa1e Anton Vorontsov 2010-03-28  228  
bb03c640 Mika Westerberg 2016-02-18  229  /**
bb03c640 Mika Westerberg 2016-02-18  230   *	ahci_rpm_get_port - Make sure the port is powered on
bb03c640 Mika Westerberg 2016-02-18  231   *	@ap: Port to power on
bb03c640 Mika Westerberg 2016-02-18  232   *
bb03c640 Mika Westerberg 2016-02-18  233   *	Whenever there is need to access the AHCI host registers outside of
bb03c640 Mika Westerberg 2016-02-18  234   *	normal execution paths, call this function to make sure the host is
bb03c640 Mika Westerberg 2016-02-18  235   *	actually powered on.
bb03c640 Mika Westerberg 2016-02-18  236   */
bb03c640 Mika Westerberg 2016-02-18  237  static int ahci_rpm_get_port(struct ata_port *ap)
bb03c640 Mika Westerberg 2016-02-18  238  {
bb03c640 Mika Westerberg 2016-02-18 @239  	return pm_runtime_get_sync(ap->dev);
bb03c640 Mika Westerberg 2016-02-18  240  }
bb03c640 Mika Westerberg 2016-02-18  241  
bb03c640 Mika Westerberg 2016-02-18  242  /**
bb03c640 Mika Westerberg 2016-02-18  243   *	ahci_rpm_put_port - Undoes ahci_rpm_get_port()
bb03c640 Mika Westerberg 2016-02-18  244   *	@ap: Port to power down
bb03c640 Mika Westerberg 2016-02-18  245   *
bb03c640 Mika Westerberg 2016-02-18  246   *	Undoes ahci_rpm_get_port() and possibly powers down the AHCI host
bb03c640 Mika Westerberg 2016-02-18  247   *	if it has no more active users.
bb03c640 Mika Westerberg 2016-02-18  248   */
bb03c640 Mika Westerberg 2016-02-18  249  static void ahci_rpm_put_port(struct ata_port *ap)
bb03c640 Mika Westerberg 2016-02-18  250  {
bb03c640 Mika Westerberg 2016-02-18 @251  	pm_runtime_put(ap->dev);
bb03c640 Mika Westerberg 2016-02-18  252  }
bb03c640 Mika Westerberg 2016-02-18  253  

:::::: The code at line 251 was first introduced by commit
:::::: bb03c640697155639b2e15e2aaa4c10f60bf0d5e ahci: Add functions to manage runtime PM of AHCI ports

:::::: TO: Mika Westerberg <mika.westerberg@linux.intel.com>
:::::: CC: Tejun Heo <tj@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30686 bytes --]

Re: [PATCH v2 2/3] phy: core: Drop unused runtime PM APIs

[kbuild test robot]

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

Hi Ulf,

I love your patch! Yet something to improve:

[auto build test ERROR on phy/next]
[also build test ERROR on v4.15-rc4 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ulf-Hansson/phy-core-Re-work-runtime-PM-deployment-and-fix-an-issue/20171223-170432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git next
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//ata/libahci.c: In function 'ahci_rpm_get_port':
>> drivers//ata/libahci.c:239:9: error: implicit declaration of function 'pm_runtime_get_sync'; did you mean 'ktime_get_ns'? [-Werror=implicit-function-declaration]
     return pm_runtime_get_sync(ap->dev);
            ^~~~~~~~~~~~~~~~~~~
            ktime_get_ns
   drivers//ata/libahci.c: In function 'ahci_rpm_put_port':
>> drivers//ata/libahci.c:251:2: error: implicit declaration of function 'pm_runtime_put'; did you mean 'of_node_put'? [-Werror=implicit-function-declaration]
     pm_runtime_put(ap->dev);
     ^~~~~~~~~~~~~~
     of_node_put
   cc1: some warnings being treated as errors

vim +251 drivers//ata/libahci.c

365cfa1e Anton Vorontsov 2010-03-28  228  
bb03c640 Mika Westerberg 2016-02-18  229  /**
bb03c640 Mika Westerberg 2016-02-18  230   *	ahci_rpm_get_port - Make sure the port is powered on
bb03c640 Mika Westerberg 2016-02-18  231   *	@ap: Port to power on
bb03c640 Mika Westerberg 2016-02-18  232   *
bb03c640 Mika Westerberg 2016-02-18  233   *	Whenever there is need to access the AHCI host registers outside of
bb03c640 Mika Westerberg 2016-02-18  234   *	normal execution paths, call this function to make sure the host is
bb03c640 Mika Westerberg 2016-02-18  235   *	actually powered on.
bb03c640 Mika Westerberg 2016-02-18  236   */
bb03c640 Mika Westerberg 2016-02-18  237  static int ahci_rpm_get_port(struct ata_port *ap)
bb03c640 Mika Westerberg 2016-02-18  238  {
bb03c640 Mika Westerberg 2016-02-18 @239  	return pm_runtime_get_sync(ap->dev);
bb03c640 Mika Westerberg 2016-02-18  240  }
bb03c640 Mika Westerberg 2016-02-18  241  
bb03c640 Mika Westerberg 2016-02-18  242  /**
bb03c640 Mika Westerberg 2016-02-18  243   *	ahci_rpm_put_port - Undoes ahci_rpm_get_port()
bb03c640 Mika Westerberg 2016-02-18  244   *	@ap: Port to power down
bb03c640 Mika Westerberg 2016-02-18  245   *
bb03c640 Mika Westerberg 2016-02-18  246   *	Undoes ahci_rpm_get_port() and possibly powers down the AHCI host
bb03c640 Mika Westerberg 2016-02-18  247   *	if it has no more active users.
bb03c640 Mika Westerberg 2016-02-18  248   */
bb03c640 Mika Westerberg 2016-02-18  249  static void ahci_rpm_put_port(struct ata_port *ap)
bb03c640 Mika Westerberg 2016-02-18  250  {
bb03c640 Mika Westerberg 2016-02-18 @251  	pm_runtime_put(ap->dev);
bb03c640 Mika Westerberg 2016-02-18  252  }
bb03c640 Mika Westerberg 2016-02-18  253  

:::::: The code at line 251 was first introduced by commit
:::::: bb03c640697155639b2e15e2aaa4c10f60bf0d5e ahci: Add functions to manage runtime PM of AHCI ports

:::::: TO: Mika Westerberg <mika.westerberg@linux.intel.com>
:::::: CC: Tejun Heo <tj@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30686 bytes --]

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 23 December 2017 at 02:35, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> The runtime PM deployment in the phy core is deployed using the phy core
>>>> device, which is created by the phy core and assigned as a child device of
>>>> the phy provider device.
>>>>
>>>> The behaviour around the runtime PM deployment cause some issues during
>>>> system suspend, in cases when the phy provider device is put into a low
>>>> power state via a call to the pm_runtime_force_suspend() helper, as is the
>>>> case for a Renesas SoC, which has its phy provider device attached to the
>>>> generic PM domain.
>>>>
>>>> In more detail, the problem in this case is that pm_runtime_force_suspend()
>>>> expects the child device of the provider device, which is the phy core
>>>> device, to be runtime suspended, else a WARN splat will be printed
>>>> (correctly) when runtime PM gets re-enabled at system resume.
>>>
>>> So we are now trying to work around issues with
>>> pm_runtime_force_suspend().  Lovely. :-/
>>
>> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or
>> are you saying we should just ignore all issues related to it?
>
> Or get rid of it?
>
> Seriously, is the resulting pain worth it?

I am not sure what pain you refer to? :-) So far I haven't got much
errors being reported because of its use, have you?

Moreover, to me, this small series fixes the problems in a rather trivial way.

>
>> Of course, if we had something that could replace
>> pm_runtime_force_suspend(), that would be great, but there isn't.
>
> I beg to differ.
>
> At least some of it could be replaced with the driver flags.

Yes, true.

On the other hand that is exactly the problem, only *some*. And more
importantly, genpd can't use them, because it can't cope with have
system wide PM callbacks to be skipped.

In other words, so far, the driver PM flags can't solve this issue.

>
>>>> In the current scenario, even if a call to phy_power_off() triggers it to
>>>> invoke pm_runtime_put() during system suspend, the phy core device doesn't
>>>> get runtime suspended, because this is prevented in the system suspend
>>>> phases by the PM core.
>>>>
>>>> To solve this problem, let's move the runtime PM deployment from the phy
>>>> core device to the phy provider device, as this provides the similar
>>>> behaviour. Changing this makes it redundant to enable runtime PM for the
>>>> phy core device, so let's avoid doing that.
>>>
>>> I'm not really convinced that this approach is the best one to be honest.
>>>
>>> I'll have a deeper look at this in the next few days, stay tuned.
>>
>> There is different ways to solve this, for sure. I picked this one,
>> because I think it's the most trivial thing to do, and it shouldn't
>> cause any other problems.
>>
>> I think any other option would involve assigning ->suspend|resume()
>> callbacks to the phy core device, but that's fine too, if you prefer
>> that.
>>
>> Also, I have considered how to deal with wakeup paths for phys,
>> although I didn't want to post changes as a part of this series, but
>> maybe I should to give a more complete picture?
>
> Yes, you should.

Okay!

>
> The point is that without genpd using pm_runtime_force_suspend() the
> phy code could very well stay the way it is.  And it is logical,
> because having a parent with enabled runtime PM without enabling
> runtime PM for its children is at least conceptually questionable.

I agree that the phy core is today logical okay. But I think that's
also the case, if we pick up this small series.

There are many reasons and cases where a children is runtime PM
disabled, while the parent is runtime PM enabled. Moreover the runtime
PM core copes fine with that.

>
> So the conclusion may be that the phy code is OK, but calling
> pm_runtime_force_suspend() from genpd isn't.

So I am worried that changing genpd in this regards, may introduce
other regressions. @subject series seems far less risky - and again,
to me, the changes are trivial.

Anyway, I will keep your suggestion in mind and think about, how/if
genpd can be changed.

Kind regards
Uffe

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Thu, Dec 21, 2017 at 2:39 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> The runtime PM deployment in the phy core is deployed using the phy core
>> device, which is created by the phy core and assigned as a child device of
>> the phy provider device.
>>
>> The behaviour around the runtime PM deployment cause some issues during
>> system suspend, in cases when the phy provider device is put into a low
>> power state via a call to the pm_runtime_force_suspend() helper, as is the
>> case for a Renesas SoC, which has its phy provider device attached to the
>> generic PM domain.
>>
>> In more detail, the problem in this case is that pm_runtime_force_suspend()
>> expects the child device of the provider device, which is the phy core
>> device, to be runtime suspended, else a WARN splat will be printed
>> (correctly) when runtime PM gets re-enabled at system resume.
>
> So we are now trying to work around issues with
> pm_runtime_force_suspend().  Lovely. :-/
>
>> In the current scenario, even if a call to phy_power_off() triggers it to
>> invoke pm_runtime_put() during system suspend, the phy core device doesn't
>> get runtime suspended, because this is prevented in the system suspend
>> phases by the PM core.
>>
>> To solve this problem, let's move the runtime PM deployment from the phy
>> core device to the phy provider device, as this provides the similar
>> behaviour. Changing this makes it redundant to enable runtime PM for the
>> phy core device, so let's avoid doing that.
>
> I'm not really convinced that this approach is the best one to be honest.
>
> I'll have a deeper look at this in the next few days, stay tuned.

So IMO the changes you are proposing make sense regardless of the
genpd issue, because they generally simplify the phy code, but the
additional use_runtime_pm field in struct phy represents redundant
information (manipulating reference counters shouldn't matter if
runtime PM is disabled), so it doesn't appear to be necessary.

[On a related note, I'm not sure why phy tries to intercept runtime PM
errors and "fix up" the reference counters.  That doesn't look right
to me at all.]

That said, the current phy code is not strictly invalid.  While it
looks more complicated than necessary, it doesn't do things documented
as invalid in principle, so saying "The behaviour around the runtime
PM deployment cause some issues during system suspend" in the
changelog is describing the problem from a very specific angle.
Simply put, pm_runtime_force_suspend() and the current phy code cannot
work together and so using them together is a bug.  None of them
individually is at fault, but combining them is incorrect.

Fortunately enough, the phy code can be modified so that it can be
used with pm_runtime_force_suspend() without problems, but picturing
it as "problematic", because it cannot do that today is not entirely
fair IMO.

Thanks,
Rafael

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 1:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 December 2017 at 02:35, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Thu, Dec 21, 2017 at 11:50 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 21 December 2017 at 02:39, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> On Wed, Dec 20, 2017 at 3:09 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> The runtime PM deployment in the phy core is deployed using the phy core
>>>>> device, which is created by the phy core and assigned as a child device of
>>>>> the phy provider device.
>>>>>
>>>>> The behaviour around the runtime PM deployment cause some issues during
>>>>> system suspend, in cases when the phy provider device is put into a low
>>>>> power state via a call to the pm_runtime_force_suspend() helper, as is the
>>>>> case for a Renesas SoC, which has its phy provider device attached to the
>>>>> generic PM domain.
>>>>>
>>>>> In more detail, the problem in this case is that pm_runtime_force_suspend()
>>>>> expects the child device of the provider device, which is the phy core
>>>>> device, to be runtime suspended, else a WARN splat will be printed
>>>>> (correctly) when runtime PM gets re-enabled at system resume.
>>>>
>>>> So we are now trying to work around issues with
>>>> pm_runtime_force_suspend().  Lovely. :-/
>>>
>>> Yes, we have to, as pm_runtime_force_suspend() is widely deployed. Or
>>> are you saying we should just ignore all issues related to it?
>>
>> Or get rid of it?
>>
>> Seriously, is the resulting pain worth it?
>
> I am not sure what pain you refer to? :-) So far I haven't got much
> errors being reported because of its use, have you?
>
> Moreover, to me, this small series fixes the problems in a rather trivial way.

Well, I agree here (please see the message regarding that I've just
sent in this thread).

>>
>>> Of course, if we had something that could replace
>>> pm_runtime_force_suspend(), that would be great, but there isn't.
>>
>> I beg to differ.
>>
>> At least some of it could be replaced with the driver flags.
>
> Yes, true.
>
> On the other hand that is exactly the problem, only *some*. And more
> importantly, genpd can't use them, because it can't cope with have
> system wide PM callbacks to be skipped.

Its own system-wide PM callbacks cannot be skipped, but it already
skips driver callbacks in some cases. :-)

> In other words, so far, the driver PM flags can't solve this issue.

It is unclear which issue you are talking about, but anyway, yes, they
aren't equivalent to pm_runtime_force_suspend/resume() which is quite
intentional, honestly ...

>>>>> In the current scenario, even if a call to phy_power_off() triggers it to
>>>>> invoke pm_runtime_put() during system suspend, the phy core device doesn't
>>>>> get runtime suspended, because this is prevented in the system suspend
>>>>> phases by the PM core.
>>>>>
>>>>> To solve this problem, let's move the runtime PM deployment from the phy
>>>>> core device to the phy provider device, as this provides the similar
>>>>> behaviour. Changing this makes it redundant to enable runtime PM for the
>>>>> phy core device, so let's avoid doing that.
>>>>
>>>> I'm not really convinced that this approach is the best one to be honest.
>>>>
>>>> I'll have a deeper look at this in the next few days, stay tuned.
>>>
>>> There is different ways to solve this, for sure. I picked this one,
>>> because I think it's the most trivial thing to do, and it shouldn't
>>> cause any other problems.
>>>
>>> I think any other option would involve assigning ->suspend|resume()
>>> callbacks to the phy core device, but that's fine too, if you prefer
>>> that.
>>>
>>> Also, I have considered how to deal with wakeup paths for phys,
>>> although I didn't want to post changes as a part of this series, but
>>> maybe I should to give a more complete picture?
>>
>> Yes, you should.
>
> Okay!
>
>>
>> The point is that without genpd using pm_runtime_force_suspend() the
>> phy code could very well stay the way it is.  And it is logical,
>> because having a parent with enabled runtime PM without enabling
>> runtime PM for its children is at least conceptually questionable.
>
> I agree that the phy core is today logical okay. But I think that's
> also the case, if we pick up this small series.
>
> There are many reasons and cases where a children is runtime PM
> disabled, while the parent is runtime PM enabled. Moreover the runtime
> PM core copes fine with that.

Fair enough.

>>
>> So the conclusion may be that the phy code is OK, but calling
>> pm_runtime_force_suspend() from genpd isn't.
>
> So I am worried that changing genpd in this regards, may introduce
> other regressions. @subject series seems far less risky - and again,
> to me, the changes are trivial.
>
> Anyway, I will keep your suggestion in mind and think about, how/if
> genpd can be changed.

Thanks a lot!

Rafael

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

[...]

>
> So IMO the changes you are proposing make sense regardless of the
> genpd issue, because they generally simplify the phy code, but the
> additional use_runtime_pm field in struct phy represents redundant
> information (manipulating reference counters shouldn't matter if
> runtime PM is disabled), so it doesn't appear to be necessary.
>

Actually, the first version I posted treated the return codes from
pm_runtime_get_sync() according to your suggestion above.

However, Kishon pointed out that it didn't work. That's because, there
are phy provider drivers that enables runtime PM *after* calling
phy_create(). And in those cases, that is because they want to treat
runtime PM themselves.

I think that's probably something we should look into to change, but I
find it being a separate issue, that I didn't want to investigate as
part of this series.

See more about the thread here:
https://www.spinics.net/lists/linux-renesas-soc/msg21711.html

> [On a related note, I'm not sure why phy tries to intercept runtime PM
> errors and "fix up" the reference counters.  That doesn't look right
> to me at all.]
>
> That said, the current phy code is not strictly invalid.  While it
> looks more complicated than necessary, it doesn't do things documented
> as invalid in principle, so saying "The behaviour around the runtime
> PM deployment cause some issues during system suspend" in the
> changelog is describing the problem from a very specific angle.
> Simply put, pm_runtime_force_suspend() and the current phy code cannot
> work together and so using them together is a bug.  None of them
> individually is at fault, but combining them is incorrect.
>
> Fortunately enough, the phy code can be modified so that it can be
> used with pm_runtime_force_suspend() without problems, but picturing
> it as "problematic", because it cannot do that today is not entirely
> fair IMO.

Right, this makes sense. Let me clarify this in the changelog.

Kind regards
Uffe

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Rafael J. Wysocki]

On Saturday, December 23, 2017 4:09:33 PM CET Ulf Hansson wrote:
> [...]
> 
> >
> > So IMO the changes you are proposing make sense regardless of the
> > genpd issue, because they generally simplify the phy code, but the
> > additional use_runtime_pm field in struct phy represents redundant
> > information (manipulating reference counters shouldn't matter if
> > runtime PM is disabled), so it doesn't appear to be necessary.
> >
> 
> Actually, the first version I posted treated the return codes from
> pm_runtime_get_sync() according to your suggestion above.
> 
> However, Kishon pointed out that it didn't work. That's because, there
> are phy provider drivers that enables runtime PM *after* calling
> phy_create(). And in those cases, that is because they want to treat
> runtime PM themselves.
> 
> I think that's probably something we should look into to change, but I
> find it being a separate issue, that I didn't want to investigate as
> part of this series.
> 
> See more about the thread here:
> https://www.spinics.net/lists/linux-renesas-soc/msg21711.html

Even so, it shouldn't matter when the provider enables runtime PM.

Calling pm_runtime_get_*()/pm_runtime_put_*() should always work as
long as they are balanced properly regardless of whether or not
runtime PM is enabled for the device or it is enabled in the meantime.
Of course, the initial runtime PM status of the device must reflect
the values of the usage counters, but that should not be too hard to
ensure.

The reason why it matters here is because the phy core tries to be smart
about manipulating reference counters by itself and that's a mistake.

So it looks to me like the whole thing needs to be reworked and yes,
that should be done in the first place IMO, because it will make the
issue with genpd go away automatically.

[Why is phy_pm_runtime_get() there at all, for instance?  It seems
to have no users and I kind of don't see use cases for it.  Also
phy_pm_runtime_get_sync() is only used by the phy core in three
places - shouldn't be too hard to fix that.]

So the issue with genpd really seems to be a messenger here. :-)

Thanks,
Rafael

Re: [PATCH v2 1/3] phy: core: Move runtime PM reference counting to the parent device

[Ulf Hansson]

On 24 December 2017 at 13:00, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Saturday, December 23, 2017 4:09:33 PM CET Ulf Hansson wrote:
>> [...]
>>
>> >
>> > So IMO the changes you are proposing make sense regardless of the
>> > genpd issue, because they generally simplify the phy code, but the
>> > additional use_runtime_pm field in struct phy represents redundant
>> > information (manipulating reference counters shouldn't matter if
>> > runtime PM is disabled), so it doesn't appear to be necessary.
>> >
>>
>> Actually, the first version I posted treated the return codes from
>> pm_runtime_get_sync() according to your suggestion above.
>>
>> However, Kishon pointed out that it didn't work. That's because, there
>> are phy provider drivers that enables runtime PM *after* calling
>> phy_create(). And in those cases, that is because they want to treat
>> runtime PM themselves.
>>
>> I think that's probably something we should look into to change, but I
>> find it being a separate issue, that I didn't want to investigate as
>> part of this series.
>>
>> See more about the thread here:
>> https://www.spinics.net/lists/linux-renesas-soc/msg21711.html
>
> Even so, it shouldn't matter when the provider enables runtime PM.
>
> Calling pm_runtime_get_*()/pm_runtime_put_*() should always work as
> long as they are balanced properly regardless of whether or not
> runtime PM is enabled for the device or it is enabled in the meantime.
> Of course, the initial runtime PM status of the device must reflect
> the values of the usage counters, but that should not be too hard to
> ensure.

Yes, this is probably the main reason.

However, as stated, I think we should look into addressing this
problem more carefully, in a separate next step.

>
> The reason why it matters here is because the phy core tries to be smart
> about manipulating reference counters by itself and that's a mistake.
>
> So it looks to me like the whole thing needs to be reworked and yes,
> that should be done in the first place IMO, because it will make the
> issue with genpd go away automatically.

Sorry, but I am not fully understanding what you suggest here. Perhaps
you didn't check patch2?

>
> [Why is phy_pm_runtime_get() there at all, for instance?  It seems
> to have no users and I kind of don't see use cases for it.  Also
> phy_pm_runtime_get_sync() is only used by the phy core in three
> places - shouldn't be too hard to fix that.]

Removing these APIs and functions is done in patch2, so I guess I am
already doing what you suggests above? No?

[...]

Kind regards
Uffe