On 12/05/2012 12:49 PM, Mike Turquette wrote:

On Wed, Dec 5, 2012 at 8:48 AM, Mark Langsdorf
[off-list ref] wrote:

quoted

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>

quoted

+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.

quoted

+                                       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.

quoted

+       }
+
+       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.