Re: [LinuxBIOS] [PATCH] VIA VT8237R support

[Uwe Hermann]

Here's one more review:

On Sat, Oct 27, 2007 at 01:16:49AM +0200, Rudolf Marek wrote:
> +void hard_reset(void)
> +{
> printk_err("NO HARD RESET ON VT8237R! FIX ME!\n");
>
> +}
>
> +void writeback(struct device *dev, u16 where,u8 what)

^
                                          missing space
> +{
> u8 regval;
> pci_write_config8(dev, where, what);
> regval = pci_read_config8(dev, where);
> if (regval != what) {
> print_debug("Writeback to ");
> print_debug_hex8(where);
> print_debug("failed ");
> print_debug_hex8(regval);
> print_debug("\n ");
> }
>
> +}

Hm, looks generally useful, why not put it in some global debug.c file?
> +void dump_south(device_t dev)
> +{
> int i, j;
> for (i = 0; i < 256; i += 16) {
> printk_debug("%02x: ", i);
> for (j = 0; j < 16; j++) {
> 	printk_debug("%02x ",
> 		     pci_read_config8(dev, i + j));
> }
> printk_debug("\n");
> }
>
> +}

Ditto. There already exists such code in (various) debug.c files in
the repo, I think.
> +static void vt8237r_enable(struct device *dev)
> +{
> struct southbridge_via_vt8237r_config *sb =
> (struct southbridge_via_vt8237r_config *) dev->chip_info;
> /* function disable 1=disabled
> 7 Dev 17 fn 6 MC97
> 6 Dev 17 fn 5 AC97
> 5 Dev 16 fn 1 USB 1.1 UHCI Ports 2-3
> 4 Dev 16 fn 0 USB 1.1 UHCI Ports 0-1
> 3 Dev 15 fn 0 Serial ATA
> 2 Dev 16 fn 2 USB 1.1 UHCI Ports 4-5
> 1 Dev 16 fn 4 USB 2.0 EHCI
> 0 Dev 16 fn 3 USB 1.1 UHCI Ports 6-7
> */

Use Linux-style comments please:

/*
 * Foo.
 * Bar.
 */
> pci_write_config8(dev, 0x50, sb->fn_ctrl_lo);
> /*
> 7 USB Device Mode 1=dis
> 6 Reserved  
> 5 Internal LAN Controller Clock Gating 1=gated
> 4 Internal LAN Controller 1=di
> 3 Internal RTC 1=en
> 2 Internal PS2 Mouse 1=en
> 1 Internal KBC Configuration 0=dis ports 0x2e/0x2f off 0xe0-0xef
> 0 Internal Keyboard Controller 1=en
> */
> pci_write_config8(dev, 0x51, sb->fn_ctrl_hi);
> /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */
>
> +}
>
> +struct chip_operations southbridge_via_vt8237r_ops = {
> CHIP_NAME("VIA VT8237R Southbridge")
>    .enable_dev = vt8237r_enable,
>
> +};
> Index: src/southbridge/via/vt8237r/Config.lb
> ===================================================================
> --- src/southbridge/via/vt8237r/Config.lb	(revision 0)
> +++ src/southbridge/via/vt8237r/Config.lb	(revision 0)
> @@ -0,0 +1,24 @@
> +##
> +## This file is part of the LinuxBIOS 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
> +##
> +config chip.h
> +driver vt8237r.o
> +driver vt8237r_ide.o
> +driver vt8237r_lpc.o
> +driver vt8237r_sata.o
> +driver vt8237r_bridge.o

UHCI? EHCI? AC97 audio / modem? LAN? What's the status there?
> Index: src/southbridge/via/vt8237r/vt8237r_ide.c
>
> --- src/southbridge/via/vt8237r/vt8237r_ide.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_ide.c	(revision 0)
> @@ -0,0 +1,124 @@
> +/*
> This file is part of the LinuxBIOS project.
> Based on other VIA SB code, no native mode. Interrupts from unconnected HDDs
> might occur if IRQ14/15 is used for PCI. Therefore no native mode support.

Don't put this in the license header please, move it somewhere below as
a normal comment.
> 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
> */
>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <console/console.h>
> +#include "vt8237r.h"
> +#include "chip.h"
>
> +#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

