| Message ID | 20200616055223.119360-1-ikjn@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: Provide future parent in clk notification | expand |
Quoting Ikjoon Jang (2020-06-15 22:52:23) > Current clk notification handlers cannot know its new parent in > PRE_RATE_CHANGE event. This patch simply adds parent clk to > clk_notifier_data so the child clk is now able to know its future > parent prior to reparenting. Yes, but why is that important? > > Change-Id: I099a784d5302a93951bdc6254d85f8df8c770462 Please remove these. > Signed-off-by: Ikjoon Jang <ikjn@chromium.org> > --- > drivers/clk/clk.c | 30 +++++++++++++++++------------- > include/linux/clk.h | 9 ++++++--- > 2 files changed, 23 insertions(+), 16 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 3f588ed06ce3..62c4e7b50ae5 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1846,7 +1849,7 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, > * take on the rate of its parent. > */ > static int __clk_speculate_rates(struct clk_core *core, > - unsigned long parent_rate) > + struct clk_core *parent) > { > struct clk_core *child; > unsigned long new_rate; > @@ -1854,11 +1857,12 @@ static int __clk_speculate_rates(struct clk_core *core, > > lockdep_assert_held(&prepare_lock); > > - new_rate = clk_recalc(core, parent_rate); > + new_rate = clk_recalc(core, parent ? parent->rate : 0); > > /* abort rate change if a driver returns NOTIFY_BAD or NOTIFY_STOP */ > if (core->notifier_count) > - ret = __clk_notify(core, PRE_RATE_CHANGE, core->rate, new_rate); > + ret = __clk_notify(core, parent, PRE_RATE_CHANGE, > + core->rate, new_rate); > > if (ret & NOTIFY_STOP_MASK) { > pr_debug("%s: clk notifier callback for clock %s aborted with error %d\n", > @@ -1867,7 +1871,7 @@ static int __clk_speculate_rates(struct clk_core *core, > } > > hlist_for_each_entry(child, &core->children, child_node) { > - ret = __clk_speculate_rates(child, new_rate); > + ret = __clk_speculate_rates(child, core); How does this work? core->rate isn't assigned yet when we're speculating rates down the tree to the leaves. So that clk_recalc() in the above hunk would need to save the rate away, which is wrong because it isn't changed yet, for this line to make sense. Given that I had to read this for a few minutes to figure this out it seems that trying to combine the parent and the rate as arguments is actually more complicated than adding another parameter. Please just add another argument. > if (ret & NOTIFY_STOP_MASK) > break; > }
On Wed, Jun 24, 2020 at 3:08 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Ikjoon Jang (2020-06-15 22:52:23) > > Current clk notification handlers cannot know its new parent in > > PRE_RATE_CHANGE event. This patch simply adds parent clk to > > clk_notifier_data so the child clk is now able to know its future > > parent prior to reparenting. > > Yes, but why is that important? Basically I wondered if there are some cases needed to check more conditions other than clock rate (e.g. parent clock's internal properties). In my case, now I'm trying to make a wrapper clock on a mux clock which has a rate adjustable PLL clock and a fixed temporary clock as its parents. clkPLL clkTMP \ / clkMUX Current device driver is using three different clocks specified from the device tree and the driver handles clocks like this way to change operating clock speed. clk_set_parent(clkMUX, clkTMP); clk_set_rate(clkPLL, HZ); udelay(10); clk_set_parent(clkMUX, clkPLL); Now what I want to do is to supply only one clock to a device node, make the driver call clk_set_rate() only, and clkMUX 's notification handler does set_parent() things instead. So it's good to know that clkMUX's rate changing is not caused by clk_set_parent() and deny its rate changing when an inappropriate parent is set. Sorry for the long story for a simple reason, and maybe there could be a more plausible reason for this in near future? > > > > > Change-Id: I099a784d5302a93951bdc6254d85f8df8c770462 > > Please remove these. Oops, thanks! > > > Signed-off-by: Ikjoon Jang <ikjn@chromium.org> > > --- > > drivers/clk/clk.c | 30 +++++++++++++++++------------- > > include/linux/clk.h | 9 ++++++--- > > 2 files changed, 23 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 3f588ed06ce3..62c4e7b50ae5 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -1846,7 +1849,7 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, > > * take on the rate of its parent. > > */ > > static int __clk_speculate_rates(struct clk_core *core, > > - unsigned long parent_rate) > > + struct clk_core *parent) > > { > > struct clk_core *child; > > unsigned long new_rate; > > @@ -1854,11 +1857,12 @@ static int __clk_speculate_rates(struct clk_core *core, > > > > lockdep_assert_held(&prepare_lock); > > > > - new_rate = clk_recalc(core, parent_rate); > > + new_rate = clk_recalc(core, parent ? parent->rate : 0); > > > > /* abort rate change if a driver returns NOTIFY_BAD or NOTIFY_STOP */ > > if (core->notifier_count) > > - ret = __clk_notify(core, PRE_RATE_CHANGE, core->rate, new_rate); > > + ret = __clk_notify(core, parent, PRE_RATE_CHANGE, > > + core->rate, new_rate); > > > > if (ret & NOTIFY_STOP_MASK) { > > pr_debug("%s: clk notifier callback for clock %s aborted with error %d\n", > > @@ -1867,7 +1871,7 @@ static int __clk_speculate_rates(struct clk_core *core, > > } > > > > hlist_for_each_entry(child, &core->children, child_node) { > > - ret = __clk_speculate_rates(child, new_rate); > > + ret = __clk_speculate_rates(child, core); > > How does this work? core->rate isn't assigned yet when we're speculating > rates down the tree to the leaves. So that clk_recalc() in the above > hunk would need to save the rate away, which is wrong because it isn't > changed yet, for this line to make sense. > Yes, you're right, this was simply wrong.. > Given that I had to read this for a few minutes to figure this out it > seems that trying to combine the parent and the rate as arguments is > actually more complicated than adding another parameter. Please just add > another argument. > > > if (ret & NOTIFY_STOP_MASK) > > break; > > } Agree, Thank you for the review.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 3f588ed06ce3..62c4e7b50ae5 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1458,6 +1458,7 @@ EXPORT_SYMBOL_GPL(clk_round_rate); /** * __clk_notify - call clk notifier chain * @core: clk that is changing rate + * @parent: new parent of core * @msg: clk notifier type (see include/linux/clk.h) * @old_rate: old clk rate * @new_rate: new clk rate @@ -1469,13 +1470,15 @@ EXPORT_SYMBOL_GPL(clk_round_rate); * called if all went well, or NOTIFY_STOP or NOTIFY_BAD immediately if * a driver returns that. */ -static int __clk_notify(struct clk_core *core, unsigned long msg, - unsigned long old_rate, unsigned long new_rate) +static int __clk_notify(struct clk_core *core, struct clk_core *parent, + unsigned long msg, unsigned long old_rate, + unsigned long new_rate) { struct clk_notifier *cn; struct clk_notifier_data cnd; int ret = NOTIFY_DONE; + cnd.parent = parent ? parent->hw->clk : NULL; cnd.old_rate = old_rate; cnd.new_rate = new_rate; @@ -1597,7 +1600,7 @@ static void __clk_recalc_rates(struct clk_core *core, unsigned long msg) * & ABORT_RATE_CHANGE notifiers */ if (core->notifier_count && msg) - __clk_notify(core, msg, old_rate, core->rate); + __clk_notify(core, core->parent, msg, old_rate, core->rate); hlist_for_each_entry(child, &core->children, child_node) __clk_recalc_rates(child, msg); @@ -1834,7 +1837,7 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, /** * __clk_speculate_rates * @core: first clk in the subtree - * @parent_rate: the "future" rate of clk's parent + * @parent: the "future" parent of core * * Walks the subtree of clks starting with clk, speculating rates as it * goes and firing off PRE_RATE_CHANGE notifications as necessary. @@ -1846,7 +1849,7 @@ static int __clk_set_parent(struct clk_core *core, struct clk_core *parent, * take on the rate of its parent. */ static int __clk_speculate_rates(struct clk_core *core, - unsigned long parent_rate) + struct clk_core *parent) { struct clk_core *child; unsigned long new_rate; @@ -1854,11 +1857,12 @@ static int __clk_speculate_rates(struct clk_core *core, lockdep_assert_held(&prepare_lock); - new_rate = clk_recalc(core, parent_rate); + new_rate = clk_recalc(core, parent ? parent->rate : 0); /* abort rate change if a driver returns NOTIFY_BAD or NOTIFY_STOP */ if (core->notifier_count) - ret = __clk_notify(core, PRE_RATE_CHANGE, core->rate, new_rate); + ret = __clk_notify(core, parent, PRE_RATE_CHANGE, + core->rate, new_rate); if (ret & NOTIFY_STOP_MASK) { pr_debug("%s: clk notifier callback for clock %s aborted with error %d\n", @@ -1867,7 +1871,7 @@ static int __clk_speculate_rates(struct clk_core *core, } hlist_for_each_entry(child, &core->children, child_node) { - ret = __clk_speculate_rates(child, new_rate); + ret = __clk_speculate_rates(child, core); if (ret & NOTIFY_STOP_MASK) break; } @@ -1996,7 +2000,8 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core, return NULL; if (core->notifier_count) { - ret = __clk_notify(core, event, core->rate, core->new_rate); + ret = __clk_notify(core, core->parent, event, + core->rate, core->new_rate); if (ret & NOTIFY_STOP_MASK) fail_clk = core; } @@ -2098,7 +2103,8 @@ static void clk_change_rate(struct clk_core *core) clk_core_disable_unprepare(parent); if (core->notifier_count && old_rate != core->rate) - __clk_notify(core, POST_RATE_CHANGE, old_rate, core->rate); + __clk_notify(core, core->parent, POST_RATE_CHANGE, + old_rate, core->rate); if (core->flags & CLK_RECALC_NEW_RATES) (void)clk_calc_new_rates(core, core->new_rate); @@ -2479,7 +2485,6 @@ static int clk_core_set_parent_nolock(struct clk_core *core, { int ret = 0; int p_index = 0; - unsigned long p_rate = 0; lockdep_assert_held(&prepare_lock); @@ -2508,7 +2513,6 @@ static int clk_core_set_parent_nolock(struct clk_core *core, __func__, parent->name, core->name); return p_index; } - p_rate = parent->rate; } ret = clk_pm_runtime_get(core); @@ -2516,7 +2520,7 @@ static int clk_core_set_parent_nolock(struct clk_core *core, return ret; /* propagate PRE_RATE_CHANGE notifications */ - ret = __clk_speculate_rates(core, p_rate); + ret = __clk_speculate_rates(core, parent); /* abort if a driver objects */ if (ret & NOTIFY_STOP_MASK) diff --git a/include/linux/clk.h b/include/linux/clk.h index 7fd6a1febcf4..e246e160b290 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -60,16 +60,19 @@ struct clk_notifier { /** * struct clk_notifier_data - rate data to pass to the notifier callback * @clk: struct clk * being changed + * @parent: new parent of this clk * @old_rate: previous rate of this clk * @new_rate: new rate of this clk * * For a pre-notifier, old_rate is the clk's rate before this rate - * change, and new_rate is what the rate will be in the future. For a - * post-notifier, old_rate and new_rate are both set to the clk's - * current rate (this was done to optimize the implementation). + * change, new_rate is what the rate will be in the future, and parent is + * new parent of the clk after new_rate is applied, For a post-notifier, + * parent, old_rate, and new_rate are all set to the clk's current state. + * (this was done to optimize the implementation). */ struct clk_notifier_data { struct clk *clk; + struct clk *parent; unsigned long old_rate; unsigned long new_rate; };
Current clk notification handlers cannot know its new parent in PRE_RATE_CHANGE event. This patch simply adds parent clk to clk_notifier_data so the child clk is now able to know its future parent prior to reparenting. Change-Id: I099a784d5302a93951bdc6254d85f8df8c770462 Signed-off-by: Ikjoon Jang <ikjn@chromium.org> --- drivers/clk/clk.c | 30 +++++++++++++++++------------- include/linux/clk.h | 9 ++++++--- 2 files changed, 23 insertions(+), 16 deletions(-)