[PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Pallipadi, Venkatesh]

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


Patch 1/3: Changes to ACPI P-state driver, to handle MSR 
based transitions frequency transitions and make the driver 
SMP aware.


diffstat dbs1.patch 
arch/i386/kernel/cpu/cpufreq/Kconfig |    4 
arch/i386/kernel/cpu/cpufreq/acpi.c  |  141
+++++++++++++++++++++++++++--------
include/acpi/processor.h             |    1 
3 files changed, 116 insertions(+), 30 deletions(-)



diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c
linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c
2003-10-20 00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c
2003-10-20 02:11:01.000000000 -0700
@@ -99,7 +99,9 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	perf->space_id = (int) reg->space_id;
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE)
{
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d]
(control_register)\n",
 			(u32) reg->space_id));
@@ -126,7 +128,8 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE)
{
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d]
(status_register)\n",
 			(u32) reg->space_id));
@@ -224,16 +227,100 @@ end:
 	return_VALUE(result);
 }
 
+struct set_performance_param {
+	int 					cpu;
+	struct acpi_processor_performance	*perf;
+	unsigned int 				state;
+	unsigned int 				async;
+	unsigned int 				retval;
+};
+
+static void
+target_cpu_set_performance(struct set_performance_param	*param)
+{
+	struct acpi_processor_performance	*perf = param->perf;
+	u16		port = 0;
+	int		state = param->state;
+	unsigned int	value = perf->states[state].control;
+	int		i = 0;
+
+	ACPI_FUNCTION_TRACE("target_cpu_set_performance");
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Writing 0x%x to MSR
0x%x\n", 
+					value, perf->control_register));
+		wrmsr(perf->control_register, value, 0);
+	} else {
+		port = perf->control_register;
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Writing 0x%04x to port 0x%04x\n", value,
port));
+
+		outl(value, port); 
+	}
+
+	if (param->async) {
+		/* We don't want to wait for the status change here, as 
+		 * that will affect performance, especially with
frequent 
+		 * frequency switches. We assume the tranisition 
+		 * gone through and continue.
+		 */
+		param->retval = perf->states[state].status;
+		return_VOID;
+	}
+
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%02x from MSR 0x%04x\n",
+			(u16) perf->states[state].status, 
+			perf->status_register));
+
+		for (i = 0; i < 100; i++) {
+			unsigned long unused_hi;
+			rdmsr(perf->status_register, value, unused_hi);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	} else {
+		port = perf->status_register;
+
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%04x from port 0x%04x\n",
+			(u16) perf->states[state].status, port));
+
+		for (i = 0; i < 100; i++) {
+			value = inl(port);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	}
+	param->retval = value;
+	return_VOID;
+}
+
+#ifdef CONFIG_SMP
+static void
+target_cpu_set_performance_ipi(void *data)
+{
+	struct set_performance_param	*param;
+
+	param = (struct set_performance_param *) data;
+	if (param->cpu != smp_processor_id())
+		return;
+
+	target_cpu_set_performance(param);
+}
+#endif
 
 static int
 acpi_processor_set_performance (
 	struct acpi_processor_performance	*perf,
 	int			state)
 {
-	u16			port = 0;
-	u16			value = 0;
+	unsigned int		value = 0;
 	int			i = 0;
 	struct cpufreq_freqs    cpufreq_freqs;
+	struct set_performance_param param;
 
 	ACPI_FUNCTION_TRACE("acpi_processor_set_performance");
 
@@ -267,8 +354,12 @@ acpi_processor_set_performance (
 
 	/* cpufreq frequency struct */
 	cpufreq_freqs.cpu = perf->pr->id;
-	cpufreq_freqs.old = perf->states[perf->state].core_frequency;
-	cpufreq_freqs.new = perf->states[state].core_frequency;
+	/*
+	 * cpufreq_notifier needs to send the freq in KHz. But acpi
+	 * data we have is in MHz
+	 */
+	cpufreq_freqs.old = perf->states[perf->state].core_frequency *
1000;
+	cpufreq_freqs.new = perf->states[state].core_frequency * 1000;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
@@ -276,35 +367,27 @@ acpi_processor_set_performance (
 	/*
 	 * First we write the target state's 'control' value to the
 	 * control_register.
-	 */
-
-	port = perf->control_register;
-	value = (u16) perf->states[state].control;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Writing 0x%04x to port 0x%04x\n", value, port));
-
-	outw(value, port); 
-
-	/*
 	 * Then we read the 'status_register' and compare the value with
the
 	 * target state's 'status' to make sure the transition was
successful.
 	 * Note that we'll poll for up to 1ms (100 cycles of 10us)
before
 	 * giving up.
 	 */
 
-	port = perf->status_register;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Looking for 0x%04x from port 0x%04x\n",
-		(u16) perf->states[state].status, port));
-
-	for (i=0; i<100; i++) {
-		value = inw(port);
-		if (value == (u16) perf->states[state].status)
-			break;
-		udelay(10);
-	}
+	param.cpu = perf - performance;
+	param.perf = perf;
+	param.state = state;
+#ifdef CONFIG_SMP
+	param.async = 1; /* We don't want to wait in IPI context */
+	if (param.cpu != smp_processor_id())
+		smp_call_function(target_cpu_set_performance_ipi,
+				(void *)&param, 1, 1);
+	else
+		target_cpu_set_performance(&param);
+#else
+	param.async = 0;
+	target_cpu_set_performance(&param);
+#endif
+	value = param.retval;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig
linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig
2003-10-14 16:28:32.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig
2003-10-20 02:27:39.000000000 -0700
@@ -36,7 +36,9 @@ config X86_ACPI_CPUFREQ
 	depends on CPU_FREQ_TABLE && ACPI_PROCESSOR
 	help
 	  This driver adds a CPUFreq driver which utilizes the ACPI
-	  Processor Performance States.
+	  Processor Performance States. This can work with variety
+	  of platforms and support both Intel Speedstep and Intel
Enhanced 
+	  Speedstep, as long as BIOS-ACPI provides the P-state
information.
 
 	  For details, take a look at linux/Documentation/cpu-freq. 
 
diff -purN linux-2.6.0-test7/include/acpi/processor.h
linux-2.6.0-test7-dbs/include/acpi/processor.h
--- linux-2.6.0-test7/include/acpi/processor.h	2003-10-20
00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/include/acpi/processor.h	2003-10-20
02:11:08.000000000 -0700
@@ -73,6 +73,7 @@ struct acpi_processor_performance {
 	u16			control_register;
 	u16			status_register;
 	int			state_count;
+	int			space_id;
 	struct acpi_processor_px states[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct cpufreq_frequency_table
freq_table[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct acpi_processor   *pr;

[-- Attachment #2: dbs1.patch --]
[-- Type: application/octet-stream, Size: 7053 bytes --]

diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c	2003-10-20 00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c	2003-10-20 02:11:01.000000000 -0700
@@ -99,7 +99,9 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	perf->space_id = (int) reg->space_id;
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d] (control_register)\n",
 			(u32) reg->space_id));
@@ -126,7 +128,8 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d] (status_register)\n",
 			(u32) reg->space_id));
