lkml.org 
[lkml]   [2009]   [Dec]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] cpufreq: Processor Clocking Control interface driver
Hey,

On Fri, Dec 11, 2009 at 10:55:40PM +0000, Naga Chumbalkar wrote:
> +The VERSION number for the driver will be of the format v.xy.ab.
> +eg: 1.00.02
> + ----- --
> + | |
> + | -- this will increase with bug fixes/enhancements to the driver
> + |-- this is the version of the PCC specification the driver adheres to

If this is _really_ necessary... Doesn't the driver version relate to some
Linux kernel version anyway?

> +2.1 scaling_available_frequencies:
> +----------------------------------
> +scaling_available_frequencies indicates the minimum and maximum speed
> +the CPU can take as advertised by the BIOS. No intermediate frequencies are
> +listed because the BIOS will try to achieve any intermediate frequency
> +requested by the governor. An intermediate frequency does not have to be
> +strictly associated with a P-state.

Why do you export scaling_available_frequencies anyway? It's made available
by the _optional_ freq-table helper module, which is wrong to use in this
case anyway.

> +2.2 cpuinfo_transition_latency:
> +-------------------------------
> +The cpuinfo_transition_latency field is 0. The PCC specification does
> +not include a field to expose this value currently.

Uh, bad specification... So does it work properly with ondemand and/or
conservative (which read out this field, and if latency=0 use a minimum
value)?

> +config X86_PCC_CPUFREQ
> + tristate "Processor Clocking Control interface driver"
> + select CPU_FREQ_TABLE
Uh, see above.

> +static int pcc_cpufreq_verify(struct cpufreq_policy *policy)
> +{
> + return cpufreq_frequency_table_verify(policy, pcc_freq_table);
> +}

Well, AFAICS, this limits the whole interface to two values: min or max. So
let's allow for any intermediate value:

static int pcc_cpufreq_verify(struct cpufreq_policy *policy)
{
cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq, policy->cpuinfo.max_freq);
}

but well... later on, you check this:

> + if (target_freq <= (ioread32(&pcch_hdr->minimum_frequency) * 1000)) {
> + target_freq = ioread32(&pcch_hdr->minimum_frequency) * 1000;
> + dprintk("target: target_freq for cpu %d was below limit, "
> + "converted it to %d\n", cpu, target_freq);
> + }

why not do this in the _verify() step? Does pcch_hdr->minimum_frequency
even change "on the fly"?

So, I'd propose to NACK this patch at the moment.

Best,
Dominik


\
 
 \ /
  Last update: 2009-12-12 00:15    [W:0.056 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site