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.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         |  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.kerr@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