Skip to contentSkip to conversation

cpufreq driver for nForce2

DISCUSSION Sebastian Witt 3 participants 6 messages Jun 3, 2004 – Jun 10, 2004

lore
Share & tools
lore
#0 lore raw

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

#1 lore raw

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

#2 lore raw

Hi Dominik,

Quoted 9 lines · Dominik Brodowski

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

Quoted 1 line

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

Quoted 4 lines

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

Quoted 7 lines

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

Quoted 4 lines

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

Quoted 3 lines

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

Quoted 4 lines

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

Quoted 4 lines

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

Quoted 6 lines

- 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

#3 lore raw
Quoted 7 lines · Sebastian Witt
Quoted 4 lines

- 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,

Signature · 4 lines
Bruno Ducrot

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