[LinuxBIOS] [PATCH] VIA VT8237R support

[Rudolf Marek]

Hello,

Its Friday night... I have no date...
I think I fixed everything what Uwe wanted. Here comes much better patch (after 
five hours ;)

Signed-off-by: Rudolf Marek

Corey,

I fixed the SMBus so it does work from the beginning. Please can you test on 
your side too?

Rudolf

[Peter Stuge]

On Sat, Oct 27, 2007 at 01:16:49AM +0200, Rudolf Marek wrote:
> +/*
> Datasheets: http://www.via.com.tw/en/downloads/datasheets/chipsets/
> 			VT8237R_SouthBridge_Revision2.06_Lead-Free.zip
>
> +*/

The second line is very long.
> +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
> */
> 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);

I think these should be a doxygen comment around the declaration of
the fn_ctrl_lo/hi members in src/southbridge/via/vt8237r/chip.h.
> /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */

How is that done?
> 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");

Looks like size-optimizing? Are you sure this is good? It's a little
more difficult to read. (Might just be copypaste and not your idea?)
> printk_debug("enables in reg 0x40 read back as 0x%x\n", enables);

I want 0x%02x but that may just be me.
> /* standard bios sets master bit. */
> enables = pci_read_config8(dev, PCI_COMMAND);
> enables |= PCI_COMMAND_MASTER | PCI_COMMAND_IO;

Discussion please? Try to find out why this is a good idea.
Is this enabling bus mastering? Why would that be a bad idea?
Please try to not use factory BIOS as justification until we've
run out of ideas completely.
> +#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

..
> PRINT_DEBUG("After reset status: ");
> PRINT_DEBUG_HEX16(inb(SMBHSTSTAT));
> PRINT_DEBUG("\r\n");
>
> +}
>
> +/* Public functions */
> +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");

..this is confusing. Are macros case insensitive? Please unify this a
little.
> +struct southbridge_via_vt8237r_config {
> /* function enable bits, check vt8237r.c for details */
> unsigned int fn_ctrl_lo;
> 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;

Please improve the _cable name. ide0_80pincable or something. No need
for comment then, and more clear code. :)
> +static void br_enable(struct device *dev)
> +{
> print_debug("B188 device dump\n");
> /* 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);
>
> +}

Ok. Can you add a reference? Maybe a page or section in the data
sheet?
> +static struct ioapicreg ioapicregvalues[] = {

..
> {23, DISABLED, NONE},
> /* Be careful and don't write past the end... */

Please clarify what this comment means.
> if ((i == 0) && (value_low == 0xffffffff)) {
> 	printk_warning("IO APIC not responding.\n");

How does this work? The code seems to just do some memory accesses?
Is the IO APIC memory mapped or how does the query/response work?
> +#define PCI_DEVICE_ID_VIA_K8T890CE_0	0x0238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_1	0x1238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_2	0x2238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_3	0x3238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_4	0x4238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_5	0x5238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_7	0x7238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEG	0xa238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX0	0xc238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX1	0xd238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX2	0xe238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX3	0xf238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_BR	0xb188

Do these really go in this patch?

Looks good otherwise!

//Peter

[Rudolf Marek]

Hi Peter,
> The second line is very long.

Ok.
> I think these should be a doxygen comment around the declaration of
> the fn_ctrl_lo/hi members in src/southbridge/via/vt8237r/chip.h.

Ok.
> > /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */
>
> How is that done?

Some bits in chipset.
> > 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");
>
> Looks like size-optimizing? Are you sure this is good? It's a little
> more difficult to read. (Might just be copypaste and not your idea?)

copypaste ;)
> > printk_debug("enables in reg 0x40 read back as 0x%x\n", enables);
>
> I want 0x%02x but that may just be me.

yep good idea.
> > /* standard bios sets master bit. */
> > enables = pci_read_config8(dev, PCI_COMMAND);
> > enables |= PCI_COMMAND_MASTER | PCI_COMMAND_IO;
>
> Discussion please? Try to find out why this is a good idea.
> Is this enabling bus mastering? Why would that be a bad idea?
> Please try to not use factory BIOS as justification until we've
> run out of ideas completely.

Well the older VIA driver in LB does it too. I think linux will switch bus 
master when needed. I think we can drop this.
> > +#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
>
> ..
> > PRINT_DEBUG("After reset status: ");
> > PRINT_DEBUG_HEX16(inb(SMBHSTSTAT));
> > PRINT_DEBUG("\r\n");
> >
> > +}
> >
> > +/* Public functions */
> > +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");
>
> ..this is confusing. Are macros case insensitive? Please unify this a
> little.

This is because most of file can switch debug off but here it is wrong. I will fix.
> Please improve the _cable name. ide0_80pincable or something. No need
> for comment then, and more clear code. :)

Ok.
> > +static void br_enable(struct device *dev)
> > +{
> > print_debug("B188 device dump\n");
> > /* 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);
> >
> > +}
>
> Ok. Can you add a reference? Maybe a page or section in the data
> sheet?

I can't. I have no information on this of any kind. Except those values.
> > +static struct ioapicreg ioapicregvalues[] = {
>
> ..
> > {23, DISABLED, NONE},
> > /* Be careful and don't write past the end... */
>
> Please clarify what this comment means.

Well this is cut and paste and it means that you may get into trouble if you go 
past the address space.
> > if ((i == 0) && (value_low == 0xffffffff)) {
> > 	printk_warning("IO APIC not responding.\n");
>
> How does this work? The code seems to just do some memory accesses?

yes
> Is the IO APIC memory mapped or how does the query/response work?

You have there two pair of regs. Addr and data. Perhaps if you read back 
0xffffffff you get in trouble... This is also from other VIA SB code.
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_0	0x0238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_1	0x1238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_2	0x2238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_3	0x3238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_4	0x4238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_5	0x5238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_7	0x7238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_PEG	0xa238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX0	0xc238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX1	0xd238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX2	0xe238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX3	0xf238
> > +#define PCI_DEVICE_ID_VIA_K8T890CE_BR	0xb188
>
> Do these really go in this patch?

Well I forgot last time ;) But some of those belong here.

I will try to fix this and generate new patch.
> Looks good otherwise!

Good to hear, I spent quite a lot of time fixing it ;)

Rudolf

[Uwe Hermann]

On Sat, Oct 27, 2007 at 05:21:17PM +0200, Rudolf Marek wrote:
> > > 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");
> >
> > Looks like size-optimizing? Are you sure this is good? It's a little
> > more difficult to read. (Might just be copypaste and not your idea?)
>
> copypaste ;)

This code is ok IMO. It's from my 82371EB patch a while a ago I think,
where I used two normal printk_info()s which made the code unnecessarily
bigger, as spotted by Stefan. The above solution fixed that.

Maybe even "enabled" -> "on" and "disabled" -> "off"? Saveѕ some more
bytes and we lose no information whatsoever.
> > > /* standard bios sets master bit. */
> > > enables = pci_read_config8(dev, PCI_COMMAND);
> > > enables |= PCI_COMMAND_MASTER | PCI_COMMAND_IO;
> >
> > Discussion please? Try to find out why this is a good idea.
> > Is this enabling bus mastering? Why would that be a bad idea?
> > Please try to not use factory BIOS as justification until we've
> > run out of ideas completely.

Full ACK.
> Well the older VIA driver in LB does it too. I think linux will switch bus 
> master when needed. I think we can drop this.

Only if non-Linux payloads are tested/guaranteed to be able to cope with
that, too.

Uwe.

[ron minnich]

one thing though. We can NEVER put a patch in that sets bus master.
Any BIOS that does this is broken, and any OS that requires it is
broken.

Thanks!

ron

[Stefan Reinauer]

* ron minnich  [071027 19:15]:
> one thing though. We can NEVER put a patch in that sets bus master.
> Any BIOS that does this is broken, and any OS that requires it is
> broken.

Which standard says this?

[ron minnich]

On 10/28/07, Stefan Reinauer  wrote:
> ron minnich  [071027 19:15]:
> > one thing though. We can NEVER put a patch in that sets bus master.
> > Any BIOS that does this is broken, and any OS that requires it is
> > broken.
>
> Which standard says this?

none. But this is a frequent discussion on this list over the years.
It's very dangerous to enable bus mastering on a device and enable it
as well. You are inviting the device to do I/O to your memory at that
point. If it is after a reset, or some other unclean shutdown (e.g.
panic) and the device is in an intermediate state, anything can
happen. That is why (e.g.) several kernels (including LInux) had to be
re-engineered a bit to properly shut down PCI devices on modunload.
This problem became particularly important when kexec came along.

Our conclusion from a few years back is that we could and would (and
should) let the OS enable bus master.

ron

[Uwe Hermann]

Here's one more review:

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

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

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

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

Use Linux-style comments please:

/*
 * Foo.
 * Bar.
 */
> pci_write_config8(dev, 0x50, sb->fn_ctrl_lo);
> /*
> 7 USB Device Mode 1=dis
> 6 Reserved  
> 5 Internal LAN Controller Clock Gating 1=gated
> 4 Internal LAN Controller 1=di
> 3 Internal RTC 1=en
> 2 Internal PS2 Mouse 1=en
> 1 Internal KBC Configuration 0=dis ports 0x2e/0x2f off 0xe0-0xef
> 0 Internal Keyboard Controller 1=en
> */
> pci_write_config8(dev, 0x51, sb->fn_ctrl_hi);
> /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */
>
> +}
>
> +struct chip_operations southbridge_via_vt8237r_ops = {
> CHIP_NAME("VIA VT8237R Southbridge")
>    .enable_dev = vt8237r_enable,
>
> +};
> Index: src/southbridge/via/vt8237r/Config.lb
> ===================================================================
> --- src/southbridge/via/vt8237r/Config.lb	(revision 0)
> +++ src/southbridge/via/vt8237r/Config.lb	(revision 0)
> @@ -0,0 +1,24 @@
> +##
> +## This file is part of the LinuxBIOS project.
> +##
> +## Copyright (C) 2007 Rudolf Marek 
> +##
> +## This program is free software; you can redistribute it and/or modify
> +## it under the terms of the GNU General Public License v2 as published by
> +## the Free Software Foundation.
> +##
> +## This program is distributed in the hope that it will be useful,
> +## but WITHOUT ANY WARRANTY; without even the implied warranty of
> +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +## GNU General Public License for more details.
> +##
> +## You should have received a copy of the GNU General Public License
> +## along with this program; if not, write to the Free Software
> +## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> +##
> +config chip.h
> +driver vt8237r.o
> +driver vt8237r_ide.o
> +driver vt8237r_lpc.o
> +driver vt8237r_sata.o
> +driver vt8237r_bridge.o

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

Don't put this in the license header please, move it somewhere below as
a normal comment.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <console/console.h>
> +#include "vt8237r.h"
> +#include "chip.h"
>
> +#define IDE_CS		0x40
> +#define IDE_CONF_I	0x41
> +#define IDE_CONF_II	0x42
> +#define IDE_CONF_FIFO	0x43
> +#define IDE_MISC_I	0x44
> +#define IDE_MISC_II	0x45
> +#define IDE_UDMA	0x50

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

Not needed IMHO if the other lines are printed already.
> printk_info("%s IDE interface %s\n", "Primary",
>     sb->ide0_enable ? "enabled" : "disabled");
> printk_info("%s IDE interface %s\n", "Secondary",
>     sb->ide1_enable ? "enabled" : "disabled");
> enables = pci_read_config8(dev, IDE_CS) & ~0x3;
> enables |= (sb->ide0_enable << 1) | sb->ide1_enable;
> pci_write_config8(dev, IDE_CS, enables);
> enables = pci_read_config8(dev, IDE_CS);
> printk_debug("enables in reg 0x40 read back as 0x%x\n", enables);

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

