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

Corey Osgood corey.osgood at gmail.com
Sun Oct 14 04:21:45 CEST 2007


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 <corey_osgood at verizon.net>
>> + *
>> + * 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 <r.marek at assembler.cz>
>> + *
>> + * 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/southbridge/amd/cs5530/chip.h
>   

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



More information about the coreboot mailing list