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:

quoted

On Wed, Jan 19, 2011 at 07:43:05AM +0530, deepaksi wrote:

quoted

Hi James,

On 1/19/2011 5:50 AM, Jamie Iles wrote:

quoted

quoted

+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