[LinuxBIOS] [Re: [PATCH] geode_v3_cleanup.patch - Re: r387 - in LinuxBIOSv3/arch/x86: . geodelx]

Marc Jones marc.jones at amd.com
Tue Jul 3 18:48:00 CEST 2007


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 at amd.com>
> Date:
> Tue, 03 Jul 2007 10:21:06 -0600
> To:
> "Uwe Hermann" <uwe at hermann-uwe.de>
> 
> To:
> "Uwe Hermann" <uwe at 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 at 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 space
>>
>>
> done
> 
>>> +            /* 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) * 
>>> 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
>>       ^^
>>    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);
>  	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 * 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;
>  		}
>  		/* 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
>   */
>  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)
> 

-- 
Marc Jones
Senior Software Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: geode_v3_cleanup.patch
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070703/7f510131/attachment.ksh>


More information about the coreboot mailing list