Subject: Re: [PATCH v3] [2.6.22] pasemi: cpufreq driver

From: [email-masked] (Olof Johansson)
Date: Sat, 28 Apr 2007 23:42:37 -0500

On Sat, Apr 28, 2007 at 10:50:21PM -0400, Dominik Brodowski wrote:
> Hi,
> 
> On Fri, Apr 27, 2007 at 12:46:01AM -0500, Olof Johansson wrote:
> > +	max_freq = (u32*) get_property(cpu, "clock-frequency", NULL);
> 
> (u32) or (32*) ?
> 
> > +	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?

Firmware normally sets only cpu 0 (since cpu 1 isn't started until after
linux is loaded and started). So it'll works well by default.

> > +#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?

After doze, we come back out in astate 0, so we need to raise it back
up. We do so by calling the restore_astate() function. If we don't have
it, and use idle=doze, we'll end up with a cpu running at astate 0 and
no way to raise it.

> Overall, the patch looks good to me.

Thanks,

-Olof