Re: [LinuxBIOS] [PATCH] VIA VT8237R support

[Rudolf Marek]

Hello all,

Uwe Hermann wrote:
> 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

Will fix.
> > +{
> > 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?

I have in plan to get rid of this function later, when I know that all writes 
succeed.
> > +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.

I will move this one there...
> > +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.
>
> */

Yes I fixed that already but with /**
    			   *
> > 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?

I have LAN disabled, cause it is not connected to any connector. AC97/UHCI/EHCI 
works, M97 should too. No settings necessary so far.
> > 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.

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

ok
> > 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)?

I have in plan to get rid of it.
> > /* 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.

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

I just dropped that.
> > 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?

yes
> > +#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?

hmm this is first DIMM address, which is allowed in general. No eeprom will 
occupy lower (except for the write protect control)
> > +/* 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.

ok
> > +/* TODO: make define? */
> > +#define SMBUS_DELAY() inb(0x80)
>
> Huh? It already is...

heh true.
> > +static void smbus_print_error(u8 host_status_register,
> > 	      int loops)
>
> "int loops" can go in the same line.

ok
> > +{
> > /* 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.

ok I may short it.
> >    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");

Hmm imho this did Corey, well I will leave it as it is for now.
> > +}
> >
> > +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?

nope, it is RWC
> > PRINT_DEBUG("\r\n");
> >
> > +}
> > +/* Public functions */
>
> Not needed.

ok
> > +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)?

It tells to start the byte data transaction, whit start bit set.
> > 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?

Hmm dont know, but I think this is safer. (this code wrote Corey IMHO)
> > +}
> >
> > +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).

I will put there better comment + some name.
> > /* Write SMBus IO base to 0xd0, and enable SMBus */
> > pci_write_config16(dev, 0xd0, VT8237R_SMBUS_IO_BASE | 0x1);
>
> 0xd0 -> SMBUS_IO_BASE_REG

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

I will check what I'm setting up later in my LPC code.
> > 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?

Should not harm. Lets see if this work for Corey.
> > /* 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?

If it works then why not. I was not sure if I can use it here.
> > 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.

ok
> > +{
> > print_debug("B188 device dump\n");
>
> B188?

it is the device ID. I dont know what it does....
> > /* 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[] = {
>    // ...
>   }

ok will take a look
> > /* 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).

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

will take a look.
> > 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.

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

do we have it in v2?
> >     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 ;)

Heh I did not wrote this ;)  I will drop it. Anyway we have no SMM support in LB 
anyway and this is the only way how to avoid that.
> > */
> >
> > +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.

ok.
> /* 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.

ok
> > 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?

ok
> > /* 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?

Yes - FFFFFFFFh to FFC00000h. But the memory space it decodes it strapped. So 
perhaps for LB I should set in CAR to maximum value.
> > /* pci_write_config8(dev, 0x41, 0x7f); */
> > /* Set bit 6 of 0x40, because Award does it (IO recovery time)
>
> "because AWARD does it" no good...

Hmm this comment was not written by me, but this nbit should be set.
> >   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)

ok
> > /* 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?

could be, but this is the default HPET address for all x86.
> > /* Power management setup */
> > setup_pm(dev);
> > /* Start the rtc */
>
> RTC

ok
> > 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?

it has just RTC and keyboard. No other stuff.
> > +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.

Yes I forgot this here, I will fix.
> > setup_i8259();
> > init_keyboard(dev);
>
> What about other superio-stuff? Not done at all? Not available in
> the VT8237R?

No...

Rudolf