Re: [PATCH v3] Common clock: To list active consumers of clocks

Stephen Boyd
2022-08-22T23:50:23Z

Quoting Vishal Badole (2022-08-02 11:09:47)
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f00d4c1..c96079f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -102,6 +102,7 @@ struct clk {
> unsigned long min_rate;
> unsigned long max_rate;
> unsigned int exclusive_count;
> + unsigned int enable_count;
> struct hlist_node clks_node;
> };
>
> @@ -1008,6 +1009,10 @@ void clk_disable(struct clk *clk)
> return;
>
> clk_core_disable_lock(clk->core);
> +
> + if (clk->enable_count > 0)
> + clk->enable_count--;
> +
> }
> EXPORT_SYMBOL_GPL(clk_disable);
>
> @@ -1169,10 +1174,16 @@ EXPORT_SYMBOL_GPL(clk_restore_context);
> */
> int clk_enable(struct clk *clk)
> {
> + int ret;
> +
> if (!clk)
> return 0;
>
> - return clk_core_enable_lock(clk->core);
> + ret = clk_core_enable_lock(clk->core);
> + if (!ret)
> + clk->enable_count++;
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(clk_enable);

We'll want the above three hunks to be a different patch so we can
discuss the merits of tracking per user enable counts. Do you have a
usecase for this or is it "just for fun"? By adding a count we have more
code, and we waste more memory to track this stat. I really would rather
not bloat just because, so please elaborate on your use case.

>
> @@ -2953,28 +2964,41 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c,
> int level)
> {
> int phase;
> + struct clk *clk_user;
> + int multi_node = 0;
>
> - seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu ",
> + seq_printf(s, "%*s%-*s %-7d %-8d %-8d %-11lu %-10lu ",
> level * 3 + 1, "",
> - 30 - level * 3, c->name,
> + 35 - level * 3, c->name,
> c->enable_count, c->prepare_count, c->protect_count,
> clk_core_get_rate_recalc(c),
> clk_core_get_accuracy_recalc(c));
>
> phase = clk_core_get_phase(c);
> if (phase >= 0)
> - seq_printf(s, "%5d", phase);
> + seq_printf(s, "%-5d", phase);
> else
> seq_puts(s, "-----");
>
> - seq_printf(s, " %6d", clk_core_get_scaled_duty_cycle(c, 100000));
> + seq_printf(s, " %-6d", clk_core_get_scaled_duty_cycle(c, 100000));
>
> if (c->ops->is_enabled)
> - seq_printf(s, " %9c\n", clk_core_is_enabled(c) ? 'Y' : 'N');
> + seq_printf(s, " %5c ", clk_core_is_enabled(c) ? 'Y' : 'N');
> else if (!c->ops->enable)
> - seq_printf(s, " %9c\n", 'Y');
> + seq_printf(s, " %5c ", 'Y');
> else
> - seq_printf(s, " %9c\n", '?');
> + seq_printf(s, " %5c ", '?');
> +
> + hlist_for_each_entry(clk_user, &c->clks, clks_node) {
> + seq_printf(s, "%*s%-*s %-4d\n",
> + level * 3 + 2 + 105 * multi_node, "",
> + 30,
> + clk_user->dev_id ? clk_user->dev_id : "deviceless",
> + clk_user->enable_count);
> +
> + multi_node = 1;

This part that prints the dev_id might be useful and can be the first
patch in the series. In that same patch, please print the con_id so we
know which clk it is for the device. We should also improve of_clk_get()
so that the index is visible to the 'struct clk::con_id' somehow. Maybe
we can convert the integer index into a string and assign that to con_id
in that case as well.