Hi Jeremy,
In power management group of linaro, we want to export debug information of clock to user space and make a common interface to all the SOC platform. Currently, each SOC platform has their own way to export clock information, like freesale and TI, which is not ideal.
To do so, I need to make somethings done, and some of them are related to common clock code, for which I am more than happy to hear your comments.
1. Create clock information based on common clock device, more specific, based on struct clk_lookup. Since platform drivers are supposed to register their clock using 'void clkdev_add(struct clk_lookup *cl)', clk_lookup should contain enough information for clock display, like clock name and platform clk definition 'struck clk'. An extra field need to be added into clk_lookup is a 'struct dentry *' to store a pointer of the dentry node which will be exported in debug fs, like below: struct clk_lookup { struct list_head node; const char *dev_id; const char *con_id; struct clk *clk; + +#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS) + struct dentry *dent; +#endif };
2. clock names is a little bit fuzzy, cause there are two names in the clk_lookup, they are dev_id and con_id. And there is no name checking in clkdev_add, which means that two clock with the same name could be register into system and it is not reasonable. Also, it is impossible to create clock information with two clocks having the same name, since user will be confused about that. So, I plan to add name checking to avoid name confilt in clkdev_add. To avoid confusion, I am going to use the combination of dev name and connection name togather to display clock. It goes like dev-id_con-id.
3. Currently, 'struct clk' is defined in platform related code, common clk interface did not impose any assumption on how each platform define their own 'struct clk', therefore, clock debug interface can not make any assumption on how much information the platform defined clock contains as well. So far, only clk_get_rate is there as a common defined interface which I can use to export clock rate information, and I plan to add some optional interface like clk_get_count, clk_get_flag, so more useful information can be displayed.
I also attached a draft patch and the clock information displayed on the console is a tree pattern like below(generated by powerdebug, a user space application which can display clock information based on the debug interface I made in the kernel).
|-- NULL-osc <flags=0x0:rate=24000000:usecount=0>
| |-- NULL-spdif_xtal_clk <flags=0x0:rate=24000000:usecount=0>
| | |-- mxc_alsa_spdif.0-NULL <flags=0x0:rate=24000000:usecount=0>
| |-- NULL-usb_phy1_clk <flags=0x0:rate=24000000:usecount=0>
| |-- NULL-lp_apm <flags=0x0:rate=24000000:usecount=0>
| | |-- NULL-ssi_lp_apm_clk <flags=0x0:rate=24000000:usecount=0>
| | | |-- mxc_ssi.1-NULL <flags=0x0:rate=12000000:usecount=0>
| | | | |-- NULL-ssi_ext2_clk <flags=0x0:rate=12000000:usecount=0>
| | | |-- mxc_ssi.0-NULL <flags=0x0:rate=12000000:usecount=0>
| | | | |-- NULL-ssi_ext1_clk <flags=0x0:rate=12000000:usecount=0>
Yong
Hi Yong,
To do so, I need to make somethings done, and some of them are related to common clock code, for which I am more than happy to hear your comments.
Definitely, I'm happy to help out.
- Create clock information based on common clock device, more specific,
based on struct clk_lookup. Since platform drivers are supposed to register their clock using 'void clkdev_add(struct clk_lookup *cl)', clk_lookup should contain enough information for clock display, like clock name and platform clk definition 'struck clk'. An extra field need to be added into clk_lookup is a 'struct dentry *' to store a pointer of the dentry node which will be exported in debug fs, like below: struct clk_lookup { struct list_head node; const char *dev_id; const char *con_id; struct clk *clk;
+#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
struct dentry *dent;+#endif };
I think that struct clk_lookup is the wrong place to put this information; you want to have debug information about the clock, so it should live in struct clk. struct clk_lookup represents a relationship between a clock and (potentially) a device using that clock, and this is why you're having the naming issues.
Unfortunately, that means modifying the 21-or-so different definitions of struct clk. I'm guessing this is why you decided on using clk_lookup instead :)
I'd see this better done as adding a 'const char name[CLK_NAME_LEN]' to struct clk, and populating this in the relevant platform code. Using the common clk API, you'd just have to change the one 'struct clk'. We already have a name parameter to INIT_CLK, so we could trivially set the name:
This would give us:
#define CLK_NAME_LEN 16
struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex; const char name[CLK_NAME_LEN]; };
#define INIT_CLK(name, o) { \ .ops = &o, \ .enable_count = 0, \ .lock.mutex = __MUTEX_INITIALIZER(name.lock.mutex), \ .name = #name, \ }
I think that allocating a dentry per clk_lookup is quite heavyweight; we could just expose one debugfs file, with the whole set of clocks available through this file, one clock per line. The seq_file interface makes this fairly straightforward to do.
- Currently, 'struct clk' is defined in platform related code, common
clk interface did not impose any assumption on how each platform define their own 'struct clk', therefore, clock debug interface can not make any assumption on how much information the platform defined clock contains as well. So far, only clk_get_rate is there as a common defined interface which I can use to export clock rate information, and I plan to add some optional interface like clk_get_count, clk_get_flag, so more useful information can be displayed.
You probably don't need to add the clk_get_count parameter, as this isn't something we would want to expose to drivers through the clk API; instead, just access clk->enable_count directly. With the common clk API, this will be valid on all platforms, as struct clk always has the enable_count. I'm not sure what information you want to expose with clk_get_flag, but the same might apply there too.
If you like, after I've finished some updates to the common clock code, I can put together a patch to deomstrate what I mean. It shouldn't be too long :)
Regards,
Jeremy
Hi Jeremy,
Thanks for comments. Please see my further discussion.
- Create clock information based on common clock device, more specific,
based on struct clk_lookup. Since platform drivers are supposed to
register
their clock using 'void clkdev_add(struct clk_lookup *cl)', clk_lookup should
contain
enough information for clock display, like clock name and platform clk definition 'struck clk'. An extra field need to be added into clk_lookup
is
a 'struct dentry *' to store a pointer of the dentry node which will be exported in debug fs, like below: struct clk_lookup { struct list_head node; const char *dev_id; const char *con_id; struct clk *clk;
+#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
struct dentry *dent;+#endif };
I think that struct clk_lookup is the wrong place to put this information; you want to have debug information about the clock, so it should live in struct clk. struct clk_lookup represents a relationship between a clock and (potentially) a device using that clock, and this is why you're having the naming issues.
True. I misunderstood the purpose of clk_lookup.
Unfortunately, that means modifying the 21-or-so different definitions of struct clk. I'm guessing this is why you decided on using clk_lookup instead :)
This is something I try to avoid. Originally, I thought I could write a unified interface for all the platform without caring about different clock definitions.
I'd see this better done as adding a 'const char name[CLK_NAME_LEN]' to struct clk, and populating this in the relevant platform code. Using the common clk API, you'd just have to change the one 'struct clk'. We already have a name parameter to INIT_CLK, so we could trivially set the name:
This would give us:
#define CLK_NAME_LEN 16
struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex; const char name[CLK_NAME_LEN]; };
#define INIT_CLK(name, o) { \ .ops = &o, \ .enable_count = 0, \ .lock.mutex = __MUTEX_INITIALIZER(name.lock.mutex), \ .name = #name, \ }
This means that you have already forced each SOC to have a unified 'struct
clk', or at least force some fields inside the struct tp be unified, right? My understanding is that leaving 'struct clk' definition to SOC related code is to give freedom for 'struct clk' definition to SOC vendors and they can define their own 'struct clk' as they want. If 'struct clk' has this much in common, why cannot we define it or define some common fields in common code, say, <linux/clk.h> or <asm/clkdev.h>?
I think that allocating a dentry per clk_lookup is quite heavyweight; we
could just expose one debugfs file, with the whole set of clocks available through this file, one clock per line. The seq_file interface makes this fairly straightforward to do.
This is another way to expose information, but we plan to have a more vivid
tree-like clock information, which had been adopted by omap already. The whole clock information is presented in a directory with many sub directories and sub nodes and powerdebug will walk through the directories to create tree-like output.
- Currently, 'struct clk' is defined in platform related code, common
clk interface did not impose any assumption on how each platform define their own 'struct clk', therefore, clock debug interface can not make any assumption on how much information the platform defined clock contains as well. So far, only clk_get_rate is there as a common defined interface which I can use to export clock rate information, and I plan to add some optional interface like clk_get_count, clk_get_flag, so more useful information can be displayed.
You probably don't need to add the clk_get_count parameter, as this isn't something we would want to expose to drivers through the clk API; instead, just access clk->enable_count directly. With the common clk API, this will be valid on all platforms, as struct clk always has the enable_count. I'm not sure what information you want to expose with clk_get_flag, but the same might apply there too.
About what can be exposed, we can decide later, they don't have to be flag or count, but the goal is to provide useful information as we can.
If you like, after I've finished some updates to the common clock code, I can put together a patch to deomstrate what I mean. It shouldn't be too long :)
Can't wait to see that. Can I know a rough schedule of that? Since this is a high priority task for me on the blueprints, I am keen to know more. Maybe if you have draft code done, I can have test as soon as possible.
Regards,
Jeremy
Hi Yong,
This means that you have already forced each SOC to have a unified 'struct clk', or at least force some fields inside the struct tp be unified, right? My understanding is that leaving 'struct clk' definition to SOC related code is to give freedom for 'struct clk' definition to SOC vendors and they can define their own 'struct clk' as they want. If 'struct clk' has this much in common, why cannot we define it or define some common fields in common code, say, <linux/clk.h> or <asm/clkdev.h>?
This is what my 'common struct clk' changes do; the aim is to define struct clk only once, then allow platform-specific code to implement their own clocks by 'subclassing' this structure, as is done in many other places in the kernel.
From the comments of the new struct clk (slightly reformatted):
/** * struct clk - hardware independent clock structure * @clk_ops: implementation-specific ops for this clock * @enable_count: count of clk_enable() calls active on this clock * @mutex: lock for enable/disable or other HW-specific ops * * The base clock object, used by drivers for hardware-independent * manipulation of clock lines. This will be 'subclassed' by device-specific * implementations, which add device-specific data to struct clk. For example: * * struct clk_foo { * struct clk; * [device specific fields] * }; * * The clock driver code will manage the device-specific data, and pass * clk_foo.clk to the common clock code. The clock driver will be called * through the @ops callbacks. * * The @enable_count and @mutex members are initialised when a clock is * registered with the arch-specific clock management code; the clock driver * code does not need to handle these. */ struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex; };
So, the platform-specific code is free to define clocks as necessary, but we keep 'clk' as the structure that contains the platform-independent stuff.
Patches at:
http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git%3Ba=shortlog%3Bh=refs/hea... common
[I'm planning to update this tree soon, so it might be best to hold off for a couple of days if you're intending to work on this branch.]
tree-like clock information, which had been adopted by omap already. The whole clock information is presented in a directory with many sub directories and sub nodes and powerdebug will walk through the directories to create tree-like output.
Are you aiming to be file-level compatible with the omap debug information exported by the kernel?
One flat file will give exactly the same functionality, but with less kernel code. The userspace utils can quite easily parse this file to reconstruct the full clock tree.
Can't wait to see that. Can I know a rough schedule of that? Since this is a high priority task for me on the blueprints, I am keen to know more. Maybe if you have draft code done, I can have test as soon as possible.
I have some pending changes to do for the common clock code, I should have those done by tomorrow. After that, I'll work this example patch, so it should be done by early next week.
However, this is only the clock infrastructure code, not the mx51-port of the common struct clk. The latter needs some updates for recent mx51 clock changes, and I don't have the capacity to do that at the moment. If you'd like to look at the original port, it's in my clk-common-mx51 branch:
http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git%3Ba=shortlog%3Bh=refs/hea... common-mx51
Cheers,
Jeremy
Hi Jeremy,
It's nice to know that your clock update is coming soon. I would not waster your time talking more in detail here now, since everything will be more clear after your code comes out. Thanks again for sharing, and talk to you later.
Yong
On Wed, Nov 10, 2010 at 4:31 PM, Jeremy Kerr jeremy.kerr@canonical.comwrote:
Hi Yong,
This means that you have already forced each SOC to have a unified 'struct clk', or at least force some fields inside the struct tp be
unified,
right? My understanding is that leaving 'struct clk' definition to SOC related code is to give freedom for 'struct clk' definition to SOC vendors and they can define their own 'struct clk' as they want. If 'struct clk' has this much in common, why cannot we define it or define some common fields in common code, say, <linux/clk.h> or <asm/clkdev.h>?
This is what my 'common struct clk' changes do; the aim is to define struct clk only once, then allow platform-specific code to implement their own clocks by 'subclassing' this structure, as is done in many other places in the kernel.
From the comments of the new struct clk (slightly reformatted):
/**
- struct clk - hardware independent clock structure
- @clk_ops: implementation-specific ops for this clock
- @enable_count: count of clk_enable() calls active on this clock
- @mutex: lock for enable/disable or other HW-specific ops
- The base clock object, used by drivers for hardware-independent
- manipulation of clock lines. This will be 'subclassed' by
device-specific
- implementations, which add device-specific data to struct clk. For
example:
- struct clk_foo {
struct clk;[device specific fields]- };
- The clock driver code will manage the device-specific data, and pass
- clk_foo.clk to the common clock code. The clock driver will be called
- through the @ops callbacks.
- The @enable_count and @mutex members are initialised when a clock is
- registered with the arch-specific clock management code; the clock
driver
- code does not need to handle these.
*/ struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex; };
So, the platform-specific code is free to define clocks as necessary, but we keep 'clk' as the structure that contains the platform-independent stuff.
Patches at:
http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git%3Ba=shortlog%3Bh=refs/hea... common
[I'm planning to update this tree soon, so it might be best to hold off for a couple of days if you're intending to work on this branch.]
tree-like clock information, which had been adopted by omap already. The whole clock information is presented in a directory with many sub directories and sub nodes and powerdebug will walk through the
directories
to create tree-like output.
Are you aiming to be file-level compatible with the omap debug information exported by the kernel?
One flat file will give exactly the same functionality, but with less kernel code. The userspace utils can quite easily parse this file to reconstruct the full clock tree.
Can't wait to see that. Can I know a rough schedule of that? Since this
is
a high priority task for me on the blueprints, I am keen to know more. Maybe if you have draft code done, I can have test as soon as possible.
I have some pending changes to do for the common clock code, I should have those done by tomorrow. After that, I'll work this example patch, so it should be done by early next week.
However, this is only the clock infrastructure code, not the mx51-port of the common struct clk. The latter needs some updates for recent mx51 clock changes, and I don't have the capacity to do that at the moment. If you'd like to look at the original port, it's in my clk-common-mx51 branch:
http://kernel.ubuntu.com/git?p=jk/dt/linux-2.6.git%3Ba=shortlog%3Bh=refs/hea... common-mx51
Cheers,
Jeremy
Add a debugfs file to expose system clocks.
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com
--- Yong: as promised, here's the sample debug code for the common struck clk
--- arch/Kconfig | 4 + arch/arm/common/clkdev.c | 2 include/linux/clk.h | 20 ++++++++ kernel/clk.c | 92 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+)
diff --git a/arch/Kconfig b/arch/Kconfig index 212bd3c..8c2e329 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -168,6 +168,10 @@ config HAVE_USER_RETURN_NOTIFIER config USE_COMMON_STRUCT_CLK bool
+config CLK_DEBUG + bool "Expose clock status information in debugfs" + depends on USE_COMMON_STRUCT_CLK && DEBUG_FS + config HAVE_PERF_EVENTS_NMI bool help diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c index e2b2bb6..7524445 100644 --- a/arch/arm/common/clkdev.c +++ b/arch/arm/common/clkdev.c @@ -97,6 +97,7 @@ void clkdev_add(struct clk_lookup *cl) { mutex_lock(&clocks_mutex); list_add_tail(&cl->node, &clocks); + clk_register(cl->clk); mutex_unlock(&clocks_mutex); } EXPORT_SYMBOL(clkdev_add); @@ -106,6 +107,7 @@ void __init clkdev_add_table(struct clk_lookup *cl, size_t num) mutex_lock(&clocks_mutex); while (num--) { list_add_tail(&cl->node, &clocks); + clk_register(cl->clk); cl++; } mutex_unlock(&clocks_mutex); diff --git a/include/linux/clk.h b/include/linux/clk.h index ae7e4ed..d6e64bf 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -20,6 +20,8 @@ struct device;
#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+#define CLK_NAME_LEN 32 + #define CLK_ATOMIC 0x1
/* If we're using the common struct clk, we define the base clk object here */ @@ -64,14 +66,25 @@ struct clk { struct mutex mutex; spinlock_t spinlock; } lock; +#ifdef CONFIG_CLK_DEBUG + const char name[CLK_NAME_LEN]; + struct list_head list; +#endif /* CONFIG_CLK_DEBUG */ };
+#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 */ @@ -80,6 +93,7 @@ struct clk { .enable_count = 0, \ .flags = CLK_ATOMIC, \ .lock.spinlock = __SPIN_LOCK_UNLOCKED(name.lock.spinlock), \ + __INIT_CLK_DEBUG(name) \ }
/** @@ -308,4 +322,10 @@ 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 +extern void clk_register(struct clk *clk); +#else +static inline void clk_register(struct clk *clk) { } +#endif + #endif diff --git a/kernel/clk.c b/kernel/clk.c index 2779abb..1969b0c 100644 --- a/kernel/clk.c +++ b/kernel/clk.c @@ -9,7 +9,11 @@ */
#include <linux/clk.h> +#include <linux/debugfs.h> +#include <linux/fs.h> #include <linux/module.h> +#include <linux/mutex.h> +#include <linux/seq_file.h>
int clk_enable(struct clk *clk) { @@ -112,3 +116,91 @@ struct clk_ops clk_fixed_ops = { .get_rate = clk_fixed_get_rate, }; EXPORT_SYMBOL_GPL(clk_fixed_ops); + +#ifdef CONFIG_CLK_DEBUG +static LIST_HEAD(clocks); +static DEFINE_MUTEX(clocks_lock); + +void clk_register(struct clk *clk) +{ + struct clk *c; + + mutex_lock(&clocks_lock); + + /* Look for duplicates. Since we're being called from clkdev_add, + * we may see multiple clk_lookups for one clock, so seeing the same + * clock is fine. However, warn if we see different clocks with the + * same name */ + list_for_each_entry(c, &clocks, list) { + if (c == clk) + goto out_unlock; + + if (!strcmp(clk->name, c->name)) + pr_warn("clock %s: duplicate name\n", clk->name); + } + + list_add(&clk->list, &clocks); +out_unlock: + mutex_unlock(&clocks_lock); +} + +static void *clk_debug_seq_start(struct seq_file *m, loff_t *pos) +{ + mutex_lock(&clocks_lock); + return seq_list_start(&clocks, *pos); +} + +static void *clk_debug_seq_next(struct seq_file *m, void *v, loff_t *pos) +{ + return seq_list_next(v, &clocks, pos); +} + +static void clk_debug_seq_stop(struct seq_file *m, void *v) +{ + mutex_unlock(&clocks_lock); +} + +static int clk_debug_seq_show(struct seq_file *m, void *v) +{ + struct clk *parent, *clk = list_entry(v, struct clk, list); + const char *parent_name = "root"; + + parent = clk_get_parent(clk); + if (parent && !IS_ERR(parent)) + parent_name = parent->name; + + seq_printf(m, "%s [parent %s] usecount %d rate %lu\n", + clk->name, parent_name, + clk->enable_count, clk_get_rate(clk)); + return 0; +} + +static const struct seq_operations clk_debug_seq_ops = { + .start = clk_debug_seq_start, + .next = clk_debug_seq_next, + .stop = clk_debug_seq_stop, + .show = clk_debug_seq_show, +}; + +static int clk_debug_open(struct inode *inode, struct file *file) +{ + return seq_open(file, &clk_debug_seq_ops); +} + +static const struct file_operations clk_debug_file_ops = { + .open = clk_debug_open, + .read = seq_read, + .llseek = seq_lseek, + .release = seq_release, +}; + +int clk_debug_init(void) +{ + debugfs_create_file("clocks", 0444, NULL, NULL, + &clk_debug_file_ops); + return 0; +} + +late_initcall(clk_debug_init); + +#endif /* CONFIG_CLK_DEBUG */
Hi Jeremy,
Thanks a lot. 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. Below is my draft implementation. Original implementation is from omap platform clock code, and I adopted to common clock device.
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>
#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) +/* + * debugfs support to trace clock tree hierarchy and attributes + */ + +static int open_file(struct inode *inode, struct file *file) +{ + file->private_data = inode->i_private; + return 0; +} + +static ssize_t read_file(struct file *file, char __user *user_buf, + size_t count, loff_t *ppos) +{ + struct clk *clk = file->private_data; + unsigned long rate; + char buf[32]; + ssize_t ret; + + rate = clk_get_rate(clk); + + ret = snprintf(buf, 32, "%d\n", rate); + + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret); + + return ret; +} + +static const struct file_operations rate_fops = { + .open = open_file, + .read = read_file, +}; + +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) + d = debugfs_create_dir(clk ? clk->name : "null", pa->dentry); + else + d = debugfs_create_dir(clk ? clk->name : "null", 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, &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; + + pa = clk_get_parent(clk); + + if ((pa != ERR_PTR(-ENOSYS)) && 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("clock", 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_PM_DEBUG) && defined(CONFIG_DEBUG_FS) */ 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; }
/* Round the requested clock rate to the nearest supported 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 };
-#define INIT_CLK(name, o) { \ +#define INIT_CLK(clk_name, o) { \ .ops = &o, \ .enable_count = 0, \ - .mutex = __MUTEX_INITIALIZER(name.mutex), \ + .mutex = __MUTEX_INITIALIZER(clk_name.mutex), \ + .name = #clk_name, \ }
struct clk_ops {
Yong
On Mon, Nov 15, 2010 at 1:41 PM, Jeremy Kerr jeremy.kerr@canonical.comwrote:
Add a debugfs file to expose system clocks.
Signed-off-by: Jeremy Kerr jeremy.kerr@canonical.com
Yong: as promised, here's the sample debug code for the common struck clk
arch/Kconfig | 4 + arch/arm/common/clkdev.c | 2 include/linux/clk.h | 20 ++++++++ kernel/clk.c | 92 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+)
diff --git a/arch/Kconfig b/arch/Kconfig index 212bd3c..8c2e329 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -168,6 +168,10 @@ config HAVE_USER_RETURN_NOTIFIER config USE_COMMON_STRUCT_CLK bool
+config CLK_DEBUG
bool "Expose clock status information in debugfs"depends on USE_COMMON_STRUCT_CLK && DEBUG_FSconfig HAVE_PERF_EVENTS_NMI bool help diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c index e2b2bb6..7524445 100644 --- a/arch/arm/common/clkdev.c +++ b/arch/arm/common/clkdev.c @@ -97,6 +97,7 @@ void clkdev_add(struct clk_lookup *cl) { mutex_lock(&clocks_mutex); list_add_tail(&cl->node, &clocks);
clk_register(cl->clk); mutex_unlock(&clocks_mutex);} EXPORT_SYMBOL(clkdev_add); @@ -106,6 +107,7 @@ void __init clkdev_add_table(struct clk_lookup *cl, size_t num) mutex_lock(&clocks_mutex); while (num--) { list_add_tail(&cl->node, &clocks);
clk_register(cl->clk); cl++; } mutex_unlock(&clocks_mutex);diff --git a/include/linux/clk.h b/include/linux/clk.h index ae7e4ed..d6e64bf 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -20,6 +20,8 @@ struct device;
#ifdef CONFIG_USE_COMMON_STRUCT_CLK
+#define CLK_NAME_LEN 32
#define CLK_ATOMIC 0x1
/* If we're using the common struct clk, we define the base clk object here */ @@ -64,14 +66,25 @@ struct clk { struct mutex mutex; spinlock_t spinlock; } lock; +#ifdef CONFIG_CLK_DEBUG
const char name[CLK_NAME_LEN];struct list_head list;+#endif /* CONFIG_CLK_DEBUG */ };
+#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 */ @@ -80,6 +93,7 @@ struct clk { .enable_count = 0, \ .flags = CLK_ATOMIC, \ .lock.spinlock = __SPIN_LOCK_UNLOCKED(name.lock.spinlock), \
__INIT_CLK_DEBUG(name) \}
/** @@ -308,4 +322,10 @@ 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 +extern void clk_register(struct clk *clk); +#else +static inline void clk_register(struct clk *clk) { } +#endif
#endif diff --git a/kernel/clk.c b/kernel/clk.c index 2779abb..1969b0c 100644 --- a/kernel/clk.c +++ b/kernel/clk.c @@ -9,7 +9,11 @@ */
#include <linux/clk.h> +#include <linux/debugfs.h> +#include <linux/fs.h> #include <linux/module.h> +#include <linux/mutex.h> +#include <linux/seq_file.h>
int clk_enable(struct clk *clk) { @@ -112,3 +116,91 @@ struct clk_ops clk_fixed_ops = { .get_rate = clk_fixed_get_rate, }; EXPORT_SYMBOL_GPL(clk_fixed_ops);
+#ifdef CONFIG_CLK_DEBUG +static LIST_HEAD(clocks); +static DEFINE_MUTEX(clocks_lock);
+void clk_register(struct clk *clk) +{
struct clk *c;mutex_lock(&clocks_lock);/* Look for duplicates. Since we're being called from clkdev_add,* we may see multiple clk_lookups for one clock, so seeing thesame
* clock is fine. However, warn if we see different clocks with the* same name */list_for_each_entry(c, &clocks, list) {if (c == clk)goto out_unlock;if (!strcmp(clk->name, c->name))pr_warn("clock %s: duplicate name\n", clk->name);}list_add(&clk->list, &clocks);+out_unlock:
mutex_unlock(&clocks_lock);+}
+static void *clk_debug_seq_start(struct seq_file *m, loff_t *pos) +{
mutex_lock(&clocks_lock);return seq_list_start(&clocks, *pos);+}
+static void *clk_debug_seq_next(struct seq_file *m, void *v, loff_t *pos) +{
return seq_list_next(v, &clocks, pos);+}
+static void clk_debug_seq_stop(struct seq_file *m, void *v) +{
mutex_unlock(&clocks_lock);+}
+static int clk_debug_seq_show(struct seq_file *m, void *v) +{
struct clk *parent, *clk = list_entry(v, struct clk, list);const char *parent_name = "root";parent = clk_get_parent(clk);if (parent && !IS_ERR(parent))parent_name = parent->name;seq_printf(m, "%s [parent %s] usecount %d rate %lu\n",clk->name, parent_name,clk->enable_count, clk_get_rate(clk));return 0;+}
+static const struct seq_operations clk_debug_seq_ops = {
.start = clk_debug_seq_start,.next = clk_debug_seq_next,.stop = clk_debug_seq_stop,.show = clk_debug_seq_show,+};
+static int clk_debug_open(struct inode *inode, struct file *file) +{
return seq_open(file, &clk_debug_seq_ops);+}
+static const struct file_operations clk_debug_file_ops = {
.open = clk_debug_open,.read = seq_read,.llseek = seq_lseek,.release = seq_release,+};
+int clk_debug_init(void) +{
debugfs_create_file("clocks", 0444, NULL, NULL,&clk_debug_file_ops);return 0;+}
+late_initcall(clk_debug_init);
+#endif /* CONFIG_CLK_DEBUG */
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
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 *);
On Tue, Nov 16, 2010 at 1:12 PM, Yong Shen yong.shen@linaro.org wrote:
Hi Yong,
After this patch, powerdebug (developed by Amit Arora in power management group of Linaro) generates information on imx51 platform like this:
You seem to be using a bit older version of PowerDebug. Please do a git pull. New features have been added and the display also has been improved. Thanks!
Regards, Amit Arora
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>
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
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; + + 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 */
Hi Yong,
A few more comments:
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
You need to depend on USE_COMMON_STRUCT_CLK (instead of COMMON_CLKDEV) here.
- 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
This is still arm-specific? Looks like you've moved some of the changes to kernel/clk.c, but not all of them.
The only change we should need to the ARM code is to call clk_debug_register in the clkdev_add and clkdev_add_table functions. See my original patch for details.
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
We should probably move this to a separate patch.
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) \
}
Looks better, but you don't need to change the macro argument to clk_name here.
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
You'll need to provide an empty declaration for the !CONFIG_CLK_DEBUG case. Also, I'd probably prefer clk_debug_* rather than clk_debugfs_*; we may add functionality that isn't tied to debugfs in future.
#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
Don't worry about making this conditional on CONFIG_CLK_DEBUG.
Cheers,
Jeremy
Hi Jeremy,
- 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
This is still arm-specific? Looks like you've moved some of the changes to kernel/clk.c, but not all of them.
The only change we should need to the ARM code is to call clk_debug_register in the clkdev_add and clkdev_add_table functions. See my original patch for details.
It is not appropriate to add clk_debug_register in clkdev_add(), since at
the time of clock registering (normally at board initialization or other early stage of boot up), debug fs system has not been initialized (happends in core_initcall). Therefore, it is better to leave it in a standalone function which will be called in late_initcall.
Yong
Hi Yong,
the time of clock registering (normally at board initialization or other early stage of boot up), debug fs system has not been initialized (happends in core_initcall). Therefore, it is better to leave it in a standalone function which will be called in late_initcall.
If you rely on discovering all clocks in an initcall, then you will miss any clocks that are added after this stage (eg, from modules).
I'd suggest allowing clk_debug_register to be called at any time, then you will not have any timing issues.
Regards,
Jeremy
Hi Jeremy,
In that case, I keep both, but add a condition checking in clkdev_add. See below:
From b8e2babd1cc30ffc1ca5fe3cc6a542a18b0ec7fa Mon Sep 17 00:00:00 2001
From: Yong Shen yong.shen@linaro.org Date: Thu, 18 Nov 2010 14:54:49 +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 debug interface.
Signed-off-by: Yong Shen yong.shen@linaro.org --- arch/arm/common/Kconfig | 7 ++++ arch/arm/common/clkdev.c | 28 ++++++++++++++ include/linux/clk.h | 13 +++++++ kernel/clk.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 0 deletions(-)
diff --git a/arch/arm/common/Kconfig b/arch/arm/common/Kconfig index 0a34c81..c84eb90 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 USE_COMMON_STRUCT_CLK && 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..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
#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 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 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); + +#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 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 + #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); #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 + 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; +} +#else /* defined CONFIG_CLK_DEBUG */ +inline int clk_debug_register(struct clk *clk) {} +#endif +EXPORT_SYMBOL_GPL(clk_debug_register);
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
Hi Jeremy,
Your suggestion is better in the architecture view and it is clean and neat, on the other side, mine is only concerning about the minimal changing of previous code. I will try your way and give your feedback.
Yong
On Mon, Nov 22, 2010 at 10:46 AM, Jeremy Kerr jeremy.kerr@canonical.comwrote:
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
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
On 10 Nov 15, Jeremy Kerr wrote:
/* If we're using the common struct clk, we define the base clk object here */ @@ -64,14 +66,25 @@ struct clk { struct mutex mutex; spinlock_t spinlock; } lock; +#ifdef CONFIG_CLK_DEBUG
- const char name[CLK_NAME_LEN];
- struct list_head list;
+#endif /* CONFIG_CLK_DEBUG */ };
So clocks have names only if DEBUG is enabled?
+#ifdef CONFIG_CLK_DEBUG +#define __INIT_CLK_DEBUG(n) .name = #n, +#else +#define __INIT_CLK_DEBUG(n) +#endif
Hi Amit,
@@ -64,14 +66,25 @@ struct clk {
struct mutex mutex; spinlock_t spinlock;} lock;
+#ifdef CONFIG_CLK_DEBUG
- const char name[CLK_NAME_LEN];
- struct list_head list;
+#endif /* CONFIG_CLK_DEBUG */
};
So clocks have names only if DEBUG is enabled?
That's right - we don't (currently) use the clock names for anything other than the debug code.
Cheers,
Jeremy
// snip
This would give us:
#define CLK_NAME_LEN 16
struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex; const char name[CLK_NAME_LEN]; };
#define INIT_CLK(name, o) { \ .ops = &o, \ .enable_count = 0, \ .lock.mutex = __MUTEX_INITIALIZER(name.lock.mutex), \ .name = #name, \ }
I think that allocating a dentry per clk_lookup is quite heavyweight; we could just expose one debugfs file, with the whole set of clocks available through this file, one clock per line. The seq_file interface makes this fairly straightforward to do.
Just a few cents, wouldn't it be good as well to add the clk attribute to each device where clock is associated? It's a bit tricky though as a device could have multiple clocks in theory.
As to the name, I agree with Jeremy that we should have a name field.
#define INIT_CLK(name, o) {
\.ops = &o,\.enable_count = 0,\.lock.mutex = __MUTEX_INITIALIZER(name.lock.mutex), \ .name = #name,\}
I think that allocating a dentry per clk_lookup is quite heavyweight; we
could
just expose one debugfs file, with the whole set of clocks available
through
this file, one clock per line. The seq_file interface makes this fairly straightforward to do.
Just a few cents, wouldn't it be good as well to add the clk attribute to each device where clock is associated? It's a bit tricky though as a device could have multiple clocks in theory.
As to the name, I agree with Jeremy that we should have a name field.
I think the 'struct clock' on most platform has a name field already.
-
Amit Arora -
Amit Kucheria -
Eric Miao -
Jeremy Kerr -
Yong Shen