linux-kernel - Re: [PATCH 6/6 v8] cpufreq, highbank: add support for highbank cpufreq

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 for Android: free password hash cracker in your pocket

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

Message-ID: <50BFC628.606@calxeda.com>
Date:	Wed, 05 Dec 2012 16:09:44 -0600
From:	Mark Langsdorf <mark.langsdorf@...xeda.com>
To:	Mike Turquette <mturquette@...aro.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"cpufreq@...r.kernel.org" <cpufreq@...r.kernel.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Shawn Guo <shawn.guo@...aro.org>
Subject: Re: [PATCH 6/6 v8] cpufreq, highbank: add support for highbank cpufreq

On 12/05/2012 12:49 PM, Mike Turquette wrote:
> On Wed, Dec 5, 2012 at 8:48 AM, Mark Langsdorf
> <mark.langsdorf@...xeda.com> wrote:
>> diff --git a/drivers/cpufreq/highbank-cpufreq.c b/drivers/cpufreq/highbank-cpufreq.c
>> new file mode 100644
>> index 0000000..1f28fa6
>> --- /dev/null
>> +++ b/drivers/cpufreq/highbank-cpufreq.c
>> @@ -0,0 +1,102 @@
> 
> Looks pretty good to me.  Some tedious nitpicks and discussion below.
> <snip>
> 
>> +static int hb_voltage_change(unsigned int freq)
>> +{
>> +       int i;
>> +       u32 msg[7];
>> +
>> +       msg[0] = HB_CPUFREQ_CHANGE_NOTE;
>> +       msg[1] = freq / 1000000;
>> +       for (i = 2; i < 7; i++)
>> +               msg[i] = 0;
>> +
>> +       return pl320_ipc_transmit(msg);
>> +}
>> +
>> +static int hb_cpufreq_clk_notify(struct notifier_block *nb,
>> +                               unsigned long action, void *hclk)
>> +{
>> +       struct clk_notifier_data *clk_data = hclk;
>> +       int i = 0;
>> +
>> +       if (action == PRE_RATE_CHANGE) {
>> +               if (clk_data->new_rate > clk_data->old_rate)
>> +                       while (hb_voltage_change(clk_data->new_rate))
>> +                               if (i++ > 15)
> 
> There are a few magic numbers here.  How about something like:
> 
> #define HB_VOLT_CHANGE_MAX_TRIES 15
> 
> Maybe do the same for the i2c message length?

Fixed.

>> +                                       return NOTIFY_STOP;
> 
> How about NOTIFY_BAD?  It more clearly signals that an error has occurred.

> Same as above.  It is true that the clock framework does nothing with
> post-rate change notifier aborts but that might change in the future.

Changed and added.

>> +       }
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block hb_cpufreq_clk_nb = {
>> +       .notifier_call = hb_cpufreq_clk_notify,
>> +};
>> +
> 
> Do you have any plans to convert your voltage change routine over to
> the regulator framework?  Likewise do you plan to use the OPP library
> in the future?  I can understand if you do not do that since your
> regulator/dvfs programming model makes things very simple for you.

I looked at treating the ECME as a voltage regulator, but it was a very
bad fit. The ECME has a certain amount of intelligence built into it and
corporate plans are to treat voltage control as a black box.

The current solution is actually nicely generic from my perspective. The
clk notifiers guarantee we can make the voltage changes at the right
time regardless of the underlying cpufreq driver implementation. I don't
think we need more until we get into cpufreq QoS issues, and even then
I'd want to stick with something like the current structure.

--Mark Langsdorf
Calxeda, Inc.
--
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/