[v3] VT8237R support

[Corey Osgood]

See attached. Not perfect, but enough to get started with.

[Corey Osgood]

And here with the actual patch.

On Mon, Mar 24, 2008 at 2:07 AM, Corey Osgood <[e-mail]>
wrote:
> See attached. Not perfect, but enough to get started with.

[Carl-Daniel Hailfinger]

Hi Corey,

this seems to have slipped through the cracks.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

[ron minnich]

I understand the complaints about magic numbers but here's a principal
I'd like to stick to:
if the driver comes from v2, and there was magic number in it, it was
accepted in v2.
SO:
If we want to ask for this for each magic
#warning please fix this magic number

That's fine.

BUT:
we should not require that we fix each and every magic # when they've
been in v2 for 6 years in some cases. We can't fix every v2 flaw on
the first try.

Plus, sometimes the magic numbers are magic at the request of the vendor ..

Just my $.02 :=)

And, corey, thanks. I am sensing some increasing v3 action on this
list. It's nice to see.

ron

[Rudolf Marek]

Hi all,

Maybe someone with more time can take mine wip2.patch with VT8237S support and 
then port it to v3 all-together. Most of the code VT8237R is bit redone, so 
perhaps it is waste of time to change the code right now. Maybe commit it as-is 
and then add support for rev S (port from v2?)

Please check the latest patch wip2.patch in thread Re: [coreboot] K8M890 VT8237S 
status update

Ideas how to proceed?

Thanks,
Rudolf

[ron minnich]

On Sun, Aug 10, 2008 at 1:32 PM, Rudolf Marek <[e-mail]> wrote:
> Hi all,
>
> Maybe someone with more time can take mine wip2.patch with VT8237S support
> and then port it to v3 all-together. Most of the code VT8237R is bit redone,
> so perhaps it is waste of time to change the code right now. Maybe commit it
> as-is and then add support for rev S (port from v2?)
>
> Please check the latest patch wip2.patch in thread Re: [coreboot] K8M890
> VT8237S status update
>
> Ideas how to proceed?

I appreciate the idea but right now ... if there is a patch here it
wins. I would love to see your code at some point as well if someone
does find the time.

Many thanks

ron

[Corey Osgood]

On Sun, Aug 10, 2008 at 3:30 PM, Carl-Daniel Hailfinger
<[e-mail]> wrote:
> Hi Corey,
>
> this seems to have slipped through the cracks.
>
> On 24.03.2008 07:08, Corey Osgood wrote:
> > Add preliminary, as yet untested support for the vt8237r, based on the code in
> > coreboot v2. This will need some fixes, but at the very least it compiles and
> > should do smbus operations.
> >
> > Signed-off-by: Corey Osgood <[e-mail]>
>
> Can you repost an updated patch? V3 has changed quite a bit in the last
> few months, especially considering PCI access functions and other stuff.

I haven't been following v3 development lately, been too busy. I'll
try to bring this up as soon as I get a chance.

---snip---
> > +     l = (unsigned long *)ioapic_base;
> > +
> > +     /* Set APIC to FSB message bus. */
> > +     l[0] = 0x3;
>
> Magic values...
> > +     val = l[4];
> > +     l[4] = (val & 0xFFFFFE) | 1;
>
> The line above is equivalent to
> l[4] = (val & 0xffffff) | 1;
>
> You decide which one is clearer. Anyway, the 0xFFFFFE is magic.
> > +
> > +     /* Set APIC ADDR - this will be VT8237R_APIC_ID. */
> > +     l[0] = 0;
> > +     val = l[4];
> > +     l[4] = (val & 0xF0FFFF) | (VT8237R_APIC_ID << 24);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ioapic_table); i++) {
> > +             l[0] = (ioapic_table[i].reg * 2) + 0x10;
> > +             l[4] = ioapic_table[i].value_low;
> > +             value_low = l[4];
> > +             l[0] = (ioapic_table[i].reg * 2) + 0x11;
> > +             l[4] = ioapic_table[i].value_high;
> > +             value_high = l[4];
> > +
> > +             if ((i == 0) && (value_low == 0xffffffff)) {
> > +                     printk_warning("IO APIC not responding.\n");
> > +                     return;
> > +             }
> > +     }
>
> Reading the code sequence above hurts. You set l[0] and l[4] always in
> combination. What about helper functions like set_apic() and get_apic()?

