Re: [coreboot] [PATCH] VIA CX700 support

[Uwe Hermann]

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
> 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.