@@ -224,16 +227,100 @@ end:
 	return_VALUE(result);
 }
 
+struct set_performance_param {
+	int 					cpu;
+	struct acpi_processor_performance	*perf;
+	unsigned int 				state;
+	unsigned int 				async;
+	unsigned int 				retval;
+};
+
+static void
+target_cpu_set_performance(struct set_performance_param	*param)
+{
+	struct acpi_processor_performance	*perf = param->perf;
+	u16		port = 0;
+	int		state = param->state;
+	unsigned int	value = perf->states[state].control;
+	int		i = 0;
+
+	ACPI_FUNCTION_TRACE("target_cpu_set_performance");
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Writing 0x%x to MSR 0x%x\n", 
+					value, perf->control_register));
+		wrmsr(perf->control_register, value, 0);
+	} else {
+		port = perf->control_register;
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Writing 0x%04x to port 0x%04x\n", value, port));
+
+		outl(value, port); 
+	}
+
+	if (param->async) {
+		/* We don't want to wait for the status change here, as 
+		 * that will affect performance, especially with frequent 
+		 * frequency switches. We assume the tranisition 
+		 * gone through and continue.
+		 */
+		param->retval = perf->states[state].status;
+		return_VOID;
+	}
+
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%02x from MSR 0x%04x\n",
+			(u16) perf->states[state].status, 
+			perf->status_register));
+
+		for (i = 0; i < 100; i++) {
+			unsigned long unused_hi;
+			rdmsr(perf->status_register, value, unused_hi);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	} else {
+		port = perf->status_register;
+
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%04x from port 0x%04x\n",
+			(u16) perf->states[state].status, port));
+
+		for (i = 0; i < 100; i++) {
+			value = inl(port);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	}
+	param->retval = value;
+	return_VOID;
+}
+
+#ifdef CONFIG_SMP
+static void
+target_cpu_set_performance_ipi(void *data)
+{
+	struct set_performance_param	*param;
+
+	param = (struct set_performance_param *) data;
+	if (param->cpu != smp_processor_id())
+		return;
+
+	target_cpu_set_performance(param);
+}
+#endif
 
 static int
 acpi_processor_set_performance (
 	struct acpi_processor_performance	*perf,
 	int			state)
 {
-	u16			port = 0;
-	u16			value = 0;
+	unsigned int		value = 0;
 	int			i = 0;
 	struct cpufreq_freqs    cpufreq_freqs;
+	struct set_performance_param param;
 
 	ACPI_FUNCTION_TRACE("acpi_processor_set_performance");
 
@@ -267,8 +354,12 @@ acpi_processor_set_performance (
 
 	/* cpufreq frequency struct */
 	cpufreq_freqs.cpu = perf->pr->id;
-	cpufreq_freqs.old = perf->states[perf->state].core_frequency;
-	cpufreq_freqs.new = perf->states[state].core_frequency;
+	/*
+	 * cpufreq_notifier needs to send the freq in KHz. But acpi
+	 * data we have is in MHz
+	 */
+	cpufreq_freqs.old = perf->states[perf->state].core_frequency * 1000;
+	cpufreq_freqs.new = perf->states[state].core_frequency * 1000;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
@@ -276,35 +367,27 @@ acpi_processor_set_performance (
 	/*
 	 * First we write the target state's 'control' value to the
 	 * control_register.
-	 */
-
-	port = perf->control_register;
-	value = (u16) perf->states[state].control;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Writing 0x%04x to port 0x%04x\n", value, port));
-
-	outw(value, port); 
-
-	/*
 	 * Then we read the 'status_register' and compare the value with the
 	 * target state's 'status' to make sure the transition was successful.
 	 * Note that we'll poll for up to 1ms (100 cycles of 10us) before
 	 * giving up.
 	 */
 
-	port = perf->status_register;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Looking for 0x%04x from port 0x%04x\n",
-		(u16) perf->states[state].status, port));
-
-	for (i=0; i<100; i++) {
-		value = inw(port);
-		if (value == (u16) perf->states[state].status)
-			break;
-		udelay(10);
-	}
+	param.cpu = perf - performance;
+	param.perf = perf;
+	param.state = state;
+#ifdef CONFIG_SMP
+	param.async = 1; /* We don't want to wait in IPI context */
+	if (param.cpu != smp_processor_id())
+		smp_call_function(target_cpu_set_performance_ipi,
+				(void *)&param, 1, 1);
+	else
+		target_cpu_set_performance(&param);
+#else
+	param.async = 0;
+	target_cpu_set_performance(&param);
+#endif
+	value = param.retval;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig	2003-10-14 16:28:32.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig	2003-10-20 02:27:39.000000000 -0700
@@ -36,7 +36,9 @@ config X86_ACPI_CPUFREQ
 	depends on CPU_FREQ_TABLE && ACPI_PROCESSOR
 	help
 	  This driver adds a CPUFreq driver which utilizes the ACPI
-	  Processor Performance States.
+	  Processor Performance States. This can work with variety
+	  of platforms and support both Intel Speedstep and Intel Enhanced 
+	  Speedstep, as long as BIOS-ACPI provides the P-state information.
 
 	  For details, take a look at linux/Documentation/cpu-freq. 
 
diff -purN linux-2.6.0-test7/include/acpi/processor.h linux-2.6.0-test7-dbs/include/acpi/processor.h
--- linux-2.6.0-test7/include/acpi/processor.h	2003-10-20 00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/include/acpi/processor.h	2003-10-20 02:11:08.000000000 -0700
@@ -73,6 +73,7 @@ struct acpi_processor_performance {
 	u16			control_register;
 	u16			status_register;
 	int			state_count;
+	int			space_id;
 	struct acpi_processor_px states[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct cpufreq_frequency_table freq_table[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct acpi_processor   *pr;

[PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Pallipadi, Venkatesh]

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


Patch 1/3: Changes to ACPI P-state driver, to handle MSR 
based transitions frequency transitions and make the driver 
SMP aware.


diffstat dbs1.patch 
arch/i386/kernel/cpu/cpufreq/Kconfig |    4 
arch/i386/kernel/cpu/cpufreq/acpi.c  |  141
+++++++++++++++++++++++++++--------
include/acpi/processor.h             |    1 
3 files changed, 116 insertions(+), 30 deletions(-)



diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c
linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c
2003-10-20 00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c
2003-10-20 02:11:01.000000000 -0700
@@ -99,7 +99,9 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	perf->space_id = (int) reg->space_id;
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE)
{
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d]
(control_register)\n",
 			(u32) reg->space_id));
@@ -126,7 +128,8 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE)
{
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d]
(status_register)\n",
 			(u32) reg->space_id));
@@ -224,16 +227,100 @@ end:
 	return_VALUE(result);
 }
 
+struct set_performance_param {
+	int 					cpu;
+	struct acpi_processor_performance	*perf;
+	unsigned int 				state;
+	unsigned int 				async;
+	unsigned int 				retval;
+};
+
+static void
+target_cpu_set_performance(struct set_performance_param	*param)
+{
+	struct acpi_processor_performance	*perf = param->perf;
+	u16		port = 0;
+	int		state = param->state;
+	unsigned int	value = perf->states[state].control;
+	int		i = 0;
+
+	ACPI_FUNCTION_TRACE("target_cpu_set_performance");
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Writing 0x%x to MSR
0x%x\n", 
+					value, perf->control_register));
+		wrmsr(perf->control_register, value, 0);
+	} else {
+		port = perf->control_register;
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Writing 0x%04x to port 0x%04x\n", value,
port));
+
+		outl(value, port); 
+	}
+
+	if (param->async) {
+		/* We don't want to wait for the status change here, as 
+		 * that will affect performance, especially with
frequent 
+		 * frequency switches. We assume the tranisition 
+		 * gone through and continue.
+		 */
+		param->retval = perf->states[state].status;
+		return_VOID;
+	}
+
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%02x from MSR 0x%04x\n",
+			(u16) perf->states[state].status, 
+			perf->status_register));
+
+		for (i = 0; i < 100; i++) {
+			unsigned long unused_hi;
+			rdmsr(perf->status_register, value, unused_hi);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	} else {
+		port = perf->status_register;
+
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%04x from port 0x%04x\n",
+			(u16) perf->states[state].status, port));
+
+		for (i = 0; i < 100; i++) {
+			value = inl(port);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	}
+	param->retval = value;
+	return_VOID;
+}
+
+#ifdef CONFIG_SMP
+static void
+target_cpu_set_performance_ipi(void *data)
+{
+	struct set_performance_param	*param;
+
+	param = (struct set_performance_param *) data;
+	if (param->cpu != smp_processor_id())
+		return;
+
+	target_cpu_set_performance(param);
+}
+#endif
 
 static int
 acpi_processor_set_performance (
 	struct acpi_processor_performance	*perf,
 	int			state)
 {
-	u16			port = 0;
-	u16			value = 0;
+	unsigned int		value = 0;
 	int			i = 0;
 	struct cpufreq_freqs    cpufreq_freqs;
+	struct set_performance_param param;
 
 	ACPI_FUNCTION_TRACE("acpi_processor_set_performance");
 
@@ -267,8 +354,12 @@ acpi_processor_set_performance (
 
 	/* cpufreq frequency struct */
 	cpufreq_freqs.cpu = perf->pr->id;
-	cpufreq_freqs.old = perf->states[perf->state].core_frequency;
-	cpufreq_freqs.new = perf->states[state].core_frequency;
+	/*
+	 * cpufreq_notifier needs to send the freq in KHz. But acpi
+	 * data we have is in MHz
+	 */
+	cpufreq_freqs.old = perf->states[perf->state].core_frequency *
1000;
+	cpufreq_freqs.new = perf->states[state].core_frequency * 1000;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
@@ -276,35 +367,27 @@ acpi_processor_set_performance (
 	/*
 	 * First we write the target state's 'control' value to the
 	 * control_register.
-	 */
-
-	port = perf->control_register;
-	value = (u16) perf->states[state].control;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Writing 0x%04x to port 0x%04x\n", value, port));
-
-	outw(value, port); 
-
-	/*
 	 * Then we read the 'status_register' and compare the value with
the
 	 * target state's 'status' to make sure the transition was
successful.
 	 * Note that we'll poll for up to 1ms (100 cycles of 10us)
before
 	 * giving up.
 	 */
 
-	port = perf->status_register;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Looking for 0x%04x from port 0x%04x\n",
-		(u16) perf->states[state].status, port));
-
-	for (i=0; i<100; i++) {
-		value = inw(port);
-		if (value == (u16) perf->states[state].status)
-			break;
-		udelay(10);
-	}
+	param.cpu = perf - performance;
+	param.perf = perf;
+	param.state = state;
+#ifdef CONFIG_SMP
+	param.async = 1; /* We don't want to wait in IPI context */
+	if (param.cpu != smp_processor_id())
+		smp_call_function(target_cpu_set_performance_ipi,
+				(void *)&param, 1, 1);
+	else
+		target_cpu_set_performance(&param);
+#else
+	param.async = 0;
+	target_cpu_set_performance(&param);
+#endif
+	value = param.retval;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig
linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig
2003-10-14 16:28:32.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig
2003-10-20 02:27:39.000000000 -0700
@@ -36,7 +36,9 @@ config X86_ACPI_CPUFREQ
 	depends on CPU_FREQ_TABLE && ACPI_PROCESSOR
 	help
 	  This driver adds a CPUFreq driver which utilizes the ACPI
-	  Processor Performance States.
+	  Processor Performance States. This can work with variety
+	  of platforms and support both Intel Speedstep and Intel
Enhanced 
+	  Speedstep, as long as BIOS-ACPI provides the P-state
information.
 
 	  For details, take a look at linux/Documentation/cpu-freq. 
 
diff -purN linux-2.6.0-test7/include/acpi/processor.h
linux-2.6.0-test7-dbs/include/acpi/processor.h
--- linux-2.6.0-test7/include/acpi/processor.h	2003-10-20
00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/include/acpi/processor.h	2003-10-20
02:11:08.000000000 -0700
@@ -73,6 +73,7 @@ struct acpi_processor_performance {
 	u16			control_register;
 	u16			status_register;
 	int			state_count;
+	int			space_id;
 	struct acpi_processor_px states[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct cpufreq_frequency_table
freq_table[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct acpi_processor   *pr;

[-- Attachment #2: dbs1.patch --]
[-- Type: application/octet-stream, Size: 7053 bytes --]

diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/acpi.c	2003-10-20 00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/acpi.c	2003-10-20 02:11:01.000000000 -0700
@@ -99,7 +99,9 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	perf->space_id = (int) reg->space_id;
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d] (control_register)\n",
 			(u32) reg->space_id));
@@ -126,7 +128,8 @@ acpi_processor_get_performance_control (
 
 	reg = (struct acpi_pct_register *) (obj.buffer.pointer);
 
-	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
+	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO && 
+			reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
 		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
 			"Unsupported address space [%d] (status_register)\n",
 			(u32) reg->space_id));
@@ -224,16 +227,100 @@ end:
 	return_VALUE(result);
 }
 
