Re: [coreboot] patch for global variables

[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