lkml.org 
[lkml]   [2003]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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/

\
 
 \ /
  Last update: 2005-03-22 13:48    [W:1.620 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site