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

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

[Stephen Boyd]

Why "5.4" in the subject?

Quoting Vishal Badole (2022-05-31 11:28:35)
> This feature lists the name of clocks and their consumer devices. This
> debug feature can be used to check the active clocks and the devices who
> have enabled them.
> 
> for example:
> debian@beaglebone:~$ cat /sys/kernel/debug/clk/clk_devices_name
>             clock_name                                  devices_name
>       ----------------
> -------------------------------------------------------------------------
>              l4-wkup-clkctrl:0008:0              44e07000.target-module
>                     l4ls-clkctrl:0074:0              4804c000.target-module
>                     l4ls-clkctrl:0058:0              48311fe0.target-module
>                  l4-rtc-clkctrl:0000:0              44e3e074.target-module
>                           clk_32768_ck                                
> 44e3e000.rtc
>                    l4ls-clkctrl:00d8:0              480c8000.target-module
>    cpsw-125mhz-clkctrl:0014:0              4a101200.target-module
> 
> Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>

Where are the Co-developed-by tags? Also, your SoB should be last.

> ---
>  drivers/clk/Kconfig |   8 ++++
>  drivers/clk/clk.c   | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 127 insertions(+), 1 deletion(-)

The patch is malformed. Not sure what's happening with your MUA.

> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c44247d..549cdda 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -23,6 +23,14 @@ config COMMON_CLK
>  menu "Common Clock Framework"
>   depends on COMMON_CLK
>  
> +config DEBUG_CLK_CONSUMER
> + bool "Debug feature to list clocks and their active consumers"

Don't think we need a new config for this. Just add it to the existing
CONFIG_DEBUGFS area.

