| Message ID | 0fcd08cda4001198c02fb41f3e6437bf758417d1.1400302114.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
Viresh, On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@linaro.org> 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. > > Tested-by: Stephen Warren <swarren@nvidia.com> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/tegra-cpufreq.c | 71 +++++++++++++++++++++-------------------- > 1 file changed, 37 insertions(+), 34 deletions(-) > > diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c > index 6e774c6..fc66442 100644 > --- a/drivers/cpufreq/tegra-cpufreq.c > +++ b/drivers/cpufreq/tegra-cpufreq.c > @@ -46,10 +46,19 @@ 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) > +{ > + return clk_get_rate(pll_p_clk) / 1000; /* kHz */ > +} > + > +static int > +tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency) Note that in the old code you used to set the "emc" clock before the transition to the intermediate clock. Now you don't. Are you sure it's OK to change this order? > @@ -98,10 +88,21 @@ 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); > + if (rate == clk_get_rate(pll_p_clk)) Shouldn't this be "rate * 1000 =="? > + goto out; > + > + ret = clk_set_rate(pll_x_clk, rate * 1000); > + if (ret) { > + pr_err("Failed to change pll_x to %lu\n", rate); > + goto out; > + } I feel like this should be in tegra_target_intermediate(), since it could fail (right?). Essentially we want to be as sure as possible that tegra_target() doesn't return an error code. > + > + ret = clk_set_parent(cpu_clk, pll_x_clk); Presumably this won't actually ever fail, since that violates the rules that target() shouldn't fail if you used an intermediate frequency. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 20 May 2014 22:19, Doug Anderson <dianders@chromium.org> wrote: > Note that in the old code you used to set the "emc" clock before the > transition to the intermediate clock. Now you don't. Are you sure > it's OK to change this order? Yeah, I have seen that and as Stephen didn't had any objection to the change I thought it is probably fine. >> @@ -98,10 +88,21 @@ 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); >> + if (rate == clk_get_rate(pll_p_clk)) > > Shouldn't this be "rate * 1000 =="? Yes. >> + goto out; >> + >> + ret = clk_set_rate(pll_x_clk, rate * 1000); >> + if (ret) { >> + pr_err("Failed to change pll_x to %lu\n", rate); >> + goto out; >> + } > > I feel like this should be in tegra_target_intermediate(), since it > could fail (right?). Essentially we want to be as sure as possible > that tegra_target() doesn't return an error code. > > >> + >> + ret = clk_set_parent(cpu_clk, pll_x_clk); > > Presumably this won't actually ever fail, since that violates the > rules that target() shouldn't fail if you used an intermediate > frequency. Don't know, it looks this rule isn't going to last long.. Let me see if I can improve it a bit. -- 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..fc66442 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -46,10 +46,19 @@ 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) +{ + return clk_get_rate(pll_p_clk) / 1000; /* kHz */ +} + +static int +tegra_target_intermediate(struct cpufreq_policy *policy, unsigned int frequency) { int ret; + WARN_ON(frequency != clk_get_rate(pll_p_clk) / 1000); + /* * Take an extra reference to the main pll so it doesn't turn * off when we move the cpu off of it @@ -57,28 +66,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 +88,21 @@ 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); + if (rate == clk_get_rate(pll_p_clk)) + goto out; + + ret = clk_set_rate(pll_x_clk, rate * 1000); + 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("cpu-tegra: Failed to set cpu frequency to %lu kHz\n", - rate); + pr_err("Failed to switch cpu to clock pll_x\n"); + +out: + clk_disable_unprepare(pll_x_clk); return ret; } @@ -137,16 +138,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 };