Line too long, and it should start with capital letter and end with full stop.
> enables = pci_read_config8(dev, IDE_CONF_II);
> enables &= ~0xc0;
> pci_write_config8(dev,IDE_CONF_II, enables);
> enables = pci_read_config8(dev, IDE_CONF_II);
> printk_debug("enables in reg 0x42 read back as 0x%x\n", enables);
> /* Enable prefetch buffers */
> enables = pci_read_config8(dev, IDE_CONF_I);
> enables |= 0xf0;
> pci_write_config8(dev, IDE_CONF_I, enables);
> /* flush FIFOs at half */
> enables = pci_read_config8(dev, IDE_CONF_FIFO);
> enables &= 0xf0;
> enables |= (1 << 2) | (1 << 0);
> pci_write_config8(dev, IDE_CONF_FIFO, enables);
> /* PIO read prefetch counter, Bus Master IDE Status Register Read Retry */
> enables = pci_read_config8(dev, IDE_MISC_I);
> enables &= 0xe2;
> enables |= (1 << 4) | (1 << 3);
> pci_write_config8(dev, IDE_MISC_I, enables);
> /* Use memory read multiple, Memory-Write-and-Invalidate */
> enables = pci_read_config8(dev, IDE_MISC_II);
> enables |= (1 << 2) | (1 << 3);
> pci_write_config8(dev, IDE_MISC_II, enables);
> /* address decoding */
> /* disable the BARs, we use std ports */

Why? Just curious...
> pci_write_config32(dev, PCI_BASE_ADDRESS_0, 0x0);
> pci_write_config32(dev, PCI_BASE_ADDRESS_1, 0x0);
> pci_write_config32(dev, PCI_BASE_ADDRESS_2, 0x0);
> pci_write_config32(dev, PCI_BASE_ADDRESS_3, 0x0);
> /* Force interrupts to use compat mode */
> pci_write_config8(dev, PCI_INTERRUPT_PIN, 0x0);
> pci_write_config8(dev, PCI_INTERRUPT_LINE, 0xff);
> /* standard bios sets master bit. */
> enables = pci_read_config8(dev, PCI_COMMAND);
> enables |= PCI_COMMAND_MASTER | PCI_COMMAND_IO;
> pci_write_config8(dev, PCI_COMMAND, enables);
> enables = pci_read_config8(dev, 0x4);
> printk_debug("command in reg 0x4 reads back as 0x%x\n", enables);
> /* cable guy */
> cablesel = pci_read_config32(dev, IDE_UDMA);
> cablesel &= ~((1 << 28) | (1 << 20) | (1 <<12) | (1 << 4));
> cablesel |= (sb->ide0_cable << 28) | (sb->ide0_cable << 20) |
> 	(sb->ide1_cable << 12) | (sb->ide1_cable << 4);
> pci_write_config32(dev, IDE_UDMA, cablesel);
>
> +}
>
> +static struct device_operations ide_ops = {
> .read_resources = pci_dev_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init = ide_init,
> .enable = 0,
> .ops_pci = 0,
>
> +};
>
> +static struct pci_driver northbridge_driver __pci_driver = {
> .ops = &ide_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_82C586_1,
>
> +};
> Index: src/southbridge/via/vt8237r/vt8237r.h
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r.h	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r.h	(revision 0)
> @@ -0,0 +1,28 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* static resources for VT8237R southbridge */
>
> +#define VT8237R_APIC_ID 	0x2
> +#define VT8237R_ACPI_IO_BASE 	0x500
> +#define VT8237R_SMBUS_IO_BASE	0x400
> +/* 0x0 disabled, 0x2 reserved 0xf = IRQ15 */

^
                           missing comma?
> +#define VT8237R_ACPI_IRQ 	0x9
> +#define VT8237R_HPET_ADDR  	0xfed00000ULL
> +#define VT8237R_APIC_BASE	0xfec00000ULL
> Index: src/southbridge/via/vt8237r/vt8237r_early_smbus.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_early_smbus.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_early_smbus.c	(revision 0)
> @@ -0,0 +1,172 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Corey Osgood
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <device/pci_ids.h>
> +#include "vt8237r.h"
>
> +#define SMBHSTSTAT		VT8237R_SMBUS_IO_BASE + 0x0
> +#define SMBSLVSTAT		VT8237R_SMBUS_IO_BASE + 0x1
> +#define SMBHSTCTL		VT8237R_SMBUS_IO_BASE + 0x2
> +#define SMBHSTCMD		VT8237R_SMBUS_IO_BASE + 0x3
> +#define SMBXMITADD		VT8237R_SMBUS_IO_BASE + 0x4
> +#define SMBHSTDAT0		VT8237R_SMBUS_IO_BASE + 0x5
>
> +/* Define register settings */
> +#define HOST_RESET 		0xff
> +#define DIMM_BASE		(0x50<<1)

Is this really needed in vt8237r_early_smbus.c? I don't think so.
This address is dependent on the board anyway, correct?
> +/* 1 in the 0 bit of SMBHSTADD states to READ */
> +#define READ_CMD		0x01
>
> +#define SMBUS_TIMEOUT		(100*1000*10)
>
> +#define  I2C_TRANS_CMD		0x40
> +#define  CLOCK_SLAVE_ADDRESS	0x69
>
> +/* Debugging macros. Only necessary if something isn't working right */
>
> +#if DEBUG_SMBUS == 1
> +#define PRINT_DEBUG(x)		print_debug(x)
> +#define PRINT_DEBUG_HEX16(x)	print_debug_hex16(x)
> +#else
> +#define PRINT_DEBUG(x)
> +#define PRINT_DEBUG_HEX16(x)
> +#endif
> +/* Internal functions */

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

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

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

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

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

[...]
if (loops >= SMBUS_TIMEOUT)
        print_err("SMBus Timout\r\n");
if (host_status_register & (1 << 4))
        print_err("Interrup/SMI# was Failed Bus Transaction\r\n");
if (host_status_register & (1 << 3))
        print_err("Bus Error\r\n");
if (host_status_register & (1 << 2))
        print_err("Device Error\r\n");
if (host_status_register & (1 << 1))
        print_err("Interrupt/SMI# was Successful Completion\r\n");
if (host_status_register & (1 << 0))
        print_err("Host Busy\r\n");
> +}
>
> +static void smbus_wait_until_ready(void)
> +{
> int loops;
> PRINT_DEBUG("Waiting until smbus ready\r\n");
> loops = 0;
> /* Yes, this is a mess, but it's the easiest way to do it */
> while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
> ++loops;
> smbus_print_error(inb(SMBHSTSTAT), loops);
>
> +}
>
> +static void smbus_reset(void)
> +{
> outb(HOST_RESET, SMBHSTSTAT);
> /* Datasheet says we have to read it to take ownership of SMBus */
> inb(SMBHSTSTAT);
> PRINT_DEBUG("After reset status: ");
> PRINT_DEBUG_HEX16(inb(SMBHSTSTAT));

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

Not needed.
> +u8 smbus_read_byte(u32 dimm, u32 offset)
> +{
> u32 val;
> print_debug("DIMM ");
> print_debug_hex8(dimm);
> print_debug(" OFFSET ");
> print_debug_hex8(offset);
> print_debug("\r\n");
> smbus_reset();
> /* clear host data port */
> outb(0x00, SMBHSTDAT0);
> SMBUS_DELAY();
> smbus_wait_until_ready();
> /* actual addr to reg format */
> dimm = (dimm << 1);
> dimm |= 1;
> outb(dimm, SMBXMITADD);
> outb(offset, SMBHSTCMD);
> outb(0x48, SMBHSTCTL);

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

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

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

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

0xd0 -> SMBUS_IO_BASE_REG

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

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

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

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

Again? Needed?
> /* reset the internal pointer */
> inb(SMBHSTCTL);
>
> +}
> Index: src/southbridge/via/vt8237r/vt8237r_sata.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_sata.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_sata.c	(revision 0)
> @@ -0,0 +1,56 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
>
> +#define SATA_MISC_CTRL 0x45
>
> +static void sata_init(struct device *dev)
> +{
> u8 reg;
> printk_debug("Configuring VIA SATA Controller\n");
> /* class IDE Disk */
> reg = pci_read_config8(dev, SATA_MISC_CTRL);
> reg &= 0x7f;		/* Sub Class Write Protect off */
> pci_write_config8(dev, SATA_MISC_CTRL, reg);
> /* change the device class to SATA from RAID */
> pci_write_config8(dev, PCI_CLASS_DEVICE, 0x1);
> reg |= 0x80;		/* Sub Class Write Protect on */
> pci_write_config8(dev, SATA_MISC_CTRL, reg);
>
> +}
>
> +static struct device_operations sata_ops = {
> .read_resources = pci_dev_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init = sata_init,
> .enable = 0,
> .ops_pci = 0,
>
> +};
>
> +static struct pci_driver northbridge_driver __pci_driver = {
> .ops = &sata_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_VT6420_SATA,
>
> +};
> Index: src/southbridge/via/vt8237r/chip.h
> ===================================================================
> --- src/southbridge/via/vt8237r/chip.h	(revision 0)
> +++ src/southbridge/via/vt8237r/chip.h	(revision 0)
> @@ -0,0 +1,37 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#ifndef SOUTHBRIDGE_VIA_VT8237R_CHIP_H
> +#define SOUTHBRIDGE_VIA_VT8237R_CHIP_H
>
> +extern struct chip_operations southbridge_via_vt8237r_ops;
>
> +struct southbridge_via_vt8237r_config {
> /* function enable bits, check vt8237r.c for details */
> unsigned int fn_ctrl_lo;

u16?
> unsigned int fn_ctrl_hi;
> int ide0_enable:1;
> int ide1_enable:1;
> /* 1 = 80-pin cable */
> int ide0_cable:1;
> int ide1_cable:1;
>
> +};
>
> +#endif /* SOUTHBRIDGE_VIA_VT8237R_CHIP_H */
> Index: src/southbridge/via/vt8237r/vt8237r_bridge.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_bridge.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_bridge.c	(revision 0)
> @@ -0,0 +1,57 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <console/console.h>
>
> +static void br_enable(struct device *dev)

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

B188?
> /* VIA recommends this, sorry no known info */
> writeback(dev, 0x40, 0x91);
> writeback(dev, 0x41, 0x40);
> writeback(dev, 0x43, 0x44);
> writeback(dev, 0x44, 0x31);
> writeback(dev, 0x45, 0x3a);
> writeback(dev, 0x46, 0x88);	/* PCI ID lo */
> writeback(dev, 0x47, 0xb1);	/* PCI ID hi */
> writeback(dev, 0x3e, 0x16);	/* bridge control */
> dump_south(dev);
>
> +}
>
> +static struct device_operations br_ops = {
> .read_resources = pci_bus_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = pci_bus_enable_resources,
> .enable = br_enable,
> .scan_bus = pci_scan_bridge,
> .reset_bus = pci_bus_reset,
> .ops_pci = 0,
>
> +};
>
> +static struct pci_driver northbridge_driver __pci_driver = {
> .ops = &br_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_K8T890CE_BR,
>
> +};
> Index: src/southbridge/via/vt8237r/vt8237r_lpc.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_lpc.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r_lpc.c	(revision 0)
> @@ -0,0 +1,307 @@
> +/*
> This file is part of the LinuxBIOS project.
> Inspiration from other VIA SB code.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <arch/io.h>
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <pc80/mc146818rtc.h>
> +#include <cpu/x86/lapic.h>
> +#include "vt8237r.h"
> +#include "chip.h"
>
> +#define ALL		(0xff << 24)
> +#define NONE		(0)
> +#define DISABLED	(1 << 16)
> +#define ENABLED		(0 << 16)
> +#define TRIGGER_EDGE	(0 << 15)
> +#define TRIGGER_LEVEL	(1 << 15)
> +#define POLARITY_HIGH	(0 << 13)
> +#define POLARITY_LOW	(1 << 13)
> +#define PHYSICAL_DEST	(0 << 11)
> +#define LOGICAL_DEST	(1 << 11)
> +#define ExtINT		(7 << 8)
> +#define NMI		(4 << 8)
> +#define SMI		(2 << 8)
> +#define INT		(1 << 8)
>
> +struct ioapicreg {
> u32 reg;
> u32 value_low, value_high;
>
> +};
>
> +static struct ioapicreg ioapicregvalues[] = {

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

static const struct {
    u32 reg;
    u32 value_low;
    u32 value_high;
  } ioapic_table[] = {
    // ...
  }
> /* IO-APIC virtual wire mode configuration */
> /* mask, trigger, polarity, destination, delivery, vector */
> {0,
> ENABLED | TRIGGER_EDGE | POLARITY_HIGH | PHYSICAL_DEST | ExtINT,
> NONE},
> {1, DISABLED, NONE},
> {2, DISABLED, NONE},
> {3, DISABLED, NONE},
> {4, DISABLED, NONE},
> {5, DISABLED, NONE},
> {6, DISABLED, NONE},
> {7, DISABLED, NONE},
> {8, DISABLED, NONE},
> {9, DISABLED, NONE},
> {10, DISABLED, NONE},
> {11, DISABLED, NONE},
> {12, DISABLED, NONE},
> {13, DISABLED, NONE},
> {14, DISABLED, NONE},
> {15, DISABLED, NONE},
> {16, DISABLED, NONE},
> {17, DISABLED, NONE},
> {18, DISABLED, NONE},
> {19, DISABLED, NONE},
> {20, DISABLED, NONE},
> {21, DISABLED, NONE},
> {22, DISABLED, NONE},
> {23, DISABLED, NONE},
> /* Be careful and don't write past the end... */
>
> +};
> +extern void dump_south(device_t dev);

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

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

