Hi, On Fri, Apr 10, 2009 at 04:31:40PM +0200, Stefan Reinauer wrote: > Add VIA CX700 support, plus VIA vt8454c reference board support.
Nice work! Quick review below. With the few issues in the code fixed it's Acked-by: Uwe Hermann <[email protected]> > Index: src/include/pc80/i8259.h > =================================================================== > --- src/include/pc80/i8259.h (revision 4087) > +++ src/include/pc80/i8259.h (working copy) > @@ -1 +1,6 @@ > +#ifndef PC80_I8259_H > +#define PC80_I8259_H > + > void setup_i8259(void); > + > +#endif /* PC80_I8259_H */ Please add the usual license header. The file is trivial, just use (C) coresystems. > Index: src/mainboard/via/vt8454c/failover.c > =================================================================== > --- src/mainboard/via/vt8454c/failover.c (revision 0) > +++ src/mainboard/via/vt8454c/failover.c (revision 0) > @@ -0,0 +1,49 @@ > +#define ASSEMBLY 1 > +#include <arch/io.h> > +#include "arch/romcc_io.h" > +#include "pc80/mc146818rtc_early.c" > + > +static unsigned long main(unsigned long bist) > +{ > + if (do_normal_boot()) { > + goto normal_image; > + } else { > + goto fallback_image; > + } > + > +normal_image: > + asm volatile ("jmp __normal_image": /* outputs */ > + :"a" (bist) /* inputs */ > + : /* clobbers */ > + ); > + > +cpu_reset: > + asm volatile ("jmp __cpu_reset": /* outputs */ > + :"a" (bist) /* inputs */ > + : /* clobbers */ > + ); > + > +fallback_image: > + return bist; > +} This file is equivalent to the global src/arch/i386/lib/failover.c, please use that one (and drop this file here) by using something like depends "$(MAINBOARD)/../../../arch/i386/lib/failover.c ../romcc" action "../romcc -E -O2 -mcpu=p2 --label-prefix=failover -I$(TOP)/src -I. $(CPPFLAGS) $(MAINBOARD)/../../../arch/i386/lib/failover.c -o $@" etc. in the board's Config.lb. > Index: src/mainboard/via/vt8454c/auto.c > =================================================================== > --- src/mainboard/via/vt8454c/auto.c (revision 0) > +++ src/mainboard/via/vt8454c/auto.c (revision 0) > @@ -0,0 +1,134 @@ > +void udelay(unsigned usecs) > +{ > + int i; > + for (i = 0; i < usecs; i++) > + outb(0xff, 0x80); > +} #include "pc80/udelay_io.c" provides this already. > +#include "lib/delay.c" Oh, we should unify delay.c and udelay_io.c at some point btw, but that's for another patch. > Index: src/mainboard/via/vt8454c/dmi.h > =================================================================== > --- src/mainboard/via/vt8454c/dmi.h (revision 0) > +++ src/mainboard/via/vt8454c/dmi.h (revision 0) > @@ -0,0 +1,31 @@ > +#define DMI_TABLE_SIZE 0x55 > + > +static u8 dmi_table[DMI_TABLE_SIZE] = { > + 0x5f, 0x53, 0x4d, 0x5f, 0x2d, 0x1f, 0x02, 0x03, 0x51, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, > + 0x5f, 0x44, 0x4d, 0x49, 0x5f, 0xeb, 0xa8, 0x03, 0xa0, 0xff, 0x0f, 0x00, > 0x01, 0x00, 0x23, 0x00, > + 0x00, 0x14, 0x00, 0x00, 0x01, 0x02, 0x00, 0xe0, 0x03, 0x07, 0x90, 0xde, > 0xcb, 0x7f, 0x00, 0x00, > + 0x00, 0x00, 0x37, 0x01, 0x63, 0x6f, 0x72, 0x65, 0x73, 0x79, 0x73, 0x74, > 0x65, 0x6d, 0x73, 0x20, > + 0x47, 0x6d, 0x62, 0x48, 0x00, 0x32, 0x2e, 0x30, 0x00, 0x30, 0x33, 0x2f, > 0x31, 0x33, 0x2f, 0x32, > + 0x30, 0x30, 0x38, 0x00, 0x00 > +}; Hm, interesting, I guess this is the first board where we use DMI in coreboot? Is this a hard requirement for the board? What is DMI used for here? Not knowing much about DMI myself, what are all those hex numbers? > Index: src/mainboard/via/vt8454c/Config.lb > =================================================================== > --- src/mainboard/via/vt8454c/Config.lb (revision 0) > +++ src/mainboard/via/vt8454c/Config.lb (revision 0) > @@ -0,0 +1,196 @@ > +makerule ./auto.inc > + depends "$(MAINBOARD)/auto.c option_table.h" > + action "$(CC) $(DISTRO_CFLAGS) -I$(TOP)/src -I. $(CPPFLAGS) > $(MAINBOARD)/auto.c -Os -nostdinc -nostdlib -fno-builtin -g -dA -fverbose-asm > -Wall -c -S -o $@" Might need small fixes to make it similar to what Carl-Daniel changed the other Config.lb files to (can be done after the commit though). > +## > +## Build our 16 bit and 32 bit linuxBIOS entry code ^^^^^^^^^ coreboot > +## > +mainboardinit cpu/x86/16bit/entry16.inc > +mainboardinit cpu/x86/32bit/entry32.inc > +ldscript /cpu/x86/16bit/entry16.lds > +ldscript /cpu/x86/32bit/entry32.lds > + > +## > +## Build our reset vector (This is where linuxBIOS is entered) coreboot > + chip drivers/pci/onboard > + device pci 0.0 on end > + register "rom_address" = "0xfff80000" #512k > image Please also add comments for 256K and 1MB images to make it easier for users to switch when using different ROM sizes. > Index: src/mainboard/via/vt8454c/irq_tables.c > =================================================================== > --- src/mainboard/via/vt8454c/irq_tables.c (revision 0) > +++ src/mainboard/via/vt8454c/irq_tables.c (revision 0) > @@ -0,0 +1,65 @@ > +/* > + * Documentation at: http://www.microsoft.com/whdc/archive/pciirq.mspx > + */ Drop this comment please (we did in most other boards). The link is in the wiki, we don't need it in all irq_table.c files. > Index: src/mainboard/via/vt8454c/debug.c > =================================================================== > --- src/mainboard/via/vt8454c/debug.c (revision 0) > +++ src/mainboard/via/vt8454c/debug.c (revision 0) > @@ -0,0 +1,108 @@ This is equivalent to the global src/lib/debug.c, except for the dump_io_resources() function. Please add that one to src/lib/debug.c, use the global file then, and drop this one. Feel free to add the (C) coresystems + GPL2 header to that global debug.c, it doesn't yet have a header. > Index: src/northbridge/via/cx700/cx700_vga.c > =================================================================== > --- src/northbridge/via/cx700/cx700_vga.c (revision 0) > +++ src/northbridge/via/cx700/cx700_vga.c (revision 0) > @@ -0,0 +1,119 @@ > +static void vga_init(device_t dev) > +{ > + u8 reg8; > + > + printk_debug("Initiailizing VGA...\n"); Typo. > Index: src/northbridge/via/cx700/raminit.c > =================================================================== > --- src/northbridge/via/cx700/raminit.c (revision 0) > +++ src/northbridge/via/cx700/raminit.c (revision 0) > @@ -0,0 +1,1480 @@ > +#define GET_SPD(i, val, tmp, reg) > \ > + do{ > \ > + val = 0; > \ > + tmp = 0; > \ > + for(i = 0; i < 2; i++) { > \ > + if(pci_read_config8(PCI_DEV(0, 0, 4), (SCRATCH_REG_BASE > + (i << 1)))) { \ > + tmp = get_spd_data(ctrl, i, reg); > \ > + if(tmp > val) > \ > + val = tmp; > \ > + } > \ > + } > \ > + } while ( 0 ) Hm, this is a bit ugly, why not make it a function instead of a #define? > +// TODO factor out to another file > +static void c7_cpu_setup(const struct mem_controller *ctrl) > +{ > + u8 size, i; > + size = sizeof(Reg_Val) / sizeof(Reg_Val[0]); > + for (i = 0; i < size; i += 2) Please use ARRAY_SIZE here. > + pci_write_config8(HOSTCTRL, Reg_Val[i], Reg_Val[i + 1]); > +} > + > Index: targets/via/vt8454c/Config-abuild.lb > =================================================================== > --- targets/via/vt8454c/Config-abuild.lb (revision 0) > +++ targets/via/vt8454c/Config-abuild.lb (revision 0) > @@ -0,0 +1,24 @@ Add the usual GPL header please. > Index: targets/via/vt8454c/Config.lb > =================================================================== > --- targets/via/vt8454c/Config.lb (revision 0) > +++ targets/via/vt8454c/Config.lb (revision 0) Add the usual GPL header please. Uwe. -- http://www.hermann-uwe.de | http://www.holsham-traders.de http://www.crazy-hacks.org | http://www.unmaintained-free-software.org -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
