[LinuxBIOS] [PATCH] VIA VT8237R support

Uwe Hermann uwe at hermann-uwe.de
Sat Oct 27 19:30:22 CEST 2007


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 <r.marek at assembler.cz>
> +##
> +## 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 <r.marek at assembler.cz>
> + *
> + * 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 <r.marek at assembler.cz>
> + *
> + * 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 <corey_osgood at verizon.net>
> + * Copyright (C) 2007 Rudolf Marek <r.marek at assembler.cz>
> + *
> + * 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 <r.marek at assembler.cz>
> + *
> + * 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 <r.marek at assembler.cz>
> + *
> + * 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 <r.marek at assembler.cz>
> + *
> + * 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 <r.marek at assembler.cz>
> + *
> + * 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.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071027/90e433d8/attachment.sig>


More information about the coreboot mailing list