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 <[EMAIL PROTECTED]>
>
If you fix the stuff mentioned in the review below, this is
Acked-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]>
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");
> };
> };
> + [EMAIL PROTECTED] {
> + /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/
--
coreboot mailing list
[email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
