| Message ID | dc2702973028425e3ded689a6da227658fb914c4.1400662383.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 05/21/2014 02:59 AM, Viresh Kumar wrote: > 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. > diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c > @@ -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; > + } I think the call to tegra_target_intermediate() is wrong here; shouldn't the cpufreq core guarantee that tegra_target_intermediate() has always been called before tegra_target(), so there's no need to repeat that call here? Also, tegra_target() doesn't seem to follow the rule documented by patch 2/3 that states ->target() should restore the orignal frequency on error. I'm not even sure if that's possible in general. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22 May 2014 22:09, Stephen Warren <swarren@wwwdotorg.org> wrote: > I think the call to tegra_target_intermediate() is wrong here; shouldn't > the cpufreq core guarantee that tegra_target_intermediate() has always > been called before tegra_target(), so there's no need to repeat that > call here? That's what Doug requested in the previous version. get_intermediate() can return zero in case drivers don't want to switch to intermediate frequency for some target frequency. Core should rather guarantee that target_index() is always called, if you want core to guarantee that target_intermediate() is also always called, then don't ever return zero from get_intermediate. I did it that way for tegra as both target_intermediate() & target_index() would have tried to set the same frequency for this particular case, i.e. when target freq == intermediate frequency. And both would have sent notification and the last notification wouldn't have made any sense, both old-freq & new-freq would have been intermediate freqs. So, yes I see the feature suggested by Doug quite useful. Like in your case. > Also, tegra_target() doesn't seem to follow the rule documented by patch > 2/3 that states ->target() should restore the orignal frequency on > error. I'm not even sure if that's possible in general. I thought I took care of that. Can you please give some example when we aren't restoring original frequency on failure ? About the rule, that has to be the expectation from core as there is no way out that for core to know what happened at end of target_index().. It can call get_rate() but that would be over engineering it looks .. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/21/2014 02:59 AM, Viresh Kumar wrote: > 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. This patch breaks Tegra. The reason is below. > 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) > +{ > + 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. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/22/2014 10:05 PM, Viresh Kumar wrote: > On 22 May 2014 22:09, Stephen Warren <swarren@wwwdotorg.org> wrote: >> I think the call to tegra_target_intermediate() is wrong here; shouldn't >> the cpufreq core guarantee that tegra_target_intermediate() has always >> been called before tegra_target(), so there's no need to repeat that >> call here? >> Also, tegra_target() doesn't seem to follow the rule documented by patch >> 2/3 that states ->target() should restore the orignal frequency on >> error. I'm not even sure if that's possible in general. > > I thought I took care of that. Can you please give some example when > we aren't restoring original frequency on failure ? > > About the rule, that has to be the expectation from core as there is no > way out that for core to know what happened at end of target_index().. > > It can call get_rate() but that would be over engineering it looks .. E.g. in the following code after the series is applied: > ret = clk_set_rate(pll_x_clk, rate * 1000); > /* Restore to earlier frequency on error, i.e. pll_x */ > if (ret) > pr_err("Failed to change pll_x to %lu\n", rate); > > ret = clk_set_parent(cpu_clk, pll_x_clk); If the clk_set_rate() failed, we have no idea what the pll_x rate is; I don't think the clock API guarantees that zero HW changes have been made if the function fails. Yet the code switches the CPU clock back to pll_x without attempting to fix the pll_x rate to be the old rate. Perhaps there's not much that can be done here though, since if one clk_set_rate() failed, perhaps the one to fix it will too. I suspect there are other issues, like switching between pll_p/pllx might not be restored on error, and the EMC frequency isn't switched back, etc. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/29/2014 07:56 PM, Viresh Kumar wrote: > On 29 May 2014 23:10, Stephen Warren <swarren@wwwdotorg.org> wrote: >> This patch breaks Tegra. The reason is below. ... >>> +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.. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29 May 2014 23:12, Stephen Warren <swarren@wwwdotorg.org> wrote: > E.g. in the following code after the series is applied: > >> ret = clk_set_rate(pll_x_clk, rate * 1000); >> /* Restore to earlier frequency on error, i.e. pll_x */ >> if (ret) >> pr_err("Failed to change pll_x to %lu\n", rate); >> >> ret = clk_set_parent(cpu_clk, pll_x_clk); > > If the clk_set_rate() failed, we have no idea what the pll_x rate is; I > don't think the clock API guarantees that zero HW changes have been made > if the function fails. Yet the code switches the CPU clock back to pll_x > without attempting to fix the pll_x rate to be the old rate. Perhaps > there's not much that can be done here though, since if one > clk_set_rate() failed, perhaps the one to fix it will too. This was the case here with the existing driver as well. CPUFreq core expects this today as well and so sends reverse notification in case of failures. > I suspect there are other issues, like switching between pll_p/pllx > might not be restored on error Another example would be useful.. > and the EMC frequency isn't switched back, etc. It wasn't in the existing code as well and so left it as is.. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c index 6e774c6..a64b970 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -46,7 +46,24 @@ static struct clk *pll_x_clk; static struct clk *pll_p_clk; static struct clk *emc_clk; -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; @@ -57,28 +74,9 @@ static int tegra_cpu_clk_set_rate(unsigned long rate) 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); -out: - clk_disable_unprepare(pll_x_clk); return ret; } @@ -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; + } + + 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); return ret; } @@ -137,16 +148,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 };
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 | 81 ++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 34 deletions(-)