Only one command per line please.
> l[4] = (val & 0xFFFFFE) | 1;
> /* set APIC ADDR - this will be VT8237R_APIC_ID */
> l[0] = 0; val = l[4];
> l[4] = (val & 0xF0FFFF) | (VT8237R_APIC_ID << 24);
> for (i = 0;
>     i < sizeof(ioapicregvalues) / sizeof(ioapicregvalues[0]);

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

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

/** rather.

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

Not sure we want _this_ in the comments ;)
> */
>
> +void setup_pm(device_t dev)
> +{
> /* Debounce LID and PWRBTN# Inputs for 16ms */
> pci_write_config8(dev, 0x80, 0x20);
> /* Set ACPI base address to IO VT8237R_ACPI_IO_BASE */
> pci_write_config16(dev, 0x88, VT8237R_ACPI_IO_BASE | 0x1);
> /* set ACPI to 9, need to set IRQ 9 override to level!
>   set PSON gating */
> pci_write_config8(dev, 0x82, 0x40 | VT8237R_ACPI_IRQ);
> /* primary interupt channel, define wake events 0=IRQ0 15=IRQ15 1=en */
> pci_write_config16(dev, 0x84, 0x30b2);
> /* SMI output level to low, 7.5us throttle clock */
> pci_write_config8(dev, 0x8d, 0x18);
> /* GP Timer Control 1s */
> pci_write_config8(dev, 0x93, 0x88);
> /* 7=SMBus Clock from RTC 32.768KHz
>   5=Internall PLL reset from susp
>   2= GPO2 is GPIO
> */

Linux-style comments please.

/* 7=SMBus Clock from RTC 32.768KHz
 * 5=Internall PLL reset from susp
 * 2= GPO2 is GPIO
 */
> pci_write_config8(dev, 0x94, 0xa4);
> /* 7=stp to sust delay 1msec,
>   6=SUSST# Deasserted Before PWRGD for STD
>   3=GPO26/GPO27 is GPO 
>   2=Disable Alert on Lan
> */

Ditto.
> pci_write_config8(dev, 0x95, 0xcc);
> /* Disable GP3 timer */
> pci_write_config8(dev, 0x98, 0);
> /* enable SATA LED, disable special CPU Frequency Change -
>   GPIO28 GPIO22 GPIO29 GPIO23 are GPIOs */
> pci_write_config8(dev, 0xe5, 0x9);
> /* REQ5 as PCI request input - should be together with  INTE-INTH */
> pci_write_config8(dev, 0xe4, 0x4);
> /* Enable ACPI accessm RTC signal gated with PSON */
> pci_write_config8(dev, 0x81, 0x84);
> /* clear status events */
> outw(0xffff, VT8237R_ACPI_IO_BASE + 0x00);
> outw(0xffff, VT8237R_ACPI_IO_BASE + 0x20);
> outw(0xffff, VT8237R_ACPI_IO_BASE + 0x28);
> outl(0xffffffff, VT8237R_ACPI_IO_BASE + 0x30);
> /* disable SCI on GPIO */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x22);
> /* disable SMI on GPIO */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x24);
> /* disable all global enable SMIs */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x2a);
> /* all SMI off, both IDE buses ON, PSON rising edge */
> outw(0x0, VT8237R_ACPI_IO_BASE + 0x2c);
> /* primary activity SMI disable */
> outl(0x0, VT8237R_ACPI_IO_BASE + 0x34);
> /* GP timer reload on none */
> outl(0x0, VT8237R_ACPI_IO_BASE + 0x38);
> /* disable extended IO traps */
> outb(0x0, VT8237R_ACPI_IO_BASE + 0x42);
> /* SCI is generated for RTC/pwrBtn/slpBtn */
> outw(0x001, VT8237R_ACPI_IO_BASE + 0x04);
>
> +}
>
> +static void vt8237r_init(struct device *dev)
> +{
> u8 enables, byte;
> printk_debug("vt8237r init\n");

"VT8237R init", or maybe drop the whole comment?
> /* enable addr/data stepping */
> byte = pci_read_config8(dev, PCI_COMMAND);
> byte |= PCI_COMMAND_WAIT;
> pci_write_config8(dev, PCI_COMMAND, byte);
> /* enable the internal I/O decode */
> enables = pci_read_config8(dev, 0x6C);
> enables |= 0x80;
> pci_write_config8(dev, 0x6C, enables);
> /* FIXME Map 4MB of FLASH into the address space,
>   this should be in CAR call */

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

"because AWARD does it" no good...
>   IMPORTANT FIX - EISA = ECLR reg at 0x4d0! Decoding must be on so that PCI 
>   interrupts can be properly marked as level triggered. */
> enables = pci_read_config8(dev, 0x40);
> enables |= 0x44;
> pci_write_config8(dev, 0x40, enables);
> /* Line buffer control */
> enables = pci_read_config8(dev, 0x42);
> enables |= 0xf8;
> pci_write_config8(dev, 0x42, enables);

Oh, and please put an empty line after such "blocks" of
enable-this-or-that lines... (easier to read)
> /* Delay Transaction Control */
> pci_write_config8(dev, 0x43, 0xb);
> /* IO Recovery time */
> pci_write_config8(dev, 0x4c, 0x44);
> /* ROM Memory Cycles Go To LPC */
> pci_write_config8(dev, 0x59, 0x80);
> /* bypass Bypass APIC De-Assert Message,  INTE#, INTF#, INTG#, INTH# as PCI  */
> pci_write_config8(dev, 0x5B, 0xb);
> /* set  Read Pass Write Control  Enable (force A2 from APIC FSB to low) */
> pci_write_config8(dev, 0x48, 0x8c);
> /* Set 0x58 to 0x43 APIC and RTC */
> pci_write_config8(dev, 0x58, 0x43);
> /* Set bit 3 of 0x4f (use INIT# as cpu reset) */
> enables = pci_read_config8(dev, 0x4f);
> enables |= 0x08;
> pci_write_config8(dev, 0x4f, enables);
> /* enable serial irq, 6PCI clocks */
> pci_write_config8(dev, 0x52, 0x9);
> /* enable HPET at: */
> pci_write_config32(dev, 0x68, (VT8237R_HPET_ADDR | 0x80));

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

RTC
> rtc_init(0);
>
> +}
>
> +void vt8237r_read_resources(device_t dev)
> +{
> struct resource *res;
> pci_dev_read_resources(dev);
> /* fixed APIC resource */
> res = new_resource(dev, 0x44);
> res->base = VT8237R_APIC_BASE;
> res->size = 256;
> res->limit = res->base + res->size - 1;
> res->align = 8;
> res->gran = 8;
> res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
>    IORESOURCE_STORED | IORESOURCE_ASSIGNED;
>
> +}
>
> +/**
> The VT8237R is not a PCI bridge and has no resources of its own (other
> than standard PC I/O addresses), however it does control the ISA bus
> and so we need to manually call enable childrens resources on that bus.
> */
>
> +void vt8237r_enable_resources(device_t dev)
> +{
> pci_dev_enable_resources(dev);
> enable_childrens_resources(dev);
>
> +}
>
> +static void init_keyboard(struct device *dev)
> +{
> u8 regval;
> regval = pci_read_config8(dev, 0x51);
> if (regval & 0x1)
> init_pc_keyboard(0x60, 0x64, 0);
>
> +}

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

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

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

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

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

Uwe.

[Rudolf Marek]

Hello all,

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

Will fix.
> > +{
> > u8 regval;
> > pci_write_config8(dev, where, what);
> > regval = pci_read_config8(dev, where);
> > if (regval != what) {
> > print_debug("Writeback to ");
> > print_debug_hex8(where);
> > print_debug("failed ");
> > print_debug_hex8(regval);
> > print_debug("\n ");
> > }
> >
> > +}
>
> Hm, looks generally useful, why not put it in some global debug.c file?

I have in plan to get rid of this function later, when I know that all writes 
succeed.
> > +void dump_south(device_t dev)
> > +{
> > int i, j;
> > for (i = 0; i < 256; i += 16) {
> > printk_debug("%02x: ", i);
> > for (j = 0; j < 16; j++) {
> > 	printk_debug("%02x ",
> > 		     pci_read_config8(dev, i + j));
> > }
> > printk_debug("\n");
> > }
> >
> > +}
>
> Ditto. There already exists such code in (various) debug.c files in
> the repo, I think.

I will move this one there...
> > +static void vt8237r_enable(struct device *dev)
> > +{
> > struct southbridge_via_vt8237r_config *sb =
> > (struct southbridge_via_vt8237r_config *) dev->chip_info;
> > /* function disable 1=disabled
> > 7 Dev 17 fn 6 MC97
> > 6 Dev 17 fn 5 AC97
> > 5 Dev 16 fn 1 USB 1.1 UHCI Ports 2-3
> > 4 Dev 16 fn 0 USB 1.1 UHCI Ports 0-1
> > 3 Dev 15 fn 0 Serial ATA
> > 2 Dev 16 fn 2 USB 1.1 UHCI Ports 4-5
> > 1 Dev 16 fn 4 USB 2.0 EHCI
> > 0 Dev 16 fn 3 USB 1.1 UHCI Ports 6-7
> > */
>
> Use Linux-style comments please:
>
> /*
> Foo.
> Bar.
>
> */

Yes I fixed that already but with /**
    			   *
> > pci_write_config8(dev, 0x50, sb->fn_ctrl_lo);
> > /*
> > 7 USB Device Mode 1=dis
> > 6 Reserved  
> > 5 Internal LAN Controller Clock Gating 1=gated
> > 4 Internal LAN Controller 1=di
> > 3 Internal RTC 1=en
> > 2 Internal PS2 Mouse 1=en
> > 1 Internal KBC Configuration 0=dis ports 0x2e/0x2f off 0xe0-0xef
> > 0 Internal Keyboard Controller 1=en
> > */
> > pci_write_config8(dev, 0x51, sb->fn_ctrl_hi);
> > /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */
> >
> > +}
> >
> > +struct chip_operations southbridge_via_vt8237r_ops = {
> > CHIP_NAME("VIA VT8237R Southbridge")
> >    .enable_dev = vt8237r_enable,
> >
> > +};
> > Index: src/southbridge/via/vt8237r/Config.lb
> > ===================================================================
> > --- src/southbridge/via/vt8237r/Config.lb	(revision 0)
> > +++ src/southbridge/via/vt8237r/Config.lb	(revision 0)
> > @@ -0,0 +1,24 @@
> > +##
> > +## This file is part of the LinuxBIOS project.
> > +##
> > +## Copyright (C) 2007 Rudolf Marek 
> > +##
> > +## This program is free software; you can redistribute it and/or modify
> > +## it under the terms of the GNU General Public License v2 as published by
> > +## the Free Software Foundation.
> > +##
> > +## This program is distributed in the hope that it will be useful,
> > +## but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +## GNU General Public License for more details.
> > +##
> > +## You should have received a copy of the GNU General Public License
> > +## along with this program; if not, write to the Free Software
> > +## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > +##
> > +config chip.h
> > +driver vt8237r.o
> > +driver vt8237r_ide.o
> > +driver vt8237r_lpc.o
> > +driver vt8237r_sata.o
> > +driver vt8237r_bridge.o
>
> UHCI? EHCI? AC97 audio / modem? LAN? What's the status there?

I have LAN disabled, cause it is not connected to any connector. AC97/UHCI/EHCI 
works, M97 should too. No settings necessary so far.
> > Index: src/southbridge/via/vt8237r/vt8237r_ide.c
> >
> > --- src/southbridge/via/vt8237r/vt8237r_ide.c	(revision 0)
> > +++ src/southbridge/via/vt8237r/vt8237r_ide.c	(revision 0)
> > @@ -0,0 +1,124 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Based on other VIA SB code, no native mode. Interrupts from unconnected HDDs
> > might occur if IRQ14/15 is used for PCI. Therefore no native mode support.
>
> Don't put this in the license header please, move it somewhere below as
> a normal comment.

Ok
> > Copyright (C) 2007 Rudolf Marek
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License v2 as published by
> > the Free Software Foundation.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > */
> >
> > +#include <device/device.h>
> > +#include <device/pci.h>
> > +#include <device/pci_ops.h>
> > +#include <device/pci_ids.h>
> > +#include <console/console.h>
> > +#include "vt8237r.h"
> > +#include "chip.h"
> >
> > +#define IDE_CS		0x40
> > +#define IDE_CONF_I	0x41
> > +#define IDE_CONF_II	0x42
> > +#define IDE_CONF_FIFO	0x43
> > +#define IDE_MISC_I	0x44
> > +#define IDE_MISC_II	0x45
> > +#define IDE_UDMA	0x50
>
> Yep, either here or vt8237.h.
> > +static void ide_init(struct device *dev)
> > +{
> > struct southbridge_via_vt8237r_config *sb =
> > (struct southbridge_via_vt8237r_config *) dev->chip_info;
> > u8 enables;
> > u32 cablesel;
> > printk_info("Enabling VIA IDE.\n");
>
> Not needed IMHO if the other lines are printed already.

ok
> > printk_info("%s IDE interface %s\n", "Primary",
> >     sb->ide0_enable ? "enabled" : "disabled");
> > printk_info("%s IDE interface %s\n", "Secondary",
> >     sb->ide1_enable ? "enabled" : "disabled");
> > enables = pci_read_config8(dev, IDE_CS) & ~0x3;
> > enables |= (sb->ide0_enable << 1) | sb->ide1_enable;
> > pci_write_config8(dev, IDE_CS, enables);
> > enables = pci_read_config8(dev, IDE_CS);
> > printk_debug("enables in reg 0x40 read back as 0x%x\n", enables);
>
> Why not use writeback() here (and in a few more places below)?

I have in plan to get rid of it.
> > /* enable only compatibility mode, cannot use IRQ14/IRQ15 for PCI anyway */
>
> Line too long, and it should start with capital letter and end with full stop.

ok
> > enables = pci_read_config8(dev, IDE_CONF_II);
> > enables &= ~0xc0;
> > pci_write_config8(dev,IDE_CONF_II, enables);
> > enables = pci_read_config8(dev, IDE_CONF_II);
> > printk_debug("enables in reg 0x42 read back as 0x%x\n", enables);
> > /* Enable prefetch buffers */
> > enables = pci_read_config8(dev, IDE_CONF_I);
> > enables |= 0xf0;
> > pci_write_config8(dev, IDE_CONF_I, enables);
> > /* flush FIFOs at half */
> > enables = pci_read_config8(dev, IDE_CONF_FIFO);
> > enables &= 0xf0;
> > enables |= (1 << 2) | (1 << 0);
> > pci_write_config8(dev, IDE_CONF_FIFO, enables);
> > /* PIO read prefetch counter, Bus Master IDE Status Register Read Retry */
> > enables = pci_read_config8(dev, IDE_MISC_I);
> > enables &= 0xe2;
> > enables |= (1 << 4) | (1 << 3);
> > pci_write_config8(dev, IDE_MISC_I, enables);
> > /* Use memory read multiple, Memory-Write-and-Invalidate */
> > enables = pci_read_config8(dev, IDE_MISC_II);
> > enables |= (1 << 2) | (1 << 3);
> > pci_write_config8(dev, IDE_MISC_II, enables);
> > /* address decoding */
> > /* disable the BARs, we use std ports */
>
> Why? Just curious...

I just dropped that.
> > pci_write_config32(dev, PCI_BASE_ADDRESS_0, 0x0);
> > pci_write_config32(dev, PCI_BASE_ADDRESS_1, 0x0);
> > pci_write_config32(dev, PCI_BASE_ADDRESS_2, 0x0);
> > pci_write_config32(dev, PCI_BASE_ADDRESS_3, 0x0);
> > /* Force interrupts to use compat mode */
> > pci_write_config8(dev, PCI_INTERRUPT_PIN, 0x0);
> > pci_write_config8(dev, PCI_INTERRUPT_LINE, 0xff);
> > /* standard bios sets master bit. */
> > enables = pci_read_config8(dev, PCI_COMMAND);
> > enables |= PCI_COMMAND_MASTER | PCI_COMMAND_IO;
> > pci_write_config8(dev, PCI_COMMAND, enables);
> > enables = pci_read_config8(dev, 0x4);
> > printk_debug("command in reg 0x4 reads back as 0x%x\n", enables);
> > /* cable guy */
> > cablesel = pci_read_config32(dev, IDE_UDMA);
> > cablesel &= ~((1 << 28) | (1 << 20) | (1 <<12) | (1 << 4));
> > cablesel |= (sb->ide0_cable << 28) | (sb->ide0_cable << 20) |
> > 	(sb->ide1_cable << 12) | (sb->ide1_cable << 4);
> > pci_write_config32(dev, IDE_UDMA, cablesel);
> >
> > +}
> >
> > +static struct device_operations ide_ops = {
> > .read_resources = pci_dev_read_resources,
> > .set_resources = pci_dev_set_resources,
> > .enable_resources = pci_dev_enable_resources,
> > .init = ide_init,
> > .enable = 0,
> > .ops_pci = 0,
> >
> > +};
> >
> > +static struct pci_driver northbridge_driver __pci_driver = {
> > .ops = &ide_ops,
> > .vendor = PCI_VENDOR_ID_VIA,
> > .device = PCI_DEVICE_ID_VIA_82C586_1,
> >
> > +};
> > Index: src/southbridge/via/vt8237r/vt8237r.h
> > ===================================================================
> > --- src/southbridge/via/vt8237r/vt8237r.h	(revision 0)
> > +++ src/southbridge/via/vt8237r/vt8237r.h	(revision 0)
> > @@ -0,0 +1,28 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Rudolf Marek
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License v2 as published by
> > the Free Software Foundation.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > */
> >
> > +/* static resources for VT8237R southbridge */
> >
> > +#define VT8237R_APIC_ID 	0x2
> > +#define VT8237R_ACPI_IO_BASE 	0x500
> > +#define VT8237R_SMBUS_IO_BASE	0x400
> > +/* 0x0 disabled, 0x2 reserved 0xf = IRQ15 */
>                             ^
>                        missing comma?

yes
> > +#define VT8237R_ACPI_IRQ 	0x9
> > +#define VT8237R_HPET_ADDR  	0xfed00000ULL
> > +#define VT8237R_APIC_BASE	0xfec00000ULL
> > Index: src/southbridge/via/vt8237r/vt8237r_early_smbus.c
> > ===================================================================
> > --- src/southbridge/via/vt8237r/vt8237r_early_smbus.c	(revision 0)
> > +++ src/southbridge/via/vt8237r/vt8237r_early_smbus.c	(revision 0)
> > @@ -0,0 +1,172 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Corey Osgood
> > Copyright (C) 2007 Rudolf Marek
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> > the Free Software Foundation; either version 2 of the License, or
> > (at your option) any later version.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > */
> >
> > +#include <device/pci_ids.h>
> > +#include "vt8237r.h"
> >
> > +#define SMBHSTSTAT		VT8237R_SMBUS_IO_BASE + 0x0
> > +#define SMBSLVSTAT		VT8237R_SMBUS_IO_BASE + 0x1
> > +#define SMBHSTCTL		VT8237R_SMBUS_IO_BASE + 0x2
> > +#define SMBHSTCMD		VT8237R_SMBUS_IO_BASE + 0x3
> > +#define SMBXMITADD		VT8237R_SMBUS_IO_BASE + 0x4
> > +#define SMBHSTDAT0		VT8237R_SMBUS_IO_BASE + 0x5
> >
> > +/* Define register settings */
> > +#define HOST_RESET 		0xff
> > +#define DIMM_BASE		(0x50<<1)
>
> Is this really needed in vt8237r_early_smbus.c? I don't think so.
> This address is dependent on the board anyway, correct?

hmm this is first DIMM address, which is allowed in general. No eeprom will 
occupy lower (except for the write protect control)
> > +/* 1 in the 0 bit of SMBHSTADD states to READ */
> > +#define READ_CMD		0x01
> >
> > +#define SMBUS_TIMEOUT		(100*1000*10)
> >
> > +#define  I2C_TRANS_CMD		0x40
> > +#define  CLOCK_SLAVE_ADDRESS	0x69
> >
> > +/* Debugging macros. Only necessary if something isn't working right */
> >
> > +#if DEBUG_SMBUS == 1
> > +#define PRINT_DEBUG(x)		print_debug(x)
> > +#define PRINT_DEBUG_HEX16(x)	print_debug_hex16(x)
> > +#else
> > +#define PRINT_DEBUG(x)
> > +#define PRINT_DEBUG_HEX16(x)
> > +#endif
> > +/* Internal functions */
>
> Drop the above comment, not needed IMO.

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

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

ok
> > +{
> > /* Check if there actually was an error */
> > if (host_status_register == 0x00 || host_status_register == 0x40 ||
>
> Pretty long name, maybe host_status or so? It's relatively clear we talk
> about a register.

ok I may short it.
> >    host_status_register == 0x42)
> > return;
> > if (loops >= SMBUS_TIMEOUT) {
> > print_err("SMBus Timout\r\n");
> > }
> > if (host_status_register & (1 << 4)) {
> > print_err("Interrup/SMI# was Failed Bus Transaction\r\n");
> > }
> > if (host_status_register & (1 << 3)) {
> > print_err("Bus Error\r\n");
> > }
> > if (host_status_register & (1 << 2)) {
> > print_err("Device Error\r\n");
> > }
> > if (host_status_register & (1 << 1)) {
> > print_err("Interrupt/SMI# was Successful Completion\r\n");
> > }
> > if (host_status_register & (1 << 0)) {
> > print_err("Host Busy\r\n");
> > }
>
> This is probably a matter of taste, but for one-liners I learned to like
> this style:
>
> [...]
> if (loops >= SMBUS_TIMEOUT)
>         print_err("SMBus Timout\r\n");
> if (host_status_register & (1 << 4))
>         print_err("Interrup/SMI# was Failed Bus Transaction\r\n");
> if (host_status_register & (1 << 3))
>         print_err("Bus Error\r\n");
> if (host_status_register & (1 << 2))
>         print_err("Device Error\r\n");
> if (host_status_register & (1 << 1))
>         print_err("Interrupt/SMI# was Successful Completion\r\n");
> if (host_status_register & (1 << 0))
>         print_err("Host Busy\r\n");

Hmm imho this did Corey, well I will leave it as it is for now.
> > +}
> >
> > +static void smbus_wait_until_ready(void)
> > +{
> > int loops;
> > PRINT_DEBUG("Waiting until smbus ready\r\n");
> > loops = 0;
> > /* Yes, this is a mess, but it's the easiest way to do it */
> > while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
> > ++loops;
> > smbus_print_error(inb(SMBHSTSTAT), loops);
> >
> > +}
> >
> > +static void smbus_reset(void)
> > +{
> > outb(HOST_RESET, SMBHSTSTAT);
> > /* Datasheet says we have to read it to take ownership of SMBus */
> > inb(SMBHSTSTAT);
> > PRINT_DEBUG("After reset status: ");
> > PRINT_DEBUG_HEX16(inb(SMBHSTSTAT));
>
> Reading it twice won't do any harm here, I assume?

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

ok
> > +u8 smbus_read_byte(u32 dimm, u32 offset)
> > +{
> > u32 val;
> > print_debug("DIMM ");
> > print_debug_hex8(dimm);
> > print_debug(" OFFSET ");
> > print_debug_hex8(offset);
> > print_debug("\r\n");
> > smbus_reset();
> > /* clear host data port */
> > outb(0x00, SMBHSTDAT0);
> > SMBUS_DELAY();
> > smbus_wait_until_ready();
> > /* actual addr to reg format */
> > dimm = (dimm << 1);
> > dimm |= 1;
> > outb(dimm, SMBXMITADD);
> > outb(offset, SMBHSTCMD);
> > outb(0x48, SMBHSTCTL);
>
> What is 0x48? What effect does it have (should be a comment)?

It tells to start the byte data transaction, whit start bit set.
> > SMBUS_DELAY();
> > smbus_wait_until_ready();
> > val = inb(SMBHSTDAT0);
> > print_debug("Read: ");
> > print_debug_hex8(val);
> > print_debug("\r\n");
> > smbus_reset();		/* probably don't have to do this, but it can't hurt */
> > return val;		/* can I just "return inb(SMBHSTDAT0)"? */
>
> Depends. Will the read value (and side-effects, if any) be the same
> after the above reset?

Hmm dont know, but I think this is safer. (this code wrote Corey IMHO)
> > +}
> >
> > +void enable_smbus(void)
> > +{
> > device_t dev;
> > /* Power management controller */
> > dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_VIA,
> > 			PCI_DEVICE_ID_VIA_VT8237R_ISA), 0);
> > if (dev == PCI_DEV_INVALID) {
> > die("Power Managment Controller not found\r\n");
> > }
> > /* Set clock source */
> > pci_write_config8(dev, 0x94, 0xa0);
>
> Needs better comment, e.g. "Set clock source to RTC. Enable Internal PLL
> Reset During Suspend" or something like that.
>
> Also, 0x94 -> VT8237R_POWER_WELL_CONTROL (or just POWER_WELL_CONTROL).

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

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

I will check what I'm setting up later in my LPC code.
> > pci_write_config8(dev, 0xd2, 0x01);
>
> ... but bow about "Enable SMB controller functions"?
>
> 0xd2 -> SMBUS_HOST_CONFIG(URATION).
> > /* make it work for I/O ... */
> > pci_write_config16(dev, PCI_COMMAND, PCI_COMMAND_IO);
> > smbus_reset();
>
> Again? Needed?

Should not harm. Lets see if this work for Corey.
> > /* reset the internal pointer */
> > inb(SMBHSTCTL);
> >
> > +}
> > Index: src/southbridge/via/vt8237r/vt8237r_sata.c
> > ===================================================================
> > --- src/southbridge/via/vt8237r/vt8237r_sata.c	(revision 0)
> > +++ src/southbridge/via/vt8237r/vt8237r_sata.c	(revision 0)
> > @@ -0,0 +1,56 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Rudolf Marek
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License v2 as published by
> > the Free Software Foundation.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > */
> >
> > +#include <console/console.h>
> > +#include <device/device.h>
> > +#include <device/pci.h>
> > +#include <device/pci_ops.h>
> > +#include <device/pci_ids.h>
> >
> > +#define SATA_MISC_CTRL 0x45
> >
> > +static void sata_init(struct device *dev)
> > +{
> > u8 reg;
> > printk_debug("Configuring VIA SATA Controller\n");
> > /* class IDE Disk */
> > reg = pci_read_config8(dev, SATA_MISC_CTRL);
> > reg &= 0x7f;		/* Sub Class Write Protect off */
> > pci_write_config8(dev, SATA_MISC_CTRL, reg);
> > /* change the device class to SATA from RAID */
> > pci_write_config8(dev, PCI_CLASS_DEVICE, 0x1);
> > reg |= 0x80;		/* Sub Class Write Protect on */
> > pci_write_config8(dev, SATA_MISC_CTRL, reg);
> >
> > +}
> >
> > +static struct device_operations sata_ops = {
> > .read_resources = pci_dev_read_resources,
> > .set_resources = pci_dev_set_resources,
> > .enable_resources = pci_dev_enable_resources,
> > .init = sata_init,
> > .enable = 0,
> > .ops_pci = 0,
> >
> > +};
> >
> > +static struct pci_driver northbridge_driver __pci_driver = {
> > .ops = &sata_ops,
> > .vendor = PCI_VENDOR_ID_VIA,
> > .device = PCI_DEVICE_ID_VIA_VT6420_SATA,
> >
> > +};
> > Index: src/southbridge/via/vt8237r/chip.h
> > ===================================================================
> > --- src/southbridge/via/vt8237r/chip.h	(revision 0)
> > +++ src/southbridge/via/vt8237r/chip.h	(revision 0)
> > @@ -0,0 +1,37 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Rudolf Marek
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License v2 as published by
> > the Free Software Foundation.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > */
> >
> > +#ifndef SOUTHBRIDGE_VIA_VT8237R_CHIP_H
> > +#define SOUTHBRIDGE_VIA_VT8237R_CHIP_H
> >
> > +extern struct chip_operations southbridge_via_vt8237r_ops;
> >
> > +struct southbridge_via_vt8237r_config {
> > /* function enable bits, check vt8237r.c for details */
> > unsigned int fn_ctrl_lo;
>
> u16?

If it works then why not. I was not sure if I can use it here.
> > unsigned int fn_ctrl_hi;
> > int ide0_enable:1;
> > int ide1_enable:1;
> > /* 1 = 80-pin cable */
> > int ide0_cable:1;
> > int ide1_cable:1;
> >
> > +};
> >
> > +#endif /* SOUTHBRIDGE_VIA_VT8237R_CHIP_H */
> > Index: src/southbridge/via/vt8237r/vt8237r_bridge.c
> > ===================================================================
> > --- src/southbridge/via/vt8237r/vt8237r_bridge.c	(revision 0)
> > +++ src/southbridge/via/vt8237r/vt8237r_bridge.c	(revision 0)
> > @@ -0,0 +1,57 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Rudolf Marek
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License v2 as published by
> > the Free Software Foundation.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > */
> >
> > +#include <device/device.h>
> > +#include <device/pci.h>
> > +#include <device/pci_ops.h>
> > +#include <device/pci_ids.h>
> > +#include <console/console.h>
> >
> > +static void br_enable(struct device *dev)
>
> bridge_enable, no need to truncate it here, IMHO.

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

it is the device ID. I dont know what it does....
> > /* VIA recommends this, sorry no known info */
> > writeback(dev, 0x40, 0x91);
> > writeback(dev, 0x41, 0x40);
> > writeback(dev, 0x43, 0x44);
> > writeback(dev, 0x44, 0x31);
> > writeback(dev, 0x45, 0x3a);
> > writeback(dev, 0x46, 0x88);	/* PCI ID lo */
> > writeback(dev, 0x47, 0xb1);	/* PCI ID hi */
> > writeback(dev, 0x3e, 0x16);	/* bridge control */
> > dump_south(dev);
> >
> > +}
> >
> > +static struct device_operations br_ops = {
> > .read_resources = pci_bus_read_resources,
> > .set_resources = pci_dev_set_resources,
> > .enable_resources = pci_bus_enable_resources,
> > .enable = br_enable,
> > .scan_bus = pci_scan_bridge,
> > .reset_bus = pci_bus_reset,
> > .ops_pci = 0,
> >
> > +};
> >
> > +static struct pci_driver northbridge_driver __pci_driver = {
> > .ops = &br_ops,
> > .vendor = PCI_VENDOR_ID_VIA,
> > .device = PCI_DEVICE_ID_VIA_K8T890CE_BR,
> >
> > +};
> > Index: src/southbridge/via/vt8237r/vt8237r_lpc.c
> > ===================================================================
> > --- src/southbridge/via/vt8237r/vt8237r_lpc.c	(revision 0)
> > +++ src/southbridge/via/vt8237r/vt8237r_lpc.c	(revision 0)
> > @@ -0,0 +1,307 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Inspiration from other VIA SB code.
> > Copyright (C) 2007 Rudolf Marek
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License v2 as published by
> > the Free Software Foundation.
> > This program is distributed in the hope that it will be useful,
> > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > GNU General Public License for more details.
> > You should have received a copy of the GNU General Public License
> > along with this program; if not, write to the Free Software
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> > */
> >
> > +#include <arch/io.h>
> > +#include <console/console.h>
> > +#include <device/device.h>
> > +#include <device/pci.h>
> > +#include <device/pci_ops.h>
> > +#include <device/pci_ids.h>
> > +#include <pc80/mc146818rtc.h>
> > +#include <cpu/x86/lapic.h>
> > +#include "vt8237r.h"
> > +#include "chip.h"
> >
> > +#define ALL		(0xff << 24)
> > +#define NONE		(0)
> > +#define DISABLED	(1 << 16)
> > +#define ENABLED		(0 << 16)
> > +#define TRIGGER_EDGE	(0 << 15)
> > +#define TRIGGER_LEVEL	(1 << 15)
> > +#define POLARITY_HIGH	(0 << 13)
> > +#define POLARITY_LOW	(1 << 13)
> > +#define PHYSICAL_DEST	(0 << 11)
> > +#define LOGICAL_DEST	(1 << 11)
> > +#define ExtINT		(7 << 8)
> > +#define NMI		(4 << 8)
> > +#define SMI		(2 << 8)
> > +#define INT		(1 << 8)
> >
> > +struct ioapicreg {
> > u32 reg;
> > u32 value_low, value_high;
> >
> > +};
> >
> > +static struct ioapicreg ioapicregvalues[] = {
>
> You can merge them like this (as the struct is never needed anywhere
> else, AFAICS):
>
> static const struct {
>    u32 reg;
>    u32 value_low;
>    u32 value_high;
>   } ioapic_table[] = {
>    // ...
>   }

ok will take a look
> > /* IO-APIC virtual wire mode configuration */
> > /* mask, trigger, polarity, destination, delivery, vector */
> > {0,
> > ENABLED | TRIGGER_EDGE | POLARITY_HIGH | PHYSICAL_DEST | ExtINT,
> > NONE},
> > {1, DISABLED, NONE},
> > {2, DISABLED, NONE},
> > {3, DISABLED, NONE},
> > {4, DISABLED, NONE},
> > {5, DISABLED, NONE},
> > {6, DISABLED, NONE},
> > {7, DISABLED, NONE},
> > {8, DISABLED, NONE},
> > {9, DISABLED, NONE},
> > {10, DISABLED, NONE},
> > {11, DISABLED, NONE},
> > {12, DISABLED, NONE},
> > {13, DISABLED, NONE},
> > {14, DISABLED, NONE},
> > {15, DISABLED, NONE},
> > {16, DISABLED, NONE},
> > {17, DISABLED, NONE},
> > {18, DISABLED, NONE},
> > {19, DISABLED, NONE},
> > {20, DISABLED, NONE},
> > {21, DISABLED, NONE},
> > {22, DISABLED, NONE},
> > {23, DISABLED, NONE},
> > /* Be careful and don't write past the end... */
> >
> > +};
> > +extern void dump_south(device_t dev);
>
> Should be at the top of the file (if at all).

yes
> > +static void setup_ioapic(u32 ioapic_base)
> > +{
> > u32 value_low, value_high, val;
> > volatile u32 *l;
> > struct ioapicreg *a = ioapicregvalues;
>
> Not needed? Use the static ioapic_table[] from above directly.

will take a look.
> > int i;
> > /* all delivered to CPU0 */
> > ioapicregvalues[0].value_high = (lapicid()) << (56 - 32);
> > l = (unsigned long *) ioapic_base;
> > /* set APIC to FSB message bus */
> > l[0] = 0x3; val = l[4];
>
> Only one command per line please.

ok
> > l[4] = (val & 0xFFFFFE) | 1;
> > /* set APIC ADDR - this will be VT8237R_APIC_ID */
> > l[0] = 0; val = l[4];
> > l[4] = (val & 0xF0FFFF) | (VT8237R_APIC_ID << 24);
> > for (i = 0;
> >     i < sizeof(ioapicregvalues) / sizeof(ioapicregvalues[0]);
>
> ARRAY_SIZE (later).

do we have it in v2?
> >     i++, a++) {
> > l[0] = (a->reg * 2) + 0x10;
> > l[4] = a->value_low;
> > value_low = l[4];
> > l[0] = (a->reg * 2) + 0x11;
> > l[4] = a->value_high;
> > value_high = l[4];
> > if ((i == 0) && (value_low == 0xffffffff)) {
> > 	printk_warning("IO APIC not responding.\n");
> > 	return;
> > }
> > }
> >
> > +}
> >
> > +static void pci_routing_fixup(struct device *dev)
> > +{
> > /* set up PCI IRQ routing, route everything through APIC */
>
> Maybe make this a Doxygen comment at the top of the function?
> > pci_write_config8(dev, 0x44, 0x00);	/* PCI PNP Interrupt Routing INTE/F - disable */
> > pci_write_config8(dev, 0x45, 0x00);	/* PCI PNP Interrupt Routing INTG/H - disable */
> > pci_write_config8(dev, 0x46, 0x10);	/* Route INTE-INTH through registers above, no map to INTA-INTD */
> > pci_write_config8(dev, 0x54, 0x00);	/* PCI Interrupt Polarity */
> > pci_write_config8(dev, 0x55, 0x00);	/* PCI INTA# Routing */
> > pci_write_config8(dev, 0x56, 0x00);	/* PCI INTB#/C# Routing */
> > pci_write_config8(dev, 0x57, 0x00);	/* PCI INTD# Routing */
> >
> > +}
> > +/*
>
> /** rather.
>
> (i.e. make it a Doxygen comment)
> > Set up the power management capabilities directly into ACPI mode.  This
> > avoids having to handle any System Management Interrupts (SMI's) which I
> > can't figure out how to do !!!!
>
> Not sure we want _this_ in the comments ;)

Heh I did not wrote this ;)  I will drop it. Anyway we have no SMM support in LB 
anyway and this is the only way how to avoid that.
> > */
> >
> > +void setup_pm(device_t dev)
> > +{
> > /* Debounce LID and PWRBTN# Inputs for 16ms */
> > pci_write_config8(dev, 0x80, 0x20);
> > /* Set ACPI base address to IO VT8237R_ACPI_IO_BASE */
> > pci_write_config16(dev, 0x88, VT8237R_ACPI_IO_BASE | 0x1);
> > /* set ACPI to 9, need to set IRQ 9 override to level!
> >   set PSON gating */
> > pci_write_config8(dev, 0x82, 0x40 | VT8237R_ACPI_IRQ);
> > /* primary interupt channel, define wake events 0=IRQ0 15=IRQ15 1=en */
> > pci_write_config16(dev, 0x84, 0x30b2);
> > /* SMI output level to low, 7.5us throttle clock */
> > pci_write_config8(dev, 0x8d, 0x18);
> > /* GP Timer Control 1s */
> > pci_write_config8(dev, 0x93, 0x88);
> > /* 7=SMBus Clock from RTC 32.768KHz
> >   5=Internall PLL reset from susp
> >   2= GPO2 is GPIO
> > */
>
> Linux-style comments please.

ok.
> /* 7=SMBus Clock from RTC 32.768KHz
> 5=Internall PLL reset from susp
> 2= GPO2 is GPIO
>
> */
> > pci_write_config8(dev, 0x94, 0xa4);
> > /* 7=stp to sust delay 1msec,
> >   6=SUSST# Deasserted Before PWRGD for STD
> >   3=GPO26/GPO27 is GPO 
> >   2=Disable Alert on Lan
> > */
>
> Ditto.

ok
> > pci_write_config8(dev, 0x95, 0xcc);
> > /* Disable GP3 timer */
> > pci_write_config8(dev, 0x98, 0);
> > /* enable SATA LED, disable special CPU Frequency Change -
> >   GPIO28 GPIO22 GPIO29 GPIO23 are GPIOs */
> > pci_write_config8(dev, 0xe5, 0x9);
> > /* REQ5 as PCI request input - should be together with  INTE-INTH */
> > pci_write_config8(dev, 0xe4, 0x4);
> > /* Enable ACPI accessm RTC signal gated with PSON */
> > pci_write_config8(dev, 0x81, 0x84);
> > /* clear status events */
> > outw(0xffff, VT8237R_ACPI_IO_BASE + 0x00);
> > outw(0xffff, VT8237R_ACPI_IO_BASE + 0x20);
> > outw(0xffff, VT8237R_ACPI_IO_BASE + 0x28);
> > outl(0xffffffff, VT8237R_ACPI_IO_BASE + 0x30);
> > /* disable SCI on GPIO */
> > outw(0x0, VT8237R_ACPI_IO_BASE + 0x22);
> > /* disable SMI on GPIO */
> > outw(0x0, VT8237R_ACPI_IO_BASE + 0x24);
> > /* disable all global enable SMIs */
> > outw(0x0, VT8237R_ACPI_IO_BASE + 0x2a);
> > /* all SMI off, both IDE buses ON, PSON rising edge */
> > outw(0x0, VT8237R_ACPI_IO_BASE + 0x2c);
> > /* primary activity SMI disable */
> > outl(0x0, VT8237R_ACPI_IO_BASE + 0x34);
> > /* GP timer reload on none */
> > outl(0x0, VT8237R_ACPI_IO_BASE + 0x38);
> > /* disable extended IO traps */
> > outb(0x0, VT8237R_ACPI_IO_BASE + 0x42);
> > /* SCI is generated for RTC/pwrBtn/slpBtn */
> > outw(0x001, VT8237R_ACPI_IO_BASE + 0x04);
> >
> > +}
> >
> > +static void vt8237r_init(struct device *dev)
> > +{
> > u8 enables, byte;
> > printk_debug("vt8237r init\n");
>
> "VT8237R init", or maybe drop the whole comment?

ok
> > /* enable addr/data stepping */
> > byte = pci_read_config8(dev, PCI_COMMAND);
> > byte |= PCI_COMMAND_WAIT;
> > pci_write_config8(dev, PCI_COMMAND, byte);
> > /* enable the internal I/O decode */
> > enables = pci_read_config8(dev, 0x6C);
> > enables |= 0x80;
> > pci_write_config8(dev, 0x6C, enables);
> > /* FIXME Map 4MB of FLASH into the address space,
> >   this should be in CAR call */
>
> Am I reading this correctly? This SB supports 4MB ROM chips then?

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

Hmm this comment was not written by me, but this nbit should be set.
> >   IMPORTANT FIX - EISA = ECLR reg at 0x4d0! Decoding must be on so that PCI 
> >   interrupts can be properly marked as level triggered. */
> > enables = pci_read_config8(dev, 0x40);
> > enables |= 0x44;
> > pci_write_config8(dev, 0x40, enables);
> > /* Line buffer control */
> > enables = pci_read_config8(dev, 0x42);
> > enables |= 0xf8;
> > pci_write_config8(dev, 0x42, enables);
>
> Oh, and please put an empty line after such "blocks" of
> enable-this-or-that lines... (easier to read)

ok
> > /* Delay Transaction Control */
> > pci_write_config8(dev, 0x43, 0xb);
> > /* IO Recovery time */
> > pci_write_config8(dev, 0x4c, 0x44);
> > /* ROM Memory Cycles Go To LPC */
> > pci_write_config8(dev, 0x59, 0x80);
> > /* bypass Bypass APIC De-Assert Message,  INTE#, INTF#, INTG#, INTH# as PCI  */
> > pci_write_config8(dev, 0x5B, 0xb);
> > /* set  Read Pass Write Control  Enable (force A2 from APIC FSB to low) */
> > pci_write_config8(dev, 0x48, 0x8c);
> > /* Set 0x58 to 0x43 APIC and RTC */
> > pci_write_config8(dev, 0x58, 0x43);
> > /* Set bit 3 of 0x4f (use INIT# as cpu reset) */
> > enables = pci_read_config8(dev, 0x4f);
> > enables |= 0x08;
> > pci_write_config8(dev, 0x4f, enables);
> > /* enable serial irq, 6PCI clocks */
> > pci_write_config8(dev, 0x52, 0x9);
> > /* enable HPET at: */
> > pci_write_config32(dev, 0x68, (VT8237R_HPET_ADDR | 0x80));
>
> Maybe make it configurable via an hpet_enable variable?

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

ok
> > rtc_init(0);
> >
> > +}
> >
> > +void vt8237r_read_resources(device_t dev)
> > +{
> > struct resource *res;
> > pci_dev_read_resources(dev);
> > /* fixed APIC resource */
> > res = new_resource(dev, 0x44);
> > res->base = VT8237R_APIC_BASE;
> > res->size = 256;
> > res->limit = res->base + res->size - 1;
> > res->align = 8;
> > res->gran = 8;
> > res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
> >    IORESOURCE_STORED | IORESOURCE_ASSIGNED;
> >
> > +}
> >
> > +/**
> > The VT8237R is not a PCI bridge and has no resources of its own (other
> > than standard PC I/O addresses), however it does control the ISA bus
> > and so we need to manually call enable childrens resources on that bus.
> > */
> >
> > +void vt8237r_enable_resources(device_t dev)
> > +{
> > pci_dev_enable_resources(dev);
> > enable_childrens_resources(dev);
> >
> > +}
> >
> > +static void init_keyboard(struct device *dev)
> > +{
> > u8 regval;
> > regval = pci_read_config8(dev, 0x51);
> > if (regval & 0x1)
> > init_pc_keyboard(0x60, 0x64, 0);
> >
> > +}
>
> Hm, this is superio stuff usually. As the VT8237R integrates these kinds
> of functions I'm not sure how to procede. Extract the Superio-related
> stuff into src/superio/via/vt8237r/* or leave it here?
>
> Opinions?

it has just RTC and keyboard. No other stuff.
> > +static void southbridge_init(struct device *dev)
> > +{
> > vt8237r_init(dev);
> > pci_routing_fixup(dev);
> > setup_ioapic(0xfec00000);
>
> Make 0xfec00000 a #define in *.h? It cannot change anyway right?
> In that case you can also drop the function parameter.

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

No...

Rudolf

[Uwe Hermann]

On Sat, Oct 27, 2007 at 09:23:19PM +0200, Rudolf Marek wrote:
> > > +config chip.h
> > > +driver vt8237r.o
> > > +driver vt8237r_ide.o
> > > +driver vt8237r_lpc.o
> > > +driver vt8237r_sata.o
> > > +driver vt8237r_bridge.o
> >
> > UHCI? EHCI? AC97 audio / modem? LAN? What's the status there?
>
> I have LAN disabled, cause it is not connected to any connector.

On your special board or on the SB in general?
> AC97/UHCI/EHCI works, M97 should too. No settings necessary so far.

Possible, yes. I don't know if it wouldn't be better to at least
"hook it up" via an ".enable = xyz_init" line (for each PCI ID)?
Maybe some init is done by the generic code even if xyx_init is empty?
I'm just guessing here, maybe someone else can say more?

I just recently noticed that it _does_ make a big difference whether the
PCI ID lines are there, at least for the (ISA) bridge. If it's not
listed (no matter if the function it calls is empty or not), the stuff
"below" won't be initialized, in my case the superio with keyboard, com
ports, parallel port and so on...
> > > /* address decoding */
> > > /* disable the BARs, we use std ports */
> >
> > Why? Just curious...
>
> I just dropped that.

The code or the comments? Why? Not needed?
> > > +#define DIMM_BASE		(0x50<<1)
> >
> > Is this really needed in vt8237r_early_smbus.c? I don't think so.
> > This address is dependent on the board anyway, correct?
>
> hmm this is first DIMM address, which is allowed in general. No eeprom will 
> occupy lower (except for the write protect control)

Hm, but you never use DIMM_BASE in the SB code. Why is it defined at all?
> > > /* actual addr to reg format */
> > > dimm = (dimm << 1);
> > > dimm |= 1;
> > > outb(dimm, SMBXMITADD);
> > > outb(offset, SMBHSTCMD);
> > > outb(0x48, SMBHSTCTL);
> >
> > What is 0x48? What effect does it have (should be a comment)?
>
> It tells to start the byte data transaction, whit start bit set.

OK, make that a comment please.
> > > +struct southbridge_via_vt8237r_config {
> > > /* function enable bits, check vt8237r.c for details */
> > > unsigned int fn_ctrl_lo;
> >
> > u16?
>
> If it works then why not. I was not sure if I can use it here.

Probably needs to include arch/i386/include/stdint.h, so:

#include <stdint.h>

But I'm not so sure I like this anyway. It's ok for now, but
these variables (ide0_enable etc) will become run-time changeable
via CMOS settings in v3 (I hope!) so it's probably better to split
this up per bit. I.e.

enable_ac97
enable_mc97
enable_rtc
enable_lan
etc.
> > > 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;
> > >
> > > +};
> > > print_debug("B188 device dump\n");
> >
> > B188?
>
> it is the device ID. I dont know what it does....

Hm, ok.
> > ARRAY_SIZE (later).
>
> do we have it in v2?

We have since r2901 :)

#include <stdlib.h>
> > > /* FIXME Map 4MB of FLASH into the address space,
> > >   this should be in CAR call */
> >
> > Am I reading this correctly? This SB supports 4MB ROM chips then?
>
> Yes - FFFFFFFFh to FFC00000h. But the memory space it decodes it strapped. 
> So perhaps for LB I should set in CAR to maximum value.

Btw, do we have flashrom support for VT8237R already? Does it work on
your board?
> > /* pci_write_config8(dev, 0x41, 0x7f); */
> > > /* Set bit 6 of 0x40, because Award does it (IO recovery time)
> >
> > "because AWARD does it" no good...
>
> Hmm this comment was not written by me, but this nbit should be set.

OK, then replace the comment with something meaningful please.
> > > /* enable HPET at: */
> > > pci_write_config32(dev, 0x68, (VT8237R_HPET_ADDR | 0x80));
> >
> > Maybe make it configurable via an hpet_enable variable?
>
> could be, but this is the default HPET address for all x86.

Yep. What I meant is to make it configurable whether to enable HPET or
not (similar to ide0_enable etc).
> > Hm, this is superio stuff usually. As the VT8237R integrates these kinds
> > of functions I'm not sure how to procede. Extract the Superio-related
> > stuff into src/superio/via/vt8237r/* or leave it here?
> > Opinions?
>
> it has just RTC and keyboard. No other stuff.

Ah, ok. In that case that stuff is better placed here, indeed.

Uwe.

[Rudolf Marek]

> On your special board or on the SB in general?

Asus put Gigabit Ethernet chip. So only on my board.
> I just recently noticed that it _does_ make a big difference whether the
> PCI ID lines are there, at least for the (ISA) bridge. If it's not
> listed (no matter if the function it calls is empty or not), the stuff
> "below" won't be initialized, in my case the superio with keyboard, com
> ports, parallel port and so on...

I have there the ISA bridge.
> > > > /* address decoding */
> > > > /* disable the BARs, we use std ports */
> > >
> > > Why? Just curious...
> >
> > I just dropped that.
>
> The code or the comments? Why? Not needed?

Whole stuff. It will get the ports from resource init.
> > > > +#define DIMM_BASE		(0x50<<1)
> > >
> > > Is this really needed in vt8237r_early_smbus.c? I don't think so.
> > > This address is dependent on the board anyway, correct?
> >
> > hmm this is first DIMM address, which is allowed in general. No eeprom will 
> > occupy lower (except for the write protect control)
>
> Hm, but you never use DIMM_BASE in the SB code. Why is it defined at all?

Yes I noticed that too, and dropped. I think there was some debug stuff which 
was used by me or Corey.
> > > > /* actual addr to reg format */
> > > > dimm = (dimm << 1);
> > > > dimm |= 1;
> > > > outb(dimm, SMBXMITADD);
> > > > outb(offset, SMBHSTCMD);
> > > > outb(0x48, SMBHSTCTL);
> > >
> > > What is 0x48? What effect does it have (should be a comment)?
> >
> > It tells to start the byte data transaction, whit start bit set.
>
> OK, make that a comment please.

I did
> > > > +struct southbridge_via_vt8237r_config {
> > > > /* function enable bits, check vt8237r.c for details */
> > > > unsigned int fn_ctrl_lo;
> > >
> > > u16?
> >
> > If it works then why not. I was not sure if I can use it here.
>
> Probably needs to include arch/i386/include/stdint.h, so:
>
> #include <stdint.h>
>
> But I'm not so sure I like this anyway. It's ok for now, but
> these variables (ide0_enable etc) will become run-time changeable
> via CMOS settings in v3 (I hope!) so it's probably better to split
> this up per bit. I.e.
>
> enable_ac97
> enable_mc97
> enable_rtc
> enable_lan
> etc.

Ok later.
> > > > 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;
> > > >
> > > > +};
> > > > print_debug("B188 device dump\n");
> > >
> > > B188?
> >
> > it is the device ID. I dont know what it does....
>
> Hm, ok.

Nope really :/
> > > ARRAY_SIZE (later).
> >
> > do we have it in v2?
>
> We have since r2901 :)
>
> #include <stdlib.h>

ok.
> > > > /* FIXME Map 4MB of FLASH into the address space,
> > > >   this should be in CAR call */
> > >
> > > Am I reading this correctly? This SB supports 4MB ROM chips then?
> >
> > Yes - FFFFFFFFh to FFC00000h. But the memory space it decodes it strapped. 
> > So perhaps for LB I should set in CAR to maximum value.
>
> Btw, do we have flashrom support for VT8237R already? Does it work on
> your board?

Yes it does work.

debian:/mnt/disc4/Moje_dilna/LinuxBIOSv2/targets/asus/a8v-e_se/a8v-e_se# 
../../../../util/flashrom/flashrom  -v linuxbios.rom
Calibrating delay loop... ok
Found LinuxBIOS table at: 00000530
vendor id: ASUS part id: A8V-E SE
Found chipset "VT8237": Enabling flash write... OK.
Pm49FL004 found at physical address: 0xfff80000
Flash part is Pm49FL004 (512 KB)
Flash image seems to be a legacy BIOS. Disabling checks.
Verifying flash - VERIFIED
> > > /* pci_write_config8(dev, 0x41, 0x7f); */
> > > > /* Set bit 6 of 0x40, because Award does it (IO recovery time)
> > >
> > > "because AWARD does it" no good...
> >
> > Hmm this comment was not written by me, but this nbit should be set.
>
> OK, then replace the comment with something meaningful please.

ok
> > > > /* enable HPET at: */
> > > > pci_write_config32(dev, 0x68, (VT8237R_HPET_ADDR | 0x80));
> > >
> > > Maybe make it configurable via an hpet_enable variable?
> >
> > could be, but this is the default HPET address for all x86.
>
> Yep. What I meant is to make it configurable whether to enable HPET or
> not (similar to ide0_enable etc).

aha ok. I think we can leave it now as it is.
> > > Hm, this is superio stuff usually. As the VT8237R integrates these kinds
> > > of functions I'm not sure how to procede. Extract the Superio-related
> > > stuff into src/superio/via/vt8237r/* or leave it here?
> > > Opinions?
> >
> > it has just RTC and keyboard. No other stuff.
>
> Ah, ok. In that case that stuff is better placed here, indeed.

Ok.

Rudolf

[Rudolf Marek]

Hi all,

Attaching fixed version. I left the dump_south and writeback there,
I have in plan to remove the "writeback" function in near future,
and for the dump_south I'm not sure how to handle the case when the debugging is
switched off. I hope it is OK to accept it as it is right now, since my primary 
goal is to make it available.

Signed-off-by: Rudolf Marek

Rudolf

[Corey Osgood]

Rudolf Marek wrote:
> Hi all,
>
> Attaching fixed version. I left the dump_south and writeback there,
> I have in plan to remove the "writeback" function in near future,
> and for the dump_south I'm not sure how to handle the case when the
> debugging is
> switched off. I hope it is OK to accept it as it is right now, since
> my primary goal is to make it available.
>
> Signed-off-by: Rudolf Marek
>
> Rudolf

Sorry I haven't gotten to this yet, been very busy lately. I'll
hopefully be back at work on the cn700 tomorrow, trying to get it ready
to go in the tree, so I'd like to see this in as well. Just a few little
things:
> Index: src/southbridge/via/vt8237r/vt8237r.c
>
> --- src/southbridge/via/vt8237r/vt8237r.c	(revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r.c	(revision 0)
> @@ -0,0 +1,82 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <pc80/keyboard.h>
> +#include "chip.h"
>
> +/*
> Datasheets: http://www.via.com.tw/en/downloads/datasheets/chipsets/
> VT8237R_SouthBridge_Revision2.06_Lead-Free.zip
>
> +*/
>
> +void hard_reset(void)
> +{
> printk_err("NO HARD RESET ON VT8237R! FIX ME!\n");
>
> +}
>
> +void writeback(struct device *dev, u16 where, u8 what)
> +{
> 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 ");
> }
>
> +}

