[coreboot] patch: dbe62 boots to FILO!

Uwe Hermann uwe at hermann-uwe.de
Thu Apr 3 14:09:36 CEST 2008


On Thu, Apr 03, 2008 at 12:26:11PM +0200, Carl-Daniel Hailfinger wrote:
> The patch looks nice, but there are a few small problems.
> Otherwise, the patch looks fine.
> 
> Reworked patch follows. It should be identical from a code point of
> view, but it would be great if you could test anyway.
> Please note that the patch is against HEAD. Same patch is also attached.
> 
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Please keep the Signed-off-by from Ron, i.e. the commit message (and
follow-up patches) should have _both_ Signed-off-by's.


> Index: LinuxBIOSv3-dbe62/southbridge/amd/cs5536/cs5536.c
> ===================================================================
> --- LinuxBIOSv3-dbe62/southbridge/amd/cs5536/cs5536.c	(Revision 647)
> +++ LinuxBIOSv3-dbe62/southbridge/amd/cs5536/cs5536.c	(Arbeitskopie)
> @@ -258,6 +258,7 @@
>  
>  	/* COM1 */
>  	if (sb->com1_enable) {
> +		printk(BIOS_SPEW, "uarts_init: enable com1\n");

com1 -> COM1 maybe.


>  		/* Set the address. */
>  		switch (sb->com1_address) {
>  		case 0x3F8:
> @@ -308,6 +309,7 @@
>  		wrmsr(MDD_UART1_CONF, msr);
>  	} else {
>  		/* Reset and disable COM1. */
> +		printk(BIOS_SPEW, "uarts_init: disable com1\n");

Ditto (and in various other places).


> Index: LinuxBIOSv3-dbe62/mainboard/artecgroup/dbe62/initram.c
> ===================================================================
> --- LinuxBIOSv3-dbe62/mainboard/artecgroup/dbe62/initram.c	(Revision 647)
> +++ LinuxBIOSv3-dbe62/mainboard/artecgroup/dbe62/initram.c	(Arbeitskopie)
> @@ -33,9 +33,9 @@
>  #include <northbridge/amd/geodelx/raminit.h>
>  #include <spd.h>
>  
> -#define MANUALCONF 0		/* Do automatic strapped PLL config */
> -#define PLLMSRHI 0x00001490	/* manual settings for the PLL */
> -#define PLLMSRLO 0x02000030
> +#define MANUALCONF 1		/* Do manual strapped PLL config */
> +#define PLLMSRHI 0x000003d9	/* manual settings for the PLL */
> +#define PLLMSRLO 0x07de0080	/* from fuctory bios */
>  #define DIMM0 ((u8) 0xA0)
>  #define DIMM1 ((u8) 0xA2)
>  
> @@ -50,28 +50,45 @@
>  	u8 address;
>  	u8 data;
>  };
> +/*
> +ok, This is what I came up with. I would be interested in the results.
> +spd : value(hex)
> +4: 8
> +5: 1
> +9: <= 7
> +12: 82
> +17: 4
> +31: 40
> +18: 10
> +23: 0
> +25: 0
> +27: 58
> +28: 3c
> +29: 58
> +30: 2d
> +42:4b
>  
> +I may have missed one so let me know. Also you might find this document helpful.
> +*/

Yep, needs a more verbose comment IMO. Also, please use the standard
commenting style

/*
 * Foo.
 * Bar.
 */

(or Doxygen-style if it should end up in Doxygen's output)


> +	dumplxmsrs();
>  	/* Check low memory */
> -	ram_check(0x00000000, 640*1024);
> +	/* passed. Don't bother any more */

This comment doesn't make much sense.


> +	/* Note that the range 0x87000 will fail; that's the stack! */
> +	/*	ram_check(0x00000000, 640*1024);*/
>  
>  	printk(BIOS_DEBUG, "stage1 returns\n");
>  	return 0;
> Index: LinuxBIOSv3-dbe62/northbridge/amd/geodelx/raminit.c
> ===================================================================
> --- LinuxBIOSv3-dbe62/northbridge/amd/geodelx/raminit.c	(Revision 647)
> +++ LinuxBIOSv3-dbe62/northbridge/amd/geodelx/raminit.c	(Arbeitskopie)
> @@ -35,6 +35,43 @@
>  
>  u8 spd_read_byte(u16 device, u8 address);
>  
> +
> +/** 
> + * Dump key MSR values for ram init. You can call this function and then use it to 

ram -> RAM.


> + * compare to a fuctory bios setting.

bios -> BIOS


> + * @param level printk level
> + */
> +void dumplxmsrs(void)
> +{
> +	static unsigned long msrs[] = {
> +        	MC_CF07_DATA, 
> +        	MC_CF8F_DATA, 
> +        	MC_CF1017_DATA, 
> +        	GLCP_DELAY_CONTROLS, 
> +        	MC_CFCLK_DBUG, 
> +        	MC_CF_PMCTR,
> +		GLCP_SYS_RSTPLL
> +	};
> +	static char *msrnames[] = {

Can be 'const' too, I guess.

(same for the other file)


> +        	"MC_CF07_DATA", 
> +        	"MC_CF8F_DATA", 
> +        	"MC_CF1017_DATA", 
> +        	"GLCP_DELAY_CONTROLS", 
> +        	"MC_CFCLK_DBUG", 
> +        	"MC_CF_PMCTR",
> +		"PLL reg"
> +	};
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(msrs); i++) {
> +		struct msr msr;
> +		msr = rdmsr(msrs[i]);
> +		printk(BIOS_DEBUG, "%s (%lx): %x.%x\n",  msrnames[i], msrs[i],
                                                       ^^
                                                only one space

(same for the other file)


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org




More information about the coreboot mailing list