linux-kernel - Re: [PATCH V4 3/3] 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]

Message-ID: <CAKohponsZOpV74+SLMDLy0oCV-j896wA9--bwbwk4GUL59m1ug@mail.gmail.com>
Date:	Fri, 30 May 2014 07:26:55 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Stephen Warren <swarren@...dotorg.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 Mailing List <linux-kernel@...r.kernel.org>,
	Arvind Chauhan <arvind.chauhan@....com>,
	Stephen Warren <swarren@...dia.com>,
	Doug Anderson <dianders@...omium.org>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>
Subject: Re: [PATCH V4 3/3] cpufreq: Tegra: implement intermediate frequency callbacks

On 29 May 2014 23:10, Stephen Warren <swarren@...dotorg.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-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/