| Message ID | 1386088611-2801-4-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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()?
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
On Tue, Dec 03, 2013 at 04:51:40PM +0000, 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 The arguments to _PDC are architecture specific, so there do need to be code changes here.
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 >
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 --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))