[PATCH 0/3] ACPI: Some patches to prepare for running ACPI on !x86 and !ia64

Some of the ACPI code is arch-dependent and make the code can't be
compiled on !x86 and !ia64, the first two patches just do some rework
on the idle_boot_override and _PDC related stuff to make the ACPI
code more arch-independent.

The third patch just introduce map_gic_id() for ACPI processor core
followed by the ACPI 5.0 spec.

These three patches are just ACPI related so I send them out as
a separate patch set.

I have compiled the kernel successful after appling this patch set
on x86, ia64 and powerpc(with cross compile tool).

Changes since last RFC version:
a) Remove the RFC tag;
b) Move idle_boot_override out of the arch directory suggested
   by Alan;
c) Make these 3 patches as a separate patch set since there are
   not not related to the ARM/ARM64 platform.

Hanjun Guo (3):
  ACPI / idle: Move idle_boot_override out of the arch directory
  ACPI / processor_core: Rework _PDC related stuff to make it more
    arch-independent
  ACPI: Introduce map_gic_id() to get apic id from MADT or _MAT method

 arch/ia64/include/asm/acpi.h         |  5 +---
 arch/ia64/include/asm/processor.h    |  3 ---
 arch/ia64/kernel/acpi.c              | 17 +++++++++++++
 arch/powerpc/include/asm/processor.h |  1 -
 arch/x86/include/asm/acpi.h          | 19 +--------------
 arch/x86/include/asm/processor.h     |  3 ---
 arch/x86/kernel/acpi/cstate.c        | 31 ++++++++++++++++++++++++
 arch/x86/kernel/process.c            |  1 +
 drivers/acpi/processor_core.c        | 47 +++++++++++++++++++++---------------
 include/linux/cpu.h                  |  8 ++++++
 10 files changed, 87 insertions(+), 48 deletions(-)

-- 
1.8.2.2

Move idle_boot_override out of the arch directory to be a single enum
including both platforms values, this will make it rather easier to
avoid ifdefs around which definitions are for which processor in
generally used ACPI code.

IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.

No functional change in this patch.

Suggested-by: Alan <redacted>
Signed-off-by: Hanjun Guo <redacted>
---
 arch/ia64/include/asm/processor.h    | 3 ---
 arch/powerpc/include/asm/processor.h | 1 -
 arch/x86/include/asm/processor.h     | 3 ---
 arch/x86/kernel/process.c            | 1 +
 include/linux/cpu.h                  | 8 ++++++++
 5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/ia64/include/asm/processor.h b/arch/ia64/include/asm/processor.h
index 5a84b3a..ccd63a0 100644
--- a/arch/ia64/include/asm/processor.h
+++ b/arch/ia64/include/asm/processor.h

@@ -698,9 +698,6 @@ prefetchw (const void *x)
 
 extern unsigned long boot_option_idle_override;
 
-enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_FORCE_MWAIT,
-			 IDLE_NOMWAIT, IDLE_POLL};
-
 void default_idle(void);
 
 #define ia64_platform_is(x) (strcmp(x, ia64_platform_name) == 0)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index fc14a38..06689c0 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h

@@ -440,7 +440,6 @@ static inline unsigned long get_clean_sp(unsigned long sp, int is_32)
 #endif
 
 extern unsigned long cpuidle_disable;
-enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
 
 extern int powersave_nap;	/* set if nap mode can be used in idle loop */
 extern void power7_nap(void);

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 7b034a4..4bee51a 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h

@@ -729,9 +729,6 @@ extern void init_amd_e400_c1e_mask(void);
 extern unsigned long		boot_option_idle_override;
 extern bool			amd_e400_c1e_detected;
 
-enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_NOMWAIT,
-			 IDLE_POLL};
-
 extern void enable_sep_cpu(void);
 extern int sysenter_setup(void);
 

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 3fb8d95..62764ff 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c

@@ -17,6 +17,7 @@
 #include <linux/stackprotector.h>
 #include <linux/tick.h>
 #include <linux/cpuidle.h>
+#include <linux/cpu.h>
 #include <trace/events/power.h>
 #include <linux/hw_breakpoint.h>
 #include <asm/cpu.h>

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 03e235ad..e324561 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h

@@ -220,6 +220,14 @@ void cpu_idle(void);
 
 void cpu_idle_poll_ctrl(bool enable);
 
+enum idle_boot_override {
+	IDLE_NO_OVERRIDE = 0,
+	IDLE_HALT,
+	IDLE_NOMWAIT,
+	IDLE_POLL,
+	IDLE_POWERSAVE_OFF
+};
+
 void arch_cpu_idle(void);
 void arch_cpu_idle_prepare(void);
 void arch_cpu_idle_enter(void);

-- 
1.8.2.2

_PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
rework the code to make it more arch-independent, no functional change
in this patch.

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

Signed-off-by: Hanjun Guo <redacted>
Signed-off-by: Graeme Gregory <redacted>
---
 arch/ia64/include/asm/acpi.h  |  5 +----
 arch/ia64/kernel/acpi.c       | 17 +++++++++++++++++
 arch/x86/include/asm/acpi.h   | 19 +------------------
 arch/x86/kernel/acpi/cstate.c | 31 +++++++++++++++++++++++++++++++
 drivers/acpi/processor_core.c | 21 ++-------------------
 5 files changed, 52 insertions(+), 41 deletions(-)

diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h
index faa1bf0..3348d07 100644
--- a/arch/ia64/include/asm/acpi.h
+++ b/arch/ia64/include/asm/acpi.h

@@ -154,10 +154,7 @@ extern int __initdata nid_to_pxm_map[MAX_NUMNODES];
 #endif
 
 static inline bool arch_has_acpi_pdc(void) { return true; }
-static inline void arch_acpi_set_pdc_bits(u32 *buf)
-{
-	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
-}
+extern void arch_acpi_set_pdc_bits(u32 *buf);
 
 #define acpi_unlazy_tlb(x)
 

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index 59d52e3..d126f15 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c

@@ -1018,3 +1018,20 @@ EXPORT_SYMBOL(acpi_unregister_ioapic);
  * TBD when when IA64 starts to support suspend...
  */
 int acpi_suspend_lowlevel(void) { return 0; }
+
+void arch_acpi_set_pdc_bits(u32 *buf)
+{
+	/* 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);
+
+	}
+
+	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
+}

diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index c8c1e70..e9f71bc 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h

@@ -147,24 +147,7 @@ static inline bool arch_has_acpi_pdc(void)
 		c->x86_vendor == X86_VENDOR_CENTAUR);
 }
 
-static inline void arch_acpi_set_pdc_bits(u32 *buf)
-{
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
-
-	if (cpu_has(c, X86_FEATURE_EST))
-		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
-
-	if (cpu_has(c, X86_FEATURE_ACPI))
-		buf[2] |= ACPI_PDC_T_FFH;
-
-	/*
-	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
-	 */
-	if (!cpu_has(c, X86_FEATURE_MWAIT))
-		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
-}
+extern void arch_acpi_set_pdc_bits(u32 *buf);
 
 #else /* !CONFIG_ACPI */
 

diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index d2b7f27..b132748 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c

@@ -16,6 +16,37 @@
 #include <asm/mwait.h>
 #include <asm/special_insns.h>
 
+void arch_acpi_set_pdc_bits(u32 *buf)
+{
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	/* 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);
+
+	}
+
+	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
+
+	if (cpu_has(c, X86_FEATURE_EST))
+		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
+
+	if (cpu_has(c, X86_FEATURE_ACPI))
+		buf[2] |= ACPI_PDC_T_FFH;
+
+	/*
+	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
+	 */
+	if (!cpu_has(c, X86_FEATURE_MWAIT))
+		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
+}
+
 /*
  * Initialize bm_flags based on the CPU cache properties
  * On SMP it depends on cache configuration

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index b3171f3..cd7b5fe 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c

@@ -274,9 +274,6 @@ static void acpi_set_pdc_bits(u32 *buf)
 	buf[0] = ACPI_PDC_REVISION_ID;
 	buf[1] = 1;
 
-	/* Enable coordination with firmware's _TSD info */
-	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
-
 	/* Twiddle arch-specific bits needed for _PDC */
 	arch_acpi_set_pdc_bits(buf);
 }

