On Wed, Jun 24, 2020 at 3:08 AM Stephen Boyd <sb...@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 <i...@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.