That would be Rudolf's migraine :p I frankly don't really understand
APIC, so I honestly have no idea what's going on there, that was
pulled right out of v2. I'll try to fix everything else up though.

-Corey

[Corey Osgood]

On Sun, Aug 10, 2008 at 3:30 PM, Carl-Daniel Hailfinger
<[e-mail]> wrote:
> Hi Corey,
>
> this seems to have slipped through the cracks.
>
> On 24.03.2008 07:08, Corey Osgood wrote:
> > Add preliminary, as yet untested support for the vt8237r, based on the code in
> > coreboot v2. This will need some fixes, but at the very least it compiles and
> > should do smbus operations.
> >
> > Signed-off-by: Corey Osgood <[e-mail]>
>
> Can you repost an updated patch? V3 has changed quite a bit in the last
> few months, especially considering PCI access functions and other stuff.

I haven't been following v3 development lately, been too busy. I'll
try to bring this up as soon as I get a chance.

---snip---
> > +     l = (unsigned long *)ioapic_base;
> > +
> > +     /* Set APIC to FSB message bus. */
> > +     l[0] = 0x3;
>
> Magic values...
> > +     val = l[4];
> > +     l[4] = (val & 0xFFFFFE) | 1;
>
> The line above is equivalent to
> l[4] = (val & 0xffffff) | 1;
>
> You decide which one is clearer. Anyway, the 0xFFFFFE is magic.
> > +
> > +     /* Set APIC ADDR - this will be VT8237R_APIC_ID. */
> > +     l[0] = 0;
> > +     val = l[4];
> > +     l[4] = (val & 0xF0FFFF) | (VT8237R_APIC_ID << 24);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ioapic_table); i++) {
> > +             l[0] = (ioapic_table[i].reg * 2) + 0x10;
> > +             l[4] = ioapic_table[i].value_low;
> > +             value_low = l[4];
> > +             l[0] = (ioapic_table[i].reg * 2) + 0x11;
> > +             l[4] = ioapic_table[i].value_high;
> > +             value_high = l[4];
> > +
> > +             if ((i == 0) && (value_low == 0xffffffff)) {
> > +                     printk_warning("IO APIC not responding.\n");
> > +                     return;
> > +             }
> > +     }
>
> Reading the code sequence above hurts. You set l[0] and l[4] always in
> combination. What about helper functions like set_apic() and get_apic()?

That would be Rudolf's migraine :p I frankly don't really understand
APIC, so I honestly have no idea what's going on there, that was
pulled right out of v2. I'll try to fix everything else up though.

-Corey

[Corey Osgood]

On Sun, Aug 10, 2008 at 3:30 PM, Carl-Daniel Hailfinger
<[e-mail]> wrote:
> Hi Corey,
>
> this seems to have slipped through the cracks.
>
> On 24.03.2008 07:08, Corey Osgood wrote:
> > Add preliminary, as yet untested support for the vt8237r, based on the code in
> > coreboot v2. This will need some fixes, but at the very least it compiles and
> > should do smbus operations.
> >
> > Signed-off-by: Corey Osgood <[e-mail]>
>
> Can you repost an updated patch? V3 has changed quite a bit in the last
> few months, especially considering PCI access functions and other stuff.

I haven't been following v3 development lately, been too busy. I'll
try to bring this up as soon as I get a chance.