@@ -301,7 +298,7 @@ static struct acpi_object_list *acpi_processor_alloc_pdc(void)
 		return NULL;
 	}
 
-	buf = kmalloc(12, GFP_KERNEL);
+	buf = kzalloc(12, GFP_KERNEL);
 	if (!buf) {
 		printk(KERN_ERR "Memory allocation error\n");
 		kfree(obj);

@@ -324,25 +321,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))

-- 
1.8.2.2

Get apic id from MADT or _MAT method is not implemented on arm/arm64,
and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
map_gic_id() to get apic id followed the ACPI 5.0 spec.

Signed-off-by: Hanjun Guo <redacted>
---
 drivers/acpi/processor_core.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index cd7b5fe..165eac7 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c

@@ -90,6 +90,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
 	return 1;
 }
 
+static int map_gic_id(struct acpi_subtable_header *entry,
+		int device_declaration, u32 acpi_id, int *apic_id)
+{
+	struct acpi_madt_generic_interrupt *gic =
+		(struct acpi_madt_generic_interrupt *)entry;
+
+	if (!(gic->flags & ACPI_MADT_ENABLED))
+		return 0;
+
+	/* In the GIC interrupt model, logical processors are
+	 * required to have a Processor Device object in the DSDT,
+	 * so we should check device_declaration here
+	 */
+	if (device_declaration && (gic->uid == acpi_id)) {
+		*apic_id = gic->gic_id;
+		return 1;
+	}
+
+	return 0;
+}
+
 static int map_madt_entry(int type, u32 acpi_id)
 {
 	unsigned long madt_end, entry;

@@ -125,6 +146,9 @@ static int map_madt_entry(int type, u32 acpi_id)
 		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
 			if (map_lsapic_id(header, type, acpi_id, &apic_id))
 				break;
+		} else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
+			if (map_gic_id(header, type, acpi_id, &apic_id))
+				break;
 		}
 		entry += header->length;
 	}

@@ -155,6 +179,8 @@ static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
 		map_lapic_id(header, acpi_id, &apic_id);
 	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
 		map_lsapic_id(header, type, acpi_id, &apic_id);
+	} else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
+		map_gic_id(header, type, acpi_id, &apic_id);
 	}
 
 exit:

-- 
1.8.2.2

On 17/01/14 02:03, Hanjun Guo wrote:

I don't think it is used in the context of ACPI. Though it's same variable name,
it looks like it just used as boot to override the cpuidle option.
Does it still make any sense to combine this ?

I do understand the idea behind this change, but IMO HALT and MWAIT are x86
specific and may not make sense for other architectures.

It will also require every architecture using ACPI to export
boot_option_idle_override which may not be really required.

Further the only users of boot_option_idle_override(outside x86) are:

1. drivers/acpi/processor_core.c
   Your second patch is moving this to x86 specific code anyway

2. drivers/acpi/processor_idle.c
   Currently idle driver is bit x86 specific and needs modifications to get it
   working on ARM

Regards,
Sudeep

On 2014-1-17 20:06, Sudeep Holla wrote:

On 17/01/14 02:03, Hanjun Guo wrote:

quoted

Move idle_boot_override out of the arch directory to be a single enum
including both platforms values, this will make it rather easier to
avoid ifdefs around which definitions are for which processor in
generally used ACPI code.

IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.

No functional change in this patch.

Suggested-by: Alan <redacted>
Signed-off-by: Hanjun Guo <redacted>
---
 arch/ia64/include/asm/processor.h    | 3 ---
 arch/powerpc/include/asm/processor.h | 1 -
 arch/x86/include/asm/processor.h     | 3 ---
 arch/x86/kernel/process.c            | 1 +
 include/linux/cpu.h                  | 8 ++++++++
 5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/ia64/include/asm/processor.h b/arch/ia64/include/asm/processor.h
index 5a84b3a..ccd63a0 100644
--- a/arch/ia64/include/asm/processor.h
+++ b/arch/ia64/include/asm/processor.h

@@ -698,9 +698,6 @@ prefetchw (const void *x)
 
 extern unsigned long boot_option_idle_override;
 
-enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_FORCE_MWAIT,
-			 IDLE_NOMWAIT, IDLE_POLL};
-
 void default_idle(void);
 
 #define ia64_platform_is(x) (strcmp(x, ia64_platform_name) == 0)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index fc14a38..06689c0 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h

@@ -440,7 +440,6 @@ static inline unsigned long get_clean_sp(unsigned long sp, int is_32)
 #endif
 
 extern unsigned long cpuidle_disable;
-enum idle_boot_override {IDLE_NO_OVERRIDE = 0, IDLE_POWERSAVE_OFF};
 

I don't think it is used in the context of ACPI. Though it's same variable name,
it looks like it just used as boot to override the cpuidle option.
Does it still make any sense to combine this ?

Yes, it is not related to ACPI on powerpc, I will investigate it will cause
compile warning or not if I don't combine this.

quoted

 extern int powersave_nap;	/* set if nap mode can be used in idle loop */
 extern void power7_nap(void);

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 7b034a4..4bee51a 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h

@@ -729,9 +729,6 @@ extern void init_amd_e400_c1e_mask(void);
 extern unsigned long		boot_option_idle_override;
 extern bool			amd_e400_c1e_detected;
 
-enum idle_boot_override {IDLE_NO_OVERRIDE=0, IDLE_HALT, IDLE_NOMWAIT,
-			 IDLE_POLL};
-
 extern void enable_sep_cpu(void);
 extern int sysenter_setup(void);
 

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 3fb8d95..62764ff 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c

@@ -17,6 +17,7 @@
 #include <linux/stackprotector.h>
 #include <linux/tick.h>
 #include <linux/cpuidle.h>
+#include <linux/cpu.h>
 #include <trace/events/power.h>
 #include <linux/hw_breakpoint.h>
 #include <asm/cpu.h>

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 03e235ad..e324561 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h

@@ -220,6 +220,14 @@ void cpu_idle(void);
 
 void cpu_idle_poll_ctrl(bool enable);
 
+enum idle_boot_override {
+	IDLE_NO_OVERRIDE = 0,
+	IDLE_HALT,
+	IDLE_NOMWAIT,
+	IDLE_POLL,
+	IDLE_POWERSAVE_OFF
+};
+

I do understand the idea behind this change, but IMO HALT and MWAIT are x86
specific and may not make sense for other architectures.

yes, this is the strange part, the value is arch-dependent.

It will also require every architecture using ACPI to export
boot_option_idle_override which may not be really required.

so, how about forget this patch and move boot_option_idle_override
related code into arch directory such as arch/x86/acpi/boot.c for
x86?

Further the only users of boot_option_idle_override(outside x86) are:

1. drivers/acpi/processor_core.c
   Your second patch is moving this to x86 specific code anyway

2. drivers/acpi/processor_idle.c
   Currently idle driver is bit x86 specific and needs modifications to get it
   working on ARM

Yes, That's why I did not enable acpi idle driver on ARM64 for now.

Thanks
Hanjun

On 2014-1-18 11:45, Hanjun Guo wrote:

On 2014-1-17 20:06, Sudeep Holla wrote:

quoted

On 17/01/14 02:03, Hanjun Guo wrote:

quoted

Move idle_boot_override out of the arch directory to be a single enum
including both platforms values, this will make it rather easier to
avoid ifdefs around which definitions are for which processor in
generally used ACPI code.

IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.

No functional change in this patch.

Suggested-by: Alan <redacted>
Signed-off-by: Hanjun Guo <redacted>
---

[...]

quoted

quoted

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 03e235ad..e324561 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h

@@ -220,6 +220,14 @@ void cpu_idle(void);
 
 void cpu_idle_poll_ctrl(bool enable);
 
+enum idle_boot_override {
+	IDLE_NO_OVERRIDE = 0,
+	IDLE_HALT,
+	IDLE_NOMWAIT,
+	IDLE_POLL,
+	IDLE_POWERSAVE_OFF
+};
+

I do understand the idea behind this change, but IMO HALT and MWAIT are x86
specific and may not make sense for other architectures.

yes, this is the strange part, the value is arch-dependent.

quoted

It will also require every architecture using ACPI to export
boot_option_idle_override which may not be really required.

