Re: [LinuxBIOS] [PATCH] K8T890/VT8237 A8V-E SE (pre)release!

[Corey Osgood]

Just to be a pain...

Uwe Hermann wrote:
> On Sat, Oct 13, 2007 at 12:00:27AM +0200, Rudolf Marek wrote:
> > +void set_led()
>
> void set_led(void)
> > +{
> > // set power led to steady now that lxbios has virtually done its job
> > //device_t dev;
> > //dev = dev_find_device(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_8237, 0);
> > //pci_write_config8(dev, 0x94, 0xb0);
> >
> > +}
>
> Is this a TODO? If this cannot work or you don't plan implementing it,
> please drop the function.

This was originally from v1, I think. From what I've read, ron had the
epia (not epia-m) set up to start flashing the power led as soon as the
system powered on. This would then return it to being solid on. The
light goes to solid-on by default, so this is no longer necessary.
> > enables |= 0xf0;
> > pci_write_config8(dev, 0x41, enables);
> > // Lower thresholds (cause award does it)
>
> ?
>
> Any noticable effect? I don't like doing stuff where we don't know _why_
> we do them.

Cause the vt8235 guys did ;) The ide controllers appear to be the same,
even the device ids are the same, and datasheet pages are very nearly
identical. The vt8235 guys knew what they were doing, and I've tried
making changes to make the controller act like it's supposed to/is
documented to. Just doesn't work quite like it should.
> > // Force interrupts to use compat mode - just like Award bios
>
> Please drop these comments (they're from vt8235 anyway). You have the
> datasheets, so please check if this stuff is required at all, and if yes
> add a descriptive comment which says _why_ it's needed and what it does.

According to the datasheets, it's not. In the real world, it is.
> > pci_write_config8(dev, 0x3d, 0x0);
> > pci_write_config8(dev, 0x3c, 0xff);
> >
> > +}
> >
> > +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,
>
> 82C586_1? Is that really correct?

Yes, yes it is. BTW, vt82c586 was used in socket 7 systems, if that
gives you any idea how long via's been using this device id. I imagine
it must make driver developer's lives hell.
> > @@ -0,0 +1,2 @@
> > +//GPL here
> > 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,250 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Corey Osgood
> > 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
> > */
>
> Let's check the diff to Corey's original version of the file. I have the
> impression it's pretty small. Can we make a generic early_smbus file for
> _all_ southbridges, where you only have to pass a few parameters for the
> specific southbridge?

