| Message ID | 1363116050-3882-2-git-send-email-ulf.hansson@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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;