[LinuxBIOS] [PATCH] Add generic Intel i82801 support

[Corey Osgood]

See patch. I'm still waiting on a response from Steve just to confirm
GPLv2 on a couple files, and I'd like Yinghai to make sure those details
are correct as well (ie were you working for Tyan at the time, and is
that the right email address to use?). Other than that, it's as done as
I'm getting it. BTW, i810 will be on its way again soon, I'm trying to
get vga working first.

-Corey

[yhlu]

On 6/5/07, Corey Osgood  wrote:
> See patch. I'm still waiting on a response from Steve just to confirm
> GPLv2 on a couple files, and I'd like Yinghai to make sure those details
> are correct as well (ie were you working for Tyan at the time, and is
> that the right email address to use?).

yeah, that email should be always work. except someday google try to
charge for that.

YH

[Corey Osgood]

yhlu wrote:
> On 6/5/07, Corey Osgood  wrote:
> > See patch. I'm still waiting on a response from Steve just to confirm
> > GPLv2 on a couple files, and I'd like Yinghai to make sure those details
> > are correct as well (ie were you working for Tyan at the time, and is
> > that the right email address to use?).
>
> yeah, that email should be always work. except someday google try to
> charge for that.
>
> YH

Alright, thanks. Steve has emailed me back and confirmed the rest of the
info, so the headers should be all good.

-Corey

[Uwe Hermann]

Hi,

On Tue, Jun 05, 2007 at 05:26:31AM -0400, Corey Osgood wrote:
> BTW, i810 will be on its way again soon, I'm trying to
> get vga working first.

Please post your current patch. Let's get a somewhat "stable" version
into svn first and fix remaining issues later. The patches will be a
lot smaller and more manageable then...

Ditto for this 82801XX code, please post your current version (maybe
with the PCI IDs stuff Stefan suggested).

Here are some more comments, but not all of this has to be fixed in
this revision, we can make extra patches later for some of these issues...
> Index: src/southbridge/intel/i82801xx/i82801xx_ac97.c
>
> --- src/southbridge/intel/i82801xx/i82801xx_ac97.c	(revision 0)
> +++ src/southbridge/intel/i82801xx/i82801xx_ac97.c	(revision 0)
> @@ -0,0 +1,62 @@
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ids.h>
> +#include <device/pci_ops.h>

pci_ops.h can be dropped, I think. It's already included by pci.h.
> +#include "i82801xx.h"
> +#include "i82801_model_specific.h"
>
> +#ifdef I82801_AC97

Can we put these #ifdefs in the Config.lb file around the
  driver i82801xx_foo.o
lines? That's more readable than having them spread over all files, IMO.
> +static void ac97_set_subsystem(device_t dev, unsigned vendor, unsigned device)
> +{
> /* Write the subsystem vendor and device id */
> pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> ((device & 0xffff) << 16) | (vendor & 0xffff));
>
> +}

I think this is not required. The generic PCI device code already does
exactly this per default, so...
> +static struct pci_operations lops_pci = {
> .set_subsystem = ac97_set_subsystem,
>
> +};

...this can be dropped, ...
> +static struct device_operations ac97_ops  = {
> .read_resources   = pci_dev_read_resources,
> .set_resources    = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init             = 0,
> .scan_bus         = 0,
> .enable           = i82801xx_enable,
> .ops_pci          = &lops_pci,

... and this can be omitted, too.

(should be tested of course, but I'm pretty confident the default
function will be applied/used if it's not overridden here)
> +static struct pci_driver ac97_audio_driver __pci_driver = {
> .ops    = &ac97_ops,
> .vendor = PCI_VENDOR_ID_INTEL,
> .device = I82801_AC97,

PCI_DEVICE_ID_INTEL_I82801XX_AC97, please (defined in pci_ids.h, or
maybe localy in 82801XX code?)

Or rather, as per Stefan's suggestion, one driver entry for each real
device (for ICH0, ICH1, ICH2, etc.) if the PCI IDs are different.
> +static struct device_operations nic_ops  = {
> .read_resources   = pci_dev_read_resources,
> .set_resources    = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init             = 0,
> .scan_bus         = 0,
>
> +};

Maybe add a comment here that this doesn't need special setup, and the
usual PCI ops are enough.
> +static void pci_init(struct device *dev)
> +{
> uint32_t dword;
> uint16_t word;

Can we use reg16 and reg32 as names here? "Word" is a bit unfortunate,
not all architectures call a 16bit value a "word".
> Index: src/southbridge/intel/i82801xx/i82801xx_lpc.c
>
> --- src/southbridge/intel/i82801xx/i82801xx_lpc.c	(revision 0)
> +++ src/southbridge/intel/i82801xx/i82801xx_lpc.c	(revision 0)
> @@ -0,0 +1,246 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2003 Linux Networx, SuSE Linux AG

Two copyright lines here, please.
> +#define NMI_OFF 0

Should be a config option maybe (later).
> +void i82801xx_enable_ioapic( struct device *dev) 
> +{
> uint32_t dword;
> volatile uint32_t *ioapic_index = (volatile uint32_t *)0xfec00000;
> volatile uint32_t *ioapic_data = (volatile uint32_t *)0xfec00010;

TODO: check if these addresses are the same for all 82801XX devices.
> dword = pci_read_config32(dev, GEN_CNTL);
> dword |= (3 << 7); /* Enable IOAPIC */
> dword |= (1 << 13); /* Coprocessor error enable */
> dword |= (1 << 1); /* Delayed transaction enable */
> dword |= (1 << 2); /* DMA collection buffer enable */

#defines badly needed here.
> +void i82801xx_enable_serial_irqs( struct device *dev)
> +{
> /* set packet length and toggle silent mode bit */
> pci_write_config8(dev, SERIRQ_CNTL, (1 << 7)|(1 << 6)|((21 - 17) << 2)|(0 << 0));
> pci_write_config8(dev, SERIRQ_CNTL, (1 << 7)|(0 << 6)|((21 - 17) << 2)|(0 << 0));
> /* TODO: Get rid of the nasty ugly confusing bit ^^^ */

#defines. Which ugly bit? What's ugly? Why?
(yes, I know you didn't write that code/comment, but we need to find out
and fix this)
> +void i82801xx_lpc_route_dma( struct device *dev, uint8_t mask) 
> +{
>    uint16_t word;
>    int i;
>    word = pci_read_config16(dev, PCI_DMA_CFG);
>    word &= 0x300;
>    for(i = 0; i < 8; i++) {
>            if (i == 4)
>                    continue;
>            word |= ((mask & (1 << i))? 3:1) << (i * 2);

Ditto. Needs #defines and good comments/explanations.
> +void i82801xx_1f0_misc(struct device *dev)
> +{
> /* Prevent LPC disabling, enable parity errors, and SERR# (System Error) */
> pci_write_config16(dev, PCI_COMMAND, 0x014f);
> /* Set ACPI base address to 0x1100 (I/O space) */
> pci_write_config32(dev, PMBASE, PM_BASE_ADDR | 1);
> /* Enable ACPI I/O and power management */
> pci_write_config8(dev, ACPI_CNTL, 0x10);

Shouldn't this stuff be in an extra *_acpi_*() function?
> /* Set GPIO base address to 0x1180 (I/O space) */
> pci_write_config32(dev, GPIO_BASE, GPIO_BASE_ADDR | 1);
> /* Enable GPIO */
> pci_write_config8(dev, GPIO_CNTL, 0x10);
> /* Route PIRQA to IRQ11, PIRQB to IRQ3, PIRQC to IRQ5, PIRQD to IRQ10 */
> pci_write_config32(dev, PIRQA_ROUT, 0x0A05030B);
> /* Route PIRQE to IRQ7. Leave PIRQF - PIRQH unrouted */
> pci_write_config8(dev, PIRQE_ROUT, 0x07);
> /* Enable access to the upper 128 byte bank of CMOS RAM */
> pci_write_config8(dev, RTC_CONF, 0x04);
> /* Decode 0x3F8-0x3FF (COM1) for COMA port, 0x2F8-0x2FF (COM2) for COMB */
> pci_write_config8(dev, COM_DEC, 0x10);
> /* LPT decode defaults to 0x378-0x37F and 0x778-0x77F
> * Floppy decode defaults to 0x3F0-0x3F5, 0x3F7 */
> /* Enable: COMA, COMB, LPT, Floppy
> * Disable: Microcontroller, Super I/O, Sound, Gameport */
> pci_write_config16(dev, LPC_EN, 0x000F);
>
> +}
> +static void enable_hpet(struct device *dev)
> +{
> +#ifdef HPET_PRESENT
>    uint32_t dword;
> uint32_t code = (0 & 0x3);
>    dword = pci_read_config32(dev, GEN_CNTL);
>    dword |= (1 << 17); /* Enable HPET */
> /*Bits [16:15]Memory Address Range
> 00 FED0_0000h - FED0_03FFh
> 01 FED0_1000h - FED0_13FFh
> 10 FED0_2000h - FED0_23FFh
> 11 FED0_3000h - FED0_33FFh*/
>    dword &= ~(3 << 15); /* Clear it */
> dword |= (code << 15);
> printk_debug("Enabling HPET @0x%x\n", HPET_ADDR | (code << 12));
>
> +#endif

Should be (runtime-)configurable later.
> Copyright (C) 2007 Corey Osgood
> Copyright (C) 2005 Digital Design Corporation

Swap order here please, keep the chronological order.
> +void i82801xx_enable(device_t dev)
> +{
> unsigned int index = 0;
> uint8_t bHasDisableBit = 0;

TODO for later: variable name fixing (should be all-lowercase).
> uint16_t cur_disable_mask, new_disable_mask;
> // All 82801 devices should be on bus 0
> unsigned int devfn = PCI_DEVFN(0x1f, 0); // lpc
> device_t lpc_dev = dev_find_slot(0, devfn); // 0
> if (!lpc_dev)
> return;

A warning would be good, in addition to just returning.
> Index: src/southbridge/intel/i82801xx/i82801xx_reset.c
>
> --- src/southbridge/intel/i82801xx/i82801xx_reset.c	(revision 0)
> +++ src/southbridge/intel/i82801xx/i82801xx_reset.c	(revision 0)
> @@ -0,0 +1,27 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2002 Eric Biederman

(C) Corey Osgood? Trivial function anyway...
> 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 <arch/io.h>
>
> +void hard_reset(void)
> +{
>    /* Try rebooting through port 0xcf9 */
>    outb((0 <<3)|(1<<2)|(1<<1), 0xcf9);

Improvement: use #defines.

Also, the (0 << 3) looks a bit strange. This will logically OR bit 3
with a 0, which doesn't have any effect at all.
If the intention is to force bit 3 to be 0, it should be something like
this:

reg8 = inb(0xcf9);
reg8 |= (1 << 2) | (1 << 1);
reg8 &= ~(1 << 3)
outb(0xcf9);

Doing an inb() and modifying the value (as opposed to hardcoding a value
and simply write it) is the "correct" thing anyway. Bits 0 and 7:4 are
marked as reserved, thus should not be changed.
It may be unlikely to cause any harm in this case, but I think we
should -- in general -- always program defensively...
> Index: src/southbridge/intel/i82801xx/i82801xx.h
>
> --- src/southbridge/intel/i82801xx/i82801xx.h	(revision 0)
> +++ src/southbridge/intel/i82801xx/i82801xx.h	(revision 0)
> @@ -0,0 +1,106 @@
> +/*
> This file is part of the LinuxBIOS project.
> 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
> */
>
> +/* Copyright holder left out intentionally, I don't believe there's anything 
> +really copyrightable in here */

Maybe, but that's the case for other files, too. Just add yourself.
Every file should have a copyright holder and license header.
> +#include <smbus.h>
> +#include <pci.h>
> +#include <arch/io.h>
> +#include "i82801_model_specific.h"
> +#include "i82801xx.h"
> +#include "i82801_smbus.h"
>
> +static int smbus_read_byte(struct bus *bus, device_t dev, uint8_t address)
> +{
> unsigned device;

unsigned -> uint8_t or similar?
> +/* This file's purpose is to define anything that may differ between different
> models within the i82801 series */
>
> +#if	I82801_MODEL == I82801AA
> +#define	I82801_PCI	0x2418	/* D30:F0, PCI Interface Hub */
> +#define	I82801_LPC	0x2410	/* D31:F0, LPC Interface Bridge */
> +#define	I82801_IDE	0x2411	/* D31:F1, IDE Controller */
> +#define	I82801_USB1	0x2412	/* D31:F2, USB Controller */
> +#define	I82801_SMBUS	0x2413	/* D31:F3, SMBUS Controller */
> +#define	I82801_AC97	0x2415	/* D31:F5, AC'97 Audio Controller */
> +#define	I82801_MC97	0x2416	/* D31:F6, AC'97 Modem Controller */

I'd prefer at least I82801XX_FOO (add the XX), but better would be the
full PCI IDs in pci_ids.h, as usual, and use the approach suggested by
Stefan.
> Index: src/southbridge/intel/i82801xx/cmos_failover.c
>
> --- src/southbridge/intel/i82801xx/cmos_failover.c	(revision 0)
> +++ src/southbridge/intel/i82801xx/cmos_failover.c	(revision 0)
> @@ -0,0 +1,32 @@
> +/*
> This file is part of the LinuxBIOS project.

Missing copyright owner.
> 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 "i82801xx.h"
>
> +static void check_cmos_failed(void) 
> +{
> uint8_t byte;
> byte = pci_read_config8(PCI_DEV(0, 0x1f, 0), GEN_PMCON_3);
> if( byte & RTC_FAILED) {
> //clear bit 1 and bit 2
> byte = cmos_read(RTC_BOOT_BYTE);
> byte &= 0x0c;
> byte |= MAX_REBOOT_CNT << 4;
> cmos_write(byte, RTC_BOOT_BYTE);
> }
>
> +}
> +#ifndef I82801XX_CHIP_H
> +#define I82801XX_CHIP_H

SOUTHBRIDGE_INTEL_I82801XX_CHIP_H
> +static void ide_init(struct device *dev)
> +{
> /* TODO: Needs to be tested for compatibility with ICH5(R) */
> /* Enable ide devices so the linux ide driver will work */
> uint16_t ideTimingConfig;
> int enable_primary = 1;
> int enable_secondary = 1;

TODO for later: make all of this configurable. See my i82371EB patches
for an example.

You're doing really great work with this patch, we need such cleanups
for many other areas of the code! This will eliminate _lots_ of
duplicated or near-duplicated code, which is a huge win.

Thanks, Uwe.

[Corey Osgood]

Uwe Hermann wrote:
> Hi,
>
> On Tue, Jun 05, 2007 at 05:26:31AM -0400, Corey Osgood wrote:
> > BTW, i810 will be on its way again soon, I'm trying to
> > get vga working first.
>
> Please post your current patch. Let's get a somewhat "stable" version
> into svn first and fix remaining issues later. The patches will be a
> lot smaller and more manageable then...
>
> Ditto for this 82801XX code, please post your current version (maybe
> with the PCI IDs stuff Stefan suggested).

Waiting for fibreglass to dry right at the moment (geeks should never do
body work on cars). I'll finish the 82801 up later tonight and post the
patches. VGA is still not working, and it seems I've now broken the
previously working kernel boot, the latter of which needs to be fixed first.
> Here are some more comments, but not all of this has to be fixed in
> this revision, we can make extra patches later for some of these issues...

And just a few responses, thanks
> > +#include "i82801xx.h"
> > +#include "i82801_model_specific.h"
> >
> > +#ifdef I82801_AC97
>
> Can we put these #ifdefs in the Config.lb file around the
>   driver i82801xx_foo.o
> lines? That's more readable than having them spread over all files, IMO.

They're going away, hopefully, so this won't be an issue. Other than that, I'm not really sure, I threw them in the code to make it simple.
> > +static void ac97_set_subsystem(device_t dev, unsigned vendor, unsigned device)
> > +{
> > /* Write the subsystem vendor and device id */
> > pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> > ((device & 0xffff) << 16) | (vendor & 0xffff));
> >
> > +}
>
> I think this is not required. The generic PCI device code already does
> exactly this per default, so...

Hmm, didn't know that. These were pulled from the i82801er, and it seems
to me that one of the devices requires some special protection method
that I haven't checked out yet.
> > +static struct pci_driver ac97_audio_driver __pci_driver = {
> > .ops    = &ac97_ops,
> > .vendor = PCI_VENDOR_ID_INTEL,
> > .device = I82801_AC97,
>
> PCI_DEVICE_ID_INTEL_I82801XX_AC97, please (defined in pci_ids.h, or
> maybe localy in 82801XX code?)
>
> Or rather, as per Stefan's suggestion, one driver entry for each real
> device (for ICH0, ICH1, ICH2, etc.) if the PCI IDs are different.

To continue with stefan's idea, they'll be dropped entirely.
> > Index: src/southbridge/intel/i82801xx/i82801xx_lpc.c
> >
> > --- src/southbridge/intel/i82801xx/i82801xx_lpc.c	(revision 0)
> > +++ src/southbridge/intel/i82801xx/i82801xx_lpc.c	(revision 0)
> > @@ -0,0 +1,246 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2003 Linux Networx, SuSE Linux AG
>
> Two copyright lines here, please.

This was the only thing copyright-wise I wasn't really sure about, if
this was written by SuSE for Linux Networx, vice versa, or if both
parties had something to do with it. This line was pulled from the
original header, in i82801er I believe.
> > +#define NMI_OFF 0
>
> Should be a config option maybe (later).

yes, later.
> > dword = pci_read_config32(dev, GEN_CNTL);
> > dword |= (3 << 7); /* Enable IOAPIC */
> > dword |= (1 << 13); /* Coprocessor error enable */
> > dword |= (1 << 1); /* Delayed transaction enable */
> > dword |= (1 << 2); /* DMA collection buffer enable */
>
> #defines badly needed here.

Agreed. I think they might already be defined, I just didn't use them,
since the code and defines were from different sources.
> > +void i82801xx_enable_serial_irqs( struct device *dev)
> > +{
> > /* set packet length and toggle silent mode bit */
> > pci_write_config8(dev, SERIRQ_CNTL, (1 << 7)|(1 << 6)|((21 - 17) << 2)|(0 << 0));
> > pci_write_config8(dev, SERIRQ_CNTL, (1 << 7)|(0 << 6)|((21 - 17) << 2)|(0 << 0));
> > /* TODO: Get rid of the nasty ugly confusing bit ^^^ */
>
> #defines. Which ugly bit? What's ugly? Why?
> (yes, I know you didn't write that code/comment, but we need to find out
> and fix this)

I did write the comment, I was talking about that entire string of ORs,
mostly the "((21 - 17) << 2)", which makes no sense whatsoever to me.
Those all need either defines or a decent comment, or both.
> > +void i82801xx_lpc_route_dma( struct device *dev, uint8_t mask) 
> > +{
> >    uint16_t word;
> >    int i;
> >    word = pci_read_config16(dev, PCI_DMA_CFG);
> >    word &= 0x300;
> >    for(i = 0; i < 8; i++) {
> >            if (i == 4)
> >                    continue;
> >            word |= ((mask & (1 << i))? 3:1) << (i * 2);
>
> Ditto. Needs #defines and good comments/explanations.

Comment yes, but I'm not sure a define could be used very well here.
> > +void i82801xx_1f0_misc(struct device *dev)
> > +{
> > /* Prevent LPC disabling, enable parity errors, and SERR# (System Error) */
> > pci_write_config16(dev, PCI_COMMAND, 0x014f);
> > /* Set ACPI base address to 0x1100 (I/O space) */
> > pci_write_config32(dev, PMBASE, PM_BASE_ADDR | 1);
> > /* Enable ACPI I/O and power management */
> > pci_write_config8(dev, ACPI_CNTL, 0x10);
>
> Shouldn't this stuff be in an extra *_acpi_*() function?

Probably, but this was how the original code did it. I think I'm leaving
this alone for now though, and put in a todo.
> > +void i82801xx_enable(device_t dev)
> > +{
> > unsigned int index = 0;
> > uint8_t bHasDisableBit = 0;
>
> TODO for later: variable name fixing (should be all-lowercase).

bHasDisableBit and a lot of that code is disappearing entirely. If the
function actually exists and can't be disabled, it seems that writes
have no effect, or at least not an effect that does anything to the
system, though this has only been tested on i82801aa. This is one of the
few remaining places those defines are used, and I've got some more
research to do on the others (watchdog and hpet).
> > +#include <arch/io.h>
> >
> > +void hard_reset(void)
> > +{
> >    /* Try rebooting through port 0xcf9 */
> >    outb((0 <<3)|(1<<2)|(1<<1), 0xcf9);
>
> Improvement: use #defines.
>
> Also, the (0 << 3) looks a bit strange. This will logically OR bit 3
> with a 0, which doesn't have any effect at all.
> If the intention is to force bit 3 to be 0, it should be something like
> this:
>
> reg8 = inb(0xcf9);
> reg8 |= (1 << 2) | (1 << 1);
> reg8 &= ~(1 << 3)
> outb(0xcf9);
>
> Doing an inb() and modifying the value (as opposed to hardcoding a value
> and simply write it) is the "correct" thing anyway. Bits 0 and 7:4 are
> marked as reserved, thus should not be changed.
> It may be unlikely to cause any harm in this case, but I think we
> should -- in general -- always program defensively...

I'll look into this, I never actually checked it out at all. This
file/function is a direct descendant from v1, so I figured it probably
worked, ie don't mess with it.
> > +/* This file's purpose is to define anything that may differ between different
> > models within the i82801 series */
> >
> > +#if	I82801_MODEL == I82801AA
> > +#define	I82801_PCI	0x2418	/* D30:F0, PCI Interface Hub */
> > +#define	I82801_LPC	0x2410	/* D31:F0, LPC Interface Bridge */
> > +#define	I82801_IDE	0x2411	/* D31:F1, IDE Controller */
> > +#define	I82801_USB1	0x2412	/* D31:F2, USB Controller */
> > +#define	I82801_SMBUS	0x2413	/* D31:F3, SMBUS Controller */
> > +#define	I82801_AC97	0x2415	/* D31:F5, AC'97 Audio Controller */
> > +#define	I82801_MC97	0x2416	/* D31:F6, AC'97 Modem Controller */
>
> I'd prefer at least I82801XX_FOO (add the XX), but better would be the
> full PCI IDs in pci_ids.h, as usual, and use the approach suggested by
> Stefan.

Yep, currently working on this. Most of the IDs are already in pci_ids.h.
> > Index: src/southbridge/intel/i82801xx/cmos_failover.c
> >
> > --- src/southbridge/intel/i82801xx/cmos_failover.c	(revision 0)
> > +++ src/southbridge/intel/i82801xx/cmos_failover.c	(revision 0)
> > @@ -0,0 +1,32 @@
> > +/*
> > This file is part of the LinuxBIOS project.
>
> Missing copyright owner.

This was this way when I pulled it from the i82801er, can't be sure who
it belongs to (Yinghai? Eric? or even older than that?)
> > +static void ide_init(struct device *dev)
> > +{
> > /* TODO: Needs to be tested for compatibility with ICH5(R) */
> > /* Enable ide devices so the linux ide driver will work */
> > uint16_t ideTimingConfig;
> > int enable_primary = 1;
> > int enable_secondary = 1;
>
> TODO for later: make all of this configurable. See my i82371EB patches
> for an example.
>
> You're doing really great work with this patch, we need such cleanups
> for many other areas of the code! This will eliminate _lots_ of
> duplicated or near-duplicated code, which is a huge win.

Yeah, it was already on the todo list, and should be pretty simple.
Later, though, for now enabling both ide controllers really can't hurt
much for a base test system.

BTW, I'm planning a much more complete and hopefully smarter rewrite for
v3, and with v3's build system this should be a lot easier to do. I've
been looking a lot at v3, and playing with qemu, and it's got me
excited, this next release should be great!

-Corey

[Stefan Reinauer]

* Corey Osgood  [070609 03:08]:
> > > Copyright (C) 2003 Linux Networx, SuSE Linux AG
> >
> > Two copyright lines here, please.
>
> This was the only thing copyright-wise I wasn't really sure about, if
> this was written by SuSE for Linux Networx, vice versa, or if both
> parties had something to do with it. This line was pulled from the
> original header, in i82801er I believe.

Eric and me were working on that code at the same time, for AMD64. I
guess at some time it was used as a base to get the intel chip working.

Stefan