Re: [PATCH 05/13] clk: keep track of the trigger of an ongoing clk_set_rate

[Benjamin Bara <bbara93@gmail.com>]

Hi!

On Tue, 19 Sept 2023 at 09:06, Maxime Ripard <mripard@kernel.org> wrote:
> On Mon, Sep 18, 2023 at 12:40:01AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > When we keep track of the rate change trigger, we can easily check if an
> > affected clock is affiliated with the trigger. Additionally, the trigger
> > is added to the notify data, so that drivers can implement workarounds
> > that might be necessary if a shared parent changes.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  drivers/clk/clk.c   | 12 ++++++++++++
> >  include/linux/clk.h |  2 ++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 4954d31899ce..8f4f92547768 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -33,6 +33,9 @@ static struct task_struct *enable_owner;
> >  static int prepare_refcnt;
> >  static int enable_refcnt;
> >
> > +/* responsible for ongoing rate change, protected by prepare_lock */
> > +static struct clk *rate_trigger_clk;
> > +
> >  static HLIST_HEAD(clk_root_list);
> >  static HLIST_HEAD(clk_orphan_list);
> >  static LIST_HEAD(clk_notifier_list);
> > @@ -1742,6 +1745,7 @@ static int __clk_notify(struct clk_core *core, unsigned long msg,
> >
> >       cnd.old_rate = old_rate;
> >       cnd.new_rate = new_rate;
> > +     cnd.trigger = rate_trigger_clk ? : core->parent->hw->clk;
> >
> >       list_for_each_entry(cn, &clk_notifier_list, node) {
> >               if (cn->clk->core == core) {
> > @@ -2513,6 +2517,8 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> >       /* prevent racing with updates to the clock topology */
> >       clk_prepare_lock();
> >
> > +     rate_trigger_clk = clk;
> > +
>
> So I don't think that interacts very well with the clk_hw_set_rate
> function you introduced. It looks like you only consider the initial
> clock here so you wouldn't update rate_trigger_clk on a clk_hw_set_rate
> call, but that creates some inconsistencies:
>
>   - If we call clk_hw_set_rate outside of the set_rate path (but in
>     .init for example), then we end up with a notifier without a trigger
>     clock set.
>
>   - More generally, depending on the path we're currently in, a call to
>     clk_hw_set_rate will notify a clock in different ways which is a bit
>     weird to me. The trigger clock can also be any clock, parent or
>     child, at any level, which definitely complicates things at the
>     driver level.
>
> The rate propagation is top-down, so could be get away with just setting
> the parent clock that triggered the notification?

As I mentioned in the other response, this implementation seems to be
just a hack to get additional context in the notifier. I think that's
also a problem Frank had in his approach. Inside the notifier, it's not
clear what to do with the incoming change. Because it could be either
"intended", meaning a sub-clock of the current clock has triggered the
change, or "unintended" (e.g. a sibling has triggered the change, but
the subtree beyond the current clock still requires the old rate, and
therefore the clock needs to adapt). Therefore I think if we use
req_rate here, we might be able to achieve the same thing in a better
way.

Thanks!