[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