Add runtime PM support for clocks (on Exynos SoC example)

Registers for some clocks might be located in the SOC area, which are under the power domain. To enable access to those registers respective domain has to be turned on. Additionally, registers for such clocks will usually loose its contents when power domain is turned off, so additional saving and restoring of them might be needed in the clock controller driver.

This patch adds basic infrastructure in the clocks core to allow implementing driver for such clocks under power domains. Clock provider can supply a struct device pointer, which is the used by clock core for tracking and managing clock's controller runtime pm state. Each clk_prepare() operation will first call pm_runtime_get_sync() on the supplied device, while clk_unprepare() will do pm_runtime_put() at the end.

Additional calls to pm_runtime_get/put functions are required to ensure that any register access (like calculating/chaning clock rates) will be done with clock controller in active runtime state.

Special handling of the case when runtime pm is disabled for clock controller's device is needed to let this feature work properly also during system sleep suspend/resume operations (runtime pm is first disabled before entering sleep state's, but controller is usually still operational until its suspend pm callback is called).

Signed-off-by Marek Szyprowski <[email protected]>

Quoted 23 lines · Marek Szyprowski

Registers for some clocks might be located in the SOC area, which are under the power domain. To enable access to those registers respective domain has to be turned on. Additionally, registers for such clocks will usually loose its contents when power domain is turned off, so additional saving and restoring of them might be needed in the clock controller driver.

This patch adds basic infrastructure in the clocks core to allow implementing driver for such clocks under power domains. Clock provider can supply a struct device pointer, which is the used by clock core for tracking and managing clock's controller runtime pm state. Each clk_prepare() operation will first call pm_runtime_get_sync() on the supplied device, while clk_unprepare() will do pm_runtime_put() at the end.

Additional calls to pm_runtime_get/put functions are required to ensure that any register access (like calculating/chaning clock rates) will be done with clock controller in active runtime state.

Special handling of the case when runtime pm is disabled for clock controller's device is needed to let this feature work properly also during system sleep suspend/resume operations (runtime pm is first disabled before entering sleep state's, but controller is usually still operational until its suspend pm callback is called).

Quoted 1 line

Signed-off-by Marek Szyprowski <[email protected]>

My "knee jerk" concern is that we're going to take a runtime PM lock underneath the prepare lock. That seems like a situation where we could hit a lock inversion if the runtime PM callbacks themselves acquire the prepare lock by calling clk APIs? But this concern is false right? We release the runtime PM lock before calling the PM callback, so we shouldn't hit any deadlock and lockdep won't complain?

Hi Stephen,

Quoted 24 lines · Stephen Boyd

Quoted 23 lines · Marek Szyprowski

Registers for some clocks might be located in the SOC area, which are under the power domain. To enable access to those registers respective domain has to be turned on. Additionally, registers for such clocks will usually loose its contents when power domain is turned off, so additional saving and restoring of them might be needed in the clock controller driver.

This patch adds basic infrastructure in the clocks core to allow implementing driver for such clocks under power domains. Clock provider can supply a struct device pointer, which is the used by clock core for tracking and managing clock's controller runtime pm state. Each clk_prepare() operation will first call pm_runtime_get_sync() on the supplied device, while clk_unprepare() will do pm_runtime_put() at the end.

Additional calls to pm_runtime_get/put functions are required to ensure that any register access (like calculating/chaning clock rates) will be done with clock controller in active runtime state.

Special handling of the case when runtime pm is disabled for clock controller's device is needed to let this feature work properly also during system sleep suspend/resume operations (runtime pm is first disabled before entering sleep state's, but controller is usually still operational until its suspend pm callback is called).

Quoted 8 lines

Quoted 1 line

Signed-off-by Marek Szyprowski <[email protected]>

My "knee jerk" concern is that we're going to take a runtime PM lock underneath the prepare lock. That seems like a situation where we could hit a lock inversion if the runtime PM callbacks themselves acquire the prepare lock by calling clk APIs? But this concern is false right? We release the runtime PM lock before calling the PM callback, so we shouldn't hit any deadlock and lockdep won't complain?

Runtime PM uses fine grained locking based on per-device locks, so there should be no problem with global clock prepare lock. The only lock interaction
is between clock controller device's rpm lock and clocks global prepare lock, but
it always done with the same access pattern. I've tested it extensively (also
with lock dep) with various use cases and found no problems.

Quoted 36 lines · Marek Szyprowski

Hi Stephen,

Quoted 24 lines · Stephen Boyd

Quoted 23 lines · Marek Szyprowski

Registers for some clocks might be located in the SOC area, which are under the power domain. To enable access to those registers respective domain has to be turned on. Additionally, registers for such clocks will usually loose its contents when power domain is turned off, so additional saving and restoring of them might be needed in the clock controller driver.

This patch adds basic infrastructure in the clocks core to allow implementing driver for such clocks under power domains. Clock provider can supply a struct device pointer, which is the used by clock core for tracking and managing clock's controller runtime pm state. Each clk_prepare() operation will first call pm_runtime_get_sync() on the supplied device, while clk_unprepare() will do pm_runtime_put() at the end.

Additional calls to pm_runtime_get/put functions are required to ensure that any register access (like calculating/chaning clock rates) will be done with clock controller in active runtime state.

Special handling of the case when runtime pm is disabled for clock controller's device is needed to let this feature work properly also during system sleep suspend/resume operations (runtime pm is first disabled before entering sleep state's, but controller is usually still operational until its suspend pm callback is called).

Quoted 8 lines

Quoted 1 line

Signed-off-by Marek Szyprowski <[email protected]>

My "knee jerk" concern is that we're going to take a runtime PM lock underneath the prepare lock. That seems like a situation where we could hit a lock inversion if the runtime PM callbacks themselves acquire the prepare lock by calling clk APIs? But this concern is false right? We release the runtime PM lock before calling the PM callback, so we shouldn't hit any deadlock and lockdep won't complain?

Quoted 9 lines

Runtime PM uses fine grained locking based on per-device locks, so there should be no problem with global clock prepare lock. The only lock interaction
is between clock controller device's rpm lock and clocks global prepare lock, but
it always done with the same access pattern. I've tested it extensively (also
with lock dep) with various use cases and found no problems.

Great! So you have runtime PM callbacks that are calling clk_prepare/unprepare?

Quoted 3 lines · Stephen Boyd

Great! So you have runtime PM callbacks that are calling clk_prepare/unprepare?

Well, not really. clock controller's runtime pm functions must not call clk_prepare/unprepare yet.

I didn't get your question. I thought that you are asking if my change won't introduce any deadlock related to prepare and dev->pm locks. My runtime pm functions doesn't do any call to clk_prepare/unprepare. Although global clock prepare lock is re-entrant from the same process, it would cause deadlock if called from runtime pm functions, because runtime pm functions might be called from the worker running on the different cpu/process.

Quoted 6 lines · Marek Szyprowski

Quoted 2 lines · Stephen Boyd

Great! So you have runtime PM callbacks that are calling clk_prepare/unprepare?

Well, not really. clock controller's runtime pm functions must not call clk_prepare/unprepare yet.

Quoted 7 lines

I didn't get your question. I thought that you are asking if my change won't introduce any deadlock related to prepare and dev->pm locks. My runtime pm functions doesn't do any call to clk_prepare/unprepare. Although global clock prepare lock is re-entrant from the same process, it would cause deadlock if called from runtime pm functions, because runtime pm functions might be called from the worker running on the different cpu/process.

I mean non-clk controller driver based runtime PM callbacks that call clk_prepare/unprepare in them. For example, some i2c or spi device driver that has clk operations in the runtime PM callbacks. That would allow lockdep to see any potential deadlock because of aliasing lock classes for the device power lock and the global prepare lock.

Quoted 17 lines · Marek Szyprowski

Registers for some clocks might be located in the SOC area, which are under the power domain. To enable access to those registers respective domain has to be turned on. Additionally, registers for such clocks will usually loose its contents when power domain is turned off, so additional saving and restoring of them might be needed in the clock controller driver.

This patch adds basic infrastructure in the clocks core to allow implementing driver for such clocks under power domains. Clock provider can supply a struct device pointer, which is the used by clock core for tracking and managing clock's controller runtime pm state. Each clk_prepare() operation will first call pm_runtime_get_sync() on the supplied device, while clk_unprepare() will do pm_runtime_put() at the end.

Additional calls to pm_runtime_get/put functions are required to ensure that any register access (like calculating/chaning clock rates) will be done with clock controller in active runtime state.

Quoted 7 lines

Special handling of the case when runtime pm is disabled for clock controller's device is needed to let this feature work properly also during system sleep suspend/resume operations (runtime pm is first disabled before entering sleep state's, but controller is usually still operational until its suspend pm callback is called).

Signed-off-by Marek Szyprowski <[email protected]>

My "knee jerk" concern is that we're going to take a runtime PM lock underneath the prepare lock. That seems like a situation where we could hit a lock inversion if the runtime PM callbacks themselves acquire the prepare lock by calling clk APIs? But this concern is false right? We release the runtime PM lock before calling the PM callback, so we shouldn't hit any deadlock and lockdep won't complain?

Registers for some clocks might be located in the SOC area, which are under the power domain. To enable access to those registers respective domain has to be turned on. Additionally, registers for such clocks will usually loose its contents when power domain is turned off, so additional saving and restoring of them might be needed in the clock controller driver.

This patch adds basic infrastructure in the clocks core to allow implementing driver for such clocks under power domains. Clock provider can supply a struct device pointer, which is the used by clock core for tracking and managing clock's controller runtime pm state. Each clk_prepare() operation will first call pm_runtime_get_sync() on the supplied device, while clk_unprepare() will do pm_runtime_put() at the end.

Additional calls to pm_runtime_get/put functions are required to ensure that any register access (like calculating/chaning clock rates) will be done with clock

controller in active runtime state.

Special handling of the case when runtime pm is disabled for clock controller's device is needed to let this feature work properly also during system sleep suspend/resume operations (runtime pm is first disabled before entering sleep state's, but controller is usually still operational until its suspend pm callback is called).

Signed-off-by Marek Szyprowski <[email protected]>

---
 drivers/clk/clk.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 8 deletions(-)

