On Wed, Oct 22, 2014 at 02:16:31PM -0700, Dmitry Torokhov wrote: > On Wed, Oct 22, 2014 at 01:14:09PM -0700, Dmitry Torokhov wrote: > > On Wed, Oct 22, 2014 at 10:02:41PM +0300, Grygorii Strashko wrote: > > > On 10/22/2014 08:38 PM, Dmitry Torokhov wrote: > > > >On Mon, Oct 20, 2014 at 03:56:02PM +0300, Grygorii Strashko wrote: > > > >>From: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > > >> > > > >>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. > > > >> > > > >>Reviewed-by: Kevin Hilman <khilman@xxxxxxxxxx> > > > >>Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> > > > >>Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx> > > > >>--- > > > >> > > > >> Pay attantion pls, that there is another series of patches > > > >> which have been posted already and which depends from this patch > > > >> "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support" > > > >> https://lkml.org/lkml/2014/10/20/105 > > > >> > > > >> drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++---------- > > > >> include/linux/pm_clock.h | 8 ++++++++ > > > >> 2 files changed, 39 insertions(+), 10 deletions(-) > > > >> > > > >>diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c > > > >>index 7836930..f14b767 100644 > > > >>--- a/drivers/base/power/clock_ops.c > > > >>+++ b/drivers/base/power/clock_ops.c > > > >>@@ -53,7 +53,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 +64,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 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id) > > > >> kfree(ce); > > > >> return -ENOMEM; > > > >> } > > > >>+ } else { > > > >>+ ce->clk = clk; > > Shouldn't this be > > ce->clk = __clk_get(clk); > > to account for clk_put() in __pm_clk_remove()? > > > > >> } > > > >> > > > >> pm_clk_acquire(dev, ce); > > > >>@@ -104,6 +100,31 @@ 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); > > > > > > > >Bikeshedding: why do we need __pm_clk_add() and not simply have > > > >"canonical" pm_clk_add_clk() and then do: > > > > > > > >int pm_clk_add(struct device *dev, const char *con_id) > > > >{ > > > > struct clk *clk; > > > > > > > > clk = clk_get(dev, con_id); > > > > ... > > > > return pm_clk_add_clk(dev, clk); > > > >} > > > > > > Hm. I did fast look at code and: > > > 1) agree - there is a lot of thing which can be optimized ;) > > > 2) in my strong opinion, this patch is the fastest and simplest > > > way to introduce new API (take a look on pm_clock_entry->con_id > > > management code) and It is exactly what we need as of now. > > > > Yeah, I guess. We are lucky we do not crash when we are tryign to print > > NULL strings (see pm_clk_acquire). > > > > BTW, what is the point of doing pm_clk_add(dev, NULL)? We add clock > > entry with status PCE_STATUS_ERROR and then have to handle it > > everywhere? Can we just return -EINVAL if someone triies to pass NULL > > ass con_id? > > Umm, no, ignore me here, I misread clk_get(dev, NULL) - it won't return > error. Still, do why do we need to keep clock entry if clk_get() fails > for some reason? OK, so what if we do something like the patch below? Thanks. -- Dmitry PM / clock_ops: Add pm_clk_remove_clk() Implement pm_clk_remove_clk() that complements the new pm_clk_add_clk(). Fix reference counting, rework the code to avoid storing invalid clocks, clean up the code. Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxxxx> --- drivers/base/power/clock_ops.c | 169 ++++++++++++++++++++--------------------- 1 file changed, 81 insertions(+), 88 deletions(-) diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index f14b767..840e133 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -12,21 +12,21 @@ #include <linux/pm.h> #include <linux/pm_clock.h> #include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/clkdev.h> #include <linux/slab.h> #include <linux/err.h> #ifdef CONFIG_PM enum pce_status { - PCE_STATUS_NONE = 0, - PCE_STATUS_ACQUIRED, + PCE_STATUS_ACQUIRED = 0, + PCE_STATUS_PREPARED, PCE_STATUS_ENABLED, - PCE_STATUS_ERROR, }; struct pm_clock_entry { struct list_head node; - char *con_id; struct clk *clk; enum pce_status status; }; @@ -47,25 +47,13 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk) } /** - * pm_clk_acquire - Acquire a device clock. - * @dev: Device whose clock is to be acquired. - * @ce: PM clock entry corresponding to the clock. + * 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. */ -static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce) -{ - if (!ce->clk) - ce->clk = clk_get(dev, ce->con_id); - if (IS_ERR(ce->clk)) { - ce->status = PCE_STATUS_ERROR; - } else { - clk_prepare(ce->clk); - ce->status = PCE_STATUS_ACQUIRED; - dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id); - } -} - -static int __pm_clk_add(struct device *dev, const char *con_id, - struct clk *clk) +int pm_clk_add_clk(struct device *dev, struct clk *clk) { struct pm_subsys_data *psd = dev_to_psd(dev); struct pm_clock_entry *ce; @@ -79,23 +67,19 @@ static int __pm_clk_add(struct device *dev, const char *con_id, return -ENOMEM; } - if (con_id) { - ce->con_id = kstrdup(con_id, GFP_KERNEL); - if (!ce->con_id) { - dev_err(dev, - "Not enough memory for clock connection ID.\n"); - kfree(ce); - return -ENOMEM; - } - } else { - ce->clk = clk; - } + __clk_get(clk); - pm_clk_acquire(dev, ce); + clk_prepare(clk); + + ce->status = PCE_STATUS_PREPARED; + ce->clk = clk; spin_lock_irq(&psd->lock); list_add_tail(&ce->node, &psd->clock_list); spin_unlock_irq(&psd->lock); + + dev_dbg(dev, "Clock %s managed by runtime PM.\n", __clk_get_name(clk)); + return 0; } @@ -109,19 +93,23 @@ static int __pm_clk_add(struct device *dev, const char *con_id, */ int pm_clk_add(struct device *dev, const char *con_id) { - return __pm_clk_add(dev, con_id, NULL); -} + struct clk *clk; + int retval; -/** - * 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. - */ -int pm_clk_add_clk(struct device *dev, struct clk *clk) -{ - return __pm_clk_add(dev, NULL, clk); + clk = clk_get(dev, con_id); + if (IS_ERR(clk)) { + retval = PTR_ERR(clk); + dev_err(dev, "Failed to locate lock (con_id %s): %d\n", + con_id, retval); + return retval; + } + + retval = pm_clk_add_clk(dev, clk); + + /* pm_clk_add_clk takes its own reference to clk */ + clk_put(clk); + + return retval; } /** @@ -133,32 +121,30 @@ static void __pm_clk_remove(struct pm_clock_entry *ce) if (!ce) return; - if (ce->status < PCE_STATUS_ERROR) { - if (ce->status == PCE_STATUS_ENABLED) - clk_disable(ce->clk); + if (ce->status == PCE_STATUS_ENABLED) + clk_disable(ce->clk); - if (ce->status >= PCE_STATUS_ACQUIRED) { - clk_unprepare(ce->clk); - clk_put(ce->clk); - } + if (ce->status >= PCE_STATUS_ACQUIRED) { + clk_unprepare(ce->clk); + clk_put(ce->clk); } - kfree(ce->con_id); kfree(ce); } /** * pm_clk_remove - Stop using a device clock for power management. * @dev: Device whose clock should not be used for PM any more. - * @con_id: Connection ID of the clock. + * @clk: Clock pointer * - * Remove the clock represented by @con_id from the list of clocks used for - * the power management of @dev. + * Remove the clock from the list of clocks used for the power + * management of @dev. */ -void pm_clk_remove(struct device *dev, const char *con_id) + +void pm_clk_remove_clk(struct device *dev, struct clk *clk) { struct pm_subsys_data *psd = dev_to_psd(dev); - struct pm_clock_entry *ce; + struct pm_clock_entry *ce, *matching_ce = NULL; if (!psd) return; @@ -166,22 +152,35 @@ void pm_clk_remove(struct device *dev, const char *con_id) spin_lock_irq(&psd->lock); list_for_each_entry(ce, &psd->clock_list, node) { - if (!con_id && !ce->con_id) - goto remove; - else if (!con_id || !ce->con_id) - continue; - else if (!strcmp(con_id, ce->con_id)) - goto remove; + if (ce->clk == clk) { + matching_ce = ce; + list_del(&ce->node); + break; + } } spin_unlock_irq(&psd->lock); - return; - remove: - list_del(&ce->node); - spin_unlock_irq(&psd->lock); + __pm_clk_remove(matching_ce); +} + +/** + * pm_clk_remove - Stop using a device clock for power management. + * @dev: Device whose clock should not be used for PM any more. + * @con_id: Connection ID of the clock. + * + * Remove the clock represented by @con_id from the list of clocks used for + * the power management of @dev. + */ +void pm_clk_remove(struct device *dev, const char *con_id) +{ + struct clk *clk; - __pm_clk_remove(ce); + clk = clk_get(dev, con_id); + if (!IS_ERR(clk)) { + pm_clk_remove_clk(dev, clk); + clk_put(clk); + } } /** @@ -266,10 +265,9 @@ int pm_clk_suspend(struct device *dev) spin_lock_irqsave(&psd->lock, flags); list_for_each_entry_reverse(ce, &psd->clock_list, node) { - if (ce->status < PCE_STATUS_ERROR) { - if (ce->status == PCE_STATUS_ENABLED) - clk_disable(ce->clk); - ce->status = PCE_STATUS_ACQUIRED; + if (ce->status == PCE_STATUS_ENABLED) { + clk_disable(ce->clk); + ce->status = PCE_STATUS_PREPARED; } } @@ -297,11 +295,9 @@ 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; - } + ret = __pm_clk_enable(dev, ce->clk); + if (!ret) + ce->status = PCE_STATUS_ENABLED; } spin_unlock_irqrestore(&psd->lock, flags); @@ -390,10 +386,9 @@ int pm_clk_suspend(struct device *dev) spin_lock_irqsave(&psd->lock, flags); list_for_each_entry_reverse(ce, &psd->clock_list, node) { - if (ce->status < PCE_STATUS_ERROR) { - if (ce->status == PCE_STATUS_ENABLED) - clk_disable(ce->clk); - ce->status = PCE_STATUS_ACQUIRED; + if (ce->status == PCE_STATUS_ENABLED) { + clk_disable(ce->clk); + ce->status = PCE_STATUS_PREPARED; } } @@ -422,11 +417,9 @@ 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; - } + ret = __pm_clk_enable(dev, ce->clk); + if (!ret) + ce->status = PCE_STATUS_ENABLED; } spin_unlock_irqrestore(&psd->lock, flags); -- 2.1.0.rc2.206.gedb03e5 : -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
