linux-kernel - Re: [PATCH V3 4/4] cpufreq: Tegra: implement intermediate frequency callbacks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 20 May 2014 09:49:35 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Arvind Chauhan <arvind.chauhan@....com>,
	Stephen Warren <swarren@...dia.com>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Stephen Warren <swarren@...dotorg.org>,
	Russell King <linux@....linux.org.uk>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>
Subject: Re: [PATCH V3 4/4] cpufreq: Tegra: implement intermediate frequency callbacks

Viresh,

On Fri, May 16, 2014 at 9:51 PM, Viresh Kumar <viresh.kumar@...aro.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@...dia.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/