More comments.
On Fri, Oct 1, 2010 at 7:06 AM, Yong Shen <yong.s...@linaro.org> wrote:
> Hi Amit,
>
> Please see my feedback embedded.
>
> On Thu, Sep 30, 2010 at 6:48 PM, Amit Kucheria <amit.kuche...@linaro.org>
> wrote:
[snip]
>> > +static int mxc_set_target(struct cpufreq_policy *policy,
>> > + unsigned int target_freq, unsigned int relation)
>> > +{
>> > + struct cpufreq_freqs freqs;
>> > + int freq_Hz;
>> > + int ret = 0;
>> > +
>> > + if (cpufreq_suspended) {
>> > + target_freq = clk_get_rate(cpu_clk) / 1000;
>> > + freq_Hz = calc_frequency_khz(target_freq, relation) *
>> > 1000;
>> > + if (freq_Hz == arm_lpm_clk)
>> > + freqs.old = cpu_wp_tbl[cpu_wp_nr - 2].cpu_rate /
>> > 1000;
>> > + else
>> > + freqs.old = arm_lpm_clk / 1000;
>> > +
>> > + freqs.new = freq_Hz / 1000;
>> > + freqs.cpu = 0;
>> > + freqs.flags = 0;
>> > + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>> > + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
>> > + return ret;
>> > + }
>> > + /*
>> > + * Some governors do not respects CPU and policy lower limits
>> > + * which leads to bad things (division by zero etc), ensure
>> > + * that such things do not happen.
>> > + */
>>
>> Isn't that a bug in the governor? Can you explain a bit?
>
> Yong: the original driver writer might have concern that some governor
> implementations will not care about the low limit suggested by cpu policy,
> therefore it is a change to correct them.
Could you confirm if this is still the case, and if not, get rid of
this code/comment?
>> > + if (target_freq < policy->cpuinfo.min_freq)
>> > + target_freq = policy->cpuinfo.min_freq;
>> > +
>> > + if (target_freq < policy->min)
>> > + target_freq = policy->min;
>> > +
>> > + freq_Hz = calc_frequency_khz(target_freq, relation) * 1000;
>> > +
[snip]
>> > + /* Set the current working point. */
>> > + cpu_wp_tbl = get_cpu_wp(&cpu_wp_nr);
>> > +
>> > + cpu_freq_khz_min = cpu_wp_tbl[0].cpu_rate / 1000;
>> > + cpu_freq_khz_max = cpu_wp_tbl[0].cpu_rate / 1000;
>> > +
>> > + for (i = 0; i < cpu_wp_nr; i++) {
>> > + imx_freq_table[cpu_wp_nr - 1 - i].index = cpu_wp_nr - i;
>>
>> cpu_wp_nr = 2 here
>>
>> 1st iteration of for loop:
>> imx_freq_table[2 - 1 - 0].index = 2 - 0
>> so, imx_freq_table[1].index = 2
>>
>> 2nd iteration:
>> imx_freq_table[2 - 1 - 1].index = 2 - 1
>> imx_freq_table[0].index = 1
>>
>> So you're trying to reverse the table order? Why not just sort the table
>> date
>> in the way you want and add a comment on the top to keep it sorted.
>
> Yong: my understanding is that the freq table defined in another file is
> sorted in descending order, so the writer tends to make imx_freq_table in a
> descending order.
Just change the order of the wp_table instead of this confusing math
to re-sort it. Look at other cpufreq rate tables in the kernel.
>> > + imx_freq_table[cpu_wp_nr - 1 - i].frequency =
>> > + cpu_wp_tbl[i].cpu_rate / 1000;
>> > +
>> > + if ((cpu_wp_tbl[i].cpu_rate / 1000) < cpu_freq_khz_min)
>> > + cpu_freq_khz_min = cpu_wp_tbl[i].cpu_rate / 1000;
>> > +
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
