[PATCH 1/5] acpi : remove latency_ticks from acpi_processor_cx structure

[PATCH 1/5] acpi : remove latency_ticks from acpi_processor_cx structure

[Daniel Lezcano]

Remove the latency_ticks field as it is not used.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    2 --
 include/acpi/processor.h      |    1 -
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index f3decb3..da92c65 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -586,7 +586,6 @@ static void acpi_processor_power_verify_c3(struct acpi_processor *pr,
 	 */
 	cx->valid = 1;
 
-	cx->latency_ticks = cx->latency;
 	/*
 	 * On older chipsets, BM_RLD needs to be set
 	 * in order for Bus Master activity to wake the
@@ -619,7 +618,6 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
 			if (!cx->address)
 				break;
 			cx->valid = 1; 
-			cx->latency_ticks = cx->latency; /* Normalize latency */
 			break;
 
 		case ACPI_STATE_C3:
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 9d65047..a70cbcc 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -59,7 +59,6 @@ struct acpi_processor_cx {
 	u8 entry_method;
 	u8 index;
 	u32 latency;
-	u32 latency_ticks;
 	u32 power;
 	u32 usage;
 	u64 time;
-- 
1.7.5.4

[PATCH 2/5] acpi : remove index from acpi_processor_cx structure

[Daniel Lezcano]

Remove the index field. It could be given without adding extra
information in the cx structure.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/x86/kernel/acpi/cstate.c |   18 +++++++++---------
 drivers/acpi/processor_idle.c |   15 +++++++--------
 include/acpi/processor.h      |   11 +++++------
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index d2b7f27..faa7f50 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c
@@ -112,8 +112,8 @@ out:
 	return retval;
 }
 
-int acpi_processor_ffh_cstate_probe(unsigned int cpu,
-		struct acpi_processor_cx *cx, struct acpi_power_register *reg)
+int acpi_processor_ffh_cstate_probe(unsigned int cpu, struct acpi_processor_cx *cx,
+				    int index, struct acpi_power_register *reg)
 {
 	struct cstate_entry *percpu_entry;
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -126,16 +126,16 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 		return -1;
 
 	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
-	percpu_entry->states[cx->index].eax = 0;
-	percpu_entry->states[cx->index].ecx = 0;
+	percpu_entry->states[index].eax = 0;
+	percpu_entry->states[index].ecx = 0;
 
 	/* Make sure we are running on right CPU */
 
 	retval = work_on_cpu(cpu, acpi_processor_ffh_cstate_probe_cpu, cx);
 	if (retval == 0) {
 		/* Use the hint in CST */
-		percpu_entry->states[cx->index].eax = cx->address;
-		percpu_entry->states[cx->index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
+		percpu_entry->states[index].eax = cx->address;
+		percpu_entry->states[index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
 	}
 
 	/*
@@ -173,14 +173,14 @@ void mwait_idle_with_hints(unsigned long ax, unsigned long cx)
 	}
 }
 
-void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
+void acpi_processor_ffh_cstate_enter(int index)
 {
 	unsigned int cpu = smp_processor_id();
 	struct cstate_entry *percpu_entry;
 
 	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
-	mwait_idle_with_hints(percpu_entry->states[cx->index].eax,
-	                      percpu_entry->states[cx->index].ecx);
+	mwait_idle_with_hints(percpu_entry->states[index].eax,
+	                      percpu_entry->states[index].ecx);
 }
 EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_enter);
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index da92c65..9808e60 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -433,12 +433,11 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 			current_count++;
 
 		cx.address = reg->address;
-		cx.index = current_count + 1;
 
 		cx.entry_method = ACPI_CSTATE_SYSTEMIO;
 		if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
-			if (acpi_processor_ffh_cstate_probe
-					(pr->id, &cx, reg) == 0) {
+			if (acpi_processor_ffh_cstate_probe(
+				    pr->id, &cx, current_count + 1, reg) == 0) {
 				cx.entry_method = ACPI_CSTATE_FFH;
 			} else if (cx.type == ACPI_STATE_C1) {
 				/*
@@ -707,13 +706,13 @@ static int acpi_idle_bm_check(void)
  *
  * Caller disables interrupt before call and enables interrupt after return.
  */
-static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
+static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx, int index)
 {
 	/* Don't trace irqs off for idle */
 	stop_critical_timings();
 	if (cx->entry_method == ACPI_CSTATE_FFH) {
 		/* Call into architectural FFH based C-state */
-		acpi_processor_ffh_cstate_enter(cx);
+		acpi_processor_ffh_cstate_enter(index);
 	} else if (cx->entry_method == ACPI_CSTATE_HALT) {
 		acpi_safe_halt();
 	} else {
@@ -754,7 +753,7 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
-	acpi_idle_do_entry(cx);
+	acpi_idle_do_entry(cx, index);
 	kt2 = ktime_get_real();
 	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
 
@@ -848,7 +847,7 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	kt1 = ktime_get_real();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
-	acpi_idle_do_entry(cx);
+	acpi_idle_do_entry(cx, index);
 	kt2 = ktime_get_real();
 	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
 	idle_time = idle_time_ns;
@@ -960,7 +959,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		ACPI_FLUSH_CPU_CACHE();
 	}
 
-	acpi_idle_do_entry(cx);
+	acpi_idle_do_entry(cx, index);
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index a70cbcc..d09c94f 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -57,7 +57,6 @@ struct acpi_processor_cx {
 	u8 type;
 	u32 address;
 	u8 entry_method;
-	u8 index;
 	u32 latency;
 	u32 power;
 	u32 usage;
@@ -249,8 +248,9 @@ void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
 					unsigned int cpu);
 int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 				    struct acpi_processor_cx *cx,
+				    int index,
 				    struct acpi_power_register *reg);
-void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
+void acpi_processor_ffh_cstate_enter(int index);
 #else
 static inline void acpi_processor_power_init_bm_check(struct
 						      acpi_processor_flags
@@ -261,13 +261,12 @@ static inline void acpi_processor_power_init_bm_check(struct
 }
 static inline int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 						  struct acpi_processor_cx *cx,
-						  struct acpi_power_register
-						  *reg)
+						  int index,
+						  struct acpi_power_register *reg)
 {
 	return -1;
 }
-static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
-						   *cstate)
+static inline void acpi_processor_ffh_cstate_enter(int index)
 {
 	return;
 }
-- 
1.7.5.4

Re: [PATCH 2/5] acpi : remove index from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the index field. It could be given without adding extra
> information in the cx structure.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

I'm not sure about this one.

The code seems to be more straightforward without it, actually.

Why exactly do you need to remove the field?

Rafael


> ---
>  arch/x86/kernel/acpi/cstate.c |   18 +++++++++---------
>  drivers/acpi/processor_idle.c |   15 +++++++--------
>  include/acpi/processor.h      |   11 +++++------
>  3 files changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index d2b7f27..faa7f50 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -112,8 +112,8 @@ out:
>  	return retval;
>  }
>  
> -int acpi_processor_ffh_cstate_probe(unsigned int cpu,
> -		struct acpi_processor_cx *cx, struct acpi_power_register *reg)
> +int acpi_processor_ffh_cstate_probe(unsigned int cpu, struct acpi_processor_cx *cx,
> +				    int index, struct acpi_power_register *reg)
>  {
>  	struct cstate_entry *percpu_entry;
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> @@ -126,16 +126,16 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  		return -1;
>  
>  	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> -	percpu_entry->states[cx->index].eax = 0;
> -	percpu_entry->states[cx->index].ecx = 0;
> +	percpu_entry->states[index].eax = 0;
> +	percpu_entry->states[index].ecx = 0;
>  
>  	/* Make sure we are running on right CPU */
>  
>  	retval = work_on_cpu(cpu, acpi_processor_ffh_cstate_probe_cpu, cx);
>  	if (retval == 0) {
>  		/* Use the hint in CST */
> -		percpu_entry->states[cx->index].eax = cx->address;
> -		percpu_entry->states[cx->index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
> +		percpu_entry->states[index].eax = cx->address;
> +		percpu_entry->states[index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
>  	}
>  
>  	/*
> @@ -173,14 +173,14 @@ void mwait_idle_with_hints(unsigned long ax, unsigned long cx)
>  	}
>  }
>  
> -void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
> +void acpi_processor_ffh_cstate_enter(int index)
>  {
>  	unsigned int cpu = smp_processor_id();
>  	struct cstate_entry *percpu_entry;
>  
>  	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> -	mwait_idle_with_hints(percpu_entry->states[cx->index].eax,
> -	                      percpu_entry->states[cx->index].ecx);
> +	mwait_idle_with_hints(percpu_entry->states[index].eax,
> +	                      percpu_entry->states[index].ecx);
>  }
>  EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_enter);
>  
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index da92c65..9808e60 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -433,12 +433,11 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
>  			current_count++;
>  
>  		cx.address = reg->address;
> -		cx.index = current_count + 1;
>  
>  		cx.entry_method = ACPI_CSTATE_SYSTEMIO;
>  		if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
> -			if (acpi_processor_ffh_cstate_probe
> -					(pr->id, &cx, reg) == 0) {
> +			if (acpi_processor_ffh_cstate_probe(
> +				    pr->id, &cx, current_count + 1, reg) == 0) {
>  				cx.entry_method = ACPI_CSTATE_FFH;
>  			} else if (cx.type == ACPI_STATE_C1) {
>  				/*
> @@ -707,13 +706,13 @@ static int acpi_idle_bm_check(void)
>   *
>   * Caller disables interrupt before call and enables interrupt after return.
>   */
> -static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
> +static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx, int index)
>  {
>  	/* Don't trace irqs off for idle */
>  	stop_critical_timings();
>  	if (cx->entry_method == ACPI_CSTATE_FFH) {
>  		/* Call into architectural FFH based C-state */
> -		acpi_processor_ffh_cstate_enter(cx);
> +		acpi_processor_ffh_cstate_enter(index);
>  	} else if (cx->entry_method == ACPI_CSTATE_HALT) {
>  		acpi_safe_halt();
>  	} else {
> @@ -754,7 +753,7 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  	kt1 = ktime_get_real();
> -	acpi_idle_do_entry(cx);
> +	acpi_idle_do_entry(cx, index);
>  	kt2 = ktime_get_real();
>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>  
> @@ -848,7 +847,7 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	kt1 = ktime_get_real();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
> -	acpi_idle_do_entry(cx);
> +	acpi_idle_do_entry(cx, index);
>  	kt2 = ktime_get_real();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
> @@ -960,7 +959,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		ACPI_FLUSH_CPU_CACHE();
>  	}
>  
> -	acpi_idle_do_entry(cx);
> +	acpi_idle_do_entry(cx, index);
>  
>  	/* Re-enable bus master arbitration */
>  	if (pr->flags.bm_check && pr->flags.bm_control) {
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index a70cbcc..d09c94f 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -57,7 +57,6 @@ struct acpi_processor_cx {
>  	u8 type;
>  	u32 address;
>  	u8 entry_method;
> -	u8 index;
>  	u32 latency;
>  	u32 power;
>  	u32 usage;
> @@ -249,8 +248,9 @@ void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
>  					unsigned int cpu);
>  int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  				    struct acpi_processor_cx *cx,
> +				    int index,
>  				    struct acpi_power_register *reg);
> -void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
> +void acpi_processor_ffh_cstate_enter(int index);
>  #else
>  static inline void acpi_processor_power_init_bm_check(struct
>  						      acpi_processor_flags
> @@ -261,13 +261,12 @@ static inline void acpi_processor_power_init_bm_check(struct
>  }
>  static inline int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  						  struct acpi_processor_cx *cx,
> -						  struct acpi_power_register
> -						  *reg)
> +						  int index,
> +						  struct acpi_power_register *reg)
>  {
>  	return -1;
>  }
> -static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
> -						   *cstate)
> +static inline void acpi_processor_ffh_cstate_enter(int index)
>  {
>  	return;
>  }
> 

Re: [PATCH 2/5] acpi : remove index from acpi_processor_cx structure

[Daniel Lezcano]

On 07/12/2012 11:44 PM, Rafael J. Wysocki wrote:
> On Thursday, July 12, 2012, Daniel Lezcano wrote:
>> Remove the index field. It could be given without adding extra
>> information in the cx structure.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> I'm not sure about this one.
> 
> The code seems to be more straightforward without it, actually.
> 
> Why exactly do you need to remove the field?

Actually, I am trying to cleaning up the different structure in the acpi
and clearly separate what is for cpuidle and what is for acpi.

I noticed there are some duplicated informations like the index and what
is described in the cx state.

I am trying to consolidate the code of the cpuidle drivers across the
different platform and I have to admit the processor_idle is the most
complicated as the cpuidle code is spreaded across different files
(processor_driver.c, processor.h, arch/x86/kernel/acpi/cstate.c, etc
...). So these small patches are to litlle by little separate these two
subsystems (acpi and cpuidle) and have everything related to cpuidle
into the processor_idle file.

For this patch, it is right it seems to introduce more complexity but
the objective is to consolidate the code. I hope by cleaning the
different structures we will gain in readability.

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] acpi : remove index from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> On 07/12/2012 11:44 PM, Rafael J. Wysocki wrote:
> > On Thursday, July 12, 2012, Daniel Lezcano wrote:
> >> Remove the index field. It could be given without adding extra
> >> information in the cx structure.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > 
> > I'm not sure about this one.
> > 
> > The code seems to be more straightforward without it, actually.
> > 
> > Why exactly do you need to remove the field?
> 
> Actually, I am trying to cleaning up the different structure in the acpi
> and clearly separate what is for cpuidle and what is for acpi.
> 
> I noticed there are some duplicated informations like the index and what
> is described in the cx state.
> 
> I am trying to consolidate the code of the cpuidle drivers across the
> different platform and I have to admit the processor_idle is the most
> complicated as the cpuidle code is spreaded across different files
> (processor_driver.c, processor.h, arch/x86/kernel/acpi/cstate.c, etc
> ...). So these small patches are to litlle by little separate these two
> subsystems (acpi and cpuidle) and have everything related to cpuidle
> into the processor_idle file.
> 
> For this patch, it is right it seems to introduce more complexity but
> the objective is to consolidate the code. I hope by cleaning the
> different structures we will gain in readability.

OK, I'm generally fine with that, but I'd prefer to defer this kind of
changes to 3.7 so as to avoid rushing them in.

Removing unused fields is entirely fine at this point, I don't see how
someone may see that objectionable, but let's just avoid playing with fields
that are used for now.

Thanks,
Rafael

Re: [PATCH 2/5] acpi : remove index from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the index field. It could be given without adding extra
> information in the cx structure.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Applied to the 'pm-cpuidle' branch of the linux-pm.git tree.

Thanks,
Rafael


> ---
>  arch/x86/kernel/acpi/cstate.c |   18 +++++++++---------
>  drivers/acpi/processor_idle.c |   15 +++++++--------
>  include/acpi/processor.h      |   11 +++++------
>  3 files changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index d2b7f27..faa7f50 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -112,8 +112,8 @@ out:
>  	return retval;
>  }
>  
> -int acpi_processor_ffh_cstate_probe(unsigned int cpu,
> -		struct acpi_processor_cx *cx, struct acpi_power_register *reg)
> +int acpi_processor_ffh_cstate_probe(unsigned int cpu, struct acpi_processor_cx *cx,
> +				    int index, struct acpi_power_register *reg)
>  {
>  	struct cstate_entry *percpu_entry;
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> @@ -126,16 +126,16 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  		return -1;
>  
>  	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> -	percpu_entry->states[cx->index].eax = 0;
> -	percpu_entry->states[cx->index].ecx = 0;
> +	percpu_entry->states[index].eax = 0;
> +	percpu_entry->states[index].ecx = 0;
>  
>  	/* Make sure we are running on right CPU */
>  
>  	retval = work_on_cpu(cpu, acpi_processor_ffh_cstate_probe_cpu, cx);
>  	if (retval == 0) {
>  		/* Use the hint in CST */
> -		percpu_entry->states[cx->index].eax = cx->address;
> -		percpu_entry->states[cx->index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
> +		percpu_entry->states[index].eax = cx->address;
> +		percpu_entry->states[index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
>  	}
>  
>  	/*
> @@ -173,14 +173,14 @@ void mwait_idle_with_hints(unsigned long ax, unsigned long cx)
>  	}
>  }
>  
> -void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
> +void acpi_processor_ffh_cstate_enter(int index)
>  {
>  	unsigned int cpu = smp_processor_id();
>  	struct cstate_entry *percpu_entry;
>  
>  	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
> -	mwait_idle_with_hints(percpu_entry->states[cx->index].eax,
> -	                      percpu_entry->states[cx->index].ecx);
> +	mwait_idle_with_hints(percpu_entry->states[index].eax,
> +	                      percpu_entry->states[index].ecx);
>  }
>  EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_enter);
>  
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index da92c65..9808e60 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -433,12 +433,11 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
>  			current_count++;
>  
>  		cx.address = reg->address;
> -		cx.index = current_count + 1;
>  
>  		cx.entry_method = ACPI_CSTATE_SYSTEMIO;
>  		if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
> -			if (acpi_processor_ffh_cstate_probe
> -					(pr->id, &cx, reg) == 0) {
> +			if (acpi_processor_ffh_cstate_probe(
> +				    pr->id, &cx, current_count + 1, reg) == 0) {
>  				cx.entry_method = ACPI_CSTATE_FFH;
>  			} else if (cx.type == ACPI_STATE_C1) {
>  				/*
> @@ -707,13 +706,13 @@ static int acpi_idle_bm_check(void)
>   *
>   * Caller disables interrupt before call and enables interrupt after return.
>   */
> -static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
> +static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx, int index)
>  {
>  	/* Don't trace irqs off for idle */
>  	stop_critical_timings();
>  	if (cx->entry_method == ACPI_CSTATE_FFH) {
>  		/* Call into architectural FFH based C-state */
> -		acpi_processor_ffh_cstate_enter(cx);
> +		acpi_processor_ffh_cstate_enter(index);
>  	} else if (cx->entry_method == ACPI_CSTATE_HALT) {
>  		acpi_safe_halt();
>  	} else {
> @@ -754,7 +753,7 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  
>  	lapic_timer_state_broadcast(pr, cx, 1);
>  	kt1 = ktime_get_real();
> -	acpi_idle_do_entry(cx);
> +	acpi_idle_do_entry(cx, index);
>  	kt2 = ktime_get_real();
>  	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
>  
> @@ -848,7 +847,7 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	kt1 = ktime_get_real();
>  	/* Tell the scheduler that we are going deep-idle: */
>  	sched_clock_idle_sleep_event();
> -	acpi_idle_do_entry(cx);
> +	acpi_idle_do_entry(cx, index);
>  	kt2 = ktime_get_real();
>  	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
>  	idle_time = idle_time_ns;
> @@ -960,7 +959,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		ACPI_FLUSH_CPU_CACHE();
>  	}
>  
> -	acpi_idle_do_entry(cx);
> +	acpi_idle_do_entry(cx, index);
>  
>  	/* Re-enable bus master arbitration */
>  	if (pr->flags.bm_check && pr->flags.bm_control) {
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index a70cbcc..d09c94f 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -57,7 +57,6 @@ struct acpi_processor_cx {
>  	u8 type;
>  	u32 address;
>  	u8 entry_method;
> -	u8 index;
>  	u32 latency;
>  	u32 power;
>  	u32 usage;
> @@ -249,8 +248,9 @@ void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
>  					unsigned int cpu);
>  int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  				    struct acpi_processor_cx *cx,
> +				    int index,
>  				    struct acpi_power_register *reg);
> -void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
> +void acpi_processor_ffh_cstate_enter(int index);
>  #else
>  static inline void acpi_processor_power_init_bm_check(struct
>  						      acpi_processor_flags
> @@ -261,13 +261,12 @@ static inline void acpi_processor_power_init_bm_check(struct
>  }
>  static inline int acpi_processor_ffh_cstate_probe(unsigned int cpu,
>  						  struct acpi_processor_cx *cx,
> -						  struct acpi_power_register
> -						  *reg)
> +						  int index,
> +						  struct acpi_power_register *reg)
>  {
>  	return -1;
>  }
> -static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
> -						   *cstate)
> +static inline void acpi_processor_ffh_cstate_enter(int index)
>  {
>  	return;
>  }
> 

Re: [PATCH 2/5] acpi : remove index from acpi_processor_cx structure

[Rafael J. Wysocki]

On Tuesday, July 17, 2012, Rafael J. Wysocki wrote:
> On Thursday, July 12, 2012, Daniel Lezcano wrote:
> > Remove the index field. It could be given without adding extra
> > information in the cx structure.
> > 
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Applied to the 'pm-cpuidle' branch of the linux-pm.git tree.

This patch wasn't applied in fact, for reasons given elsewhere.

Sorry for sending a misleading message.

Rafael

[PATCH 3/5] acpi : remove usage from acpi_processor_cx structure

[Daniel Lezcano]

Remove the usage field as it is not used.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    5 -----
 include/acpi/processor.h      |    1 -
 2 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 9808e60..d044588 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -761,7 +761,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 	dev->last_residency = (int)idle_time;
 
 	local_irq_enable();
-	cx->usage++;
 	lapic_timer_state_broadcast(pr, cx, 0);
 
 	return index;
@@ -863,8 +862,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	if (cx->entry_method != ACPI_CSTATE_FFH)
 		current_thread_info()->status |= TS_POLLING;
 
-	cx->usage++;
-
 	lapic_timer_state_broadcast(pr, cx, 0);
 	cx->time += idle_time;
 	return index;
@@ -983,8 +980,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 	if (cx->entry_method != ACPI_CSTATE_FFH)
 		current_thread_info()->status |= TS_POLLING;
 
-	cx->usage++;
-
 	lapic_timer_state_broadcast(pr, cx, 0);
 	cx->time += idle_time;
 	return index;
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index d09c94f..0957457 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -59,7 +59,6 @@ struct acpi_processor_cx {
 	u8 entry_method;
 	u32 latency;
 	u32 power;
-	u32 usage;
 	u64 time;
 	u8 bm_sts_skip;
 	char desc[ACPI_CX_DESC_LEN];
-- 
1.7.5.4

Re: [PATCH 3/5] acpi : remove usage from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the usage field as it is not used.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Looks good to me.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_idle.c |    5 -----
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 9808e60..d044588 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -761,7 +761,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  	dev->last_residency = (int)idle_time;
>  
>  	local_irq_enable();
> -	cx->usage++;
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  
>  	return index;
> @@ -863,8 +862,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> -	cx->usage++;
> -
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  	cx->time += idle_time;
>  	return index;
> @@ -983,8 +980,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> -	cx->usage++;
> -
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  	cx->time += idle_time;
>  	return index;
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index d09c94f..0957457 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -59,7 +59,6 @@ struct acpi_processor_cx {
>  	u8 entry_method;
>  	u32 latency;
>  	u32 power;
> -	u32 usage;
>  	u64 time;
>  	u8 bm_sts_skip;
>  	char desc[ACPI_CX_DESC_LEN];
> 

Re: [PATCH 3/5] acpi : remove usage from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the usage field as it is not used.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Applied to the 'pm-cpuidle' branch of the linux-pm.git tree.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_idle.c |    5 -----
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 9808e60..d044588 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -761,7 +761,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
>  	dev->last_residency = (int)idle_time;
>  
>  	local_irq_enable();
> -	cx->usage++;
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  
>  	return index;
> @@ -863,8 +862,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> -	cx->usage++;
> -
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  	cx->time += idle_time;
>  	return index;
> @@ -983,8 +980,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  	if (cx->entry_method != ACPI_CSTATE_FFH)
>  		current_thread_info()->status |= TS_POLLING;
>  
> -	cx->usage++;
> -
>  	lapic_timer_state_broadcast(pr, cx, 0);
>  	cx->time += idle_time;
>  	return index;
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index d09c94f..0957457 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -59,7 +59,6 @@ struct acpi_processor_cx {
>  	u8 entry_method;
>  	u32 latency;
>  	u32 power;
> -	u32 usage;
>  	u64 time;
>  	u8 bm_sts_skip;
>  	char desc[ACPI_CX_DESC_LEN];
> 

[PATCH 4/5] acpi : remove power from acpi_processor_cx structure

[Daniel Lezcano]

Remove the power field as it is not used.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    2 --
 include/acpi/processor.h      |    1 -
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index d044588..99ba58f 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -485,8 +485,6 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 		if (obj->type != ACPI_TYPE_INTEGER)
 			continue;
 
-		cx.power = obj->integer.value;
-
 		current_count++;
 		memcpy(&(pr->power.states[current_count]), &cx, sizeof(cx));
 
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 0957457..87bb9d7 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -58,7 +58,6 @@ struct acpi_processor_cx {
 	u32 address;
 	u8 entry_method;
 	u32 latency;
-	u32 power;
 	u64 time;
 	u8 bm_sts_skip;
 	char desc[ACPI_CX_DESC_LEN];
-- 
1.7.5.4

Re: [PATCH 4/5] acpi : remove power from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the power field as it is not used.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Looks good to me.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_idle.c |    2 --
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index d044588..99ba58f 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -485,8 +485,6 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
>  		if (obj->type != ACPI_TYPE_INTEGER)
>  			continue;
>  
> -		cx.power = obj->integer.value;
> -
>  		current_count++;
>  		memcpy(&(pr->power.states[current_count]), &cx, sizeof(cx));
>  
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 0957457..87bb9d7 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -58,7 +58,6 @@ struct acpi_processor_cx {
>  	u32 address;
>  	u8 entry_method;
>  	u32 latency;
> -	u32 power;
>  	u64 time;
>  	u8 bm_sts_skip;
>  	char desc[ACPI_CX_DESC_LEN];
> 

Re: [PATCH 4/5] acpi : remove power from acpi_processor_cx structure

[Konrad Rzeszutek Wilk]

On Thu, Jul 12, 2012 at 11:03:45PM +0200, Daniel Lezcano wrote:
> Remove the power field as it is not used.
> 
It looks to be used in drivers/xen/xen-acpi-processor.c.

I could emulate some value and stick it in there.. but I am
more curious - what is the intent of this value?


> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/acpi/processor_idle.c |    2 --
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index d044588..99ba58f 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -485,8 +485,6 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
>  		if (obj->type != ACPI_TYPE_INTEGER)
>  			continue;
>  
> -		cx.power = obj->integer.value;
> -
>  		current_count++;
>  		memcpy(&(pr->power.states[current_count]), &cx, sizeof(cx));
>  
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 0957457..87bb9d7 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -58,7 +58,6 @@ struct acpi_processor_cx {
>  	u32 address;
>  	u8 entry_method;
>  	u32 latency;
> -	u32 power;
>  	u64 time;
>  	u8 bm_sts_skip;
>  	char desc[ACPI_CX_DESC_LEN];
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] acpi : remove power from acpi_processor_cx structure

[Daniel Lezcano]

On 07/13/2012 02:17 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, Jul 12, 2012 at 11:03:45PM +0200, Daniel Lezcano wrote:
>> Remove the power field as it is not used.
>>
> It looks to be used in drivers/xen/xen-acpi-processor.c.

Right, the field is assigned but not used AFAICT.

 dst_cx->power = cx->power;

Did you see it used in another place ?

Thanks
  -- Daniel

> I could emulate some value and stick it in there.. but I am
> more curious - what is the intent of this value?
> 
> 
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/acpi/processor_idle.c |    2 --
>>  include/acpi/processor.h      |    1 -
>>  2 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index d044588..99ba58f 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -485,8 +485,6 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
>>  		if (obj->type != ACPI_TYPE_INTEGER)
>>  			continue;
>>  
>> -		cx.power = obj->integer.value;
>> -
>>  		current_count++;
>>  		memcpy(&(pr->power.states[current_count]), &cx, sizeof(cx));
>>  
>> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
>> index 0957457..87bb9d7 100644
>> --- a/include/acpi/processor.h
>> +++ b/include/acpi/processor.h
>> @@ -58,7 +58,6 @@ struct acpi_processor_cx {
>>  	u32 address;
>>  	u8 entry_method;
>>  	u32 latency;
>> -	u32 power;
>>  	u64 time;
>>  	u8 bm_sts_skip;
>>  	char desc[ACPI_CX_DESC_LEN];
>> -- 
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] acpi : remove power from acpi_processor_cx structure

[Daniel Lezcano]

On 07/13/2012 02:17 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, Jul 12, 2012 at 11:03:45PM +0200, Daniel Lezcano wrote:
>> Remove the power field as it is not used.
>>
> It looks to be used in drivers/xen/xen-acpi-processor.c.
> 
> I could emulate some value and stick it in there.. but I am
> more curious - what is the intent of this value?

At the first glance, this value is the power consumption of the
specified state. I am not sure all acpi returns a correct value.

I can imagine the power should be copied to the cpuidle_state structure
to the power field where it is used by the governor to choose the better
C-state. As it is not specified, cpuidle will assume the C-State N
consumes less than the C-State N-1.

If we want to add the power consumption we should also set the
'power_specified' flag for the driver, but that could change the
behavior of the cpuidle driver.

Anyway, IMO, this field is useless for this structure and should be
specified later, if that makes sense, directly in the cpuidle_state
structure like the other drivers do.

If nobody complains, I will remove the field also from Xen and resend
this patch.

Thanks
  -- Daniel

>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/acpi/processor_idle.c |    2 --
>>  include/acpi/processor.h      |    1 -
>>  2 files changed, 0 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index d044588..99ba58f 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -485,8 +485,6 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
>>  		if (obj->type != ACPI_TYPE_INTEGER)
>>  			continue;
>>  
>> -		cx.power = obj->integer.value;
>> -
>>  		current_count++;
>>  		memcpy(&(pr->power.states[current_count]), &cx, sizeof(cx));
>>  
>> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
>> index 0957457..87bb9d7 100644
>> --- a/include/acpi/processor.h
>> +++ b/include/acpi/processor.h
>> @@ -58,7 +58,6 @@ struct acpi_processor_cx {
>>  	u32 address;
>>  	u8 entry_method;
>>  	u32 latency;
>> -	u32 power;
>>  	u64 time;
>>  	u8 bm_sts_skip;
>>  	char desc[ACPI_CX_DESC_LEN];
>> -- 
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] acpi : remove power from acpi_processor_cx structure

[Rafael J. Wysocki]

On Friday, July 13, 2012, Daniel Lezcano wrote:
> On 07/13/2012 02:17 AM, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jul 12, 2012 at 11:03:45PM +0200, Daniel Lezcano wrote:
> >> Remove the power field as it is not used.
> >>
> > It looks to be used in drivers/xen/xen-acpi-processor.c.
> > 
> > I could emulate some value and stick it in there.. but I am
> > more curious - what is the intent of this value?
> 
> At the first glance, this value is the power consumption of the
> specified state. I am not sure all acpi returns a correct value.
> 
> I can imagine the power should be copied to the cpuidle_state structure
> to the power field where it is used by the governor to choose the better
> C-state. As it is not specified, cpuidle will assume the C-State N
> consumes less than the C-State N-1.
> 
> If we want to add the power consumption we should also set the
> 'power_specified' flag for the driver, but that could change the
> behavior of the cpuidle driver.
> 
> Anyway, IMO, this field is useless for this structure and should be
> specified later, if that makes sense, directly in the cpuidle_state
> structure like the other drivers do.
> 
> If nobody complains, I will remove the field also from Xen and resend
> this patch.

Please do.

Thanks,
Rafael

Re: [PATCH 4/5] acpi : remove power from acpi_processor_cx structure

[Konrad Rzeszutek Wilk]

On Fri, Jul 13, 2012 at 02:23:01PM +0200, Daniel Lezcano wrote:
> On 07/13/2012 02:17 AM, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jul 12, 2012 at 11:03:45PM +0200, Daniel Lezcano wrote:
> >> Remove the power field as it is not used.
> >>
> > It looks to be used in drivers/xen/xen-acpi-processor.c.
> > 
> > I could emulate some value and stick it in there.. but I am
> > more curious - what is the intent of this value?
> 
> At the first glance, this value is the power consumption of the
> specified state. I am not sure all acpi returns a correct value.

Like in milliwatts?
> 
> I can imagine the power should be copied to the cpuidle_state structure
> to the power field where it is used by the governor to choose the better
> C-state. As it is not specified, cpuidle will assume the C-State N
> consumes less than the C-State N-1.
> 
> If we want to add the power consumption we should also set the
> 'power_specified' flag for the driver, but that could change the
> behavior of the cpuidle driver.
> 
> Anyway, IMO, this field is useless for this structure and should be
> specified later, if that makes sense, directly in the cpuidle_state
> structure like the other drivers do.
> 
> If nobody complains, I will remove the field also from Xen and resend
> this patch.

Go for it. Looking at the Xen hypervisor code it just assigns the value
to its own structure and does nothing to it.

> 
> Thanks
>   -- Daniel
> 
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
> >>  drivers/acpi/processor_idle.c |    2 --
> >>  include/acpi/processor.h      |    1 -
> >>  2 files changed, 0 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index d044588..99ba58f 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -485,8 +485,6 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
> >>  		if (obj->type != ACPI_TYPE_INTEGER)
> >>  			continue;
> >>  
> >> -		cx.power = obj->integer.value;
> >> -
> >>  		current_count++;
> >>  		memcpy(&(pr->power.states[current_count]), &cx, sizeof(cx));
> >>  
> >> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> >> index 0957457..87bb9d7 100644
> >> --- a/include/acpi/processor.h
> >> +++ b/include/acpi/processor.h
> >> @@ -58,7 +58,6 @@ struct acpi_processor_cx {
> >>  	u32 address;
> >>  	u8 entry_method;
> >>  	u32 latency;
> >> -	u32 power;
> >>  	u64 time;
> >>  	u8 bm_sts_skip;
> >>  	char desc[ACPI_CX_DESC_LEN];
> >> -- 
> >> 1.7.5.4
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> -- 
>  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] acpi : remove time from acpi_processor_cx structure

[Daniel Lezcano]

Remove the time field as it is not used.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    2 --
 include/acpi/processor.h      |    1 -
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 99ba58f..bc1ce58 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -861,7 +861,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 		current_thread_info()->status |= TS_POLLING;
 
 	lapic_timer_state_broadcast(pr, cx, 0);
-	cx->time += idle_time;
 	return index;
 }
 
@@ -979,7 +978,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		current_thread_info()->status |= TS_POLLING;
 
 	lapic_timer_state_broadcast(pr, cx, 0);
-	cx->time += idle_time;
 	return index;
 }
 
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 87bb9d7..3998334 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -58,7 +58,6 @@ struct acpi_processor_cx {
 	u32 address;
 	u8 entry_method;
 	u32 latency;
-	u64 time;
 	u8 bm_sts_skip;
 	char desc[ACPI_CX_DESC_LEN];
 };
-- 
1.7.5.4

Re: [PATCH 5/5] acpi : remove time from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the time field as it is not used.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Looks good to me.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_idle.c |    2 --
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 99ba58f..bc1ce58 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -861,7 +861,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  		current_thread_info()->status |= TS_POLLING;
>  
>  	lapic_timer_state_broadcast(pr, cx, 0);
> -	cx->time += idle_time;
>  	return index;
>  }
>  
> @@ -979,7 +978,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		current_thread_info()->status |= TS_POLLING;
>  
>  	lapic_timer_state_broadcast(pr, cx, 0);
> -	cx->time += idle_time;
>  	return index;
>  }
>  
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 87bb9d7..3998334 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -58,7 +58,6 @@ struct acpi_processor_cx {
>  	u32 address;
>  	u8 entry_method;
>  	u32 latency;
> -	u64 time;
>  	u8 bm_sts_skip;
>  	char desc[ACPI_CX_DESC_LEN];
>  };
> 

Re: [PATCH 5/5] acpi : remove time from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the time field as it is not used.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Applied to the 'pm-cpuidle' branch of the linux-pm.git tree.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_idle.c |    2 --
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 99ba58f..bc1ce58 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -861,7 +861,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
>  		current_thread_info()->status |= TS_POLLING;
>  
>  	lapic_timer_state_broadcast(pr, cx, 0);
> -	cx->time += idle_time;
>  	return index;
>  }
>  
> @@ -979,7 +978,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
>  		current_thread_info()->status |= TS_POLLING;
>  
>  	lapic_timer_state_broadcast(pr, cx, 0);
> -	cx->time += idle_time;
>  	return index;
>  }
>  
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 87bb9d7..3998334 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -58,7 +58,6 @@ struct acpi_processor_cx {
>  	u32 address;
>  	u8 entry_method;
>  	u32 latency;
> -	u64 time;
>  	u8 bm_sts_skip;
>  	char desc[ACPI_CX_DESC_LEN];
>  };
> 

Re: [PATCH 1/5] acpi : remove latency_ticks from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the latency_ticks field as it is not used.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

This one looks good to me.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_idle.c |    2 --
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index f3decb3..da92c65 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -586,7 +586,6 @@ static void acpi_processor_power_verify_c3(struct acpi_processor *pr,
>  	 */
>  	cx->valid = 1;
>  
> -	cx->latency_ticks = cx->latency;
>  	/*
>  	 * On older chipsets, BM_RLD needs to be set
>  	 * in order for Bus Master activity to wake the
> @@ -619,7 +618,6 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
>  			if (!cx->address)
>  				break;
>  			cx->valid = 1; 
> -			cx->latency_ticks = cx->latency; /* Normalize latency */
>  			break;
>  
>  		case ACPI_STATE_C3:
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 9d65047..a70cbcc 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -59,7 +59,6 @@ struct acpi_processor_cx {
>  	u8 entry_method;
>  	u8 index;
>  	u32 latency;
> -	u32 latency_ticks;
>  	u32 power;
>  	u32 usage;
>  	u64 time;
> 

Re: [PATCH 1/5] acpi : remove latency_ticks from acpi_processor_cx structure

[Rafael J. Wysocki]

On Thursday, July 12, 2012, Daniel Lezcano wrote:
> Remove the latency_ticks field as it is not used.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Applied to the 'pm-cpuidle' branch of the linux-pm.git tree.

Thanks,
Rafael


> ---
>  drivers/acpi/processor_idle.c |    2 --
>  include/acpi/processor.h      |    1 -
>  2 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index f3decb3..da92c65 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -586,7 +586,6 @@ static void acpi_processor_power_verify_c3(struct acpi_processor *pr,
>  	 */
>  	cx->valid = 1;
>  
> -	cx->latency_ticks = cx->latency;
>  	/*
>  	 * On older chipsets, BM_RLD needs to be set
>  	 * in order for Bus Master activity to wake the
> @@ -619,7 +618,6 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
>  			if (!cx->address)
>  				break;
>  			cx->valid = 1; 
> -			cx->latency_ticks = cx->latency; /* Normalize latency */
>  			break;
>  
>  		case ACPI_STATE_C3:
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 9d65047..a70cbcc 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -59,7 +59,6 @@ struct acpi_processor_cx {
>  	u8 entry_method;
>  	u8 index;
>  	u32 latency;
> -	u32 latency_ticks;
>  	u32 power;
>  	u32 usage;
>  	u64 time;
> 

[PATCH 1/5] acpi : remove latency_ticks from acpi_processor_cx structure

[Daniel Lezcano]

Remove the latency_ticks field as it is not used.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/acpi/processor_idle.c |    2 --
 include/acpi/processor.h      |    1 -
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index b894627..9ef007d 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -583,7 +583,6 @@ static void acpi_processor_power_verify_c3(struct acpi_processor *pr,
 	 */
 	cx->valid = 1;
 
-	cx->latency_ticks = cx->latency;
 	/*
 	 * On older chipsets, BM_RLD needs to be set
 	 * in order for Bus Master activity to wake the
@@ -616,7 +615,6 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
 			if (!cx->address)
 				break;
 			cx->valid = 1; 
-			cx->latency_ticks = cx->latency; /* Normalize latency */
 			break;
 
 		case ACPI_STATE_C3:
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index ac3bff6..19423c3 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -59,7 +59,6 @@ struct acpi_processor_cx {
 	u8 entry_method;
 	u8 index;
 	u32 latency;
-	u32 latency_ticks;
 	u32 power;
 	u32 usage;
 	u64 time;
-- 
1.7.5.4

[PATCH 2/5] acpi : remove index from acpi_processor_cx structure

[Daniel Lezcano]

Remove the index field. It could be given without adding extra
information in the cx structure.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 arch/x86/kernel/acpi/cstate.c |   18 +++++++++---------
 drivers/acpi/processor_idle.c |   15 +++++++--------
 include/acpi/processor.h      |   11 +++++------
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index d2b7f27..faa7f50 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c
@@ -112,8 +112,8 @@ out:
 	return retval;
 }
 
-int acpi_processor_ffh_cstate_probe(unsigned int cpu,
-		struct acpi_processor_cx *cx, struct acpi_power_register *reg)
+int acpi_processor_ffh_cstate_probe(unsigned int cpu, struct acpi_processor_cx *cx,
+				    int index, struct acpi_power_register *reg)
 {
 	struct cstate_entry *percpu_entry;
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -126,16 +126,16 @@ int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 		return -1;
 
 	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
-	percpu_entry->states[cx->index].eax = 0;
-	percpu_entry->states[cx->index].ecx = 0;
+	percpu_entry->states[index].eax = 0;
+	percpu_entry->states[index].ecx = 0;
 
 	/* Make sure we are running on right CPU */
 
 	retval = work_on_cpu(cpu, acpi_processor_ffh_cstate_probe_cpu, cx);
 	if (retval == 0) {
 		/* Use the hint in CST */
-		percpu_entry->states[cx->index].eax = cx->address;
-		percpu_entry->states[cx->index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
+		percpu_entry->states[index].eax = cx->address;
+		percpu_entry->states[index].ecx = MWAIT_ECX_INTERRUPT_BREAK;
 	}
 
 	/*
@@ -173,14 +173,14 @@ void mwait_idle_with_hints(unsigned long ax, unsigned long cx)
 	}
 }
 
-void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
+void acpi_processor_ffh_cstate_enter(int index)
 {
 	unsigned int cpu = smp_processor_id();
 	struct cstate_entry *percpu_entry;
 
 	percpu_entry = per_cpu_ptr(cpu_cstate_entry, cpu);
-	mwait_idle_with_hints(percpu_entry->states[cx->index].eax,
-	                      percpu_entry->states[cx->index].ecx);
+	mwait_idle_with_hints(percpu_entry->states[index].eax,
+	                      percpu_entry->states[index].ecx);
 }
 EXPORT_SYMBOL_GPL(acpi_processor_ffh_cstate_enter);
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 9ef007d..0922143 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -430,12 +430,11 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 			current_count++;
 
 		cx.address = reg->address;
-		cx.index = current_count + 1;
 
 		cx.entry_method = ACPI_CSTATE_SYSTEMIO;
 		if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
-			if (acpi_processor_ffh_cstate_probe
-					(pr->id, &cx, reg) == 0) {
+			if (acpi_processor_ffh_cstate_probe(
+				    pr->id, &cx, current_count + 1, reg) == 0) {
 				cx.entry_method = ACPI_CSTATE_FFH;
 			} else if (cx.type == ACPI_STATE_C1) {
 				/*
@@ -704,13 +703,13 @@ static int acpi_idle_bm_check(void)
  *
  * Caller disables interrupt before call and enables interrupt after return.
  */
-static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx)
+static inline void acpi_idle_do_entry(struct acpi_processor_cx *cx, int index)
 {
 	/* Don't trace irqs off for idle */
 	stop_critical_timings();
 	if (cx->entry_method == ACPI_CSTATE_FFH) {
 		/* Call into architectural FFH based C-state */
-		acpi_processor_ffh_cstate_enter(cx);
+		acpi_processor_ffh_cstate_enter(index);
 	} else if (cx->entry_method == ACPI_CSTATE_HALT) {
 		acpi_safe_halt();
 	} else {
@@ -752,7 +751,7 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 
 	lapic_timer_state_broadcast(pr, cx, 1);
 	kt1 = ktime_get_real();
-	acpi_idle_do_entry(cx);
+	acpi_idle_do_entry(cx, index);
 	kt2 = ktime_get_real();
 	idle_time =  ktime_to_us(ktime_sub(kt2, kt1));
 
@@ -847,7 +846,7 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	kt1 = ktime_get_real();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
-	acpi_idle_do_entry(cx);
+	acpi_idle_do_entry(cx, index);
 	kt2 = ktime_get_real();
 	idle_time_ns = ktime_to_ns(ktime_sub(kt2, kt1));
 	idle_time = idle_time_ns;
@@ -960,7 +959,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev,
 		ACPI_FLUSH_CPU_CACHE();
 	}
 
-	acpi_idle_do_entry(cx);
+	acpi_idle_do_entry(cx, index);
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 19423c3..d2016a1 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -57,7 +57,6 @@ struct acpi_processor_cx {
 	u8 type;
 	u32 address;
 	u8 entry_method;
-	u8 index;
 	u32 latency;
 	u32 power;
 	u32 usage;
@@ -249,8 +248,9 @@ void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
 					unsigned int cpu);
 int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 				    struct acpi_processor_cx *cx,
+				    int index,
 				    struct acpi_power_register *reg);
-void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cstate);
+void acpi_processor_ffh_cstate_enter(int index);
 #else
 static inline void acpi_processor_power_init_bm_check(struct
 						      acpi_processor_flags
@@ -261,13 +261,12 @@ static inline void acpi_processor_power_init_bm_check(struct
 }
 static inline int acpi_processor_ffh_cstate_probe(unsigned int cpu,
 						  struct acpi_processor_cx *cx,
-						  struct acpi_power_register
-						  *reg)
+						  int index,
+						  struct acpi_power_register *reg)
 {
 	return -1;
 }
-static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
-						   *cstate)
+static inline void acpi_processor_ffh_cstate_enter(int index)
 {
 	return;
 }
-- 
1.7.5.4