cpufreq driver for nForce2

cpufreq driver for nForce2

[Sebastian Witt]

Hello,

I'm writing a cpufreq driver for nForce2 mainboards which changes the 
FSB instead of using any power-save functions of the CPU.

The good thing is: It works with all CPUs on nForce2 plattforms, the bad 
thing: currently the minimum FSB is on the most boards at boot FSB - 60.
But I think with a high multiplier the CPU frequency range is high enough.

A test version is available at: 
http://www.hasw.net/linux/cpufreq-nforce2-0.1.tar.bz2

Tested with kernel 2.6.6.

It would be nice to hear suggestions, bug reports, etc.

Regards,
Sebastian

Re: cpufreq driver for nForce2

[Dominik Brodowski]

Hi Sebastian,

A few remarks on your driver:

- nforce2_verify should assert that there's at least one valid state in
  between policy->min and policy->max. If policy->min were 55300 kHz and
  policy->max 55400 kHz, nforce2_verify wouldn't adapt. Something like
	fsb_pol_max = policy->max / (fid * 1000);
	if (policy->min < (fsb_pol_max * fid * 1000))
		policy->max = (fsb_pol_max + 1) * fid * 1000;
  Or does the code in nforce2_calc_pll mean that there might be only one
  "state" available in a 4 MHz FSB interval? If so, the code above needs to
  be modified.

- Why is the new PLL value written _forty_ times?

- nforce2_target should be ordered a bit differently: calculate
	target_fsb = target_freq / (fid * 1000);
  first, then set
	freqs.new = target_fsb * fid * 1000;

- implement a ->get() function, please. It's (probably) only
static unsigned int nforce2_get(unsigned int cpu)
{
	if (cpu)
		return 0;
	return nforce2_fsb_read() * fid * 1000;
}

- if there's an user-specified max_fsb which is larger than it was booted
  at, there should be a big warning -- if anyone is overclocking the CPU,
  there might be random crashes etc. These should not need to be debugged by
  kernel developers.

- Speaking of stability: changing the FSB on the fly proved to be difficult
  on other platforms. While I think the method used in nforce2 is less
  error-prone, it might be good to add some big warning to the kernel logs

- Next issue wrt stability: where did you get the information from? While I
  don't really care, if it's from "official" documentation, it's less prone
  to stability errors.

- cpufreq_frequency_table_put_attr(policy->cpu) in nforce2_cpu_exit is
  invalid, as you're not using cpufreq table helpers. Also, the
  corresponding get_attr call is missing, so... Also, this means you don't
  need to have a nforce2_attr or a .attr in struct nforce2_driver.

- please convert MODULE_PARM to module_param

- s/NFORCE2_SAVE_DISTANCE/NFORCE2_SAFE_DISTANCE

- check for !cpu_khz in ->init(). While unlikely, it is possible that
  cpu_khz isn't initialized correctly due to broken TSC etc.

- Implementing .5 FIDs shouldn't be too difficult.
	x change the semantics of FID to represent _ten_ times the FID
	x 
	if (!fid)
		fid = cpu_khz / (fsb * 100);
	if (fid % 5) {
		unsigned int mfid = fid % 5;
		if (mfid > 2)
			fid += 5 - mfid;
		else
			fid -= mfid;
	}


Thanks,
	Dominik

Re: cpufreq driver for nForce2

[Sebastian Witt]

Hi Dominik,

Dominik Brodowski wrote:

> - nforce2_verify should assert that there's at least one valid state in
>   between policy->min and policy->max. If policy->min were 55300 kHz and
>   policy->max 55400 kHz, nforce2_verify wouldn't adapt. Something like
> 	fsb_pol_max = policy->max / (fid * 1000);
> 	if (policy->min < (fsb_pol_max * fid * 1000))
> 		policy->max = (fsb_pol_max + 1) * fid * 1000;
>   Or does the code in nforce2_calc_pll mean that there might be only one
>   "state" available in a 4 MHz FSB interval? If so, the code above needs to
>   be modified.

I will change this.

> - Why is the new PLL value written _forty_ times?

It's written 64 times. The chipset applies the new FSB when the 
'counter' (PLLADR) reaches 0x40.

> - nforce2_target should be ordered a bit differently: calculate
> 	target_fsb = target_freq / (fid * 1000);
>   first, then set
> 	freqs.new = target_fsb * fid * 1000;

Ok, changed.

> - implement a ->get() function, please. It's (probably) only
> static unsigned int nforce2_get(unsigned int cpu)
> {
> 	if (cpu)
> 		return 0;
> 	return nforce2_fsb_read() * fid * 1000;
> }

This also.

> - if there's an user-specified max_fsb which is larger than it was booted
>   at, there should be a big warning -- if anyone is overclocking the CPU,
>   there might be random crashes etc. These should not need to be debugged by
>   kernel developers.

I've added a warning when max_fsb is greater than the current FSB.

> - Speaking of stability: changing the FSB on the fly proved to be difficult
>   on other platforms. While I think the method used in nforce2 is less
>   error-prone, it might be good to add some big warning to the kernel logs

Yes, this is only possible because the PCI/AGP clock is independent from 
the FSB. I added a warning.

> 
> - Next issue wrt stability: where did you get the information from? While I
>   don't really care, if it's from "official" documentation, it's less prone
>   to stability errors.

I don't know if there is any official documentation, it's based on 
8rdavcore (www.hasw.net/8rdavcore) and the FSB changing code is tested 
for several months. If the FSB is changed between a limited range 
(max_fsb = bootfsb and min_fsb = bootfsb - 60 works on most boards), 
there are no problems.

On many boards is a greater range possible, but this depends too much on 
the chipset parameters set by the Bios. But FSB changing was never 
planned and is out of specifications so this is all experimental.

> - cpufreq_frequency_table_put_attr(policy->cpu) in nforce2_cpu_exit is
>   invalid, as you're not using cpufreq table helpers. Also, the
>   corresponding get_attr call is missing, so... Also, this means you don't
>   need to have a nforce2_attr or a .attr in struct nforce2_driver.

You're right, removed it.

> - please convert MODULE_PARM to module_param
> 
> - s/NFORCE2_SAVE_DISTANCE/NFORCE2_SAFE_DISTANCE

> - check for !cpu_khz in ->init(). While unlikely, it is possible that
>   cpu_khz isn't initialized correctly due to broken TSC etc.

Done.

Thanks for your help,
Sebastian

Re: cpufreq driver for nForce2

[Bruno Ducrot]

On Sun, Jun 06, 2004 at 09:34:00PM +0200, Sebastian Witt wrote:
> 
> >- Speaking of stability: changing the FSB on the fly proved to be difficult
> >  on other platforms. While I think the method used in nforce2 is less
> >  error-prone, it might be good to add some big warning to the kernel logs
> 
> Yes, this is only possible because the PCI/AGP clock is independent from 
> the FSB. I added a warning.

Are you sure?  It's very likely PCI and AGP clocks are multiples
of SYSCLK (but I must admit I know nothing about nForce2)?

Cheers,

-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.

Re: cpufreq driver for nForce2

[Sebastian Witt]

Dominik Brodowski wrote:
> Hi Sebastian,
> 
> A few remarks on your driver:
> ...

I've now applied the changes you suggested and some other fixes:

http://www.hasw.net/linux/cpufreq-nforce2-0.2.tar.bz2

Thanks,
Sebastian

Re: cpufreq driver for nForce2

[Dominik Brodowski]

Hi Sebastian,


On Wed, Jun 09, 2004 at 09:50:59PM +0200, Sebastian Witt wrote:
> http://www.hasw.net/linux/cpufreq-nforce2-0.2.tar.bz2

Looks quite good now, just two minor issues:

nforce2_get() is missing in struct cpufreq_driver nforce2_driver

//#include <linux/slab.h>
can be removed, AFAICS.

	Dominik