Hi Jeremy,
Thanks for those comments. I attached a formal patch below for further
review, hope I catched all your points right.
Du to that I am behind a firewall, my free internet access is problematic,
so I send out patch review by copying it to email.
After this patch, powerdebug (developed by Amit Arora in power management
group of Linaro) generates information on imx51 platform like this:
bash-2.05b# /powerdebug -vcd
Clock Tree :
**********
/
|-- kpp_clk.clk <flags=0x0:rate=0:usecount=0>
|-- pll2_sw_clk.clk <flags=0x0:rate=665000000:usecount=0>
| |-- main_bus_clk.clk <flags=0x0:rate=665000000:usecount=0>
| | |-- ahb_clk.clk <flags=0x0:rate=133000000:usecount=0>
| |-- usboh3_clk.clk <flags=0x0:rate=166250000:usecount=0>
|-- hsi2c_clk.clk <flags=0x0:rate=66500000:usecount=0>
|-- i2c2_clk.clk <flags=0x0:rate=66500000:usecount=0>
|-- i2c1_clk.clk <flags=0x0:rate=66500000:usecount=0>
|-- fec_clk.clk <flags=0x0:rate=66500000:usecount=0>
|-- gpt_clk.clk <flags=0x0:rate=66500000:usecount=0>
|-- uart3_clk.clk <flags=0x0:rate=66500000:usecount=0>
|-- uart2_clk.clk <flags=0x0:rate=66500000:usecount=0>
|-- uart1_clk.clk <flags=0x0:rate=66500000:usecount=0>
Patch:
>From 7bb4a43abf0d36a322a61a055f7e7aef18d242ab Mon Sep 17 00:00:00 2001
From: Yong Shen <yong.s...@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.s...@linaro.org>
---
arch/arm/common/Kconfig | 7 +++
arch/arm/common/clkdev.c | 106
++++++++++++++++++++++++++++++++++++++
arch/arm/plat-mxc/clock-common.c | 5 ++-
include/linux/clk.h | 18 ++++++-
4 files changed, 133 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..cf81e70 100644
--- a/arch/arm/common/clkdev.c
+++ b/arch/arm/common/clkdev.c
@@ -20,6 +20,10 @@
#include <linux/clk.h>
#include <linux/slab.h>
+#ifdef CONFIG_CLK_DEBUG
+#include <linux/debugfs.h>
+#endif
+
#include <asm/clkdev.h>
#ifndef CONFIG_USE_COMMON_STRUCT_CLK
@@ -186,3 +190,105 @@ void clkdev_drop(struct clk_lookup *cl)
kfree(cl);
}
EXPORT_SYMBOL(clkdev_drop);
+
+#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 struct dentry *clk_debugfs_root;
+
+static int clk_debugfs_register_one(struct clk *clk)
+{
+ 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, clk_debugfs_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;
+}
+
+static int clk_debugfs_register(struct clk *clk)
+{
+ int err;
+ struct clk *pa;
+
+ pa = clk_get_parent(clk);
+
+ if (pa && !IS_ERR(pa) && !pa->dentry) {
+ err = clk_debugfs_register(pa);
+ if (err)
+ return err;
+ }
+
+ if (!clk->dentry) {
+ err = clk_debugfs_register_one(clk);
+ if (err)
+ return err;
+ }
+ return 0;
+}
+
+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;
+ clk_debugfs_root = d;
+
+ list_for_each_entry(cl, &clocks, node) {
+ err = clk_debugfs_register(cl->clk);
+ if (err)
+ goto err_out;
+ }
+ return 0;
+err_out:
+ debugfs_remove_recursive(clk_debugfs_root);
+ return err;
+}
+late_initcall(clk_debugfs_init);
+
+#endif /* defined CONFIG_CLK_DEBUG */
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
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
struct clk_ops {
int (*enable)(struct clk *);
--
1.7.0.4
Thanks
Yong
On Mon, Nov 15, 2010 at 5:05 PM, Jeremy Kerr <jeremy.k...@canonical.com>wrote:
> Hi Yong,
>
> > Yes, it's also nice to have a file containing all the clock information
> > which you have implemented in the email.
> > Since we expect more features like enable/disable clocks in the debugfs,
> we
> > also like to have tree-like debugfs for clock information.
>
> Sure; if you want the extended functionality, I think your tree approach is
> best.
>
> > Below is my draft implementation. Original implementation is from omap
> > platform clock code, and I adopted to common clock device.
>
> A couple of comments inline...
>
> > diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c
> > index 9e4c4d9..e7a629a 100644
> > --- a/arch/arm/common/clkdev.c
> > +++ b/arch/arm/common/clkdev.c
> > @@ -19,6 +19,7 @@
> >
> > #include <linux/mutex.h>
> > #include <linux/clk.h>
> > #include <linux/slab.h>
> >
> > +#include <linux/debugfs.h>
>
> Minor nitpick: no need to put this #inlude on its own.
>
> >
> > #include <asm/clkdev.h>
> >
> > @@ -186,3 +187,128 @@ void clkdev_drop(struct clk_lookup *cl)
> >
> > kfree(cl);
> >
> > }
> > EXPORT_SYMBOL(clkdev_drop);
> >
> > +
> > +#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
>
> Might be good to add CONFIG_CLK_DEBUG (which would depend on
> CONFIG_PM_DEBUG
> and CONFIG_DEBUG_FS), so we can enable/disable it separately.
>
> > +/*
> > + * debugfs support to trace clock tree hierarchy and attributes
> > + */
> > +
> > +static int open_file(struct inode *inode, struct file *file)
>
> I think you can replace this (and read_file, and rate_fops) with something
> like:
>
> 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, "%lu\n");
>
> and then in clk_debugfs_register_one:
>
> debugfs_create_file("rate", S_IRUGO, clk->dentry,
> clk, &clk_debug_rate_fops);
>
>
> > +static struct dentry *clk_debugfs_root;
> > +
> > +static int clk_debugfs_register_one(struct clk *clk)
> > +{
> > + int err;
> > + struct dentry *d, *child, *child_tmp;
> > + struct clk *pa = clk_get_parent(clk);
> > +
> > + if ((pa != ERR_PTR(-ENOSYS)) && pa)
>
> I'd suggest:
>
> if (pa && !IS_ERR(pa))
>
> > + d = debugfs_create_dir(clk ? clk->name : "null", pa->dentry);
> > + else
> > + d = debugfs_create_dir(clk ? clk->name : "null", clk_debugfs_root);
>
> Why do you need to check for clk == NULL here? We probably don't want
> 'null'
> directories hanging around.
>
> > +
> > + 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,
> > &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;
> > +}
> > +
> > +static int clk_debugfs_register(struct clk *clk)
> > +{
> > + int err;
> > + struct clk *pa;
> > +
> > + if (clk == ERR_PTR(-ENOSYS))
> > + return -1;
> > +
> > + if (!clk)
> > + return -1;
>
> Do we need these checks?
>
> > +
> > + pa = clk_get_parent(clk);
> > +
> > + if ((pa != ERR_PTR(-ENOSYS)) && pa && !pa->dentry) {
>
> Again, this might be better:
>
> if (pa && !IS_ERR(pa) && !pa->dentry)
>
> .. but this makes me think we should probably drop the -ENOSYS return code
> from clk_get_parent, but that's a patch for another time.. :)
>
> > + err = clk_debugfs_register(pa);
> > + if (err)
> > + return err;
> > + }
> > +
> > + if (!clk->dentry) {
> > + err = clk_debugfs_register_one(clk);
> > + if (err)
> > + return err;
> > + }
> > + return 0;
> > +}
> > +
> > +static int __init clk_debugfs_init(void)
> > +{
> > + struct clk_lookup *cl;
> > + struct dentry *d;
> > + int err;
> > +
> > + d = debugfs_create_dir("clock", NULL);
>
> I'd suggest "clocks" instead of "clock", but this is fairly minor.
>
>
> > diff --git a/arch/arm/plat-mxc/clock-common.c
> > b/arch/arm/plat-mxc/clock-common.c
> > index 8911813..96e19a4 100644
> > --- a/arch/arm/plat-mxc/clock-common.c
> > +++ b/arch/arm/plat-mxc/clock-common.c
> > @@ -93,11 +93,14 @@ void clk_mxc_disable(struct clk *_clk)
> >
> > unsigned long clk_mxc_get_rate(struct clk *_clk)
> > {
> >
> > struct clk_mxc *clk = to_clk_mxc(_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;
>
> I see you found this oops too :)
>
>
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 56416b7..0dc3443 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -44,16 +44,22 @@ 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
> >
> > struct clk {
> >
> > const struct clk_ops *ops;
> > unsigned int enable_count;
> > struct mutex mutex;
> >
> > + char name[CLK_NAME_LEN];
> > +#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
> > + struct dentry *dentry;
> > +#endif
>
> Probably best to put 'name' into the CLK_DEBUG case; then you'll just need
> to
> use the __INIT_CLK_DEBUG macros in my previous patch.
>
> BTW, the spacing in the patch seems to be a little off; not sure if this is
> due to your mailer or the patch itself. If it's the latter, you might want
> to
> clean this up before submitting.
>
> Cheers,
>
>
> Jeremy
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