drivers/clk/clk.c +2 -0

index 820a939fb6bb..a1934e9b4e95 100644
@@ -21,6 +21,7 @@
 #include <linux/of.h>
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/pm_runtime.h>
 #include <linux/sched.h>
 #include <linux/clkdev.h>
 

@@ -46,6 +47,7 @@ struct clk_core {
        const struct clk_ops    *ops;
        struct clk_hw           *hw;
        struct module           *owner;
+       struct device           *dev;
        struct clk_core         *parent;
        const char              **parent_names;
        struct clk_core         **parents;

@@ -87,6 +89,42 @@ struct clk {
        struct hlist_node clks_node;
 };

drivers/clk/clk.c +17 -0

+       if (!core->dev)
+               return;
+
+       if (pm_runtime_enabled(core->dev))
+               pm_runtime_put(core->dev);
+       else
+               pm_runtime_put_noidle(core->dev);
+}
+
+static bool clk_pm_runtime_suspended(struct clk_core *core)
+{
+       if (!core->dev)
+               return 0;
+
+       return pm_runtime_suspended(core->dev);
+}
+
 /***           locking             ***/
 static void clk_prepare_lock(void)
 {

drivers/clk/clk.c +32 -9

        /*
         * .is_enabled is only mandatory for clocks that gate
         * fall back to software usage counter if .is_enabled is missing

@@ -489,6 +533,8 @@ static void clk_core_unprepare(struct clk_core *core)
        if (core->ops->unprepare)
                core->ops->unprepare(core->hw);
 
+       clk_pm_runtime_put(core);
+
        trace_clk_unprepare_complete(core);
        clk_core_unprepare(core->parent);
 }

@@ -530,10 +576,14 @@ static int clk_core_prepare(struct clk_core *core)
                return 0;
 
        if (core->prepare_count == 0) {
-               ret = clk_core_prepare(core->parent);
+               ret = clk_pm_runtime_get(core);
                if (ret)
                        return ret;
 
+               ret = clk_core_prepare(core->parent);
+               if (ret)
+                       goto runtime_put;
+
                trace_clk_prepare(core);
 
                if (core->ops->prepare)

@@ -541,15 +591,18 @@ static int clk_core_prepare(struct clk_core *core)
 
                trace_clk_prepare_complete(core);
 
-               if (ret) {
-                       clk_core_unprepare(core->parent);
-                       return ret;
-               }
+               if (ret)
+                       goto unprepare;
        }
 
        core->prepare_count++;
 
        return 0;
+unprepare:
+       clk_core_unprepare(core->parent);
+runtime_put:
+       clk_pm_runtime_put(core);
+       return ret;
 }
 
 static int clk_core_prepare_lock(struct clk_core *core)

@@ -1563,6 +1616,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 {
        struct clk_core *top, *fail_clk;
        unsigned long rate = req_rate;
+       int ret = 0;
 
        if (!core)
                return 0;

@@ -1579,21 +1633,28 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
        if (!top)
                return -EINVAL;
 
+       ret = clk_pm_runtime_get(core);
+       if (ret)
+               return ret;
+
        /* notify that we are about to change rates */
        fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
        if (fail_clk) {
                pr_debug("%s: failed to set %s rate\n", __func__,
                                fail_clk->name);
                clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
-               return -EBUSY;
+               ret = -EBUSY;
+               goto err;
        }
 
        /* change the rates */
        clk_change_rate(top);
 
        core->req_rate = req_rate;
+err:
+       clk_pm_runtime_put(core);
 
-       return 0;
+       return ret;
 }
 
 /**

@@ -1824,12 +1885,16 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
                p_rate = parent->rate;
        }
 
+       ret = clk_pm_runtime_get(core);
+       if (ret)
+               goto out;
+
        /* propagate PRE_RATE_CHANGE notifications */
        ret = __clk_speculate_rates(core, p_rate);
 
        /* abort if a driver objects */
        if (ret & NOTIFY_STOP_MASK)
-               goto out;
+               goto runtime_put;
 
        /* do the re-parent */
        ret = __clk_set_parent(core, parent, p_index);

@@ -1842,6 +1907,8 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
                __clk_recalc_accuracies(core);
        }
 
+runtime_put:
+       clk_pm_runtime_put(core);
 out:
        clk_prepare_unlock();
 

@@ -2546,6 +2613,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
                goto fail_name;
        }
        core->ops = hw->init->ops;
+       core->dev = dev;
        if (dev && dev->driver)
                core->owner = dev->driver->owner;
        core->hw = hw;
--

1.9.1

Quoted 5 lines · Marek Szyprowski

Registers for some clocks might be located in the SOC area, which are under the power domain. To enable access to those registers respective domain has to be turned on. Additionally, registers for such clocks will usually loose its contents when power domain is turned off, so additional saving and restoring of them might be needed in the clock controller driver.

This is indeed correct, I can confirm that the UX500 SoC's PRCC clock controllers also needs to be managed like this.

Quoted 6 lines

This patch adds basic infrastructure in the clocks core to allow implementing driver for such clocks under power domains. Clock provider can supply a struct device pointer, which is the used by clock core for tracking and managing clock's controller runtime pm state. Each clk_prepare() operation will first call pm_runtime_get_sync() on the supplied device, while clk_unprepare() will do pm_runtime_put() at the end.

