Re: [coreboot] [ patch] v3 Geode graphics init.

[Carl-Daniel Hailfinger]

On 21.02.2008 01:09, Marc Jones wrote:
> This patch assumes the vsa/idt patch is in place.

Comments inline.
> Removed CONFIG_VIDE0_MB in Kconfig and set the Geode
> graphics memory size through the Geode northbridge pci dts.
> This is a better way to handle a cpu/mainboard specific value.
>
> Signed-off-by: Marc Jones
> Index: coreboot-v3/northbridge/amd/geodelx/geodelx.c
>
> --- coreboot-v3.orig/northbridge/amd/geodelx/geodelx.c	2008-02-20 10:07:27.000000000 -0700
> +++ coreboot-v3/northbridge/amd/geodelx/geodelx.c	2008-02-20 12:12:07.000000000 -0700
> @@ -1,7 +1,7 @@
>  /*
> This file is part of the coreboot project.
> Copyright (C) 2007 Advanced Micro Devices, Inc.
> Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
> 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
>
> @@ -25,12 +25,14 @@
>  #include <device/pci_ids.h>
>  #include <msr.h>
>  #include <amd_geodelx.h>
> +#include <statictree.h>
> +#include "geodelink.h"
>
> /* Function prototypes */
>  extern void chipsetinit(void);
> -extern u32 get_systop(void);
>  extern void northbridge_init_early(void);
>
> +extern void graphics_init(struct northbridge_amd_geodelx_pci_config *nb_pci);
> +extern int sizeram(void);

Do we want sizeram() as a function on all subarches? If yes, we have
to either call it something like sizeram_4G() or we have to change the
return type to u64.
> /**
> Currently not set up.
>
> @@ -41,6 +43,37 @@
>  {
>  }
>
> +/**
> TODO.
> @return TODO.
> */
>
> +u32 get_systop(struct northbridge_amd_geodelx_pci_config *nb_pci)
> +{
> struct gliutable *gl = NULL;
> u32 systop;
> struct msr msr;
> int i;
> for (i = 0; gliu0table[i].desc_name != GL_END; i++) {
> if (gliu0table[i].desc_type == R_SYSMEM) {
> 	gl = &gliu0table[i];
> 	break;
> }
> }
> if (gl) {
> msr = rdmsr(gl->desc_name);
> systop = ((msr.hi & 0xFF) << 24) | ((msr.lo & 0xFFF00000) >> 8);
> systop += 4 * 1024;	/* 4K */
> } else {
> systop =
>     ((sizeram() - nb_pci->geode_video_mb) * 1024) - SMM_SIZE - 1024;
> }
> return systop;
>
> +}
>
> /**
> Initialize the northbridge PCI device.
> Right now this a no op. We leave it here as a hook for later use.
>
> @@ -116,6 +149,8 @@
>  {
>    int idx;
>    struct device *mc_dev;
>
> struct northbridge_amd_geodelx_pci_config *nb_pci =
> (struct northbridge_amd_geodelx_pci_config *)dev->device_configuration;
>
> printk(BIOS_SPEW, ">> Entering northbridge.c: %s\n", __FUNCTION__);
>
> @@ -127,7 +162,7 @@
>    	ram_resource(dev, idx++, 0, 640);
>    	/* 1 MB .. (Systop - 1 MB) (converted to KB) */