+struct set_performance_param {
+	int 					cpu;
+	struct acpi_processor_performance	*perf;
+	unsigned int 				state;
+	unsigned int 				async;
+	unsigned int 				retval;
+};
+
+static void
+target_cpu_set_performance(struct set_performance_param	*param)
+{
+	struct acpi_processor_performance	*perf = param->perf;
+	u16		port = 0;
+	int		state = param->state;
+	unsigned int	value = perf->states[state].control;
+	int		i = 0;
+
+	ACPI_FUNCTION_TRACE("target_cpu_set_performance");
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Writing 0x%x to MSR 0x%x\n", 
+					value, perf->control_register));
+		wrmsr(perf->control_register, value, 0);
+	} else {
+		port = perf->control_register;
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Writing 0x%04x to port 0x%04x\n", value, port));
+
+		outl(value, port); 
+	}
+
+	if (param->async) {
+		/* We don't want to wait for the status change here, as 
+		 * that will affect performance, especially with frequent 
+		 * frequency switches. We assume the tranisition 
+		 * gone through and continue.
+		 */
+		param->retval = perf->states[state].status;
+		return_VOID;
+	}
+
+	if (perf->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%02x from MSR 0x%04x\n",
+			(u16) perf->states[state].status, 
+			perf->status_register));
+
+		for (i = 0; i < 100; i++) {
+			unsigned long unused_hi;
+			rdmsr(perf->status_register, value, unused_hi);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	} else {
+		port = perf->status_register;
+
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
+			"Looking for 0x%04x from port 0x%04x\n",
+			(u16) perf->states[state].status, port));
+
+		for (i = 0; i < 100; i++) {
+			value = inl(port);
+			if (value == perf->states[state].status)
+				break;
+			udelay(10);
+		}
+	}
+	param->retval = value;
+	return_VOID;
+}
+
+#ifdef CONFIG_SMP
+static void
+target_cpu_set_performance_ipi(void *data)
+{
+	struct set_performance_param	*param;
+
+	param = (struct set_performance_param *) data;
+	if (param->cpu != smp_processor_id())
+		return;
+
+	target_cpu_set_performance(param);
+}
+#endif
 
 static int
 acpi_processor_set_performance (
 	struct acpi_processor_performance	*perf,
 	int			state)
 {
-	u16			port = 0;
-	u16			value = 0;
+	unsigned int		value = 0;
 	int			i = 0;
 	struct cpufreq_freqs    cpufreq_freqs;
+	struct set_performance_param param;
 
 	ACPI_FUNCTION_TRACE("acpi_processor_set_performance");
 
@@ -267,8 +354,12 @@ acpi_processor_set_performance (
 
 	/* cpufreq frequency struct */
 	cpufreq_freqs.cpu = perf->pr->id;
-	cpufreq_freqs.old = perf->states[perf->state].core_frequency;
-	cpufreq_freqs.new = perf->states[state].core_frequency;
+	/*
+	 * cpufreq_notifier needs to send the freq in KHz. But acpi
+	 * data we have is in MHz
+	 */
+	cpufreq_freqs.old = perf->states[perf->state].core_frequency * 1000;
+	cpufreq_freqs.new = perf->states[state].core_frequency * 1000;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_PRECHANGE);
@@ -276,35 +367,27 @@ acpi_processor_set_performance (
 	/*
 	 * First we write the target state's 'control' value to the
 	 * control_register.
-	 */
-
-	port = perf->control_register;
-	value = (u16) perf->states[state].control;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Writing 0x%04x to port 0x%04x\n", value, port));
-
-	outw(value, port); 
-
-	/*
 	 * Then we read the 'status_register' and compare the value with the
 	 * target state's 'status' to make sure the transition was successful.
 	 * Note that we'll poll for up to 1ms (100 cycles of 10us) before
 	 * giving up.
 	 */
 
-	port = perf->status_register;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, 
-		"Looking for 0x%04x from port 0x%04x\n",
-		(u16) perf->states[state].status, port));
-
-	for (i=0; i<100; i++) {
-		value = inw(port);
-		if (value == (u16) perf->states[state].status)
-			break;
-		udelay(10);
-	}
+	param.cpu = perf - performance;
+	param.perf = perf;
+	param.state = state;
+#ifdef CONFIG_SMP
+	param.async = 1; /* We don't want to wait in IPI context */
+	if (param.cpu != smp_processor_id())
+		smp_call_function(target_cpu_set_performance_ipi,
+				(void *)&param, 1, 1);
+	else
+		target_cpu_set_performance(&param);
+#else
+	param.async = 0;
+	target_cpu_set_performance(&param);
+#endif
+	value = param.retval;
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
diff -purN linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig
--- linux-2.6.0-test7/arch/i386/kernel/cpu/cpufreq/Kconfig	2003-10-14 16:28:32.000000000 -0700
+++ linux-2.6.0-test7-dbs/arch/i386/kernel/cpu/cpufreq/Kconfig	2003-10-20 02:27:39.000000000 -0700
@@ -36,7 +36,9 @@ config X86_ACPI_CPUFREQ
 	depends on CPU_FREQ_TABLE && ACPI_PROCESSOR
 	help
 	  This driver adds a CPUFreq driver which utilizes the ACPI
-	  Processor Performance States.
+	  Processor Performance States. This can work with variety
+	  of platforms and support both Intel Speedstep and Intel Enhanced 
+	  Speedstep, as long as BIOS-ACPI provides the P-state information.
 
 	  For details, take a look at linux/Documentation/cpu-freq. 
 
diff -purN linux-2.6.0-test7/include/acpi/processor.h linux-2.6.0-test7-dbs/include/acpi/processor.h
--- linux-2.6.0-test7/include/acpi/processor.h	2003-10-20 00:24:43.000000000 -0700
+++ linux-2.6.0-test7-dbs/include/acpi/processor.h	2003-10-20 02:11:08.000000000 -0700
@@ -73,6 +73,7 @@ struct acpi_processor_performance {
 	u16			control_register;
 	u16			status_register;
 	int			state_count;
+	int			space_id;
 	struct acpi_processor_px states[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct cpufreq_frequency_table freq_table[ACPI_PROCESSOR_MAX_PERFORMANCE];
 	struct acpi_processor   *pr;

[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Cpufreq mailing list
Cpufreq@www.linux.org.uk
http://www.linux.org.uk/mailman/listinfo/cpufreq

Re: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Ducrot Bruno]

There is already a patch from Dominik Brodowski for the apci p-state which IMHO
is better at least by design.  Your do not handle correctly other processors
than Intel.  But the HT stuff may be interresting, though.

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Ducrot Bruno]

There is already a patch from Dominik Brodowski for the apci p-state which IMHO
is better at least by design.  Your do not handle correctly other processors
than Intel.  But the HT stuff may be interresting, though.

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

RE: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Grover, Andrew]