Yep, either here or vt8237.h.
> +static void ide_init(struct device *dev)
> +{
> struct southbridge_via_vt8237r_config *sb =
> (struct southbridge_via_vt8237r_config *) dev->chip_info;
> u8 enables;
> u32 cablesel;
> printk_info("Enabling VIA IDE.\n");

Not needed IMHO if the other lines are printed already.
> printk_info("%s IDE interface %s\n", "Primary",
>     sb->ide0_enable ? "enabled" : "disabled");
> printk_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_debug("enables in reg 0x40 read back as 0x%x\n", enables);

Why not use writeback() here (and in a few more places below)?
> /* enable only compatibility mode, cannot use IRQ14/IRQ15 for PCI anyway */

Line too long, and it should start with capital letter and end with full stop.
> enables = pci_read_config8(dev, IDE_CONF_II);
> enables &= ~0xc0;
> pci_write_config8(dev,IDE_CONF_II, enables);
> enables = pci_read_config8(dev, IDE_CONF_II);
> printk_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;
> 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 Register 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);
> /* address decoding */
> /* disable the BARs, we use std ports */

Why? Just curious...
> pci_write_config32(dev, PCI_BASE_ADDRESS_0, 0x0);
> pci_write_config32(dev, PCI_BASE_ADDRESS_1, 0x0);
> pci_write_config32(dev, PCI_BASE_ADDRESS_2, 0x0);
> pci_write_config32(dev, PCI_BASE_ADDRESS_3, 0x0);
> /* Force interrupts to use compat mode */
> pci_write_config8(dev, PCI_INTERRUPT_PIN, 0x0);
> pci_write_config8(dev, PCI_INTERRUPT_LINE, 0xff);
> /* standard bios sets master bit. */
> enables = pci_read_config8(dev, PCI_COMMAND);
> enables |= PCI_COMMAND_MASTER | PCI_COMMAND_IO;
> pci_write_config8(dev, PCI_COMMAND, enables);
> enables = pci_read_config8(dev, 0x4);
> printk_debug("command in reg 0x4 reads back as 0x%x\n", enables);
> /* cable guy */
> cablesel = pci_read_config32(dev, IDE_UDMA);
> cablesel &= ~((1 << 28) | (1 << 20) | (1 <<12) | (1 << 4));
> cablesel |= (sb->ide0_cable << 28) | (sb->ide0_cable << 20) |
> 	(sb->ide1_cable << 12) | (sb->ide1_cable << 4);
> pci_write_config32(dev, IDE_UDMA, cablesel);
>
> +}
>
> +static struct device_operations ide_ops = {
> .read_resources = pci_dev_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init = ide_init,
> .enable = 0,
> .ops_pci = 0,
>
> +};
>
> +static struct pci_driver northbridge_driver __pci_driver = {
> .ops = &ide_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_82C586_1,
>
> +};
> Index: src/southbridge/via/vt8237r/vt8237r.h
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r.h	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r.h	(revision 0)
> @@ -0,0 +1,28 @@
> +/*
> This file is part of the LinuxBIOS 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
> */
>
> +/* static resources for 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 */

^
                           missing comma?
> +#define VT8237R_ACPI_IRQ 	0x9
> +#define VT8237R_HPET_ADDR  	0xfed00000ULL
> +#define VT8237R_APIC_BASE	0xfec00000ULL
> Index: src/southbridge/via/vt8237r/vt8237r_early_smbus.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_early_smbus.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_early_smbus.c	(revision 0)
> @@ -0,0 +1,172 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 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 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 <device/pci_ids.h>
> +#include "vt8237r.h"
>
> +#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 register settings */
> +#define HOST_RESET 		0xff
> +#define DIMM_BASE		(0x50<<1)

Is this really needed in vt8237r_early_smbus.c? I don't think so.
This address is dependent on the board anyway, correct?
> +/* 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
>
> +/* Debugging macros. Only necessary if something isn't working right */
>
> +#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
> +/* Internal functions */

Drop the above comment, not needed IMO.
> +/* TODO: make define? */
> +#define SMBUS_DELAY() inb(0x80)

Huh? It already is...
> +static void smbus_print_error(u8 host_status_register,
> 	      int loops)

