Messages in this thread | | | Date | Fri, 22 Aug 2003 15:43:40 +0100 | From | Dave Jones <> | Subject | Re: Cpufreq for opteron |
| |
On Fri, Aug 22, 2003 at 03:59:46PM +0200, Pavel Machek wrote:
> + * > + * Based on the powernow-k7.c module written by Dave Jones. > + * (C) 2003 Dave Jones <davej@suse.de>
Please change this to "(C) 2003 Dave Jones <davej@codemonkey.org.uk> on behalf of SuSE Labs." Whilst that email address is dead, it was SuSE who sponsered me to do that work, so they should retain some credit.
> +#include <linux/cpufreq.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include <linux/cpufreq.h>
cpufreq.h included twice.
> +#undef DEBUG > +#undef TRACE > + > +#define PFX "amd64-cpuf: "
Precedent on other cpufreq modules has been to use module name as PFX. (Ie, "powernow-k8:")
> +#define VERSION "version 1.00.06 - August 13, 2003"
minor nit: pre-driver VERSION strings go stale quickly once merged to the kernel and people start adding small changes bypassing maintainer..
> +#ifdef CONFIG_SMP > +#error cpufreq support is disabled for config_smp > +#endif
Why ? The core itself should be safe now, if you know of problems, I'd like to know about them. It's also extraneous given that you do a runtime test for >1 CPU in check_supported_cpu() and abort in that case.
> +#ifdef CONFIG_PREEMPT > +#warning noone tested this on preempt system, beware > +#endif
This is likely to be lots of 'fun'. The multiple stage state machine that the opteron powernow uses could be preempted at any stage. Might not be that big a deal for UP (except for any timing specific routines, that need explicit disable/enable around them). But for SMP, where you could wind up on a different CPU when you return to kernel space, 'bad shit' will happen. Good luck!
> +#ifdef LOG_CHANGES > +static char *vid_name(u32 vid); > +static char *fid_name(u32 fid); > +#endif > + > +static u32 convert_fid_to_vco_fid(u32 fid); > +static inline u32 find_freq_from_fid(u32 fid); > +static inline u32 find_fid_from_freq(u32 freq); > +static u32 find_closest_fid(u32 freq, int searchup); > +static inline void sort_pst(struct pst_s *ppst, u32 numpstates); > +static inline void count_off_vst(void); > +static inline void count_off_irt(void); > +static inline int transition_frequency(u32 * preq, u32 * pmin, u32 * pmax, > + u32 searchup); > +static int find_match(u32 * ptargfreq, u32 * pmin, u32 * pmax, int searchup, > + u32 * pfid, u32 * pvid); > +static inline int transition_fid_vid(u32 reqfid, u32 reqvid); > +static int decrease_vid_code_by_step(u32 reqvid, u32 step); > +static int write_new_fid(u32 fid); > +static int write_new_vid(u32 vid); > +static inline int pending_bit_stuck(void); > +static int query_current_values_with_pending_wait(void); > +static inline int core_voltage_pre_transition(u32 reqvid); > +static inline int core_voltage_post_transition(u32 reqvid); > +static inline int core_frequency_transition(u32 reqfid); > +static inline int find_psb_table(void); > +static inline int check_supported_cpu(void); > +static int drv_verify(struct cpufreq_policy *pol); > +static int drv_target(struct cpufreq_policy *pol, unsigned targfreq, > + unsigned relation); > +static int __init drv_cpu_init(struct cpufreq_policy *pol); > +static void __exit drv_exit(void);
Dump this lot in powernow-k8.h please, or better yet, remove them where possible.
> +static int onbattery = 1; /* Set if running on battery, reset otherwise. */ > + /* Of no relevance unless batterypstates < */ > + /* numpstates, as defined in the PSB/PST. */
Where is this set ? My guess is you're going to need ACPI hooks to do this, in which case it shouldn't be static.
> +#define SEARCH_UP 1 > +#define SEARCH_DOWN 0 > + > +#ifdef LOG_CHANGES > +static char *pVIDs[] = { > + "1.550V", "1.525V", "1.500V", > + "1.475V", "1.450V", "1.425V", "1.400V", > + "1.375V", "1.350V", "1.325V", "1.300V", > + "1.275V", "1.250V", "1.225V", "1.200V", > + "1.175V", "1.150V", "1.125V", "1.100V", > + "1.075V", "1.050V", "1.025V", "1.000V", > + "0.975V", "0.950V", "0.925V", "0.900V", > + "0.875V", "0.850V", "0.825V", "0.800V", > + "off", "error - impossible vid" > +};
Passing these around as strings seems odd. See what powernow-k7 did. You have to do all the extra division & multiplying, but it's less icky IMO.
> +/* Sort the fid/vid frequency table into ascending order by fid. The spec */ > +/* implies that it will be sorted by BIOS, but, it only implies it, and I */ > +/* prefer not to trust when I can check. */ > +/* Yes, it is a simple bubble sort, but the PST is really small, so the */ > +/* choice of algorithm is pretty irrelevant. */ > +static inline void > +sort_pst(struct pst_s *ppst, u32 numpstates) > +{ > + u32 i; > + u8 tempfid; > + u8 tempvid; > + int swaps = 1; > + > + while (swaps) { > + swaps = 0; > + for (i = 0; i < (numpstates - 1); i++) { > + if (ppst[i].fid > ppst[i + 1].fid) { > + swaps = 1; > + tempfid = ppst[i].fid; > + tempvid = ppst[i].vid; > + ppst[i].fid = ppst[i + 1].fid; > + ppst[i].vid = ppst[i + 1].vid; > + ppst[i + 1].fid = tempfid; > + ppst[i + 1].vid = tempvid; > + } > + } > + } > + > + return; > +}
Remind me why it needs to be sorted at all ? We just walk the table linearly looking for a specific frequency iirc.
> +#ifdef DEBUG > ... deletia > +#endif
There's a *lot* of this in this driver. Does it really need that much debugging info ? Additionally, the combination of dprintk, tprintk, printk (KERN_DEBUG is really messy, and kind of defeats the point of having these macros. If they're not going to be consistent, don't use them at all.
> + /* WARNING - the cpufreq calls end up doing nothing in a SMP kernel. */ > + /* This code will not work too well in such a kernel. This module protects */ > + /* itself from being compiled ifdef CONFIG_SMP. */
Again, why ? Have you actually tried this ? If you have any ideas whats wrong here, we'd like to get this fixed up.
> +#ifdef LOG_CHANGES > + printk(KERN_INFO PFX > + "cpu_init done, current fid 0x%x (%s), vid 0x%x (%s)\n", currfid, > + fid_name(currfid), currvid, vid_name(currvid)); > +#else > + printk(KERN_INFO PFX "cpu_init done, current fid 0x%x, vid 0x%x\n", > + currfid, currvid); > +#endif > + return 0; > +}
Just do the string and lose the ifdef. Actually, if you change it not to pass strings around, but integers, you get this for free without needing any 'fid_name()' routines and such.
> +/* Model Specific Registers for p-state transitions. MSRs are 64-bit. For */ > +/* writes (wrmsr - opcode 0f 30), the register number is placed in ecx, and */ > +/* the value to write is placed in edx:eax. For reads (rdmsr - opcode 0f 32), */ > +/* the register number is placed in ecx, and the data is returned in edx:eax. */ > + > +#define MSR_FIDVID_CTL 0xc0010041 > +#define MSR_FIDVID_STATUS 0xc0010042 > + > +/* Field definitions within the FID VID Low Control MSR : */ > +#define MSR_C_LO_INIT_FID_VID 0x00010000 > +#define MSR_C_LO_NEW_VID 0x00001f00 > +#define MSR_C_LO_NEW_FID 0x0000002f > +#define MSR_C_LO_VID_SHIFT 8 > + > +/* Field definitions within the FID VID High Control MSR : */ > +#define MSR_C_HI_STP_GNT_TO 0x000fffff > + > +/* Field definitions within the FID VID Low Status MSR : */ > +#define MSR_S_LO_CHANGE_PENDING 0x80000000 /* cleared when completed */ > +#define MSR_S_LO_MAX_RAMP_VID 0x1f000000 > +#define MSR_S_LO_MAX_FID 0x003f0000 > +#define MSR_S_LO_START_FID 0x00003f00 > +#define MSR_S_LO_CURRENT_FID 0x0000003f > + > +/* Field definitions within the FID VID High Status MSR : */ > +#define MSR_S_HI_MAX_WORKING_VID 0x001f0000 > +#define MSR_S_HI_START_VID 0x00001f00 > +#define MSR_S_HI_CURRENT_VID 0x0000001f
Are you intending to use the i386 driver on x86-64 using the symlink trick Andi did for some other parts of arch/i386/kernel/ ? If not, move this lot to include/asm-[i386/x86_64]/msr.h If so, it can stay to reduce duplication.
> +#ifdef LOG_CHANGES > +#define lprintk(msg...) printk(msg) > +#else > +#define lprintk(msg...) do { } while(0) > +#endif > + > +#ifdef DEBUG > +#define dprintk(msg...) printk(msg) > +#else > +#define dprintk(msg...) do { } while(0) > +#endif > + > +#ifdef TRACE > +#define tprintk(msg...) printk(msg) > +#else > +#define tprintk(msg...) do { } while(0) > +#endif
See above, this lot seems to be adding more clutter than anything. The fact most of users are already inside ifdef blocks themselves doesn't help.
> +/* Attempt to force the BIOS to enable power management in the chipset if > + * it has not already done so. At least 1 BIOS is delaying the enablement > + * until ACPI init, which never happens without an ACPI enabled Linux > + * kernel. This can be regarded as a BIOS bug, but that is of little help > + * when you are facing the situation. Do not enable this code unless you > + * are sure as to what you are doing. */ > +#ifdef CHIPSET_HACK > +#define chipset_force_pm() do { \ > + unsigned val; \ > + unsigned port = 0x1010; \ > + val = inw( port ); \ > + printk( KERN_INFO PFX "power management enable port 0x%x = 0x%x\n", port, val ); \ > + val |= 0x300; \ > + outw( val, port ); \ > + udelay( 200 ); \ > + val = inw( port ); \ > + printk( KERN_INFO PFX "port 0x%x modified to 0x%x\n", port, val ); \ > + } while(0) > +#else > +#define chipset_force_pm() do { } while(0) > +#endif > + > +#ifdef TRACE > +#define chipset_force_pm() do { \ > + unsigned val; \ > + unsigned port = 0x1010; \ > + val = inw( port ); \ > + printk( KERN_INFO PFX "power management enable port 0x%x = 0x%x\n", port, val ); \ > + } while(0) > +#else > +#define chipset_force_pm() do { } while(0) > +#endif
This is gross. Get the DMI strings of the offending BIOS and add a quirk that cpufreq can fix up when it starts. No more ifdefs, and it'll also do the right thing on boxes with and without the bug.
Summary: Mostly minor nits, but it does need some work to make it look like a kernel driver.
Dave
-- Dave Jones http://www.codemonkey.org.uk - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |