Re: [PATCH] cpufreq: Processor Clocking Control interface driver

[Dominik Brodowski <linux@dominikbrodowski.net>]

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