[coreboot] patch for global variables

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Aug 27 23:46:55 CEST 2008


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 <rminnich at gmail.com>
>   

If you fix the stuff mentioned in the review below, this is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>


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 at 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

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list