Re: [coreboot] [v3] VT8237R support

[Carl-Daniel Hailfinger]

Hi Corey,

this seems to have slipped through the cracks.

On 24.03.2008 07:08, Corey Osgood wrote:
> Add preliminary, as yet untested support for the vt8237r, based on the code in 
> coreboot v2. This will need some fixes, but at the very least it compiles and 
> should do smbus operations.
>
> Signed-off-by: Corey Osgood

Can you repost an updated patch? V3 has changed quite a bit in the last
few months, especially considering PCI access functions and other stuff.

Review follows:
> Index: southbridge/via/vt8237r/vt8237r.c
>
> --- southbridge/via/vt8237r/vt8237r.c	(revision 0)
> +++ southbridge/via/vt8237r/vt8237r.c	(revision 0)
> @@ -0,0 +1,465 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2007 Rudolf Marek
> Copyright (C) 2008 Corey Osgood
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <types.h>
> +#include <console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <mc146818rtc.h>
> +#include <string.h>
> +#include <keyboard.h>
> +#include <io.h>
> +#include <legacy.h>
> +#include <statictree.h>
> +#include "vt8237r.h"
>
> +#define ALL		(0xff << 24)
> +#define NONE		(0)
> +#define DISABLED	(1 << 16)
> +#define ENABLED		(0 << 16)
> +#define TRIGGER_EDGE	(0 << 15)
> +#define TRIGGER_LEVEL	(1 << 15)
> +#define POLARITY_HIGH	(0 << 13)
> +#define POLARITY_LOW	(1 << 13)
> +#define PHYSICAL_DEST	(0 << 11)
> +#define LOGICAL_DEST	(1 << 11)
> +#define ExtINT		(7 << 8)
> +#define NMI		(4 << 8)
> +#define SMI		(2 << 8)
> +#define INT		(1 << 8)
>
> +static struct ioapicreg {
> u32 reg;
> u32 value_low;
> u32 value_high;
>
> +} ioapic_table[] = {
> /* IO-APIC virtual wire mode configuration. */
> /* mask, trigger, polarity, destination, delivery, vector */
> {0,  ENABLED | TRIGGER_EDGE | POLARITY_HIGH | PHYSICAL_DEST |
>     ExtINT, NONE},
> {1,  DISABLED, NONE},
> {2,  DISABLED, NONE},
> {3,  DISABLED, NONE},
> {4,  DISABLED, NONE},
> {5,  DISABLED, NONE},
> {6,  DISABLED, NONE},
> {7,  DISABLED, NONE},
> {8,  DISABLED, NONE},
> {9,  DISABLED, NONE},
> {10, DISABLED, NONE},
> {11, DISABLED, NONE},
> {12, DISABLED, NONE},
> {13, DISABLED, NONE},
> {14, DISABLED, NONE},
> {15, DISABLED, NONE},
> {16, DISABLED, NONE},
> {17, DISABLED, NONE},
> {18, DISABLED, NONE},
> {19, DISABLED, NONE},
> {20, DISABLED, NONE},
> {21, DISABLED, NONE},
> {22, DISABLED, NONE},
> {23, DISABLED, NONE},
>
> +};
>
> +static void setup_ioapic(u32 ioapic_base)
> +{
> +#if 0  /* Commented because lapicid() doesn't exist, yet */

Can you add a #warning before the #if 0? That way, we won't forget it.
> u32 value_low, value_high, val;
> volatile u32 *l;

There has been an effort to use readX/writeX functions instead of
volatile accesses. You may want to change that. Then again, I don't have
a strong opinion either way.
> int i;
> /* All delivered to CPU0. */
> ioapic_table[0].value_high = (lapicid()) << (56 - 32);

Can you use a #define for the magic 56?
> l = (unsigned long *)ioapic_base;
> /* Set APIC to FSB message bus. */
> l[0] = 0x3;

Magic values...
> val = l[4];
> l[4] = (val & 0xFFFFFE) | 1;

The line above is equivalent to
l[4] = (val & 0xffffff) | 1;

You decide which one is clearer. Anyway, the 0xFFFFFE is magic.
> /* Set APIC ADDR - this will be VT8237R_APIC_ID. */
> l[0] = 0;
> val = l[4];
> l[4] = (val & 0xF0FFFF) | (VT8237R_APIC_ID << 24);
> for (i = 0; i < ARRAY_SIZE(ioapic_table); i++) {
> l[0] = (ioapic_table[i].reg * 2) + 0x10;
> l[4] = ioapic_table[i].value_low;
> value_low = l[4];
> l[0] = (ioapic_table[i].reg * 2) + 0x11;
> l[4] = ioapic_table[i].value_high;
> value_high = l[4];
> if ((i == 0) && (value_low == 0xffffffff)) {
> 	printk_warning("IO APIC not responding.\n");
> 	return;
> }
> }

Reading the code sequence above hurts. You set l[0] and l[4] always in
combination. What about helper functions like set_apic() and get_apic()?
> +#endif
> +}
>
> +/** Set up PCI IRQ routing, route everything through APIC. */
> +static void pci_routing_fixup(struct device *dev)
> +{
> /* PCI PNP Interrupt Routing INTE/F - disable */
> pci_write_config8(dev, 0x44, 0x00);
> /* PCI PNP Interrupt Routing INTG/H - disable */
> pci_write_config8(dev, 0x45, 0x00);
> /* Route INTE-INTH through registers above, no map to INTA-INTD. */
> pci_write_config8(dev, 0x46, 0x10);
> /* PCI Interrupt Polarity */
> pci_write_config8(dev, 0x54, 0x00);
> /* PCI INTA# Routing */
> pci_write_config8(dev, 0x55, 0x00);
> /* PCI INTB#/C# Routing */
> pci_write_config8(dev, 0x56, 0x00);
> /* PCI INTD# Routing */
> pci_write_config8(dev, 0x57, 0x00);

Well-commented, but I'd appreciate #defines for the magic locations as
well. Your call, though.
> +}
>
> +/**
> Set up the power management capabilities directly into ACPI mode.
> This avoids having to handle any System Management Interrupts (SMIs).
> */
>
> +static void setup_pm(struct device *dev)

A comment about which device this function is called with would be great.
> +{
> /* Debounce LID and PWRBTN# Inputs for 16ms. */
> pci_write_config8(dev, 0x80, 0x20);
> /* Set ACPI base address to I/O VT8237R_ACPI_IO_BASE. */
> pci_write_config16(dev, 0x88, VT8237R_ACPI_IO_BASE | 0x1);
> /* Set ACPI to 9, must set IRQ 9 override to level! Set PSON gating. */
> pci_write_config8(dev, 0x82, 0x40 | VT8237R_ACPI_IRQ);
> /* Primary interupt channel, define wake events 0=IRQ0 15=IRQ15 1=en. */
> pci_write_config16(dev, 0x84, 0x30b2);
> /* SMI output level to low, 7.5us throttle clock */
> pci_write_config8(dev, 0x8d, 0x18);
> /* GP Timer Control 1s */
> pci_write_config8(dev, 0x93, 0x88);
> /* 7 = SMBus clock from RTC 32.768KHz
> * 5 = Internal PLL reset from susp
> * 2 = GPO2 is GPIO
> */
> pci_write_config8(dev, 0x94, 0xa4);
> /* 7 = stp to sust delay 1msec
> * 6 = SUSST# Deasserted Before PWRGD for STD
> * 3 = GPO26/GPO27 is GPO 
> * 2 = Disable Alert on Lan
> */
> pci_write_config8(dev, 0x95, 0xcc);
> /* Disable GP3 timer. */
> pci_write_config8(dev, 0x98, 0);
> /* Enable SATA LED, disable special CPU Frequency Change -
> * GPIO28 GPIO22 GPIO29 GPIO23 are GPIOs.
> */
> pci_write_config8(dev, 0xe5, 0x9);
> /* REQ5 as PCI request input - should be together with INTE-INTH. */
> pci_write_config8(dev, 0xe4, 0x4);
> /* Enable ACPI accessm RTC signal gated with PSON. */
> pci_write_config8(dev, 0x81, 0x84);
> /* Clear status events. */
> outw(0xffff, VT8237R_ACPI_IO_BASE + 0x00);
> outw(0xffff, VT8237R_ACPI_IO_BASE + 0x20);
> outw(0xffff, VT8237R_ACPI_IO_BASE + 0x28);
> outl(0xffffffff, VT8237R_ACPI_IO_BASE + 0x30);
> /* Disable SCI on GPIO. */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x22);
> /* Disable SMI on GPIO. */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x24);
> /* Disable all global enable SMIs. */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x2a);
> /* All SMI off, both IDE buses ON, PSON rising edge. */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x2c);
> /* Primary activity SMI disable. */
> outl(0x0, VT8237R_ACPI_IO_BASE + 0x34);
> /* GP timer reload on none. */
> outl(0x0, VT8237R_ACPI_IO_BASE + 0x38);
> /* Disable extended IO traps. */
> outb(0x0, VT8237R_ACPI_IO_BASE + 0x42);
> /* SCI is generated for RTC/pwrBtn/slpBtn. */
> outw(0x001, VT8237R_ACPI_IO_BASE + 0x04);
> /* FIXME: Intel needs more bit set for C2/C3. */
> /* Allow SLP# signal to assert LDTSTOP_L.
> * Will work for C3 and for FID/VID change.
> */
> outb(0x1, VT8237R_ACPI_IO_BASE + 0x11);
>
> +}
>
> +static void vt8237r_init(struct device *dev)

Doxygen comment, please. Including a hint which PCI device this is
called with.
> +{
> u8 enables, byte;
> /* Enable addr/data stepping. */
> byte = pci_read_config8(dev, PCI_COMMAND);
> byte |= PCI_COMMAND_WAIT;
> pci_write_config8(dev, PCI_COMMAND, byte);
> /* Enable the internal I/O decode. */
> enables = pci_read_config8(dev, 0x6C);
> enables |= 0x80;
> pci_write_config8(dev, 0x6C, enables);
> /* FIXME: Map 4MB of flash into the address space,
> * this should be in CAR call.

Not really. This belongs in hardware_stage1() or vt8237_stage1().
> */
> /* pci_write_config8(dev, 0x41, 0x7f); */
> /* Set bit 6 of 0x40 (I/O recovery time).
> * IMPORTANT FIX - EISA = ECLR reg at 0x4d0! Decoding must be on so
> * that PCI interrupts can be properly marked as level triggered.
> */
> enables = pci_read_config8(dev, 0x40);
> enables |= 0x44;
> pci_write_config8(dev, 0x40, enables);
> /* Line buffer control */
> enables = pci_read_config8(dev, 0x42);
> enables |= 0xf8;
> pci_write_config8(dev, 0x42, enables);
> /* Delay transaction control */
> pci_write_config8(dev, 0x43, 0xb);
> /* I/O recovery time */
> pci_write_config8(dev, 0x4c, 0x44);
> /* ROM memory cycles go to LPC. */
> pci_write_config8(dev, 0x59, 0x80);
> /* Bypass APIC De-Assert Message, INTE#, INTF#, INTG#, INTH# as PCI. */
> pci_write_config8(dev, 0x5B, 0xb);
> /* Set Read Pass Write Control Enable (force A2 from APIC FSB to low). */
> pci_write_config8(dev, 0x48, 0x8c);
> /* Set 0x58 to 0x43 APIC and RTC. */
> pci_write_config8(dev, 0x58, 0x43);
> /* Set bit 3 of 0x4f (use INIT# as CPU reset). */
> enables = pci_read_config8(dev, 0x4f);
> enables |= 0x08;
> pci_write_config8(dev, 0x4f, enables);
> /* Enable serial IRQ, 6PCI clocks. */
> pci_write_config8(dev, 0x52, 0x9);
> /* Enable HPET at VT8237R_HPET_ADDR. */
> pci_write_config32(dev, 0x68, (VT8237R_HPET_ADDR | 0x80));
> /* Power management setup */
> setup_pm(dev);
> /* Start the RTC. */
> rtc_init(0);
>
> +}
>
> +static void vt8237r_read_resources(struct device *dev)
> +{
> struct resource *res;
> pci_dev_read_resources(dev);
> /* Fixed APIC resource */
> res = new_resource(dev, 0x44);
> res->base = VT8237R_APIC_BASE;
> res->size = 256;
> res->limit = res->base + res->size - 1;
> res->align = 8;
> res->gran = 8;
> res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
>      IORESOURCE_STORED | IORESOURCE_ASSIGNED;
>
> +}
>
> +/**
> The VT8237R is not a PCI bridge and has no resources of its own (other
> than standard PC I/O addresses), however it does control the ISA bus
> and so we need to manually call enable childrens resources on that bus.
> */
>
> +static void vt8237r_enable_resources(struct device *dev)
> +{
> pci_dev_enable_resources(dev);
> enable_childrens_resources(dev);
>
> +}
>
> +static void init_keyboard(struct device *dev)
> +{
> u8 regval = pci_read_config8(dev, 0x51);
> if (regval & 0x1)
> init_pc_keyboard(0x60, 0x64, 0);
>
> +}
>
> +static void vt8237r_lpc_init(struct device *dev)
> +{
> vt8237r_init(dev);
> pci_routing_fixup(dev);
> setup_ioapic(VT8237R_APIC_BASE);
> setup_i8259();
> init_keyboard(dev);
>
> +}
>
> +static void vt8237r_ide_init(struct device *dev)
> +{
> struct southbridge_via_vt8237r_ide_config *sb =
>    (struct southbridge_via_vt8237r_ide_config *)dev->device_configuration;
> u8 enables;
> u32 cablesel;
> printk(BIOS_INFO, "%s IDE interface %s\n", "Primary",
>     sb->ide0_enable ? "enabled" : "disabled");
> printk(BIOS_INFO, "%s IDE interface %s\n", "Secondary",
>     sb->ide1_enable ? "enabled" : "disabled");
> enables = pci_read_config8(dev, IDE_CS) & ~0x3;
> enables |= (sb->ide0_enable << 1) | sb->ide1_enable;
> pci_write_config8(dev, IDE_CS, enables);
> enables = pci_read_config8(dev, IDE_CS);
> printk(BIOS_DEBUG, "Enables in reg 0x40 read back as 0x%x\n", enables);
> /* Enable only compatibility mode. */
> enables = pci_read_config8(dev, IDE_CONF_II);
> enables &= ~0xc0;

Magic 0xc0.
> pci_write_config8(dev, IDE_CONF_II, enables);
> enables = pci_read_config8(dev, IDE_CONF_II);
> printk(BIOS_DEBUG, "Enables in reg 0x42 read back as 0x%x\n", enables);
> /* Enable prefetch buffers. */
> enables = pci_read_config8(dev, IDE_CONF_I);
> enables |= 0xf0;

Magic 0xf0.
> pci_write_config8(dev, IDE_CONF_I, enables);
> /* Flush FIFOs at half. */
> enables = pci_read_config8(dev, IDE_CONF_FIFO);
> enables &= 0xf0;
> enables |= (1 << 2) | (1 << 0);
> pci_write_config8(dev, IDE_CONF_FIFO, enables);
> /* PIO read prefetch counter, Bus Master IDE Status Reg. Read Retry. */
> enables = pci_read_config8(dev, IDE_MISC_I);
> enables &= 0xe2;
> enables |= (1 << 4) | (1 << 3);
> pci_write_config8(dev, IDE_MISC_I, enables);
> /* Use memory read multiple, Memory-Write-and-Invalidate. */
> enables = pci_read_config8(dev, IDE_MISC_II);
> enables |= (1 << 2) | (1 << 3);
> pci_write_config8(dev, IDE_MISC_II, enables);
> /* Force interrupts to use compat mode. */
> pci_write_config8(dev, PCI_INTERRUPT_PIN, 0x0);
> pci_write_config8(dev, PCI_INTERRUPT_LINE, 0xff);
> /* Cable guy... */
> cablesel = pci_read_config32(dev, IDE_UDMA);
> cablesel &= ~((1 << 28) | (1 << 20) | (1 << 12) | (1 << 4));
> cablesel |= (sb->ide0_80pin_cable << 28) |
>     (sb->ide0_80pin_cable << 20) |
>     (sb->ide1_80pin_cable << 12) |
>     (sb->ide1_80pin_cable << 4);
> pci_write_config32(dev, IDE_UDMA, cablesel);
>
> +}
>
> +static void vt8237r_sata_init(struct device *dev)
> +{
> u8 reg;
> printk(BIOS_DEBUG, "Configuring VIA SATA controller\n");
> /* Class IDE Disk */
> reg = pci_read_config8(dev, SATA_MISC_CTRL);
> reg &= 0x7f;		/* Sub Class Write Protect off */
> pci_write_config8(dev, SATA_MISC_CTRL, reg);
> /* Change the device class to SATA from RAID. */
> pci_write_config8(dev, PCI_CLASS_DEVICE, 0x1);
> reg |= 0x80;		/* Sub Class Write Protect on */
> pci_write_config8(dev, SATA_MISC_CTRL, reg);
>
> +}
>
> +/* You can override or extend each operation as needed for the device. */
> +struct device_operations vt8237r_lpc = {
> .id = {.type = DEVICE_ID_PCI,
> .u = {.pci = {	.vendor = PCI_VENDOR_ID_VIA,

Remove the ".u =" because this is now an anonymous union.
> 		.device = PCI_DEVICE_ID_VIA_VT8237R_LPC}}},
> .constructor		 = default_device_constructor,
> //.phase3_scan		 = 0,
> .phase4_read_resources	 = pci_dev_read_resources,
> .phase4_set_resources	 = pci_dev_set_resources,
> .phase4_enable_disable	 = 0,
> .phase5_enable_resources = pci_dev_enable_resources,
> .phase6_init		 = vt8237r_lpc_init,
> .ops_pci		 = &pci_dev_ops_pci,
>
> +};
>
> +struct device_operations vt8237r_ide = {
> .id = {.type = DEVICE_ID_PCI,
> .u = {.pci = {	.vendor = PCI_VENDOR_ID_VIA, 

.u removal
> 		.device = PCI_DEVICE_ID_VIA_VT82XXX_IDE}}},
> .constructor		 = default_device_constructor,
> .phase3_scan		 = 0,
> .phase4_read_resources	 = pci_dev_read_resources,
> .phase4_set_resources	 = pci_dev_set_resources,
> .phase4_enable_disable	 = 0,
> .phase5_enable_resources = pci_dev_enable_resources,
> .phase6_init		 = vt8237r_ide_init,
> .ops_pci		 = &pci_dev_ops_pci,
>
> +};
>
> +struct device_operations vt8237r_sata = {
> .id = {.type = DEVICE_ID_PCI,
> .u = {.pci = {	.vendor = PCI_VENDOR_ID_VIA,

.u removal
> 		.device = PCI_DEVICE_ID_VIA_VT8237R_SATA}}},
> .constructor		 = default_device_constructor,
> .phase3_scan		 = 0,
> .phase4_read_resources	 = pci_dev_read_resources,
> .phase4_set_resources	 = pci_dev_set_resources,
> .phase4_enable_disable	 = 0,
> .phase5_enable_resources = pci_dev_enable_resources,
> .phase6_init		 = vt8237r_sata_init,
> .ops_pci		 = &pci_dev_ops_pci,
>
> +};
> Index: southbridge/via/vt8237r/ide
> ===================================================================
> --- southbridge/via/vt8237r/ide	(revision 0)
> +++ southbridge/via/vt8237r/ide	(revision 0)
> @@ -0,0 +1,27 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Corey Osgood
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +{
> device_operations = "vt8237r_ide";
> ide0_enable = "1";
> ide0_80pin_cable = "0";
> ide1_enable = "0";
> ide1_80pin_cable = "0";
>
> +};
> Index: southbridge/via/vt8237r/vt8237r.h
> ===================================================================
> --- southbridge/via/vt8237r/vt8237r.h	(revision 0)
> +++ southbridge/via/vt8237r/vt8237r.h	(revision 0)
> @@ -0,0 +1,74 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#ifndef SOUTHBRIDGE_VIA_VT8237R_VT8237R_H
> +#define SOUTHBRIDGE_VIA_VT8237R_VT8237R_H
>
> +/* Static resources for the VT8237R southbridge */
>
> +#define VT8237R_APIC_ID			0x2
> +#define VT8237R_ACPI_IO_BASE		0x500
> +#define VT8237R_SMBUS_IO_BASE		0x400
> +/* 0x0 disabled, 0x2 reserved, 0xf = IRQ15 */
> +#define VT8237R_ACPI_IRQ		0x9
> +#define VT8237R_HPET_ADDR		0xfed00000ULL
> +#define VT8237R_APIC_BASE		0xfec00000ULL
>
> +/* IDE */
> +#define IDE_CS				0x40
> +#define IDE_CONF_I			0x41
> +#define IDE_CONF_II			0x42
> +#define IDE_CONF_FIFO			0x43
> +#define IDE_MISC_I			0x44
> +#define IDE_MISC_II			0x45
> +#define IDE_UDMA			0x50
> +#define SATA_MISC_CTRL			0x45
>
> +/* SMBus */
> +#define VT8237R_POWER_WELL		0x94
> +#define VT8237R_SMBUS_IO_BASE_REG	0xd0
> +#define VT8237R_SMBUS_HOST_CONF		0xd2
>
> +#define SMBHSTSTAT			(VT8237R_SMBUS_IO_BASE + 0x0)
> +#define SMBSLVSTAT			(VT8237R_SMBUS_IO_BASE + 0x1)
> +#define SMBHSTCTL			(VT8237R_SMBUS_IO_BASE + 0x2)
> +#define SMBHSTCMD			(VT8237R_SMBUS_IO_BASE + 0x3)
> +#define SMBXMITADD			(VT8237R_SMBUS_IO_BASE + 0x4)
> +#define SMBHSTDAT0			(VT8237R_SMBUS_IO_BASE + 0x5)
>
> +#define HOST_RESET 			0xff
> +/* 1 in the 0 bit of SMBHSTADD states to READ. */
> +#define READ_CMD			0x01
> +#define SMBUS_TIMEOUT			(100 * 1000 * 10)
> +#define I2C_TRANS_CMD			0x40
> +#define CLOCK_SLAVE_ADDRESS		0x69
>
> +#if DEBUG_SMBUS == 1
> +#define PRINT_DEBUG(x)		print_debug(x)
> +#define PRINT_DEBUG_HEX16(x)	print_debug_hex16(x)
> +#else
> +#define PRINT_DEBUG(x)
> +#define PRINT_DEBUG_HEX16(x)
> +#endif
>
> +#define SMBUS_DELAY() inb(0x80)
>
> +void vt8237r_stage1(void);
>
> +#endif
> Index: southbridge/via/vt8237r/stage1.c
> ===================================================================
> --- southbridge/via/vt8237r/stage1.c	(revision 0)
> +++ southbridge/via/vt8237r/stage1.c	(revision 0)
> @@ -0,0 +1,36 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Corey Osgood
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <statictree.h>
> +#include "vt8237r.h"
>
> +void vt8237r_stage1(void)
> +{
> +/*	struct device *dev = dev_find_pci_device(PCI_VENDOR_ID_VIA,
> 		       PCI_DEVICE_ID_VIA_VT8237R_LPC, 0);
> struct southbridge_via_vt8237r_dts_config *sb =
>    (struct southbridge_via_vt8237r_dts_config *)dev->device_operations;
> pci_write_config8(dev, 0x50, sb->fn_ctrl_lo);
> pci_write_config8(dev, 0x51, sb->fn_ctrl_hi);
>
> +*/
> +}
> Index: southbridge/via/vt8237r/dts
>
> --- southbridge/via/vt8237r/dts	(revision 0)
> +++ southbridge/via/vt8237r/dts	(revision 0)
> @@ -0,0 +1,25 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Corey Osgood
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +{
> fn_ctrl_lo = "0x00"
> fn_ctrl_hi = "0x00"
> device_operations = "vt8237r_lpc";
>
> +};
> Index: southbridge/via/vt8237r/Makefile
> ===================================================================
> --- southbridge/via/vt8237r/Makefile	(revision 0)
> +++ southbridge/via/vt8237r/Makefile	(revision 0)
> @@ -0,0 +1,26 @@
> +##
> +## This file is part of the coreboot project.
> +##
> +## Copyright (C) 2008 Corey Osgood 
> +##
> +## This program is free software; you can redistribute it and/or modify
> +## it under the terms of the GNU General Public License as published by
> +## the Free Software Foundation; either version 2 of the License, or
> +## (at your option) any later version.
> +##
> +## This program is distributed in the hope that it will be useful,
> +## but WITHOUT ANY WARRANTY; without even the implied warranty of
> +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +## GNU General Public License for more details.
> +##
> +## You should have received a copy of the GNU General Public License
> +## along with this program; if not, write to the Free Software
> +## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> +##
>
> +ifeq ($(CONFIG_SOUTHBRIDGE_VIA_VT8237R),y)
>
> +STAGE2_CHIPSET_OBJ += $(obj)/southbridge/via/vt8237r/vt8237r.o
> +STAGE0_CHIPSET_OBJ += $(obj)/southbridge/via/vt8237r/stage1.o
>
> +endif
> Index: southbridge/via/vt8237r/smbus_initram.c
> ===================================================================
> --- southbridge/via/vt8237r/smbus_initram.c	(revision 0)
> +++ southbridge/via/vt8237r/smbus_initram.c	(revision 0)
> @@ -0,0 +1,208 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2007-2008 Corey Osgood
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <types.h>
> +#include <console.h>
> +#include <io.h>
> +#include <device/pci_ids.h>
> +#include "vt8237r.h"
>
> +/**
> Print an error, should it occur. If no error, just exit.
> @param host_status The data returned on the host status register after
>       a transaction is processed.
> @param loops The number of times a transaction was attempted.
> */
>
> +static void smbus_print_error(u8 host_status, int loops)
> +{
> /* Check if there actually was an error. */
> if ((host_status == 0x00 || host_status == 0x40 ||
>     host_status == 0x42) && (loops < SMBUS_TIMEOUT))
> return;
> if (loops >= SMBUS_TIMEOUT)
> printk(BIOS_ERR, "SMBus timeout\r\n");
> if (host_status & (1 << 4))
> printk(BIOS_ERR, "Interrupt/SMI# was Failed Bus Transaction\r\n");
> if (host_status & (1 << 3))
> printk(BIOS_ERR, "Bus error\r\n");
> if (host_status & (1 << 2))
> printk(BIOS_ERR, "Device error\r\n");
> if (host_status & (1 << 1))
> printk(BIOS_DEBUG, "Interrupt/SMI# completed successfully\r\n");
> if (host_status & (1 << 0))
> printk(BIOS_ERR, "Host busy\r\n");
>
> +}
>
> +/**
> Wait for the SMBus to become ready to process the next transaction.
> */
>
> +static void smbus_wait_until_ready(void)
> +{
> int loops;
> loops = 0;
> /* Yes, this is a mess, but it's the easiest way to do it. */
> while ((inb(SMBHSTSTAT) & 1) == 1 && loops < SMBUS_TIMEOUT)
> ++loops;
> smbus_print_error(inb(SMBHSTSTAT), loops);
>
> +}
>
> +/**
> Reset and take ownership of the SMBus.
> */
>
> +static void smbus_reset(void)
> +{
> outb(HOST_RESET, SMBHSTSTAT);
> /* Datasheet says we have to read it to take ownership of SMBus. */
> inb(SMBHSTSTAT);
>
> +}
>
> +/**
> Read a byte from the SMBus.
> @param dimm The address location of the DIMM on the SMBus.
> @param offset The offset the data is located at.
> */
>
> +u8 smbus_read_byte(u8 dimm, u8 offset)
> +{
> u8 val;
> smbus_reset();
> /* Clear host data port. */
> outb(0x00, SMBHSTDAT0);
> SMBUS_DELAY();
> smbus_wait_until_ready();
> /* Actual addr to reg format. */
> dimm = (dimm << 1);
> dimm |= 1;
> outb(dimm, SMBXMITADD);
> outb(offset, SMBHSTCMD);
> /* Start transaction, byte data read. */
> outb(0x48, SMBHSTCTL);
> SMBUS_DELAY();
> smbus_wait_until_ready();
> val = inb(SMBHSTDAT0);
> /* Probably don't have to do this, but it can't hurt. */
> smbus_reset();
> return val;
>
> +}
> +static void smbus_enable(void)
> +{
> struct device *dev;
> /* Power management controller */
> dev = dev_find_pci_device(PCI_VENDOR_ID_VIA,
> 		       PCI_DEVICE_ID_VIA_VT8237R_LPC, 0);
> if (dev == PCI_DEV_INVALID)
> die("Power management controller not found\r\n");
> /* 7 = SMBus Clock from RTC 32.768KHz
> * 5 = Internal PLL reset from susp
> */
> pci_write_config8(dev, VT8237R_POWER_WELL, 0xa0);
> /* Enable SMBus. */
> pci_write_config16(dev, VT8237R_SMBUS_IO_BASE_REG,
> 	   VT8237R_SMBUS_IO_BASE | 0x1);
> /* SMBus Host Configuration, enable. */
> pci_write_config8(dev, VT8237R_SMBUS_HOST_CONF, 0x01);
> /* Make it work for I/O. */
> pci_write_config16(dev, PCI_COMMAND, PCI_COMMAND_IO);
> smbus_reset();
> /* Reset the internal pointer. */
> inb(SMBHSTCTL);}

What's that trailing "}"?
> +}
>
> +/**
> A fixup for some systems that need time for the SMBus to "warm up". This is
> needed on some VT823x based systems, where the SMBus spurts out bad data for
> a short time after power on. This has been seen on the VIA Epia series and
> Jetway J7F2-series. It reads the ID byte from SMBus, looking for
> known-good data from a slot/address. Exits on either good data or a timeout.
> TODO: This should probably go into some global file, but one would need to
>   be created just for it. If some other chip needs/wants it, we can
>   worry about it then.
> @param ctrl The memory controller and SMBus addresses.
> */
>
> +void smbus_fixup(const struct mem_controller *ctrl)
> +{
> int i, ram_slots, current_slot = 0;
> u8 result = 0;
> ram_slots = ARRAY_SIZE(ctrl->channel0);
> if (!ram_slots) {
> printk(BIOS_ERR, "smbus_fixup() thinks there are no RAM slots!\r\n");
> return;
> }
> /*
> * Bad SPD data should be either 0 or 0xff, but YMMV. So we look for
> * the ID bytes of SDRAM, DDR, DDR2, and DDR3 (and anything in between).
> * VT8237R has only been seen on DDR and DDR2 based systems, so far.
> */
> for (i = 0; (i < SMBUS_TIMEOUT && ((result < SPD_MEMORY_TYPE_SDRAM) ||
> 		(result > SPD_MEMORY_TYPE_SDRAM_DDR3))); i++) {
> if (current_slot > ram_slots)
> 	current_slot = 0;
> result = smbus_read_byte(ctrl->channel0[current_slot],
> 			 SPD_MEMORY_TYPE);
> current_slot++;
> PRINT_DEBUG(".");

printk() please.
> }
> if (i >= SMBUS_TIMEOUT)
> print_err("SMBus timed out while warming up\r\n");

printk() please.
> else
> PRINT_DEBUG("Done\r\n");

printk() please.
> +}
>
> +void enable_rom_decode(void)
> +{
> struct device *dev;
> /* Bus Control and Power Management  */
> dev = dev_find_pci_device(PCI_VENDOR_ID_VIA,
> 		       PCI_DEVICE_ID_VIA_VT8237R_LPC, 0);
> if (dev == PCI_DEV_INVALID)
> die("SB not found\r\n");
> /* ROM decode last 1MB FFC00000 - FFFFFFFF */
> pci_write_config8(dev, 0x41, 0x7f);
>
> +}
> Index: Kconfig
> ===================================================================
> --- Kconfig	(revision 644)
> +++ Kconfig	(working copy)
> @@ -80,6 +80,8 @@
>    boolean
>  config SOUTHBRIDGE_INTEL_I82371EB
>    boolean
> +config SOUTHBRIDGE_VIA_VT8237R
> boolean
>
> # Super I/Os:
>  config SUPERIO_WINBOND_W83627HF
> Index: include/device/pci_ids.h
> ===================================================================
> --- include/device/pci_ids.h	(revision 644)
> +++ include/device/pci_ids.h	(working copy)
> @@ -155,4 +155,10 @@
>  #define PCI_VENDOR_ID_CIRRUS			0x1013
>  #define PCI_DEVICE_ID_CIRRUS_5446		0x00b8	/* Used by QEMU */
>
> +/* Need to check these */
> +#define PCI_VENDOR_ID_VIA			0x1106
> +#define PCI_DEVICE_ID_VIA_VT8237R_LPC		0x3074
> +#define PCI_DEVICE_ID_VIA_VT82XXX_IDE		0x0571
> +#define PCI_DEVICE_ID_VIA_VT8237R_SATA		0x3149
>
> #endif /* DEVICE_PCI_IDS_H */

I think with another iteration this will be golden.
Bonus points if you fix this in v2 as well.

Regards,
Carl-Daniel