[PATCH v4 0/3] PM / clock_ops: add pm_clk_add_clk()

[PATCH v4 0/3] PM / clock_ops: add pm_clk_add_clk()

[Grygorii Strashko]

Hi Santosh, Kevin,

I've separated these patches in standalone series as requested by
Santosh (https://lkml.org/lkml/2014/10/24/591). Also, I've kept
versioning of patches from original series and therefor this
is v4 of patches.

The patch 1 was originally introduced in [1] (Apr 2014) and it allows
to fill per-device list of clock from DT.

The patch 2 is small clean up needed for patch 3.

The patch 3 was created as was discussed in v2 and first introduced in v3.
Here I marked it as RFC, because I've found some disadvantages of such
approach - see comments to the patch itself.

Finally, this series is needed to enable to enable Runtime
PM for Keystone 2, but honestly I need only patch 1.

RFC version of patches can be found at [2].

Changes in v4:
 - pm_clk_add_clk() changed to get reference on clock as
   requested by Dmitry Torokhov (see v3). Now the caller has to
   use clk_put() on clock pointer when done.

Changes in v3:
 - handling of the case when !CONFIG_PM_RUNTIME has been
   moved in PM clock framework.

Changes in v2:
- minor comments applied and rebased on top of Linux 3.18-rc1.

Links on previous versions:
v3:  https://lkml.org/lkml/2014/10/23/342
v2:  https://lkml.org/lkml/2014/10/20/248
v1:  https://lkml.org/lkml/2014/9/29/382

[1] "[PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core"
  https://lkml.org/lkml/2014/4/24/1118

[2] "[RFC PATCH 0/4] ARM: keystone: pm: switch to use generic pm domains"
  https://lkml.org/lkml/2014/9/25/364

CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Kevin Hilman <khilman@linaro.org>
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Geert Uytterhoeven (1):
  PM / clock_ops: Add pm_clk_add_clk()

Grygorii Strashko (2):
  PM / clock_ops: make __pm_clk_enable more generic
  PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME

 drivers/base/power/clock_ops.c | 89 +++++++++++++++++++++++++++---------------
 include/linux/pm_clock.h       |  8 ++++
 2 files changed, 65 insertions(+), 32 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Grygorii Strashko]

From: Geert Uytterhoeven <geert+renesas@glider.be>

The existing pm_clk_add() allows to pass a clock by con_id. However,
when referring to a specific clock from DT, no con_id is available.

Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
The will will increment refcount on clock pointer, so the caller has
to use clk_put() on clock pointer when done.

CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Reviewed-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/base/power/clock_ops.c | 47 +++++++++++++++++++++++++++++++++---------
 include/linux/pm_clock.h       |  8 +++++++
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 7836930..f061eaa 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -12,6 +12,7 @@
 #include <linux/pm.h>
 #include <linux/pm_clock.h>
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 #include <linux/slab.h>
 #include <linux/err.h>
 
@@ -53,7 +54,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
  */
 static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 {
-	ce->clk = clk_get(dev, ce->con_id);
+	if (!ce->clk)
+		ce->clk = clk_get(dev, ce->con_id);
 	if (IS_ERR(ce->clk)) {
 		ce->status = PCE_STATUS_ERROR;
 	} else {
@@ -63,15 +65,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 	}
 }
 
-/**
- * pm_clk_add - Start using a device clock for power management.
- * @dev: Device whose clock is going to be used for power management.
- * @con_id: Connection ID of the clock.
- *
- * Add the clock represented by @con_id to the list of clocks used for
- * the power management of @dev.
- */
-int pm_clk_add(struct device *dev, const char *con_id)
+static int __pm_clk_add(struct device *dev, const char *con_id,
+			struct clk *clk)
 {
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
@@ -93,6 +88,12 @@ int pm_clk_add(struct device *dev, const char *con_id)
 			kfree(ce);
 			return -ENOMEM;
 		}
+	} else {
+		if (IS_ERR(ce->clk) || !__clk_get(clk)) {
+			kfree(ce);
+			return -ENOENT;
+		}
+		ce->clk = clk;
 	}
 
 	pm_clk_acquire(dev, ce);
@@ -104,6 +105,32 @@ int pm_clk_add(struct device *dev, const char *con_id)
 }
 
 /**
+ * pm_clk_add - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @con_id: Connection ID of the clock.
+ *
+ * Add the clock represented by @con_id to the list of clocks used for
+ * the power management of @dev.
+ */
+int pm_clk_add(struct device *dev, const char *con_id)
+{
+	return __pm_clk_add(dev, con_id, NULL);
+}
+
+/**
+ * pm_clk_add_clk - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @clk: Clock pointer
+ *
+ * Add the clock to the list of clocks used for the power management of @dev.
+ * It will increment refcount on clock pointer, use clk_put() on it when done.
+ */
+int pm_clk_add_clk(struct device *dev, struct clk *clk)
+{
+	return __pm_clk_add(dev, NULL, clk);
+}
+
+/**
  * __pm_clk_remove - Destroy PM clock entry.
  * @ce: PM clock entry to destroy.
  */
diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
index 8348866..0b00396 100644
--- a/include/linux/pm_clock.h
+++ b/include/linux/pm_clock.h
@@ -18,6 +18,8 @@ struct pm_clk_notifier_block {
 	char *con_ids[];
 };
 
+struct clk;
+
 #ifdef CONFIG_PM_CLK
 static inline bool pm_clk_no_clocks(struct device *dev)
 {
@@ -29,6 +31,7 @@ extern void pm_clk_init(struct device *dev);
 extern int pm_clk_create(struct device *dev);
 extern void pm_clk_destroy(struct device *dev);
 extern int pm_clk_add(struct device *dev, const char *con_id);
+extern int pm_clk_add_clk(struct device *dev, struct clk *clk);
 extern void pm_clk_remove(struct device *dev, const char *con_id);
 extern int pm_clk_suspend(struct device *dev);
 extern int pm_clk_resume(struct device *dev);
@@ -51,6 +54,11 @@ static inline int pm_clk_add(struct device *dev, const char *con_id)
 {
 	return -EINVAL;
 }
+
+static inline int pm_clk_add_clk(struct device *dev, struct clk *clk)
+{
+	return -EINVAL;
+}
 static inline void pm_clk_remove(struct device *dev, const char *con_id)
 {
 }
-- 
1.9.1

[PATCH v4 2/3] PM / clock_ops: make __pm_clk_enable more generic

[Grygorii Strashko]

Now there are two places in code which do the same things,
so allow __pm_clk_enable() to accept pointer on pm_clock_entry
structure as second parameter instead of pointer on clock and
remove duplicated code.

Also, updated function intended to be used by following patch.

CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Kevin Hilman <khilman@linaro.org>
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/base/power/clock_ops.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index f061eaa..b32b5d4 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -35,14 +35,20 @@ struct pm_clock_entry {
 /**
  * pm_clk_enable - Enable a clock, reporting any errors
  * @dev: The device for the given clock
- * @clk: The clock being enabled.
+ * @ce: PM clock entry corresponding to the clock.
  */
-static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
+static inline int __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce)
 {
-	int ret = clk_enable(clk);
-	if (ret)
-		dev_err(dev, "%s: failed to enable clk %p, error %d\n",
-			__func__, clk, ret);
+	int ret;
+
+	if (ce->status < PCE_STATUS_ERROR) {
+		ret = clk_enable(ce->clk);
+		if (!ret)
+			ce->status = PCE_STATUS_ENABLED;
+		else
+			dev_err(dev, "%s: failed to enable clk %p, error %d\n",
+				__func__, ce->clk, ret);
+	}
 
 	return ret;
 }
@@ -293,7 +299,6 @@ int pm_clk_resume(struct device *dev)
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
 	unsigned long flags;
-	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -302,13 +307,8 @@ int pm_clk_resume(struct device *dev)
 
 	spin_lock_irqsave(&psd->lock, flags);
 
-	list_for_each_entry(ce, &psd->clock_list, node) {
-		if (ce->status < PCE_STATUS_ERROR) {
-			ret = __pm_clk_enable(dev, ce->clk);
-			if (!ret)
-				ce->status = PCE_STATUS_ENABLED;
-		}
-	}
+	list_for_each_entry(ce, &psd->clock_list, node)
+		__pm_clk_enable(dev, ce);
 
 	spin_unlock_irqrestore(&psd->lock, flags);
 
@@ -417,7 +417,6 @@ int pm_clk_resume(struct device *dev)
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
 	unsigned long flags;
-	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -427,13 +426,8 @@ int pm_clk_resume(struct device *dev)
 
 	spin_lock_irqsave(&psd->lock, flags);
 
-	list_for_each_entry(ce, &psd->clock_list, node) {
-		if (ce->status < PCE_STATUS_ERROR) {
-			ret = __pm_clk_enable(dev, ce->clk);
-			if (!ret)
-				ce->status = PCE_STATUS_ENABLED;
-		}
-	}
+	list_for_each_entry(ce, &psd->clock_list, node)
+		__pm_clk_enable(dev, ce);
 
 	spin_unlock_irqrestore(&psd->lock, flags);
 
-- 
1.9.1

[RFC PATCH v4 3/3] PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME

[Grygorii Strashko]

Device's clocks need to be enabled always at probing time
if !CONFIG_PM_RUNTIME - in that way, device will become accesible
and, later, its clocks can be disabled/enabled during system
suspend/resume by using pm_clk_suspend/pm_clk_resume APIs.

But now, the clocks management code doesn't enable clocks when
clocks are being registered per-device by using pm_clk_add/_clk().
So, update pm_clk_acquire(), which is called from pm_clk_add/_clk(),
to enable clock always if !CONFIG_PM_RUNTIME.

Also, Platform PM domains drivers will not have to add code like below
each time they need to handle the case when !CONFIG_PM_RUNTIME [1 - 3]:

       if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
               ret = pm_clk_resume(dev);
               if (ret) {
                       dev_err(dev, "pm_clk_resume failed %d\n", ret);
                       goto clk_err;
               };
       }

[1] https://lkml.org/lkml/2014/10/3/306
[2] https://lkml.org/lkml/2014/10/16/305
[3] https://lkml.org/lkml/2014/10/20/249

CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Kevin Hilman <khilman@linaro.org>
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Geert Uytterhoeven <geert@linux-m68k.org>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Philipp Zabel <p.zabel@pengutronix.de>
CC: Caesar Wang <caesar.wang@rock-chips.com>

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
Hi,

I marked it as RFC, because:
  Generic clock manipulation PM callbacks can be used for any device
  for which list of clocks need to be maintained - and it doesn't mean
  that this framework need to be always connected to GPD/Runtime PM or
  used during suspend resume. Also, for DT-use case - it doesn't perform
  any actions (except clk_prepare) on clocks by itself and only
  by request from framework's consumer.

 For example, it can be reused by Power domain/controller drivers like:
 - Rockchip RK3288 PM Domain (https://lkml.org/lkml/2014/11/6/30)
 - IMX.6 PU power domain (http://www.spinics.net/lists/arm-kernel/msg360709.html)

 to maintain list of clocks for GPD itself and these clocks should not
 be enabled by default, because they are used to power on/off Power domain
 in a specific way:
     - enable all clocks assigned to PD
     - power on/off PD
     - disable all clocks assigned to PD.
 After this patch it will be impossible to reuse Generic clock manipulation PM
 callbacks in cases described above.

 drivers/base/power/clock_ops.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index b32b5d4..36cdff7 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -69,6 +69,10 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 		ce->status = PCE_STATUS_ACQUIRED;
 		dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
 	}
+
+	if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
+		__pm_clk_enable(dev, ce);
+	}
 }
 
 static int __pm_clk_add(struct device *dev, const char *con_id,
-- 
1.9.1

Re: [PATCH v4 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Dmitry Torokhov]

On Thu, Nov 06, 2014 at 03:51:00PM +0200, Grygorii Strashko wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> The existing pm_clk_add() allows to pass a clock by con_id. However,
> when referring to a specific clock from DT, no con_id is available.
> 
> Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
> The will will increment refcount on clock pointer, so the caller has
> to use clk_put() on clock pointer when done.
> 
> CC: Ulf Hansson <ulf.hansson@linaro.org>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/base/power/clock_ops.c | 47 +++++++++++++++++++++++++++++++++---------
>  include/linux/pm_clock.h       |  8 +++++++
>  2 files changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index 7836930..f061eaa 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -12,6 +12,7 @@
>  #include <linux/pm.h>
>  #include <linux/pm_clock.h>
>  #include <linux/clk.h>
> +#include <linux/clkdev.h>
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  
> @@ -53,7 +54,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
>   */
>  static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>  {
> -	ce->clk = clk_get(dev, ce->con_id);
> +	if (!ce->clk)
> +		ce->clk = clk_get(dev, ce->con_id);
>  	if (IS_ERR(ce->clk)) {
>  		ce->status = PCE_STATUS_ERROR;
>  	} else {
> @@ -63,15 +65,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>  	}
>  }
>  
> -/**
> - * pm_clk_add - Start using a device clock for power management.
> - * @dev: Device whose clock is going to be used for power management.
> - * @con_id: Connection ID of the clock.
> - *
> - * Add the clock represented by @con_id to the list of clocks used for
> - * the power management of @dev.
> - */
> -int pm_clk_add(struct device *dev, const char *con_id)
> +static int __pm_clk_add(struct device *dev, const char *con_id,
> +			struct clk *clk)
>  {
>  	struct pm_subsys_data *psd = dev_to_psd(dev);
>  	struct pm_clock_entry *ce;
> @@ -93,6 +88,12 @@ int pm_clk_add(struct device *dev, const char *con_id)
>  			kfree(ce);
>  			return -ENOMEM;
>  		}
> +	} else {
> +		if (IS_ERR(ce->clk) || !__clk_get(clk)) {
> +			kfree(ce);
> +			return -ENOENT;
> +		}
> +		ce->clk = clk;
>  	}
>  
>  	pm_clk_acquire(dev, ce);
> @@ -104,6 +105,32 @@ int pm_clk_add(struct device *dev, const char *con_id)
>  }
>  
>  /**
> + * pm_clk_add - Start using a device clock for power management.
> + * @dev: Device whose clock is going to be used for power management.
> + * @con_id: Connection ID of the clock.
> + *
> + * Add the clock represented by @con_id to the list of clocks used for
> + * the power management of @dev.
> + */
> +int pm_clk_add(struct device *dev, const char *con_id)
> +{
> +	return __pm_clk_add(dev, con_id, NULL);
> +}
> +
> +/**
> + * pm_clk_add_clk - Start using a device clock for power management.
> + * @dev: Device whose clock is going to be used for power management.
> + * @clk: Clock pointer
> + *
> + * Add the clock to the list of clocks used for the power management of @dev.
> + * It will increment refcount on clock pointer, use clk_put() on it when done.
> + */
> +int pm_clk_add_clk(struct device *dev, struct clk *clk)
> +{
> +	return __pm_clk_add(dev, NULL, clk);
> +}
> +
> +/**
>   * __pm_clk_remove - Destroy PM clock entry.
>   * @ce: PM clock entry to destroy.
>   */
> diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
> index 8348866..0b00396 100644
> --- a/include/linux/pm_clock.h
> +++ b/include/linux/pm_clock.h
> @@ -18,6 +18,8 @@ struct pm_clk_notifier_block {
>  	char *con_ids[];
>  };
>  
> +struct clk;
> +
>  #ifdef CONFIG_PM_CLK
>  static inline bool pm_clk_no_clocks(struct device *dev)
>  {
> @@ -29,6 +31,7 @@ extern void pm_clk_init(struct device *dev);
>  extern int pm_clk_create(struct device *dev);
>  extern void pm_clk_destroy(struct device *dev);
>  extern int pm_clk_add(struct device *dev, const char *con_id);
> +extern int pm_clk_add_clk(struct device *dev, struct clk *clk);
>  extern void pm_clk_remove(struct device *dev, const char *con_id);
>  extern int pm_clk_suspend(struct device *dev);
>  extern int pm_clk_resume(struct device *dev);
> @@ -51,6 +54,11 @@ static inline int pm_clk_add(struct device *dev, const char *con_id)
>  {
>  	return -EINVAL;
>  }
> +
> +static inline int pm_clk_add_clk(struct device *dev, struct clk *clk)
> +{
> +	return -EINVAL;
> +}
>  static inline void pm_clk_remove(struct device *dev, const char *con_id)
>  {
>  }
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH v4 2/3] PM / clock_ops: make __pm_clk_enable more generic

