[coreboot] [PATCH] K8M890 support

[Carl-Daniel Hailfinger]

On 20.03.2008 00:59, Rudolf Marek wrote:
> Following patch adds K8M890 support. It initializes the AGP and
> graphics UMA.
> The V-link setup and HT bridge is redone, because VT8237A has it in
> another
> device. So far following combination of chipsets should work:
>
> K8T890CE + VT8237R
> K8M890 + VT8237R
>
> I will work on "VT8237A" as next, but trying to get from VIA VT8237S
> datasheets.
>
> As for the patch, some cosmetics may be needed. What I need is to
> check that I
> did not break anything - it works for me and PCI dump looks OK too.
> The new part
> is untested, but for example lowering total size of mem works. Other
> additions
> are quite simple to check. Comments / suggestions welcome.
>
> Oh and lets state: Signed-off-by: Rudolf Marek <r.marek at assembler.cz>

Great work!

foo
> Index: src/southbridge/via/vt8237r/vt8237r_bridge.c
> ===================================================================
> --- src/southbridge/via/vt8237r/vt8237r_bridge.c	(revision 3148)
> +++ src/southbridge/via/vt8237r/vt8237r_bridge.c	(working copy)
> @@ -1,56 +0,0 @@

This file is removed completely.

> Index: src/southbridge/via/k8t890/k8t890_traf_ctrl.c
> ===================================================================
> --- src/southbridge/via/k8t890/k8t890_traf_ctrl.c	(revision 3169)
> +++ src/southbridge/via/k8t890/k8t890_traf_ctrl.c	(working copy)
> @@ -68,15 +68,15 @@
>  	res->flags = IORESOURCE_MEM;
>  }
>  
> -static void traf_ctrl_enable(struct device *dev)
> +static void traf_ctrl_enable_generic(struct device *dev)
>  {
>  	volatile u32 *apic;
>  	u32 data;
>  
> -	/* Enable D3F1-D3F3, no device2 redirect, enable just one device behind
> +	/* no device2 redirect, enable just one device behind
>  	 * bridge device 2 and device 3).
>  	 */
> -	pci_write_config8(dev, 0x60, 0x88);
> +	pci_write_config8(dev, 0x60, 0x08);
>  
>  	/* Will enable MMCONFIG later. */
>  	pci_write_config8(dev, 0x64, 0x23);
> @@ -104,16 +104,41 @@
>  	apic[4] = (data & 0xF0FFFF) | (K8T890_APIC_ID << 24);
>  }
>  
> -static const struct device_operations traf_ctrl_ops = {
> +static void traf_ctrl_enable_kt890(struct device *dev)

KT890 or K8T890? The same problem also happens elsewhere.

> +{
> +	u8 reg;
> +
> +	traf_ctrl_enable_generic(dev);
> +
> +	/* Enable D3F1-D3F3 */
> +	reg = pci_read_config8(dev, 0x60);
> +	pci_write_config8(dev, 0x60, 0x80 | reg);
> +}
> +
> +static const struct device_operations traf_ctrl_ops_m = {
>  	.read_resources		= apic_mmconfig_read_resources,
>  	.set_resources		= mmconfig_set_resources,
>  	.enable_resources	= pci_dev_enable_resources,
> -	.enable			= traf_ctrl_enable,
> +	.enable			= traf_ctrl_enable_generic,

*_generic

>  	.ops_pci		= 0,
>  };
>  
> -static const struct pci_driver northbridge_driver __pci_driver = {
> -	.ops	= &traf_ctrl_ops,
> +static const struct device_operations traf_ctrl_ops_t = {
> +	.read_resources		= apic_mmconfig_read_resources,
> +	.set_resources		= mmconfig_set_resources,
> +	.enable_resources	= pci_dev_enable_resources,
> +	.enable			= traf_ctrl_enable_kt890,

*_kt890
I'm not really happy about the generic/kt890 naming. Sure, it is
efficient, but also a bit confusing. Maybe a simple wrapper for
k8m890 would look better.

> +	.ops_pci		= 0,
> +};
> +
> +static const struct pci_driver northbridge_driver_t __pci_driver = {
> +	.ops	= &traf_ctrl_ops_t,
>  	.vendor	= PCI_VENDOR_ID_VIA,
>  	.device	= PCI_DEVICE_ID_VIA_K8T890CE_5,
>  };
> +
> +static const struct pci_driver northbridge_driver_m __pci_driver = {
> +	.ops	= &traf_ctrl_ops_m,
> +	.vendor	= PCI_VENDOR_ID_VIA,
> +	.device	= PCI_DEVICE_ID_VIA_K8M890CE_5,
> +};
> Index: src/southbridge/via/k8t890/k8t890_dram.c
> ===================================================================
> --- src/southbridge/via/k8t890/k8t890_dram.c	(revision 3169)
> +++ src/southbridge/via/k8t890/k8t890_dram.c	(working copy)
> @@ -23,6 +23,8 @@
>  #include <console/console.h>
>  #include <cpu/x86/msr.h>
>  #include <cpu/amd/mtrr.h>
> +#include <bitops.h>
> +#include "k8t890.h"
>  
>  static void dram_enable(struct device *dev)
>  {
> @@ -59,10 +61,73 @@
>  	reg = pci_read_config16(dev, 0x88);
>  	reg &= 0xf800;
>  
> -	pci_write_config16(dev, 0x88, (msr.lo >> 24) | reg);
> +	/* The Address Next to the Last Valid DRAM Address */
> + 	pci_write_config16(dev, 0x88, (msr.lo >> 24) | reg);

The line above is a pure whitespace change (it adds a space at the
beginning of the line).

>  }
>  
> -static const struct device_operations dram_ops = {
> +static struct resource *resmax;
> +
> +static void get_memres(void *gp, struct device *dev, struct resource *res)
> +{
> +	unsigned int *fbsize = (unsigned int *) gp;
> +	uint64_t proposed_base = res->base + res->size - *fbsize;
> +
> +	printk_debug("get_memres: res->base=%llx res->size=%llx %d %d %d\n",
> +			res->base, res->size, (res->size > *fbsize), 
> +			(!(proposed_base & (*fbsize - 1))),
> +			(proposed_base < ((uint64_t) 0xffffffff)));
> +
> +	/* if we fit and also align OK, and must be below 4GB */
> +	if ((res->size > *fbsize) && (!(proposed_base & (*fbsize - 1))) && 
> +		(proposed_base < ((uint64_t) 0xffffffff) )) {
> +		resmax = res;
> +	}
> +}
> +
> +	/* if internal VGA is to be used here:
> +	 * enable the internal GFX bit 7 0xa1
> +	 * 6:4 X fbuffer size will be  2^(X+2) or 100 = 64MB, 101 = 128MB 
> +	 * 3:0 BASE [31:28]
> +	 * 0xa0 7:1 BASE [27:21] bit0 enable CPU access
> +	 */

This comment seems to belong elsewhere (inside the function?) and
I have to admit I don't understand it, maybe due to missing context.

> +
> +static void dram_init_fb(struct device *dev)
> +{
> +	u8 tmp;
> +	uint64_t proposed_base;
> +	unsigned int fbsize = (K8M890_FBSIZEMB * 1024 * 1024);
> +
> +	resmax = NULL;
> +	search_global_resources(
> +                IORESOURCE_MEM | IORESOURCE_CACHEABLE, IORESOURCE_MEM | IORESOURCE_CACHEABLE,
> +                get_memres, (void *) &fbsize);
> +
> +	/* no space for FB */
> +	if (!resmax)
> +		return;

Maybe spit out an error message here.

> +
> +	proposed_base = resmax->base + resmax->size - fbsize;
> +	resmax->size -= fbsize;
> +
> +	printk_debug("VIA FB proposed base: %llx\n", proposed_base);
> +
> +	/* enable UMA but no FB */
> +	pci_write_config8(dev, 0xa1, 0x80);
> +
> +	/* 27:21 goes to 7:1, 0 is enable CPU access */
> +	tmp = (proposed_base >> 20) | 0x1;
> +	pci_write_config8(dev, 0xa0, tmp);
> +
> +	/* 31:28 goes to 3:0 */
> +	tmp = ((proposed_base >> 28) & 0xf);
> +	tmp = ((log2(K8M890_FBSIZEMB) - 2) << 4);
> +	tmp |= 0x80;
> +	pci_write_config8(dev, 0xa1, tmp);
> +
> +	/* TODO K8 needs some UMA fine tuning too maybe call some generic routine here? */
> +}
> +
> +static const struct device_operations dram_ops_t = {
>  	.read_resources		= pci_dev_read_resources,
>  	.set_resources		= pci_dev_set_resources,
>  	.enable_resources	= pci_dev_enable_resources,
> @@ -70,8 +135,23 @@
>  	.ops_pci		= 0,
>  };
>  
> -static const struct pci_driver northbridge_driver __pci_driver = {
> -	.ops	= &dram_ops,
> +static const struct device_operations dram_ops_m = {
> +	.read_resources		= pci_dev_read_resources,
> +	.set_resources		= pci_dev_set_resources,
> +	.enable_resources	= pci_dev_enable_resources,
> +	.enable			= dram_enable,
> +	.init			= dram_init_fb,
> +	.ops_pci		= 0,
> +};
> +
> +static const struct pci_driver northbridge_driver_t __pci_driver = {
> +	.ops	= &dram_ops_t,
>  	.vendor	= PCI_VENDOR_ID_VIA,
>  	.device	= PCI_DEVICE_ID_VIA_K8T890CE_3,
>  };
> +
> +static const struct pci_driver northbridge_driver_m __pci_driver = {
> +	.ops	= &dram_ops_m,
> +	.vendor	= PCI_VENDOR_ID_VIA,
> +	.device	= PCI_DEVICE_ID_VIA_K8M890CE_3,
> +};
> Index: src/southbridge/via/k8t890/k8t890_ctrl.c
> ===================================================================
> --- src/southbridge/via/k8t890/k8t890_ctrl.c	(revision 3169)
> +++ src/southbridge/via/k8t890/k8t890_ctrl.c	(working copy)
> @@ -23,6 +23,73 @@
>  #include <device/pci_ids.h>
>  #include <console/console.h>
>  
> +/* Supports K8M890/KT890 and VT8237R PCI1/Vlink which setup is not in separate 
> + * device (0:11.7) but here

Could you try to rephrase the sentence?

> + */
> +
> +static void vt8237r_cfg(struct device *dev, struct device *devsb)
> +{
> +	u8 regm, regm2, regm3;
> +
> +	device_t devfun3;
> +
> +	/* Magic init for the VT8237R, it is new 0:11.7 device for newer VT8237A/S VT8251 Chips.

Same here.

> +           This is not well documented :/ */
> +
> +	devfun3 = dev_find_device(PCI_VENDOR_ID_VIA,
> +					   PCI_DEVICE_ID_VIA_K8T890CE_3, 0);
> +
> +		if (!devfun3)
> +			devfun3 = dev_find_device(PCI_VENDOR_ID_VIA,
> +					   PCI_DEVICE_ID_VIA_K8M890CE_3, 0);
> +
> +	pci_write_config8(dev, 0x70, 0xc2);
> +
> +	/* PCI Control */
> +	pci_write_config8(dev, 0x72, 0xee);
> +	pci_write_config8(dev, 0x73, 0x01);
> +	pci_write_config8(dev, 0x74, 0x24);
> +	pci_write_config8(dev, 0x75, 0x0f);
> +	pci_write_config8(dev, 0x76, 0x50);
> +	pci_write_config8(dev, 0x77, 0x08);
> +	pci_write_config8(dev, 0x78, 0x01);
> +	/* APIC on HT */
> +	pci_write_config8(dev, 0x7c, 0x7f);
> +	pci_write_config8(dev, 0x7f, 0x02);
> +
> +	/* WARNING: Need to copy some registers from NB (D0F3) to SB (D0F7). */
> +
> +	regm = pci_read_config8(devfun3, 0x88);	/* Shadow mem CTRL */
> +	pci_write_config8(dev, 0x57, regm);
> +
> +	regm = pci_read_config8(devfun3, 0x80);	/* Shadow page C */
> +	pci_write_config8(dev, 0x61, regm);
> +
> +	regm = pci_read_config8(devfun3, 0x81);	/* Shadow page D */
> +	pci_write_config8(dev, 0x62, regm);
> +
> +	regm = pci_read_config8(devfun3, 0x86);	/* SMM and APIC decoding */
> +	pci_write_config8(dev, 0xe6, regm);
> +
> +	regm3 = pci_read_config8(devfun3, 0x82);/* Shadow page E */
> +
> +	/*
> +	 * All access bits for 0xE0000-0xEFFFF encode as just 2 bits!
> +	 * So the NB reg is quite inconsistent, we expect there only 0xff or 0x00,
> +	 * and write them to 0x63 7-6 but! VIA 8237A has the mirror at 0x64!
> +	 */
> +	if (regm3 == 0xff)
> +		regm3 = 0xc0;
> +	else
> +		regm3 = 0x0;
> +
> +	/* Shadow page F + memhole copy */
> +	regm = pci_read_config8(devfun3, 0x83);
> +	pci_write_config8(dev, 0x63, regm3 | (regm & 0x3F));
> +}
> +
> +
> +
>  /**
>   * Setup the V-Link for VT8237R, 8X mode.
>   *
> Index: src/southbridge/via/k8t890/k8t890.h
> ===================================================================
> --- src/southbridge/via/k8t890/k8t890.h	(revision 3169)
> +++ src/southbridge/via/k8t890/k8t890.h	(working copy)
> @@ -32,4 +32,7 @@
>  #define K8T890_MMCONFIG_MBAR	0x61
>  #define K8T890_MULTIPLE_FN_EN	0x4f
>  
> +/* the FB size in MB min is 8MB max is 512MB */

Maybe a comma/parenthesis here?

/* FB size in MB (min is 8MB max is 512MB) */


> +#define K8M890_FBSIZEMB		64
> +
>  #endif


With the changes above, the patch is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/