"int loops" can go in the same line.
> +{
> /* Check if there actually was an error */
> if (host_status_register == 0x00 || host_status_register == 0x40 ||

Pretty long name, maybe host_status or so? It's relatively clear we talk
about a register.
>    host_status_register == 0x42)
> return;
> if (loops >= SMBUS_TIMEOUT) {
> print_err("SMBus Timout\r\n");
> }
> if (host_status_register & (1 << 4)) {
> print_err("Interrup/SMI# was Failed Bus Transaction\r\n");
> }
> if (host_status_register & (1 << 3)) {
> print_err("Bus Error\r\n");
> }
> if (host_status_register & (1 << 2)) {
> print_err("Device Error\r\n");
> }
> if (host_status_register & (1 << 1)) {
> print_err("Interrupt/SMI# was Successful Completion\r\n");
> }
> if (host_status_register & (1 << 0)) {
> print_err("Host Busy\r\n");
> }

This is probably a matter of taste, but for one-liners I learned to like
this style:

[...]
if (loops >= SMBUS_TIMEOUT)
        print_err("SMBus Timout\r\n");
if (host_status_register & (1 << 4))
        print_err("Interrup/SMI# was Failed Bus Transaction\r\n");
if (host_status_register & (1 << 3))
        print_err("Bus Error\r\n");
if (host_status_register & (1 << 2))
        print_err("Device Error\r\n");
if (host_status_register & (1 << 1))
        print_err("Interrupt/SMI# was Successful Completion\r\n");
if (host_status_register & (1 << 0))
        print_err("Host Busy\r\n");
> +}
>
> +static void smbus_wait_until_ready(void)
> +{
> int loops;
> PRINT_DEBUG("Waiting until smbus ready\r\n");
> 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);
>
> +}
>
> +static void smbus_reset(void)
> +{
> outb(HOST_RESET, SMBHSTSTAT);
> /* Datasheet says we have to read it to take ownership of SMBus */
> inb(SMBHSTSTAT);
> PRINT_DEBUG("After reset status: ");
> PRINT_DEBUG_HEX16(inb(SMBHSTSTAT));

Reading it twice won't do any harm here, I assume?
> PRINT_DEBUG("\r\n");
>
> +}
> +/* Public functions */

Not needed.
> +u8 smbus_read_byte(u32 dimm, u32 offset)
> +{
> u32 val;
> print_debug("DIMM ");
> print_debug_hex8(dimm);
> print_debug(" OFFSET ");
> print_debug_hex8(offset);
> print_debug("\r\n");
> 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);
> outb(0x48, SMBHSTCTL);

What is 0x48? What effect does it have (should be a comment)?
> SMBUS_DELAY();
> smbus_wait_until_ready();
> val = inb(SMBHSTDAT0);
> print_debug("Read: ");
> print_debug_hex8(val);
> print_debug("\r\n");
> smbus_reset();		/* probably don't have to do this, but it can't hurt */
> return val;		/* can I just "return inb(SMBHSTDAT0)"? */

Depends. Will the read value (and side-effects, if any) be the same
after the above reset?
> +}
>
> +void enable_smbus(void)
> +{
> device_t dev;
> /* Power management controller */
> dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
> 			PCI_DEVICE_ID_VIA_VT8237R_ISA), 0);
> if (dev == PCI_DEV_INVALID) {
> die("Power Managment Controller not found\r\n");
> }
> /* Set clock source */
> pci_write_config8(dev, 0x94, 0xa0);

Needs better comment, e.g. "Set clock source to RTC. Enable Internal PLL
Reset During Suspend" or something like that.

Also, 0x94 -> VT8237R_POWER_WELL_CONTROL (or just POWER_WELL_CONTROL).
> /* Write SMBus IO base to 0xd0, and enable SMBus */
> pci_write_config16(dev, 0xd0, VT8237R_SMBUS_IO_BASE | 0x1);

0xd0 -> SMBUS_IO_BASE_REG

(yes, the "REG" sucks, but in thise cause it can be confuѕed with
 VT8237R_SMBUS_IO_BASE otherwise)
> /* Set to Award value */

Nope...
> pci_write_config8(dev, 0xd2, 0x01);

... but bow about "Enable SMB controller functions"?

0xd2 -> SMBUS_HOST_CONFIG(URATION).
> /* make it work for I/O ... */
> pci_write_config16(dev, PCI_COMMAND, PCI_COMMAND_IO);
> smbus_reset();

