Yong

On Mon, Nov 22, 2010 at 10:46 AM, Jeremy Kerr <jeremy.kerr@canonical.com> wrote:

Hi Yong,

> diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c
> index 9e4c4d9..936684f 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

With changes suggested below, we won't need this

>
> #include <asm/clkdev.h>
>
> @@ -104,6 +107,10 @@ EXPORT_SYMBOL(clk_put);
>
> void clkdev_add(struct clk_lookup *cl)
> {
> +#ifdef CONFIG_CLK_DEBUG
> + if (debugfs_initialized())
> + clk_debug_register(cl->clk);
> +#endif

Don't make this conditional on CONFIG_CLK_DEBUG. the clk_debug_register should
do this checking, and for the !CONFIG_CLK_DEBUG case, clk_debug_register
should be an empty inline.

> mutex_lock(&clocks_mutex);
> list_add_tail(&cl->node, &clocks);
> mutex_unlock(&clocks_mutex);
> @@ -114,6 +121,10 @@ void __init clkdev_add_table(struct clk_lookup *cl,
> size_t num)
> {
> mutex_lock(&clocks_mutex);
> while (num--) {
> +#ifdef CONFIG_CLK_DEBUG
> + if (debugfs_initialized())
> + clk_debug_register(cl->clk);
> +#endif

Same here.

> list_add_tail(&cl->node, &clocks);
> cl++;
> }
> @@ -186,3 +197,20 @@ 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;
> + int err;
> +
> + list_for_each_entry(cl, &clocks, node) {
> + err = clk_debug_register(cl->clk);
> + if (err)
> + return err;
> + }
> + return 0;
> +}
> +late_initcall(clk_debugfs_init);

Would be much better to contain all the debugging functionality in
kernel/clk.c. I think we'd be better of exposing a single function:

clk_debug_register(struct clk *);

- and implementing the details in kernel/clk.c. The caller should not have to
care about the internal implementation details of the debug code (ie, that
debugfs is not initialised).

So, this function must be able to register clocks at any time.

> +
> +#endif
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 56416b7..5a0139c 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

I'd put this before the above comment, so the kerneldoc is properly located.

> 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
> };
>
> +#ifdef CONFIG_CLK_DEBUG
> +#define __INIT_CLK_DEBUG(n) .name = #n,
> +#else
> +#define __INIT_CLK_DEBUG(n)
> +#endif

looks good :)

> +
> #define INIT_CLK(name, o) { \
> .ops = &o, \
> .enable_count = 0, \
> .mutex = __MUTEX_INITIALIZER(name.mutex), \
> + __INIT_CLK_DEBUG(name) \
> }
>
> struct clk_ops {
> @@ -245,4 +257,5 @@ 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);
>
> +int clk_debug_register(struct clk *clk);

We need the inline here too:

#ifdef CONFIG_CLK_DEBUG
extern int clk_debug_register(struct clk *clk);
#else
static inline int clk_debug_register(struct clk *clk) { return 0; }
#endif

Also, we should probably make this return void; it's unlikely that callers are
going to check (or care about) the return value here.

> #endif
> diff --git a/kernel/clk.c b/kernel/clk.c
> index 32f25ef..df4da68 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

Just include unconditionally.

> +
> int clk_enable(struct clk *clk)
> {
> int ret = 0;
> @@ -113,3 +117,89 @@ 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 struct dentry *clk_root;
> +static int clk_debug_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 {
> + if (!clk_root)
> + clk_root = debugfs_create_dir("clocks", NULL);
> + if (!clk_root)
> + return -ENOMEM;
> + d = debugfs_create_dir(clk->name, clk_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_debug_register(struct clk *clk)
> +{
> + int err;
> + struct clk *pa;
> +
> + pa = clk_get_parent(clk);
> +
> + if (pa && !IS_ERR(pa) && !pa->dentry) {
> + err = clk_debug_register(pa);
> + if (err)
> + return err;
> + }
> +
> + if (!clk->dentry) {
> + err = clk_debug_register_one(clk);
> + if (err)
> + return err;
> + }
> + return 0;
> +}

How about something like:

struct preinit_clk {
struct list_head list;
struct clk *clk;
};

static LIST_HEAD(preinit_clks);
static DEFINE_MUTEX(preinit_lock);
static int init_done;

void clk_debug_register(struct clk *clk)
{
mutex_lock(&preinit_lock);
if (init_done) {
/* normal register path ... */
} else {
struct preinit_clk *p;

p = kmalloc(sizeof(*p), GFP_KERNEL);
if (!p)
goto out_unlock;
p->clk = clk;
list_add(&p->list, &preinit_clks);
}

out_unlock:
mutex_unlock(&preinit_lock);
}

and then in clk_debug_init, we add all the clocks from preinit_list, then set
init_done = 1. Calls to clk_debug_register after clk_debug_init() will have
the dentry set up directly.

This way, the caller does not have to care about the state of the clk_debug
code.

How does this sound?

> +#else /* defined CONFIG_CLK_DEBUG */
> +inline int clk_debug_register(struct clk *clk) {}

We'll define this in the header, so you can remove this definition.

> +#endif
> +EXPORT_SYMBOL_GPL(clk_debug_register);

Cheers,

Jeremy