2022-06-10 19:47:17

by Stephen Boyd

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

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 <[email protected]>
> Signed-off-by: Chinmoy Ghosh <[email protected]>
> Signed-off-by: Mintu Patel <[email protected]>
> Signed-off-by: Vimal Kumar <[email protected]>

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);


2022-06-22 16:45:36

by <Vishal Badole>

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


From eba241016ea868b841473ba73ece16173a6b5aee Mon Sep 17 00:00:00 2001
From: Vishal Badole <[email protected]>
Date: Tue, 31 May 2022 21:23:34 +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.

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 <[email protected]>
Signed-off-by: Chinmoy Ghosh <[email protected]>
Signed-off-by: Mintu Patel <[email protected]>
Signed-off-by: Vimal Kumar <[email protected]>
---
drivers/clk/Kconfig | 8 ++++
drivers/clk/clk.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 127 insertions(+), 1 deletion(-)

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"
+ 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*/
+ 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
}
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) {
+ 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,
+ &clk_devices_fops);
+#endif
mutex_lock(&clk_debug_lock);
hlist_for_each_entry(core, &clk_debug_list, debug_node)
clk_debug_create_one(core, rootdir);
--
2.7.4

2022-06-22 17:09:07

by <Vishal Badole>

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


From f2e9d78bd0f135206fbfbf2e0178a5782b972939 Mon Sep 17 00:00:00 2001
From: Vishal Badole <[email protected]>
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 <[email protected]>
Signed-off-by: Chinmoy Ghosh <[email protected]>
Co-developed-by: Mintu Patel <[email protected]>
Signed-off-by: Mintu Patel <[email protected]>
Acked-by: Vimal Kumar <[email protected]>
Signed-off-by: Vishal Badole <[email protected]>
---
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

2022-06-22 19:42:10

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH] Common clock: ​​To list ac tive consumers of clocks



On 6/22/22 09:32, <Vishal Badole> wrote:
> 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"
> + depends on DEBUG_FS && COMMON_CLK
> + help
> + Clock consumer debug feature supports for clock debugging. Chose y

Choose y

> + to get debug entry in file system to list clocks and their active
> + consumer devices.

--
~Randy