Re: [coreboot] [PATCH] v3: vt8237 stage1 support

[Carl-Daniel Hailfinger]

On 14.10.2008 05:33, Corey Osgood wrote:
> See attached. Stage2 has to wait until CAR can be disabled on C7.
>
> Add stage1 support for vt8237[RS] to v3.
>
> Signed-off-by: Corey Osgood

Looks good. I didn't understand one part of the code and would
appreciate clarifications, though.
Acked-by: Carl-Daniel Hailfinger
> Index: southbridge/via/vt8237/vt8237.h
>
> --- southbridge/via/vt8237/vt8237.h	(revision 0)
> +++ southbridge/via/vt8237/vt8237.h	(revision 0)
> @@ -0,0 +1,90 @@
> +/*
> This file is part of the coreboot 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
> */
>
> +#ifndef SOUTHBRIDGE_VIA_VT8237_VT8237_H
> +#define SOUTHBRIDGE_VIA_VT8237_VT8237_H
>
> +#include <types.h>
>
> +/* Static resources for the VT8237R southbridge */
>
> +#define VT8237R_APIC_ID			0x2
> +#define VT8237R_ACPI_IO_BASE		0x500
> +/* 0x0 disabled, 0x2 reserved, 0xf = IRQ15 */
> +#define VT8237R_ACPI_IRQ		0x9
> +#define VT8237S_SPI_MEM_BASE		0xfed02000ULL
> +#define VT8237R_HPET_ADDR		0xfed00000ULL
> +#define VT8237R_APIC_BASE		0xfec00000ULL
>
> +/* IDE */
> +#define IDE_CS				0x40
> +#define IDE_CONF_I			0x41
> +#define IDE_CONF_II			0x42
> +#define IDE_CONF_FIFO			0x43
> +#define IDE_MISC_I			0x44
> +#define IDE_MISC_II			0x45
> +#define IDE_UDMA			0x50
>
> +/* SMBus */
> +#define VT8237R_POWER_WELL		0x94
> +#define VT8237R_SMBUS_IO_BASE_REG	0xd0
> +#define VT8237R_SMBUS_HOST_CONF		0xd2
>
> +#define SMBHSTSTAT			0x0
> +#define SMBSLVSTAT			0x1
> +#define SMBHSTCTL			0x2
> +#define SMBHSTCMD			0x3
> +#define SMBXMITADD			0x4
> +#define SMBHSTDAT0			0x5
>
> +#define HOST_RESET 			0xff
> +/* 1 in the 0 bit of SMBHSTADD states to READ. */
> +#define READ_CMD			0x01
> +#define SMBUS_TIMEOUT			(100 * 1000 * 10)
> +#define I2C_TRANS_CMD			0x40
> +#define CLOCK_SLAVE_ADDRESS		0x69
>
> +struct vt8237_network_rom {
> u8 mac_address[6];
> u8 phy_addr;
> u8 res1;
> u16 sub_sid;
> u16 sub_vid;
> u16 pid;
> u16 vid;
> u8 pmcc;
> u8 data_sel;
> u8 pmu_data_reg;
> u8 aux_curr;
> u16 reserved;
> u8 min_gnt;
> u8 max_lat;
> u8 bcr0;
> u8 bcr1;
> u8 cfg_a;
> u8 cfg_b;
> u8 cfg_c;
> u8 cfg_d;
> u8 checksum;
>
> +} __attribute__ ((packed));
>
> +void enable_smbus(u16);
> +u8 smbus_read_byte(u16, u8, u16);
>
> +#endif
> Index: southbridge/via/vt8237/stage1.c
> ===================================================================
> --- southbridge/via/vt8237/stage1.c	(revision 0)
> +++ southbridge/via/vt8237/stage1.c	(revision 0)
> @@ -0,0 +1,322 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2007-2008 Corey Osgood
> 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 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 <types.h>
> +#include <lib.h>
> +#include <console.h>
> +#include <device/pci.h>
> +#include <io.h>
> +#include <device/pci_ids.h>
> +#include <spd.h>
> +#include "vt8237.h"
>
> +/* TODO List:
> Merge the rest of the functions from v2, except smbus_fixup which doesn't
> seem to be necessary any more (?)
> Clean up vt8237_early_network_init.
> Comments in code indicate that it's broken?
> Figure out if the smbus actually needs to be reset after every transaction.
> */
>
> +/**
> Print an error, should it occur. If no error, just exit.
> @param host_status The data returned on the host status register after
>       a transaction is processed.
> @param loops The number of times a transaction was attempted.
> */
>
> +static void smbus_print_error(u8 host_status, int loops)
> +{
> /* Check if there actually was an error. */
> if ((host_status == 0x00 || host_status == 0x40 ||
> host_status == 0x42) && (loops < SMBUS_TIMEOUT))
> {
> printk(BIOS_SPEW, "SMBus Ready/Completed Successfully\n");
> return;
> }
> if (loops >= SMBUS_TIMEOUT)
> printk(BIOS_ERR, "SMBus Timed out\n");
> if (host_status & (1 << 4))
> printk(BIOS_ERR, "Interrupt/SMI# was Failed Bus Transaction\n");
> if (host_status & (1 << 3))
> printk(BIOS_ERR, "Bus error\n");
> if (host_status & (1 << 2))
> printk(BIOS_ERR, "Device error\n");
> if (host_status & (1 << 1))
> printk(BIOS_SPEW, "Interrupt/SMI# completed successfully\n");
> if (host_status & (1 << 0))
> printk(BIOS_ERR, "Host busy\n");
>
> +}
>
> +/**
> Reset and take ownership of the SMBus.
> */
>
> +static void smbus_reset(u16 smbus_io_base)
> +{
> outb(HOST_RESET, smbus_io_base + SMBHSTSTAT);
> /* Datasheet says we have to read it to take ownership of SMBus. */
> smbus_print_error(inb(smbus_io_base + SMBHSTSTAT), 0);
>
> +}
>
> +/**
> Wait for the SMBus to become ready to process the next transaction.
> */
>
> +static void smbus_wait_until_ready(u16 smbus_io_base)
> +{
> int loops;
> printk(BIOS_SPEW, "Waiting until SMBus ready\n");
> loops = 0;
> while ((inb(smbus_io_base + SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
> ++loops;
> smbus_print_error(inb(smbus_io_base + SMBHSTSTAT), loops);
>
> +}
>
> +/**
> Read a byte from the SMBus.
> @param dimm The address location of the DIMM on the SMBus.
> @param offset The offset the data is located at.
> */
>
> +u8 smbus_read_byte(u16 dimm, u8 offset, u16 smbus_io_base)
> +{
> u8 val;
> printk(BIOS_SPEW, "SMBus Read from DIMM %d at address 0x%x\n",
> 		(int)dimm, offset);
> smbus_reset(smbus_io_base);
> /* Clear host data port. */
> outb(0x00, smbus_io_base + SMBHSTDAT0);
> /* Doesn't seem to be necessary...*/
> /* udelay(1); */
> smbus_wait_until_ready(smbus_io_base);
> /* With this, addresses are 0x50, 0x51, etc. Without it,
> * addresses would be 0xa1, 0xa3, etc */
> dimm = (dimm << 1) | 1;
> outb(dimm, smbus_io_base + SMBXMITADD);
> outb(offset, smbus_io_base + SMBHSTCMD);
> /* Start transaction, byte data read. */
> outb(0x48, smbus_io_base + SMBHSTCTL);
> /* udelay(1); */
> smbus_wait_until_ready(smbus_io_base);
> val = inb(smbus_io_base + SMBHSTDAT0);
> printk(BIOS_SPEW, "Read: 0x%x\n", val);
> /* TODO: Is this necessary? */
> smbus_reset(smbus_io_base);
> return val;
>
> +}

Two empty lines
> +/**
> Enable the smbus on vt8237-based systems
> @param smbus_io_base: The SMBus I/O base, usually 0x400
> */
>
> +void enable_smbus(u16 smbus_io_base)
> +{
> u32 dev;
> /* Power management controller */
> pci_conf1_find_device(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VT8237R_LPC,
> 		&dev);

OK, here you look for PCI_DEVICE_ID_VIA_VT8237R_LPC
> if (pci_conf1_read_config16(dev, PCI_DEVICE_ID) !=
> 		PCI_DEVICE_ID_VIA_VT8237R_LPC) {

And here you check what? If pci_conf1_find_device is not broken and
returns a nonzero value, the expression above should be always false. I
believe this is a romcc relic.
> /* Power management controller */
> pci_conf1_find_device(PCI_VENDOR_ID_VIA,
> 			PCI_DEVICE_ID_VIA_VT8237S_LPC, &dev);
> if (pci_conf1_read_config16(dev, PCI_DEVICE_ID) != 
> 			PCI_DEVICE_ID_VIA_VT8237R_LPC)

Same here.
> 	{
> 		printk(BIOS_ERR, "Power management controller "
> 		"	not found! Using hardcoded default.\n");
> 		dev = PCI_BDF(0, 17, 0);
> 	} else {
> 		printk(BIOS_DEBUG, "VT8237S Power management "
> 			"controller found at 0x%x\n", dev);
> }

Indentation of the brace above.
> } else {
> printk(BIOS_DEBUG, "VT8237R Power management controller found "
> 				"at 0x%x\n", dev);
> }
> /* 7 = SMBus Clock from RTC 32.768KHz
> * 5 = Internal PLL reset from susp
> */
> pci_conf1_write_config8(dev, VT8237R_POWER_WELL, 0xa0);
> /* Enable SMBus. */
> pci_conf1_write_config16(dev, VT8237R_SMBUS_IO_BASE_REG,
> 	   smbus_io_base | 0x1);
> /* SMBus Host Configuration, enable. */
> pci_conf1_write_config8(dev, VT8237R_SMBUS_HOST_CONF, 0x01);
> /* Make it work for I/O. */
> pci_conf1_write_config16(dev, PCI_COMMAND, PCI_COMMAND_IO);
> /* reset smbus */
> smbus_reset(smbus_io_base);
> /* Reset the internal pointer. */
> inb(smbus_io_base + SMBHSTCTL);
>
> +}
>
> +/* TODO:
> Magic numbers -> #defines
> fix?
> clean up
> */
>
> +/* offset 0x58
> 31:20 	reserved
> 19:16	4 bit position in shadow EEPROM
> 15:0		data to write
> offset 0x5c
> 31:28	reserved
> 27 		ERDBG - enable read from 0x5c
> 26		reserved
> 25		SEELD
> 24		SEEPR - write 1 when done updating, wait until SEELD is set to 1, sticky
> cleared by reset, if it is 1 writing is disabled
> 19:16	4 bit position in shadow EEPROM
> 15:0		data from shadow EEPROM
> after PCIRESET SEELD and SEEPR must be 1 and 1
>
> +*/
>
> +/* 1 = needs PCI reset, 0 don't reset, network initialized */
>
> +/* fixme maybe close the debug register after use? */
>
> +#define LAN_TIMEOUT 0x7FFFFFFF
>
> +int vt8237_early_network_init(struct vt8237_network_rom *rom) {
> struct vt8237_network_rom n;
> int loops = 0;
> u32 dev;
> u32 tmp;
> u8 status;
> u16 *rom_write;
> unsigned int checksum;
> int i;
> /* Network adapter */
> pci_conf1_find_device(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_VT8237_LAN,
> 		&dev);
> if (pci_conf1_read_config16(dev, PCI_DEVICE_ID) !=
> 		PCI_DEVICE_ID_VIA_VT8237_LAN) {
>    printk(BIOS_ERR, "Network is disabled, please enable\n");

Similar problem as above. pci_conf1_find_device has a perfectly usable
return code.
>    return 0;
> }
> tmp = pci_conf1_read_config32(dev, 0x5c);
> /* enable ERDBG */
> tmp |= 0x08000000;
> pci_conf1_write_config32(dev, 0x5c, tmp);
> status = ((pci_conf1_read_config32(dev, 0x5c) >> 24) & 0x3);
> if (status == 3) {
>    /* network controller OK, EEPROM loaded */
>    return 0;
> }
> if (rom == NULL) {
>    printk(BIOS_ERR, "No configuration data specified, using default MAC!\n");
> n.mac_address[0] = 0x0;
> n.mac_address[1] = 0x0;
> n.mac_address[2] = 0xde;
> n.mac_address[3] = 0xad;
> n.mac_address[4] = 0xbe;
> n.mac_address[5] = 0xef;
> n.phy_addr = 0x1;
> n.res1 = 0x0;
> n.sub_sid = 0x102;
> n.sub_vid = 0x1106;
> n.pid = 0x3065;
> n.vid = 0x1106;
> n.pmcc = 0x1f;
> n.data_sel = 0x10;
> n.pmu_data_reg = 0x0;
> n.aux_curr = 0x0;
> n.reserved = 0x0;
> n.min_gnt = 0x3;
> n.max_lat = 0x8;
> n.bcr0 = 0x9;
> n.bcr1 = 0xe;
> n.cfg_a = 0x3;
> n.cfg_b = 0x0;
> n.cfg_c = 0x40;
> n.cfg_d = 0x82;
> n.checksum = 0x0;
> rom = &n;
> }
> rom_write = (u16 *) rom;
> checksum = 0;
> /* write all data except checksum and second to last byte */
> tmp &= 0xff000000; /* leave reserved bits in */
> for (i = 0; i < 15; i++) {
> pci_conf1_write_config32(dev, 0x58, tmp | (i << 16) | rom_write[i]);
> /* lame code fixme */
> checksum += rom_write[i] & 0xff;
> //checksum %= 256;
> checksum += (rom_write[i] >> 8) & 0xff;
> //checksum %= 256;
> }
> checksum += (rom_write[15] & 0xff);
> checksum = ~(checksum & 0xff);
> tmp |= (((checksum & 0xff) << 8) | rom_write[15]);
> /* write last byte and checksum */
> pci_conf1_write_config32(dev, 0x58, (15 << 16) |  tmp);
> tmp = pci_conf1_read_config32(dev, 0x5c);
> pci_conf1_write_config32(dev, 0x5c, tmp | 0x01000000); /* toggle SEEPR */
> /* Yes, this is a mess, but it's the easiest way to do it. */
> while ( (((pci_conf1_read_config32(dev, 0x5c) >> 25) & 1) == 0)
> 	&& (loops < LAN_TIMEOUT))
> ++loops;
> if (loops >= LAN_TIMEOUT) {
>    printk(BIOS_ERR, "Timout - LAN controller did not accept configuration\n");
>    return 0;
> }
> /* we are done, config will be used after PCIRST# */
> return 1;
>
> +}
> Index: southbridge/via/vt8237/Makefile
> ===================================================================
> --- southbridge/via/vt8237/Makefile	(revision 0)
> +++ southbridge/via/vt8237/Makefile	(revision 0)
> @@ -0,0 +1,25 @@
> +##
> +## This file is part of the coreboot project.
> +##
> +## Copyright (C) 2008 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
> +##
>
> +ifeq ($(CONFIG_SOUTHBRIDGE_VIA_VT8237),y)
>
> +STAGE2_CHIPSET_SRC +=
>
> +endif
> Index: Kconfig
> ===================================================================
> --- Kconfig	(revision 923)
> +++ Kconfig	(working copy)
> @@ -106,6 +108,8 @@
>    boolean
>  config SOUTHBRIDGE_AMD_RS690
>    boolean
> +config SOUTHBRIDGE_VIA_VT8237
> boolean
>
> # Super I/Os:
>  config SUPERIO_WINBOND_W83627HF
> Index: include/device/pci_ids.h
> ===================================================================
> --- include/device/pci_ids.h	(revision 923)
> +++ include/device/pci_ids.h	(working copy)
> @@ -263,4 +263,16 @@
>  #define PCI_DEVICE_ID_NVIDIA_MCP55_TRIM         0x036A
>  #define PCI_DEVICE_ID_NVIDIA_MCP55_PMU          0x036B
>
> +#define PCI_VENDOR_ID_VIA			0x1106
> +#define PCI_DEVICE_ID_VIA_VT8237_EHCI		0x3104
> +#define PCI_DEVICE_ID_VIA_VT8237_LAN		0x3065
> +#define PCI_DEVICE_ID_VIA_VT8237R_LPC		0x3227
> +#define PCI_DEVICE_ID_VIA_VT8237S_LPC		0x3372
> +#define PCI_DEVICE_ID_VIA_VT8237_PATA		0x0571
> +#define PCI_DEVICE_ID_VIA_VT8237R_SATA		0x3149
> +#define PCI_DEVICE_ID_VIA_VT8237S_SATA		0x5372
> +#define PCI_DEVICE_ID_VIA_VT8237_UHCI		0x3038
> +#define PCI_DEVICE_ID_VIA_VT8237_VLINK		0x287e
>
> #endif /* DEVICE_PCI_IDS_H */
> Index: include/spd.h
> ===================================================================
> --- include/spd.h	(revision 923)
> +++ include/spd.h	(working copy)
> @@ -105,10 +105,12 @@
>  #define SPD_tRAS                            SPD_MIN_ACTIVE_TO_PRECHARGE_DELAY
>  #define SPD_BANK_DENSITY                    SPD_DENSITY_OF_EACH_ROW_ON_MODULE
>  #define SPD_ADDRESS_CMD_HOLD                SPD_CMD_SIGNAL_INPUT_HOLD_TIME
> -#define SPD_tRC				    41	/* SDRAM Device Minimum Active to Active/Auto Refresh Time (tRC) */
> -#define SPD_tRFC			    42	/* SDRAM Device Minimum Auto Refresh to Active/Auto Refresh (tRFC) */
> +#define SPD_tWR				    SPD_WRITE_RECOVERY_TIME
> +#define SPD_tWTR			    SPD_INT_WRITE_TO_READ_DELAY
> +#define SPD_tRTP			    SPD_INT_READ_TO_PRECHARGE_DELAY
> +#define SPD_tRC				    SPD_MIN_ACT_TO_ACT_AUTO_REFRESH
> +#define SPD_tRFC			    SPD_MIN_AUTO_REFRESH_TO_ACT
>
> /* SPD_MEMORY_TYPE values. */
>  #define SPD_MEMORY_TYPE_FPM_DRAM         1
>  #define SPD_MEMORY_TYPE_EDO              2