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.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 | 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.k...@canonical.com>wrote:
>
> > 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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
