On 30 May 2014 21:56, Stephen Warren swarren@wwwdotorg.org wrote:
I believe the issue is this:
We can't change the rate of pll_x while it's being used as a source for something. I'm not 100% sure why. I assume the PLL output simply isn't stable enough while changing rates; perhaps it can go out-of-range, or generate glitches.
This means we must switch the CPU clock source to something else (we use pll_p) while changing the pll_x rate.
Since the CPU is the only thing that uses pll_x, if we switch the CPU to some other clock source, pll_x will become unused, so it will be automatically disabled.
Disabling the PLL, and then re-enabling it later when switching the CPU back to it, presumably takes some extra time (e.g. waiting for PLL lock when it starts back up), so the code takes an extra reference to the clock (prepare_enable) before switching the CPU away from it, and drops that (disable_unprepare) after switching the CPU back to it.
The only case pll_x is disabled is when we use a cpufreq of 216MHz; that frequency is provided by pll_p itself, so we never switch back to pll_x in this case, and do want to shut down pll_x to save some power.
Now, in your patch when switching from 216MHz to pll_x, the initial prepare_enable(pll_x) never happens, then the CPU is switched back to pll_x which turns it on, then the unpaired disable_unprepare(pll_x) happens which turns off pll_x even though the CPU is using it as clock source. Arguably the clock API has a bug in that it shouldn't allow these unpaired calls to break the reference counting, but that's the way the API is right now.
Okay, that was very helpful..
What about this ? (Attached for testing) :
Author: Viresh Kumar viresh.kumar@linaro.org Date: Fri May 16 14:22:40 2014 +0530
cpufreq: Tegra: implement intermediate frequency callbacks
Tegra had always been switching to intermediate frequency (pll_p_clk) since ever. CPUFreq core has better support for handling notifications for these frequencies and so we can adapt Tegra's driver to it.
Also do a WARN() if clk_set_parent() fails while moving back to pll_x as we should have atleast restored to earlier frequency on error.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/tegra-cpufreq.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------- 1 file changed, 62 insertions(+), 35 deletions(-)
diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c index 6e774c6..25c145a 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -45,46 +45,51 @@ static struct clk *cpu_clk; static struct clk *pll_x_clk; static struct clk *pll_p_clk; static struct clk *emc_clk; +static int pll_p_clk_count;
-static int tegra_cpu_clk_set_rate(unsigned long rate) +static unsigned int tegra_get_intermediate(struct cpufreq_policy *policy, + unsigned int index) +{ + 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; + + return ifreq; +} + +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 + * off when we move the cpu off of it as enabling it again while we + * switch to it from tegra_target() would take additional time. Though + * when target-freq is intermediate freq, we don't need to take this + * reference. */ clk_prepare_enable(pll_x_clk);
ret = clk_set_parent(cpu_clk, pll_p_clk); - if (ret) { - pr_err("Failed to switch cpu to clock pll_p\n"); - goto out; - } - - if (rate == clk_get_rate(pll_p_clk)) - goto out; - - ret = clk_set_rate(pll_x_clk, rate); - if (ret) { - pr_err("Failed to change pll_x to %lu\n", rate); - goto out; - } - - ret = clk_set_parent(cpu_clk, pll_x_clk); - if (ret) { - pr_err("Failed to switch cpu to clock pll_x\n"); - goto out; - } + if (ret) + clk_disable_unprepare(pll_x_clk); + else + pll_p_clk_count++;
-out: - clk_disable_unprepare(pll_x_clk); return ret; }
static int tegra_target(struct cpufreq_policy *policy, unsigned int index) { unsigned long rate = freq_table[index].frequency; + unsigned int ifreq = clk_get_rate(pll_p_clk) / 1000; int ret = 0;
/* @@ -98,10 +103,30 @@ 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, don't need to take extra reference to pll_x_clk + * as it isn't used anymore. + */ + if (rate == ifreq) + return clk_set_parent(cpu_clk, pll_p_clk); + + 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); + + /* + * Drop count to pll_x clock only if we switched to intermediate freq + * earlier while transitioning to a target frequency. + */ + if (pll_p_clk_count) { + clk_disable_unprepare(pll_x_clk); + pll_p_clk_count--; + }
return ret; } @@ -137,16 +162,18 @@ static int tegra_cpu_exit(struct cpufreq_policy *policy) }
static struct cpufreq_driver tegra_cpufreq_driver = { - .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK, - .verify = cpufreq_generic_frequency_table_verify, - .target_index = tegra_target, - .get = cpufreq_generic_get, - .init = tegra_cpu_init, - .exit = tegra_cpu_exit, - .name = "tegra", - .attr = cpufreq_generic_attr, + .flags = CPUFREQ_NEED_INITIAL_FREQ_CHECK, + .verify = cpufreq_generic_frequency_table_verify, + .get_intermediate = tegra_get_intermediate, + .target_intermediate = tegra_target_intermediate, + .target_index = tegra_target, + .get = cpufreq_generic_get, + .init = tegra_cpu_init, + .exit = tegra_cpu_exit, + .name = "tegra", + .attr = cpufreq_generic_attr, #ifdef CONFIG_PM - .suspend = cpufreq_generic_suspend, + .suspend = cpufreq_generic_suspend, #endif };