RE: [PATCH 2/3] Add pm-debug counters
>"ext Peter 'p2' De Schrijver" <[EMAIL PROTECTED]> writes:
>
>> This patch provides the debugfs entries and a function which will be
>> called by the PM code to register the time spent per domain
>per state.
>>
>> Signed-off-by: Peter 'p2' De Schrijver <[EMAIL PROTECTED]>
>> ---
>> arch/arm/mach-omap2/pm-debug.c| 175
>+
>> arch/arm/mach-omap2/pm.h |2 +
>> 2 files changed, 177 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/pm-debug.c
>> b/arch/arm/mach-omap2/pm-debug.c index 0b5c044..4ba6cec 100644
>> --- a/arch/arm/mach-omap2/pm-debug.c
>> +++ b/arch/arm/mach-omap2/pm-debug.c
>> @@ -29,6 +29,8 @@
>>
>> #include
>> #include
>> +#include
>> +#include
>>
>> #include "prm.h"
>> #include "cm.h"
>> @@ -288,4 +290,177 @@ void omap2_pm_dump(int mode, int
>resume, unsigned int us)
>> printk("%-20s: 0x%08x\n", regs[i].name, regs[i].val); }
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +#include
>> +#include
>> +
>> +struct dentry *pm_dbg_dir;
>> +
>> +static int pm_dbg_init_done;
>> +
>> +enum {
>> +DEBUG_FILE_COUNTERS = 0,
>> +DEBUG_FILE_TIMERS,
>> +};
>> +
>> +static const char pwrdm_state_names[][4] = {
>> +"OFF",
>> +"RET",
>> +"INA",
>> +"ON"
>> +};
>> +
>> +void pm_dbg_update_time(struct powerdomain *pwrdm, int prev) {
>> +s64 t;
>> +struct timespec now;
>> +
>> +if (!pm_dbg_init_done)
>> +return ;
>> +
>> +/* Update timer for previous state */
>> +getnstimeofday(&now);
>> +t = timespec_to_ns(&now);
>> +
>> +pwrdm->state_timer[prev] += t - pwrdm->timer;
>> +
>> +pwrdm->timer = t;
>> +}
>> +
>> +static int clkdm_dbg_show_counter(struct clockdomain *clkdm, void
>> +*user) {
>> +struct seq_file *s = (struct seq_file *)user;
>> +
>> +if (strcmp(clkdm->name, "emu_clkdm") == 0 ||
>> +strcmp(clkdm->name, "wkup_clkdm") == 0)
>> +return 0;
>
>Why emu and wkup are not taken into account? If wkup is closed
>out here, I think also prm_clkdm and cm_clkdm should be.
>
>> +
>> +seq_printf(s, "%s->%s (%d)", clkdm->name,
>> +clkdm->pwrdm.ptr->name,
>> +atomic_read(&clkdm->usecount));
>> +seq_printf(s, "\n");
>> +
>> +return 0;
>> +}
>> +
>> +static int pwrdm_dbg_show_counter(struct powerdomain *pwrdm, void
>> +*user) {
>> +struct seq_file *s = (struct seq_file *)user;
>> +int i;
>> +
>> +if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
>> +strcmp(pwrdm->name, "wkup_pwrdm") == 0)
>> +return 0;
>
>Why emu is closed out here? It has pwstst register.
I closed out emu and wkup powerdomains here because they
did not show any useful information. They are missing either
prepwstst or pwstst register and this debug stuff relies on
accessibility of both of them. If you do not have one, you
get "interesting" results (missing prepwstst => OFF count hits
the roof as the software assumes you are entering off mode
during each sleep period, missing pwstst => you are in OFF state
always.) You could show the current state for emu powerdomain yes,
but not counters.
>I think
>also dpll1..5_pwrdm should be closed out here. I'm not sure
>why those are even modelled in a clocktree.
I tracked the powerdomain code a bit, and it seems dpll1...dpll5
are mapped to some real software accessible powerdomains. E.g. we
have dpll1 -> MPU_MOD. Now, a side effect of this is that when you
access such a powerdomain, you may alter the HW registers of another
powerdomain, which you probably did not want to do. Nasty part of this
is that some code does pwrdm_for_each() calls to modify status of
all powerdomains, like pm34xx.c. Question for Paul: Is the powerdomain
code safe against this kind of stuff?
>
>> +
>> +if (pwrdm->state != pwrdm_read_pwrst(pwrdm))
>> +printk(KERN_ERR "pwrdm state mismatch(%s) %d != %d\n",
>> +pwrdm->name, pwrdm->state,
>pwrdm_read_pwrst(pwrdm));
>> +
>> +seq_printf(s, "%s (%s)", pwrdm->name,
>> +pwrdm_state_names[pwrdm->state]);
>> +for (i = 0; i < 4; i++)
>> +seq_printf(s, ",%s:%d", pwrdm_state_names[i],
>> +pwrdm->state_counter[i]);
>> +
>> +seq_printf(s, "\n");
>> +
>> +return 0;
>> +}
>> +
>> +static int pwrdm_dbg_show_timer(struct powerdomain *pwrdm, void
>> +*user) {
>> +struct seq_file *s = (struct seq_file *)user;
>> +int i;
>> +
>> +if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
>> +strcmp(pwrdm->name, "wkup_pwrdm") == 0)
>> +return 0;
>
>Same comment as above.
>
>> +
>> +pwrdm_state_switch(pwrdm);
>> +
>> +seq_printf(s, "%s (%s)", pwrdm->name,
>> +pwrdm_state_names[pwrdm->state]);
>> +
>> +for (i = 0; i < 4; i++)
>> +seq_printf(s, ",%s:%lld", pwrdm_state_names[i],
>> +pwrdm->state_timer[i]);
>> +
>> +seq_printf(s, "\n");
>> +
>> +return 0;
>> +
Re: [PATCH 2/3] Add pm-debug counters
"ext Peter 'p2' De Schrijver" <[EMAIL PROTECTED]> writes:
> This patch provides the debugfs entries and a function which will be called
> by the PM code to register the time spent per domain per state.
>
> Signed-off-by: Peter 'p2' De Schrijver <[EMAIL PROTECTED]>
> ---
> arch/arm/mach-omap2/pm-debug.c| 175
> +
> arch/arm/mach-omap2/pm.h |2 +
> 2 files changed, 177 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index 0b5c044..4ba6cec 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -29,6 +29,8 @@
>
> #include
> #include
> +#include
> +#include
>
> #include "prm.h"
> #include "cm.h"
> @@ -288,4 +290,177 @@ void omap2_pm_dump(int mode, int resume, unsigned int
> us)
> printk("%-20s: 0x%08x\n", regs[i].name, regs[i].val);
> }
>
> +#ifdef CONFIG_DEBUG_FS
> +#include
> +#include
> +
> +struct dentry *pm_dbg_dir;
> +
> +static int pm_dbg_init_done;
> +
> +enum {
> + DEBUG_FILE_COUNTERS = 0,
> + DEBUG_FILE_TIMERS,
> +};
> +
> +static const char pwrdm_state_names[][4] = {
> + "OFF",
> + "RET",
> + "INA",
> + "ON"
> +};
> +
> +void pm_dbg_update_time(struct powerdomain *pwrdm, int prev)
> +{
> + s64 t;
> + struct timespec now;
> +
> + if (!pm_dbg_init_done)
> + return ;
> +
> + /* Update timer for previous state */
> + getnstimeofday(&now);
> + t = timespec_to_ns(&now);
> +
> + pwrdm->state_timer[prev] += t - pwrdm->timer;
> +
> + pwrdm->timer = t;
> +}
> +
> +static int clkdm_dbg_show_counter(struct clockdomain *clkdm, void *user)
> +{
> + struct seq_file *s = (struct seq_file *)user;
> +
> + if (strcmp(clkdm->name, "emu_clkdm") == 0 ||
> + strcmp(clkdm->name, "wkup_clkdm") == 0)
> + return 0;
Why emu and wkup are not taken into account? If wkup is closed out
here, I think also prm_clkdm and cm_clkdm should be.
> +
> + seq_printf(s, "%s->%s (%d)", clkdm->name,
> + clkdm->pwrdm.ptr->name,
> + atomic_read(&clkdm->usecount));
> + seq_printf(s, "\n");
> +
> + return 0;
> +}
> +
> +static int pwrdm_dbg_show_counter(struct powerdomain *pwrdm, void *user)
> +{
> + struct seq_file *s = (struct seq_file *)user;
> + int i;
> +
> + if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
> + strcmp(pwrdm->name, "wkup_pwrdm") == 0)
> + return 0;
Why emu is closed out here? It has pwstst register. I think also
dpll1..5_pwrdm should be closed out here. I'm not sure why those are
even modelled in a clocktree.
> +
> + if (pwrdm->state != pwrdm_read_pwrst(pwrdm))
> + printk(KERN_ERR "pwrdm state mismatch(%s) %d != %d\n",
> + pwrdm->name, pwrdm->state, pwrdm_read_pwrst(pwrdm));
> +
> + seq_printf(s, "%s (%s)", pwrdm->name,
> + pwrdm_state_names[pwrdm->state]);
> + for (i = 0; i < 4; i++)
> + seq_printf(s, ",%s:%d", pwrdm_state_names[i],
> + pwrdm->state_counter[i]);
> +
> + seq_printf(s, "\n");
> +
> + return 0;
> +}
> +
> +static int pwrdm_dbg_show_timer(struct powerdomain *pwrdm, void *user)
> +{
> + struct seq_file *s = (struct seq_file *)user;
> + int i;
> +
> + if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
> + strcmp(pwrdm->name, "wkup_pwrdm") == 0)
> + return 0;
Same comment as above.
> +
> + pwrdm_state_switch(pwrdm);
> +
> + seq_printf(s, "%s (%s)", pwrdm->name,
> + pwrdm_state_names[pwrdm->state]);
> +
> + for (i = 0; i < 4; i++)
> + seq_printf(s, ",%s:%lld", pwrdm_state_names[i],
> + pwrdm->state_timer[i]);
> +
> + seq_printf(s, "\n");
> +
> + return 0;
> +}
> +
> +static void pm_dbg_show_counters(struct seq_file *s, void *unused)
> +{
> + pwrdm_for_each(pwrdm_dbg_show_counter, s);
> + clkdm_for_each(clkdm_dbg_show_counter, s);
> +}
> +
> +static void pm_dbg_show_timers(struct seq_file *s, void *unused)
> +{
> + pwrdm_for_each(pwrdm_dbg_show_timer, s);
> +}
> +
> +static int pm_dbg_open(struct inode *inode, struct file *file)
> +{
> + switch ((int)inode->i_private) {
> + case DEBUG_FILE_COUNTERS:
> + return single_open(file, pm_dbg_show_counters,
> + &inode->i_private);
> + case DEBUG_FILE_TIMERS:
> + default:
> + return single_open(file, pm_dbg_show_timers,
> + &inode->i_private);
> + };
> +}
> +
> +static const struct file_operations debug_fops = {
> + .open = pm_dbg_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release= single_release,
> +};
> +
> +static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
> +{
> + int i;
> + s64 t;
> + struct timespec now;
> +
> +
Re: [PATCH 2/3] Add pm-debug counters
Hi Peter
a few quick comments...
On Tue, 14 Oct 2008, Peter 'p2' De Schrijver wrote:
> This patch provides the debugfs entries and a function which will be called
> by the PM code to register the time spent per domain per state.
>
> Signed-off-by: Peter 'p2' De Schrijver <[EMAIL PROTECTED]>
> ---
> arch/arm/mach-omap2/pm-debug.c| 175
> +
> arch/arm/mach-omap2/pm.h |2 +
> 2 files changed, 177 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index 0b5c044..4ba6cec 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -29,6 +29,8 @@
>
> #include
> #include
> +#include
> +#include
>
> #include "prm.h"
> #include "cm.h"
> @@ -288,4 +290,177 @@ void omap2_pm_dump(int mode, int resume, unsigned int
> us)
> printk("%-20s: 0x%08x\n", regs[i].name, regs[i].val);
> }
>
> +#ifdef CONFIG_DEBUG_FS
> +#include
> +#include
> +
> +struct dentry *pm_dbg_dir;
> +
> +static int pm_dbg_init_done;
> +
> +enum {
> + DEBUG_FILE_COUNTERS = 0,
> + DEBUG_FILE_TIMERS,
> +};
> +
> +static const char pwrdm_state_names[][4] = {
> + "OFF",
> + "RET",
> + "INA",
> + "ON"
> +};
> +
> +void pm_dbg_update_time(struct powerdomain *pwrdm, int prev)
> +{
> + s64 t;
> + struct timespec now;
> +
> + if (!pm_dbg_init_done)
> + return ;
> +
> + /* Update timer for previous state */
> + getnstimeofday(&now);
> + t = timespec_to_ns(&now);
> +
> + pwrdm->state_timer[prev] += t - pwrdm->timer;
> +
> + pwrdm->timer = t;
> +}
> +
> +static int clkdm_dbg_show_counter(struct clockdomain *clkdm, void *user)
> +{
> + struct seq_file *s = (struct seq_file *)user;
> +
> + if (strcmp(clkdm->name, "emu_clkdm") == 0 ||
> + strcmp(clkdm->name, "wkup_clkdm") == 0)
> + return 0;
Is this code intended to skip clockdomains that have no hardware counters?
Maybe we should add a flag in the struct clockdomain for this.
> +
> + seq_printf(s, "%s->%s (%d)", clkdm->name,
> + clkdm->pwrdm.ptr->name,
> + atomic_read(&clkdm->usecount));
> + seq_printf(s, "\n");
> +
> + return 0;
> +}
> +
> +static int pwrdm_dbg_show_counter(struct powerdomain *pwrdm, void *user)
> +{
> + struct seq_file *s = (struct seq_file *)user;
> + int i;
> +
> + if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
> + strcmp(pwrdm->name, "wkup_pwrdm") == 0)
> + return 0;
likewise
> +
> + if (pwrdm->state != pwrdm_read_pwrst(pwrdm))
> + printk(KERN_ERR "pwrdm state mismatch(%s) %d != %d\n",
> + pwrdm->name, pwrdm->state, pwrdm_read_pwrst(pwrdm));
> +
> + seq_printf(s, "%s (%s)", pwrdm->name,
> + pwrdm_state_names[pwrdm->state]);
> + for (i = 0; i < 4; i++)
how about
for (i = 0; i < ARRAY_SIZE(pwrdm_state_names); i++)
?
> + seq_printf(s, ",%s:%d", pwrdm_state_names[i],
> + pwrdm->state_counter[i]);
> +
> + seq_printf(s, "\n");
> +
> + return 0;
> +}
> +
> +static int pwrdm_dbg_show_timer(struct powerdomain *pwrdm, void *user)
> +{
> + struct seq_file *s = (struct seq_file *)user;
> + int i;
> +
> + if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
> + strcmp(pwrdm->name, "wkup_pwrdm") == 0)
> + return 0;
likewise with the clockdomain flag comment
> +
> + pwrdm_state_switch(pwrdm);
> +
> + seq_printf(s, "%s (%s)", pwrdm->name,
> + pwrdm_state_names[pwrdm->state]);
> +
> + for (i = 0; i < 4; i++)
ARRAY_SIZE()
> + seq_printf(s, ",%s:%lld", pwrdm_state_names[i],
> + pwrdm->state_timer[i]);
> +
> + seq_printf(s, "\n");
> +
> + return 0;
> +}
> +
> +static void pm_dbg_show_counters(struct seq_file *s, void *unused)
> +{
> + pwrdm_for_each(pwrdm_dbg_show_counter, s);
> + clkdm_for_each(clkdm_dbg_show_counter, s);
> +}
> +
> +static void pm_dbg_show_timers(struct seq_file *s, void *unused)
> +{
> + pwrdm_for_each(pwrdm_dbg_show_timer, s);
> +}
> +
> +static int pm_dbg_open(struct inode *inode, struct file *file)
> +{
> + switch ((int)inode->i_private) {
> + case DEBUG_FILE_COUNTERS:
> + return single_open(file, pm_dbg_show_counters,
> + &inode->i_private);
> + case DEBUG_FILE_TIMERS:
> + default:
> + return single_open(file, pm_dbg_show_timers,
> + &inode->i_private);
> + };
> +}
> +
> +static const struct file_operations debug_fops = {
> + .open = pm_dbg_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release= single_release,
> +};
> +
> +static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
> +{
> + int i;
> + s64 t;
> + struct timespec now;
> +
> +
