Sorry, I lost my comment and signed-off-by in the patch. This should be correct. Marc
Marc Jones wrote:
Subject: Re: [LinuxBIOS] [PATCH] geode_v3_cleanup.patch - Re: r387 - in LinuxBIOSv3/arch/x86: . geodelx From: "Marc Jones" marc.jones@amd.com Date: Tue, 03 Jul 2007 10:21:06 -0600 To: "Uwe Hermann" uwe@hermann-uwe.de
To: "Uwe Hermann" uwe@hermann-uwe.de
Patch updated and comments inline.
Uwe Hermann wrote:
On Mon, Jul 02, 2007 at 05:55:12PM -0600, Marc Jones wrote:
Clean up comments and #defines in Geode LX code.
Signed-off-by: Marc Jones marc.jones@amd.com
Index: LinuxBIOSv3/arch/x86/geodelx/geodelx.c
--- LinuxBIOSv3.orig/arch/x86/geodelx/geodelx.c 2007-07-02 11:23:59.000000000 -0600 +++ LinuxBIOSv3/arch/x86/geodelx/geodelx.c 2007-07-02 16:27:43.000000000 -0600 @@ -40,8 +40,21 @@ */
/** - * start_time1 Starts Timer 1 for port 61 use. FIXME try to figure
- out what these values mean.
- start_time1 Starts Timer 1 for port 61 use.
You can omit the "start_time1" and other function names in Doxygen comments, they're not required at all. Doxygen knows which function you intend to document.
done
- 0x43 is PIT command/control.
- 0x41 is PIT counter 1.
These should have #defines too, I think.
See follow on patch. Requires a change to a core file and should be reviewed separately.
- The command 0x56 means write counter 1 lower 8 bits in next IO,
- set the counter mode to square wave generator (count down to 0
- from programmed value twice in a row, alternating the output
signal)
- counting in 16-bit binary mode.
- 0x12 is counter/timer 1 and signals the PIT to do a RAM refresh
- approximately every 15us written to the counter.
- The PIT typically generating 1.19318 MHz
- Timer 1 was used for RAM refresh on XT/AT and can be read on
port61.
*/
- Port61 is used by many timing loops for calibration.
void start_timer1(void) { @@ -134,42 +147,44 @@ */ void pll_reset(int manualconf, u32 pll_hi, u32 pll_lo) {
- struct msr msrGlcpSysRstpll;
- struct msr msr_glcp_sys_pll;
- msrGlcpSysRstpll = rdmsr(GLCP_SYS_RSTPLL);
msr_glcp_sys_pll = rdmsr(GLCP_SYS_RSTPLL);
printk(BIOS_DEBUG, - "_MSR GLCP_SYS_RSTPLL (%08x) value
is: %08x:%08x\n", msrGlcpSysRstpll.hi, msrGlcpSysRstpll.lo);
"_MSR GLCP_SYS_RSTPLL (%08x) value is: %08x:%08x\n",msr_glcp_sys_pll.hi, msr_glcp_sys_pll.lo); post_code(POST_PLL_INIT);
- if (!(msrGlcpSysRstpll.lo & (1 << RSTPLL_LOWER_SWFLAGS_SHIFT))) {
- if (!(msr_glcp_sys_pll.lo & (1 << RSTPLL_LOWER_SWFLAGS_SHIFT))) { printk(BIOS_DEBUG,"Configuring PLL\n"); if (manualconf) { post_code(POST_PLL_MANUAL); /* CPU and GLIU mult/div (GLMC_CLK = GLIU_CLK / 2) */
msrGlcpSysRstpll.hi = pll_hi;
msr_glcp_sys_pll.hi = pll_hi; /* Hold Count - how long we will sit in reset */
msrGlcpSysRstpll.lo = pll_lo;
msr_glcp_sys_pll.lo = pll_lo; } else { /*automatic configuration (straps) */ post_code(POST_PLL_STRAP);
msrGlcpSysRstpll.lo &=
/* Hold 0xDE*16clocks during reset. */^ missing spacedone
/* AMD recomended value for proper PLL reset.*/msr_glcp_sys_pll.lo &= ~(0xFF << RSTPPL_LOWER_HOLD_COUNT_SHIFT);Please also add the reason why AMD recommends the value (rationale) to the code comment.
I indicated it was the validated value. The rationale has been lost to time.
msrGlcpSysRstpll.lo |=
msr_glcp_sys_pll.lo |= (0xDE << RSTPPL_LOWER_HOLD_COUNT_SHIFT);
msrGlcpSysRstpll.lo &=
msr_glcp_sys_pll.lo &= ~(RSTPPL_LOWER_COREBYPASS_SET | RSTPPL_LOWER_MBBYPASS_SET);
msrGlcpSysRstpll.lo |=
msr_glcp_sys_pll.lo |= RSTPPL_LOWER_COREPD_SET | RSTPPL_LOWER_CLPD_SET; } /* Use SWFLAGS to remember: "we've already been here" */
msrGlcpSysRstpll.lo |= (1 << RSTPLL_LOWER_SWFLAGS_SHIFT);
msr_glcp_sys_pll.lo |= (1 << RSTPLL_LOWER_SWFLAGS_SHIFT); /* "reset the chip" value */
msrGlcpSysRstpll.lo |= RSTPPL_LOWER_CHIP_RESET_SET;wrmsr(GLCP_SYS_RSTPLL, msrGlcpSysRstpll);
msr_glcp_sys_pll.lo |= RSTPPL_LOWER_CHIP_RESET_SET;wrmsr(GLCP_SYS_RSTPLL, msr_glcp_sys_pll); /* You should never get here..... The chip has reset. */ printk(BIOS_EMERG,"CONFIGURING PLL FAILURE -- HALT\n");@@ -183,9 +198,8 @@
/**
- Return the CPU clock rate. Rates in this system are always returned
- as multkiples of 33 Mhz.
- Return the CPU clock rate from the PLL MSR.
- @return CPU speed in MHz
^^no spaces here, please
done
Was the above comment incorrect? Are Mhz returned? Or multiples of 33 MHz?
yes, the MSR holds setting is in multiples of 33MHz. The function returns that actual MHz.
*/ u32 cpu_speed(void) { @@ -193,17 +207,16 @@ struct msr msr;
msr = rdmsr(GLCP_SYS_RSTPLL);
- speed = ((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) & 0x1F) + 1) *
- / 10;
- if ((((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) & 0x1F) + 1) *
- % 10) > 5) {
- speed = ((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) &
RSTPLL_UPPER_CPUMULT_MASK) + 1) * 333) / 10;
- if ((((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) &
RSTPLL_UPPER_CPUMULT_MASK) + 1) * 333) % 10) > 5) { ++speed; } return (speed); }
/**
- Return the Geode Link clock rate. Rates in this system are always
- returned as multkiples of 33 Mhz.
- Return the GeodeLink clock rate from the PLL MSR.
- @return GeodeLink speed in MHz
^^no spaces
(also in some other places)
done
Patch looks good, I'd say we commit the next revision.
Uwe.
Marc
Index: LinuxBIOSv3/arch/x86/geodelx/geodelx.c
--- LinuxBIOSv3.orig/arch/x86/geodelx/geodelx.c 2007-07-02 11:42:59.000000000 -0600 +++ LinuxBIOSv3/arch/x86/geodelx/geodelx.c 2007-07-03 08:53:38.000000000 -0600 @@ -40,8 +40,21 @@ */
/**
- start_time1 Starts Timer 1 for port 61 use. FIXME try to figure
- out what these values mean.
- Starts Timer 1 for port 61 use.
- 0x43 is PIT command/control.
- 0x41 is PIT counter 1.
- The command 0x56 means write counter 1 lower 8 bits in next IO,
- set the counter mode to square wave generator (count down to 0
- from programmed value twice in a row, alternating the output signal)
- counting in 16-bit binary mode.
- 0x12 is counter/timer 1 and signals the PIT to do a RAM refresh
- approximately every 15us written to the counter.
- The PIT typically generating 1.19318 MHz
- Timer 1 was used for RAM refresh on XT/AT and can be read on port61.
*/
- Port61 is used by many timing loops for calibration.
void start_timer1(void) { @@ -134,42 +147,44 @@ */ void pll_reset(int manualconf, u32 pll_hi, u32 pll_lo) {
- struct msr msrGlcpSysRstpll;
- struct msr msr_glcp_sys_pll;
- msrGlcpSysRstpll = rdmsr(GLCP_SYS_RSTPLL);
msr_glcp_sys_pll = rdmsr(GLCP_SYS_RSTPLL);
printk(BIOS_DEBUG,
"_MSR GLCP_SYS_RSTPLL (%08x) value is: %08x:%08x\n", msrGlcpSysRstpll.hi, msrGlcpSysRstpll.lo);
"_MSR GLCP_SYS_RSTPLL (%08x) value is: %08x:%08x\n", msr_glcp_sys_pll.hi, msr_glcp_sys_pll.lo);
- if (!(msrGlcpSysRstpll.lo & (1 << RSTPLL_LOWER_SWFLAGS_SHIFT))) {
- if (!(msr_glcp_sys_pll.lo & (1 << RSTPLL_LOWER_SWFLAGS_SHIFT))) { printk(BIOS_DEBUG,"Configuring PLL\n"); if (manualconf) { post_code(POST_PLL_MANUAL); /* CPU and GLIU mult/div (GLMC_CLK = GLIU_CLK / 2) */
msrGlcpSysRstpll.hi = pll_hi;
msr_glcp_sys_pll.hi = pll_hi; /* Hold Count - how long we will sit in reset */
msrGlcpSysRstpll.lo = pll_lo;
msr_glcp_sys_pll.lo = pll_lo;
msrGlcpSysRstpll.lo &=
/* Hold 0xDE * 16 clocks during reset. *//* AMD recomended value for PLL reset from silicon validation. */msr_glcp_sys_pll.lo &= ~(0xFF << RSTPPL_LOWER_HOLD_COUNT_SHIFT);
msrGlcpSysRstpll.lo |=
msr_glcp_sys_pll.lo |= (0xDE << RSTPPL_LOWER_HOLD_COUNT_SHIFT);
msrGlcpSysRstpll.lo &=
msr_glcp_sys_pll.lo &= ~(RSTPPL_LOWER_COREBYPASS_SET | RSTPPL_LOWER_MBBYPASS_SET);
msrGlcpSysRstpll.lo |=
msr_glcp_sys_pll.lo |= RSTPPL_LOWER_COREPD_SET | RSTPPL_LOWER_CLPD_SET;
msrGlcpSysRstpll.lo |= (1 << RSTPLL_LOWER_SWFLAGS_SHIFT);
msr_glcp_sys_pll.lo |= (1 << RSTPLL_LOWER_SWFLAGS_SHIFT);/* "reset the chip" value */
msrGlcpSysRstpll.lo |= RSTPPL_LOWER_CHIP_RESET_SET;wrmsr(GLCP_SYS_RSTPLL, msrGlcpSysRstpll);
msr_glcp_sys_pll.lo |= RSTPPL_LOWER_CHIP_RESET_SET;wrmsr(GLCP_SYS_RSTPLL, msr_glcp_sys_pll);/* You should never get here..... The chip has reset. */ printk(BIOS_EMERG,"CONFIGURING PLL FAILURE -- HALT\n");
@@ -183,9 +198,8 @@
/**
- Return the CPU clock rate. Rates in this system are always returned
- as multkiples of 33 Mhz.
- Return the CPU clock rate from the PLL MSR.
*/
- @return CPU speed in MHz
u32 cpu_speed(void) { @@ -193,17 +207,16 @@ struct msr msr;
msr = rdmsr(GLCP_SYS_RSTPLL);
- speed = ((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) & 0x1F) + 1) * 333) / 10;
- if ((((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) & 0x1F) + 1) * 333) % 10) > 5) {
- speed = ((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) & RSTPLL_UPPER_CPUMULT_MASK) + 1) * 333) / 10;
- if ((((((msr.hi >> RSTPLL_UPPER_CPUMULT_SHIFT) & RSTPLL_UPPER_CPUMULT_MASK) + 1) * 333) % 10) > 5) { ++speed; } return (speed);
}
/**
- Return the Geode Link clock rate. Rates in this system are always
- returned as multkiples of 33 Mhz.
- Return the GeodeLink clock rate from the PLL MSR.
*/
- @return GeodeLink speed in MHz
u32 geode_link_speed(void) { @@ -211,8 +224,8 @@ struct msr msr;
msr = rdmsr(GLCP_SYS_RSTPLL);
- speed = ((((msr.hi >> RSTPLL_UPPER_GLMULT_SHIFT) & 0x1F) + 1) * 333) / 10;
- if ((((((msr.hi >> RSTPLL_UPPER_GLMULT_SHIFT) & 0x1F) + 1) * 333) % 10) > 5) {
- speed = ((((msr.hi >> RSTPLL_UPPER_GLMULT_SHIFT) & RSTPLL_UPPER_GLMULT_MASK) + 1) * 333) / 10;
- if ((((((msr.hi >> RSTPLL_UPPER_GLMULT_SHIFT) & RSTPLL_UPPER_GLMULT_MASK) + 1) * 333) % 10) > 5) { ++speed; } return (speed);
@@ -220,9 +233,8 @@
/**
- Return the PCI bus clock rate. Rates in this system are always
- returned as multkiples of 33 Mhz.
- Return the PCI bus clock rate from the PLL MSR.
*/
- @return PCI speed in MHz
u32 pci_speed(void) { @@ -295,7 +307,7 @@ */ msrnum = GLCP_DELAY_CONTROLS; msr = rdmsr(msrnum);
- if (msr.lo & ~(0x7C0)) {
- if (msr.lo & ~(DELAY_LOWER_STATUS_MASK)) { return; }
Index: LinuxBIOSv3/arch/x86/geodelx/stage1.c
--- LinuxBIOSv3.orig/arch/x86/geodelx/stage1.c 2007-07-02 11:42:59.000000000 -0600 +++ LinuxBIOSv3/arch/x86/geodelx/stage1.c 2007-07-02 11:43:04.000000000 -0600 @@ -52,20 +52,20 @@
/* Setup access to the cache for under 640K. Note MC not setup yet. */ msr.hi = 0x20000000;
- msr.lo = 0xfff80;
- wrmsr(MSR_GLIU0 + 0x20, msr);
msr.lo = 0x000fff80; /* 0-0x7FFFF */
wrmsr(MSR_GLIU0_BASE1, msr);
msr.hi = 0x20000000;
- msr.lo = 0x80fffe0;
- wrmsr(MSR_GLIU0 + 0x21, msr);
msr.lo = 0x080fffe0; /* 0x80000-0x9FFFF */
wrmsr(MSR_GLIU0_BASE2, msr);
msr.hi = 0x20000000;
- msr.lo = 0xfff80;
- wrmsr(MSR_GLIU1 + 0x20, msr);
msr.lo = 0x000fff80; /* 0-0x7FFFF */
wrmsr(MSR_GLIU1_BASE1, msr);
msr.hi = 0x20000000;
- msr.lo = 0x80fffe0;
- wrmsr(MSR_GLIU1 + 0x21, msr);
- msr.lo = 0x080fffe0; /* 0x80000-0x9FFFF */
- wrmsr(MSR_GLIU0_BASE2, msr);
}
Index: LinuxBIOSv3/include/arch/x86/amd_geodelx.h
--- LinuxBIOSv3.orig/include/arch/x86/amd_geodelx.h 2007-07-02 11:42:59.000000000 -0600 +++ LinuxBIOSv3/include/arch/x86/amd_geodelx.h 2007-07-02 11:43:04.000000000 -0600 @@ -354,10 +354,13 @@ #define GLCP_GLD_MSR_ERROR (MSR_GLCP + 0x2003) #define GLCP_GLD_MSR_PM (MSR_GLCP + 0x2004) #define GLCP_DELAY_CONTROLS (MSR_GLCP + 0x0F) +#define DELAY_LOWER_STATUS_MASK 0x7C0 #define GLCP_SYS_RSTPLL (MSR_GLCP + 0x14) /* R/W */ #define RSTPLL_UPPER_GLMULT_SHIFT 7 +#define RSTPLL_UPPER_GLMULT_MASK 0x1F #define RSTPLL_UPPER_GLDIV_SHIFT 6 #define RSTPLL_UPPER_CPUMULT_SHIFT 1 +#define RSTPLL_UPPER_CPUMULT_MASK 0x1F #define RSTPLL_UPPER_CPUDIV_SHIFT 0 #define RSTPLL_LOWER_SWFLAGS_SHIFT 26 #define RSTPLL_LOWER_SWFLAGS_MASK (0x03F << RSTPLL_LOWER_SWFLAGS_SHIFT)