so, how about forget this patch and move boot_option_idle_override
related code into arch directory such as arch/x86/acpi/boot.c for
x86?

The general idea is that we can move all the arch-dependent codes
in ACPI driver to arch directory, then make codes in drivers/acpi/
arch independent.

Thanks
Hanjun

On Saturday, January 18, 2014 11:52:18 AM Hanjun Guo wrote:

On 2014-1-18 11:45, Hanjun Guo wrote:

quoted

On 2014-1-17 20:06, Sudeep Holla wrote:

quoted

On 17/01/14 02:03, Hanjun Guo wrote:

quoted

Move idle_boot_override out of the arch directory to be a single enum
including both platforms values, this will make it rather easier to
avoid ifdefs around which definitions are for which processor in
generally used ACPI code.

IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.

No functional change in this patch.

Suggested-by: Alan <redacted>
Signed-off-by: Hanjun Guo <redacted>
---

[...]

quoted

quoted

quoted

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 03e235ad..e324561 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h

@@ -220,6 +220,14 @@ void cpu_idle(void);
 
 void cpu_idle_poll_ctrl(bool enable);
 
+enum idle_boot_override {
+	IDLE_NO_OVERRIDE = 0,
+	IDLE_HALT,
+	IDLE_NOMWAIT,
+	IDLE_POLL,
+	IDLE_POWERSAVE_OFF
+};
+

I do understand the idea behind this change, but IMO HALT and MWAIT are x86
specific and may not make sense for other architectures.

yes, this is the strange part, the value is arch-dependent.

quoted

It will also require every architecture using ACPI to export
boot_option_idle_override which may not be really required.

so, how about forget this patch and move boot_option_idle_override
related code into arch directory such as arch/x86/acpi/boot.c for
x86?

The general idea is that we can move all the arch-dependent codes
in ACPI driver to arch directory, then make codes in drivers/acpi/
arch independent.

Well, MWAIT is arch-dependent, so I'm not sure how IDLE_NOMWAIT fits into
include/linux/cpu.h?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

On 2014?01?18? 21:47, Rafael J. Wysocki wrote:

On Saturday, January 18, 2014 11:52:18 AM Hanjun Guo wrote:

quoted

On 2014-1-18 11:45, Hanjun Guo wrote:

quoted

On 2014-1-17 20:06, Sudeep Holla wrote:

quoted

On 17/01/14 02:03, Hanjun Guo wrote:

quoted

Move idle_boot_override out of the arch directory to be a single enum
including both platforms values, this will make it rather easier to
avoid ifdefs around which definitions are for which processor in
generally used ACPI code.

IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.

No functional change in this patch.

Suggested-by: Alan <redacted>
Signed-off-by: Hanjun Guo <redacted>
---

[...]

quoted

quoted

quoted

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 03e235ad..e324561 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h

@@ -220,6 +220,14 @@ void cpu_idle(void);
  
  void cpu_idle_poll_ctrl(bool enable);
  
+enum idle_boot_override {
+	IDLE_NO_OVERRIDE = 0,
+	IDLE_HALT,
+	IDLE_NOMWAIT,
+	IDLE_POLL,
+	IDLE_POWERSAVE_OFF
+};
+

I do understand the idea behind this change, but IMO HALT and MWAIT are x86
specific and may not make sense for other architectures.

yes, this is the strange part, the value is arch-dependent.

quoted

It will also require every architecture using ACPI to export
boot_option_idle_override which may not be really required.

so, how about forget this patch and move boot_option_idle_override
related code into arch directory such as arch/x86/acpi/boot.c for
x86?

The general idea is that we can move all the arch-dependent codes
in ACPI driver to arch directory, then make codes in drivers/acpi/
arch independent.

Well, MWAIT is arch-dependent, so I'm not sure how IDLE_NOMWAIT fits into
include/linux/cpu.h?

So you will not happy with this patch and should find another solution?

Thanks
Hanjun

On Monday, January 20, 2014 10:08:41 PM Hanjun Guo wrote:

On 2014?01?18? 21:47, Rafael J. Wysocki wrote:

quoted

On Saturday, January 18, 2014 11:52:18 AM Hanjun Guo wrote:

quoted

On 2014-1-18 11:45, Hanjun Guo wrote:

quoted

On 2014-1-17 20:06, Sudeep Holla wrote:

quoted

On 17/01/14 02:03, Hanjun Guo wrote:

quoted

Move idle_boot_override out of the arch directory to be a single enum
including both platforms values, this will make it rather easier to
avoid ifdefs around which definitions are for which processor in
generally used ACPI code.

IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.

No functional change in this patch.

Suggested-by: Alan <redacted>
Signed-off-by: Hanjun Guo <redacted>
---

[...]

quoted

quoted

quoted

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 03e235ad..e324561 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h

@@ -220,6 +220,14 @@ void cpu_idle(void);
  
  void cpu_idle_poll_ctrl(bool enable);
  
+enum idle_boot_override {
+	IDLE_NO_OVERRIDE = 0,
+	IDLE_HALT,
+	IDLE_NOMWAIT,
+	IDLE_POLL,
+	IDLE_POWERSAVE_OFF
+};
+

I do understand the idea behind this change, but IMO HALT and MWAIT are x86
specific and may not make sense for other architectures.

yes, this is the strange part, the value is arch-dependent.

quoted

It will also require every architecture using ACPI to export
boot_option_idle_override which may not be really required.

so, how about forget this patch and move boot_option_idle_override
related code into arch directory such as arch/x86/acpi/boot.c for
x86?

The general idea is that we can move all the arch-dependent codes
in ACPI driver to arch directory, then make codes in drivers/acpi/
arch independent.

Well, MWAIT is arch-dependent, so I'm not sure how IDLE_NOMWAIT fits into
include/linux/cpu.h?

So you will not happy with this patch and should find another solution?

No, I'm not happy with it.

If you want to move that to an arch-agnostic header, the symbol names cannot
be arch-dependent any more.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

On 2014-1-21 7:34, Rafael J. Wysocki wrote:

On Monday, January 20, 2014 10:08:41 PM Hanjun Guo wrote:

quoted

On 2014?01?18? 21:47, Rafael J. Wysocki wrote:

quoted

On Saturday, January 18, 2014 11:52:18 AM Hanjun Guo wrote:

quoted

On 2014-1-18 11:45, Hanjun Guo wrote:

quoted

On 2014-1-17 20:06, Sudeep Holla wrote:

quoted

On 17/01/14 02:03, Hanjun Guo wrote:

quoted

Move idle_boot_override out of the arch directory to be a single enum
including both platforms values, this will make it rather easier to
avoid ifdefs around which definitions are for which processor in
generally used ACPI code.

IDLE_FORCE_MWAIT for IA64 is not used anywhere, so romove it.

No functional change in this patch.

Suggested-by: Alan <redacted>
Signed-off-by: Hanjun Guo <redacted>
---

[...]

quoted

quoted

quoted

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 03e235ad..e324561 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h

@@ -220,6 +220,14 @@ void cpu_idle(void);
  
  void cpu_idle_poll_ctrl(bool enable);
  
+enum idle_boot_override {
+	IDLE_NO_OVERRIDE = 0,
+	IDLE_HALT,
+	IDLE_NOMWAIT,
+	IDLE_POLL,
+	IDLE_POWERSAVE_OFF
+};
+

I do understand the idea behind this change, but IMO HALT and MWAIT are x86
specific and may not make sense for other architectures.

yes, this is the strange part, the value is arch-dependent.

quoted

It will also require every architecture using ACPI to export
boot_option_idle_override which may not be really required.

so, how about forget this patch and move boot_option_idle_override
related code into arch directory such as arch/x86/acpi/boot.c for
x86?

The general idea is that we can move all the arch-dependent codes
in ACPI driver to arch directory, then make codes in drivers/acpi/
arch independent.

Well, MWAIT is arch-dependent, so I'm not sure how IDLE_NOMWAIT fits into
include/linux/cpu.h?

So you will not happy with this patch and should find another solution?

No, I'm not happy with it.

If you want to move that to an arch-agnostic header, the symbol names cannot
be arch-dependent any more.

Ok, will find another solution for that, thanks for your comments :)

Hanjun