Again? Needed?
> /* reset the internal pointer */
> inb(SMBHSTCTL);
>
> +}
> Index: src/southbridge/via/vt8237r/vt8237r_sata.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_sata.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_sata.c	(revision 0)
> @@ -0,0 +1,56 @@
> +/*
> This file is part of the LinuxBIOS 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
> */
>
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
>
> +#define SATA_MISC_CTRL 0x45
>
> +static void sata_init(struct device *dev)
> +{
> u8 reg;
> printk_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);
>
> +}
>
> +static struct device_operations sata_ops = {
> .read_resources = pci_dev_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init = sata_init,
> .enable = 0,
> .ops_pci = 0,
>
> +};
>
> +static struct pci_driver northbridge_driver __pci_driver = {
> .ops = &sata_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_VT6420_SATA,
>
> +};
> Index: src/southbridge/via/vt8237r/chip.h
> ===================================================================
> --- src/southbridge/via/vt8237r/chip.h	(revision 0)
> +++ src/southbridge/via/vt8237r/chip.h	(revision 0)
> @@ -0,0 +1,37 @@
> +/*
> This file is part of the LinuxBIOS 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_CHIP_H
> +#define SOUTHBRIDGE_VIA_VT8237R_CHIP_H
>
> +extern struct chip_operations southbridge_via_vt8237r_ops;
>
> +struct southbridge_via_vt8237r_config {
> /* function enable bits, check vt8237r.c for details */
> unsigned int fn_ctrl_lo;

u16?
> unsigned int fn_ctrl_hi;
> int ide0_enable:1;
> int ide1_enable:1;
> /* 1 = 80-pin cable */
> int ide0_cable:1;
> int ide1_cable:1;
>
> +};
>
> +#endif /* SOUTHBRIDGE_VIA_VT8237R_CHIP_H */
> Index: src/southbridge/via/vt8237r/vt8237r_bridge.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_bridge.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_bridge.c	(revision 0)
> @@ -0,0 +1,57 @@
> +/*
> This file is part of the LinuxBIOS 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
> */
>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <console/console.h>
>
> +static void br_enable(struct device *dev)

bridge_enable, no need to truncate it here, IMHO.
> +{
> print_debug("B188 device dump\n");

B188?
> /* VIA recommends this, sorry no known info */
> writeback(dev, 0x40, 0x91);
> writeback(dev, 0x41, 0x40);
> writeback(dev, 0x43, 0x44);
> writeback(dev, 0x44, 0x31);
> writeback(dev, 0x45, 0x3a);
> writeback(dev, 0x46, 0x88);	/* PCI ID lo */
> writeback(dev, 0x47, 0xb1);	/* PCI ID hi */
> writeback(dev, 0x3e, 0x16);	/* bridge control */
> dump_south(dev);
>
> +}
>
> +static struct device_operations br_ops = {
> .read_resources = pci_bus_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = pci_bus_enable_resources,
> .enable = br_enable,
> .scan_bus = pci_scan_bridge,
> .reset_bus = pci_bus_reset,
> .ops_pci = 0,
>
> +};
>
> +static struct pci_driver northbridge_driver __pci_driver = {
> .ops = &br_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_K8T890CE_BR,
>
> +};
> Index: src/southbridge/via/vt8237r/vt8237r_lpc.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_lpc.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_lpc.c	(revision 0)
> @@ -0,0 +1,307 @@
> +/*
> This file is part of the LinuxBIOS project.
> Inspiration from other VIA SB code.
> 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
> */
>
> +#include <arch/io.h>
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <pc80/mc146818rtc.h>
> +#include <cpu/x86/lapic.h>
> +#include "vt8237r.h"
> +#include "chip.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)
>
> +struct ioapicreg {
> u32 reg;
> u32 value_low, value_high;
>
> +};
>
> +static struct ioapicreg ioapicregvalues[] = {

You can merge them like this (as the struct is never needed anywhere
else, AFAICS):

static const struct {
    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},
> /* Be careful and don't write past the end... */
>
> +};
> +extern void dump_south(device_t dev);

