Skip to contentSkip to conversation

CPUFreq PowerOP integration, Centrino PM Core and OPs registration 2/3

RFC In review Eugeny S. Mints 3 participants 5 messages Sep 14, 2006 – Oct 9, 2006 ACPI

lore
Share & tools
lore

Eugeny S. Mints

author
#0 lore raw

RFC 1/1 CPUFreq PowerOP integration, Centrino PM Core and OPs registration 2/3

This patch adds support for centrino PM Core and operating points registration module.

Operating points registration module uses hardcoded values or values retrieved from ACPI to register operating points using PowerOP interface. The module is completely independent from either cpufreq or centrino PM Core.

PM Core defines power parameters for the platform and handles all hw related steps to set system to an operating point or retrieve a power parameter value.

Both speedstep-centrino-pm_core.c and speedstep-centrino.c do not belong to cpufreq anymore and should be removed from cpufreq folder but were left there to simplify the diff investigation.

arch/i386/Kconfig +12 -0
index f71fb4a..62d7523 100644
@@ -814,6 +814,18 @@ source kernel/power/Kconfig
 
 source "drivers/acpi/Kconfig"
 
+menu "PM Core"
+
+config PM_CORE
+	tristate "PM Core"
+	depends POWEROP
+	help
+	This enables i386 PM Core to control platform freq/voltage via
+	PowerOP interface
+
+endmenu
+
+
 menu "APM (Advanced Power Management) BIOS Support"
 depends on PM && !X86_VISWS
arch/i386/kernel/cpu/cpufreq/Kconfig +3 -9
index ccc1edf..a1ce5a3 100644
@@ -106,17 +106,11 @@ config X86_GX_SUSPMOD
 	 If in doubt, say N.
 
 config X86_SPEEDSTEP_CENTRINO
-	tristate "Intel Enhanced SpeedStep"
-	select CPU_FREQ_TABLE
+	tristate "SpeedStep operatin points registration"
+	depends on POWEROP
 	select X86_SPEEDSTEP_CENTRINO_TABLE if (!X86_SPEEDSTEP_CENTRINO_ACPI)
 	help
-	  This adds the CPUFreq driver for Enhanced SpeedStep enabled
-	  mobile CPUs.  This means Intel Pentium M (Centrino) CPUs. However,
-	  you also need to say Y to "Use ACPI tables to decode..." below
-	  [which might imply enabling ACPI] if you want to use this driver
-	  on non-Banias CPUs.
-
-	  For details, take a look at <file:Documentation/cpu-freq/>.
+	  Opeatin points registration module for SpeedStep Centrino
 
 	  If in doubt, say N.
arch/i386/kernel/cpu/cpufreq/Makefile +1 -0
index 2e894f1..62753f6 100644
@@ -8,6 +8,7 @@ obj-$(CONFIG_X86_LONGRUN)		+= longrun.o 
 obj-$(CONFIG_X86_GX_SUSPMOD)		+= gx-suspmod.o
 obj-$(CONFIG_X86_SPEEDSTEP_ICH)		+= speedstep-ich.o
 obj-$(CONFIG_X86_SPEEDSTEP_CENTRINO)	+= speedstep-centrino.o
+obj-$(CONFIG_PM_CORE)			+= speedstep-centrino-pm_core.o
 obj-$(CONFIG_X86_SPEEDSTEP_LIB)		+= speedstep-lib.o
 obj-$(CONFIG_X86_SPEEDSTEP_SMI)		+= speedstep-smi.o
 obj-$(CONFIG_X86_ACPI_CPUFREQ)		+= acpi-cpufreq.o
arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c new file +644 -0

644 lines not on this page · raw message

arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c +176 -505

896 lines not on this page · raw message

drivers/acpi/processor_perflib.c +3 -7
index 7ba5e49..277b4d2 100644
@@ -682,12 +682,8 @@ int acpi_processor_preregister_performan
 		/* Validate the Domain info */
 		count_target = pdomain->num_processors;
 		count = 1;
-		if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
-			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL;
-		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
-			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_HW;
-		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
-			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+
+		pr->performance->shared_type = pdomain->coord_type;
 
 		for_each_possible_cpu(j) {
 			if (i == j)

@@ -751,7 +747,7 @@ err_ret:
 		if (retval) {
 			cpus_clear(pr->performance->shared_cpu_map);
 			cpu_set(i, pr->performance->shared_cpu_map);
-			pr->performance->shared_type = CPUFREQ_SHARED_TYPE_ALL;
+			pr->performance->shared_type = DOMAIN_COORD_TYPE_SW_ALL;
 		}
 		pr->performance = NULL; /* Will be set for real in register */
 	}
include/asm-i386/pm_core.h new file +72 -0

72 lines not on this page · raw message

#1 lore raw

Hi!

arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c line 131

Quoted 6 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c new file +6 -0
+/* 
+ * FIXME: very temporary implementation, just to prove the concept !! 
+ */
+static int 
+process_pwr_param(struct pm_core_point *opt, int op, char *param_name,
+		  int va_arg)

Ok, so can we get final implementation? Parsing strings in drivers is evil...

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#2 lore raw
Quoted 11 lines · Pavel Machek

Hi!

arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c line 131

Quoted 6 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c new file +6 -0
+/* 
+ * FIXME: very temporary implementation, just to prove the concept !! 
+ */
+static int 
+process_pwr_param(struct pm_core_point *opt, int op, char *param_name,
+		  int va_arg)

Ok, so can we get final implementation? Parsing strings in drivers is evil...

ok, i saw this your comment when you mentioned it for the first time and I explicitly put it in TODO letter sent along with the patchset.

Can you instead of such implementation details try to review and give us your comments if any on interfaces approach and namely on cpufreq/PowerOP integration at cpufreq_driver layer; comments _based_ on the code. The issue with string parsing does not affect cpufreq/PowerOP integration interfaces since currently I keep going with parameter names(strings) interface for PowerOP Core regardless of whether parsing will eventually occur - at PM Core or POwerOP Core.

Eugeny

Quoted 3 lines

Pavel

#3 lore raw
In reply to Eugeny S. Mints

Hi,

Quoted 31 lines · Eugeny S. Mints
arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c new file +31 -0
+static int 
+process_pwr_param(struct pm_core_point *opt, int op, char *param_name,
+		  int va_arg)
+{
+	int cpu = 0;
+	char buf[8];
+
+	for (cpu = 0; cpu < NR_CPUS; cpu++)
+	{
+		sprintf(buf, "v%d", cpu);
+
+		if (strcmp(param_name, buf) == 0) {
+			if (op == PWR_PARAM_SET)
+				opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_V] = 
+									va_arg;
+			else if (opt != NULL)
+				*(int *)va_arg = 
+				     opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_V];
+			else if ((*(int *)va_arg = get_vtg(cpu)) <= 0)
+				return -EINVAL;
+			return 0;
+		}
+
+		sprintf(buf, "freq%d", cpu);
+
+		if (strcmp(param_name, buf) == 0) {
+			if (op == PWR_PARAM_SET)
+				opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_FREQ] = 
+									va_arg;
+			else if (opt != NULL)
+				*(int *)va_arg =

arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c line 169

Quoted 10 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c new file +10 -0
+				  opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_FREQ];
+			else if ((*(int *)va_arg = get_freq(cpu)) <= 0)
+				return -EINVAL;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}

Ouch. IIRC Pavel had some fine comments about such string parsing deep in arch code... Other than that I see lots of indirection, lots of code being added (~400 lines) for no gain in functionality for the x86 case.

Thanks,
Dominik

author
#4 lore raw
Quoted 41 lines · Dominik Brodowski

Hi,

Quoted 37 lines · Eugeny S. Mints
arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c new file +37 -0
+static int 
+process_pwr_param(struct pm_core_point *opt, int op, char *param_name,
+		  int va_arg)
+{
+	int cpu = 0;
+	char buf[8];
+
+	for (cpu = 0; cpu < NR_CPUS; cpu++)
+	{
+		sprintf(buf, "v%d", cpu);
+
+		if (strcmp(param_name, buf) == 0) {
+			if (op == PWR_PARAM_SET)
+				opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_V] = 
+									va_arg;
+			else if (opt != NULL)
+				*(int *)va_arg = 
+				     opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_V];
+			else if ((*(int *)va_arg = get_vtg(cpu)) <= 0)
+				return -EINVAL;
+			return 0;
+		}
+
+		sprintf(buf, "freq%d", cpu);
+
+		if (strcmp(param_name, buf) == 0) {
+			if (op == PWR_PARAM_SET)
+				opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_FREQ] = 
+									va_arg;
+			else if (opt != NULL)
+				*(int *)va_arg = 
+				  opt->opt[cpu].pwpr[_I386_PM_CORE_POINT_FREQ];
+			else if ((*(int *)va_arg = get_freq(cpu)) <= 0)
+				return -EINVAL;
+
+			return 0;
+		}
Quoted 8 lines

arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c line 121

Quoted 4 lines
arch/i386/kernel/cpu/cpufreq/speedstep-centrino-pm_core.c new file +4 -0
+	}
+
+	return -EINVAL;
+}

Ouch. IIRC Pavel had some fine comments about such string parsing deep in arch code... Other than that I see lots of indirection, lots of code being added (~400 lines) for no gain in functionality for the x86 case.

THis code is no longer anyhow relevant to the latest take of PowerOP. Please check to make sure that you are looking at the very latest PowerOP code submitted for discussion. Latest PowerOP take gets rid of all parsing. Latest patches do not contain x86 patch yet but omap PM core reference code is supplied. So please use omap code for the discussion. FYI, forward porting of x86 patches to the latest POwerOP is on the way.

Thanks,
Eugeny

Quoted 4 lines

Thanks,
Dominik