> + depends on DEBUG_FS && COMMON_CLK
> + help
> +  Clock consumer debug feature supports for clock debugging. Chose y
> +  to get debug entry in file system to list clocks and their active
> +  consumer devices.
> +
>  config COMMON_CLK_WM831X
>   tristate "Clock driver for WM831x/2x PMICs"
>   depends on MFD_WM831X
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 13332f8..dccbd35 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -105,6 +105,84 @@ struct clk {
>   struct hlist_node clks_node;
>  };
>  
> +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> +/*Linked List Node*/
> +struct clk_dev_list {
> + struct list_head list;
> + const char *clk_name;
> + const char *dev_name;
> +};
> +
> +/*Declare and init the head node of the linked list*/
> +LIST_HEAD(head_node);
> +
> +static void clk_dev_entry(struct clk *clk_ptr)
> +{
> + struct clk_dev_list *new_node_ptr = NULL;
> + struct clk_dev_list *temp_node_ptr = NULL;
> + int repeat_count = 0;
> + static bool is_first_node;
> + const char *clk_name_ptr = NULL;
> + const char *dev_name_ptr = NULL;
> +
> + if (clk_ptr->dev) {
> + dev_name_ptr = dev_name(clk_ptr->dev);
> +
> + clk_name_ptr = clk_ptr->core->name;
> +
> + if (is_first_node) {
> + /* Iterate the list to check duplicate entry */
> + list_for_each_entry(temp_node_ptr, &head_node, list) {
> + if (temp_node_ptr->clk_name == clk_name_ptr &&
> +    temp_node_ptr->dev_name == dev_name_ptr) {
> + repeat_count++;
> + break;
> + }
> + }
> + }
> +
> + is_first_node = 1;
> +
> + if (!repeat_count && clk_ptr->core->enable_count) {
> + /*Creating Node*/
> + new_node_ptr = kmalloc(sizeof(*new_node_ptr),
> +       GFP_KERNEL);
> + if (!new_node_ptr)
> + return;
> +
> + /*Assgin the data that is received*/

Typo in Assign.

> + new_node_ptr->clk_name = clk_name_ptr;
> + new_node_ptr->dev_name = dev_name_ptr;
> +
> + /*Init the list within the struct*/
> + INIT_LIST_HEAD(&new_node_ptr->list);
> +
> + /*Add Node to Linked List*/
> + list_add_tail(&new_node_ptr->list, &head_node);
> + }
> + }
> +}
> +
> +/* Function to remove the clk and device entry */
> +static void clk_dev_dentry(struct clk *clk)
> +{
> + struct clk_dev_list *temp_node_ptr = NULL;
> + struct clk_dev_list *cur_node_ptr = NULL;
> +
> + if (clk->dev) {
> + /* Go through the list and free the memory */
> + list_for_each_entry_safe(cur_node_ptr, temp_node_ptr,
> + &head_node, list) {
> + if (cur_node_ptr->clk_name == clk->core->name &&
> +    cur_node_ptr->dev_name == dev_name(clk->dev)) {
> + list_del(&cur_node_ptr->list);
> + kfree(cur_node_ptr);
> + }
> + }
> + }
> +}
> +#endif
> +
>  /***           runtime pm          ***/
>  static int clk_pm_runtime_get(struct clk_core *core)
>  {
> @@ -1020,6 +1098,9 @@ void clk_disable(struct clk *clk)
>   return;
>  
>   clk_core_disable_lock(clk->core);
> +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> + clk_dev_dentry(clk);
> +#endif
>  }
>  EXPORT_SYMBOL_GPL(clk_disable);
>  
> @@ -1181,10 +1262,21 @@ EXPORT_SYMBOL_GPL(clk_restore_context);
>   */
>  int clk_enable(struct clk *clk)
>  {
> +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> + int ret = 0;
> +#endif
>   if (!clk)
>   return 0;
>  
> +#ifndef CONFIG_DEBUG_CLK_CONSUMER
>   return clk_core_enable_lock(clk->core);
> +#else
> + ret = clk_core_enable_lock(clk->core);
> + if (!ret)
> + clk_dev_entry(clk);
> +
> + return ret;
> +#endif

Not sure what this is doing.

>  }
>  EXPORT_SYMBOL_GPL(clk_enable);
>  
> @@ -2986,6 +3078,29 @@ static void clk_dump_one(struct seq_file *s, struct
> clk_core *c, int level)
>     clk_core_get_scaled_duty_cycle(c, 100000));
>  }
>  
> +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> +static int clk_devices_show(struct seq_file *s, void *data)
> +{
> + struct clk_dev_list *clk_dev_node;
> +
> + seq_puts(s, "            clock_name                                
>  devices_name\n");
> + seq_puts(s,
> "-------------------------------------------------------------------------\n");
> +
> + clk_prepare_lock();
> +
> + /*Traversing Linked List and Print its Members*/
> + list_for_each_entry(clk_dev_node, &head_node, list) {

It's hard to read but we already have a list of all clk consumers for a
clk_hw pointer, see clk_core_link_consumer(). And we stash the device
consuming it with clk_hw_create_clk(). That should be sufficient.


> + seq_printf(s, "%35s %35s\n", clk_dev_node->clk_name,
> +   clk_dev_node->dev_name);
> + }
> +
> + clk_prepare_unlock();
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(clk_devices);
> +#endif
> +
>  static void clk_dump_subtree(struct seq_file *s, struct clk_core *c, int
> level)
>  {
>   struct clk_core *child;
> @@ -3256,7 +3371,10 @@ static int __init clk_debug_init(void)
>      &clk_summary_fops);
>   debugfs_create_file("clk_orphan_dump", 0444, rootdir, &orphan_list,
>      &clk_dump_fops);
> -
> +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> + debugfs_create_file("clk_devices_name", 0444, rootdir, NULL,

Call it 'clk_consumers' please.

> +    &clk_devices_fops);

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

[<Vishal Badole>]

Hi Stephen,
Thanks for reviewing and giving your valueable review points for this
patch. Shortly I will attach a revised patch based on your review
points.

Thanks,
Vishal
On Tue, Jun 14, 2022 at 11:25:01PM +0530, chinmoy ghosh wrote:
> Agreed here .
> I feel , if  dev_id <https://elixir.bootlin.com/linux/latest/C/ident/dev_id>
> is not from dts property then dont think we can easily find out consumers
> from clk_hw_create_clk.Depends on how the legacy driver maintained the BSP
> code.
> Check the next .
> 
> Thanks
> Chinmoy
> 
> On Sat, Jun 11, 2022 at 1:10 AM Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Why "5.4" in the subject?
> >
> > Quoting Vishal Badole (2022-05-31 11:28:35)
> > > This feature lists the name of clocks and their consumer devices. This
> > > debug feature can be used to check the active clocks and the devices who
> > > have enabled them.
> > >
> > > for example:
> > > debian@beaglebone:~$ cat /sys/kernel/debug/clk/clk_devices_name
> > >             clock_name                                  devices_name
> > >       ----------------
> > > -------------------------------------------------------------------------
> > >              l4-wkup-clkctrl:0008:0              44e07000.target-module
> > >                     l4ls-clkctrl:0074:0
> >  4804c000.target-module
> > >                     l4ls-clkctrl:0058:0
> >  48311fe0.target-module
> > >                  l4-rtc-clkctrl:0000:0
> >  44e3e074.target-module
> > >                           clk_32768_ck
> > > 44e3e000.rtc
> > >                    l4ls-clkctrl:00d8:0
> >  480c8000.target-module
> > >    cpsw-125mhz-clkctrl:0014:0              4a101200.target-module
> > >
> > > Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> > > Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> > > Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> > > Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> >
> > Where are the Co-developed-by tags? Also, your SoB should be last.
> >
> > > ---
> > >  drivers/clk/Kconfig |   8 ++++
> > >  drivers/clk/clk.c   | 120
> > +++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 127 insertions(+), 1 deletion(-)
> >
> > The patch is malformed. Not sure what's happening with your MUA.
> >
> > >
> > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > > index c44247d..549cdda 100644
> > > --- a/drivers/clk/Kconfig
> > > +++ b/drivers/clk/Kconfig
> > > @@ -23,6 +23,14 @@ config COMMON_CLK
> > >  menu "Common Clock Framework"
> > >   depends on COMMON_CLK
> > >
> > > +config DEBUG_CLK_CONSUMER
> > > + bool "Debug feature to list clocks and their active consumers"
> >
> > Don't think we need a new config for this. Just add it to the existing
> > CONFIG_DEBUGFS area.
> >
> > > + depends on DEBUG_FS && COMMON_CLK
> > > + help
> > > +  Clock consumer debug feature supports for clock debugging. Chose y
> > > +  to get debug entry in file system to list clocks and their active
> > > +  consumer devices.
> > > +
> > >  config COMMON_CLK_WM831X
> > >   tristate "Clock driver for WM831x/2x PMICs"
> > >   depends on MFD_WM831X
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 13332f8..dccbd35 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -105,6 +105,84 @@ struct clk {
> > >   struct hlist_node clks_node;
> > >  };
> > >
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > +/*Linked List Node*/
> > > +struct clk_dev_list {
> > > + struct list_head list;
> > > + const char *clk_name;
> > > + const char *dev_name;
> > > +};
> > > +
> > > +/*Declare and init the head node of the linked list*/
> > > +LIST_HEAD(head_node);
> > > +
> > > +static void clk_dev_entry(struct clk *clk_ptr)
> > > +{
> > > + struct clk_dev_list *new_node_ptr = NULL;
> > > + struct clk_dev_list *temp_node_ptr = NULL;
> > > + int repeat_count = 0;
> > > + static bool is_first_node;
> > > + const char *clk_name_ptr = NULL;
> > > + const char *dev_name_ptr = NULL;
> > > +
> > > + if (clk_ptr->dev) {
> > > + dev_name_ptr = dev_name(clk_ptr->dev);
> > > +
> > > + clk_name_ptr = clk_ptr->core->name;
> > > +
> > > + if (is_first_node) {
> > > + /* Iterate the list to check duplicate entry */
> > > + list_for_each_entry(temp_node_ptr, &head_node, list) {
> > > + if (temp_node_ptr->clk_name == clk_name_ptr &&
> > > +    temp_node_ptr->dev_name == dev_name_ptr) {
> > > + repeat_count++;
> > > + break;
> > > + }
> > > + }
> > > + }
> > > +
> > > + is_first_node = 1;
> > > +
> > > + if (!repeat_count && clk_ptr->core->enable_count) {
> > > + /*Creating Node*/
> > > + new_node_ptr = kmalloc(sizeof(*new_node_ptr),
> > > +       GFP_KERNEL);
> > > + if (!new_node_ptr)
> > > + return;
> > > +
> > > + /*Assgin the data that is received*/
> >
> > Typo in Assign.
> >
> > > + new_node_ptr->clk_name = clk_name_ptr;
> > > + new_node_ptr->dev_name = dev_name_ptr;
> > > +
> > > + /*Init the list within the struct*/
> > > + INIT_LIST_HEAD(&new_node_ptr->list);
> > > +
> > > + /*Add Node to Linked List*/
> > > + list_add_tail(&new_node_ptr->list, &head_node);
> > > + }
> > > + }
> > > +}
> > > +
> > > +/* Function to remove the clk and device entry */
> > > +static void clk_dev_dentry(struct clk *clk)
> > > +{
> > > + struct clk_dev_list *temp_node_ptr = NULL;
> > > + struct clk_dev_list *cur_node_ptr = NULL;
> > > +
> > > + if (clk->dev) {
> > > + /* Go through the list and free the memory */
> > > + list_for_each_entry_safe(cur_node_ptr, temp_node_ptr,
> > > + &head_node, list) {
> > > + if (cur_node_ptr->clk_name == clk->core->name &&
> > > +    cur_node_ptr->dev_name == dev_name(clk->dev)) {
> > > + list_del(&cur_node_ptr->list);
> > > + kfree(cur_node_ptr);
> > > + }
> > > + }
> > > + }
> > > +}
> > > +#endif
> > > +
> > >  /***           runtime pm          ***/
> > >  static int clk_pm_runtime_get(struct clk_core *core)
> > >  {
> > > @@ -1020,6 +1098,9 @@ void clk_disable(struct clk *clk)
> > >   return;
> > >
> > >   clk_core_disable_lock(clk->core);
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > + clk_dev_dentry(clk);
> > > +#endif
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_disable);
> > >
> > > @@ -1181,10 +1262,21 @@ EXPORT_SYMBOL_GPL(clk_restore_context);
> > >   */
> > >  int clk_enable(struct clk *clk)
> > >  {
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > + int ret = 0;
> > > +#endif
> > >   if (!clk)
> > >   return 0;
> > >
> > > +#ifndef CONFIG_DEBUG_CLK_CONSUMER
> > >   return clk_core_enable_lock(clk->core);
> > > +#else
> > > + ret = clk_core_enable_lock(clk->core);
> > > + if (!ret)
> > > + clk_dev_entry(clk);
> > > +
> > > + return ret;
> > > +#endif
> >
> > Not sure what this is doing.
> >
> > >  }
> > >  EXPORT_SYMBOL_GPL(clk_enable);
> > >
> > > @@ -2986,6 +3078,29 @@ static void clk_dump_one(struct seq_file *s,
> > struct
> > > clk_core *c, int level)
> > >     clk_core_get_scaled_duty_cycle(c, 100000));
> > >  }
> > >
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > +static int clk_devices_show(struct seq_file *s, void *data)
> > > +{
> > > + struct clk_dev_list *clk_dev_node;
> > > +
> > > + seq_puts(s, "            clock_name
> > >  devices_name\n");
> > > + seq_puts(s,
> > >
> > "-------------------------------------------------------------------------\n");
> > > +
> > > + clk_prepare_lock();
> > > +
> > > + /*Traversing Linked List and Print its Members*/
> > > + list_for_each_entry(clk_dev_node, &head_node, list) {
> >
> > It's hard to read but we already have a list of all clk consumers for a
> > clk_hw pointer, see clk_core_link_consumer(). And we stash the device
> > consuming it with clk_hw_create_clk(). That should be sufficient.
> >
> >
> > > + seq_printf(s, "%35s %35s\n", clk_dev_node->clk_name,
> > > +   clk_dev_node->dev_name);
> > > + }
> > > +
> > > + clk_prepare_unlock();
> > > +
> > > + return 0;
> > > +}
> > > +DEFINE_SHOW_ATTRIBUTE(clk_devices);
> > > +#endif
> > > +
> > >  static void clk_dump_subtree(struct seq_file *s, struct clk_core *c, int
> > > level)
> > >  {
> > >   struct clk_core *child;
> > > @@ -3256,7 +3371,10 @@ static int __init clk_debug_init(void)
> > >      &clk_summary_fops);
> > >   debugfs_create_file("clk_orphan_dump", 0444, rootdir, &orphan_list,
> > >      &clk_dump_fops);
> > > -
> > > +#ifdef CONFIG_DEBUG_CLK_CONSUMER
> > > + debugfs_create_file("clk_devices_name", 0444, rootdir, NULL,
> >
> > Call it 'clk_consumers' please.
> >
> > > +    &clk_devices_fops);
> >

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

[<Vishal Badole>]

From f2e9d78bd0f135206fbfbf2e0178a5782b972939 Mon Sep 17 00:00:00 2001
From: Vishal Badole <badolevishal1116@gmail.com>
Date: Tue, 21 Jun 2022 09:55:51 +0530
Subject: [PATCH] Common clock: To list active consumers of clocks

This feature lists the name of clocks and their consumer devices.
Using this feature user can easily check which device is using a
perticular clock. Consumers without dev_id are listed as no_dev_id.

Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
Acked-by: Vimal Kumar <vimal.kumar32@gmail.com>
Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
---
 drivers/clk/clk.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ed11918..b191009 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3018,6 +3018,63 @@ static int clk_summary_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(clk_summary);
 
+static void clk_consumer_show_one(struct seq_file *s, struct clk_core *core, int level)
+{
+	struct clk *clk_user;
+	const char *consumer;
+
+	hlist_for_each_entry(clk_user, &core->clks, clks_node) {
+		if (!clk_user->dev_id)
+			consumer = "no_dev_id";
+		else
+			consumer = clk_user->dev_id;
+
+		seq_printf(s, "%*s%-*s %30s %5d %7d ",
+			   level * 3 + 1, "",
+			   30 - level * 3, clk_user->core->name, consumer,
+			   clk_user->core->enable_count, clk_user->core->prepare_count);
+
+		if (clk_user->core->ops->is_enabled)
+			seq_printf(s, " %8c\n", clk_core_is_enabled(clk_user->core) ? 'Y' : 'N');
+		else if (!clk_user->core->ops->enable)
+			seq_printf(s, " %8c\n", 'Y');
+		else
+			seq_printf(s, " %8c\n", '?');
+	}
+}
+
+static void clk_consumer_show_subtree(struct seq_file *s, struct clk_core *c, int level)
+{
+	struct clk_core *child;
+
+	clk_consumer_show_one(s, c, level);
+
+	hlist_for_each_entry(child, &c->children, child_node)
+		clk_consumer_show_subtree(s, child, level + 1);
+}
+
+static int clk_consumer_show(struct seq_file *s, void *data)
+{
+	struct clk_core *c;
+	struct hlist_head **lists = (struct hlist_head **)s->private;
+
+	seq_puts(s, "                                                              enable   prepare   hardware\n");
+	seq_puts(s, "   clock                                       consumer        count     count     enable\n");
+	seq_puts(s, "-----------------------------------------------------------------------------------------\n");
+	clk_prepare_lock();
+
+	/*Traversing Linked List to print clock consumer*/
+
+	for (; *lists; lists++)
+		hlist_for_each_entry(c, *lists, child_node)
+			clk_consumer_show_subtree(s, c, 0);
+
+	clk_prepare_unlock();
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(clk_consumer);
+
 static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level)
 {
 	int phase;
@@ -3437,6 +3494,8 @@ static int __init clk_debug_init(void)
 			    &clk_summary_fops);
 	debugfs_create_file("clk_orphan_dump", 0444, rootdir, &orphan_list,
 			    &clk_dump_fops);
+	debugfs_create_file("clk_consumer", 0444, rootdir, &all_lists,
+			    &clk_consumer_fops);
 
 	mutex_lock(&clk_debug_lock);
 	hlist_for_each_entry(core, &clk_debug_list, debug_node)
-- 
2.7.4

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

[Vishal Badole]

This feature lists the clock consumer's name and per-user enable count
in clock summary. Using this feature user can easily check which device
has acquired a perticular clock and it is enabled by respective device
or not.
for example:
$ cat /sys/kernel/debug/clk/clk_summary
                      enable  prepare  protect                           duty  hardware                            per-user
   clock               count    count    count    rate   accuracy phase cycle    enable   consumer                    count
----------------------------------------------------------------------------------------------------------------------------
 clk_mcasp0_fixed         0        0        0    24576000      0     0  50000     Y      deviceless                      0
                                                                                         deviceless                      0
    clk_mcasp0            0        0        0    24576000      0     0  50000     N          simple-audio-card,cpu           0
                                                                                             deviceless                      0

Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
Co-developed-by: Vimal Kumar <vimal.kumar32@gmail.com>
Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
---
 drivers/clk/clk.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

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);
 