> From: Ducrot Bruno [mailto:ducrot@poupinou.org] 

> Looking around some other asl, it seems also that if you verify that
> _PCT return FFixedHW, but length and address are both different to 0,
> it may be possible that a generic driver via MSR could work, but there
> is nothing in specs afaik (and specs can be changed if compatible..).

My understanding was that if the operation region is FFixedHW, the
parameters are meaningless (and should be 0). FFixedHW means the OS
knows without ACPI telling it how to switch states. That is, while R40
lists the MSRs for the addresses, that is useless and confusing, because
the OS should a priori know via a driver how to switch, and therefore
isn't looking at those parameters.

Regards -- Andy

RE: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Grover, Andrew]

> From: Ducrot Bruno [mailto:ducrot@poupinou.org] 

> Looking around some other asl, it seems also that if you verify that
> _PCT return FFixedHW, but length and address are both different to 0,
> it may be possible that a generic driver via MSR could work, but there
> is nothing in specs afaik (and specs can be changed if compatible..).

My understanding was that if the operation region is FFixedHW, the
parameters are meaningless (and should be 0). FFixedHW means the OS
knows without ACPI telling it how to switch states. That is, while R40
lists the MSRs for the addresses, that is useless and confusing, because
the OS should a priori know via a driver how to switch, and therefore
isn't looking at those parameters.

Regards -- Andy

Re: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Ducrot Bruno]

On Tue, Oct 21, 2003 at 11:57:58AM -0700, Pallipadi, Venkatesh wrote:
> 
> 
> > -----Original Message-----
> > From: Ducrot Bruno [mailto:ducrot@poupinou.org] 
> > Sent: Tuesday, October 21, 2003 10:56 AM
> > 
> > > It will work any processor 
> > > that is ACPI compatible and again there are no specific 
> > > checks for Intel here.
> > > 
> > 
> > On a K7 with powernow for example, perf_ctrl and perf_data 
> > will be MSR 0
> > with your patch, that do not make sence.
> > Even if you know the correct MSRs, the values for 'control' 
> > and 'status'
> > in _PSS packages will be only bit-fields, and they can *not* be
> > written nor read directly to the (correct) MSRs (again for K7 
> > powernow).
> > 
> > This is because the FfixedHW is only an indication that a CPU specific
> > 'feature' (even though already somehow defined in ACPI like P-state,
> > C-state, etc.) have to be handled by the OS in a non-acpi driver, as
> > per ACPI spec, and that will be dependant of the CPU.
> > 
> 
> Agree with most of your comments. 
> Current ACPI P-state driver ignored everything other than SYSTEM_IO.
> And we are trying to add support for FIXED_HW. I was unaware of
> any other CPU using ACPI PCT FIXED_HW to mean anything other than
> MSR. As you mentioned, if K7 indeed exports some ACPI-PCT information 
> to mean something else, then that is a real bug in my patch. I will 
> add CPU specific abstraction to handle FIXED_HW get/set functions in 
> the next revision of the patch. 

That is why Dominik seperated the acpi cpufreq driver in 2 files in his
cleanup patch.  Others cpufreq drivers may use acpi to retrieve infos
if needed.  Even though by now K7 do not support that, it can be changed
for (the doc. seems to be under NDA, but AMD seems willing to cooperate).

Looking around some other asl, it seems also that if you verify that
_PCT return FFixedHW, but length and address are both different to 0,
it may be possible that a generic driver via MSR could work, but there
is nothing in specs afaik (and specs can be changed if compatible..).

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Ducrot Bruno]

On Tue, Oct 21, 2003 at 11:57:58AM -0700, Pallipadi, Venkatesh wrote:
> 
> 
> > -----Original Message-----
> > From: Ducrot Bruno [mailto:ducrot@poupinou.org] 
> > Sent: Tuesday, October 21, 2003 10:56 AM
> > 
> > > It will work any processor 
> > > that is ACPI compatible and again there are no specific 
> > > checks for Intel here.
> > > 
> > 
> > On a K7 with powernow for example, perf_ctrl and perf_data 
> > will be MSR 0
> > with your patch, that do not make sence.
> > Even if you know the correct MSRs, the values for 'control' 
> > and 'status'
> > in _PSS packages will be only bit-fields, and they can *not* be
> > written nor read directly to the (correct) MSRs (again for K7 
> > powernow).
> > 
> > This is because the FfixedHW is only an indication that a CPU specific
> > 'feature' (even though already somehow defined in ACPI like P-state,
> > C-state, etc.) have to be handled by the OS in a non-acpi driver, as
> > per ACPI spec, and that will be dependant of the CPU.
> > 
> 
> Agree with most of your comments. 
> Current ACPI P-state driver ignored everything other than SYSTEM_IO.
> And we are trying to add support for FIXED_HW. I was unaware of
> any other CPU using ACPI PCT FIXED_HW to mean anything other than
> MSR. As you mentioned, if K7 indeed exports some ACPI-PCT information 
> to mean something else, then that is a real bug in my patch. I will 
> add CPU specific abstraction to handle FIXED_HW get/set functions in 
> the next revision of the patch. 

