Re: [PATCH] clk: Provide future parent in clk notification

[Ikjoon Jang]

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