Re: [coreboot] patch: dbe62 boots to FILO!

[Uwe Hermann]

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

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.