@@ -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;
+	}
+
 }
 
 static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
@@ -2995,9 +3019,9 @@ static int clk_summary_show(struct seq_file *s, void *data)
 	struct clk_core *c;
 	struct hlist_head **lists = (struct hlist_head **)s->private;
 
-	seq_puts(s, "                                 enable  prepare  protect                                duty  hardware\n");
-	seq_puts(s, "   clock                          count    count    count        rate   accuracy phase  cycle    enable\n");
-	seq_puts(s, "-------------------------------------------------------------------------------------------------------\n");
+	seq_puts(s, "                                 enable  prepare  protect                                duty  hardware                            per-user\n");
+	seq_puts(s, "   clock                          count    count    count        rate   accuracy phase  cycle    enable   consumer                    count\n");
+	seq_puts(s, "-------------------------------------------------------------------------------------------------------------------------------------------\n");
 
 	clk_prepare_lock();
 
-- 
2.7.4

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

[<Vishal Badole>]

On Tue, Aug 02, 2022 at 11:39:47PM +0530, Vishal Badole wrote:
> This feature lists the clock consumer's name and per-user enable count
> in clock summary. Using this feature user can easily check which device
> has acquired a perticular clock and it is enabled by respective device
> or not.
> for example:
> $ cat /sys/kernel/debug/clk/clk_summary
>                       enable  prepare  protect                           duty  hardware                            per-user
>    clock               count    count    count    rate   accuracy phase cycle    enable   consumer                    count
> ----------------------------------------------------------------------------------------------------------------------------
>  clk_mcasp0_fixed         0        0        0    24576000      0     0  50000     Y      deviceless                      0
>                                                                                          deviceless                      0
>     clk_mcasp0            0        0        0    24576000      0     0  50000     N          simple-audio-card,cpu           0
>                                                                                              deviceless                      0
> 
> Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
> Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> Co-developed-by: Vimal Kumar <vimal.kumar32@gmail.com>
> Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> ---
>  drivers/clk/clk.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> 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);
>  
> @@ -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;
> +	}
> +
>  }
>  
>  static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
> @@ -2995,9 +3019,9 @@ static int clk_summary_show(struct seq_file *s, void *data)
>  	struct clk_core *c;
>  	struct hlist_head **lists = (struct hlist_head **)s->private;
>  
> -	seq_puts(s, "                                 enable  prepare  protect                                duty  hardware\n");
> -	seq_puts(s, "   clock                          count    count    count        rate   accuracy phase  cycle    enable\n");
> -	seq_puts(s, "-------------------------------------------------------------------------------------------------------\n");
> +	seq_puts(s, "                                 enable  prepare  protect                                duty  hardware                            per-user\n");
> +	seq_puts(s, "   clock                          count    count    count        rate   accuracy phase  cycle    enable   consumer                    count\n");
> +	seq_puts(s, "-------------------------------------------------------------------------------------------------------------------------------------------\n");
>  
>  	clk_prepare_lock();
>  
> -- 
> 2.7.4
>
Hi Stephen,
Have you got a chance to review the above patch? 
We have made the changes as per the reviews, please have a look on the
patch.

