From: Benjamin Bara <[email protected]>
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 <[email protected]>
---
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;
+
if (clk->exclusive_count)
clk_core_rate_unprotect(clk->core);
@@ -2521,6 +2527,8 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
if (clk->exclusive_count)
clk_core_rate_protect(clk->core);
+ rate_trigger_clk = NULL;
+
clk_prepare_unlock();
return ret;
@@ -2556,6 +2564,8 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
/* prevent racing with updates to the clock topology */
clk_prepare_lock();
+ rate_trigger_clk = clk;
+
/*
* The temporary protection removal is not here, on purpose
* This function is meant to be used instead of clk_rate_protect,
@@ -2568,6 +2578,8 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
clk->exclusive_count++;
}
+ rate_trigger_clk = NULL;
+
clk_prepare_unlock();
return ret;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 06f1b292f8a0..f0fe78c7a0f1 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -60,6 +60,7 @@ struct clk_notifier {
/**
* struct clk_notifier_data - rate data to pass to the notifier callback
* @clk: struct clk * being changed
+ * @trigger: struct clk * being responsible for the change
* @old_rate: previous rate of this clk
* @new_rate: new rate of this clk
*
@@ -70,6 +71,7 @@ struct clk_notifier {
*/
struct clk_notifier_data {
struct clk *clk;
+ struct clk *trigger;
unsigned long old_rate;
unsigned long new_rate;
};
--
2.34.1
On Mon, Sep 18, 2023 at 12:40:01AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <[email protected]>
>
> 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 <[email protected]>
> ---
> 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?
Either way, we need unit tests for that too.
Maxime
Hi!
On Tue, 19 Sept 2023 at 09:06, Maxime Ripard <[email protected]> wrote:
> On Mon, Sep 18, 2023 at 12:40:01AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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!