[coreboot] [PATCH] VIA CX700 support
Uwe Hermann
uwe at hermann-uwe.de
Thu Apr 16 17:05:07 CEST 2009
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 <uwe at hermann-uwe.de>
> 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
More information about the coreboot
mailing list