You subtract only 1 MB here.
> ram_resource(dev, idx++, 1024,
> 	     (get_systop() - (1 * 1024 * 1024)) / 1024);
> 	     (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
>
> }
>
> phase4_assign_resources(&dev->link[0]);
>
> @@ -147,6 +182,9 @@
>   */
>  static void geodelx_pci_domain_phase2(struct device *dev)
>  {
>
> struct northbridge_amd_geodelx_pci_config *nb_pci =
> (struct northbridge_amd_geodelx_pci_config *)dev->device_configuration;
>
> void do_vsmbios(void);
>
> printk(BIOS_SPEW, ">> Entering northbridge.c: %s\n", __FUNCTION__);
>
> @@ -161,8 +199,7 @@
>    printk(BIOS_SPEW, "After VSA:\n");
>    /* print_conf(); */
>
> -#warning graphics_init is disabled.
> /* graphics_init(); */
> graphics_init(nb_pci);
> pci_set_method(dev);
>
> }
> Index: coreboot-v3/northbridge/amd/geodelx/geodelxinit.c
>
> --- coreboot-v3.orig/northbridge/amd/geodelx/geodelxinit.c	2008-02-19 10:46:51.000000000 -0700
> +++ coreboot-v3/northbridge/amd/geodelx/geodelxinit.c	2008-02-20 10:59:05.000000000 -0700
> @@ -1,7 +1,7 @@
>  /*
> This file is part of the coreboot project.
> Copyright (C) 2007 Advanced Micro Devices, Inc.
> Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
> 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
>
> @@ -23,63 +23,8 @@
>  #include <msr.h>
>  #include <cpu.h>
>  #include <amd_geodelx.h>
> +#include "geodelink.h"
>
> -/* Function prototypes */
>
> -struct gliutable {
> unsigned long desc_name;
> unsigned short desc_type;
> unsigned long hi, lo;
>
> -};
>
> -static struct gliutable gliu0table[] = {
> /* 0-7FFFF to MC */
> {.desc_name = MSR_GLIU0_BASE1,.desc_type = BM,.hi = MSR_MC + 0x0,
> .lo = 0x0FFF80},
> /* 80000-9FFFF to MC */
> {.desc_name = MSR_GLIU0_BASE2,.desc_type = BM,.hi = MSR_MC + 0x0,
> .lo = (0x80 << 20) + 0x0FFFE0},
> /* C0000-FFFFF split to MC and PCI (sub decode) A0000-BFFFF
> * handled by SoftVideo.
> */
> {.desc_name = MSR_GLIU0_SHADOW,.desc_type = SC_SHADOW,
> .hi = MSR_MC + 0x0,.lo = 0x03},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU0_SYSMEM,.desc_type = R_SYSMEM,
> .hi = MSR_MC,.lo = 0x0},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU0_SMM,.desc_type = BMO_SMM,
> .hi = MSR_MC,.lo = 0x0},
> {.desc_name = GLIU0_GLD_MSR_COH,.desc_type = OTHER,
> .hi = 0x0,.lo = GL0_CPU},
> {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
>
> -};
>
> -static struct gliutable gliu1table[] = {
> /* 0-7FFFF to MC */
> {.desc_name = MSR_GLIU1_BASE1,.desc_type = BM,.hi = MSR_GL0 + 0x0,
> .lo = 0x0FFF80},
> /* 80000-9FFFF to MC */
> {.desc_name = MSR_GLIU1_BASE2,.desc_type = BM,.hi = MSR_GL0 + 0x0,
> .lo = (0x80 << 20) + 0x0FFFE0},
> /* C0000-Fffff split to MC and PCI (sub decode) */
> {.desc_name = MSR_GLIU1_SHADOW,.desc_type = SC_SHADOW,
> .hi = MSR_GL0 + 0x0,.lo = 0x03},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU1_SYSMEM,.desc_type = R_SYSMEM,
> .hi = MSR_GL0,.lo = 0x0},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU1_SMM,.desc_type = BM_SMM,
> .hi = MSR_GL0,.lo = 0x0},
> {.desc_name = GLIU1_GLD_MSR_COH,.desc_type = OTHER,
> .hi = 0x0,.lo = GL1_GLIU0},
> /* FooGlue FPU 0xF0 */
> {.desc_name = MSR_GLIU1_FPU_TRAP,.desc_type = SCIO,
> .hi = (GL1_GLCP << 29) + 0x0,.lo = 0x033000F0},
> {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
>
> -};
>
> -static struct gliutable *gliutables[] = { gliu0table, gliu1table, 0 };
>
> static struct msrinit clock_gating_default[] = {
>    {GLIU0_GLD_MSR_PM,	{.hi = 0x00,.lo = 0x0005}},
> @@ -763,10 +708,6 @@
>    printk(BIOS_DEBUG, "L2 cache enabled\n");
>  }
>
> -#ifndef CONFIG_VIDEO_MB
> -#warning "CONFIG_VIDEO_MB was not defined"
> -#define CONFIG_VIDEO_MB 8
> -#endif
>
> /**
> Set up all LX cache registers, L1, L2, and x86.
>
> @@ -790,36 +731,6 @@
>  }
>
> /**
> TODO.
> @return TODO.
> */
>
> -u32 get_systop(void)
> -{
> struct gliutable *gl = NULL;
> u32 systop;
> struct msr msr;
> int i;
> for (i = 0; gliu0table[i].desc_name != GL_END; i++) {
> if (gliu0table[i].desc_type == R_SYSMEM) {
> 	gl = &gliu0table[i];
> 	break;
> }
> }
> if (gl) {
> msr = rdmsr(gl->desc_name);
> systop = ((msr.hi & 0xFF) << 24) | ((msr.lo & 0xFFF00000) >> 8);
> systop += 4 * 1024;	/* 4K */
> } else {
> systop =
>     ((sizeram() - CONFIG_VIDEO_MB) * 1024) - SMM_SIZE - 1024;
> }
> return systop;
>
> -}
>
> -/**
> Do all the Nasty Bits that have to happen.
> These can be done once memory is up, but before much else is done.
> Index: coreboot-v3/northbridge/amd/geodelx/grphinit.c
>
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v3/northbridge/amd/geodelx/grphinit.c	2008-02-20 10:59:05.000000000 -0700
> @@ -0,0 +1,62 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2007-2008 Advanced Micro Devices, Inc.
> 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 <io.h>
> +#include <amd_geodelx.h>
> +#include <console.h>
> +#include <statictree.h>
> /*
> This function mirrors the Graphics_Init routine in GeodeROM.
> */
>
> +void graphics_init(struct northbridge_amd_geodelx_pci_config *nb_pci)
> +{
> u16 wClassIndex, wData, res;
> /* SoftVG initialization */

That's scary. Do we really need VSA support even if there is a native
geodelx framebuffer/X driver?
> printk(BIOS_DEBUG, "Graphics init...\n");
> /* Call SoftVG with the main configuration parameters. */
> /* NOTE: SoftVG expects the memory size to be given in 2MB blocks */
> wClassIndex = (VRC_VG << 8) + VG_CONFIG;
> /*
> * Graphics Driver Enabled (13)			0, NO (lets BIOS controls the GP)
> * External Monochrome Card Support(12)		0, NO
> * Controller Priority Select(11)		1, Primary
> * Display Select(10:8)				0x0, CRT
> * Graphics Memory Size(7:1)			CONFIG_VIDEO_MB >> 1,
> *	defined in mainboard/../dts
> * PLL Reference Clock Bypass(0)		0, Default
> */
> /* Video RAM has to be given in 2MB chunks
> *   the value is read @ 7:1 (value in 7:0 looks like /2)

Typo?

+ * the value is read @ 7:1 (value in 7:0 looks like *2)

Besides that, being limited to multiples of 2MB for VRAM seems not to mix
well with subtracting only 1MB from systop.
> *   so we can add the real value in megabytes.
> */
> wData = VG_CFG_DRIVER | VG_CFG_PRIORITY |
> 	VG_CFG_DSCRT | (nb_pci->geode_video_mb & VG_MEM_MASK);
> vr_write(wClassIndex, wData);
> res = vr_read(wClassIndex);
> printk(BIOS_DEBUG, "VRC_VG value: 0x%04x\n", res);
>
> +}
> Index: coreboot-v3/northbridge/amd/geodelx/geodelink.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v3/northbridge/amd/geodelx/geodelink.h	2008-02-20 10:59:05.000000000 -0700
> @@ -0,0 +1,78 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 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 GEODELINK_H
> +#define GEODELINK_H
>
> +struct gliutable {
> unsigned long desc_name;
> unsigned short desc_type;
> unsigned long hi, lo;
>
> +};
>
> +static struct gliutable gliu0table[] = {
> /* 0-7FFFF to MC */
> {.desc_name = MSR_GLIU0_BASE1,.desc_type = BM,.hi = MSR_MC + 0x0,
> .lo = 0x0FFF80},
> /* 80000-9FFFF to MC */
> {.desc_name = MSR_GLIU0_BASE2,.desc_type = BM,.hi = MSR_MC + 0x0,
> .lo = (0x80 << 20) + 0x0FFFE0},
> /* C0000-FFFFF split to MC and PCI (sub decode) A0000-BFFFF
> * handled by SoftVideo.
> */
> {.desc_name = MSR_GLIU0_SHADOW,.desc_type = SC_SHADOW,
> .hi = MSR_MC + 0x0,.lo = 0x03},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU0_SYSMEM,.desc_type = R_SYSMEM,
> .hi = MSR_MC,.lo = 0x0},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU0_SMM,.desc_type = BMO_SMM,
> .hi = MSR_MC,.lo = 0x0},
> {.desc_name = GLIU0_GLD_MSR_COH,.desc_type = OTHER,
> .hi = 0x0,.lo = GL0_CPU},
> {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
>
> +};
>
> +static struct gliutable gliu1table[] = {
> /* 0-7FFFF to MC */
> {.desc_name = MSR_GLIU1_BASE1,.desc_type = BM,.hi = MSR_GL0 + 0x0,
> .lo = 0x0FFF80},
> /* 80000-9FFFF to MC */
> {.desc_name = MSR_GLIU1_BASE2,.desc_type = BM,.hi = MSR_GL0 + 0x0,
> .lo = (0x80 << 20) + 0x0FFFE0},
> /* C0000-Fffff split to MC and PCI (sub decode) */
> {.desc_name = MSR_GLIU1_SHADOW,.desc_type = SC_SHADOW,
> .hi = MSR_GL0 + 0x0,.lo = 0x03},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU1_SYSMEM,.desc_type = R_SYSMEM,
> .hi = MSR_GL0,.lo = 0x0},
> /* Catch and fix dynamically. */
> {.desc_name = MSR_GLIU1_SMM,.desc_type = BM_SMM,
> .hi = MSR_GL0,.lo = 0x0},
> {.desc_name = GLIU1_GLD_MSR_COH,.desc_type = OTHER,
> .hi = 0x0,.lo = GL1_GLIU0},
> /* FooGlue FPU 0xF0 */
> {.desc_name = MSR_GLIU1_FPU_TRAP,.desc_type = SCIO,
> .hi = (GL1_GLCP << 29) + 0x0,.lo = 0x033000F0},
> {.desc_name = GL_END,.desc_type = GL_END,.hi = 0x0,.lo = 0x0},
>
> +};

