patch for global variables

[ron minnich]

Attached. tested on dbe62.

Minor change, just moves global variables from console.h to a new
file, and that new file gets included almost
nowhere (whereas console.h gets included everywhere, which is a big problem).

ron

[Carl-Daniel Hailfinger]

On 27.08.2008 20:53, ron minnich wrote:
> Attached. tested on dbe62.

Review follows.
> Minor change, just moves global variables from console.h to a new
> file, and that new file gets included almost
> nowhere (whereas console.h gets included everywhere, which is a big problem).
>
> We need the sys_info struct in the global variables struct for 
> cache as ram on k8. The sys_info struct is generally very useful
> so it makes sense to start accomodating it.
>
> This patch adds an (empty for now) sys_info struct for geode. 
> It add sys_info to the global variables struct.
>
> It removes global variables from console.h to a new file, 
> globalvars.h. Very little code needs to include this file.
>
> This patch is tested on the dbe62 and simnow with no problems.
>
> Signed-off-by: Ronald G. Minnich

If you fix the stuff mentioned in the review below, this is
Acked-by: Carl-Daniel Hailfinger

You forgot to svn add globalvars.h.
> Index: include/console.h
>
> --- include/console.h	(revision 825)
> +++ include/console.h	(working copy)
> @@ -60,19 +60,6 @@
>  };
>  #endif
>
> -/*
> struct global_vars is managed entirely from C code. Keep in mind that there
> is NO buffer at the end of the struct, so having zero-sized arrays at the
> end or similar stuff for which the compiler can't determine the final size
> will corrupt memory. If you don't try to be clever, everything will be fine.
> */
>
> -struct global_vars {
> -#ifdef CONFIG_CONSOLE_BUFFER
> struct printk_buffer *printk_buffer;
>
> -#endif
> unsigned int loglevel;
>
> -};
>
> int printk(int msg_level, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>  EXPORT_SYMBOL(printk);
>  void banner(int msg_level, const char *msg);
> Index: include/arch/x86/amd_geodelx.h
> ===================================================================
> --- include/arch/x86/amd_geodelx.h	(revision 825)
> +++ include/arch/x86/amd_geodelx.h	(working copy)
> @@ -1258,6 +1258,18 @@
>
> #ifndef __ASSEMBLER__
>
> +/* This is new.
> We're not using it yet on Geode.
> K8 requires it and, for future ports, we are going to require it.
> it's a useful placeholder for platform info that usually ends up
> scattered everywhere. On K8, it is initially stored at the base of stack
> in cache-as-ram and then copied out once ram is started.
> */
>
> +struct sys_info {
> int empty;
>
> +};
>
> /*
> Write to a Virtual Register
> @param class_index The register index
> Index: include/arch/x86/amd/k8/k8.h
>
> --- include/arch/x86/amd/k8/k8.h	(revision 826)
> +++ include/arch/x86/amd/k8/k8.h	(working copy)
> @@ -20,6 +20,8 @@
>
> /* Until we resolve a better way to do this, work around it with a value "too large to fail" */
>
> +#ifndef AMD_K8_H
> +#define AMD_K8_H
>  /* Socket types */
>  #define SOCKET_AM2 0x11
>  #define SOCKET_L1	0x10
> @@ -627,3 +629,5 @@
>  struct hw_mem_hole_info get_hw_mem_hole_info(void);
>
> #endif /* ! ASSEMBLY */
>
> +#endif /* AMD_K8_H */
> Index: mainboard/amd/Kconfig
> ===================================================================
> --- mainboard/amd/Kconfig	(revision 825)
> +++ mainboard/amd/Kconfig	(working copy)
> @@ -53,6 +53,7 @@
>    select CPU_AMD_K8
>    select NORTHBRIDGE_AMD_K8
>    select SOUTHBRIDGE_AMD_AMD8111
> select SUPERIO_WINBOND_W83627HF
> select IOAPIC
> help
> AMD Serengeti
> Index: mainboard/amd/serengeti/mainboard.h
>
> --- mainboard/amd/serengeti/mainboard.h	(revision 826)
> +++ mainboard/amd/serengeti/mainboard.h	(working copy)
> @@ -31,3 +31,5 @@
>  #define HT_CHAIN_END_UNITID_BASE 0x6
>  #define SB_HT_CHAIN_ON_BUS0 2
>  #define SB_HT_CHAIN_UNITID_OFFSET_ONLY 1
> +#define SERIAL_DEV W83627HF_SP1
> +#define SERIAL_IOBASE 0x3f8

Please don't! This will cause a total synchronization headache between
dts and mainboard.h.
> Index: mainboard/amd/serengeti/initram.c
>
> --- mainboard/amd/serengeti/initram.c	(revision 826)
> +++ mainboard/amd/serengeti/initram.c	(working copy)
> @@ -25,6 +25,7 @@
>  #include <types.h>
>  #include <lib.h>
>  #include <console.h>
> +#include <globalvars.h>
>  #include <device/device.h>
>  #include <device/pci.h>
>  #include <string.h>
> @@ -35,6 +36,20 @@
>  #include <mc146818rtc.h>
>  #include <spd.h>
>
> +#define RC0 ((1<<0)<<8)
> +#define RC1 ((1<<1)<<8)
> +#define RC2 ((1<<2)<<8)
> +#define RC3 ((1<<3)<<8)
>
> +#define DIMM0 0x50
> +#define DIMM1 0x51
> +#define DIMM2 0x52
> +#define DIMM3 0x53
> +#define DIMM4 0x54
> +#define DIMM5 0x55
> +#define DIMM6 0x56
> +#define DIMM7 0x57
>
> # warning fix hard_reset
>  void hard_reset(void)
>  {
> @@ -49,17 +64,57 @@
>
> void activate_spd_rom(const struct mem_controller *ctrl)
>  {
> /* nothing to do */
>
> +#define SMBUS_HUB 0x18
> int smbus_write_byte(u16 device, u16 address, u8 val);
>   int ret,i;
>    u16 device=(ctrl->channel0[0])>>8;
>    /* the very first write always get COL_STS=1 and ABRT_STS=1, so try another time*/
>    i=2;
>    do {
>            ret = smbus_write_byte(SMBUS_HUB, 0x01, device);
>    } while ((ret!=0) && (i-->0));
>    smbus_write_byte(SMBUS_HUB, 0x03, 0);
>
> }
>
> +u8 spd_read_byte(u16 device, u8 address)
> +{
> int smbus_read_byte(u16 device, u16 address);
>    return smbus_read_byte(device, address);
>
> +}
>
> /**
> main for initram for the Gigabyte m57sli.
> main for initram for the AMD Serengeti
> */
>
> int main(void)
>  {
> static const u16 spd_addr[] = {
> 	//first node
>                    RC0|DIMM0, RC0|DIMM2, 0, 0,
>                    RC0|DIMM1, RC0|DIMM3, 0, 0,
>
> +#if CONFIG_MAX_PHYSICAL_CPUS > 1
> 	//second node
>                    RC1|DIMM0, RC1|DIMM2, RC1|DIMM4, RC1|DIMM6,
>                    RC1|DIMM1, RC1|DIMM3, RC1|DIMM5, RC1|DIMM7,
>
> +#endif
> +#if CONFIG_MAX_PHYSICAL_CPUS > 2
>                    // third node
>                    RC2|DIMM0, RC2|DIMM2, 0, 0,
>                    RC2|DIMM1, RC2|DIMM3, 0, 0,
>                    // four node
>                    RC3|DIMM0, RC3|DIMM2, RC3|DIMM4, RC3|DIMM6,
>                    RC3|DIMM1, RC3|DIMM3, RC3|DIMM5, RC3|DIMM7,
>
> +#endif
>
> };
>
> struct sys_info *sysinfo;
>    int needs_reset; int i;
>    unsigned bsp_apicid = 0;
>
> printk(BIOS_DEBUG, "Hi there from stage1\n");
>  post_code(POST_START_OF_MAIN);
>
> sysinfo = &(global_vars()->sys_info);
>
> printk(BIOS_DEBUG, "stage1 returns\n");
> return 0;
> Index: mainboard/amd/serengeti/stage1.c
>
> --- mainboard/amd/serengeti/stage1.c	(revision 826)
> +++ mainboard/amd/serengeti/stage1.c	(working copy)
> @@ -21,19 +21,19 @@
>
> #include <mainboard.h>
>  #include <types.h>
> +#include <amd/k8/k8.h>
> +#include <amd/k8/sysconf.h>
>  #include <lib.h>
>  #include <console.h>
>  #include <device/device.h>
>  #include <cpu.h>
> -#include <amd/k8/k8.h>
> -#include <amd/k8/sysconf.h>

I trust you to have good reasons for rearranging the include files.
However, if our include files are indeed order sensitive, we ought to
fix them.
> #include <device/pci.h>
>  #include <string.h>
>  #include <msr.h>
>  #include <io.h>
>  #include <arch/x86/msr.h>
> +#include <superio/winbond/w83627hf/w83627hf.h>
>
> static const struct rmap register_values[] = {
>    /* Careful set limit registers before base registers which contain the enables */
>    /* DRAM Limit i Registers
> @@ -291,6 +291,7 @@
>
> void hardware_stage1(void)
>  {
> void w83627hf_enable_serial(u8 dev, u8 serial, u16 iobase);

That declaration should be in superio/winbond/w83627hf/w83627hf.h,
otherwise there's no reason to include it. Please remove the line above.
> void enumerate_ht_chain(void);
>    int max;
>    printk(BIOS_ERR, "Stage1: enable rom ...\n");
> @@ -299,6 +300,7 @@
>    enumerate_ht_chain();
>    amd8111_enable_rom();
>    printk(BIOS_ERR, "Done.\n");
> w83627hf_enable_serial(0x2e, SERIAL_DEV, SERIAL_IOBASE);
> post_code(POST_START_OF_MAIN);
>
> }
> Index: mainboard/amd/serengeti/dts
> ===================================================================
> --- mainboard/amd/serengeti/dts	(revision 825)
> +++ mainboard/amd/serengeti/dts	(working copy)
> @@ -40,5 +40,9 @@
>    			/config/("southbridge/amd/amd8111/nic.dts");
>    		};
>    	};
> ioport@2e {
> 	/config/("superio/winbond/w83627hf/dts");
> 	com1enable = "1";
> };
> };
>
> };
> Index: lib/console.c
> ===================================================================
> --- lib/console.c	(revision 825)
> +++ lib/console.c	(working copy)
> @@ -1,6 +1,7 @@
>  #include <types.h>
>  #include <cpu.h>
>  #include <console.h>
> +#include <globalvars.h>
>  #include <uart8250.h>
>  #include <stdarg.h>
>  #include <string.h>
> Index: northbridge/amd/k8/dqs.c
> ===================================================================
> --- northbridge/amd/k8/dqs.c	(revision 826)
> +++ northbridge/amd/k8/dqs.c	(working copy)
> @@ -27,12 +27,12 @@
>  #include <spd.h>
>  #include <cpu.h>
>  #include <msr.h>
> -#include <amd/k8/k8.h>
> -#include <amd/k8/sysconf.h>
>  #include <device/pci.h>
>  #include <pci_ops.h>
>  #include <mc146818rtc.h>
>  #include <lib.h>
> +#include <amd/k8/k8.h>
> +#include <amd/k8/sysconf.h>

Same comment about include order.
> #include <spd_ddr2.h>
>  /*
> Index: arch/x86/stage1.c
> ===================================================================
> --- arch/x86/stage1.c	(revision 825)
> +++ arch/x86/stage1.c	(working copy)
> @@ -21,6 +21,7 @@
>  #include <types.h>
>  #include <io.h>
>  #include <console.h>
> +#include <globalvars.h>
>  #include <lar.h>
>  #include <string.h>
>  #include <tables.h>

Regards,
Carl-Daniel

[ron minnich]

I responded as best I could.

On Wed, Aug 27, 2008 at 2:46 PM, Carl-Daniel Hailfinger
 wrote:
> > Index: mainboard/amd/serengeti/mainboard.h
> >
> > --- mainboard/amd/serengeti/mainboard.h       (revision 826)
> > +++ mainboard/amd/serengeti/mainboard.h       (working copy)
> > @@ -31,3 +31,5 @@
> >  #define HT_CHAIN_END_UNITID_BASE 0x6
> >  #define SB_HT_CHAIN_ON_BUS0 2
> >  #define SB_HT_CHAIN_UNITID_OFFSET_ONLY 1
> > +#define SERIAL_DEV W83627HF_SP1
> > +#define SERIAL_IOBASE 0x3f8

I moved these to the stage1.c as they are clearly stage1 things, not
mainboard.h things.

Good catch.
> I trust you to have good reasons for rearranging the include files.
> However, if our include files are indeed order sensitive, we ought to
> fix them.

I undid this change, sorry for it.
> > void hardware_stage1(void)
> >  {
> > void w83627hf_enable_serial(u8 dev, u8 serial, u16 iobase);
>
> That declaration should be in superio/winbond/w83627hf/w83627hf.h,
> otherwise there's no reason to include it. Please remove the line above.

It's a stage1 thing. Not removed it, corrections welcome.
> Same comment about include order.

damn, I just realized I missed this one. We can fix it.

I would actually like to have mainboard.h include all things known to
be needed for the mainboard, it would reduce all this #include
silliness.

ron

[Carl-Daniel Hailfinger]

On 28.08.2008 00:48, ron minnich wrote:
> I responded as best I could.

The checkin you did in r828 slightly differs from the patch I acked. The
checkin has a very subtle stack corruption bug on Geode and generic
i586, maybe also on K8 (I need to recheck that).

Problems listed below in an excerpt of the unreviewed part of r828.
> Modified: coreboot-v3/arch/x86/stage1.c
>
> --- coreboot-v3/arch/x86/stage1.c	2008-08-27 22:29:38 UTC (rev 827)
> +++ coreboot-v3/arch/x86/stage1.c	2008-08-27 22:43:18 UTC (rev 828)
> @@ -138,12 +139,16 @@
>  }
>  #endif /* CONFIG_PAYLOAD_ELF_LOADER */
>
> -/*
> +/**
> This function is called from assembler code with its argument on the
> stack. Force the compiler to generate always correct code for this case.
> We have cache as ram running and can start executing code in C.
> @param bist Built In Self Test value
> @param init_detected This (optionally set) value is used on some platforms (e.g. k8) to indicate
> that we are restarting after some sort of reconfiguration. Note that we could use it on geode but
> do not at present.
> */
>
> -void __attribute__((stdcall)) stage1_main(u32 bist)
> +void __attribute__((stdcall)) stage1_main(u32 bist, u32 init_detected)

You changed the calling convention of stage1_main (one additional
parameter) which means that accesses to init_detected on Geode and
generic i586 trigger a stack underflow because their stage0 asm has not
been adjusted. Very nasty.
> {
>    struct global_vars globvars;
>    int ret;
> @@ -178,6 +183,8 @@
>     * NEVER run this on an AP!
>     */
>    global_vars_init(&globvars);
> globvars.bist = bist;
> globvars.init_detected = init_detected;

And that one will break once the code is used in MP environments. Global
variables may only be changed via accessor functions. global_vars_init()
is the place to do that.

Thank you for pointing this problem out in practical use. Improved
documentation has been committed in r831.
> hardware_stage1();
> On Wed, Aug 27, 2008 at 2:46 PM, Carl-Daniel Hailfinger
>  wrote:
> > > Index: mainboard/amd/serengeti/mainboard.h
> > >
> > > --- mainboard/amd/serengeti/mainboard.h       (revision 826)
> > > +++ mainboard/amd/serengeti/mainboard.h       (working copy)
> > > @@ -31,3 +31,5 @@
> > >  #define HT_CHAIN_END_UNITID_BASE 0x6
> > >  #define SB_HT_CHAIN_ON_BUS0 2
> > >  #define SB_HT_CHAIN_UNITID_OFFSET_ONLY 1
> > > +#define SERIAL_DEV W83627HF_SP1
> > > +#define SERIAL_IOBASE 0x3f8
>
> I moved these to the stage1.c as they are clearly stage1 things, not
> mainboard.h things.
>
> Good catch.

The duplication remains a problem, but it's not your fault. We need a
dtc output mode which emits only #defines.
> > I trust you to have good reasons for rearranging the include files.
> > However, if our include files are indeed order sensitive, we ought to
> > fix them.
>
> I undid this change, sorry for it.

No problem.
> > > void hardware_stage1(void)
> > >  {
> > > void w83627hf_enable_serial(u8 dev, u8 serial, u16 iobase);
> >
> > That declaration should be in superio/winbond/w83627hf/w83627hf.h,
> > otherwise there's no reason to include it. Please remove the line above.
>
> It's a stage1 thing. Not removed it, corrections welcome.

I have to go over all that code anyway some time in the future, right
after we are able to create stage1 #defines from the DTS.
> > Same comment about include order.
>
> damn, I just realized I missed this one. We can fix it.
>
> I would actually like to have mainboard.h include all things known to
> be needed for the mainboard, it would reduce all this #include
> silliness.

Hm. I'm not sure whether that will lead to another #include hell.

Regards,
Carl-Daniel

[ron minnich]

Sorry I messed it up, if you can hit me with a patch for my broken
patch I'll ack it.

ron

[PATCH] v3: Handle processor BIST in a sensible way

[Carl-Daniel Hailfinger]

On 28.08.2008 01:48, ron minnich wrote:
> Sorry I messed it up, if you can hit me with a patch for my broken
> patch I'll ack it.

(Patch is also attached.)

Hardest part first:
BIST handling. Unless I'm mistaken, we already die() in stage1_main() if
processor BIST is nonzero. Checking it in initram makes no sense. Having
it as global variable is unnecessary as well. Link BIST is an entirely
different animal.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-bist_sanitization/include/globalvars.h
===================================================================
--- corebootv3-bist_sanitization/include/globalvars.h	(Revision 833)
+++ corebootv3-bist_sanitization/include/globalvars.h	(Arbeitskopie)
@@ -47,7 +47,6 @@
 #endif
    unsigned int loglevel;
    /* these two values are of interest in many stages */
-	u32 bist;
    u32 init_detected;
    struct sys_info sys_info;
 };
Index: corebootv3-bist_sanitization/mainboard/amd/serengeti/initram.c
===================================================================
--- corebootv3-bist_sanitization/mainboard/amd/serengeti/initram.c	(Revision 833)
+++ corebootv3-bist_sanitization/mainboard/amd/serengeti/initram.c	(Arbeitskopie)
@@ -86,12 +86,11 @@

/** 
   * main for initram for the AMD Serengeti
- * @param bist Built In Self Test, which is used to indicate status of self test
  * @param init_detected Used to indicate that we have been started via init
  * @returns 0 on success
  * The purpose of this code is to not only get ram going, but get any other cpus/cores going. 
  * The two activities are very tightly connected and not really seperable. 
- * The BSP (boot strap processor? ) Core 0 is responsible for all training or all sockets. Note that
+ * The BSP (boot strap processor) Core 0 (BSC) is responsible for all training or all sockets. Note that
  * this need not be socket 0; one great strength of coreboot, as opposed to other BIOSes, is that it could
  * always boot with with a CPU in any socket, and even with empty sockets (as opposed to, e.g., the BIOS
  * that came installed on the Sun Ultra 40, which would freeze if one CPU were not installed).
@@ -100,9 +99,6 @@
  * 
   */
 /* 
- * bist is defined by the CPU hardware and is present in EAX on first instruction of coreboot. 
- * Its value is implementation defined. 
- * 
  * init_detected is used to determine if we did a soft reset as required by a reprogramming of the 
  * hypertransport links. If we did this kind of reset, bit 11 will be set in the MTRRdefType_MSR MSR. 
  * That may seem crazy, but there are not lots of places to hide a bit when the CPU does a reset. 
@@ -111,7 +107,7 @@
 int main(void)
 {
    void enable_smbus(void);
-	u32 bist, u32 init_detected;
+	u32 init_detected;
    static const u16 spd_addr[] = {
    		//first node
                         RC0|DIMM0, RC0|DIMM2, 0, 0,
@@ -139,17 +135,11 @@
    post_code(POST_START_OF_MAIN);
    sysinfo = &(global_vars()->sys_info);

-	bist = sysinfo->bist;
    init_detected = sysinfo->init_detected;
-	/* We start out by looking at bist. Where was bist set? */
    /* well, here we are. For starters, we need to know if this is cpu0 core0. 
     * cpu0 core 0 will do all the DRAM setup. 
     */
-	if (bist) {
-		printk(BIOS_EMERG, "Bist 0x%x\n", bist);
-		die("bist failure");
-	} else
-		bsp_apicid = init_cpus(init_detected, sysinfo);
+	bsp_apicid = init_cpus(init_detected, sysinfo);

//	dump_mem(DCACHE_RAM_BASE+DCACHE_RAM_SIZE-0x200, DCACHE_RAM_BASE+DCACHE_RAM_SIZE);

Index: corebootv3-bist_sanitization/arch/x86/stage1.c
===================================================================
--- corebootv3-bist_sanitization/arch/x86/stage1.c	(Revision 833)
+++ corebootv3-bist_sanitization/arch/x86/stage1.c	(Arbeitskopie)
@@ -143,7 +143,9 @@
  * This function is called from assembler code with its argument on the
  * stack. Force the compiler to generate always correct code for this case.
  * We have cache as ram running and can start executing code in C.
- * @param bist Built In Self Test value
+ * @param bist Built In Self Test, which is used to indicate status of self test.
+ * bist is defined by the CPU hardware and is present in EAX on first instruction of coreboot. 
+ * Its value is implementation defined.
  * @param init_detected This (optionally set) value is used on some platforms (e.g. k8) to indicate
  * that we are restarting after some sort of reconfiguration. Note that we could use it on geode but 
  * do not at present. 
@@ -174,6 +176,7 @@

// before we do anything, we want to stop if we dont run
    // on the bootstrap processor.
+#warning We do not want to check BIST here, we want to check whether we are BSC!
    if (bist==0) {
    	// stop secondaries
    	stop_ap();
@@ -183,7 +186,6 @@
     * NEVER run this on an AP!
     */
    global_vars_init(&globvars);
-	globvars.bist = bist;
    globvars.init_detected = init_detected;

hardware_stage1();

[PATCH] v3: Handle processor BIST in a sensible way

[ron minnich]

Acked-by: Ronald G. Minnich

Thanks!

ron

[PATCH] v3: Handle processor BIST in a sensible way

[Carl-Daniel Hailfinger]

On 28.08.2008 03:52, ron minnich wrote:
> Acked-by: Ronald G. Minnich

Thanks for the speedy review. Committed in r834.
> Thanks!

You're welcome. We will make v3 absolutely awesome. :-)

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 28.08.2008 01:48, ron minnich wrote:
> [...] if you can hit me with a patch [...]

We need struct sys_info defined somewhere for qemu. Can you create a
small header file for that purpose or find an existing one where we can
stick it in?
I'm not sure about the name of that header file. Qemu compilation is
broken right now and such a header file would hopefully fix it.

The sys_info accessor patch in my development queue depends on this.

Regards,
Carl-Daniel

[ron minnich]

well you can try this ...

ron

[Carl-Daniel Hailfinger]

On 28.08.2008 03:59, ron minnich wrote:
> well you can try this ...
>
> Patch to support sys_info for qemu.

Looks fine except for one comment which I already changed in the file
you are basing this code on.
Thanks for coming up with this patch so fast.
> Signed-off-by: Ronald G. Minnich

Acked-by: Carl-Daniel Hailfinger
> Index: include/globalvars.h
>
> --- include/globalvars.h	(revision 828)
> +++ include/globalvars.h	(working copy)
> @@ -31,6 +31,10 @@
>  #include <amd_geodelx.h>
>  #endif
>
> +#ifdef CONFIG_BOARD_EMULATION_QEMU_X86
> +#include <qemu.h>
> +#endif
>
> /*
> struct global_vars is managed entirely from C code. Keep in mind that there
> is NO buffer at the end of the struct, so having zero-sized arrays at the
> Index: include/arch/x86/qemu.h
>
> --- include/arch/x86/qemu.h	(revision 0)
> +++ include/arch/x86/qemu.h	(revision 0)
> @@ -0,0 +1,40 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Ronald G. Minnich
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#ifndef CPU_QEMU_H
> +#define CPU_QEMU_H
>
> +#ifndef __ASSEMBLER__
>
> +/* This is new.
> We're not using it yet.
> K8 requires it and, for future ports, we are going to require it.
> it's a useful placeholder for platform info that usually ends up
> scattered everywhere. On K8, it is initially stored at the base of stack
> in cache-as-ram and then copied out once ram is started.

Replace the two lines above with the text below, please.

* scattered everywhere. On K8, it is stored near the base of stack
 * in the global variable struct.
> */
>
> +struct sys_info {
> int empty;
>
> +};
>
> +#endif
>
> +#endif

Regards,
Carl-Daniel

[ron minnich]

Committed revision 835.

with your requested change.

thanks again

ron

[PATCH] v3: fix stack underflow for i586 and GeodeLX

[Carl-Daniel Hailfinger]

stage1_main() calling conventions changed to use two parameters instead
of one. This resulted in a stack underflow for i586 and GeodeLX because
the stack was set up for only one parameter in their stage0 asm code.

Untested. Note that this didn't trigger on K8 because the stack setup
there has excessive room, something that should be fixed as well.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-stage1_init_detected/arch/x86/geodelx/stage0.S
===================================================================
--- corebootv3-stage1_init_detected/arch/x86/geodelx/stage0.S	(Revision 832)
+++ corebootv3-stage1_init_detected/arch/x86/geodelx/stage0.S	(Arbeitskopie)
@@ -365,6 +365,9 @@
    movl    $0, %eax
    pushl   %eax

+	/* Store zero for the unused init_detected parameter. */
+	pushl   %eax
+
    /* Restore the BIST result. */
    movl	%ebp, %eax

Index: corebootv3-stage1_init_detected/arch/x86/stage0_i586.S
===================================================================
--- corebootv3-stage1_init_detected/arch/x86/stage0_i586.S	(Revision 832)
+++ corebootv3-stage1_init_detected/arch/x86/stage0_i586.S	(Arbeitskopie)
@@ -439,6 +439,9 @@
    movl    $0, %eax
    pushl   %eax

+	/* Store zero for the unused init_detected parameter. */
+	pushl   %eax
+
    /* Restore the BIST result */
    movl	%ebp, %eax
    /* We need to set ebp ? No need */

[PATCH] v3: fix stack underflow for i586 and GeodeLX

[ron minnich]

Acked-by: Ronald G. Minnich

[PATCH] v3: fix stack underflow for i586 and GeodeLX

[Carl-Daniel Hailfinger]

On 28.08.2008 04:01, ron minnich wrote:
> Acked-by: Ronald G. Minnich

Forgive me, but I want to retract that patch. While it is correct, it is
a band-aid instead of a real fix.
Can't we determine init_detected from C code and avoid working with it
in asm?
The Fam10h BKDG states in section 2.3.1:
"determine the type of this reset. One method is to use [The Link
Initialization Control Register] F0x6C[InitDet] bit."
The same register is suggested by the Athlon64/Operon BKDG and the
Fam0fh BKDG.

That should work from C and make our lives easier. To be honest, I have
no idea why we use bit 11 of MTRRdefType_MSR for the init_detected purpose.

Regards,
Carl-Daniel

[PATCH] v3: fix stack underflow for i586 and GeodeLX

[ron minnich]

I plan to change that in future. For now, I want to change as little
as I can, since this code is so mysterious anyway. I would say let's
leave it in with a warning of some sort.

ron

[PATCH] v3: fix stack underflow for i586 and GeodeLX

[Carl-Daniel Hailfinger]

On 28.08.2008 04:28, ron minnich wrote:
> I plan to change that in future. For now, I want to change as little
> as I can, since this code is so mysterious anyway. I would say let's
> leave it in with a warning of some sort.

Thanks, r838 with this discussion in the commit message.

Regards,
Carl-Daniel