diff mbox

[RFC,part1,3/7] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

Message ID 1386088611-2801-4-git-send-email-hanjun.guo@linaro.org
State New
Headers show

Commit Message

Hanjun Guo Dec. 3, 2013, 4:36 p.m. UTC 
_PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
rework the code to make it more arch-independent.

The return value of acpi_processor_eval_pdc() should be 'acpi_status' but
defined as 'int', fix it too.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
---
 drivers/acpi/processor_core.c |   27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

Comments

Matthew Garrett Dec. 3, 2013, 4:46 p.m. UTC | #1 
On Wed, Dec 04, 2013 at 12:36:47AM +0800, Hanjun Guo wrote:

> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>  	/* Enable coordination with firmware's _TSD info */
>  	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
> +	if (boot_option_idle_override == IDLE_NOMWAIT) {
> +		/*
> +		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
> +		 * mode will be disabled in the parameter of _PDC object.
> +		 * Of course C1_FFH access mode will also be disabled.
> +		 */
> +		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> +
> +	}
> +#endif

This is (fairly) arch-specific, so why not move it to 
arch_acpi_set_pdc_bits()?
Alan Cox Dec. 3, 2013, 4:51 p.m. UTC | #2 
On Wed,  4 Dec 2013 00:36:47 +0800
Hanjun Guo <hanjun.guo@linaro.org> wrote:

> _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
> rework the code to make it more arch-independent.
> 
> The return value of acpi_processor_eval_pdc() should be 'acpi_status' but
> defined as 'int', fix it too.

Why not just define boot_options_idle_override as well. Then you can
leave the code unchanged. Also more importantly you can have override
values for ARM when it turns out you need those too and the logic will be
the same for both processor families

Alan
Hanjun Guo Dec. 4, 2013, 2:11 p.m. UTC | #3 
On 2013年12月04日 00:46, Matthew Garrett wrote:
> On Wed, Dec 04, 2013 at 12:36:47AM +0800, Hanjun Guo wrote:
>
>> +#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>>   	/* Enable coordination with firmware's _TSD info */
>>   	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
>> +	if (boot_option_idle_override == IDLE_NOMWAIT) {
>> +		/*
>> +		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
>> +		 * mode will be disabled in the parameter of _PDC object.
>> +		 * Of course C1_FFH access mode will also be disabled.
>> +		 */
>> +		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
>> +
>> +	}
>> +#endif
> This is (fairly) arch-specific, so why not move it to
> arch_acpi_set_pdc_bits()?

Ok, it will make the code much cleaner, will update in next version.

Thanks
Hanjun


>
Hanjun Guo Dec. 4, 2013, 2:16 p.m. UTC | #4 
On 2013年12月04日 00:51, One Thousand Gnomes wrote:
> On Wed,  4 Dec 2013 00:36:47 +0800
> Hanjun Guo <hanjun.guo@linaro.org> wrote:
>
>> _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
>> rework the code to make it more arch-independent.
>>
>> The return value of acpi_processor_eval_pdc() should be 'acpi_status' but
>> defined as 'int', fix it too.
> Why not just define boot_options_idle_override as well. Then you can
> leave the code unchanged. Also more importantly you can have override
> values for ARM when it turns out you need those too and the logic will be
> the same for both processor families

There is a platform dependent head file such as pdc_intel.h which contains
some macros, some of those macros are used in arch-independent file processor_core.c,
that's why I posted this patch.

Thanks
Hanjun
diff mbox

Patch

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 34e7b3c..9931435 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -273,8 +273,19 @@  static void acpi_set_pdc_bits(u32 *buf)
 	buf[0] = ACPI_PDC_REVISION_ID;
 	buf[1] = 1;
 
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
 	/* Enable coordination with firmware's _TSD info */
 	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
+	if (boot_option_idle_override == IDLE_NOMWAIT) {
+		/*
+		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
+		 * mode will be disabled in the parameter of _PDC object.
+		 * Of course C1_FFH access mode will also be disabled.
+		 */
+		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
+
+	}
+#endif
 
 	/* Twiddle arch-specific bits needed for _PDC */
 	arch_acpi_set_pdc_bits(buf);
@@ -323,25 +334,11 @@  static struct acpi_object_list *acpi_processor_alloc_pdc(void)
  * _PDC is required for a BIOS-OS handshake for most of the newer
  * ACPI processor features.
  */
-static int
+static acpi_status
 acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
 {
 	acpi_status status = AE_OK;
 
-	if (boot_option_idle_override == IDLE_NOMWAIT) {
-		/*
-		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
-		 * mode will be disabled in the parameter of _PDC object.
-		 * Of course C1_FFH access mode will also be disabled.
-		 */
-		union acpi_object *obj;
-		u32 *buffer = NULL;
-
-		obj = pdc_in->pointer;
-		buffer = (u32 *)(obj->buffer.pointer);
-		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
-
-	}
 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
 
 	if (ACPI_FAILURE(status))