Skip to contentSkip to conversation

CPUFreq PowerOP integratoin, CPUFreq core 1/3

RFC In review Eugeny S. Mints 2 participants 2 messages Sep 14, 2006 – Oct 7, 2006 CPU frequency scaling framework

lore
Share & tools
lore
#0 lore raw

RFC 1/1 CPUFreq PowerOP integratoin, CPUFreq core 1/3

This patch touches cpufreq.c and freq_tables.c

drivers/cpufreq/Makefile +1 -4
index 71fc3b4..73f2e37 100644
@@ -1,5 +1,5 @@
 # CPUfreq core
-obj-$(CONFIG_CPU_FREQ)			+= cpufreq.o
+obj-$(CONFIG_CPU_FREQ)			+= cpufreq.o freq_helpers.o
 # CPUfreq stats
 obj-$(CONFIG_CPU_FREQ_STAT)             += cpufreq_stats.o
 
@@ -10,6 +10,3 @@ obj-$(CONFIG_CPU_FREQ_GOV_USERSPACE)	+= 
 obj-$(CONFIG_CPU_FREQ_GOV_ONDEMAND)	+= cpufreq_ondemand.o
 obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE)	+= cpufreq_conservative.o
 
-# CPUfreq cross-arch helpers
-obj-$(CONFIG_CPU_FREQ_TABLE)		+= freq_table.o
-
drivers/cpufreq/cpufreq.c +272 -260

945 lines not on this page · raw message

drivers/cpufreq/freq_helpers.c new file +317 -0

317 lines not on this page · raw message

drivers/cpufreq/freq_helpers.h new file +22 -0
new file mode 100644
index 0000000..78780ef
@@ -0,0 +1,22 @@
+/*
+ * cpufreq core helpers
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __LINUX_CPUFREQ_HELPERS_H__
+#define __LINUX_CPUFREQ_HELPERS_H__
+
+#include <linux/cpufreq.h>
+
+#define CPUFREQ_ENTRY_INVALID	~0
+
+int cpufreq_cpuinfo(struct cpufreq_policy *policy);
+int cpufreq_verify(struct cpufreq_policy *policy);
+int cpufreq_target(struct cpufreq_policy *policy, unsigned int target_freq,
+		   unsigned int relation);
+int cpufreq_setpolicy(struct cpufreq_policy *policy);
+
+#endif /* __LINUX_CPUFREQ_POWEROP_H__ */
+
drivers/cpufreq/freq_table.c deleted +0 -227

227 lines not on this page · raw message

include/linux/cpufreq.h +18 -72

129 lines not on this page · raw message

#1 lore raw

Hi,

drivers/cpufreq/Makefile line 2

Quoted 2 lines
drivers/cpufreq/Makefile +1 -1
-obj-$(CONFIG_CPU_FREQ)			+= cpufreq.o
+obj-$(CONFIG_CPU_FREQ)			+= cpufreq.o freq_helpers.o

So every driver is now required to use freq_helpers, even if it does not even allow for the setting of a specific frequency, but only a frequency range (longrun)? And drivers are limitied to a specific amount of CPU frequency states to export?

drivers/cpufreq/cpufreq.c void cpufreq_notify_transition(struct cp · line 244

Quoted 5 lines
drivers/cpufreq/cpufreq.c +1 -1
-	freqs->flags = cpufreq_driver->flags;
 	dprintk("notification %u of frequency transition to %u kHz\n",
 		state, freqs->new);
 
+	/* FIXME: don't we need a lock here? */

We're already holding a lock here, unless you changed the calling conventions.

Quoted 5 lines
drivers/cpufreq/cpufreq.c +0 -5
-		if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
-			if ((policy) && (policy->cpu == freqs->cpu) &&
-			    (policy->cur) && (policy->cur != freqs->old)) {
-				dprintk("Warning: CPU frequency is"
-					" %u, cpufreq assumed %u kHz.\n",

drivers/cpufreq/cpufreq.c void cpufreq_notify_transition(struct cp · line 259

Quoted 10 lines
drivers/cpufreq/cpufreq.c +6 -3
-					freqs->old, policy->cur);
-				freqs->old = policy->cur;
-			}
+		if ((policy) && (policy->cpu == freqs->cpu) &&
+		    (policy->cur) && (policy->cur != freqs->old)) {
+			dprintk("Warning: CPU frequency is"
+				" %u, cpufreq assumed %u kHz.\n",
+				freqs->old, policy->cur);
+			freqs->old = policy->cur;
 		}

Why are you removing the check for CONST_LOOPS here?

drivers/cpufreq/cpufreq.c static ssize_t show_scaling_governor (st · line 431

Quoted 5 lines
drivers/cpufreq/cpufreq.c +5 -0
+/**
+ * show_scaling_governor - show the current policy for the specified CPU
+ */
+static ssize_t 
+show_available_freqs(struct cpufreq_policy *policy, char *buf)

Description does not match function name; also please return type and function on one line (yes, I know, the current cpufreq core gets this wrong too, but still...)

drivers/cpufreq/cpufreq.c static ssize_t store_scaling_governor (s · line 481

Quoted 8 lines

/**
- * show_scaling_driver - show the cpufreq driver currently loaded
- */

drivers/cpufreq/cpufreq.c +0 -5
-static ssize_t show_scaling_driver (struct cpufreq_policy * policy, char *buf)
-{
-	return scnprintf(buf, CPUFREQ_NAME_LEN, "%s\n", cpufreq_driver->name);
-}
-

That breaks userspace.

Quoted 7 lines
drivers/cpufreq/cpufreq.c +1 -0
+extern cpumask_t arch_get_dependent_cpus_mask(int cpu);
 
 
 
 /**
  * cpufreq_add_dev - add a CPU device

drivers/cpufreq/cpufreq.c static int cpufreq_add_dev (struct sys_d · line 641

Quoted 8 lines
drivers/cpufreq/cpufreq.c +1 -1
@@ -617,10 +635,10 @@ static int cpufreq_add_dev (struct sys_d
 	int ret = 0;
 	struct cpufreq_policy new_policy;
 	struct cpufreq_policy *policy;
-	struct freq_attr **drv_attr;
 	struct sys_device *cpu_sys_dev;
 	unsigned long flags;
 	unsigned int j;
+	char freq_n[CPUFREQ_FREQ_STR_SIZE];

Will cause troubles for gx-suspmod which would want to export >10.000 states.

drivers/cpufreq/cpufreq.c #endif · line 651

Quoted 9 lines
drivers/cpufreq/cpufreq.c +1 -1
 #ifdef CONFIG_SMP
 	struct cpufreq_policy *managed_policy;
 #endif
@@ -630,7 +648,7 @@ #endif
 
 	cpufreq_debug_disable_ratelimit();
 	dprintk("adding CPU %u\n", cpu);
-
+	

Inserting trailing whitespace

drivers/cpufreq/cpufreq.c #ifdef CONFIG_SMP · line 661

Quoted 11 lines
drivers/cpufreq/cpufreq.c +1 -1
 #ifdef CONFIG_SMP
 	/* check whether a different CPU already registered this
 	 * CPU because it is in the same boat. */
@@ -638,15 +656,9 @@ #ifdef CONFIG_SMP
 	if (unlikely(policy)) {
 		cpufreq_cpu_put(policy);
 		cpufreq_debug_enable_ratelimit();
-		return 0;
+		return ret;
 	}
 #endif

Why are you returning an error here?

drivers/cpufreq/cpufreq.c #endif · line 674

Quoted 11 lines
drivers/cpufreq/cpufreq.c +6 -1
@@ -654,23 +666,38 @@ #endif
 	}
 
 	policy->cpu = cpu;
-	policy->cpus = cpumask_of_cpu(cpu);
+
+	/* 
+	 * FIXME: this will go away once notifications moved to lower layers 
+	 * presumably to clock/voltage framework level
+	 */
+	policy->cpus = arch_get_dependent_cpus_mask(cpu);

Can't find arch_get_dependent_cpus_mask defined anywhere or in this patch. As each patch should at least compile and boot, that won't work.

Quoted 4 lines
drivers/cpufreq/cpufreq.c +0 -0
 	mutex_init(&policy->lock);
 	mutex_lock(&policy->lock);
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update, (void *)(long)cpu);

drivers/cpufreq/cpufreq.c #endif · line 684

Quoted 10 lines
drivers/cpufreq/cpufreq.c +4 -5
 
-	/* call driver. From then on the cpufreq must be able
-	 * to accept all calls to ->verify and ->setpolicy for this CPU
-	 */
-	ret = cpufreq_driver->init(policy);
-	if (ret) {
+	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
+
+	/* FIXME: soon this will come from PowerOP Core as well */
+	policy->cpuinfo.transition_latency = 10000; /* 10uS latency */

Ouch.

Quoted 6 lines
drivers/cpufreq/cpufreq.c +0 -2
@@ -692,8 +719,16 @@ #ifdef CONFIG_SMP
 
 			cpufreq_debug_enable_ratelimit();
 			mutex_unlock(&policy->lock);
-			ret = 0;
-			goto err_out_driver_exit; /* call driver->exit() */

drivers/cpufreq/cpufreq.c #ifdef CONFIG_SMP · line 731

Quoted 10 lines
drivers/cpufreq/cpufreq.c +10 -0
+			/* 
+			 * FIXME: interesting... cpufreq handles one policy 
+			 * entry for all affected CPUs... it works since 
+			 * currently same set of freq/voltage pairs are always 
+			 * registered for all CPUs. That bacomes not the case
+			 * with PowerOP approach when we can have different
+			 * set of operating points even for affected CPUs
+			 */ 
+			kfree(policy);
+			return 0;

No, that's not true -- "affected CPUs" can only be set to the same frequency at any point. It's the same "clock domain". Therefore, your statement is invalid.

drivers/cpufreq/cpufreq.c unsigned int cpufreq_quick_get(unsigned · line 977

Quoted 7 lines
drivers/cpufreq/cpufreq.c +1 -0
@@ -944,6 +972,7 @@ unsigned int cpufreq_quick_get(unsigned 
 EXPORT_SYMBOL(cpufreq_quick_get);
 
 
+/* FIXME: REVISIT: this routine returns 0 on error! */
 /**
  * cpufreq_get - get the current CPU frequency (in kHz)

Yes. For if it were to return -EINVAL, userspace would tell you that you've got at ~4294 GHz CPU.

drivers/cpufreq/cpufreq.c #endif /* CONFIG_HOTPLUG_CPU */ · line 1683

Quoted 2 lines
drivers/cpufreq/cpufreq.c +2 -0
+module_param(smart_cpu, uint, 0644);
+MODULE_PARM_DESC(smart_cpu, "Define if cpu handles frequency intervals insted oftarget frequencies");

Ouch.

These were just a few comments on the code. On the "big picture", I'll write a separate e-mail.

Thanks,
Dominik