That is why Dominik seperated the acpi cpufreq driver in 2 files in his
cleanup patch.  Others cpufreq drivers may use acpi to retrieve infos
if needed.  Even though by now K7 do not support that, it can be changed
for (the doc. seems to be under NDA, but AMD seems willing to cooperate).

Looking around some other asl, it seems also that if you verify that
_PCT return FFixedHW, but length and address are both different to 0,
it may be possible that a generic driver via MSR could work, but there
is nothing in specs afaik (and specs can be changed if compatible..).

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Dominik Brodowski]

On Tue, Oct 21, 2003 at 10:15:31AM -0700, Pallipadi, Venkatesh wrote:
> 
> > -----Original Message-----
> > From: Ducrot Bruno [mailto:ducrot@poupinou.org] 
> > 
> > There is already a patch from Dominik Brodowski for the apci 
> > p-state which IMHO
> > is better at least by design.  
> 
> Are you referring to cleanup of ACPI P-state driver by Dominik?
> That patch is indeed nice and clean. But that is mostly 
> orthogonal to this patch. I mean, 
> - SMP awareness in P-state driver 
> - P-state coordination between HT siblings
> will still be required even after Dominik's patch. Though exact 
> location of these changes will change when applied over Dominik's 
> patch.
> 
> There is a small overlap in handling MSR based P-state transitions, 
> but that is a real minor change in my patch and I am reusing most 
> of the existing IO based transitions code for MSR based ones.

Indeed there is. However, as 2.6. is in a "stability freeze" right now, my
pretty invasive cleanup won't have a chance of going in soon. I'd like to
separate the issues with the p-state driver from the demandbased cpufreq
governor, though...

	Dominik

RE: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Pallipadi, Venkatesh]

> -----Original Message-----
> From: Ducrot Bruno [mailto:ducrot@poupinou.org] 
> Sent: Tuesday, October 21, 2003 10:56 AM
> 
> > It will work any processor 
> > that is ACPI compatible and again there are no specific 
> > checks for Intel here.
> > 
> 
> On a K7 with powernow for example, perf_ctrl and perf_data 
> will be MSR 0
> with your patch, that do not make sence.
> Even if you know the correct MSRs, the values for 'control' 
> and 'status'
> in _PSS packages will be only bit-fields, and they can *not* be
> written nor read directly to the (correct) MSRs (again for K7 
> powernow).
> 
> This is because the FfixedHW is only an indication that a CPU specific
> 'feature' (even though already somehow defined in ACPI like P-state,
> C-state, etc.) have to be handled by the OS in a non-acpi driver, as
> per ACPI spec, and that will be dependant of the CPU.
> 

Agree with most of your comments. 
Current ACPI P-state driver ignored everything other than SYSTEM_IO.
And we are trying to add support for FIXED_HW. I was unaware of
any other CPU using ACPI PCT FIXED_HW to mean anything other than
MSR. As you mentioned, if K7 indeed exports some ACPI-PCT information 
to mean something else, then that is a real bug in my patch. I will 
add CPU specific abstraction to handle FIXED_HW get/set functions in 
the next revision of the patch. 

Thanks,
-Venkatesh

RE: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Pallipadi, Venkatesh]

> -----Original Message-----
> From: Ducrot Bruno [mailto:ducrot@poupinou.org] 
> Sent: Tuesday, October 21, 2003 10:56 AM
> 
> > It will work any processor 
> > that is ACPI compatible and again there are no specific 
> > checks for Intel here.
> > 
> 
> On a K7 with powernow for example, perf_ctrl and perf_data 
> will be MSR 0
> with your patch, that do not make sence.
> Even if you know the correct MSRs, the values for 'control' 
> and 'status'
> in _PSS packages will be only bit-fields, and they can *not* be
> written nor read directly to the (correct) MSRs (again for K7 
> powernow).
> 
> This is because the FfixedHW is only an indication that a CPU specific
> 'feature' (even though already somehow defined in ACPI like P-state,
> C-state, etc.) have to be handled by the OS in a non-acpi driver, as
> per ACPI spec, and that will be dependant of the CPU.
> 

Agree with most of your comments. 
Current ACPI P-state driver ignored everything other than SYSTEM_IO.
And we are trying to add support for FIXED_HW. I was unaware of
any other CPU using ACPI PCT FIXED_HW to mean anything other than
MSR. As you mentioned, if K7 indeed exports some ACPI-PCT information 
to mean something else, then that is a real bug in my patch. I will 
add CPU specific abstraction to handle FIXED_HW get/set functions in 
the next revision of the patch. 

Thanks,
-Venkatesh

Re: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Ducrot Bruno]

On Tue, Oct 21, 2003 at 10:15:31AM -0700, Pallipadi, Venkatesh wrote:
> 
> > -----Original Message-----
> > From: Ducrot Bruno [mailto:ducrot@poupinou.org] 


...

> > Your do not handle correctly 
> > other processors
> > than Intel.  
> 
> I am sorry. I do not understand this comment. 
> - Major part of Patch 1 is adding SMP awareness, which has
> nothing specific to Intel at all.
> - A part of patch 1 adds MSR based transition capability. 
> This is based on ACPI spec.

Could you tell me where you find in ACPI spec. that FfixedHW means 
always MSR?  That not true for C-states definitions via _CST for
example (the first entry being always an FFixedHW, because it
is C1 and will be the single asm instruction: 'hlt').
Look 2.0b page 228.

> It will work any processor 
> that is ACPI compatible and again there are no specific 
> checks for Intel here.
> 

On a K7 with powernow for example, perf_ctrl and perf_data will be MSR 0
with your patch, that do not make sence.
Even if you know the correct MSRs, the values for 'control' and 'status'
in _PSS packages will be only bit-fields, and they can *not* be
written nor read directly to the (correct) MSRs (again for K7 powernow).

This is because the FfixedHW is only an indication that a CPU specific
'feature' (even though already somehow defined in ACPI like P-state,
C-state, etc.) have to be handled by the OS in a non-acpi driver, as
per ACPI spec, and that will be dependant of the CPU.


-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Ducrot Bruno]

On Tue, Oct 21, 2003 at 10:15:31AM -0700, Pallipadi, Venkatesh wrote:
> 
> > -----Original Message-----
> > From: Ducrot Bruno [mailto:ducrot@poupinou.org] 


...

> > Your do not handle correctly 
> > other processors
> > than Intel.  
> 
> I am sorry. I do not understand this comment. 
> - Major part of Patch 1 is adding SMP awareness, which has
> nothing specific to Intel at all.
> - A part of patch 1 adds MSR based transition capability. 
> This is based on ACPI spec.

Could you tell me where you find in ACPI spec. that FfixedHW means 
always MSR?  That not true for C-states definitions via _CST for
example (the first entry being always an FFixedHW, because it
is C1 and will be the single asm instruction: 'hlt').
Look 2.0b page 228.

> It will work any processor 
> that is ACPI compatible and again there are no specific 
> checks for Intel here.
> 

On a K7 with powernow for example, perf_ctrl and perf_data will be MSR 0
with your patch, that do not make sence.
Even if you know the correct MSRs, the values for 'control' and 'status'
in _PSS packages will be only bit-fields, and they can *not* be
written nor read directly to the (correct) MSRs (again for K7 powernow).

This is because the FfixedHW is only an indication that a CPU specific
'feature' (even though already somehow defined in ACPI like P-state,
C-state, etc.) have to be handled by the OS in a non-acpi driver, as
per ACPI spec, and that will be dependant of the CPU.


-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

RE: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Pallipadi, Venkatesh]

> -----Original Message-----
> From: Ducrot Bruno [mailto:ducrot@poupinou.org] 
> 
> There is already a patch from Dominik Brodowski for the apci 
> p-state which IMHO
> is better at least by design.  

Are you referring to cleanup of ACPI P-state driver by Dominik?
That patch is indeed nice and clean. But that is mostly 
orthogonal to this patch. I mean, 
- SMP awareness in P-state driver 
- P-state coordination between HT siblings
will still be required even after Dominik's patch. Though exact 
location of these changes will change when applied over Dominik's 
patch.

There is a small overlap in handling MSR based P-state transitions, 
but that is a real minor change in my patch and I am reusing most 
of the existing IO based transitions code for MSR based ones.


> Your do not handle correctly 
> other processors
> than Intel.  

I am sorry. I do not understand this comment. 
- Major part of Patch 1 is adding SMP awareness, which has
nothing specific to Intel at all.
- A part of patch 1 adds MSR based transition capability. 
This is based on ACPI spec. It will work any processor 
that is ACPI compatible and again there are no specific 
checks for Intel here.


> But the HT stuff may be interresting, though.

Thanks. 
-Venkatesh

RE: [PATCH] 1/3 Dynamic cpufreq governor and updates to ACPI P-state driver

[Pallipadi, Venkatesh]

> -----Original Message-----
> From: Ducrot Bruno [mailto:ducrot@poupinou.org] 
> 
> There is already a patch from Dominik Brodowski for the apci 
> p-state which IMHO
> is better at least by design.  

Are you referring to cleanup of ACPI P-state driver by Dominik?
That patch is indeed nice and clean. But that is mostly 
orthogonal to this patch. I mean, 
- SMP awareness in P-state driver 
- P-state coordination between HT siblings
will still be required even after Dominik's patch. Though exact 
location of these changes will change when applied over Dominik's 
patch.

There is a small overlap in handling MSR based P-state transitions, 
but that is a real minor change in my patch and I am reusing most 
of the existing IO based transitions code for MSR based ones.


> Your do not handle correctly 
> other processors
> than Intel.  

I am sorry. I do not understand this comment. 
- Major part of Patch 1 is adding SMP awareness, which has
nothing specific to Intel at all.
- A part of patch 1 adds MSR based transition capability. 
This is based on ACPI spec. It will work any processor 
that is ACPI compatible and again there are no specific 
checks for Intel here.


> But the HT stuff may be interresting, though.

Thanks. 
-Venkatesh