Messages in this thread | | | Date | Fri, 30 May 2014 07:26:55 +0530 | Subject | Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks | From | Viresh Kumar <> |
| |
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.
| |