It's debatable whether gliu0table and gliu1table should really be in a
header file as opposed to a normal .c code file. This max cause additional
warnings from gcc.
If any of the tables are constant, please mark them as const.
> +static struct gliutable *gliutables[] = { gliu0table, gliu1table, 0 };
>
> +#endif /* GEODELINK_H */
> Index: coreboot-v3/include/arch/x86/amd_geodelx.h
> ===================================================================
> --- coreboot-v3.orig/include/arch/x86/amd_geodelx.h	2008-02-19 10:46:52.000000000 -0700
> +++ coreboot-v3/include/arch/x86/amd_geodelx.h	2008-02-20 10:59:05.000000000 -0700
> @@ -1254,7 +1254,7 @@
> @param class_index The register index
> @return the 16-bit word of data
>
> */
> -static inline u16 vr_ead(u16 class_index)
> +static inline u16 vr_read(u16 class_index)
>  {
>    u16 data;
>    outl(((u32) VR_UNLOCK << 16) | class_index, VRC_INDEX);
> Index: coreboot-v3/mainboard/pcengines/alix1c/dts
> ===================================================================
> --- coreboot-v3.orig/mainboard/pcengines/alix1c/dts	2008-02-19 10:46:52.000000000 -0700
> +++ coreboot-v3/mainboard/pcengines/alix1c/dts	2008-02-20 10:59:05.000000000 -0700
> @@ -29,6 +29,8 @@
>    	/config/("northbridge/amd/geodelx/domain");
>    	pci@1,0 {
>    		/config/("northbridge/amd/geodelx/pci");
> 	/* Video RAM has to be in 2MB chunks. */
> 	geode_video_mb = "8";
> };
> pci@15,0 {
> 	/config/("southbridge/amd/cs5536/dts");
> Index: coreboot-v3/northbridge/amd/geodelx/pci
>
> --- coreboot-v3.orig/northbridge/amd/geodelx/pci	2008-02-19 10:46:52.000000000 -0700
> +++ coreboot-v3/northbridge/amd/geodelx/pci	2008-02-20 10:59:05.000000000 -0700
> @@ -2,6 +2,7 @@
> This file is part of the coreboot project.
> Copyright (C) 2008 Ronald G. Minnich
> Copyright (C) 2008 Advanced Micro Devices, Inc.
> 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
>
> @@ -20,5 +21,8 @@
>
> {
>    device_operations = "geodelx_north_pci";
> /* Video RAM has to be in 2MB chunks. */
> geode_video_mb = "0";
>
> };
> Index: coreboot-v3/northbridge/amd/geodelx/Makefile
>
> --- coreboot-v3.orig/northbridge/amd/geodelx/Makefile	2008-02-20 11:34:34.000000000 -0700
> +++ coreboot-v3/northbridge/amd/geodelx/Makefile	2008-02-20 11:35:28.000000000 -0700
> @@ -23,6 +23,7 @@
>
> STAGE2_CHIPSET_OBJ += $(obj)/northbridge/amd/geodelx/geodelx.o \
>    	      $(obj)/northbridge/amd/geodelx/vsmsetup.o \
>       $(obj)/util/x86emu/vm86_gdt.o
>       $(obj)/util/x86emu/vm86_gdt.o \
>       $(obj)/northbridge/amd/geodelx/grphinit.o
>
> endif

With my comments addressed, the patch is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel