On 24 October 2014 18:39, Dmitry Torokhov [off-list ref] wrote:

On Fri, Oct 24, 2014 at 11:53:05AM +0200, Ulf Hansson wrote:

quoted

On 23 October 2014 16:37, Grygorii Strashko [off-list ref] wrote:

quoted

Hi Ulf,

On 10/23/2014 11:11 AM, Ulf Hansson wrote:

quoted

On 22 October 2014 17:44, Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson [off-list ref] wrote:

quoted

On 22 October 2014 17:09, Geert Uytterhoeven [off-list ref] wrote:

quoted

On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson [off-list ref] wrote:

quoted

quoted

quoted

quoted

+void keystone_pm_domain_attach_dev(struct device *dev)
   {
+    struct clk *clk;
       int ret;
+    int i = 0;

       dev_dbg(dev, "%s\n", __func__);

-    ret = pm_generic_runtime_suspend(dev);
-    if (ret)
-        return ret;
-
-    ret = pm_clk_suspend(dev);
+    ret = pm_clk_create(dev);
       if (ret) {
-        pm_generic_runtime_resume(dev);
-        return ret;
+        dev_err(dev, "pm_clk_create failed %d\n", ret);
+        return;
+    };
+
+    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
+        ret = pm_clk_add_clk(dev, clk);
+        if (ret) {
+            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
+            goto clk_err;
+        };
       }

-    return 0;
+    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {

Can we not okkup two seperate callbacks instead of above check ?
I don't like this CONFIG check here. Its slightly better version of
ifdef in middle of the code.

I've found more-less similar comment on patch
"Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
https://lkml.org/lkml/2014/10/17/257

So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
in case !IS_ENABLED(CONFIG_PM_RUNTIME)

I am wondering whether we actually should/could do this, no matter of
CONFIG_PM_RUNTIME.

Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
clocks through pm_clk_suspend(), will be gated once the device becomes
runtime PM suspended. Right?

Doing it unconditionally means we'll have lots of unneeded clocks running
for a short while.

quoted

As long as the pm_clk_add() is being invoked from the ->attach_dev()
callback, we are in the probe path. Certainly we would like to have
clocks enabled while probing, don't you think?

If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
those be enabled?

They will be enabled when the driver does

         pm_runtime_enable(dev);
         pm_runtime_get_sync(dev);

in its .probe() method.

No! This doesn't work for drivers which have used
pm_runtime_set_active() prior pm_runtime_enable().

Sorry, but some misunderstanding is here:
1) If some code call pm_runtime_set_active() it has to ensure
that all PM resources switched to ON state. All! So, it will
be ok to call enable & get after that - these functions will only
adjust counters.

Correct.

This is also the key problem with your approach. You requires a
pm_runtime_get_sync() to trigger the runtime PM resume callbacks to be
invoked. That's a fragile design.

Why is this fragile design? Having pm_runtime_get_sync() result in
resuming the device (and in turn the PM domain it is in) if device is
suspended is the proper behavior, no?

It's fragile, because the device may very well be "runtime PM active"
at the point when we invoke pm_runtime_get_sync(). Thus the runtime PM
resume callback isn't invoked, which is a requirement for these cases
to work.

Kind regards
Uffe