---snip---
> > +     l = (unsigned long *)ioapic_base;
> > +
> > +     /* Set APIC to FSB message bus. */
> > +     l[0] = 0x3;
>
> Magic values...
> > +     val = l[4];
> > +     l[4] = (val & 0xFFFFFE) | 1;
>
> The line above is equivalent to
> l[4] = (val & 0xffffff) | 1;
>
> You decide which one is clearer. Anyway, the 0xFFFFFE is magic.
> > +
> > +     /* Set APIC ADDR - this will be VT8237R_APIC_ID. */
> > +     l[0] = 0;
> > +     val = l[4];
> > +     l[4] = (val & 0xF0FFFF) | (VT8237R_APIC_ID << 24);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(ioapic_table); i++) {
> > +             l[0] = (ioapic_table[i].reg * 2) + 0x10;
> > +             l[4] = ioapic_table[i].value_low;
> > +             value_low = l[4];
> > +             l[0] = (ioapic_table[i].reg * 2) + 0x11;
> > +             l[4] = ioapic_table[i].value_high;
> > +             value_high = l[4];
> > +
> > +             if ((i == 0) && (value_low == 0xffffffff)) {
> > +                     printk_warning("IO APIC not responding.\n");
> > +                     return;
> > +             }
> > +     }
>
> Reading the code sequence above hurts. You set l[0] and l[4] always in
> combination. What about helper functions like set_apic() and get_apic()?

That would be Rudolf's migraine :p I frankly don't really understand
APIC, so I honestly have no idea what's going on there, that was
pulled right out of v2. I'll try to fix everything else up though.

-Corey

PS: sorry if this gets sent more then once, gmail is acting up today :(

[bari]

Corey Osgood wrote:
> On Sun, Aug 10, 2008 at 3:30 PM, Carl-Daniel Hailfinger
> <[e-mail]> wrote:
> > Hi Corey,
> >
> > this seems to have slipped through the cracks.
> >
> > On 24.03.2008 07:08, Corey Osgood wrote:
> > > Add preliminary, as yet untested support for the vt8237r, based on the code in
> > > coreboot v2. This will need some fixes, but at the very least it compiles and
> > > should do smbus operations.
> > >
> > > Signed-off-by: Corey Osgood <[e-mail]>
> >
> > Can you repost an updated patch? V3 has changed quite a bit in the last
> > few months, especially considering PCI access functions and other stuff.
>
> I haven't been following v3 development lately, been too busy. I'll
> try to bring this up as soon as I get a chance.

The current vt8237x tree has changed quite a bit (vt8237s is being added 
as well) and will have a few more changes. Let us know when/if you have 
time and we will hopefully have a nice working V2 by then.

-Bari

[Corey Osgood]

Quick question: what southbridge should I be looking at as a good
example of how trying to do things? Does amd/8111 have a correct
dts/Kconfig setup?

Thanks,
Corey

On Tue, Aug 12, 2008 at 11:22 PM, bari <[e-mail]> wrote:
> Corey Osgood wrote:
> > On Sun, Aug 10, 2008 at 3:30 PM, Carl-Daniel Hailfinger
> > <[e-mail]> wrote:
> > > Hi Corey,
> > >
> > > this seems to have slipped through the cracks.
> > >
> > > On 24.03.2008 07:08, Corey Osgood wrote:
> > > > Add preliminary, as yet untested support for the vt8237r, based on the
> > > > code in
> > > > coreboot v2. This will need some fixes, but at the very least it
> > > > compiles and
> > > > should do smbus operations.
> > > >
> > > > Signed-off-by: Corey Osgood <[e-mail]>
> > >
> > > Can you repost an updated patch? V3 has changed quite a bit in the last
> > > few months, especially considering PCI access functions and other stuff.
> >
> > I haven't been following v3 development lately, been too busy. I'll
> > try to bring this up as soon as I get a chance.
>
> The current vt8237x tree has changed quite a bit (vt8237s is being added as
> well) and will have a few more changes. Let us know when/if you have time
> and we will hopefully have a nice working V2 by then.
>
> -Bari

[ron minnich]

On Thu, Aug 28, 2008 at 12:21 AM, Corey Osgood <[e-mail]> wrote:
> Quick question: what southbridge should I be looking at as a good
> example of how trying to do things? Does amd/8111 have a correct
> dts/Kconfig setup?

yeah, 8111 I think is the best example for now.

ron