Re: [PATCH] clocks: add clock debugging file

On 10 Nov 17, Yong Shen wrote:

Hi Jeremy,

Great ideal to make it beyond ARM specific. Below is the updated patch as per your comments.

From 5a3e2d3bf577e3059c9254a61d671910ab170623 Mon Sep 17 00:00:00 2001 From: Yong Shen yong.shen@linaro.org Date: Tue, 16 Nov 2010 15:00:28 +0800 Subject: [PATCH] export clock debug information to user space

create a tree-like directory structure in debugfs so user space tools like powerdebug can generate readable clock information. more functions tend to be add in, like individual clock enable/disable by writing to this debugfs.

Signed-off-by: Yong Shen yong.shen@linaro.org

arch/arm/common/Kconfig | 7 +++ arch/arm/common/clkdev.c | 28 +++++++++++++ arch/arm/plat-mxc/clock-common.c | 5 ++- include/linux/clk.h | 19 ++++++++- kernel/clk.c | 83 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 139 insertions(+), 3 deletions(-)

diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig index 0a34c81..0300c95 100644 --- a/arch/arm/common/Kconfig +++ b/arch/arm/common/Kconfig @@ -41,3 +41,10 @@ config SHARP_SCOOP config COMMON_CLKDEV bool select HAVE_CLK

+config CLK_DEBUG

• bool "clock debug information export to user space"
• depends on COMMON_CLKDEV && PM_DEBUG && DEBUG_FS
• default n
• help
• export clk debug information to user space

diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c index 9e4c4d9..5f4a309 100644 --- a/arch/arm/common/clkdev.c +++ b/arch/arm/common/clkdev.c @@ -19,6 +19,9 @@ #include <linux/mutex.h> #include <linux/clk.h> #include <linux/slab.h> +#ifdef CONFIG_CLK_DEBUG +#include <linux/debugfs.h> +#endif

#include <asm/clkdev.h>

@@ -186,3 +189,28 @@ void clkdev_drop(struct clk_lookup *cl) kfree(cl); } EXPORT_SYMBOL(clkdev_drop);

+#ifdef CONFIG_CLK_DEBUG +static int __init clk_debugfs_init(void) +{

• struct clk_lookup *cl;
• struct dentry *d;
• int err;
• d = debugfs_create_dir("clocks", NULL);
• if (!d)
• return -ENOMEM;

Is mutt going crazy or is the indentation here completely off?

The same thing throughout the patch.

• list_for_each_entry(cl, &clocks, node) {
• err = clk_debugfs_register(cl->clk, d);
• if (err)
• goto err_out;
• }
• return 0;

+err_out:

• debugfs_remove_recursive(d);
• return err;

+} +late_initcall(clk_debugfs_init);

+#endif diff --git a/arch/arm/plat-mxc/clock-common.c b/arch/arm/plat-mxc/clock-common.c index 8911813..20d38a8 100644 --- a/arch/arm/plat-mxc/clock-common.c +++ b/arch/arm/plat-mxc/clock-common.c @@ -97,7 +97,10 @@ unsigned long clk_mxc_get_rate(struct clk *_clk) if (clk->get_rate) return clk->get_rate(clk);

• return clk_get_rate(clk->parent);

• if (clk->parent)
• return clk_get_rate(clk->parent);
• return 0;

}

/* Round the requested clock rate to the nearest supported diff --git a/include/linux/clk.h b/include/linux/clk.h index 56416b7..751c086 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -44,16 +44,28 @@ struct device;

• registered with the arch-specific clock management code; the clock

driver

• code does not need to handle these.

*/ +#define CLK_NAME_LEN 32 struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex; +#ifdef CONFIG_CLK_DEBUG

• char name[CLK_NAME_LEN];
• struct dentry *dentry;

+#endif };

-#define INIT_CLK(name, o) { \ +#ifdef CONFIG_CLK_DEBUG +#define __INIT_CLK_DEBUG(n) .name = #n, +#else +#define __INIT_CLK_DEBUG(n) +#endif

+#define INIT_CLK(clk_name, o) { \ .ops = &o, \ .enable_count = 0, \

• .mutex = __MUTEX_INITIALIZER(name.mutex), \

• .mutex = __MUTEX_INITIALIZER(clk_name.mutex), \
• __INIT_CLK_DEBUG(clk_name) \

}

struct clk_ops { @@ -245,4 +257,7 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id); int clk_add_alias(const char *alias, const char *alias_dev_name, char *id, struct device *dev);

+#ifdef CONFIG_CLK_DEBUG +int clk_debugfs_register(struct clk *clk, struct dentry *root); +#endif #endif diff --git a/kernel/clk.c b/kernel/clk.c index 32f25ef..97e5e66 100644 --- a/kernel/clk.c +++ b/kernel/clk.c @@ -12,6 +12,10 @@ #include <linux/mutex.h> #include <linux/module.h>

+#ifdef CONFIG_CLK_DEBUG +#include <linux/debugfs.h> +#endif

int clk_enable(struct clk *clk) { int ret = 0; @@ -113,3 +117,82 @@ struct clk_ops clk_fixed_ops = { .get_rate = clk_fixed_get_rate, }; EXPORT_SYMBOL_GPL(clk_fixed_ops);

+#ifdef CONFIG_CLK_DEBUG +/*

• • debugfs support to trace clock tree hierarchy and attributes
• */

+static int clk_debug_rate_get(void *data, u64 *val) +{

• struct clk *clk = data;
• *val = (u64)clk_get_rate(clk);
• return 0;

+} +DEFINE_SIMPLE_ATTRIBUTE(clk_debug_rate_fops, clk_debug_rate_get, NULL,

• "%llu\n");

+static int clk_debugfs_register_one(struct clk *clk, struct dentry *root) +{

• int err;
• struct dentry *d, *child, *child_tmp;
• struct clk *pa = clk_get_parent(clk);
• if (pa && !IS_ERR(pa))
• d = debugfs_create_dir(clk->name, pa->dentry);
• else
• d = debugfs_create_dir(clk->name, root);
• if (!d)
• return -ENOMEM;
• clk->dentry = d;
• d = debugfs_create_u32("enable_count", S_IRUGO, clk->dentry,
• (u32 *)&clk->enable_count);
• if (!d) {
• err = -ENOMEM;
• goto err_out;
• }
• d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
• &clk_debug_rate_fops);
• if (!d) {
• err = -ENOMEM;
• goto err_out;
• }
• return 0;

+err_out:

• d = clk->dentry;
• list_for_each_entry_safe(child, child_tmp, &d->d_subdirs, d_u.d_child)
• debugfs_remove(child);
• debugfs_remove(clk->dentry);
• return err;

+}

+int clk_debugfs_register(struct clk *clk, struct dentry *root) +{

• int err;
• struct clk *pa;
• pa = clk_get_parent(clk);
• if (pa && !IS_ERR(pa) && !pa->dentry) {
• err = clk_debugfs_register(pa, root);
• if (err)
• return err;
• }
• if (!clk->dentry) {
• err = clk_debugfs_register_one(clk, root);
• if (err)
• return err;
• }
• return 0;

+} +EXPORT_SYMBOL_GPL(clk_debugfs_register);

+#endif /* defined CONFIG_CLK_DEBUG */

1.7.0.4

Cheers Yong

On Tue, Nov 16, 2010 at 5:16 PM, Jeremy Kerr jeremy.kerr@canonical.comwrote:

...

Hi Yong,

Looks good, just a couple of things:

...

diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c index 9e4c4d9..cf81e70 100644 --- a/arch/arm/common/clkdev.c +++ b/arch/arm/common/clkdev.c

Why not do this in the core clock code (kernel/clk.c) instead? No need to make it ARM-specific.

...

diff --git a/arch/arm/plat-mxc/clock-common.c b/arch/arm/plat-mxc/clock-common.c index 8911813..20d38a8 100644 --- a/arch/arm/plat-mxc/clock-common.c +++ b/arch/arm/plat-mxc/clock-common.c @@ -97,7 +97,10 @@ unsigned long clk_mxc_get_rate(struct clk *_clk) if (clk->get_rate) return clk->get_rate(clk);

• return clk_get_rate(clk->parent);

• if (clk->parent)
• return clk_get_rate(clk->parent);
• return 0;

}

/* Round the requested clock rate to the nearest supported diff --git a/include/linux/clk.h b/include/linux/clk.h index 56416b7..1102c2c 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -44,17 +44,31 @@ struct device;

• registered with the arch-specific clock management code; the clock

driver

• code does not need to handle these.

*/ +#define CLK_NAME_LEN 16

We might need more than 16 bytes here; 32 should be fine.

...

struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex; +#ifdef CONFIG_CLK_DEBUG

• char name[CLK_NAME_LEN];
• struct dentry *dentry;

+#endif };

-#define INIT_CLK(name, o) { \ +#ifndef CONFIG_CLK_DEBUG +#define INIT_CLK(clk_name, o) { \ .ops = &o, \ .enable_count = 0, \

• .mutex = __MUTEX_INITIALIZER(name.mutex), \

• .mutex = __MUTEX_INITIALIZER(clk_name.mutex), \

} +#else +#define INIT_CLK(clk_name, o) { \

• .ops = &o, \
• .enable_count = 0, \
• .mutex = __MUTEX_INITIALIZER(clk_name.mutex), \
• .name = #clk_name, \

+} +#endif

How about this instead:

+#ifdef CONFIG_CLK_DEBUG +#define __INIT_CLK_DEBUG(n) .name = #n, +#else +#define __INIT_CLK_DEBUG(n) +#endif

/* static initialiser for non-atomic clocks */ #define INIT_CLK(name, o) { \ .ops = &o, \ .enable_count = 0, \ .flags = 0, \ .lock.mutex = __MUTEX_INITIALIZER(name.lock.mutex), \

•   __INIT_CLK_DEBUG(name)                                          \
  

}

/* static initialiser for atomic clocks */

• otherwise, you're going to need to add another definition of

INIT_CLK_ATOMIC too, giving four in total.

Cheers,

Jeremy

---

linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev