[2.6.22] pasemi: cpufreq driver

arch/powerpc/platforms/pasemi/cpufreq.c new file +5 -0

+static int pas_cpufreq_cpu_init (struct cpufreq_policy *policy)
+{
+	u32 *max_freq;
+	int i, cur_astate;
+	struct device_node *cpu;

arch/powerpc/platforms/Kconfig +1 -1

@@ -155,7 +155,7 @@ source "drivers/cpufreq/Kconfig"
 
 config CPU_FREQ_PMAC
 	bool "Support for Apple PowerBooks"
-	depends on CPU_FREQ && ADB_PMU && PPC32
+	depends on ADB_PMU && PPC32
 	select CPU_FREQ_TABLE
 	help
 	  This adds support for frequency switching on Apple PowerBooks,

arch/powerpc/platforms/pasemi/cpufreq.c line 45

Quoted 6 lines · Olof Johansson

arch/powerpc/platforms/pasemi/cpufreq.c new file +6 -0

+/* This should eventually come out of the device tree */
+#define SDCPWR_BASE		0xfc104000
+#define SDCPWR_SIZE		0x1000
+
+#define SDCASR_BASE		0xfc120000
+#define SDCASR_SIZE		0x2000

the comment is right. Why don't you do it then? ;-)

Quoted 13 lines

arch/powerpc/platforms/pasemi/cpufreq.c new file +13 -0

+static int pas_cpufreq_cpu_init (struct cpufreq_policy *policy)
+{
+	u32 *max_freq;
+	int i, cur_astate;
+	struct device_node *cpu;
+
+	cpu = of_get_cpu_node(policy->cpu, NULL);
+
+	if(!cpu)
+		return -ENODEV;
+
+	sdcpwr_mapbase = ioremap(SDCPWR_BASE, SDCPWR_SIZE);
+	sdcasr_mapbase = ioremap(SDCASR_BASE, SDCASR_SIZE);

arch/powerpc/platforms/pasemi/cpufreq.c line 144

Quoted 2 lines

arch/powerpc/platforms/pasemi/cpufreq.c new file +2 -0

+	if (!sdcpwr_mapbase || !sdcasr_mapbase)
+		panic("SDCMAP: Cannot map registers!");

I can't see any check in here that finds out if you are actually running on the right hardware.
The proper way to implement this driver would be to register an of_platform_driver for the cpufreq device and then get the register addresses from there,

Quoted 11 lines

arch/powerpc/platforms/Kconfig +1 -1

@@ -155,7 +155,7 @@ source "drivers/cpufreq/Kconfig"
 
 config CPU_FREQ_PMAC
 	bool "Support for Apple PowerBooks"
-	depends on CPU_FREQ && ADB_PMU && PPC32
+	depends on ADB_PMU && PPC32
 	select CPU_FREQ_TABLE
 	help
 	  This adds support for frequency switching on Apple PowerBooks,

@@ -164,11 +164,21 @@ config CPU_FREQ_PMAC
 

arch/powerpc/platforms/Kconfig config CPU_FREQ_PMAC · line 171

Quoted 8 lines

arch/powerpc/platforms/Kconfig +1 -1

 config CPU_FREQ_PMAC64
 	bool "Support for some Apple G5s"
-	depends on CPU_FREQ && PPC64
+	depends on PPC64
 	select CPU_FREQ_TABLE
 	help
 	  This adds support for frequency switching on Apple iMac G5,
 	  and some of the more recent desktop G5 machines as well.

Why this change?

Quoted 4 lines

arch/powerpc/platforms/Kconfig +4 -0

+config PPC_PASEMI_CPUFREQ
+	bool "Support for PA Semi PWRficient"
+	depends on PPC_PASEMI
+	default y

arch/powerpc/platforms/Kconfig config CPU_FREQ_PMAC · line 182

Quoted 6 lines

arch/powerpc/platforms/Kconfig +5 -0

+	select CPU_FREQ_TABLE
+	help
+	  This adds the support for frequency switching on PA Semi
+	  PWRficient processors.
+
 endmenu

Why bool and not tristate?

2) See the idle loop interaction: It can go tristate once we have runtime selection of idle loop, but until then we need it for idle=doze.

I can't see any check in here that finds out if you are actually running on the right hardware.

Quoted 2 lines · Arnd Bergmann

I can't see any check in here that finds out if you are actually running on the right hardware.

Seems like this came along with our use of cbe_cpufreq.c as a basis. You don't do it either! :-)

Enabling CONFIG_CBE_CPUFREQ makes my machine die a horrible death, see below. I'd post a patch if I knew what machine compatible fields to compare with, but I have no clue what's considered approprate to check for on cell.

arch/powerpc/platforms/cell/cbe_cpufreq.c +1 -1

@@ -315,7 +318,7 @@ static int __init cbe_cpufreq_init(void)
 
 static void __exit cbe_cpufreq_exit(void)
 {
-	if(pmi_dev)
+	if (pmi_dev)
 		pmi_unregister_handler(pmi_dev, &cbe_pmi_handler);
 
 	cpufreq_unregister_driver(&cbe_cpufreq_driver);

Quoted 4 lines · Olof Johansson

arch/powerpc/platforms/cell/cbe_cpufreq.c +0 -0

@@ -303,6 +303,9 @@ static int __init cbe_cpufreq_init(void)
 {
 	struct device_node *np;
 

arch/powerpc/platforms/cell/cbe_cpufreq.c static int __init cbe_cpufreq_init(void) · line 308

Quoted 3 lines

arch/powerpc/platforms/cell/cbe_cpufreq.c +3 -0

+	if (!machine_is(cell))
+		return 0;
+

Please return -ENODEV to prevent loading module.

Otherwise it will cause crash when unloading the module because of cpufreq_unregister_driver() with unregistered driver.

cbe_cpufreq cleanups:

* comment format
* whitespace
* don't init on non-cell platforms

Signed-off-by Olof Johansson <[email protected]>

I chose not to do this as an of_platform driver since it doesn't fit that well with the cpufreq driver model; having 3 levels of init/probe functions is excessive.

arch/powerpc/platforms/pasemi/cpufreq.c new file +17 -0

+	dn = of_find_compatible_node(NULL, "sdc", "1682m-sdc");
+	if (!dn)
+		goto out;
+	err = of_address_to_resource(dn, 0, &res);
+	of_node_put(dn);
+	if (err)
+		goto out;
+	sdcasr_mapbase = ioremap(res.start + SDCASR_OFFSET, 0x2000);
+	if (!sdcasr_mapbase) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	dn = of_find_compatible_node(NULL, "gizmo", "1682m-gizmo");
+	if (!dn) {
+		err = -ENODEV;
+		goto out_unmap_sdcasr;

Quoted 3 lines · Olof Johansson

I chose not to do this as an of_platform driver since it doesn't fit that well with the cpufreq driver model; having 3 levels of init/probe functions is excessive.

<snip>

Quoted 23 lines

arch/powerpc/platforms/pasemi/cpufreq.c new file +23 -0

+	dn = of_find_compatible_node(NULL, "sdc", "1682m-sdc");
+	if (!dn)
+		goto out;
+	err = of_address_to_resource(dn, 0, &res);
+	of_node_put(dn);
+	if (err)
+		goto out;
+	sdcasr_mapbase = ioremap(res.start + SDCASR_OFFSET, 0x2000);
+	if (!sdcasr_mapbase) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	dn = of_find_compatible_node(NULL, "gizmo", "1682m-gizmo");
+	if (!dn) {
+		err = -ENODEV;
+		goto out_unmap_sdcasr;
+	}
+	err = of_address_to_resource(dn, 0, &res);
+	of_node_put(dn);
+	if (err)
+		goto out_unmap_sdcasr;
+	sdcpwr_mapbase = ioremap(res.start, 0x1000);

arch/powerpc/platforms/pasemi/cpufreq.c line 174

Quoted 4 lines

arch/powerpc/platforms/pasemi/cpufreq.c new file +4 -0

+	if (!sdcpwr_mapbase) {
+		err = -EINVAL;
+		goto out_unmap_sdcasr;
+	}

What are sdc and gizmo anyway? If they are both only used for cpufreq, maybe the easiest way to do this with an of_platform_driver would be to have a single node that has two register ranges.

SDC is the system and debug controller, it contains a number of smaller devices such as the PIC, the PMU (Gizmo), RNG, and various debug features. Some already have drivers submitted, others will later on.

Unfortunately the setting of the current active state is done to an SDC register, while information of the states is in the PMU, so access to both is needed in the driver.

Quoted 4 lines

Unfortunately the setting of the current active state is done to an SDC
register, while information of the states is in the PMU, so access to both is needed in the driver.

One thing that you could do is to list only this one register of the SDC in the reg property, not all of the SDC.

Quoted 7 lines

Quoted 4 lines

Unfortunately the setting of the current active state is done to an SDC
register, while information of the states is in the PMU, so access to both is needed in the driver.

One thing that you could do is to list only this one register of the SDC in the reg property, not all of the SDC.

The system-controller node should really describe all of the system controller. If the Linux driver needs to access two separate devices, it should really just do that. Don't try to make things "simpler" in the device tree.

You probably should map the SDC registers in the platform startop code, and then have the various platform drivers use that. Maybe even do an abstraction ;-)

Quoted 4 lines

You probably should map the SDC registers in the
platform startop code, and then have the various
platform drivers use that. Maybe even do an
abstraction ;-)

Yep, it's a good idea to do and I'll look at it for 2.6.23 time frame.

Quoted 3 lines · Olof Johansson

SDC is the system and debug controller, it contains a number of smaller devices such as the PIC, the PMU (Gizmo), RNG, and various debug features. Some already have drivers submitted, others will later on.

Oh well, these chips all seem to be the same. On cell, we have solved the problem by introducing the 'cbe_regs' helper library that gives access to all those miscellaneous registers to the individual device drivers, so that not all of them need to scan the device tree for the same registers and map them individually.

The problem with an of_platform_driver for this would be that you can only have _one_ driver attached to the registers.

Quoted 3 lines

Unfortunately the setting of the current active state is done to an SDC register, while information of the states is in the PMU, so access to both is needed in the driver.

One thing that you could do is to list only this one register of the SDC in the reg property, not all of the SDC.

Having a driver/library for the SDC that the gizmo driver can use is a good idea, similar to what you've done on cell. I'll try to have that ready for 2.6.23, but that shouldn't stop this base driver from going in now.

arch/powerpc/platforms/pasemi/cpufreq.c line 204

Quoted 3 lines · Olof Johansson

arch/powerpc/platforms/pasemi/cpufreq.c new file +2 -0

+       policy->cur = pas_freqs[cur_astate].frequency;
+       policy->cpus = cpu_possible_map; 

That doesn't seem right.

Either, all your processors scale along each other in which case you should use cpu_online_map here, or they scale each on their own in which case you just set a single bit here.

The generic code works like this:
CPU 0 is brought online and cpufreq initialised for it
CPU 1 is brought online and cpufreq initialised for it. if cpus includes more than a single bit, cpufreq is linked to the first other CPU in policy->cpus and then cpufreq for CPU1 is deinitialised again.

We have the same bug on powermac but for some reason the patch to fix it that I posted a long time ago (look for "powermac: fix G5-cpufreq for cpu on/offline") wasn't applied yet. Look at it though, it includes a comment on what needs to be done.

You can set the requested speed per-processor, but the actual speed will be the max of them. Because of this, it's less surprising to set it on both at the same time since then you'll at least know what you're running at.

So yes, I'll change it to online_map instead of possible_map.

Hmm, I just tried adding debugging to the cpu_init code, and I'm not seeing it called more than once (i.e. only for cpu 0). Mind you, I don't have cpu hotplug support at this time.

Quoted 3 lines

Hmm, I just tried adding debugging to the cpu_init code, and I'm not seeing it called more than once (i.e. only for cpu 0). Mind you, I don't have cpu hotplug support at this time.

Oh right, you need cpu hotplug support for the bug to show up. What happens then is you unplug and replug a CPU and the sysfs cpufreq stuff for the CPU you just replugged is gone.

Hi,

arch/powerpc/platforms/pasemi/cpufreq.c line 178

Quoted 1 line · Olof Johansson

arch/powerpc/platforms/pasemi/cpufreq.c new file +1 -0

+	max_freq = (u32*) get_property(cpu, "clock-frequency", NULL);

(u32) or (32*) ?

Quoted 4 lines · On Sat, 28 Apr 2007 22:50:21 -0400 Dominik Brodowski…

Hi,

On Fri, Apr 27, 2007 at 12:46:01AM -0500, Olof Johansson wrote:

Quoted 3 lines

arch/powerpc/platforms/pasemi/cpufreq.c line 178

Quoted 1 line

arch/powerpc/platforms/pasemi/cpufreq.c new file +1 -0

+	max_freq = (u32*) get_property(cpu, "clock-frequency", NULL);

(u32) or (32*) ?

Neither, get_property now returns (void *) so casting is not needed or wanted.

Also, later in the patch, you modify the property value in place. Do not do this. To help you remember, if you had not done the cast, gcc would have complained because get_property return a const void *.

Also, get_property is now called of_get_property.

Hi,

arch/powerpc/platforms/pasemi/cpufreq.c line 178

Quoted 1 line · Olof Johansson

arch/powerpc/platforms/pasemi/cpufreq.c new file +1 -0

+	max_freq = (u32*) get_property(cpu, "clock-frequency", NULL);

(u32) or (32*) ?

arch/powerpc/platforms/pasemi/cpufreq.c line 200

Quoted 1 line

arch/powerpc/platforms/pasemi/cpufreq.c new file +1 -0

+	cur_astate = get_cur_astate(policy->cpu);

May the different cores have different settings at initalization? e.g.

core 0: freq A
core 1: freq B

with (freq B > freq A)? If so, cur_astate is set wrongly, as the _effective_ frequency would be freq B, right?

arch/powerpc/platforms/pasemi/idle.c static int pasemi_system_reset_exception · line 79

Quoted 5 lines

arch/powerpc/platforms/pasemi/idle.c +5 -0

+#ifndef CONFIG_PPC_PASEMI_CPUFREQ
+	printk(KERN_WARNING "No cpufreq driver, powersavings modes disabled\n");
+	current_mode = 0;
+#endif
+

This confuses me a bit -- does something else than cpufreq not work if cpufreq is disabled?

Overall, the patch looks good to me.