[PATCH] cpufreq for freescale mx51

[Amit Kucheria]

More comments.

On Fri, Oct 1, 2010 at 7:06 AM, Yong Shen <yong.shen at linaro.org> wrote:
> Hi Amit,
>
> Please see my feedback embedded.
>
> On Thu, Sep 30, 2010 at 6:48 PM, Amit Kucheria <amit.kucheria at 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;
>> > +