This make sense!

Quoted 2 lines

Additional calls to pm_runtime_get/put functions are required to ensure that any register access (like calculating/chaning clock rates) will be done with clock

/s/chaning/changing

Quoted 1 line

controller in active runtime state.

/s/active runtime/runtime resumed

Quoted 5 lines

Special handling of the case when runtime pm is disabled for clock controller's device is needed to let this feature work properly also during system sleep suspend/resume operations (runtime pm is first disabled before entering sleep state's, but controller is usually still operational until its suspend pm callback is called).

This needs to be clarified. I agree we need to cover system PM as well, but let's try be a bit more precise about it.

Quoted 2 lines

Signed-off-by Marek Szyprowski <[email protected]>

I would also like to extend the change log to describe a little bit of how a clk provider should interact with this new and nice feature. Something like:

*) It needs to provide a struct device to the core when registering the provider.
**) It needs to enable runtime PM.
***) It needs to make sure the runtime PM status of the controller device reflects the HW state.

Quoted 28 lines

---
  drivers/clk/clk.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++------
  1 file changed, 76 insertions(+), 8 deletions(-)

drivers/clk/clk.c +2 -0

index 820a939fb6bb..a1934e9b4e95 100644
@@ -21,6 +21,7 @@
  #include <linux/of.h>
  #include <linux/device.h>
  #include <linux/init.h>
+#include <linux/pm_runtime.h>
  #include <linux/sched.h>
  #include <linux/clkdev.h>
 

