Viresh,

On Wed, May 21, 2014 at 1:59 AM, Viresh Kumar viresh.kumar@linaro.org wrote:

@@ -1841,9 +1876,23 @@ static int __target_index(struct cpufreq_policy *policy, pr_err("%s: Failed to change cpu frequency: %d\n", __func__, retval);

•   if (notify)
  

•   if (notify) {
            cpufreq_freq_transition_end(policy, &freqs, retval);
  
  

•           /*
  

•            * Failed after setting to intermediate freq? Driver should have
  

•            * reverted back to initial frequency and so should we. Check
  

•            * here for intermediate_freq instead of get_intermediate, in
  

•            * case we have't switched to intermediate freq at all.
  

•            */
  

•           if (unlikely(retval && intermediate_freq)) {
  

•                   freqs.old = intermediate_freq;
  

•                   freqs.new = policy->restore_freq;
  

•                   cpufreq_freq_transition_begin(policy, &freqs);
  

•                   cpufreq_freq_transition_end(policy, &freqs, retval);
  

As far as I can tell this notification says "I tried to switch from "intermediate_freq" to "policy->restore_freq" but I failed, so I'm still at "intermediate_freq". I think you probably want to pass 0 as the last argument to cpufreq_freq_transition_end() to fix...

Other than that this looks good to me. I'll do a final review when you spin the next version (since I think you need to fix stuff for Stephen too). I'll probably wait and re-review the Tegra version when you and Stephen come to a consensus on it.

-Doug

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.

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 };