Hi!
On Tue, 19 Sept 2023 at 09:06, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> On Mon, Sep 18, 2023 at 12:40:01AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > 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!