[Dmitry Torokhov]

On Thu, Nov 06, 2014 at 03:51:01PM +0200, Grygorii Strashko wrote:
> Now there are two places in code which do the same things,
> so allow __pm_clk_enable() to accept pointer on pm_clock_entry
> structure as second parameter instead of pointer on clock and
> remove duplicated code.
> 
> Also, updated function intended to be used by following patch.
> 
> CC: Santosh Shilimkar <ssantosh@kernel.org>
> CC: Kevin Hilman <khilman@linaro.org>
> CC: Ulf Hansson <ulf.hansson@linaro.org>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/base/power/clock_ops.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index f061eaa..b32b5d4 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -35,14 +35,20 @@ struct pm_clock_entry {
>  /**
>   * pm_clk_enable - Enable a clock, reporting any errors
>   * @dev: The device for the given clock
> - * @clk: The clock being enabled.
> + * @ce: PM clock entry corresponding to the clock.
>   */
> -static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
> +static inline int __pm_clk_enable(struct device *dev, struct pm_clock_entry *ce)
>  {
> -	int ret = clk_enable(clk);
> -	if (ret)
> -		dev_err(dev, "%s: failed to enable clk %p, error %d\n",
> -			__func__, clk, ret);
> +	int ret;
> +
> +	if (ce->status < PCE_STATUS_ERROR) {
> +		ret = clk_enable(ce->clk);
> +		if (!ret)
> +			ce->status = PCE_STATUS_ENABLED;
> +		else
> +			dev_err(dev, "%s: failed to enable clk %p, error %d\n",
> +				__func__, ce->clk, ret);
> +	}
>  
>  	return ret;
>  }
> @@ -293,7 +299,6 @@ int pm_clk_resume(struct device *dev)
>  	struct pm_subsys_data *psd = dev_to_psd(dev);
>  	struct pm_clock_entry *ce;
>  	unsigned long flags;
> -	int ret;
>  
>  	dev_dbg(dev, "%s()\n", __func__);
>  
> @@ -302,13 +307,8 @@ int pm_clk_resume(struct device *dev)
>  
>  	spin_lock_irqsave(&psd->lock, flags);
>  
> -	list_for_each_entry(ce, &psd->clock_list, node) {
> -		if (ce->status < PCE_STATUS_ERROR) {
> -			ret = __pm_clk_enable(dev, ce->clk);
> -			if (!ret)
> -				ce->status = PCE_STATUS_ENABLED;
> -		}
> -	}
> +	list_for_each_entry(ce, &psd->clock_list, node)
> +		__pm_clk_enable(dev, ce);
>  
>  	spin_unlock_irqrestore(&psd->lock, flags);
>  
> @@ -417,7 +417,6 @@ int pm_clk_resume(struct device *dev)
>  	struct pm_subsys_data *psd = dev_to_psd(dev);
>  	struct pm_clock_entry *ce;
>  	unsigned long flags;
> -	int ret;
>  
>  	dev_dbg(dev, "%s()\n", __func__);
>  
> @@ -427,13 +426,8 @@ int pm_clk_resume(struct device *dev)
>  
>  	spin_lock_irqsave(&psd->lock, flags);
>  
> -	list_for_each_entry(ce, &psd->clock_list, node) {
> -		if (ce->status < PCE_STATUS_ERROR) {
> -			ret = __pm_clk_enable(dev, ce->clk);
> -			if (!ret)
> -				ce->status = PCE_STATUS_ENABLED;
> -		}
> -	}
> +	list_for_each_entry(ce, &psd->clock_list, node)
> +		__pm_clk_enable(dev, ce);
>  
>  	spin_unlock_irqrestore(&psd->lock, flags);
>  
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [RFC PATCH v4 3/3] PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME

[Dmitry Torokhov]

On Thu, Nov 06, 2014 at 03:51:02PM +0200, Grygorii Strashko wrote:
> Device's clocks need to be enabled always at probing time
> if !CONFIG_PM_RUNTIME - in that way, device will become accesible
> and, later, its clocks can be disabled/enabled during system
> suspend/resume by using pm_clk_suspend/pm_clk_resume APIs.
> 
> But now, the clocks management code doesn't enable clocks when
> clocks are being registered per-device by using pm_clk_add/_clk().
> So, update pm_clk_acquire(), which is called from pm_clk_add/_clk(),
> to enable clock always if !CONFIG_PM_RUNTIME.
> 
> Also, Platform PM domains drivers will not have to add code like below
> each time they need to handle the case when !CONFIG_PM_RUNTIME [1 - 3]:
> 
>        if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>                ret = pm_clk_resume(dev);
>                if (ret) {
>                        dev_err(dev, "pm_clk_resume failed %d\n", ret);
>                        goto clk_err;
>                };
>        }
> 
> [1] https://lkml.org/lkml/2014/10/3/306
> [2] https://lkml.org/lkml/2014/10/16/305
> [3] https://lkml.org/lkml/2014/10/20/249
> 
> CC: Santosh Shilimkar <ssantosh@kernel.org>
> CC: Kevin Hilman <khilman@linaro.org>
> CC: Ulf Hansson <ulf.hansson@linaro.org>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> CC: Philipp Zabel <p.zabel@pengutronix.de>
> CC: Caesar Wang <caesar.wang@rock-chips.com>
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> Hi,
> 
> I marked it as RFC, because:
>   Generic clock manipulation PM callbacks can be used for any device
>   for which list of clocks need to be maintained - and it doesn't mean
>   that this framework need to be always connected to GPD/Runtime PM or
>   used during suspend resume. Also, for DT-use case - it doesn't perform
>   any actions (except clk_prepare) on clocks by itself and only
>   by request from framework's consumer.
> 
>  For example, it can be reused by Power domain/controller drivers like:
>  - Rockchip RK3288 PM Domain (https://lkml.org/lkml/2014/11/6/30)
>  - IMX.6 PU power domain (http://www.spinics.net/lists/arm-kernel/msg360709.html)
> 
>  to maintain list of clocks for GPD itself and these clocks should not
>  be enabled by default, because they are used to power on/off Power domain
>  in a specific way:
>      - enable all clocks assigned to PD
>      - power on/off PD
>      - disable all clocks assigned to PD.
>  After this patch it will be impossible to reuse Generic clock manipulation PM
>  callbacks in cases described above.

I'd rather keep it separate and concentrate on making pm_clk* code
working with the power management core. Individual drivers can easily
keep list of their clocks for whatever additional purpose they need
without involving pm_clk* API.

Thanks.

-- 
Dmitry

Re: [RFC PATCH v4 3/3] PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME

[Grygorii Strashko]

On 11/06/2014 08:09 PM, Dmitry Torokhov wrote:
> On Thu, Nov 06, 2014 at 03:51:02PM +0200, Grygorii Strashko wrote:
>> Device's clocks need to be enabled always at probing time
>> if !CONFIG_PM_RUNTIME - in that way, device will become accesible
>> and, later, its clocks can be disabled/enabled during system
>> suspend/resume by using pm_clk_suspend/pm_clk_resume APIs.
>>
>> But now, the clocks management code doesn't enable clocks when
>> clocks are being registered per-device by using pm_clk_add/_clk().
>> So, update pm_clk_acquire(), which is called from pm_clk_add/_clk(),
>> to enable clock always if !CONFIG_PM_RUNTIME.
>>
>> Also, Platform PM domains drivers will not have to add code like below
>> each time they need to handle the case when !CONFIG_PM_RUNTIME [1 - 3]:
>>
>>         if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>                 ret = pm_clk_resume(dev);
>>                 if (ret) {
>>                         dev_err(dev, "pm_clk_resume failed %d\n", ret);
>>                         goto clk_err;
>>                 };
>>         }
>>
>> [1] https://lkml.org/lkml/2014/10/3/306
>> [2] https://lkml.org/lkml/2014/10/16/305
>> [3] https://lkml.org/lkml/2014/10/20/249
>>
>> CC: Santosh Shilimkar <ssantosh@kernel.org>
>> CC: Kevin Hilman <khilman@linaro.org>
>> CC: Ulf Hansson <ulf.hansson@linaro.org>
>> CC: Geert Uytterhoeven <geert@linux-m68k.org>
>> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> CC: Philipp Zabel <p.zabel@pengutronix.de>
>> CC: Caesar Wang <caesar.wang@rock-chips.com>
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> Hi,
>>
>> I marked it as RFC, because:
>>    Generic clock manipulation PM callbacks can be used for any device
>>    for which list of clocks need to be maintained - and it doesn't mean
>>    that this framework need to be always connected to GPD/Runtime PM or
>>    used during suspend resume. Also, for DT-use case - it doesn't perform
>>    any actions (except clk_prepare) on clocks by itself and only
>>    by request from framework's consumer.
>>
>>   For example, it can be reused by Power domain/controller drivers like:
>>   - Rockchip RK3288 PM Domain (https://lkml.org/lkml/2014/11/6/30)
>>   - IMX.6 PU power domain (http://www.spinics.net/lists/arm-kernel/msg360709.html)
>>
>>   to maintain list of clocks for GPD itself and these clocks should not
>>   be enabled by default, because they are used to power on/off Power domain
>>   in a specific way:
>>       - enable all clocks assigned to PD
>>       - power on/off PD
>>       - disable all clocks assigned to PD.
>>   After this patch it will be impossible to reuse Generic clock manipulation PM
>>   callbacks in cases described above.
>
> I'd rather keep it separate and concentrate on making pm_clk* code
> working with the power management core. Individual drivers can easily
> keep list of their clocks for whatever additional purpose they need
> without involving pm_clk* API.
>

In general, I'm ok with this patch for Keystone 2 proposes,
just added some notes about introduced limitations. And  all
current users of pm_clk* APIs will not regress with this patch, because
they will go through different code path if !CONFIG_PM_RUNTIME.

Regards,
-grygorii

Re: [PATCH v4 2/3] PM / clock_ops: make __pm_clk_enable more generic

[Kevin Hilman]

Grygorii Strashko <grygorii.strashko@ti.com> writes:

> Now there are two places in code which do the same things,
> so allow __pm_clk_enable() to accept pointer on pm_clock_entry
> structure as second parameter instead of pointer on clock and
> remove duplicated code.
>
> Also, updated function intended to be used by following patch.
>
> CC: Santosh Shilimkar <ssantosh@kernel.org>
> CC: Kevin Hilman <khilman@linaro.org>
> CC: Ulf Hansson <ulf.hansson@linaro.org>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Reviewed-by: Kevin Hilman <khilman@linaro.org>

Re: [PATCH v4 0/3] PM / clock_ops: add pm_clk_add_clk()

[Rafael J. Wysocki]

On Thursday, November 06, 2014 03:50:59 PM Grygorii Strashko wrote:
> Hi Santosh, Kevin,
> 
> I've separated these patches in standalone series as requested by
> Santosh (https://lkml.org/lkml/2014/10/24/591). Also, I've kept
> versioning of patches from original series and therefor this
> is v4 of patches.
> 
> The patch 1 was originally introduced in [1] (Apr 2014) and it allows
> to fill per-device list of clock from DT.
> 
> The patch 2 is small clean up needed for patch 3.
> 
> The patch 3 was created as was discussed in v2 and first introduced in v3.
> Here I marked it as RFC, because I've found some disadvantages of such
> approach - see comments to the patch itself.
> 
> Finally, this series is needed to enable to enable Runtime
> PM for Keystone 2, but honestly I need only patch 1.
> 
> RFC version of patches can be found at [2].
> 
> Changes in v4:
>  - pm_clk_add_clk() changed to get reference on clock as
>    requested by Dmitry Torokhov (see v3). Now the caller has to
>    use clk_put() on clock pointer when done.
> 
> Changes in v3:
>  - handling of the case when !CONFIG_PM_RUNTIME has been
>    moved in PM clock framework.
> 
> Changes in v2:
> - minor comments applied and rebased on top of Linux 3.18-rc1.
> 
> Links on previous versions:
> v3:  https://lkml.org/lkml/2014/10/23/342
> v2:  https://lkml.org/lkml/2014/10/20/248
> v1:  https://lkml.org/lkml/2014/9/29/382
> 
> [1] "[PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core"
>   https://lkml.org/lkml/2014/4/24/1118
> 
> [2] "[RFC PATCH 0/4] ARM: keystone: pm: switch to use generic pm domains"
>   https://lkml.org/lkml/2014/9/25/364
> 
> CC: Santosh Shilimkar <ssantosh@kernel.org>
> CC: Kevin Hilman <khilman@linaro.org>
> CC: Ulf Hansson <ulf.hansson@linaro.org>
> CC: Geert Uytterhoeven <geert@linux-m68k.org>
> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Geert Uytterhoeven (1):
>   PM / clock_ops: Add pm_clk_add_clk()
> 
> Grygorii Strashko (2):
>   PM / clock_ops: make __pm_clk_enable more generic
>   PM / clock_ops: add and enable clocks always if !CONFIG_PM_RUNTIME
> 
>  drivers/base/power/clock_ops.c | 89 +++++++++++++++++++++++++++---------------
>  include/linux/pm_clock.h       |  8 ++++
>  2 files changed, 65 insertions(+), 32 deletions(-)

Patches [1-2/3] queued up for 3.19, thanks!

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