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

[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