Re: [PATCH 2/3] perf: add OMAP support for the new power events
On Wednesday 05 January 2011 12:05:18 Jean Pihet wrote:
Hi Paul,
On Tue, Jan 4, 2011 at 7:48 PM, Paul Walmsley p...@pwsan.com wrote:
Hello Jean,
On Tue, 4 Jan 2011, jean.pi...@newoldbits.com wrote:
From: Jean Pihet j-pi...@ti.com
The patch adds the new power management trace points for
the OMAP architecture.
The trace points are for:
- default idle handler. Since the cpuidle framework is
instrumented in the generic way there is no need to
add trace points in the OMAP specific cpuidle handler;
- cpufreq (DVFS),
- clocks changes (enable, disable, set_rate),
A question about these. Are these only meant to track calls to these
functions from outside the clock code? Or meant to track actual hardware
clock changes?
The former: this is used to track the clock requests from outside the
clock framework.
If the latter, then it might make sense to put these
trace points into the functions that actually change the hardware
registers, e.g., omap2_dflt_clk_{enable,disable}(), etc., since a
clk_enable() on a leaf clock may result in many internal system clocks
being enabled up the clock tree.
I agree with you it is better to track the actual clock changes instead.
I propose to move the tracepoints to omap2_clk_{enable...} which
enables all the clocks irrespectively of the installed handler.
Note about the clock handlers: omap2_dflt_clk_enable happens to be the
handler for all controllable clocks but could that change in the
future?
Looks like there is cpuidle34xx using cpuidle framework on specific boards
only.
And pm34xx.c and others override pm_idle and use the same low level
functions to reduce power consumption as cpuidle34xx.
Ideally pm34xx.c (and others) would not override pm_idle, but register as
a cpuidle driver. Then the idle events would be tracked by the
cpuidle subsystem automatically (with my latest patches).
But this would be a more intrusive change (are there efforts to do that?).
Even if it should happen at some point, adding some additional events for
people to better debug/monitor the stuff now does not hurt.
Thomas
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] perf: add OMAP support for the new power events
Hello Jean,
On Wed, 5 Jan 2011, Jean Pihet wrote:
On Tue, Jan 4, 2011 at 7:48 PM, Paul Walmsley p...@pwsan.com wrote:
If the latter, then it might make sense to put these
trace points into the functions that actually change the hardware
registers, e.g., omap2_dflt_clk_{enable,disable}(), etc., since a
clk_enable() on a leaf clock may result in many internal system clocks
being enabled up the clock tree.
I agree with you it is better to track the actual clock changes instead.
I propose to move the tracepoints to omap2_clk_{enable...} which
enables all the clocks irrespectively of the installed handler.
Yes, that is a better place.
Note about the clock handlers: omap2_dflt_clk_enable happens to be the
handler for all controllable clocks but could that change in the
future?
Yes, it certainly could.
- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] perf: add OMAP support for the new power events
Thomas Renninger tr...@suse.de writes:
On Wednesday 05 January 2011 12:05:18 Jean Pihet wrote:
Hi Paul,
On Tue, Jan 4, 2011 at 7:48 PM, Paul Walmsley p...@pwsan.com wrote:
Hello Jean,
On Tue, 4 Jan 2011, jean.pi...@newoldbits.com wrote:
From: Jean Pihet j-pi...@ti.com
The patch adds the new power management trace points for
the OMAP architecture.
The trace points are for:
- default idle handler. Since the cpuidle framework is
instrumented in the generic way there is no need to
add trace points in the OMAP specific cpuidle handler;
- cpufreq (DVFS),
- clocks changes (enable, disable, set_rate),
A question about these. Are these only meant to track calls to these
functions from outside the clock code? Or meant to track actual hardware
clock changes?
The former: this is used to track the clock requests from outside the
clock framework.
If the latter, then it might make sense to put these
trace points into the functions that actually change the hardware
registers, e.g., omap2_dflt_clk_{enable,disable}(), etc., since a
clk_enable() on a leaf clock may result in many internal system clocks
being enabled up the clock tree.
I agree with you it is better to track the actual clock changes instead.
I propose to move the tracepoints to omap2_clk_{enable...} which
enables all the clocks irrespectively of the installed handler.
Note about the clock handlers: omap2_dflt_clk_enable happens to be the
handler for all controllable clocks but could that change in the
future?
Looks like there is cpuidle34xx using cpuidle framework on specific
boards only. And pm34xx.c and others override pm_idle and use the
same low level functions to reduce power consumption as cpuidle34xx.
Ideally pm34xx.c (and others) would not override pm_idle, but register as
a cpuidle driver. Then the idle events would be tracked by the
cpuidle subsystem automatically (with my latest patches).
But this would be a more intrusive change (are there efforts to do that?).
Whenever CPUidle is enabled though, the cpuidle34xx code is used so the
pm_idle in pm34xx is not used. This allows us to have a way to do PM
with and without CPUidle, so without CPUidle, we can still get some
basic PM in idle.
Even if it should happen at some point, adding some additional events for
people to better debug/monitor the stuff now does not hurt.
I agree, as the two both very useful.
Tracking CPUidle transitions gives us some high-level information on PM
transitions, but what is most interesting for real PM analysis on OMAP
is exactly which powerdomains, clockdomains and clocks transitions (or
didn't transition) for a given state.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] perf: add OMAP support for the new power events
On Tue, Jan 4, 2011 at 7:03 PM, Nishanth Menon n...@ti.com wrote: jean.pi...@newoldbits.com had written, on 01/04/2011 04:17 AM, the following: [..] diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c index 0ec8a04..0ee0b0e 100644 --- a/arch/arm/mach-omap2/pm34xx.c +++ b/arch/arm/mach-omap2/pm34xx.c @@ -29,6 +29,7 @@ #include linux/delay.h #include linux/slab.h #include linux/console.h +#include trace/events/power.h #include plat/sram.h #include plat/clockdomain.h @@ -506,8 +507,14 @@ static void omap3_pm_idle(void) if (omap_irq_pending() || need_resched()) goto out; + trace_power_start(POWER_CSTATE, 1, smp_processor_id()); + trace_cpu_idle(1, smp_processor_id()); + omap_sram_idle(); + trace_power_end(smp_processor_id()); + trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id()); Dumb question: it just tells me which C state was attempted - not if actually succeeded in hitting it rt? Does'nt this give us a false data? Yes this tracks the idle requests only. The actual hit state might be different depending on various factors: HW state, enabled clocks, power domains dependencies etc. Debugging the actual PM hit state requires other tools, which could use the trace API. There is definitely more to come on that topic. [..] diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c index fc62fb5..7cbb09b 100644 --- a/arch/arm/plat-omap/clock.c +++ b/arch/arm/plat-omap/clock.c (from an offline discussion on a related topic): Would it also be nice to hook on mach-omap2/clock.c points as well to hook on indirect changes? [..] -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] perf: add OMAP support for the new power events
Hi Paul,
On Tue, Jan 4, 2011 at 7:48 PM, Paul Walmsley p...@pwsan.com wrote:
Hello Jean,
On Tue, 4 Jan 2011, jean.pi...@newoldbits.com wrote:
From: Jean Pihet j-pi...@ti.com
The patch adds the new power management trace points for
the OMAP architecture.
The trace points are for:
- default idle handler. Since the cpuidle framework is
instrumented in the generic way there is no need to
add trace points in the OMAP specific cpuidle handler;
- cpufreq (DVFS),
- clocks changes (enable, disable, set_rate),
A question about these. Are these only meant to track calls to these
functions from outside the clock code? Or meant to track actual hardware
clock changes?
The former: this is used to track the clock requests from outside the
clock framework.
If the latter, then it might make sense to put these
trace points into the functions that actually change the hardware
registers, e.g., omap2_dflt_clk_{enable,disable}(), etc., since a
clk_enable() on a leaf clock may result in many internal system clocks
being enabled up the clock tree.
I agree with you it is better to track the actual clock changes instead.
I propose to move the tracepoints to omap2_clk_{enable...} which
enables all the clocks irrespectively of the installed handler.
Note about the clock handlers: omap2_dflt_clk_enable happens to be the
handler for all controllable clocks but could that change in the
future?
- Paul
Thanks,
Jean
- change of power domains next power states.
Signed-off-by: Jean Pihet j-pi...@ti.com
---
arch/arm/mach-omap2/pm34xx.c | 7 +++
arch/arm/mach-omap2/powerdomain.c | 3 +++
arch/arm/plat-omap/clock.c | 13 ++---
3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0ec8a04..0ee0b0e 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -29,6 +29,7 @@
#include linux/delay.h
#include linux/slab.h
#include linux/console.h
+#include trace/events/power.h
#include plat/sram.h
#include plat/clockdomain.h
@@ -506,8 +507,14 @@ static void omap3_pm_idle(void)
if (omap_irq_pending() || need_resched())
goto out;
+ trace_power_start(POWER_CSTATE, 1, smp_processor_id());
+ trace_cpu_idle(1, smp_processor_id());
+
omap_sram_idle();
+ trace_power_end(smp_processor_id());
+ trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
+
out:
local_fiq_enable();
local_irq_enable();
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 6527ec3..73cbe9a 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -23,6 +23,7 @@
#include linux/errno.h
#include linux/err.h
#include linux/io.h
+#include trace/events/power.h
#include asm/atomic.h
@@ -440,6 +441,8 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8
pwrst)
pr_debug(powerdomain: setting next powerstate for %s to %0x\n,
pwrdm-name, pwrst);
+ trace_power_domain_target(pwrdm-name, pwrst, smp_processor_id());
+
prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
(pwrst OMAP_POWERSTATE_SHIFT),
pwrdm-prcm_offs, pwrstctrl_reg_offs);
diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
index fc62fb5..7cbb09b 100644
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c
@@ -21,6 +21,7 @@
#include linux/cpufreq.h
#include linux/debugfs.h
#include linux/io.h
+#include trace/events/power.h
#include plat/clock.h
@@ -43,8 +44,10 @@ int clk_enable(struct clk *clk)
return -EINVAL;
spin_lock_irqsave(clockfw_lock, flags);
- if (arch_clock-clk_enable)
+ if (arch_clock-clk_enable) {
+ trace_clock_enable(clk-name, 1, smp_processor_id());
ret = arch_clock-clk_enable(clk);
+ }
spin_unlock_irqrestore(clockfw_lock, flags);
return ret;
@@ -66,8 +69,10 @@ void clk_disable(struct clk *clk)
goto out;
}
- if (arch_clock-clk_disable)
+ if (arch_clock-clk_disable) {
+ trace_clock_disable(clk-name, 0, smp_processor_id());
arch_clock-clk_disable(clk);
+ }
out:
spin_unlock_irqrestore(clockfw_lock, flags);
@@ -120,8 +125,10 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
return ret;
spin_lock_irqsave(clockfw_lock, flags);
- if (arch_clock-clk_set_rate)
+ if (arch_clock-clk_set_rate) {
+ trace_clock_set_rate(clk-name, rate, smp_processor_id());
ret = arch_clock-clk_set_rate(clk, rate);
+ }
if (ret == 0) {
if (clk-recalc)
clk-rate = clk-recalc(clk);
--
1.7.2.3
- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to
[PATCH 2/3] perf: add OMAP support for the new power events
From: Jean Pihet j-pi...@ti.com
The patch adds the new power management trace points for
the OMAP architecture.
The trace points are for:
- default idle handler. Since the cpuidle framework is
instrumented in the generic way there is no need to
add trace points in the OMAP specific cpuidle handler;
- cpufreq (DVFS),
- clocks changes (enable, disable, set_rate),
- change of power domains next power states.
Signed-off-by: Jean Pihet j-pi...@ti.com
---
arch/arm/mach-omap2/pm34xx.c |7 +++
arch/arm/mach-omap2/powerdomain.c |3 +++
arch/arm/plat-omap/clock.c| 13 ++---
3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0ec8a04..0ee0b0e 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -29,6 +29,7 @@
#include linux/delay.h
#include linux/slab.h
#include linux/console.h
+#include trace/events/power.h
#include plat/sram.h
#include plat/clockdomain.h
@@ -506,8 +507,14 @@ static void omap3_pm_idle(void)
if (omap_irq_pending() || need_resched())
goto out;
+ trace_power_start(POWER_CSTATE, 1, smp_processor_id());
+ trace_cpu_idle(1, smp_processor_id());
+
omap_sram_idle();
+ trace_power_end(smp_processor_id());
+ trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
+
out:
local_fiq_enable();
local_irq_enable();
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 6527ec3..73cbe9a 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -23,6 +23,7 @@
#include linux/errno.h
#include linux/err.h
#include linux/io.h
+#include trace/events/power.h
#include asm/atomic.h
@@ -440,6 +441,8 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8
pwrst)
pr_debug(powerdomain: setting next powerstate for %s to %0x\n,
pwrdm-name, pwrst);
+ trace_power_domain_target(pwrdm-name, pwrst, smp_processor_id());
+
prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
(pwrst OMAP_POWERSTATE_SHIFT),
pwrdm-prcm_offs, pwrstctrl_reg_offs);
diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
index fc62fb5..7cbb09b 100644
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c
@@ -21,6 +21,7 @@
#include linux/cpufreq.h
#include linux/debugfs.h
#include linux/io.h
+#include trace/events/power.h
#include plat/clock.h
@@ -43,8 +44,10 @@ int clk_enable(struct clk *clk)
return -EINVAL;
spin_lock_irqsave(clockfw_lock, flags);
- if (arch_clock-clk_enable)
+ if (arch_clock-clk_enable) {
+ trace_clock_enable(clk-name, 1, smp_processor_id());
ret = arch_clock-clk_enable(clk);
+ }
spin_unlock_irqrestore(clockfw_lock, flags);
return ret;
@@ -66,8 +69,10 @@ void clk_disable(struct clk *clk)
goto out;
}
- if (arch_clock-clk_disable)
+ if (arch_clock-clk_disable) {
+ trace_clock_disable(clk-name, 0, smp_processor_id());
arch_clock-clk_disable(clk);
+ }
out:
spin_unlock_irqrestore(clockfw_lock, flags);
@@ -120,8 +125,10 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
return ret;
spin_lock_irqsave(clockfw_lock, flags);
- if (arch_clock-clk_set_rate)
+ if (arch_clock-clk_set_rate) {
+ trace_clock_set_rate(clk-name, rate, smp_processor_id());
ret = arch_clock-clk_set_rate(clk, rate);
+ }
if (ret == 0) {
if (clk-recalc)
clk-rate = clk-recalc(clk);
--
1.7.2.3
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] perf: add OMAP support for the new power events
jean.pi...@newoldbits.com had written, on 01/04/2011 04:17 AM, the following: [..] diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c index 0ec8a04..0ee0b0e 100644 --- a/arch/arm/mach-omap2/pm34xx.c +++ b/arch/arm/mach-omap2/pm34xx.c @@ -29,6 +29,7 @@ #include linux/delay.h #include linux/slab.h #include linux/console.h +#include trace/events/power.h #include plat/sram.h #include plat/clockdomain.h @@ -506,8 +507,14 @@ static void omap3_pm_idle(void) if (omap_irq_pending() || need_resched()) goto out; + trace_power_start(POWER_CSTATE, 1, smp_processor_id()); + trace_cpu_idle(1, smp_processor_id()); + omap_sram_idle(); + trace_power_end(smp_processor_id()); + trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id()); Dumb question: it just tells me which C state was attempted - not if actually succeeded in hitting it rt? Does'nt this give us a false data? [..] diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c index fc62fb5..7cbb09b 100644 --- a/arch/arm/plat-omap/clock.c +++ b/arch/arm/plat-omap/clock.c (from an offline discussion on a related topic): Would it also be nice to hook on mach-omap2/clock.c points as well to hook on indirect changes? [..] -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] perf: add OMAP support for the new power events
Hello Jean,
On Tue, 4 Jan 2011, jean.pi...@newoldbits.com wrote:
From: Jean Pihet j-pi...@ti.com
The patch adds the new power management trace points for
the OMAP architecture.
The trace points are for:
- default idle handler. Since the cpuidle framework is
instrumented in the generic way there is no need to
add trace points in the OMAP specific cpuidle handler;
- cpufreq (DVFS),
- clocks changes (enable, disable, set_rate),
A question about these. Are these only meant to track calls to these
functions from outside the clock code? Or meant to track actual hardware
clock changes? If the latter, then it might make sense to put these
trace points into the functions that actually change the hardware
registers, e.g., omap2_dflt_clk_{enable,disable}(), etc., since a
clk_enable() on a leaf clock may result in many internal system clocks
being enabled up the clock tree.
- Paul
- change of power domains next power states.
Signed-off-by: Jean Pihet j-pi...@ti.com
---
arch/arm/mach-omap2/pm34xx.c |7 +++
arch/arm/mach-omap2/powerdomain.c |3 +++
arch/arm/plat-omap/clock.c| 13 ++---
3 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 0ec8a04..0ee0b0e 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -29,6 +29,7 @@
#include linux/delay.h
#include linux/slab.h
#include linux/console.h
+#include trace/events/power.h
#include plat/sram.h
#include plat/clockdomain.h
@@ -506,8 +507,14 @@ static void omap3_pm_idle(void)
if (omap_irq_pending() || need_resched())
goto out;
+ trace_power_start(POWER_CSTATE, 1, smp_processor_id());
+ trace_cpu_idle(1, smp_processor_id());
+
omap_sram_idle();
+ trace_power_end(smp_processor_id());
+ trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
+
out:
local_fiq_enable();
local_irq_enable();
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 6527ec3..73cbe9a 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -23,6 +23,7 @@
#include linux/errno.h
#include linux/err.h
#include linux/io.h
+#include trace/events/power.h
#include asm/atomic.h
@@ -440,6 +441,8 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8
pwrst)
pr_debug(powerdomain: setting next powerstate for %s to %0x\n,
pwrdm-name, pwrst);
+ trace_power_domain_target(pwrdm-name, pwrst, smp_processor_id());
+
prm_rmw_mod_reg_bits(OMAP_POWERSTATE_MASK,
(pwrst OMAP_POWERSTATE_SHIFT),
pwrdm-prcm_offs, pwrstctrl_reg_offs);
diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
index fc62fb5..7cbb09b 100644
--- a/arch/arm/plat-omap/clock.c
+++ b/arch/arm/plat-omap/clock.c
@@ -21,6 +21,7 @@
#include linux/cpufreq.h
#include linux/debugfs.h
#include linux/io.h
+#include trace/events/power.h
#include plat/clock.h
@@ -43,8 +44,10 @@ int clk_enable(struct clk *clk)
return -EINVAL;
spin_lock_irqsave(clockfw_lock, flags);
- if (arch_clock-clk_enable)
+ if (arch_clock-clk_enable) {
+ trace_clock_enable(clk-name, 1, smp_processor_id());
ret = arch_clock-clk_enable(clk);
+ }
spin_unlock_irqrestore(clockfw_lock, flags);
return ret;
@@ -66,8 +69,10 @@ void clk_disable(struct clk *clk)
goto out;
}
- if (arch_clock-clk_disable)
+ if (arch_clock-clk_disable) {
+ trace_clock_disable(clk-name, 0, smp_processor_id());
arch_clock-clk_disable(clk);
+ }
out:
spin_unlock_irqrestore(clockfw_lock, flags);
@@ -120,8 +125,10 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
return ret;
spin_lock_irqsave(clockfw_lock, flags);
- if (arch_clock-clk_set_rate)
+ if (arch_clock-clk_set_rate) {
+ trace_clock_set_rate(clk-name, rate, smp_processor_id());
ret = arch_clock-clk_set_rate(clk, rate);
+ }
if (ret == 0) {
if (clk-recalc)
clk-rate = clk-recalc(clk);
--
1.7.2.3
- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
