[PATCH 0/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[PATCH 0/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Marc Titinger]

- change arg3 to a state name string: we got the current CPU from the trace
backend already. This also prepares for multiple/named states in the power
domain.

- tested with Juno Dev Platform

<idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
<idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
<idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0

- compiled for Omap2+

Depends on this patch set from Axel Haslam:

[v7,1/5] PM / Domains: prepare for multiple states
[v7,2/5] PM / Domains: core changes for multiple states
[v7,3/5] PM / Domains: make governor select deepest state
[v7,4/5] ARM: imx6: pm: declare pm domain latency on power_state struct.
[v7,5/5] PM / Domains: remove old power on/off latencies.


Marc Titinger (2):
  Trace: PM: promote event 'power_domain_target' to generic power
    domains.
  arm: omap2+: PM: change trace_power_domain_target event format.

 arch/arm/mach-omap2/powerdomain.c | 32 ++++++++++++++++++++------------
 drivers/base/power/domain.c       |  9 +++++++++
 include/trace/events/power.h      | 29 +++++++++++++++--------------
 3 files changed, 44 insertions(+), 26 deletions(-)

-- 
1.9.1

[PATCH 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Marc Titinger]

From: Marc Titinger <mtitinger@baylibre.com>

- change arg3 to a state name string: we got the current CPU rom the trace
backend already. This also prepares for multiple/named states in the power
domain, consistent with idle-states. states in the domain may match given
CPU states in this case, we will trace them by their name, and potentially
use arg2 as a link to their index for the cpuidle driver.

- tested with Juno DP

<idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
<idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
<idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0

- compiled for Omap2+

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/base/power/domain.c  |  9 +++++++++
 include/trace/events/power.h | 29 +++++++++++++++--------------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 59ccd92..b9e2a37 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -21,6 +21,10 @@
 #include <linux/export.h>
 #include <linux/sort.h>
 
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+#include <trace/events/power.h>
+#endif
+
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
 #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
@@ -328,6 +332,11 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
 
  out:
 	genpd->status = GPD_STATE_ACTIVE;
+
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+	trace_power_domain_target(genpd->name, genpd->state_idx,
+				genpd->states[genpd->state_idx].name);
+#endif
 	return 0;
 
  err:
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 284244e..8f93be6 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -237,9 +237,9 @@ DECLARE_EVENT_CLASS(clock,
 	TP_ARGS(name, state, cpu_id),
 
 	TP_STRUCT__entry(
-		__string(       name,           name            )
-		__field(        u64,            state           )
-		__field(        u64,            cpu_id          )
+		__string(name, name)
+		__field(u64, state)
+		__field(u64, cpu_id)
 	),
 
 	TP_fast_assign(
@@ -278,31 +278,32 @@ DEFINE_EVENT(clock, clock_set_rate,
  */
 DECLARE_EVENT_CLASS(power_domain,
 
-	TP_PROTO(const char *name, unsigned int state, unsigned int cpu_id),
+	TP_PROTO(const char *name, unsigned int index, const char *state_name),
 
-	TP_ARGS(name, state, cpu_id),
+	TP_ARGS(name, index, state_name),
 
 	TP_STRUCT__entry(
-		__string(       name,           name            )
-		__field(        u64,            state           )
-		__field(        u64,            cpu_id          )
+		__string(name, name)
+		__field(u64, index)
+		__string(state_name, state_name)
 	),
 
 	TP_fast_assign(
 		__assign_str(name, name);
-		__entry->state = state;
-		__entry->cpu_id = cpu_id;
+		__entry->index = index;
+		__assign_str(state_name, state_name);
 ),
 
-	TP_printk("%s state=%lu cpu_id=%lu", __get_str(name),
-		(unsigned long)__entry->state, (unsigned long)__entry->cpu_id)
+	TP_printk("%s index=%lu '%s'", __get_str(name),
+		(unsigned long)__entry->index,
+		__get_str(state_name))
 );
 
 DEFINE_EVENT(power_domain, power_domain_target,
 
-	TP_PROTO(const char *name, unsigned int state, unsigned int cpu_id),
+	TP_PROTO(const char *name, unsigned int index, const char *state_name),
 
-	TP_ARGS(name, state, cpu_id)
+	TP_ARGS(name, index, state_name)
 );
 
 /*
-- 
1.9.1

Re: [PATCH 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Steven Rostedt]

On Fri, 25 Sep 2015 15:22:24 +0200
Marc Titinger <mtitinger@baylibre.com> wrote:

> From: Marc Titinger <mtitinger@baylibre.com>
> 
> - change arg3 to a state name string: we got the current CPU rom the trace
> backend already. This also prepares for multiple/named states in the power
> domain, consistent with idle-states. states in the domain may match given
> CPU states in this case, we will trace them by their name, and potentially
> use arg2 as a link to their index for the cpuidle driver.
> 
> - tested with Juno DP
> 
> <idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
> <idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
> <idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0
> 
> - compiled for Omap2+
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/base/power/domain.c  |  9 +++++++++
>  include/trace/events/power.h | 29 +++++++++++++++--------------
>  2 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 59ccd92..b9e2a37 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -21,6 +21,10 @@
>  #include <linux/export.h>
>  #include <linux/sort.h>
>  
> +#ifdef CONFIG_PM_ADVANCED_DEBUG
> +#include <trace/events/power.h>
> +#endif

Are the events in events/power.h only available when PM_ADVANCED_DEBUG
is enabled? If so, this should probably be encapsulated in that header
file, with an "#else" that makes all the trace_power_*() calls into
static inlined nops. Then you can get rid of the ugly #ifdef in this
file.


> +
>  #define GENPD_RETRY_MAX_MS	250		/* Approximate */
>  
>  #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
> @@ -328,6 +332,11 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
>  
>   out:
>  	genpd->status = GPD_STATE_ACTIVE;
> +
> +#ifdef CONFIG_PM_ADVANCED_DEBUG
> +	trace_power_domain_target(genpd->name, genpd->state_idx,
> +				genpd->states[genpd->state_idx].name);
> +#endif
>  	return 0;
>  
>   err:
> diff --git a/include/trace/events/power.h b/include/trace/events/power.h
> index 284244e..8f93be6 100644
> --- a/include/trace/events/power.h
> +++ b/include/trace/events/power.h
> @@ -237,9 +237,9 @@ DECLARE_EVENT_CLASS(clock,
>  	TP_ARGS(name, state, cpu_id),
>  
>  	TP_STRUCT__entry(
> -		__string(       name,           name            )
> -		__field(        u64,            state           )
> -		__field(        u64,            cpu_id          )
> +		__string(name, name)
> +		__field(u64, state)
> +		__field(u64, cpu_id)

Note, the standard formatting for the tracepoints is with the spaces.
Igonre checkpatch for that, tracepoints don't follow it.

-- Steve

>  	),
>  
>  	TP_fast_assign(
> @@ -278,31 +278,32 @@ DEFINE_EVENT(clock, clock_set_rate,
>   */
>  DECLARE_EVENT_CLASS(power_domain,

[PATCH v2 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Marc Titinger]

- change arg3 to a state name string: we got the current CPU rom the trace
backend already. This also prepares for multiple/named states in the power
domain, consistent with idle-states. states in the domain may match given
CPU states in this case, we will trace them by their name, and potentially
use arg2 as a link to their index for the cpuidle driver.

- tested with Juno DP

<idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
<idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
<idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0

- compiled for Omap2+

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/base/power/domain.c  |  5 +++++
 include/trace/events/power.h | 21 +++++++++++----------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 59ccd92..017c151 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -21,6 +21,8 @@
 #include <linux/export.h>
 #include <linux/sort.h>
 
+#include <trace/events/power.h>
+
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
 #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
@@ -328,6 +330,9 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
 
  out:
 	genpd->status = GPD_STATE_ACTIVE;
+
+	trace_power_domain_target(genpd->name, genpd->state_idx,
+				genpd->states[genpd->state_idx].name);
 	return 0;
 
  err:
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 284244e..8172d93 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -278,31 +278,32 @@ DEFINE_EVENT(clock, clock_set_rate,
  */
 DECLARE_EVENT_CLASS(power_domain,
 
-	TP_PROTO(const char *name, unsigned int state, unsigned int cpu_id),
+	TP_PROTO(const char *name, unsigned int index, const char *state_name),
 
-	TP_ARGS(name, state, cpu_id),
+	TP_ARGS(name, index, state_name),
 
 	TP_STRUCT__entry(
 		__string(       name,           name            )
-		__field(        u64,            state           )
-		__field(        u64,            cpu_id          )
+		__field(        u64,            index           )
+		__string(       state_name,     state_name      )
 	),
 
 	TP_fast_assign(
 		__assign_str(name, name);
-		__entry->state = state;
-		__entry->cpu_id = cpu_id;
+		__entry->index = index;
+		__assign_str(state_name, state_name);
 ),
 
-	TP_printk("%s state=%lu cpu_id=%lu", __get_str(name),
-		(unsigned long)__entry->state, (unsigned long)__entry->cpu_id)
+	TP_printk("%s index=%lu '%s'", __get_str(name),
+		(unsigned long)__entry->index,
+		__get_str(state_name))
 );
 
 DEFINE_EVENT(power_domain, power_domain_target,
 
-	TP_PROTO(const char *name, unsigned int state, unsigned int cpu_id),
+	TP_PROTO(const char *name, unsigned int index, const char *state_name),
 
-	TP_ARGS(name, state, cpu_id)
+	TP_ARGS(name, index, state_name)
 );
 
 /*
-- 
1.9.1

[PATCH v2 2/2] arm: omap2+: PM: change trace_power_domain_target event format.

[Marc Titinger]

power_domain_target arg3 is now a string (event name) with generic power
domains. In the case of Omap, it is a hint to the prev/next switch op.

Compiled for Omap2+ but not tested.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 arch/arm/mach-omap2/powerdomain.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 78af6d8..02167c2 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -160,7 +160,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
 static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 {
 
-	int prev, next, state, trace_state = 0;
+	int prev, state;
 
 	if (pwrdm == NULL)
 		return -EINVAL;
@@ -177,17 +177,21 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 			pwrdm->state_counter[prev]++;
 		if (prev == PWRDM_POWER_RET)
 			_update_logic_membank_counters(pwrdm);
-		/*
-		 * If the power domain did not hit the desired state,
-		 * generate a trace event with both the desired and hit states
-		 */
-		next = pwrdm_read_next_pwrst(pwrdm);
-		if (next != prev) {
-			trace_state = (PWRDM_TRACE_STATES_FLAG |
+
+		if (trace_power_domain_target_enabled()) {
+			/*
+			 * If the power domain did not hit the desired state,
+			 * generate a trace event with both the desired and hit
+			 * states */
+			int next = pwrdm_read_next_pwrst(pwrdm);
+
+			if (next != prev) {
+				int trace_state = (PWRDM_TRACE_STATES_FLAG |
 				       ((next & OMAP_POWERSTATE_MASK) << 8) |
 				       ((prev & OMAP_POWERSTATE_MASK) << 0));
-			trace_power_domain_target(pwrdm->name, trace_state,
-						  smp_processor_id());
+				trace_power_domain_target(pwrdm->name,
+					trace_state, "PWRDM_STATE_PREV");
+			}
 		}
 		break;
 	default:
@@ -523,8 +527,7 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
 
 	if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst) {
 		/* Trace the pwrdm desired target state */
-		trace_power_domain_target(pwrdm->name, pwrst,
-					  smp_processor_id());
+		trace_power_domain_target(pwrdm->name, pwrst, "set_next_pwrst");
 		/* Program the pwrdm desired target state */
 		ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
 	}
-- 
1.9.1

[PATCH v3 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Marc Titinger]

- change arg3 to a state name string: we got the current CPU rom the trace
backend already. This also prepares for multiple/named states in the power
domain, consistent with idle-states. states in the domain may match given
CPU states in this case, we will trace them by their name, and potentially
use arg2 as a link to their index for the cpuidle driver.

- tested with Juno DP

<idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
<idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
<idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0

- compiled for Omap2+

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---

 v3: make this patch set applicable to current HEAD, since there is no
     dependency.

 drivers/base/power/domain.c  |  5 +++++
 include/trace/events/power.h | 21 +++++++++++----------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 16550c6..6661a80 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -20,6 +20,8 @@
 #include <linux/suspend.h>
 #include <linux/export.h>
 
+#include <trace/events/power.h>
+
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
 #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
@@ -268,6 +270,9 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
 
  out:
 	genpd->status = GPD_STATE_ACTIVE;
+
+	trace_power_domain_target(genpd->name, genpd->state_idx,
+				genpd->states[genpd->state_idx].name);
 	return 0;
 
  err:
diff --git a/include/trace/events/power.h b/include/trace/events/power.h
index 284244e..8172d93 100644
--- a/include/trace/events/power.h
+++ b/include/trace/events/power.h
@@ -278,31 +278,32 @@ DEFINE_EVENT(clock, clock_set_rate,
  */
 DECLARE_EVENT_CLASS(power_domain,
 
-	TP_PROTO(const char *name, unsigned int state, unsigned int cpu_id),
+	TP_PROTO(const char *name, unsigned int index, const char *state_name),
 
-	TP_ARGS(name, state, cpu_id),
+	TP_ARGS(name, index, state_name),
 
 	TP_STRUCT__entry(
 		__string(       name,           name            )
-		__field(        u64,            state           )
-		__field(        u64,            cpu_id          )
+		__field(        u64,            index           )
+		__string(       state_name,     state_name      )
 	),
 
 	TP_fast_assign(
 		__assign_str(name, name);
-		__entry->state = state;
-		__entry->cpu_id = cpu_id;
+		__entry->index = index;
+		__assign_str(state_name, state_name);
 ),
 
-	TP_printk("%s state=%lu cpu_id=%lu", __get_str(name),
-		(unsigned long)__entry->state, (unsigned long)__entry->cpu_id)
+	TP_printk("%s index=%lu '%s'", __get_str(name),
+		(unsigned long)__entry->index,
+		__get_str(state_name))
 );
 
 DEFINE_EVENT(power_domain, power_domain_target,
 
-	TP_PROTO(const char *name, unsigned int state, unsigned int cpu_id),
+	TP_PROTO(const char *name, unsigned int index, const char *state_name),
 
-	TP_ARGS(name, state, cpu_id)
+	TP_ARGS(name, index, state_name)
 );
 
 /*
-- 
1.9.1

Re: [PATCH v3 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

Hi Marc,

[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]

config: i386-allmodconfig (attached as .config)
reproduce:
  git checkout cc6f7469844987395d46013269b2d2f4e1ad735c
  # save the attached .config to linux build tree
  make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   drivers/base/power/domain.c: In function '__pm_genpd_poweron':
>> drivers/base/power/domain.c:274:46: error: 'struct generic_pm_domain' has no member named 'state_idx'
     trace_power_domain_target(genpd->name, genpd->state_idx,
                                                 ^
>> drivers/base/power/domain.c:275:10: error: 'struct generic_pm_domain' has no member named 'states'
        genpd->states[genpd->state_idx].name);
             ^
   drivers/base/power/domain.c:275:24: error: 'struct generic_pm_domain' has no member named 'state_idx'
        genpd->states[genpd->state_idx].name);
                           ^

vim +274 drivers/base/power/domain.c

   268		if (ret)
   269			goto err;
   270	
   271	 out:
   272		genpd->status = GPD_STATE_ACTIVE;
   273	
 > 274		trace_power_domain_target(genpd->name, genpd->state_idx,
 > 275					genpd->states[genpd->state_idx].name);
   276		return 0;
   277	
   278	 err:

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 51590 bytes --]

Re: [PATCH v3 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Rafael J. Wysocki]

On Monday, September 28, 2015 03:20:44 PM Marc Titinger wrote:
> - change arg3 to a state name string: we got the current CPU rom the trace
> backend already. This also prepares for multiple/named states in the power
> domain, consistent with idle-states. states in the domain may match given
> CPU states in this case, we will trace them by their name, and potentially
> use arg2 as a link to their index for the cpuidle driver.
> 
> - tested with Juno DP
> 
> <idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
> <idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
> <idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0
> 
> - compiled for Omap2+
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>

Hi,

What's your intent regarding this series?  Do you want it to be applied
separately, or is it going to be part of a larger series?

Thanks,
Rafael

Re: [PATCH v3 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Marc Titinger]

On 14/10/2015 02:55, Rafael J. Wysocki wrote:
> On Monday, September 28, 2015 03:20:44 PM Marc Titinger wrote:
>> - change arg3 to a state name string: we got the current CPU rom the trace
>> backend already. This also prepares for multiple/named states in the power
>> domain, consistent with idle-states. states in the domain may match given
>> CPU states in this case, we will trace them by their name, and potentially
>> use arg2 as a link to their index for the cpuidle driver.
>>
>> - tested with Juno DP
>>
>> <idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
>> <idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
>> <idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0
>>
>> - compiled for Omap2+
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>
> Hi,
>
> What's your intent regarding this series?  Do you want it to be applied
> separately, or is it going to be part of a larger series?

Hi Rafael,

v3 is a missed attempt to rebase this on the current head, but I did not 
fix v3 because thinking twice it's not that useful until Axel Haslam's 
multiple genpd states stuff is merged in. I'm finding v2 useful, shall I 
re-post v2 as v4 for clarity then ?

Thanks,
Marc.


>
> Thanks,
> Rafael
>

Re: [PATCH v3 1/2] Trace: PM: promote event 'power_domain_target' to generic power domains.

[Rafael J. Wysocki]

On Wednesday, October 14, 2015 10:18:27 AM Marc Titinger wrote:
> On 14/10/2015 02:55, Rafael J. Wysocki wrote:
> > On Monday, September 28, 2015 03:20:44 PM Marc Titinger wrote:
> >> - change arg3 to a state name string: we got the current CPU rom the trace
> >> backend already. This also prepares for multiple/named states in the power
> >> domain, consistent with idle-states. states in the domain may match given
> >> CPU states in this case, we will trace them by their name, and potentially
> >> use arg2 as a link to their index for the cpuidle driver.
> >>
> >> - tested with Juno DP
> >>
> >> <idle>-0     [000]    42.395510: power_domain_target:  a53_pd index=0 'cluster-sleep-0'
> >> <idle>-0     [000]    42.395528: cpu_idle:             state=4294967295 cpu_id=0
> >> <idle>-0     [000]    42.395668: cpu_idle:             state=2 cpu_id=0
> >>
> >> - compiled for Omap2+
> >>
> >> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> >
> > Hi,
> >
> > What's your intent regarding this series?  Do you want it to be applied
> > separately, or is it going to be part of a larger series?
> 
> Hi Rafael,
> 
> v3 is a missed attempt to rebase this on the current head, but I did not 
> fix v3 because thinking twice it's not that useful until Axel Haslam's 
> multiple genpd states stuff is merged in. I'm finding v2 useful, shall I 
> re-post v2 as v4 for clarity then ?

That's up to you mostly.

If I get ACKs from all of the appropriate people on those, I can easily apply them.

Thanks,
Rafael

[PATCH v3 2/2] arm: omap2+: PM: change trace_power_domain_target event format.

[Marc Titinger]

power_domain_target arg3 is now a string (event name) with generic power
domains. In the case of Omap, it is a hint to the prev/next switch op.

Compiled for Omap2+ but not tested.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 arch/arm/mach-omap2/powerdomain.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 78af6d8..02167c2 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -160,7 +160,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
 static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 {
 
-	int prev, next, state, trace_state = 0;
+	int prev, state;
 
 	if (pwrdm == NULL)
 		return -EINVAL;
@@ -177,17 +177,21 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 			pwrdm->state_counter[prev]++;
 		if (prev == PWRDM_POWER_RET)
 			_update_logic_membank_counters(pwrdm);
-		/*
-		 * If the power domain did not hit the desired state,
-		 * generate a trace event with both the desired and hit states
-		 */
-		next = pwrdm_read_next_pwrst(pwrdm);
-		if (next != prev) {
-			trace_state = (PWRDM_TRACE_STATES_FLAG |
+
+		if (trace_power_domain_target_enabled()) {
+			/*
+			 * If the power domain did not hit the desired state,
+			 * generate a trace event with both the desired and hit
+			 * states */
+			int next = pwrdm_read_next_pwrst(pwrdm);
+
+			if (next != prev) {
+				int trace_state = (PWRDM_TRACE_STATES_FLAG |
 				       ((next & OMAP_POWERSTATE_MASK) << 8) |
 				       ((prev & OMAP_POWERSTATE_MASK) << 0));
-			trace_power_domain_target(pwrdm->name, trace_state,
-						  smp_processor_id());
+				trace_power_domain_target(pwrdm->name,
+					trace_state, "PWRDM_STATE_PREV");
+			}
 		}
 		break;
 	default:
@@ -523,8 +527,7 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
 
 	if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst) {
 		/* Trace the pwrdm desired target state */
-		trace_power_domain_target(pwrdm->name, pwrst,
-					  smp_processor_id());
+		trace_power_domain_target(pwrdm->name, pwrst, "set_next_pwrst");
 		/* Program the pwrdm desired target state */
 		ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
 	}
-- 
1.9.1

[PATCH 2/2] arm: omap2+: PM: change trace_power_domain_target event format.

[Marc Titinger]

From: Marc Titinger <mtitinger@baylibre.com>

power_domain_target arg3 is now a string (event name) with generic power
domains. In the case of Omap, it is a hint to the prev/next switch op.
Incidentally this trace is now conditioned by CONFIG_PM_ADVANCED_DEBUG.

Compiled for Omap2+ but not tested.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 arch/arm/mach-omap2/powerdomain.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 78af6d8..cd77696 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -160,7 +160,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
 static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 {
 
-	int prev, next, state, trace_state = 0;
+	int prev, state;
 
 	if (pwrdm == NULL)
 		return -EINVAL;
@@ -177,18 +177,25 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 			pwrdm->state_counter[prev]++;
 		if (prev == PWRDM_POWER_RET)
 			_update_logic_membank_counters(pwrdm);
-		/*
-		 * If the power domain did not hit the desired state,
-		 * generate a trace event with both the desired and hit states
-		 */
-		next = pwrdm_read_next_pwrst(pwrdm);
-		if (next != prev) {
-			trace_state = (PWRDM_TRACE_STATES_FLAG |
+
+#ifdef CONFIG_PM_ADVANCED_DEBUG
+		{
+			/*
+			 * If the power domain did not hit the desired state,
+			 * generate a trace event with both the desired and hit
+			 * states */
+			int next = pwrdm_read_next_pwrst(pwrdm);
+
+			if (next != prev) {
+				int trace_state = (PWRDM_TRACE_STATES_FLAG |
 				       ((next & OMAP_POWERSTATE_MASK) << 8) |
 				       ((prev & OMAP_POWERSTATE_MASK) << 0));
-			trace_power_domain_target(pwrdm->name, trace_state,
-						  smp_processor_id());
+				trace_power_domain_target(pwrdm->name,
+					trace_state, "PWRDM_STATE_PREV");
+			}
 		}
+#endif
+
 		break;
 	default:
 		return -EINVAL;
@@ -522,9 +529,10 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
 		 pwrdm->name, pwrst);
 
 	if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst) {
+#ifdef CONFIG_PM_ADVANCED_DEBUG
 		/* Trace the pwrdm desired target state */
-		trace_power_domain_target(pwrdm->name, pwrst,
-					  smp_processor_id());
+		trace_power_domain_target(pwrdm->name, pwrst, "set_next_pwrst");
+#endif
 		/* Program the pwrdm desired target state */
 		ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
 	}
-- 
1.9.1

Re: [PATCH 2/2] arm: omap2+: PM: change trace_power_domain_target event format.

[Steven Rostedt]

On Fri, 25 Sep 2015 15:22:25 +0200
Marc Titinger <mtitinger@baylibre.com> wrote:

> From: Marc Titinger <mtitinger@baylibre.com>
> 
> power_domain_target arg3 is now a string (event name) with generic power
> domains. In the case of Omap, it is a hint to the prev/next switch op.
> Incidentally this trace is now conditioned by CONFIG_PM_ADVANCED_DEBUG.

I'm curious to why the addition of this config option?

> 
> Compiled for Omap2+ but not tested.
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  arch/arm/mach-omap2/powerdomain.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 78af6d8..cd77696 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -160,7 +160,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
>  static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>  {
>  
> -	int prev, next, state, trace_state = 0;
> +	int prev, state;
>  
>  	if (pwrdm == NULL)
>  		return -EINVAL;
> @@ -177,18 +177,25 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>  			pwrdm->state_counter[prev]++;
>  		if (prev == PWRDM_POWER_RET)
>  			_update_logic_membank_counters(pwrdm);
> -		/*
> -		 * If the power domain did not hit the desired state,
> -		 * generate a trace event with both the desired and hit states
> -		 */
> -		next = pwrdm_read_next_pwrst(pwrdm);
> -		if (next != prev) {
> -			trace_state = (PWRDM_TRACE_STATES_FLAG |
> +
> +#ifdef CONFIG_PM_ADVANCED_DEBUG

You do realize that you can add this to the block:


	if (trace_power_domain_target_enabled()) {

as it seems this code is only run to pass data to the tracepoint. The
above if statement will keep this block in the 'out-of-line' path when
tracing is not enabled via a static_key (jump-label).

-- Steve

> +		{
> +			/*
> +			 * If the power domain did not hit the desired state,
> +			 * generate a trace event with both the desired and hit
> +			 * states */
> +			int next = pwrdm_read_next_pwrst(pwrdm);
> +
> +			if (next != prev) {
> +				int trace_state = (PWRDM_TRACE_STATES_FLAG |
>  				       ((next & OMAP_POWERSTATE_MASK) << 8) |
>  				       ((prev & OMAP_POWERSTATE_MASK) << 0));
> -			trace_power_domain_target(pwrdm->name, trace_state,
> -						  smp_processor_id());
> +				trace_power_domain_target(pwrdm->name,
> +					trace_state, "PWRDM_STATE_PREV");
> +			}
>  		}
> +#endif
> +
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -522,9 +529,10 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
>  		 pwrdm->name, pwrst);
>  
>  	if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst) {
> +#ifdef CONFIG_PM_ADVANCED_DEBUG
>  		/* Trace the pwrdm desired target state */
> -		trace_power_domain_target(pwrdm->name, pwrst,
> -					  smp_processor_id());
> +		trace_power_domain_target(pwrdm->name, pwrst, "set_next_pwrst");
> +#endif
>  		/* Program the pwrdm desired target state */
>  		ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
>  	}

Re: [PATCH 2/2] arm: omap2+: PM: change trace_power_domain_target event format.

[Marc Titinger]

On 25/09/2015 16:10, Steven Rostedt wrote:
> On Fri, 25 Sep 2015 15:22:25 +0200
> Marc Titinger <mtitinger@baylibre.com> wrote:
>
>> From: Marc Titinger <mtitinger@baylibre.com>
>>
>> power_domain_target arg3 is now a string (event name) with generic power
>> domains. In the case of Omap, it is a hint to the prev/next switch op.
>> Incidentally this trace is now conditioned by CONFIG_PM_ADVANCED_DEBUG.
> I'm curious to why the addition of this config option?
>
I meant to be consistent with Juno/generic-power-domains, so that this 
trace always (or never) requires this switch.
I think I will remove this condition for both actually.

>> Compiled for Omap2+ but not tested.
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> ---
>>   arch/arm/mach-omap2/powerdomain.c | 32 ++++++++++++++++++++------------
>>   1 file changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>> index 78af6d8..cd77696 100644
>> --- a/arch/arm/mach-omap2/powerdomain.c
>> +++ b/arch/arm/mach-omap2/powerdomain.c
>> @@ -160,7 +160,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
>>   static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>>   {
>>   
>> -	int prev, next, state, trace_state = 0;
>> +	int prev, state;
>>   
>>   	if (pwrdm == NULL)
>>   		return -EINVAL;
>> @@ -177,18 +177,25 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>>   			pwrdm->state_counter[prev]++;
>>   		if (prev == PWRDM_POWER_RET)
>>   			_update_logic_membank_counters(pwrdm);
>> -		/*
>> -		 * If the power domain did not hit the desired state,
>> -		 * generate a trace event with both the desired and hit states
>> -		 */
>> -		next = pwrdm_read_next_pwrst(pwrdm);
>> -		if (next != prev) {
>> -			trace_state = (PWRDM_TRACE_STATES_FLAG |
>> +
>> +#ifdef CONFIG_PM_ADVANCED_DEBUG
> You do realize that you can add this to the block:
>
>
> 	if (trace_power_domain_target_enabled()) {
Nope I didn't, but now I do ;) thanks.

Marc.

>
> as it seems this code is only run to pass data to the tracepoint. The
> above if statement will keep this block in the 'out-of-line' path when
> tracing is not enabled via a static_key (jump-label).
>
> -- Steve
>
>> +		{
>> +			/*
>> +			 * If the power domain did not hit the desired state,
>> +			 * generate a trace event with both the desired and hit
>> +			 * states */
>> +			int next = pwrdm_read_next_pwrst(pwrdm);
>> +
>> +			if (next != prev) {
>> +				int trace_state = (PWRDM_TRACE_STATES_FLAG |
>>   				       ((next & OMAP_POWERSTATE_MASK) << 8) |
>>   				       ((prev & OMAP_POWERSTATE_MASK) << 0));
>> -			trace_power_domain_target(pwrdm->name, trace_state,
>> -						  smp_processor_id());
>> +				trace_power_domain_target(pwrdm->name,
>> +					trace_state, "PWRDM_STATE_PREV");
>> +			}
>>   		}
>> +#endif
>> +
>>   		break;
>>   	default:
>>   		return -EINVAL;
>> @@ -522,9 +529,10 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
>>   		 pwrdm->name, pwrst);
>>   
>>   	if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst) {
>> +#ifdef CONFIG_PM_ADVANCED_DEBUG
>>   		/* Trace the pwrdm desired target state */
>> -		trace_power_domain_target(pwrdm->name, pwrst,
>> -					  smp_processor_id());
>> +		trace_power_domain_target(pwrdm->name, pwrst, "set_next_pwrst");
>> +#endif
>>   		/* Program the pwrdm desired target state */
>>   		ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
>>   	}

Re: [PATCH 2/2] arm: omap2+: PM: change trace_power_domain_target event format.

[Tony Lindgren]

* Marc Titinger <mtitinger@baylibre.com> [150925 08:02]:
> 
> 
> On 25/09/2015 16:10, Steven Rostedt wrote:
> >On Fri, 25 Sep 2015 15:22:25 +0200
> >Marc Titinger <mtitinger@baylibre.com> wrote:
> >
> >>From: Marc Titinger <mtitinger@baylibre.com>
> >>
> >>power_domain_target arg3 is now a string (event name) with generic power
> >>domains. In the case of Omap, it is a hint to the prev/next switch op.
> >>Incidentally this trace is now conditioned by CONFIG_PM_ADVANCED_DEBUG.
> >I'm curious to why the addition of this config option?
> >
> I meant to be consistent with Juno/generic-power-domains, so that this trace
> always (or never) requires this switch.
> I think I will remove this condition for both actually.
> 
> >>Compiled for Omap2+ but not tested.
> >>
> >>Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> >>---
> >>  arch/arm/mach-omap2/powerdomain.c | 32 ++++++++++++++++++++------------
> >>  1 file changed, 20 insertions(+), 12 deletions(-)
> >>
> >>diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> >>index 78af6d8..cd77696 100644
> >>--- a/arch/arm/mach-omap2/powerdomain.c
> >>+++ b/arch/arm/mach-omap2/powerdomain.c
> >>@@ -160,7 +160,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
> >>  static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
> >>  {
> >>-	int prev, next, state, trace_state = 0;
> >>+	int prev, state;
> >>  	if (pwrdm == NULL)
> >>  		return -EINVAL;
> >>@@ -177,18 +177,25 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
> >>  			pwrdm->state_counter[prev]++;
> >>  		if (prev == PWRDM_POWER_RET)
> >>  			_update_logic_membank_counters(pwrdm);
> >>-		/*
> >>-		 * If the power domain did not hit the desired state,
> >>-		 * generate a trace event with both the desired and hit states
> >>-		 */
> >>-		next = pwrdm_read_next_pwrst(pwrdm);
> >>-		if (next != prev) {
> >>-			trace_state = (PWRDM_TRACE_STATES_FLAG |
> >>+
> >>+#ifdef CONFIG_PM_ADVANCED_DEBUG
> >You do realize that you can add this to the block:
> >
> >
> >	if (trace_power_domain_target_enabled()) {
> Nope I didn't, but now I do ;) thanks.

Probably best to keep this with your series, it should not cause merge conflicts,
so:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH 2/2] arm: omap2+: PM: change trace_power_domain_target event format.

[Marc Titinger]

On 13/10/2015 01:33, Tony Lindgren wrote:
> * Marc Titinger <mtitinger@baylibre.com> [150925 08:02]:
>>
>>
>> On 25/09/2015 16:10, Steven Rostedt wrote:
>>> On Fri, 25 Sep 2015 15:22:25 +0200
>>> Marc Titinger <mtitinger@baylibre.com> wrote:
>>>
>>>> From: Marc Titinger <mtitinger@baylibre.com>
>>>>
>>>> power_domain_target arg3 is now a string (event name) with generic power
>>>> domains. In the case of Omap, it is a hint to the prev/next switch op.
>>>> Incidentally this trace is now conditioned by CONFIG_PM_ADVANCED_DEBUG.
>>> I'm curious to why the addition of this config option?
>>>
>> I meant to be consistent with Juno/generic-power-domains, so that this trace
>> always (or never) requires this switch.
>> I think I will remove this condition for both actually.
>>
>>>> Compiled for Omap2+ but not tested.
>>>>
>>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>>>> ---
>>>>   arch/arm/mach-omap2/powerdomain.c | 32 ++++++++++++++++++++------------
>>>>   1 file changed, 20 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>>>> index 78af6d8..cd77696 100644
>>>> --- a/arch/arm/mach-omap2/powerdomain.c
>>>> +++ b/arch/arm/mach-omap2/powerdomain.c
>>>> @@ -160,7 +160,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
>>>>   static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>>>>   {
>>>> -	int prev, next, state, trace_state = 0;
>>>> +	int prev, state;
>>>>   	if (pwrdm == NULL)
>>>>   		return -EINVAL;
>>>> @@ -177,18 +177,25 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
>>>>   			pwrdm->state_counter[prev]++;
>>>>   		if (prev == PWRDM_POWER_RET)
>>>>   			_update_logic_membank_counters(pwrdm);
>>>> -		/*
>>>> -		 * If the power domain did not hit the desired state,
>>>> -		 * generate a trace event with both the desired and hit states
>>>> -		 */
>>>> -		next = pwrdm_read_next_pwrst(pwrdm);
>>>> -		if (next != prev) {
>>>> -			trace_state = (PWRDM_TRACE_STATES_FLAG |
>>>> +
>>>> +#ifdef CONFIG_PM_ADVANCED_DEBUG
>>> You do realize that you can add this to the block:
>>>
>>>
>>> 	if (trace_power_domain_target_enabled()) {
>> Nope I didn't, but now I do ;) thanks.
>
> Probably best to keep this with your series, it should not cause merge conflicts,
> so:
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>

Thanks for the ack. Indeed, I rebased it on current but figured that 
it's not that usefull until 'multiple states' are merged in.

M.