Perhaps something like

#if DEFAULT_CONSOLE_LOGLEVEL < 8 //8 = debug level
inline void writeback(...)
{
    pci_write_config8(...);
}
#else
(original function)
#endif

writeback() seems like it could be a very useful bit of debugging code.
Renaming to writeback8 (for clarity's sake) would also be good too. The
biggest problem with the above is that it won't turn on/off if the
console log level is changed by, say, lxbios or uwe's boot menu. Only a
suggestion though
> +static void vt8237r_enable(struct device *dev)
> +{
> struct southbridge_via_vt8237r_config *sb =
> (struct southbridge_via_vt8237r_config *) dev->chip_info;
> pci_write_config8(dev, 0x50, sb->fn_ctrl_lo);
> pci_write_config8(dev, 0x51, sb->fn_ctrl_hi);
> /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */

I have a little piece of code to do this, I've just got to figure out
where/how to do it. Leave it as is and I'll take care of later.
> +}
>
> +struct chip_operations southbridge_via_vt8237r_ops = {
> CHIP_NAME("VIA VT8237R Southbridge")
>    .enable_dev = vt8237r_enable,
>
> +};
> 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,305 @@
> +/*
> This file is part of the LinuxBIOS project.
> Inspiration from other VIA SB code.
> Copyright (C) 2007 Rudolf Marek
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License v2 as published by
> the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <arch/io.h>
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <pc80/mc146818rtc.h>
> +#include <cpu/x86/lapic.h>
> +#include <stdlib.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)
>
> +extern void dump_south(device_t dev);
>
> +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)
> +{
> u32 value_low, value_high, val;
> volatile u32 *l;
> int i;
> /* all delivered to CPU0 */
> ioapic_table[0].value_high = (lapicid()) << (56 - 32);
> l = (unsigned long *) ioapic_base;
> /* set APIC to FSB message bus */
> l[0] = 0x3;
> val = l[4];
> 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 < 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;
> }
> }
>
> +}
>
> +static void pci_routing_fixup(struct device *dev)
> +{
> /* set up PCI IRQ routing, route everything through APIC */
> 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 */
>
> +}
>
> +/*
> Set up the power management capabilities directly into ACPI mode.  This
> avoids having to handle any System Management Interrupts (SMI's)
> */
>
> +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
> */
> 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);
>
> +}
>
> +static void vt8237r_init(struct device *dev)
> +{
> 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 */
> /* pci_write_config8(dev, 0x41, 0x7f); */
> /* Set bit 6 of 0x40, (IO 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);
> /* 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));
> /* Power management setup */
> setup_pm(dev);
> /* Start the 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);
>
> +}
>
> +static void southbridge_init(struct device *dev)
> +{
> vt8237r_init(dev);
> pci_routing_fixup(dev);
> setup_ioapic(VT8237R_APIC_BASE);
> setup_i8259();
> init_keyboard(dev);
>
> +}
>
> +static struct device_operations vt8237r_lpc_ops = {
> .read_resources = vt8237r_read_resources,
> .set_resources = pci_dev_set_resources,
> .enable_resources = vt8237r_enable_resources,
> .init = &southbridge_init,
> .scan_bus = scan_static_bus,
>
> +};
>
> +static struct pci_driver lpc_driver __pci_driver = {
> .ops = &vt8237r_lpc_ops,
> .vendor = PCI_VENDOR_ID_VIA,
> .device = PCI_DEVICE_ID_VIA_VT8237R_ISA,

PCI_DEVICE_ID_VIA_VT8237R_LPC please. Datasheet refers to it as lpc, as
does the file name.
> +};
> Index: src/include/device/pci_ids.h
> ===================================================================
> --- src/include/device/pci_ids.h	(revision 2901)
> +++ src/include/device/pci_ids.h	(working copy)
> @@ -1138,6 +1138,21 @@
>  #define PCI_DEVICE_ID_VIA_8505_1	0x8605
>  #define PCI_DEVICE_ID_VIA_8633_1	0xB091
>  #define PCI_DEVICE_ID_VIA_8367_1	0xB099
> +#define PCI_DEVICE_ID_VIA_K8T890CE_0	0x0238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_1	0x1238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_2	0x2238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_3	0x3238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_4	0x4238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_5	0x5238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_7	0x7238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEG	0xa238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX0	0xc238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX1	0xd238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX2	0xe238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_PEX3	0xf238
> +#define PCI_DEVICE_ID_VIA_K8T890CE_BR	0xb188
> +#define PCI_DEVICE_ID_VIA_VT6420_SATA	0x3149
> +#define PCI_DEVICE_ID_VIA_VT8237R_ISA	0x3227

Same here. With that one nitpick:

Acked-by: Corey Osgood

-Corey

[Rudolf Marek]

Hi all,
> Sorry I haven't gotten to this yet, been very busy lately. I'll
> hopefully be back at work on the cn700 tomorrow, trying to get it ready
> to go in the tree, so I'd like to see this in as well. Just a few little
> things:

ok thanks,
> Perhaps something like
>
> #if DEFAULT_CONSOLE_LOGLEVEL < 8 //8 = debug level
> inline void writeback(...)
> {
>     pci_write_config8(...);
> }
> #else
> (original function)
> #endif

ok sounds good.
  +
> > /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI specs */
>
> I have a little piece of code to do this, I've just got to figure out
> where/how to do it. Leave it as is and I'll take care of later.

It is in "VT8237R Plus BIOS Porting Guide" just few lines of code, i can write 
it in next mail so NDA is "satisfied".
> PCI_DEVICE_ID_VIA_VT8237R_LPC please. Datasheet refers to it as lpc, as
> does the file name.

Hmm i tried to stick the to the name common in LB and in in PCIID

Rudolf

[Corey Osgood]

Rudolf Marek wrote:
> Hi all,
> > Sorry I haven't gotten to this yet, been very busy lately. I'll
> > hopefully be back at work on the cn700 tomorrow, trying to get it ready
> > to go in the tree, so I'd like to see this in as well. Just a few little
> > things:
>
> ok thanks,
> > Perhaps something like
> >
> > #if DEFAULT_CONSOLE_LOGLEVEL < 8 //8 = debug level
> > inline void writeback(...)
> > {
> >     pci_write_config8(...);
> > }
> > #else
> > (original function)
> > #endif
>
> ok sounds good.
> > > /* TODO: if SATA is disabled, move IDE to fn0 to conform PCI
> > >
> > > specs */
> >
> > I have a little piece of code to do this, I've just got to figure out
> > where/how to do it. Leave it as is and I'll take care of later.
>
> It is in "VT8237R Plus BIOS Porting Guide" just few lines of code, i
> can write it in next mail so NDA is "satisfied".

OK. Problem is though it has to be taken care of before the devices are
scanned, so that no device drops off or appears and confuses LB. If the
lpc bridge were scanned before the sata one, it could be done then, but
unfortunately it isn't. I've been doing it during ram init, but I
imagine it could be done somewhere shortly thereafter.
> > PCI_DEVICE_ID_VIA_VT8237R_LPC please. Datasheet refers to it as lpc, as
> > does the file name.
>
> Hmm i tried to stick the to the name common in LB and in in PCIID

IMO, it needs to remain consistent one way or another. I still vote for
LPC, since that is what the datasheet refers to it as and it's the main
function of the controller. ISA is a dying breed, it's only on there for
super io and sensor support. But either that or change the file name,
doesn't really matter.

-Corey

[Rudolf Marek]

> > It is in "VT8237R Plus BIOS Porting Guide" just few lines of code, i
> > can write it in next mail so NDA is "satisfied".
>
> OK. Problem is though it has to be taken care of before the devices are
> scanned, so that no device drops off or appears and confuses LB. If the
> lpc bridge were scanned before the sata one, it could be done then, but
> unfortunately it isn't. I've been doing it during ram init, but I
> imagine it could be done somewhere shortly thereafter.

Imho that function I placed the comment is called very very early, because it is
static device??
> > > PCI_DEVICE_ID_VIA_VT8237R_LPC please. Datasheet refers to it as lpc, as
> > > does the file name.
> >
> > Hmm i tried to stick the to the name common in LB and in in PCIID
>
> IMO, it needs to remain consistent one way or another. I still vote for
> LPC, since that is what the datasheet refers to it as and it's the main
> function of the controller. ISA is a dying breed, it's only on there for
> super io and sensor support. But either that or change the file name,
> doesn't really matter.

Ok I will rename and respin somehow. Perhaps tomorrow?

Rudolf

[Corey Osgood]

Rudolf Marek wrote:
> > > It is in "VT8237R Plus BIOS Porting Guide" just few lines of code, i
> > > can write it in next mail so NDA is "satisfied".
> >
> > OK. Problem is though it has to be taken care of before the devices are
> > scanned, so that no device drops off or appears and confuses LB. If the
> > lpc bridge were scanned before the sata one, it could be done then, but
> > unfortunately it isn't. I've been doing it during ram init, but I
> > imagine it could be done somewhere shortly thereafter.
>
> Imho that function I placed the comment is called very very early, because it is
> static device??

I think I overlooked it, I'll have to check again.
> > > > PCI_DEVICE_ID_VIA_VT8237R_LPC please. Datasheet refers to it as lpc, as
> > > > does the file name.
> > >
> > > Hmm i tried to stick the to the name common in LB and in in PCIID
> >
> > IMO, it needs to remain consistent one way or another. I still vote for
> > LPC, since that is what the datasheet refers to it as and it's the main
> > function of the controller. ISA is a dying breed, it's only on there for
> > super io and sensor support. But either that or change the file name,
> > doesn't really matter.
>
> Ok I will rename and respin somehow. Perhaps tomorrow?
>
> Rudolf

You don't have to remake the patch, just edit it. Perhaps even the
committer can do it?

-Corey

[Rudolf Marek]

Hi all,

here is fixed version, writeback is guarded with debug level.

ISA->LPC

Signed-off-by: Rudolf Marek

Rudolf

[Rudolf Marek]

Rudolf Marek wrote:
> Hi all,
>
> here is fixed version, writeback is guarded with debug level.
>
> ISA->LPC
>
> Signed-off-by: Rudolf Marek
>
> Rudolf

Hmm my check and send extension overseen that I forgot the attachment :/

Sorry.

Rudolf

[Corey Osgood]

Rudolf Marek wrote:
> Rudolf Marek wrote:
> > Hi all,
> >
> > here is fixed version, writeback is guarded with debug level.
> >
> > ISA->LPC
> >
> > Signed-off-by: Rudolf Marek
> >
> > Rudolf
>
> Hmm my check and send extension overseen that I forgot the attachment :/
>
> Sorry.

No worries ;)
> Rudolf

Looks good from here.

Acked-by:

[Uwe Hermann]

On Mon, Oct 29, 2007 at 06:31:15PM -0400, Corey Osgood wrote:
> > > Signed-off-by: Rudolf Marek
>
> Acked-by:

Thanks a lot for the patch and for merging the suggestions from the
various reviews! That's very appreviated!

Committed in r2907 with some minor cosmetics.

Uwe.

[Peter Stuge]

On Sun, Oct 28, 2007 at 03:03:26PM -0400, Corey Osgood wrote:
> > > PCI_DEVICE_ID_VIA_VT8237R_LPC please. Datasheet refers to it as
> > > lpc, as does the file name.
> >
> > Hmm i tried to stick the to the name common in LB and in in PCIID
>
> IMO, it needs to remain consistent one way or another. I still vote
> for LPC, since that is what the datasheet refers to it as and it's
> the main function of the controller.

I like LPC too.

//Peter