* [RESEND PATCH 0/3] clk: Fixup issues for clk_set_parent
@ 2013-03-12 19:20 Ulf Hansson
2013-03-12 19:20 ` [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code Ulf Hansson
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ulf Hansson @ 2013-03-12 19:20 UTC (permalink / raw)
To: linux-arm-kernel
From: Ulf Hansson <ulf.hansson@linaro.org>
Issues with racing for fetching spinlocks is present in the
clk_set_parent API. This patchset is trying to fixup these issues.
In short, while updating the clock tree toplogy the spinlock must be held
to prevent enable_count from being messed up.
Patch 1 and 2 prepares for patch 3, which is where the real issue are
resolved.
Ulf Hansson (3):
clk: Remove _clk_reparent from API and restructure code
clk: Improve errorhandling for clk_set_parent
clk: Fixup locking issues for clk_set_parent
drivers/clk/clk.c | 192 +++++++++++++++++++++++++++---------------
include/linux/clk-provider.h | 1 -
2 files changed, 125 insertions(+), 68 deletions(-)
--
1.7.10
^ permalink raw reply [flat|nested] 9+ messages in thread* [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code 2013-03-12 19:20 [RESEND PATCH 0/3] clk: Fixup issues for clk_set_parent Ulf Hansson @ 2013-03-12 19:20 ` Ulf Hansson 2013-03-19 21:20 ` Mike Turquette 2013-03-12 19:20 ` [RESEND PATCH 2/3] clk: Improve errorhandling for clk_set_parent Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 3/3] clk: Fixup locking issues " Ulf Hansson 2 siblings, 1 reply; 9+ messages in thread From: Ulf Hansson @ 2013-03-12 19:20 UTC (permalink / raw) To: linux-arm-kernel From: Ulf Hansson <ulf.hansson@linaro.org> There shall be no reason for keeping this API available for clock providers. So we remove it from the API and restrcuture the code so for example the COMMON_CLK_DEBUG part is separated. This patch will also make it possible to hold the spinlock over the actual update of the clock tree topology, which could not be done before when both debugfs updates and clock rate updates was done within the same function. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/clk/clk.c | 82 ++++++++++++++++++++++++------------------ include/linux/clk-provider.h | 1 - 2 files changed, 48 insertions(+), 35 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 593a2e4..2e10cc1 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -284,6 +284,39 @@ out: } /** + * clk_debug_reparent - reparent clk node in the debugfs clk tree + * @clk: the clk being reparented + * @new_parent: the new clk parent, may be NULL + * + * Rename clk entry in the debugfs clk tree if debugfs has been + * initialized. Otherwise it bails out early since the debugfs clk tree + * will be created lazily by clk_debug_init as part of a late_initcall. + * + * Caller must hold prepare_lock. + */ +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent) +{ + struct dentry *d; + struct dentry *new_parent_d; + + if (!inited) + return; + + if (new_parent) + new_parent_d = new_parent->dentry; + else + new_parent_d = orphandir; + + d = debugfs_rename(clk->dentry->d_parent, clk->dentry, + new_parent_d, clk->name); + if (d) + clk->dentry = d; + else + pr_debug("%s: failed to rename debugfs entry for %s\n", + __func__, clk->name); +} + +/** * clk_debug_init - lazily create the debugfs clk tree visualization * * clks are often initialized very early during boot before memory can @@ -338,6 +371,9 @@ static int __init clk_debug_init(void) late_initcall(clk_debug_init); #else static inline int clk_debug_register(struct clk *clk) { return 0; } +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent) +{ +} #endif /* caller must hold prepare_lock */ @@ -1179,16 +1215,8 @@ out: return ret; } -void __clk_reparent(struct clk *clk, struct clk *new_parent) +static void clk_reparent(struct clk *clk, struct clk *new_parent) { -#ifdef CONFIG_COMMON_CLK_DEBUG - struct dentry *d; - struct dentry *new_parent_d; -#endif - - if (!clk || !new_parent) - return; - hlist_del(&clk->child_node); if (new_parent) @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) else hlist_add_head(&clk->child_node, &clk_orphan_list); -#ifdef CONFIG_COMMON_CLK_DEBUG - if (!inited) - goto out; - - if (new_parent) - new_parent_d = new_parent->dentry; - else - new_parent_d = orphandir; - - d = debugfs_rename(clk->dentry->d_parent, clk->dentry, - new_parent_d, clk->name); - if (d) - clk->dentry = d; - else - pr_debug("%s: failed to rename debugfs entry for %s\n", - __func__, clk->name); -out: -#endif - clk->parent = new_parent; - - __clk_recalc_rates(clk, POST_RATE_CHANGE); } static int __clk_set_parent(struct clk *clk, struct clk *parent) @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent) } /* propagate rate recalculation downstream */ - __clk_reparent(clk, parent); + clk_reparent(clk, parent); + clk_debug_reparent(clk, parent); + __clk_recalc_rates(clk, POST_RATE_CHANGE); out: mutex_unlock(&prepare_lock); @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk) hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) { if (orphan->ops->get_parent) { i = orphan->ops->get_parent(orphan->hw); - if (!strcmp(clk->name, orphan->parent_names[i])) - __clk_reparent(orphan, clk); + if (!strcmp(clk->name, orphan->parent_names[i])) { + clk_reparent(orphan, clk); + clk_debug_reparent(orphan, clk); + __clk_recalc_rates(orphan, POST_RATE_CHANGE); + } continue; } for (i = 0; i < orphan->num_parents; i++) if (!strcmp(clk->name, orphan->parent_names[i])) { - __clk_reparent(orphan, clk); + clk_reparent(orphan, clk); + clk_debug_reparent(orphan, clk); + __clk_recalc_rates(orphan, POST_RATE_CHANGE); break; } } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 4989b8a..87a7c2c 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name); */ int __clk_prepare(struct clk *clk); void __clk_unprepare(struct clk *clk); -void __clk_reparent(struct clk *clk, struct clk *new_parent); unsigned long __clk_round_rate(struct clk *clk, unsigned long rate); struct of_device_id; -- 1.7.10 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code 2013-03-12 19:20 ` [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code Ulf Hansson @ 2013-03-19 21:20 ` Mike Turquette 2013-03-20 9:45 ` Ulf Hansson 0 siblings, 1 reply; 9+ messages in thread From: Mike Turquette @ 2013-03-19 21:20 UTC (permalink / raw) To: linux-arm-kernel Quoting Ulf Hansson (2013-03-12 12:20:48) > From: Ulf Hansson <ulf.hansson@linaro.org> > > There shall be no reason for keeping this API available for clock > providers. So we remove it from the API and restrcuture the code so > for example the COMMON_CLK_DEBUG part is separated. > Hi Ulf, There is one reason to keep this api. OMAP currently uses it to change the parent of a PLL during .set_rate. This is kind of a legacy mechanism however, and the reentrancy series I posted actually takes care of this one corner case: http://article.gmane.org/gmane.linux.kernel/1448449 Let me see how the OMAP folks feel about taking that patch in, which eliminates the only external user of the API. +Rajendra & Benoit Thanks, Mike > This patch will also make it possible to hold the spinlock over the > actual update of the clock tree topology, which could not be done > before when both debugfs updates and clock rate updates was done > within the same function. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/clk/clk.c | 82 ++++++++++++++++++++++++------------------ > include/linux/clk-provider.h | 1 - > 2 files changed, 48 insertions(+), 35 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 593a2e4..2e10cc1 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -284,6 +284,39 @@ out: > } > > /** > + * clk_debug_reparent - reparent clk node in the debugfs clk tree > + * @clk: the clk being reparented > + * @new_parent: the new clk parent, may be NULL > + * > + * Rename clk entry in the debugfs clk tree if debugfs has been > + * initialized. Otherwise it bails out early since the debugfs clk tree > + * will be created lazily by clk_debug_init as part of a late_initcall. > + * > + * Caller must hold prepare_lock. > + */ > +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent) > +{ > + struct dentry *d; > + struct dentry *new_parent_d; > + > + if (!inited) > + return; > + > + if (new_parent) > + new_parent_d = new_parent->dentry; > + else > + new_parent_d = orphandir; > + > + d = debugfs_rename(clk->dentry->d_parent, clk->dentry, > + new_parent_d, clk->name); > + if (d) > + clk->dentry = d; > + else > + pr_debug("%s: failed to rename debugfs entry for %s\n", > + __func__, clk->name); > +} > + > +/** > * clk_debug_init - lazily create the debugfs clk tree visualization > * > * clks are often initialized very early during boot before memory can > @@ -338,6 +371,9 @@ static int __init clk_debug_init(void) > late_initcall(clk_debug_init); > #else > static inline int clk_debug_register(struct clk *clk) { return 0; } > +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent) > +{ > +} > #endif > > /* caller must hold prepare_lock */ > @@ -1179,16 +1215,8 @@ out: > return ret; > } > > -void __clk_reparent(struct clk *clk, struct clk *new_parent) > +static void clk_reparent(struct clk *clk, struct clk *new_parent) > { > -#ifdef CONFIG_COMMON_CLK_DEBUG > - struct dentry *d; > - struct dentry *new_parent_d; > -#endif > - > - if (!clk || !new_parent) > - return; > - > hlist_del(&clk->child_node); > > if (new_parent) > @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) > else > hlist_add_head(&clk->child_node, &clk_orphan_list); > > -#ifdef CONFIG_COMMON_CLK_DEBUG > - if (!inited) > - goto out; > - > - if (new_parent) > - new_parent_d = new_parent->dentry; > - else > - new_parent_d = orphandir; > - > - d = debugfs_rename(clk->dentry->d_parent, clk->dentry, > - new_parent_d, clk->name); > - if (d) > - clk->dentry = d; > - else > - pr_debug("%s: failed to rename debugfs entry for %s\n", > - __func__, clk->name); > -out: > -#endif > - > clk->parent = new_parent; > - > - __clk_recalc_rates(clk, POST_RATE_CHANGE); > } > > static int __clk_set_parent(struct clk *clk, struct clk *parent) > @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > } > > /* propagate rate recalculation downstream */ > - __clk_reparent(clk, parent); > + clk_reparent(clk, parent); > + clk_debug_reparent(clk, parent); > + __clk_recalc_rates(clk, POST_RATE_CHANGE); > > out: > mutex_unlock(&prepare_lock); > @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk) > hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) { > if (orphan->ops->get_parent) { > i = orphan->ops->get_parent(orphan->hw); > - if (!strcmp(clk->name, orphan->parent_names[i])) > - __clk_reparent(orphan, clk); > + if (!strcmp(clk->name, orphan->parent_names[i])) { > + clk_reparent(orphan, clk); > + clk_debug_reparent(orphan, clk); > + __clk_recalc_rates(orphan, POST_RATE_CHANGE); > + } > continue; > } > > for (i = 0; i < orphan->num_parents; i++) > if (!strcmp(clk->name, orphan->parent_names[i])) { > - __clk_reparent(orphan, clk); > + clk_reparent(orphan, clk); > + clk_debug_reparent(orphan, clk); > + __clk_recalc_rates(orphan, POST_RATE_CHANGE); > break; > } > } > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 4989b8a..87a7c2c 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name); > */ > int __clk_prepare(struct clk *clk); > void __clk_unprepare(struct clk *clk); > -void __clk_reparent(struct clk *clk, struct clk *new_parent); > unsigned long __clk_round_rate(struct clk *clk, unsigned long rate); > > struct of_device_id; > -- > 1.7.10 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code 2013-03-19 21:20 ` Mike Turquette @ 2013-03-20 9:45 ` Ulf Hansson 2013-03-20 10:13 ` Rajendra Nayak 0 siblings, 1 reply; 9+ messages in thread From: Ulf Hansson @ 2013-03-20 9:45 UTC (permalink / raw) To: linux-arm-kernel On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote: > Quoting Ulf Hansson (2013-03-12 12:20:48) >> From: Ulf Hansson <ulf.hansson@linaro.org> >> >> There shall be no reason for keeping this API available for clock >> providers. So we remove it from the API and restrcuture the code so >> for example the COMMON_CLK_DEBUG part is separated. >> > > Hi Ulf, > > There is one reason to keep this api. OMAP currently uses it to change > the parent of a PLL during .set_rate. This is kind of a legacy > mechanism however, and the reentrancy series I posted actually takes > care of this one corner case: > http://article.gmane.org/gmane.linux.kernel/1448449 Hi Mike, It was a while ago I created this patch, so I guess this has beed merged without me noticing, sorry. > > Let me see how the OMAP folks feel about taking that patch in, which > eliminates the only external user of the API. +Rajendra & Benoit > I have no problem rebasing the patch and keep the API, if that is the easiest way forward. Although, I guess in the end you want to remove these kind of internal clk API functions. So, if we get ack from Rajendra & Benoit, it is better to remove the API right? Kind regards Ulf Hansson > Thanks, > Mike > >> This patch will also make it possible to hold the spinlock over the >> actual update of the clock tree topology, which could not be done >> before when both debugfs updates and clock rate updates was done >> within the same function. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/clk/clk.c | 82 ++++++++++++++++++++++++------------------ >> include/linux/clk-provider.h | 1 - >> 2 files changed, 48 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 593a2e4..2e10cc1 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -284,6 +284,39 @@ out: >> } >> >> /** >> + * clk_debug_reparent - reparent clk node in the debugfs clk tree >> + * @clk: the clk being reparented >> + * @new_parent: the new clk parent, may be NULL >> + * >> + * Rename clk entry in the debugfs clk tree if debugfs has been >> + * initialized. Otherwise it bails out early since the debugfs clk tree >> + * will be created lazily by clk_debug_init as part of a late_initcall. >> + * >> + * Caller must hold prepare_lock. >> + */ >> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent) >> +{ >> + struct dentry *d; >> + struct dentry *new_parent_d; >> + >> + if (!inited) >> + return; >> + >> + if (new_parent) >> + new_parent_d = new_parent->dentry; >> + else >> + new_parent_d = orphandir; >> + >> + d = debugfs_rename(clk->dentry->d_parent, clk->dentry, >> + new_parent_d, clk->name); >> + if (d) >> + clk->dentry = d; >> + else >> + pr_debug("%s: failed to rename debugfs entry for %s\n", >> + __func__, clk->name); >> +} >> + >> +/** >> * clk_debug_init - lazily create the debugfs clk tree visualization >> * >> * clks are often initialized very early during boot before memory can >> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void) >> late_initcall(clk_debug_init); >> #else >> static inline int clk_debug_register(struct clk *clk) { return 0; } >> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent) >> +{ >> +} >> #endif >> >> /* caller must hold prepare_lock */ >> @@ -1179,16 +1215,8 @@ out: >> return ret; >> } >> >> -void __clk_reparent(struct clk *clk, struct clk *new_parent) >> +static void clk_reparent(struct clk *clk, struct clk *new_parent) >> { >> -#ifdef CONFIG_COMMON_CLK_DEBUG >> - struct dentry *d; >> - struct dentry *new_parent_d; >> -#endif >> - >> - if (!clk || !new_parent) >> - return; >> - >> hlist_del(&clk->child_node); >> >> if (new_parent) >> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) >> else >> hlist_add_head(&clk->child_node, &clk_orphan_list); >> >> -#ifdef CONFIG_COMMON_CLK_DEBUG >> - if (!inited) >> - goto out; >> - >> - if (new_parent) >> - new_parent_d = new_parent->dentry; >> - else >> - new_parent_d = orphandir; >> - >> - d = debugfs_rename(clk->dentry->d_parent, clk->dentry, >> - new_parent_d, clk->name); >> - if (d) >> - clk->dentry = d; >> - else >> - pr_debug("%s: failed to rename debugfs entry for %s\n", >> - __func__, clk->name); >> -out: >> -#endif >> - >> clk->parent = new_parent; >> - >> - __clk_recalc_rates(clk, POST_RATE_CHANGE); >> } >> >> static int __clk_set_parent(struct clk *clk, struct clk *parent) >> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent) >> } >> >> /* propagate rate recalculation downstream */ >> - __clk_reparent(clk, parent); >> + clk_reparent(clk, parent); >> + clk_debug_reparent(clk, parent); >> + __clk_recalc_rates(clk, POST_RATE_CHANGE); >> >> out: >> mutex_unlock(&prepare_lock); >> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk) >> hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) { >> if (orphan->ops->get_parent) { >> i = orphan->ops->get_parent(orphan->hw); >> - if (!strcmp(clk->name, orphan->parent_names[i])) >> - __clk_reparent(orphan, clk); >> + if (!strcmp(clk->name, orphan->parent_names[i])) { >> + clk_reparent(orphan, clk); >> + clk_debug_reparent(orphan, clk); >> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); >> + } >> continue; >> } >> >> for (i = 0; i < orphan->num_parents; i++) >> if (!strcmp(clk->name, orphan->parent_names[i])) { >> - __clk_reparent(orphan, clk); >> + clk_reparent(orphan, clk); >> + clk_debug_reparent(orphan, clk); >> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); >> break; >> } >> } >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> index 4989b8a..87a7c2c 100644 >> --- a/include/linux/clk-provider.h >> +++ b/include/linux/clk-provider.h >> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name); >> */ >> int __clk_prepare(struct clk *clk); >> void __clk_unprepare(struct clk *clk); >> -void __clk_reparent(struct clk *clk, struct clk *new_parent); >> unsigned long __clk_round_rate(struct clk *clk, unsigned long rate); >> >> struct of_device_id; >> -- >> 1.7.10 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code 2013-03-20 9:45 ` Ulf Hansson @ 2013-03-20 10:13 ` Rajendra Nayak 2013-03-20 15:03 ` Mike Turquette 0 siblings, 1 reply; 9+ messages in thread From: Rajendra Nayak @ 2013-03-20 10:13 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 20 March 2013 03:15 PM, Ulf Hansson wrote: > On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote: >> Quoting Ulf Hansson (2013-03-12 12:20:48) >>> From: Ulf Hansson <ulf.hansson@linaro.org> >>> >>> There shall be no reason for keeping this API available for clock >>> providers. So we remove it from the API and restrcuture the code so >>> for example the COMMON_CLK_DEBUG part is separated. >>> >> >> Hi Ulf, >> >> There is one reason to keep this api. OMAP currently uses it to change >> the parent of a PLL during .set_rate. This is kind of a legacy >> mechanism however, and the reentrancy series I posted actually takes >> care of this one corner case: >> http://article.gmane.org/gmane.linux.kernel/1448449 > > Hi Mike, > > It was a while ago I created this patch, so I guess this has beed > merged without me noticing, sorry. > >> >> Let me see how the OMAP folks feel about taking that patch in, which >> eliminates the only external user of the API. +Rajendra & Benoit >> > > I have no problem rebasing the patch and keep the API, if that is the > easiest way forward. Ulf, It would be good if you could keep the api for now. I seemed to have missed out on Mikes patch getting rid of the the api from OMAP PLL .set_rate. I will take a stab at that to get rid of that api along with the other OMAP only things that exist today as part of Common clk. regards, Rajendra > Although, I guess in the end you want to remove these kind of internal > clk API functions. So, if we get ack from Rajendra & Benoit, it is > better to remove the API right? > > Kind regards > Ulf Hansson > >> Thanks, >> Mike >> >>> This patch will also make it possible to hold the spinlock over the >>> actual update of the clock tree topology, which could not be done >>> before when both debugfs updates and clock rate updates was done >>> within the same function. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> drivers/clk/clk.c | 82 ++++++++++++++++++++++++------------------ >>> include/linux/clk-provider.h | 1 - >>> 2 files changed, 48 insertions(+), 35 deletions(-) >>> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>> index 593a2e4..2e10cc1 100644 >>> --- a/drivers/clk/clk.c >>> +++ b/drivers/clk/clk.c >>> @@ -284,6 +284,39 @@ out: >>> } >>> >>> /** >>> + * clk_debug_reparent - reparent clk node in the debugfs clk tree >>> + * @clk: the clk being reparented >>> + * @new_parent: the new clk parent, may be NULL >>> + * >>> + * Rename clk entry in the debugfs clk tree if debugfs has been >>> + * initialized. Otherwise it bails out early since the debugfs clk tree >>> + * will be created lazily by clk_debug_init as part of a late_initcall. >>> + * >>> + * Caller must hold prepare_lock. >>> + */ >>> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent) >>> +{ >>> + struct dentry *d; >>> + struct dentry *new_parent_d; >>> + >>> + if (!inited) >>> + return; >>> + >>> + if (new_parent) >>> + new_parent_d = new_parent->dentry; >>> + else >>> + new_parent_d = orphandir; >>> + >>> + d = debugfs_rename(clk->dentry->d_parent, clk->dentry, >>> + new_parent_d, clk->name); >>> + if (d) >>> + clk->dentry = d; >>> + else >>> + pr_debug("%s: failed to rename debugfs entry for %s\n", >>> + __func__, clk->name); >>> +} >>> + >>> +/** >>> * clk_debug_init - lazily create the debugfs clk tree visualization >>> * >>> * clks are often initialized very early during boot before memory can >>> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void) >>> late_initcall(clk_debug_init); >>> #else >>> static inline int clk_debug_register(struct clk *clk) { return 0; } >>> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent) >>> +{ >>> +} >>> #endif >>> >>> /* caller must hold prepare_lock */ >>> @@ -1179,16 +1215,8 @@ out: >>> return ret; >>> } >>> >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent) >>> +static void clk_reparent(struct clk *clk, struct clk *new_parent) >>> { >>> -#ifdef CONFIG_COMMON_CLK_DEBUG >>> - struct dentry *d; >>> - struct dentry *new_parent_d; >>> -#endif >>> - >>> - if (!clk || !new_parent) >>> - return; >>> - >>> hlist_del(&clk->child_node); >>> >>> if (new_parent) >>> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) >>> else >>> hlist_add_head(&clk->child_node, &clk_orphan_list); >>> >>> -#ifdef CONFIG_COMMON_CLK_DEBUG >>> - if (!inited) >>> - goto out; >>> - >>> - if (new_parent) >>> - new_parent_d = new_parent->dentry; >>> - else >>> - new_parent_d = orphandir; >>> - >>> - d = debugfs_rename(clk->dentry->d_parent, clk->dentry, >>> - new_parent_d, clk->name); >>> - if (d) >>> - clk->dentry = d; >>> - else >>> - pr_debug("%s: failed to rename debugfs entry for %s\n", >>> - __func__, clk->name); >>> -out: >>> -#endif >>> - >>> clk->parent = new_parent; >>> - >>> - __clk_recalc_rates(clk, POST_RATE_CHANGE); >>> } >>> >>> static int __clk_set_parent(struct clk *clk, struct clk *parent) >>> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent) >>> } >>> >>> /* propagate rate recalculation downstream */ >>> - __clk_reparent(clk, parent); >>> + clk_reparent(clk, parent); >>> + clk_debug_reparent(clk, parent); >>> + __clk_recalc_rates(clk, POST_RATE_CHANGE); >>> >>> out: >>> mutex_unlock(&prepare_lock); >>> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk) >>> hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) { >>> if (orphan->ops->get_parent) { >>> i = orphan->ops->get_parent(orphan->hw); >>> - if (!strcmp(clk->name, orphan->parent_names[i])) >>> - __clk_reparent(orphan, clk); >>> + if (!strcmp(clk->name, orphan->parent_names[i])) { >>> + clk_reparent(orphan, clk); >>> + clk_debug_reparent(orphan, clk); >>> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); >>> + } >>> continue; >>> } >>> >>> for (i = 0; i < orphan->num_parents; i++) >>> if (!strcmp(clk->name, orphan->parent_names[i])) { >>> - __clk_reparent(orphan, clk); >>> + clk_reparent(orphan, clk); >>> + clk_debug_reparent(orphan, clk); >>> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); >>> break; >>> } >>> } >>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >>> index 4989b8a..87a7c2c 100644 >>> --- a/include/linux/clk-provider.h >>> +++ b/include/linux/clk-provider.h >>> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name); >>> */ >>> int __clk_prepare(struct clk *clk); >>> void __clk_unprepare(struct clk *clk); >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent); >>> unsigned long __clk_round_rate(struct clk *clk, unsigned long rate); >>> >>> struct of_device_id; >>> -- >>> 1.7.10 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code 2013-03-20 10:13 ` Rajendra Nayak @ 2013-03-20 15:03 ` Mike Turquette 2013-03-20 20:39 ` Ulf Hansson 0 siblings, 1 reply; 9+ messages in thread From: Mike Turquette @ 2013-03-20 15:03 UTC (permalink / raw) To: linux-arm-kernel Quoting Rajendra Nayak (2013-03-20 03:13:36) > On Wednesday 20 March 2013 03:15 PM, Ulf Hansson wrote: > > On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote: > >> Quoting Ulf Hansson (2013-03-12 12:20:48) > >>> From: Ulf Hansson <ulf.hansson@linaro.org> > >>> > >>> There shall be no reason for keeping this API available for clock > >>> providers. So we remove it from the API and restrcuture the code so > >>> for example the COMMON_CLK_DEBUG part is separated. > >>> > >> > >> Hi Ulf, > >> > >> There is one reason to keep this api. OMAP currently uses it to change > >> the parent of a PLL during .set_rate. This is kind of a legacy > >> mechanism however, and the reentrancy series I posted actually takes > >> care of this one corner case: > >> http://article.gmane.org/gmane.linux.kernel/1448449 > > > > Hi Mike, > > > > It was a while ago I created this patch, so I guess this has beed > > merged without me noticing, sorry. > > > >> > >> Let me see how the OMAP folks feel about taking that patch in, which > >> eliminates the only external user of the API. +Rajendra & Benoit > >> > > > > I have no problem rebasing the patch and keep the API, if that is the > > easiest way forward. > > Ulf, It would be good if you could keep the api for now. I seemed to have > missed out on Mikes patch getting rid of the the api from OMAP PLL .set_rate. > I will take a stab at that to get rid of that api along with the other OMAP > only things that exist today as part of Common clk. > Rajendra, Yeah, the patch I linked to above was actually just "example code" showing how to reentrantly call clk_set_parent from within a .set_rate callback. It's no suprise you missed it since the patch $SUBJECT starts with "HACK:" ;-) The trick is to recognize that OMAP's PLLs are mux clocks and have them properly support clk_set_parent. This would have made my life easier in the bad days of DPLL cascading back in 2011. > regards, > Rajendra > > > Although, I guess in the end you want to remove these kind of internal > > clk API functions. So, if we get ack from Rajendra & Benoit, it is > > better to remove the API right? > > Ulf, Correct, as we discussed at LCA I do want to get rid of all of these internal accessor functions. Thanks, Mike > > Kind regards > > Ulf Hansson > > > >> Thanks, > >> Mike > >> > >>> This patch will also make it possible to hold the spinlock over the > >>> actual update of the clock tree topology, which could not be done > >>> before when both debugfs updates and clock rate updates was done > >>> within the same function. > >>> > >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >>> --- > >>> drivers/clk/clk.c | 82 ++++++++++++++++++++++++------------------ > >>> include/linux/clk-provider.h | 1 - > >>> 2 files changed, 48 insertions(+), 35 deletions(-) > >>> > >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > >>> index 593a2e4..2e10cc1 100644 > >>> --- a/drivers/clk/clk.c > >>> +++ b/drivers/clk/clk.c > >>> @@ -284,6 +284,39 @@ out: > >>> } > >>> > >>> /** > >>> + * clk_debug_reparent - reparent clk node in the debugfs clk tree > >>> + * @clk: the clk being reparented > >>> + * @new_parent: the new clk parent, may be NULL > >>> + * > >>> + * Rename clk entry in the debugfs clk tree if debugfs has been > >>> + * initialized. Otherwise it bails out early since the debugfs clk tree > >>> + * will be created lazily by clk_debug_init as part of a late_initcall. > >>> + * > >>> + * Caller must hold prepare_lock. > >>> + */ > >>> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent) > >>> +{ > >>> + struct dentry *d; > >>> + struct dentry *new_parent_d; > >>> + > >>> + if (!inited) > >>> + return; > >>> + > >>> + if (new_parent) > >>> + new_parent_d = new_parent->dentry; > >>> + else > >>> + new_parent_d = orphandir; > >>> + > >>> + d = debugfs_rename(clk->dentry->d_parent, clk->dentry, > >>> + new_parent_d, clk->name); > >>> + if (d) > >>> + clk->dentry = d; > >>> + else > >>> + pr_debug("%s: failed to rename debugfs entry for %s\n", > >>> + __func__, clk->name); > >>> +} > >>> + > >>> +/** > >>> * clk_debug_init - lazily create the debugfs clk tree visualization > >>> * > >>> * clks are often initialized very early during boot before memory can > >>> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void) > >>> late_initcall(clk_debug_init); > >>> #else > >>> static inline int clk_debug_register(struct clk *clk) { return 0; } > >>> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent) > >>> +{ > >>> +} > >>> #endif > >>> > >>> /* caller must hold prepare_lock */ > >>> @@ -1179,16 +1215,8 @@ out: > >>> return ret; > >>> } > >>> > >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent) > >>> +static void clk_reparent(struct clk *clk, struct clk *new_parent) > >>> { > >>> -#ifdef CONFIG_COMMON_CLK_DEBUG > >>> - struct dentry *d; > >>> - struct dentry *new_parent_d; > >>> -#endif > >>> - > >>> - if (!clk || !new_parent) > >>> - return; > >>> - > >>> hlist_del(&clk->child_node); > >>> > >>> if (new_parent) > >>> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) > >>> else > >>> hlist_add_head(&clk->child_node, &clk_orphan_list); > >>> > >>> -#ifdef CONFIG_COMMON_CLK_DEBUG > >>> - if (!inited) > >>> - goto out; > >>> - > >>> - if (new_parent) > >>> - new_parent_d = new_parent->dentry; > >>> - else > >>> - new_parent_d = orphandir; > >>> - > >>> - d = debugfs_rename(clk->dentry->d_parent, clk->dentry, > >>> - new_parent_d, clk->name); > >>> - if (d) > >>> - clk->dentry = d; > >>> - else > >>> - pr_debug("%s: failed to rename debugfs entry for %s\n", > >>> - __func__, clk->name); > >>> -out: > >>> -#endif > >>> - > >>> clk->parent = new_parent; > >>> - > >>> - __clk_recalc_rates(clk, POST_RATE_CHANGE); > >>> } > >>> > >>> static int __clk_set_parent(struct clk *clk, struct clk *parent) > >>> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > >>> } > >>> > >>> /* propagate rate recalculation downstream */ > >>> - __clk_reparent(clk, parent); > >>> + clk_reparent(clk, parent); > >>> + clk_debug_reparent(clk, parent); > >>> + __clk_recalc_rates(clk, POST_RATE_CHANGE); > >>> > >>> out: > >>> mutex_unlock(&prepare_lock); > >>> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk) > >>> hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) { > >>> if (orphan->ops->get_parent) { > >>> i = orphan->ops->get_parent(orphan->hw); > >>> - if (!strcmp(clk->name, orphan->parent_names[i])) > >>> - __clk_reparent(orphan, clk); > >>> + if (!strcmp(clk->name, orphan->parent_names[i])) { > >>> + clk_reparent(orphan, clk); > >>> + clk_debug_reparent(orphan, clk); > >>> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); > >>> + } > >>> continue; > >>> } > >>> > >>> for (i = 0; i < orphan->num_parents; i++) > >>> if (!strcmp(clk->name, orphan->parent_names[i])) { > >>> - __clk_reparent(orphan, clk); > >>> + clk_reparent(orphan, clk); > >>> + clk_debug_reparent(orphan, clk); > >>> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); > >>> break; > >>> } > >>> } > >>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > >>> index 4989b8a..87a7c2c 100644 > >>> --- a/include/linux/clk-provider.h > >>> +++ b/include/linux/clk-provider.h > >>> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name); > >>> */ > >>> int __clk_prepare(struct clk *clk); > >>> void __clk_unprepare(struct clk *clk); > >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent); > >>> unsigned long __clk_round_rate(struct clk *clk, unsigned long rate); > >>> > >>> struct of_device_id; > >>> -- > >>> 1.7.10 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code 2013-03-20 15:03 ` Mike Turquette @ 2013-03-20 20:39 ` Ulf Hansson 0 siblings, 0 replies; 9+ messages in thread From: Ulf Hansson @ 2013-03-20 20:39 UTC (permalink / raw) To: linux-arm-kernel On 20 March 2013 16:03, Mike Turquette <mturquette@linaro.org> wrote: > Quoting Rajendra Nayak (2013-03-20 03:13:36) >> On Wednesday 20 March 2013 03:15 PM, Ulf Hansson wrote: >> > On 19 March 2013 22:20, Mike Turquette <mturquette@linaro.org> wrote: >> >> Quoting Ulf Hansson (2013-03-12 12:20:48) >> >>> From: Ulf Hansson <ulf.hansson@linaro.org> >> >>> >> >>> There shall be no reason for keeping this API available for clock >> >>> providers. So we remove it from the API and restrcuture the code so >> >>> for example the COMMON_CLK_DEBUG part is separated. >> >>> >> >> >> >> Hi Ulf, >> >> >> >> There is one reason to keep this api. OMAP currently uses it to change >> >> the parent of a PLL during .set_rate. This is kind of a legacy >> >> mechanism however, and the reentrancy series I posted actually takes >> >> care of this one corner case: >> >> http://article.gmane.org/gmane.linux.kernel/1448449 >> > >> > Hi Mike, >> > >> > It was a while ago I created this patch, so I guess this has beed >> > merged without me noticing, sorry. >> > >> >> >> >> Let me see how the OMAP folks feel about taking that patch in, which >> >> eliminates the only external user of the API. +Rajendra & Benoit >> >> >> > >> > I have no problem rebasing the patch and keep the API, if that is the >> > easiest way forward. >> >> Ulf, It would be good if you could keep the api for now. I seemed to have >> missed out on Mikes patch getting rid of the the api from OMAP PLL .set_rate. >> I will take a stab at that to get rid of that api along with the other OMAP >> only things that exist today as part of Common clk. >> > > Rajendra, > > Yeah, the patch I linked to above was actually just "example code" > showing how to reentrantly call clk_set_parent from within a .set_rate > callback. It's no suprise you missed it since the patch $SUBJECT starts > with "HACK:" ;-) > > The trick is to recognize that OMAP's PLLs are mux clocks and have them > properly support clk_set_parent. This would have made my life easier in > the bad days of DPLL cascading back in 2011. > >> regards, >> Rajendra >> >> > Although, I guess in the end you want to remove these kind of internal >> > clk API functions. So, if we get ack from Rajendra & Benoit, it is >> > better to remove the API right? >> > > > Ulf, > > Correct, as we discussed at LCA I do want to get rid of all of these > internal accessor functions. > > Thanks, > Mike > Thanks for your responses. I will send a new version which keeps the API for now. Kind regards Uffe >> > Kind regards >> > Ulf Hansson >> > >> >> Thanks, >> >> Mike >> >> >> >>> This patch will also make it possible to hold the spinlock over the >> >>> actual update of the clock tree topology, which could not be done >> >>> before when both debugfs updates and clock rate updates was done >> >>> within the same function. >> >>> >> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> >>> --- >> >>> drivers/clk/clk.c | 82 ++++++++++++++++++++++++------------------ >> >>> include/linux/clk-provider.h | 1 - >> >>> 2 files changed, 48 insertions(+), 35 deletions(-) >> >>> >> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> >>> index 593a2e4..2e10cc1 100644 >> >>> --- a/drivers/clk/clk.c >> >>> +++ b/drivers/clk/clk.c >> >>> @@ -284,6 +284,39 @@ out: >> >>> } >> >>> >> >>> /** >> >>> + * clk_debug_reparent - reparent clk node in the debugfs clk tree >> >>> + * @clk: the clk being reparented >> >>> + * @new_parent: the new clk parent, may be NULL >> >>> + * >> >>> + * Rename clk entry in the debugfs clk tree if debugfs has been >> >>> + * initialized. Otherwise it bails out early since the debugfs clk tree >> >>> + * will be created lazily by clk_debug_init as part of a late_initcall. >> >>> + * >> >>> + * Caller must hold prepare_lock. >> >>> + */ >> >>> +static void clk_debug_reparent(struct clk *clk, struct clk *new_parent) >> >>> +{ >> >>> + struct dentry *d; >> >>> + struct dentry *new_parent_d; >> >>> + >> >>> + if (!inited) >> >>> + return; >> >>> + >> >>> + if (new_parent) >> >>> + new_parent_d = new_parent->dentry; >> >>> + else >> >>> + new_parent_d = orphandir; >> >>> + >> >>> + d = debugfs_rename(clk->dentry->d_parent, clk->dentry, >> >>> + new_parent_d, clk->name); >> >>> + if (d) >> >>> + clk->dentry = d; >> >>> + else >> >>> + pr_debug("%s: failed to rename debugfs entry for %s\n", >> >>> + __func__, clk->name); >> >>> +} >> >>> + >> >>> +/** >> >>> * clk_debug_init - lazily create the debugfs clk tree visualization >> >>> * >> >>> * clks are often initialized very early during boot before memory can >> >>> @@ -338,6 +371,9 @@ static int __init clk_debug_init(void) >> >>> late_initcall(clk_debug_init); >> >>> #else >> >>> static inline int clk_debug_register(struct clk *clk) { return 0; } >> >>> +static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent) >> >>> +{ >> >>> +} >> >>> #endif >> >>> >> >>> /* caller must hold prepare_lock */ >> >>> @@ -1179,16 +1215,8 @@ out: >> >>> return ret; >> >>> } >> >>> >> >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent) >> >>> +static void clk_reparent(struct clk *clk, struct clk *new_parent) >> >>> { >> >>> -#ifdef CONFIG_COMMON_CLK_DEBUG >> >>> - struct dentry *d; >> >>> - struct dentry *new_parent_d; >> >>> -#endif >> >>> - >> >>> - if (!clk || !new_parent) >> >>> - return; >> >>> - >> >>> hlist_del(&clk->child_node); >> >>> >> >>> if (new_parent) >> >>> @@ -1196,28 +1224,7 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent) >> >>> else >> >>> hlist_add_head(&clk->child_node, &clk_orphan_list); >> >>> >> >>> -#ifdef CONFIG_COMMON_CLK_DEBUG >> >>> - if (!inited) >> >>> - goto out; >> >>> - >> >>> - if (new_parent) >> >>> - new_parent_d = new_parent->dentry; >> >>> - else >> >>> - new_parent_d = orphandir; >> >>> - >> >>> - d = debugfs_rename(clk->dentry->d_parent, clk->dentry, >> >>> - new_parent_d, clk->name); >> >>> - if (d) >> >>> - clk->dentry = d; >> >>> - else >> >>> - pr_debug("%s: failed to rename debugfs entry for %s\n", >> >>> - __func__, clk->name); >> >>> -out: >> >>> -#endif >> >>> - >> >>> clk->parent = new_parent; >> >>> - >> >>> - __clk_recalc_rates(clk, POST_RATE_CHANGE); >> >>> } >> >>> >> >>> static int __clk_set_parent(struct clk *clk, struct clk *parent) >> >>> @@ -1329,7 +1336,9 @@ int clk_set_parent(struct clk *clk, struct clk *parent) >> >>> } >> >>> >> >>> /* propagate rate recalculation downstream */ >> >>> - __clk_reparent(clk, parent); >> >>> + clk_reparent(clk, parent); >> >>> + clk_debug_reparent(clk, parent); >> >>> + __clk_recalc_rates(clk, POST_RATE_CHANGE); >> >>> >> >>> out: >> >>> mutex_unlock(&prepare_lock); >> >>> @@ -1453,14 +1462,19 @@ int __clk_init(struct device *dev, struct clk *clk) >> >>> hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node) { >> >>> if (orphan->ops->get_parent) { >> >>> i = orphan->ops->get_parent(orphan->hw); >> >>> - if (!strcmp(clk->name, orphan->parent_names[i])) >> >>> - __clk_reparent(orphan, clk); >> >>> + if (!strcmp(clk->name, orphan->parent_names[i])) { >> >>> + clk_reparent(orphan, clk); >> >>> + clk_debug_reparent(orphan, clk); >> >>> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); >> >>> + } >> >>> continue; >> >>> } >> >>> >> >>> for (i = 0; i < orphan->num_parents; i++) >> >>> if (!strcmp(clk->name, orphan->parent_names[i])) { >> >>> - __clk_reparent(orphan, clk); >> >>> + clk_reparent(orphan, clk); >> >>> + clk_debug_reparent(orphan, clk); >> >>> + __clk_recalc_rates(orphan, POST_RATE_CHANGE); >> >>> break; >> >>> } >> >>> } >> >>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> >>> index 4989b8a..87a7c2c 100644 >> >>> --- a/include/linux/clk-provider.h >> >>> +++ b/include/linux/clk-provider.h >> >>> @@ -359,7 +359,6 @@ struct clk *__clk_lookup(const char *name); >> >>> */ >> >>> int __clk_prepare(struct clk *clk); >> >>> void __clk_unprepare(struct clk *clk); >> >>> -void __clk_reparent(struct clk *clk, struct clk *new_parent); >> >>> unsigned long __clk_round_rate(struct clk *clk, unsigned long rate); >> >>> >> >>> struct of_device_id; >> >>> -- >> >>> 1.7.10 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [RESEND PATCH 2/3] clk: Improve errorhandling for clk_set_parent 2013-03-12 19:20 [RESEND PATCH 0/3] clk: Fixup issues for clk_set_parent Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code Ulf Hansson @ 2013-03-12 19:20 ` Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 3/3] clk: Fixup locking issues " Ulf Hansson 2 siblings, 0 replies; 9+ messages in thread From: Ulf Hansson @ 2013-03-12 19:20 UTC (permalink / raw) To: linux-arm-kernel From: Ulf Hansson <ulf.hansson@linaro.org> By verifying all the needed static information before doing the clk notifications, we minimize number of unwanted rollback notifications with ABORT_RATE_CHANGE message to occur. Additionally make sure the parent are valid pointer before using it. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/clk/clk.c | 48 +++++++++++++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 2e10cc1..a61cf6d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1227,15 +1227,10 @@ static void clk_reparent(struct clk *clk, struct clk *new_parent) clk->parent = new_parent; } -static int __clk_set_parent(struct clk *clk, struct clk *parent) +static u8 clk_fetch_parent_index(struct clk *clk, struct clk *parent) { - struct clk *old_parent; - unsigned long flags; - int ret = -EINVAL; u8 i; - old_parent = clk->parent; - if (!clk->parents) clk->parents = kzalloc((sizeof(struct clk*) * clk->num_parents), GFP_KERNEL); @@ -1255,11 +1250,14 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) } } - if (i == clk->num_parents) { - pr_debug("%s: clock %s is not a possible parent of clock %s\n", - __func__, parent->name, clk->name); - goto out; - } + return i; +} + +static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index) +{ + unsigned long flags; + int ret; + struct clk *old_parent = clk->parent; /* migrate prepare and enable */ if (clk->prepare_count) @@ -1272,7 +1270,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) spin_unlock_irqrestore(&enable_lock, flags); /* change clock input source */ - ret = clk->ops->set_parent(clk->hw, i); + ret = clk->ops->set_parent(clk->hw, p_index); /* clean up old prepare and enable */ spin_lock_irqsave(&enable_lock, flags); @@ -1283,7 +1281,6 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent) if (clk->prepare_count) __clk_unprepare(old_parent); -out: return ret; } @@ -1302,8 +1299,9 @@ out: int clk_set_parent(struct clk *clk, struct clk *parent) { int ret = 0; + u8 p_index; - if (!clk || !clk->ops) + if (!clk || !clk->ops || !parent) return -EINVAL; if (!clk->ops->set_parent) @@ -1315,6 +1313,21 @@ int clk_set_parent(struct clk *clk, struct clk *parent) if (clk->parent == parent) goto out; + /* check that we are allowed to re-parent if the clock is in use */ + if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) { + ret = -EBUSY; + goto out; + } + + /* try finding the new parent index */ + p_index = clk_fetch_parent_index(clk, parent); + if (p_index == clk->num_parents) { + pr_debug("%s: clock %s is not a possible parent of clock %s\n", + __func__, parent->name, clk->name); + ret = -EINVAL; + goto out; + } + /* propagate PRE_RATE_CHANGE notifications */ if (clk->notifier_count) ret = __clk_speculate_rates(clk, parent->rate); @@ -1323,11 +1336,8 @@ int clk_set_parent(struct clk *clk, struct clk *parent) if (ret == NOTIFY_STOP) goto out; - /* only re-parent if the clock is not in use */ - if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count) - ret = -EBUSY; - else - ret = __clk_set_parent(clk, parent); + /* do the re-parent */ + ret = __clk_set_parent(clk, parent, p_index); /* propagate ABORT_RATE_CHANGE if .set_parent failed */ if (ret) { -- 1.7.10 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RESEND PATCH 3/3] clk: Fixup locking issues for clk_set_parent 2013-03-12 19:20 [RESEND PATCH 0/3] clk: Fixup issues for clk_set_parent Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 2/3] clk: Improve errorhandling for clk_set_parent Ulf Hansson @ 2013-03-12 19:20 ` Ulf Hansson 2 siblings, 0 replies; 9+ messages in thread From: Ulf Hansson @ 2013-03-12 19:20 UTC (permalink / raw) To: linux-arm-kernel From: Ulf Hansson <ulf.hansson@linaro.org> Updating the clock tree topology must be protected with the spinlock when doing clk_set_parent, otherwise we can not handle the migration of the enable_count in a safe manner. While issuing the .set_parent callback to make the clk-hw perform the switch to the new parent, we can not hold the spinlock since it is must be allowed to be slow path. This complicates error handling, but is still possible to achieve. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/clk/clk.c | 68 +++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 51 insertions(+), 17 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index a61cf6d..0c19420 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1258,30 +1258,69 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent, u8 p_index) unsigned long flags; int ret; struct clk *old_parent = clk->parent; + bool migrated_enable = false; - /* migrate prepare and enable */ + /* migrate prepare */ if (clk->prepare_count) __clk_prepare(parent); - /* FIXME replace with clk_is_enabled(clk) someday */ spin_lock_irqsave(&enable_lock, flags); - if (clk->enable_count) + + /* migrate enable */ + if (clk->enable_count) { __clk_enable(parent); + migrated_enable = true; + } + + /* update the clk tree topology */ + clk_reparent(clk, parent); + spin_unlock_irqrestore(&enable_lock, flags); /* change clock input source */ ret = clk->ops->set_parent(clk->hw, p_index); + if (ret) { + /* + * The error handling is tricky due to that we need to release + * the spinlock while issuing the .set_parent callback. This + * means the new parent might have been enabled/disabled in + * between, which must be considered when doing rollback. + */ + spin_lock_irqsave(&enable_lock, flags); - /* clean up old prepare and enable */ - spin_lock_irqsave(&enable_lock, flags); - if (clk->enable_count) + clk_reparent(clk, old_parent); + + if (migrated_enable && clk->enable_count) { + __clk_disable(parent); + } else if (migrated_enable && (clk->enable_count == 0)) { + __clk_disable(old_parent); + } else if (!migrated_enable && clk->enable_count) { + __clk_disable(parent); + __clk_enable(old_parent); + } + + spin_unlock_irqrestore(&enable_lock, flags); + + if (clk->prepare_count) + __clk_unprepare(parent); + + return ret; + } + + /* clean up enable for old parent if migration was done */ + if (migrated_enable) { + spin_lock_irqsave(&enable_lock, flags); __clk_disable(old_parent); - spin_unlock_irqrestore(&enable_lock, flags); + spin_unlock_irqrestore(&enable_lock, flags); + } + /* clean up prepare for old parent if migration was done */ if (clk->prepare_count) __clk_unprepare(old_parent); - return ret; + /* update debugfs with new clk tree topology */ + clk_debug_reparent(clk, parent); + return 0; } /** @@ -1339,16 +1378,11 @@ int clk_set_parent(struct clk *clk, struct clk *parent) /* do the re-parent */ ret = __clk_set_parent(clk, parent, p_index); - /* propagate ABORT_RATE_CHANGE if .set_parent failed */ - if (ret) { + /* propagate rate recalculation accordingly */ + if (ret) __clk_recalc_rates(clk, ABORT_RATE_CHANGE); - goto out; - } - - /* propagate rate recalculation downstream */ - clk_reparent(clk, parent); - clk_debug_reparent(clk, parent); - __clk_recalc_rates(clk, POST_RATE_CHANGE); + else + __clk_recalc_rates(clk, POST_RATE_CHANGE); out: mutex_unlock(&prepare_lock); -- 1.7.10 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-03-20 20:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-03-12 19:20 [RESEND PATCH 0/3] clk: Fixup issues for clk_set_parent Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 1/3] clk: Remove _clk_reparent from API and restructure code Ulf Hansson 2013-03-19 21:20 ` Mike Turquette 2013-03-20 9:45 ` Ulf Hansson 2013-03-20 10:13 ` Rajendra Nayak 2013-03-20 15:03 ` Mike Turquette 2013-03-20 20:39 ` Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 2/3] clk: Improve errorhandling for clk_set_parent Ulf Hansson 2013-03-12 19:20 ` [RESEND PATCH 3/3] clk: Fixup locking issues " Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).