Regards,
Vishal

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

[<Vishal Badole>]

On Tue, Aug 02, 2022 at 11:39:47PM +0530, Vishal Badole wrote:
> This feature lists the clock consumer's name and per-user enable count
> in clock summary. Using this feature user can easily check which device
> has acquired a perticular clock and it is enabled by respective device
> or not.
> for example:
> $ cat /sys/kernel/debug/clk/clk_summary
>                       enable  prepare  protect                           duty  hardware                            per-user
>    clock               count    count    count    rate   accuracy phase cycle    enable   consumer                    count
> ----------------------------------------------------------------------------------------------------------------------------
>  clk_mcasp0_fixed         0        0        0    24576000      0     0  50000     Y      deviceless                      0
>                                                                                          deviceless                      0
>     clk_mcasp0            0        0        0    24576000      0     0  50000     N          simple-audio-card,cpu           0
>                                                                                              deviceless                      0
> 
> Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
> Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> Co-developed-by: Vimal Kumar <vimal.kumar32@gmail.com>
> Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> ---
>  drivers/clk/clk.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> 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);
>  
> @@ -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;
> +	}
> +
>  }
>  
>  static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c,
> @@ -2995,9 +3019,9 @@ static int clk_summary_show(struct seq_file *s, void *data)
>  	struct clk_core *c;
>  	struct hlist_head **lists = (struct hlist_head **)s->private;
>  
> -	seq_puts(s, "                                 enable  prepare  protect                                duty  hardware\n");
> -	seq_puts(s, "   clock                          count    count    count        rate   accuracy phase  cycle    enable\n");
> -	seq_puts(s, "-------------------------------------------------------------------------------------------------------\n");
> +	seq_puts(s, "                                 enable  prepare  protect                                duty  hardware                            per-user\n");
> +	seq_puts(s, "   clock                          count    count    count        rate   accuracy phase  cycle    enable   consumer                    count\n");
> +	seq_puts(s, "-------------------------------------------------------------------------------------------------------------------------------------------\n");
>  
>  	clk_prepare_lock();
>  
> -- 
> 2.7.4
>

