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

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

Hi Jamie,

On Wed, Jan 19, 2011 at 02:30:08PM +0530, Shiraz Hashim wrote:
> 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.

On second thought it should always be there irrespective of failures
as in our case it may happen that eventually CPU sets to a clock <=
desired clock. So we would do following

cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
ret = clk_set_rate(cpu_clk, freqs.new * 1000);
if (ret)
        pr_err("Could not change cpu clk rate\n");
freqs.new = clk_get_rate(cpu_clk);
cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);

Is it fine?

-- 
regards
Shiraz