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

Viresh Kumar
2014-05-30T01:56:57Z

On 29 May 2014 23:10, Stephen Warren <swarren@wwwdotorg.org> 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.