[PATCH V4 38/62] SPEAr CPU freq: Adding support for CPU Freq framework

[shiraz.hashim@st.com (Shiraz Hashim)]

Hi James,

On Wed, Jan 19, 2011 at 04:35:27PM +0800, Jamie Iles wrote:
> On Wed, Jan 19, 2011 at 07:43:05AM +0530, deepaksi wrote:
> > Hi James,
> > 
> > On 1/19/2011 5:50 AM, Jamie Iles wrote:
> > >> +static int spear_cpufreq_target(struct cpufreq_policy *policy,
> > >> +		unsigned int target_freq, unsigned int relation)
> > >> +{
> > >> +	struct cpufreq_freqs freqs;
> > >> +	int ret = 0;
> > >> +	int index;
> > >> +
> > >> +	if (policy->cpu != 0)
> > >> +		return -EINVAL;
> > >> +
> > >> +	if (cpufreq_frequency_table_target(policy, spear_freq_tbl,
> > >> +				target_freq, relation, &index))
> > >> +		return -EINVAL;
> > >> +
> > >> +	freqs.old = spear_cpufreq_get(0);
> > >> +	freqs.new = spear_cpu_freq[index];
> > >> +	freqs.cpu = policy->cpu;
> > >> +
> > >> +	if (freqs.old == target_freq)
> > >> +		return 0;
> > >> +
> > >> +	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > >> +	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> > >> +	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > >>     
> > > If clk_set_rate() failed then do you need to do freqs.new = 
> > > clk_get_rate(cpu_clk) before doing the CPUFREQ_POSTCHANGE to make sure you 
> > > don't notify the wrong frequency?
> > >
> > >   
> > I do have a point over here. In few of the drivers where the the pre
> > notifiers have been added, the operations have been stopped (assume a
> > network driver where we stalled a dma operation), and then we set the
> > clock rate, followed by a post notifier where driver resumes it operations.
> > 
> > Now lets take the case, when the clock set rate fails (  Most failure
> > could only be when the driver is unable to find any clock rate which is
> > less then/equal to requested rate) .
> > The post transition notifier should happen so as to allow the driver to
> > resume its functions. If there are no clock rate changes because of set
> > rate failures, the driver would be aware of that by using calls such as
> > clk_get_rate, and would carry forward it's operations accordingly.
> > 
> > How do you suggest to go about it ?
> 
> Sorry, I didn't mean to remove the post notifier, just do something like:
> 
> 	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> 	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> 	if (ret)
> 		freqs.new = clk_get_rate(cpu_clk);
> 	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> 
> Or:
> 
> 	cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> 	ret = clk_set_rate(cpu_clk, freqs.new * 1000);
> 	if (ret)
> 		freqs.new = freqs.old;
> 	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> 
> So that you're notifying the cpufreq subsystem of the frequency that you're 
> actually running at with the post change notifier rather than the one you 
> tried to go for.  If you don't do this then for UP you'll end up recaculating 
> lpj based on the wrong frequency.

Agree. We would add it here. Thanks for pointing out.

-- 
regards
Shiraz