Hi Stephen,
Please review the above patch. Here we have made the changes as per your
review points. 
Note: The example format in commit meassage is getting changed during
copy paste but we are getting proper formatted and parsable output on
actual target.


Regards,
Vishal

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

[Stephen Boyd]

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.

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

[<Vishal Badole>]

On Mon, Aug 22, 2022 at 04:50:12PM -0700, Stephen Boyd wrote:
> 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. 
Agreed, we will create a separate patch for the same.

> 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.
> 
Use case for per user count: If a consumer acquires the clocks without 
calling clk_get() or devm_clk_get() and enables without calling clk_enable() 
or clk_prepare_enable() (by bypassing the common clock framework), then dev_id 
will not be sufficient to tell about how clk is acquired. Here per user enable 
count can be used to tell that how clk is acquired and it is enabled by
that particular device or not in case also where dev_id is NULL.

We referred regualtor framework suggested by you in one of review point
where they are also using enable count.
> >  
> > @@ -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.

Agreed, We will create a fresh patch where we will print dev_id and
consumer id in clock summary.

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

[<Vishal Badole>]

Hi Stephen,
As per your suggestions, we have updated and sent another gerrit with
message Id <1669569799-8526-1-git-send-email-badolevishal1116@gmail.com>
In this new patch we are listing the clock consumers name along with
consumer id in clk_summary.

