Re: [LinuxBIOS] [PATCH] Add generic Intel i82801 support

[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.