I've done some more work on this file (since it was all I could do while
I waited for datasheets), so it has evolved a bit. I can either patch
against this version or send in a patch of my own, but my patch does
require a tiny change to early mainboard init (auto.c or car's
equivalent) (add "smbus_fixup()" into auto.c right before calling any
functions that call smbus functions).
> > +static void smbus_reset(void)
> > +{
> > /* four resets? a little overboard? or just frustrated? */
> > outb(HOST_RESET, SMBHSTSTAT);
> >
> > +#if 0
> > outb(HOST_RESET, SMBHSTSTAT);
> > outb(HOST_RESET, SMBHSTSTAT);
> > outb(HOST_RESET, SMBHSTSTAT);
> >
> > +#endif
>
> Please check the datasheet whether it's necessary to do this four times.
> If no, drop the other calls.

It's not, the comment can be removed as well. Adopted from vt8235, guess
they were having smbus problems.
> > +/* Public functions */
> >
> > +static unsigned int get_spd_data(unsigned int dimm, unsigned int offset)
>
> It's static, so not really public?

Oops, it shouldn't be.
> > +{
> > unsigned int 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();
> > /* Do some mathmatic magic */
> > dimm = (dimm << 1);
> >
> > +//      dimm &= 0x0E;
> > +//      dimm |= 0xA1;
> > dimm |= 1;
> > outb(dimm, SMBXMITADD);
> > outb(offset, SMBHSTCMD);
> > outb(0x48, SMBHSTCTL);
>
> Please explain (add a comment).

I'll fix it in another patch. These are just "normal" smbus functions,
the only calculation going on was so I could use a dimm number instead
of an address.
> > 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)"? */
> >
> > +}
> >
> > +static unsigned int smbus_read_byte(unsigned int dimm, unsigned int offset)
> > +{
> > unsigned int data;
> > data = get_spd_data(dimm, offset);
> > return data;
>
> Why not
>
> return get_spd_data(dimm, offset);

Just rename get_spd_data to smbus_read_byte, drop the wrapper function,
and remove those two commented lines altogether.
> > /* make it work for I/O ... */
> > pci_write_config16(dev, 0x04, 0x0001);
> > /* The other, slightly hackish, fixup method */
> > for (c = 0; c < 30; c++)
> > get_spd_data(0x50, c);
>
> ?

Sorry, either he or I removed the previous comment to this. The
vt8237r's smbus isn't ready instantly, so this just starts pulling data
from it until it gets some non-invalid data. vt8235 uses a large delay
that I didn't like. I've moved this into another function (the
aformentioned smbus_fixup), and I still need to make it cycle through
the possible dimms looking for valid data, instead of just probing the
first one.
> > 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>
> > +#include "vt8237r.h"
> >
> > +static void sata_init(struct device *dev)
> > +{
> > uint8_t reg;
> > printk_debug("Configuring VIA SATA Controller\n");
> > /* class IDE Disk */
> > reg = pci_read_config8(dev, 0x45);
>
> 0x45 #define

If we start #define-ing every single register we'll have a couple pages
of just defines. Especially once you see the cn700's ram init files. And
then if someone wants to check a half a dozen registers against the
datasheets and lspci, they need to look at the value (say, SATA_CC for
this one), find that in either the top of the file or some header, then
go back to the function call, and compare the values. For one register,
it's a minor annoyance, but for many it's a big hassle that slows down
what should be a quick, simple process. I'd much rather see a quick
comment that describes the register (say /* Set SATA to IDE class, not
RAID */ in this case) than a define that obfuscates the register number.
> > +extern struct chip_operations southbridge_via_vt8237r_ops;
> >
> > +struct southbridge_via_vt8237r_config {
> > /* PCI function enables */
> > /* i.e. so that pci scan bus will find them. */
> > /* I am putting in IDE as an example but obviously this needs
> > * to be more complete!
> > */
> > int enable_ide;
> > /* enables of functions of devices */
> > /* USB removed, code comments essentially said it should have been long ago */
> > int enable_native_ide;
> > /* No serial on vt8237 */
> > int enable_keyboard;
> > int enable_nvram;
>
> I think you're not yet using most of this. Drop the comments please, and
> add only the variables you actually use for now. Example:
> http://tracker.linuxbios.org/trac/LinuxBIOS/browser/trunk/LinuxBIOSv2/src/so...

USB will return, unfortunately. Not sure why I need it and he doesn't.
All of these variables can be dropped for now, we'll add them back
later. This won't be as bad as the i810, I'll keep working on this to
make sure it's done right in the long run.
> > +};
> > 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,338 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Based on other VIA SB code.
>
> Drop this, or if the amount of copied code is non-trivial add the
> respective copyright holder here, too.

Please keep the comment and add the other copyright holders. Not sure if
they're listed in the old files, but svn info should tell you who
committed them, and from what I gather it was mostly the developers
committing their own patches back then.
> > /* 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... */
> >
> > +};

Can you please explain this a little better? Do these need to be fixed
up for proper IOAPIC routing? Perhaps a comment for idiots like me?
> > +static void setup_ioapic(unsigned long ioapic_base)
> > +{
> > int i;
> > unsigned long value_low, value_high, adr;
> > volatile unsigned long *l;
> > struct ioapicreg *a = ioapicregvalues;
> > //delivered to CPU0
> > ioapicregvalues[0].value_high = (lapicid()) << (56 - 32);
> > l = (unsigned long *) ioapic_base;
> > l[0] = 3;		//set APIC to FSB message
> > adr = l[4];
> > l[4] = (adr & 0xFFFFFE) | 1;
> > l[0] = 0;		//set APIC ADDR - this will be APIC 2
> > adr = l[4];
> > l[4] = (adr & 0xF0FFFF) | (2 << 24);
> > for (i = 0;
> >     i < sizeof(ioapicregvalues) / sizeof(ioapicregvalues[0]);
>
> Should be ARRAY_SIZE later, not sure if we already added that to 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 */
> > 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) which I
> > can't figure out how to do !!!!
> > */
> >
> > +void setup_pm(device_t dev)
> > +{
> > // Set gen config 0
> > pci_write_config8(dev, 0x80, 0x20);
> > // Set ACPI base address to IO 0x500
> > pci_write_config16(dev, 0x88, 0x501);
> > // set ACPI irq to 9, need to set IRQ 9 override to level!
> > pci_write_config8(dev, 0x82, 0x49);
> > // primary interupt channel
> > pci_write_config16(dev, 0x84, 0x30b2);
> > // throttle / stop clock control
> > pci_write_config8(dev, 0x8d, 0x18);
> > pci_write_config8(dev, 0x93, 0x88);
> > pci_write_config8(dev, 0x94, 0xa4);
> > pci_write_config8(dev, 0x95, 0xcc);
> > pci_write_config8(dev, 0x98, 0);
> > pci_write_config8(dev, 0x99, 0x1e);	//??
> > /* enable SATA LED, disable CPU Frequency Change  */
>
> Why the CPU freq. change? Can you elaborate?

Nope, that's just exactly what the datasheet says. The value matches the
jetway's bios, and frequency scaling still works, so I'm not sure what
the deal with it is.
> > 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 access (and setup like award)
> > pci_write_config8(dev, 0x81, 0x84);
> > outw(0xffff, 0x500);
> > outw(0xffff, 0x520);
> > outw(0xffff, 0x528);
> > outl(0xffffffff, 0x530);
> > outw(0x0, 0x524);
> > outw(0x0, 0x52a);
> > outw(0x0, 0x52c);
> > outl(0x0, 0x534);
> > outl(0x0, 0x538);	//fix
> > outb(0x0, 0x542);
> > outw(0x001, 0x504);
>
> ?
>
> Needs some comments.
> From vt8235, isn't needed on vt8237r, at least not for me.
> > // 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
> > pci_write_config8(dev, 0x41, 0x0);
>
> Should be the maximum possible, does that make sense? Or it could be
> a config option.

I'll test this soon. Do as you wish with it, I can patch it up later if
needed.
> > // Set bit 6 of 0x40, because Award does it (IO recovery time)
> > // IMPORTANT FIX - EISA 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);
> > // Set 0x42 to 0xf8 to match Award bios
>
> Nah, please don't try to blindly match what other BIOSes do, only if
> there's a good reason (functionality or performance). We want to
> know exactly _why_ this is done (or not).

It's interrupt options, explaning would be a pain without just copy and
pasting the datasheet. Perhaps a "refer to the datasheet" would be best.
> > 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 to match award (use INIT# as cpu reset) */
> > enables = pci_read_config8(dev, 0x4f);
> > enables |= 0x08;
> > pci_write_config8(dev, 0x4f, enables);
> > /* enable serial irq */
> > pci_write_config8(dev, 0x52, 0x9);
> > /* dma */
> > //pci_write_config8(dev, 0x53, 0x00);
> > //enable HPET, ACPI has define to fixed addr
> >
> > +#define HPET_ADDR  0xfed00000ULL
>
> Put this at the top of the file or in the *.h file. Does it make sense
> to make a config option out of it?

No config option, it's a fixed address. I'm writing this from the bottom
up, so the lower posts explain a bit more. Should be at the top of the
file though, or else hardcoded (or perhaps even defined in acpi files,
since these seem to be generic values). It might already be defined in
some acpi file.
> > +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 = 0xfec00000;
>
> Is this different from HPET_ADDR? Should it?

Yes, this is ioapic not hpet.
> > vt8237r_init(dev);
> > pci_routing_fixup(dev);
> > setup_ioapic(0xfec00000);
>
> Why hardcode this number here again?

This is a non-configurable address, messing around with it will bork
things up.
> > if (resource) {
> > report_resource_stored(dev, resource, "<mmconfig>");
> > /* Remember this resource has been stored */
> > resource->flags |= IORESOURCE_STORED;
> > pci_write_config8(dev, 0x61, (resource->base >> 28));
> > reg = pci_read_config8(dev, 0x60);
> > reg |= 0x3;
> > /* enable MMCONFIG decoding */
> > pci_write_config8(dev, 0x60, reg);
> > }
> > pci_dev_set_resources(dev);
> >
> > +}
> >
> > +static void apic_mmconfig_read_resources(device_t dev)
> > +{
> > struct resource *res;
> > pci_dev_read_resources(dev);
> > res = new_resource(dev, 0x40);
> > /* NB APIC fixed to this addr */
> > res->base = 0xfecc0000;
>
> Hardcoded again. I think this should be a #define in *.h or even a
> config option, rather than hardcoding it in all places.

Another fixed address, I think this one is LAPIC. Config options are not
an option, these aren't configurable.
> > +##
> > +## Build code to export an x86 MP table
> > +## Useful for specifying IRQ routing values
> > +##
> > +default HAVE_MP_TABLE=1
> >
> > +##
> > +## Build code to export a CMOS option table
> > +##
> > +##default HAVE_OPTION_TABLE=1
>
> Either enable it, or drop cmos.layout below.

Huh? afaik it is enabled (with default USE_OPTION_TABLE !=
USE_FALLBACK_IMAGE below).
> > Index: src/mainboard/asus/a8v/acpi_tables.c
> >
> > --- src/mainboard/asus/a8v/acpi_tables.c	(revision 0)
> > +++ src/mainboard/asus/a8v/acpi_tables.c	(revision 0)
> > @@ -0,0 +1,155 @@
> > +/*
> > LinuxBIOS ACPI Table support
> > written by Stefan Reinauer
> > ACPI FADT, FACS, and DSDT table support added by
> > Nick Barker , and those portions
> > (C) Copyright 2004 Nick Barker
> > (C) Copyright 2005 Stefan Reinauer
> > */
>
> Missing license. Please check back with Stefan/Nick, but I guess it's
> GPL (v2? v2-or-later?).
>
> How similar is this file to the original one? Does it make sense to
> drop this file and make the original one a generic version which can be
> used by multiple chipsets?

Nope, different boards have different tables. A uniprocessor board has
fewer tables than a multiprocessor board, and different archs also have
different/other tables. I think acpi is as generic as it's going to get.
> > Index: src/mainboard/asus/a8v/fadt.c
> >
> > --- src/mainboard/asus/a8v/fadt.c	(revision 0)
> > +++ src/mainboard/asus/a8v/fadt.c	(revision 0)
> > @@ -0,0 +1,157 @@
> > +/*
> > ACPI - create the Fixed ACPI Description Tables (FADT)
> > (C) Copyright 2004 Nick Barker
> > 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 <string.h>
> > +#include <arch/acpi.h>
> >
> > +void acpi_create_fadt(acpi_fadt_t * fadt, acpi_facs_t * facs, void *dsdt)
> > +{
>
> Did you modify this file? How? Should probably be a common global file,
> not copied in each target?

The FADT is/should be mainboard-specific. So please don't try to
generic-ize it. My fadt is somewhat different, FWIW.

-Corey