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

Corey Osgood corey.osgood at gmail.com
Sat Oct 13 06:06:44 CEST 2007


Few comments, please resend with these fixes and I'll ack, I need this
to get in as well (so I can figure out how to patch against it without
breaking things on your end).

> Index: src/southbridge/via/vt8237r/TODO
> ===================================================================
> --- src/southbridge/via/vt8237r/TODO    (revision 0)
> +++ src/southbridge/via/vt8237r/TODO    (revision 0)
> @@ -0,0 +1,26 @@
> +NB & SB
> +
> +Fix DMA to 0xe0000-0xeffff - done
> +Move NB APIC init to NB code - done
> +MMCONFIG for PCI is reserved but not propagated (0xe0000000) - done
> +HPET is disabled - fixed
> +
> +
> +Motherboard code:
> +Fix reboot -done
> +
> +
> +K8 CODE:
> +
> +Fix the 2 dimms memory - done
> +Fix the resource typo - done?
> +
> +
> +Fix enable more than 512KB flash chip

Hrm, not sure about this. vt8237r supports 4Mb to 16Mb flash, but only
if it's strapped correctly. I'm not sure why they did that, but it's
extremely annoying. So unless your chip is strapped correctly (or you
change the strapping) you can't use any larger than the motherboard
manufacturer decided to (here, it's 4MB :( )

> +Fix HPET acpi record to match the ACPI reported table
> +Fix the e820 reserved region for MMCONFIG
> +Fix PCIe startup - done
> +
> +Fix error conditiion when PCIe Link is not trained correctly.
> +Disable GART in NB?
> +

I have no objection to a TODO list, I rather like the idea. But please
have separate lists for each part.

> Index: src/southbridge/via/vt8237r/romstrap.inc
> ===================================================================
> --- src/southbridge/via/vt8237r/romstrap.inc	(revision 0)
> +++ src/southbridge/via/vt8237r/romstrap.inc	(revision 0)
> Index: src/southbridge/via/vt8237r/romstrap.lds
> ===================================================================
> --- src/southbridge/via/vt8237r/romstrap.lds    (revision 0)
> +++ src/southbridge/via/vt8237r/romstrap.lds    (revision 0)

What are these files all about? Do they really belong in vt8237r?

> +void set_led()
> +{
> +	// 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);
> +}

Please just remove this function and all calls to it.

> Index: src/southbridge/via/vt8237r/vt8237r.h
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r.h    (revision 0)
> +++ src/southbridge/via/vt8237r/vt8237r.h    (revision 0)
> @@ -0,0 +1,2 @@
> +//GPL here
> +

Same here, no need for an empty file.

> +static unsigned int get_spd_data(unsigned int dimm, unsigned int offset)
> +{
> +    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);
> +
> +    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;
> +}

Please just rename get_spd_data and remove the commented lines entirely.
I know I said otherwise in the past. And nuke that line about mathmatic
magic, I think I must have been high when I wrote that.

> Index: src/southbridge/via/vt8237r/vt8237_bridge.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237_bridge.c    (revision 0)
> +++ src/southbridge/via/vt8237r/vt8237_bridge.c    (revision 0)
> @@ -0,0 +1,60 @@
> +/*
> + * 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 <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ops.h>
> +#include <device/pci_ids.h>
> +#include <console/console.h>
> +
> +static void smth_enable(struct device *dev)
> +{
> +
> +//      print_err("B188 device dump\n");
> +//    VIA recommends this, sorry no known info
> +    writeback(dev, 0x40, 0x91);
> +    writeback(dev, 0x41, 0x40);
> +    writeback(dev, 0x43, 0x44);
> +    writeback(dev, 0x44, 0x31);
> +    writeback(dev, 0x45, 0x3a);
> +    writeback(dev, 0x46, 0x88);    //PCI ID
> +    writeback(dev, 0x47, 0xb1);    //PCI ID
> +    writeback(dev, 0x3e, 0x16);    //bridge control????
> +
> +//    dump_south(dev);
> +
> +
> +}
> +
> +static struct device_operations smth_ops = {
> +    .read_resources = pci_bus_read_resources,
> +    .set_resources = pci_dev_set_resources,
> +    .enable_resources = pci_bus_enable_resources,
> +    .enable = smth_enable,
> +    .scan_bus = pci_scan_bridge,
> +    .reset_bus = pci_bus_reset,
> +    .ops_pci = 0,
> +};
> +
> +
> +static struct pci_driver northbridge_driver __pci_driver = {
> +    .ops = &smth_ops,
> +    .vendor = PCI_VENDOR_ID_VIA,
> +    .device = 0xb188,
> +};

I don't think this is really part of the southbridge, rather the
northbridge. Biggest justification for that idea is that the device ID
is different from mine (0xb198), and mine is listed in the northbridge
docs. I know lspci lists it as part of the southbridge, but it's
probably wrong.

> Index: src/cpu/x86/lapic/lapic_cpu_init.c
> ===================================================================
> --- src/cpu/x86/lapic/lapic_cpu_init.c    (revision 2776)
> +++ src/cpu/x86/lapic/lapic_cpu_init.c    (working copy)
> @@ -60,7 +60,7 @@
>       * Starting actual IPI sequence...
>       */
>  
> -    printk_spew("Asserting INIT.\n");
> +    printk_err("Asserting INIT. on %x\n", apicid);
>  
>      /*
>       * Turn INIT on target chip

Yes to the change in the message, no to the change in printk level.
Please leave at spew or debug.

> Index: src/mainboard/asus/a8v/acpi-dsdt.aml
> ===================================================================
> Cannot display: file marked as a binary type.
> svn:mime-type = application/octet-stream
>
> Property changes on: src/mainboard/asus/a8v/acpi-dsdt.aml
> ___________________________________________________________________
> Name: svn:mime-type
>    + application/octet-stream

Remove this from the patch please ;)

Aside from that, there's a bunch of whitespace issues, etc, which can be
fixed later.

-Corey







More information about the coreboot mailing list