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 <[EMAIL PROTECTED]>
> +##
> +## 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 <[EMAIL PROTECTED]>
> + *
> + * 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 <[EMAIL PROTECTED]>
> + *
> + * 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 <[EMAIL PROTECTED]>
> + * Copyright (C) 2007 Rudolf Marek <[EMAIL PROTECTED]>
> + *
> + * 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 <[EMAIL PROTECTED]>
> + *
> + * 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 <[EMAIL PROTECTED]>
> + *
> + * 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 <[EMAIL PROTECTED]>
> + *
> + * 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 <[EMAIL PROTECTED]>
> + *
> + * 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
signature.asc
Description: Digital signature
-- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios
