Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

[Viresh Kumar]

On 29 May 2014 23:10, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
> This patch breaks Tegra. The reason is below.

Lets see what blunder I made :)

>> diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c
>
>> -static int tegra_cpu_clk_set_rate(unsigned long rate)
>> +static unsigned int
>> +tegra_get_intermediate(struct cpufreq_policy *policy, unsigned int index)
>
> (BTW, can we please not put the return type on a separate line; it's
> inconsistent with the rest of the code in this file)

Sure.

>> +{
>> +     unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000;
>> +
>> +     /*
>> +      * Don't switch to intermediate freq if:
>> +      * - we are already at it, i.e. policy->cur == ifreq
>> +      * - index corresponds to ifreq
>> +      */
>> +     if ((freq_table[index].frequency == ifreq) || (policy->cur == ifreq))
>> +             return 0;
>
> If policy->cur == ifreq here, then tegra_target_intermediate() isn't
> called by the cpufreq core, so ...
>
>> +static int
>> +tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int index)
>>  {
>>       int ret;
>>
>>       /*
>>        * Take an extra reference to the main pll so it doesn't turn
>>        * off when we move the cpu off of it
>>        */
>>       clk_prepare_enable(pll_x_clk);
>
> ... that reference isn't added...
>
>> @@ -98,10 +96,23 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
>>       else
>>               clk_set_rate(emc_clk, 100000000);  /* emc 50Mhz */
>>
>> -     ret = tegra_cpu_clk_set_rate(rate * 1000);
>> +     /* target freq == pll_p */
>> +     if (rate * 1000 == clk_get_rate(pll_p_clk)) {
>> +             ret = tegra_target_intermediate(policy, index);
>> +             goto disable_pll_x;
>> +     }
>
> ... and this code doesn't call it either, since we could be switching
> from the pll_p rate to something faster ...
>
>> +
>> +     ret = clk_set_rate(pll_x_clk, rate * 1000);
>> +     /* Restore to earlier frequency on error, i.e. pll_x */
>>       if (ret)
>> -             pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n",
>> -                     rate);
>> +             pr_err("Failed to change pll_x to %lu\n", rate);
>> +
>> +     ret = clk_set_parent(cpu_clk, pll_x_clk);
>> +     /* This shouldn't fail while changing or restoring */
>> +     WARN_ON(ret);
>> +
>> +disable_pll_x:
>> +     clk_disable_unprepare(pll_x_clk);
>
> ... so this turns off pll_x even though we're running from it.

Can you describe the role of the enable/disable of this pll_x_clk please?
Which all clocks depend on it, etc? So that I understand why its important
to enable it and for which clocks. And also if we need to disable it after
changing to any freq..

> It would be simpler if Tegra *always* used an intermediate frequency,
> and hence the core *always* called tegra_target_intermediate().
> Admittedly, this would result in tegra_target() sometimes (when
> switching CPU clock rate to the pll_p rate) doing nothing other than
> removing the extra reference on pll_x, but I think that the code would
> be simpler to follow and more robust.

Ok, will check what suits best.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/