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

[Doug Anderson]

Viresh,

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