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

[Mark Langsdorf]

On 12/05/2012 12:49 PM, Mike Turquette wrote:
> On Wed, Dec 5, 2012 at 8:48 AM, Mark Langsdorf
> <mark.langsdorf@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/