example:
cat /sys/kernel/debug/clk/clk_summary
                      enable  prepare  protect
                             		                             duty  hardware                                Connection
clock               count    count    count  rate   accuracy phase  cycle    enable       consumer                         Id
------------------------------------------------------------------------------------------------------------------------------
clk_mcasp0_fixed       0        0        0   24576000     0     0   50000     Y            deviceless                   of_clk_get_from_provider   
			                                                                   deviceless                   no_connection_id   
  clk_mcasp0           0        0        0   24576000     0     0   50000     N              simple-audio-card,cpu        deviceless
											     no_connection_id   	  no_connection_id

Please review the latest patch.

New patch details:

Message ID: <1669569799-8526-1-git-send-email-badolevishal1116@gmail.com>
Subject:	[PATCH v5] Common clock: To list active consumers of clocks

Regards,
Vishal

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

[<Vishal Badole>]

On Sun, Aug 21, 2022 at 05:07:00AM +0000, Elliott, Robert (Servers) wrote:
> 
> 
> > -----Original Message-----
> > From: <Vishal Badole> <badolevishal1116@gmail.com>
> > Sent: Monday, August 8, 2022 12:00 PM
> > To: Elliott, Robert (Servers) <elliott@hpe.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>; mturquette@baylibre.com; inux-
> > clk@vger.kernel.org; linux-kernel@vger.kernel.org; chinmoyghosh2001@gmail.com;
> > mintupatel89@gmail.com; vimal.kumar32@gmail.com
> > Subject: Re: [PATCH] Common clock: ​​To list active consumers of clocks
> > 
> > On Tue, Aug 02, 2022 at 10:49:17PM +0000, Elliott, Robert (Servers) wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: <Vishal Badole> <badolevishal1116@gmail.com>
> > > > Sent: Sunday, June 26, 2022 1:25 PM
> > > > To: Stephen Boyd <sboyd@kernel.org>; mturquette@baylibre.com; inux-
> > > > clk@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > Cc: chinmoyghosh2001@gmail.com; mintupatel89@gmail.com;
> > > > vimal.kumar32@gmail.com
> > > > Subject: Re: [PATCH] Common clock: ​​To list active consumers of clocks
> > > >
> > > ...
> > > > We will remove above prints in the revised patch. We are facing
> > > > indentation issue whle printing consumer in summary
> > > > as given below
> > > >                                  enable  prepare  protect
> > duty  hardware            per-user
> > > >   clock                          count    count    count
> > rateccuracy phase  cycle    enable  consumer   count
> > > >   clk_mcasp0_fixed                   0        0        0
> > 24576000     0  50000         Y
> > > >   deviceless        0
> > >
> > > Consider making the kernel output simple, greppable, and parseable (e.g.,
> > > comma-separated fields, one entry per line, no multi-line column headers)
> > > and let a userspace tool do the fancy formatting.
> > >
> > Hi Robert,
> > We have raised another patch for the same. Please find the below link
> > for reference:
> > 
> > https://www.spinics.net/lists/kernel/msg4459705.html
> 
> That output is still not parsable.
> 
> I suggest making the kernel output more like:
>   clock,enable count,prepare count,protect count,rate,accuracy,phase,duty cycle,hardware enable,consumer,per-user count
>   clk_mcasp0_fixed,0,0,0,24576000,0,0,50000,Y,deviceless,0
>   clk_mcasp0,0,0,0,24576000,0,0,50000,N,simple-audio-card;cpu,0
> 
> and make a userspace program like lsmod, lscpu, lsblk, lspci, 
> or lsusb to print the data with fancy columns or apply
> other filters.
> 
> That allows adding or removing column headers, assuming the
> userspace program doesn't hardcode assumptions about them.
>

Hi Robert,
As per the review given by stephen Boyd, who is one of maintainer of
clk.c, suggested to add consumer's name and per user count in clock
summary only. We are also getting proper formatted and parsable output
on our target board console but when we copy and paste the same in
commit message its format is getting changed. Please apply this patch
and check on your target.

Regards,
Vishal