Should be at the top of the file (if at all).
> +static void setup_ioapic(u32 ioapic_base)
> +{
> u32 value_low, value_high, val;
> volatile u32 *l;
> struct ioapicreg *a = ioapicregvalues;

Not needed? Use the static ioapic_table[] from above directly.
> int i;
> /* all delivered to CPU0 */
> ioapicregvalues[0].value_high = (lapicid()) << (56 - 32);
> l = (unsigned long *) ioapic_base;
> /* set APIC to FSB message bus */
> l[0] = 0x3; val = l[4];

Only one command per line please.
> l[4] = (val & 0xFFFFFE) | 1;
> /* 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 < sizeof(ioapicregvalues) / sizeof(ioapicregvalues[0]);

ARRAY_SIZE (later).
>     i++, a++) {
> l[0] = (a->reg * 2) + 0x10;
> l[4] = a->value_low;
> value_low = l[4];
> l[0] = (a->reg * 2) + 0x11;
> l[4] = a->value_high;
> value_high = l[4];
> if ((i == 0) && (value_low == 0xffffffff)) {
> 	printk_warning("IO APIC not responding.\n");
> 	return;
> }
> }
>
> +}
>
> +static void pci_routing_fixup(struct device *dev)
> +{
> /* set up PCI IRQ routing, route everything through APIC */

Maybe make this a Doxygen comment at the top of the function?
> pci_write_config8(dev, 0x44, 0x00);	/* PCI PNP Interrupt Routing INTE/F - disable */
> pci_write_config8(dev, 0x45, 0x00);	/* PCI PNP Interrupt Routing INTG/H - disable */
> pci_write_config8(dev, 0x46, 0x10);	/* Route INTE-INTH through registers above, no map to INTA-INTD */
> pci_write_config8(dev, 0x54, 0x00);	/* PCI Interrupt Polarity */
> pci_write_config8(dev, 0x55, 0x00);	/* PCI INTA# Routing */
> pci_write_config8(dev, 0x56, 0x00);	/* PCI INTB#/C# Routing */
> pci_write_config8(dev, 0x57, 0x00);	/* PCI INTD# Routing */
>
> +}
> +/*

/** rather.

(i.e. make it a Doxygen comment)
> Set up the power management capabilities directly into ACPI mode.  This
> avoids having to handle any System Management Interrupts (SMI's) which I
> can't figure out how to do !!!!

Not sure we want _this_ in the comments ;)
> */
>
> +void setup_pm(device_t dev)
> +{
> /* Debounce LID and PWRBTN# Inputs for 16ms */
> pci_write_config8(dev, 0x80, 0x20);
> /* Set ACPI base address to IO VT8237R_ACPI_IO_BASE */
> pci_write_config16(dev, 0x88, VT8237R_ACPI_IO_BASE | 0x1);
> /* set ACPI to 9, need to 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=Internall PLL reset from susp
>   2= GPO2 is GPIO
> */

Linux-style comments please.

/* 7=SMBus Clock from RTC 32.768KHz
 * 5=Internall 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
> */

Ditto.
> 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);
>
> +}
>
> +static void vt8237r_init(struct device *dev)
> +{
> u8 enables, byte;
> printk_debug("vt8237r init\n");

"VT8237R init", or maybe drop the whole comment?
> /* 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 */

Am I reading this correctly? This SB supports 4MB ROM chips then?
> /* pci_write_config8(dev, 0x41, 0x7f); */
> /* Set bit 6 of 0x40, because Award does it (IO recovery time)

"because AWARD does it" no good...
>   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);

Oh, and please put an empty line after such "blocks" of
enable-this-or-that lines... (easier to read)
> /* Delay Transaction Control */
> pci_write_config8(dev, 0x43, 0xb);
> /* IO Recovery time */
> pci_write_config8(dev, 0x4c, 0x44);
> /* ROM Memory Cycles Go To LPC */
> pci_write_config8(dev, 0x59, 0x80);
> /* bypass 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: */
> pci_write_config32(dev, 0x68, (VT8237R_HPET_ADDR | 0x80));

Maybe make it configurable via an hpet_enable variable?
> /* Power management setup */
> setup_pm(dev);
> /* Start the rtc */

RTC
> rtc_init(0);
>
> +}
>
> +void vt8237r_read_resources(device_t 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.
> */
>
> +void vt8237r_enable_resources(device_t dev)
> +{
> pci_dev_enable_resources(dev);
> enable_childrens_resources(dev);
>
> +}
>
> +static void init_keyboard(struct device *dev)
> +{
> u8 regval;
> regval = pci_read_config8(dev, 0x51);
> if (regval & 0x1)
> init_pc_keyboard(0x60, 0x64, 0);
>
> +}

Hm, this is superio stuff usually. As the VT8237R integrates these kinds
of functions I'm not sure how to procede. Extract the Superio-related
stuff into src/superio/via/vt8237r/* or leave it here?

Opinions?
> +static void southbridge_init(struct device *dev)
> +{
> vt8237r_init(dev);
> pci_routing_fixup(dev);
> setup_ioapic(0xfec00000);

Make 0xfec00000 a #define in *.h? It cannot change anyway right?
In that case you can also drop the function parameter.
> setup_i8259();
> init_keyboard(dev);

What about other superio-stuff? Not done at all? Not available in
the VT8237R?

Yep, looks committable. Please repost with above fixes and Peters fixes.

Uwe.