CC'ing linaro-dev
On Tue, Jan 4, 2011 at 1:18 PM, Yong Shen yong.shen@linaro.org wrote:
hi there,
In last weekly meeting, we talked about the real time display of clock information in powerdebug.
What do you mean by real-time display?
powerdebug will only refresh it's information every 'n' seconds. At that point, the 'show' function for the sysfs rate entry should call get_rate() for the clock.
Actually, this feature can be easily supported. if you found some clocks' information stay unchanged after your drivers change clock setting, it may be caused by the wrong way of clock information exposing.
In the last several weeks, Jeremy and I reviewed the clock debug code based on common clock struct. In this code, I used below code to expose clock information:
+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");
.....
- d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
- &clk_debug_rate_fops);
Therefore, whenever the clock information is accessed, it can reflect the truth, since it calls clk interface like clk_get_rate() to get the right value.
Why is it called clk_debug_rate_get()? Is there not a standard clk_rate_get() that we can use?
Previously, some of you might use a variable to store clock information like rate or count, and expose these variable to sysfs, which is why they are not updated when the real clock information is changing.
Right, storing in variables will give wrong results if you're in the middle of a rate refresh.
On 4 January 2011 09:58, Amit Kucheria amit.kucheria@linaro.org wrote:
CC'ing linaro-dev
On Tue, Jan 4, 2011 at 1:18 PM, Yong Shen yong.shen@linaro.org wrote:
hi there,
In last weekly meeting, we talked about the real time display of clock information in powerdebug.
What do you mean by real-time display?
powerdebug will only refresh it's information every 'n' seconds. At that point, the 'show' function for the sysfs rate entry should call get_rate() for the clock.
Actually, this feature can be easily supported. if you found some clocks' information stay unchanged after your drivers change clock setting, it may be caused by the wrong way of clock information exposing.
In the last several weeks, Jeremy and I reviewed the clock debug code based on common clock struct. In this code, I used below code to expose clock information:
ok for the clock rate but how do you manage a change of the clock's parent ? I mean , if a driver sets a new parent of a clock with clk_set_parent, we should also update the debugfs clock tree in order to reflect this modification. I'm not sure that we need to update the debugfs clock tree in a real time manner but we should be able to force a refresh of the clock debugfs.
+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");
.....
- d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
- &clk_debug_rate_fops);
Therefore, whenever the clock information is accessed, it can reflect the truth, since it calls clk interface like clk_get_rate() to get the right value.
Why is it called clk_debug_rate_get()? Is there not a standard clk_rate_get() that we can use?
Previously, some of you might use a variable to store clock information like rate or count, and expose these variable to sysfs, which is why they are not updated when the real clock information is changing.
Right, storing in variables will give wrong results if you're in the middle of a rate refresh.
linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
On Tue, Jan 4, 2011 at 5:45 PM, Vincent Guittot vincent.guittot@linaro.org wrote:
On 4 January 2011 09:58, Amit Kucheria amit.kucheria@linaro.org wrote:
CC'ing linaro-dev
On Tue, Jan 4, 2011 at 1:18 PM, Yong Shen yong.shen@linaro.org wrote:
hi there,
In last weekly meeting, we talked about the real time display of clock information in powerdebug.
What do you mean by real-time display?
powerdebug will only refresh it's information every 'n' seconds. At that point, the 'show' function for the sysfs rate entry should call get_rate() for the clock.
Actually, this feature can be easily supported. if you found some clocks' information stay unchanged after your drivers change clock setting, it may be caused by the wrong way of clock information exposing.
In the last several weeks, Jeremy and I reviewed the clock debug code based on common clock struct. In this code, I used below code to expose clock information:
ok for the clock rate but how do you manage a change of the clock's parent ? I mean , if a driver sets a new parent of a clock with clk_set_parent, we should also update the debugfs clock tree in order to reflect this modification. I'm not sure that we need to update the debugfs clock tree in a real time manner but we should be able to force a refresh of the clock debugfs.
True. So far, the whole clock tree is build up at clock registering phase, which means it lacks of this ability to display parent change. But IMHO, device drivers are unlikely to change clock's parent at run time, so it is not that urgent to support this 'nice to have' feature. We can put this new function into our future plan.
+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");
.....
- d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
- &clk_debug_rate_fops);
Therefore, whenever the clock information is accessed, it can reflect the truth, since it calls clk interface like clk_get_rate() to get the right value.
Why is it called clk_debug_rate_get()? Is there not a standard clk_rate_get() that we can use?
Previously, some of you might use a variable to store clock information like rate or count, and expose these variable to sysfs, which is why they are not updated when the real clock information is changing.
Right, storing in variables will give wrong results if you're in the middle of a rate refresh.
linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Hi Amit,
Please find out inline feedback.
On Tue, Jan 4, 2011 at 4:58 PM, Amit Kucheria amit.kucheria@linaro.org wrote:
CC'ing linaro-dev
On Tue, Jan 4, 2011 at 1:18 PM, Yong Shen yong.shen@linaro.org wrote:
hi there,
In last weekly meeting, we talked about the real time display of clock information in powerdebug.
What do you mean by real-time display?
powerdebug will only refresh it's information every 'n' seconds. At that point, the 'show' function for the sysfs rate entry should call get_rate() for the clock.
I know this refresh every 'n' seconds. Here, 'real time display' may not be the right word to describe the situation. I meant to say that how powerdebug can reflect the real clock information in time.
Actually, this feature can be easily supported. if you found some clocks' information stay unchanged after your drivers change clock setting, it may be caused by the wrong way of clock information exposing.
In the last several weeks, Jeremy and I reviewed the clock debug code based on common clock struct. In this code, I used below code to expose clock information:
+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");
.....
- d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
- &clk_debug_rate_fops);
Therefore, whenever the clock information is accessed, it can reflect the truth, since it calls clk interface like clk_get_rate() to get the right value.
Why is it called clk_debug_rate_get()? Is there not a standard clk_rate_get() that we can use?
There is. clk_get_rate() is called inside this function. By using these lines of code, the purpose is to advocate using functions provided by clock system like clk_rate_get() directly, instead of using variable to store clock information.
Previously, some of you might use a variable to store clock information like rate or count, and expose these variable to sysfs, which is why they are not updated when the real clock information is changing.
Right, storing in variables will give wrong results if you're in the middle of a rate refresh.
On Tue, Jan 4, 2011 at 3:16 PM, Yong Shen yong.shen@linaro.org wrote:
In the last several weeks, Jeremy and I reviewed the clock debug code based on common clock struct. In this code, I used below code to expose clock information:
+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");
.....
- d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
- &clk_debug_rate_fops);
Therefore, whenever the clock information is accessed, it can reflect the truth, since it calls clk interface like clk_get_rate() to get the right value.
Why is it called clk_debug_rate_get()? Is there not a standard clk_rate_get() that we can use?
There is. clk_get_rate() is called inside this function. By using these lines of code, the purpose is to advocate using functions provided by clock system like clk_rate_get() directly, instead of using variable to store clock information.
I guess I am confused why we don't directly use clk_get_rate() instead of clk_debug_get_rate() in the 'show' call for the sysfs entry.
/Amit
On Tue, Jan 4, 2011 at 7:54 PM, Amit Kucheria amit.kucheria@linaro.org wrote:
On Tue, Jan 4, 2011 at 3:16 PM, Yong Shen yong.shen@linaro.org wrote:
In the last several weeks, Jeremy and I reviewed the clock debug code based on common clock struct. In this code, I used below code to expose clock information:
+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");
.....
- d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
- &clk_debug_rate_fops);
Therefore, whenever the clock information is accessed, it can reflect the truth, since it calls clk interface like clk_get_rate() to get the right value.
Why is it called clk_debug_rate_get()? Is there not a standard clk_rate_get() that we can use?
There is. clk_get_rate() is called inside this function. By using these lines of code, the purpose is to advocate using functions provided by clock system like clk_rate_get() directly, instead of using variable to store clock information.
I guess I am confused why we don't directly use clk_get_rate() instead of clk_debug_get_rate() in the 'show' call for the sysfs entry.
Obviously, the kernel macro "DEFINE_SIMPLE_ATTRIBUTE" only take function definition like: int (*get)(void *, u64 *) which is why clk_get_rate() had been wrapped by clk_debug_get_rate().
Yong
/Amit
-
Amit Kucheria -
Vincent Guittot -
Yong Shen