@@ -46,6 +47,7 @@ struct clk_core {
         const struct clk_ops    *ops;
         struct clk_hw           *hw;
         struct module           *owner;
+       struct device           *dev;
         struct clk_core         *parent;
         const char              **parent_names;
         struct clk_core         **parents;

@@ -87,6 +89,42 @@ struct clk {
         struct hlist_node clks_node;
  };
 

drivers/clk/clk.c struct clk { · line 100

Quoted 9 lines

drivers/clk/clk.c +9 -0

+/***           runtime pm          ***/
+static int clk_pm_runtime_get(struct clk_core *core)
+{
+       int ret = 0;
+
+       if (!core->dev)
+               return 0;
+
+       if (pm_runtime_enabled(core->dev)) {

Why do you need to check for this?

drivers/clk/clk.c struct clk { · line 104

Quoted 5 lines

drivers/clk/clk.c +4 -0

+               ret = pm_runtime_get_sync(core->dev);
+       } else {
+               if (!pm_runtime_status_suspended(core->dev))
+                       pm_runtime_get_noresume(core->dev);

This looks weird. I guess it's related to the system PM case somehow?

drivers/clk/clk.c struct clk { · line 110

Quoted 6 lines

drivers/clk/clk.c +6 -0

+       }
+       return ret < 0 ? ret : 0;
+}
+
+static void clk_pm_runtime_put(struct clk_core *core)
+{

Similar comments as for clk_pm_runtime_get().

Quoted 21 lines

drivers/clk/clk.c +17 -0

+       if (!core->dev)
+               return;
+
+       if (pm_runtime_enabled(core->dev))
+               pm_runtime_put(core->dev);
+       else
+               pm_runtime_put_noidle(core->dev);
+}
+
+static bool clk_pm_runtime_suspended(struct clk_core *core)
+{
+       if (!core->dev)
+               return 0;
+
+       return pm_runtime_suspended(core->dev);
+}
+
  /***           locking             ***/
  static void clk_prepare_lock(void)
  {

drivers/clk/clk.c static void clk_enable_unlock(unsigned long flags) · line 193

Quoted 6 lines

drivers/clk/clk.c +3 -0

@@ -150,6 +188,9 @@ static void clk_enable_unlock(unsigned long flags)
 
  static bool clk_core_is_prepared(struct clk_core *core)
  {
+       if (clk_pm_runtime_suspended(core))
+               return false;
+

This isn't safe, as even if the clock controller is runtime resumed at this point, that's *not* a guarantee that is stays runtime resumed while invoking the ->ops->is_prepared().

Instead you must call a pm_runtime_get_noresume() before you check the runtime PM status, as that should avoid the device from being runtime suspended. Then when the ->ops->is_prepared() has been invoked, we should call pm_runtime_put().

Although, I am not sure the above change becomes entirely correct as I think we are mixing the runtime PM status with the clock prepare status here. In other words, the next time the clock controller becomes runtime resumed, it may very well restore some register context which may prepare the clock, unless someone explicitly has unprepared it.

Of course, it all depends on how clk_core_is_prepared() is used by the clock framework.

drivers/clk/clk.c static bool clk_core_is_prepared(struct clk_core *core) · line 208

Quoted 11 lines

drivers/clk/clk.c +3 -0

         /*
          * .is_prepared is optional for clocks that can prepare
          * fall back to software usage counter if it is missing

@@ -162,6 +203,9 @@ static bool clk_core_is_prepared(struct clk_core *core)
 
  static bool clk_core_is_enabled(struct clk_core *core)
  {
+       if (clk_pm_runtime_suspended(core))
+               return false;
+

Similar comment as for clk_core_is_prepared().

Quoted 117 lines

drivers/clk/clk.c +31 -8

         /*
          * .is_enabled is only mandatory for clocks that gate
          * fall back to software usage counter if .is_enabled is missing

@@ -489,6 +533,8 @@ static void clk_core_unprepare(struct clk_core *core)
         if (core->ops->unprepare)
                 core->ops->unprepare(core->hw);
 
+       clk_pm_runtime_put(core);
+
         trace_clk_unprepare_complete(core);
         clk_core_unprepare(core->parent);
  }

@@ -530,10 +576,14 @@ static int clk_core_prepare(struct clk_core *core)
                 return 0;
 
         if (core->prepare_count == 0) {
-               ret = clk_core_prepare(core->parent);
+               ret = clk_pm_runtime_get(core);
                 if (ret)
                         return ret;
 
+               ret = clk_core_prepare(core->parent);
+               if (ret)
+                       goto runtime_put;
+
                 trace_clk_prepare(core);
 
                 if (core->ops->prepare)

@@ -541,15 +591,18 @@ static int clk_core_prepare(struct clk_core *core)
 
                 trace_clk_prepare_complete(core);
 
-               if (ret) {
-                       clk_core_unprepare(core->parent);
-                       return ret;
-               }
+               if (ret)
+                       goto unprepare;
         }
 
         core->prepare_count++;
 
         return 0;
+unprepare:
+       clk_core_unprepare(core->parent);
+runtime_put:
+       clk_pm_runtime_put(core);
+       return ret;
  }
 
  static int clk_core_prepare_lock(struct clk_core *core)

@@ -1563,6 +1616,7 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
  {
         struct clk_core *top, *fail_clk;
         unsigned long rate = req_rate;
+       int ret = 0;
 
         if (!core)
                 return 0;

@@ -1579,21 +1633,28 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
         if (!top)
                 return -EINVAL;
 
+       ret = clk_pm_runtime_get(core);
+       if (ret)
+               return ret;
+
         /* notify that we are about to change rates */
         fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
         if (fail_clk) {
                 pr_debug("%s: failed to set %s rate\n", __func__,
                                 fail_clk->name);
                 clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
-               return -EBUSY;
+               ret = -EBUSY;
+               goto err;
         }
 
         /* change the rates */
         clk_change_rate(top);
 
         core->req_rate = req_rate;
+err:
+       clk_pm_runtime_put(core);
 
-       return 0;
+       return ret;
  }
 
  /**

@@ -1824,12 +1885,16 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
                 p_rate = parent->rate;
         }
 
+       ret = clk_pm_runtime_get(core);
+       if (ret)
+               goto out;
+
         /* propagate PRE_RATE_CHANGE notifications */
         ret = __clk_speculate_rates(core, p_rate);
 
         /* abort if a driver objects */
         if (ret & NOTIFY_STOP_MASK)
-               goto out;
+               goto runtime_put;
 
         /* do the re-parent */
         ret = __clk_set_parent(core, parent, p_index);

@@ -1842,6 +1907,8 @@ static int clk_core_set_parent(struct clk_core *core, struct clk_core *parent)
                 __clk_recalc_accuracies(core);
         }
 
+runtime_put:
+       clk_pm_runtime_put(core);
  out:
         clk_prepare_unlock();
 

Quoted 11 lines

drivers/clk/clk.c +1 -1

@@ -2546,6 +2613,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
                 goto fail_name;
         }
         core->ops = hw->init->ops;
+       core->dev = dev;
         if (dev && dev->driver)
                 core->owner = dev->driver->owner;
         core->hw = hw;
--

1.9.1

I believe we are also accessing the clock controller HW from the late_initcall_sync(clk_disable_unused) function.

More precisely, in clk_disable_unused_subtree(), we probably need a pm_runtime_get_sync() before calling clk_core_is_enabled(). And then restore that with a pm_runtime_put() after the clock has been disabled.
The similar is needed in clk_unprepare_unused_subtree().

Quoted 3 lines

This needs to be clarified. I agree we need to cover system PM as well, but let's try be a bit more precise about it.

Right, I wasn't precise here. I've developed this code on older (v4.1 and v4.6)
kernels, which had a code which disables runtime pm during system sleep transition
time. Maybe I need to revisit it and consider your change merged to v4.8-rc1, which
keeps runtime pm enabled during system sleep transitions.

Quoted 22 lines

drivers/clk/clk.c static void clk_enable_unlock(unsigned long flags) · line 193

Quoted 5 lines

  static bool clk_core_is_prepared(struct clk_core *core)
  {

drivers/clk/clk.c +3 -0

+       if (clk_pm_runtime_suspended(core))
+               return false;
+

This isn't safe, as even if the clock controller is runtime resumed at this point, that's *not* a guarantee that is stays runtime resumed while invoking the ->ops->is_prepared().

Instead you must call a pm_runtime_get_noresume() before you check the runtime PM status, as that should avoid the device from being runtime suspended. Then when the ->ops->is_prepared() has been invoked, we should call pm_runtime_put().

Although, I am not sure the above change becomes entirely correct as I think we are mixing the runtime PM status with the clock prepare status here. In other words, the next time the clock controller becomes runtime resumed, it may very well restore some register context which may prepare the clock, unless someone explicitly has unprepared it.

Quoted 2 lines

Of course, it all depends on how clk_core_is_prepared() is used by the clock framework.

clk_core_is_prepared() is mainly used by disable_unused_tree_*. You are right that it mixes a bit clock prepared state with runtime pm active state of clock controller's, but I assumed here that clock cannot be prepared if runtime pm state of controller is suspended. Other approach here would be to call pm_runtime_get(), check status and then pm_runtime_put(). If you prefer such approach, I will change it.

Quoted 3 lines

I believe we are also accessing the clock controller HW from the late_initcall_sync(clk_disable_unused) function.

This was indirectly handled by the runtime pm state check in is_prepared and is_enabled().

Quoted 6 lines

More precisely, in clk_disable_unused_subtree(), we probably need a pm_runtime_get_sync() before calling clk_core_is_enabled(). And then restore that with a pm_runtime_put() after the clock has been disabled.
The similar is needed in clk_unprepare_unused_subtree().

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[...]

Quoted 10 lines

Quoted 2 lines

This needs to be clarified. I agree we need to cover system PM as well, but let's try be a bit more precise about it.

Right, I wasn't precise here. I've developed this code on older (v4.1 and v4.6)
kernels, which had a code which disables runtime pm during system sleep transition
time. Maybe I need to revisit it and consider your change merged to v4.8-rc1, which
keeps runtime pm enabled during system sleep transitions.

Right, I see.

Quoted 24 lines

Quoted 21 lines

drivers/clk/clk.c static void clk_enable_unlock(unsigned long flags) · line 193

Quoted 5 lines

   static bool clk_core_is_prepared(struct clk_core *core)
   {

drivers/clk/clk.c +3 -0

+       if (clk_pm_runtime_suspended(core))
+               return false;
+

This isn't safe, as even if the clock controller is runtime resumed at this point, that's *not* a guarantee that is stays runtime resumed while invoking the ->ops->is_prepared().

Instead you must call a pm_runtime_get_noresume() before you check the runtime PM status, as that should avoid the device from being runtime suspended. Then when the ->ops->is_prepared() has been invoked, we should call pm_runtime_put().

Although, I am not sure the above change becomes entirely correct as I think we are mixing the runtime PM status with the clock prepare status here. In other words, the next time the clock controller becomes runtime resumed, it may very well restore some register context which may prepare the clock, unless someone explicitly has unprepared it.

Quoted 2 lines

Of course, it all depends on how clk_core_is_prepared() is used by the clock framework.

Quoted 6 lines

clk_core_is_prepared() is mainly used by disable_unused_tree_*. You are right that it mixes a bit clock prepared state with runtime pm active state of clock controller's, but I assumed here that clock cannot be prepared if runtime pm state of controller is suspended. Other approach here would be to call pm_runtime_get(), check status and then pm_runtime_put(). If you prefer such approach, I will change it.

Using pm_runtime_get|put() would work for the clk_core_is_prepared() case, although perhaps not for the clk_core_is_enabled() case.

The reason is that I guess the clk_core_is_enabled() API may be called from atomic context? Thus we would need to enable pm_runtime_irq_safe() for the clock provider device, which I *really* would like to avoid.

[...]

Quoted 5 lines

Quoted 2 lines

I believe we are also accessing the clock controller HW from the late_initcall_sync(clk_disable_unused) function.

This was indirectly handled by the runtime pm state check in is_prepared and is_enabled().

I see.

Although, I was thinking that you explicitly would like to disable/unprepare unused clocks in this phase, so then it isn't sufficient to rely on the runtime PM status to know whether the clock is prepared/enabled.

Perhaps, this is the only case when you actually need a pm_runtime_get|put() around the ->is_enabled|prepared()!?

Exynos4412 clock controller contains some additional clocks for FIMC-ISP (Camera ISP) subsystem. Registers for those clocks are partially located in the SOC area, which belongs to ISP power domain.

This patch extends clock controller node with ISP clock sub-node and link (phandle) to ISP power domain.

Signed-off-by Marek Szyprowski <[email protected]>

---
 arch/arm/boot/dts/exynos4x12.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/boot/dts/exynos4x12.dtsi +0 -0

index 3394bdcf10ae..4daea67546b9 100644

Quoted 23 lines · Marek Szyprowski

Exynos4412 clock controller contains some additional clocks for FIMC-ISP (Camera ISP) subsystem. Registers for those clocks are partially located in the SOC area, which belongs to ISP power domain.

This patch extends clock controller node with ISP clock sub-node and link (phandle) to ISP power domain.

Signed-off-by Marek Szyprowski <[email protected]>

---
  arch/arm/boot/dts/exynos4x12.dtsi | 5 +++++
  1 file changed, 5 insertions(+)

arch/arm/boot/dts/exynos4x12.dtsi +3 -0

index 3394bdcf10ae..4daea67546b9 100644
@@ -74,6 +74,11 @@
  		compatible = "samsung,exynos4412-clock";
  		reg = <0x10030000 0x20000>;
  		#clock-cells = <1>;
+
+		isp-clock-controller {
+			compatible = "samsung,exynos4412-isp-clock";

arch/arm/boot/dts/exynos4x12.dtsi line 81

Quoted 2 lines

arch/arm/boot/dts/exynos4x12.dtsi +2 -0

+			power-domains = <&pd_isp>;
+		};

Why can't we extend support in power domains code to have multiple domains for a single device node? i.e. power-domains = <&pd_isp>, <&pd_foo>, <&pd_bar>, and then pick the right one with power-domain-names or something like that? Making a subnode (which seems to turn into a child platform device?) seems like a quick solution for larger problems.

Hi Stephen,

Quoted 24 lines · Stephen Boyd

Quoted 23 lines · Marek Szyprowski

Exynos4412 clock controller contains some additional clocks for FIMC-ISP (Camera ISP) subsystem. Registers for those clocks are partially located in the SOC area, which belongs to ISP power domain.

This patch extends clock controller node with ISP clock sub-node and link (phandle) to ISP power domain.

Signed-off-by Marek Szyprowski <[email protected]>

---
 arch/arm/boot/dts/exynos4x12.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/boot/dts/exynos4x12.dtsi +3 -0

index 3394bdcf10ae..4daea67546b9 100644
@@ -74,6 +74,11 @@
 		compatible = "samsung,exynos4412-clock";
 		reg = <0x10030000 0x20000>;
 		#clock-cells = <1>;
+
+		isp-clock-controller {
+			compatible = "samsung,exynos4412-isp-clock";

Quoted 8 lines

arch/arm/boot/dts/exynos4x12.dtsi line 81

Quoted 2 lines

arch/arm/boot/dts/exynos4x12.dtsi +2 -0

+			power-domains = <&pd_isp>;
+		};

Why can't we extend support in power domains code to have multiple domains for a single device node? i.e. power-domains = <&pd_isp>, <&pd_foo>, <&pd_bar>, and then pick the right one with power-domain-names or something like that? Making a subnode (which seems to turn into a child platform device?) seems like a quick solution for larger problems.

The larger problem here is the fact that clock controller is partially located
in different power areas of SoC. Majority of the clock controllers is located
in the area which is typically always powered (besides system sleep case), while a few Camera ISP registers are located in the ISP block, which have separate power domain. Having a separate nodes for sub-parts of the device is
rather common approach, already practices by some more complex devices.

I see some serious design problems with multiple entries in power domains property. First how to show that some part of the device IS NOT in any domain?

The question is how the automated assignment to domains would be handled for such case?

The second is related to Linux kernel internals. Right now device drivers are
not aware of the power domains - there are no direct calls to power domains code, everything is hidden behind runtime pm which does all the hard work.

Similar situation is on Exynos 542x/5800, which will look more or less like this:

        clock: clock-controller at 10010000 {
             compatible = "samsung,exynos5420-clock";
             reg = <0x10010000 0x30000>;
             #clock-cells = <1>;

(quoted diff) +25 -0

+
+            gsc-clock-controller {
+                 compatible = "samsung,exynos5420-gsc-clock";
+                 power-domains = <&gsc_pd>;
+            };
+
+            isp-clock-controller {
+                 compatible = "samsung,exynos5420-isp-clock";
+                 power-domains = <&isp_pd>;
+            };
+
+            mfc-clock-controller {
+                 compatible = "samsung,exynos5420-mfc-clock";
+                 power-domains = <&mfc_pd>;
+            };
+
+            msc-clock-controller {
+                 compatible = "samsung,exynos5420-msc-clock";
+                 power-domains = <&msc_pd>;
+            };
+
+            disp-clock-controller {
+                 compatible = "samsung,exynos5420-disp-clock";
+                 power-domains = <&disp_pd>;
+            };
         };

The patch is not yet ready, so I didn't include it in this patchset.

Quoted 43 lines · Marek Szyprowski

Quoted 24 lines · Stephen Boyd

Quoted 23 lines · Marek Szyprowski

Exynos4412 clock controller contains some additional clocks for FIMC-ISP (Camera ISP) subsystem. Registers for those clocks are partially located in the SOC area, which belongs to ISP power domain.

This patch extends clock controller node with ISP clock sub-node and link (phandle) to ISP power domain.

Signed-off-by Marek Szyprowski <[email protected]>

---
  arch/arm/boot/dts/exynos4x12.dtsi | 5 +++++
  1 file changed, 5 insertions(+)

arch/arm/boot/dts/exynos4x12.dtsi +3 -0

index 3394bdcf10ae..4daea67546b9 100644
@@ -74,6 +74,11 @@
  		compatible = "samsung,exynos4412-clock";
  		reg = <0x10030000 0x20000>;
  		#clock-cells = <1>;
+
+		isp-clock-controller {
+			compatible = "samsung,exynos4412-isp-clock";

Quoted 8 lines

arch/arm/boot/dts/exynos4x12.dtsi line 81

Quoted 2 lines

arch/arm/boot/dts/exynos4x12.dtsi +2 -0

+			power-domains = <&pd_isp>;
+		};

Why can't we extend support in power domains code to have multiple domains for a single device node? i.e. power-domains = <&pd_isp>, <&pd_foo>, <&pd_bar>, and then pick the right one with power-domain-names or something like that? Making a subnode (which seems to turn into a child platform device?) seems like a quick solution for larger problems.

The larger problem here is the fact that clock controller is partially located
in different power areas of SoC. Majority of the clock controllers is located
in the area which is typically always powered (besides system sleep case), while a few Camera ISP registers are located in the ISP block, which have separate power domain. Having a separate nodes for sub-parts of the device is
rather common approach, already practices by some more complex devices.

Quoted 3 lines

I see some serious design problems with multiple entries in power domains property. First how to show that some part of the device IS NOT in any domain?

Is that even possible? Every device should be in some power domain, even if it's just an "always on" power domain that we don't really control from software.

Quoted 3 lines

The question is how the automated assignment to domains would be handled for such case?

I don't get this part. Do you mean how we indicate to the driver which power domain to use at the right time?

Quoted 5 lines

The second is related to Linux kernel internals. Right now device drivers are
not aware of the power domains - there are no direct calls to power domains code, everything is hidden behind runtime pm which does all the hard work.

Right. Runtime PM will need to be improved to allow this case.

Quoted 34 lines

Similar situation is on Exynos 542x/5800, which will look more or less like this:

         clock: clock-controller at 10010000 {
              compatible = "samsung,exynos5420-clock";
              reg = <0x10010000 0x30000>;
              #clock-cells = <1>;

(quoted diff) +25 -0

+
+            gsc-clock-controller {
+                 compatible = "samsung,exynos5420-gsc-clock";
+                 power-domains = <&gsc_pd>;
+            };
+
+            isp-clock-controller {
+                 compatible = "samsung,exynos5420-isp-clock";
+                 power-domains = <&isp_pd>;
+            };
+
+            mfc-clock-controller {
+                 compatible = "samsung,exynos5420-mfc-clock";
+                 power-domains = <&mfc_pd>;
+            };
+
+            msc-clock-controller {
+                 compatible = "samsung,exynos5420-msc-clock";
+                 power-domains = <&msc_pd>;
+            };
+
+            disp-clock-controller {
+                 compatible = "samsung,exynos5420-disp-clock";
+                 power-domains = <&disp_pd>;
+            };
          };

Quoted 1 line

The patch is not yet ready, so I didn't include it in this patchset.

Ok. From a DT perspective the sub-nodes seem to be a workaround for how the linux device model is mapped to power domains. I'm not sure we want to make subnodes in the clk controller just to make sub devices that we can target from the clk registration path. Those sub nodes aren't devices at all. I understand why it's being done this way, I just don't see how it fits into DT design methodologies.

Quoted 4 lines

Quoted 3 lines

I see some serious design problems with multiple entries in power domains property. First how to show that some part of the device IS NOT in any domain?

Quoted 3 lines

Is that even possible? Every device should be in some power domain, even if it's just an "always on" power domain that we don't really control from software.

Right now none dts of which I'm aware of doesn't define the power domain for the parts of the SoC, which are always on and doesn't need any additional management.

Add runtime pm support for all clock controller units (CMU), which belongs to power domains and require special handling during on/off operations. Typically special values has to be written to MUX registers to change internal clocks parents to OSC clock before turning power off. During such operation all clocks, which enters CMU has to be enabled to let MUX to stabilize. Also for each CMU there is one special parent clock, which has to be enabled all the time when any access to CMU registers is done.

This patch solves most of the mysterious external abort and freeze issues caused by a lack of proper parent CMU clock enabled or incorrect turn off procedure.

Signed-off-by Marek Szyprowski <[email protected]>

---
 drivers/clk/samsung/clk-exynos5433.c | 462 +++++++++++++++++++++++++++--------
 1 file changed, 363 insertions(+), 99 deletions(-)

drivers/clk/samsung/clk-exynos5433.c +25 -0

index ea1608682d7f..9fb896b56ddf 100644
@@ -9,9 +9,14 @@
  * Common Clock Framework support for Exynos5443 SoC.
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
 
 #include <dt-bindings/clock/exynos5433.h>
 

@@ -19,6 +24,37 @@
 #include "clk-cpu.h"
 #include "clk-pll.h"
 
+struct exynos5433_cmu_info {
+	/* total number of clocks with IDs assigned*/
+	unsigned int nr_clk_ids;
+	/* list of pll clocks and respective count */
+	const struct samsung_pll_clock *pll_clks;
+	unsigned int nr_pll_clks;
+	/* list of mux clocks and respective count */
+	const struct samsung_mux_clock *mux_clks;
+	unsigned int nr_mux_clks;
+	/* list of div clocks and respective count */
+	const struct samsung_div_clock *div_clks;
+	unsigned int nr_div_clks;
+	/* list of gate clocks and respective count */
+	const struct samsung_gate_clock *gate_clks;
+	unsigned int nr_gate_clks;
+	/* list of fixed clocks and respective count */
+	const struct samsung_fixed_rate_clock *fixed_clks;
+	unsigned int nr_fixed_clks;
+	/* list of fixed factor clocks and respective count */
+	const struct samsung_fixed_factor_clock *fixed_factor_clks;