cpufreq driver for nForce2
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
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
Hi Dominik,
Quoted 9 lines · Dominik Brodowski
-
nforce2_verifyshould assert that there's at least one valid state in betweenpolicy->minandpolicy->max. Ifpolicy->minwere 55300 kHz andpolicy->max55400 kHz,nforce2_verifywouldn't adapt. Something likefsb_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_pllmean 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_targetshould be ordered a bit differently: calculatetarget_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_fsbwhich 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)innforce2_cpu_exitis invalid, as you're not using cpufreq table helpers. Also, the correspondingget_attrcall is missing, so... Also, this means you don't need to have anforce2_attror a .attr in structnforce2_driver.
You're right, removed it.
Quoted 6 lines
- please convert
MODULE_PARMtomodule_param- s/NFORCE2_SAVE_DISTANCE/NFORCE2_SAFE_DISTANCE
- check for !cpu_khz in ->init(). While unlikely, it is possible that
cpu_khzisn't initialized correctly due to broken TSC etc.
Done.
Thanks for your help,
Sebastian
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.
Quoted 4 lines · Dominik Brodowski
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
Hi Sebastian,
Quoted 1 line · Sebastian Witt
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