[ patch] v3 Geode graphics init.

[Marc Jones]

This patch assumes the vsa/idt patch is in place.

Marc

[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

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> 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
> > +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.

I don't know if any other subarchs will implement this but I like the 
idea. I will make it u64.

...
> > @@ -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);
> > }

As the comment say. 0-640K is claimed prior to the 1MB to systop.
> > /* SoftVG initialization */
>
> That's scary. Do we really need VSA support even if there is a native
> geodelx framebuffer/X driver?

Not scary. Geode needs VSA for the PCI headers. VSA needs to know how 
much memory is claimed to report it in the BAR.

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

Maybe that needs a better explanation. Bit 0 is used but video_mb is not 
shifted. Or you could look at it as video_mb/2 << 1.

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

These need to be marked const. I will think about if this is the right 
way to do this.

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

Thanks, I'll work on these things.
Marc

[Carl-Daniel Hailfinger]

On 21.02.2008 02:54, Marc Jones wrote:
> Carl-Daniel Hailfinger wrote:
> > 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
> > > +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.
>
> I don't know if any other subarchs will implement this but I like the 
> idea. I will make it u64.

Great. Please do note that I forgot to mention the somewhat grim reality 
of 64bit machines with more than 4 GB of RAM where we have two different 
notions of top of RAM: The address where the 32bit hardware decoded 
space begins and the real top of RAM, which is not equal to the size of 
installed RAM due to the hole below 4 GB.
> ...
> > > @@ -127,7 +162,7 @@
> > >          ram_resource(dev, idx++, 0, 640);
> > >          /* 1 MB .. (Systop - 1 MB) (converted to KB) */
> >
> > You subtract only 1 MB here.

My comment above referred to your comment above in conjunction with your 
code below.
> > >      ram_resource(dev, idx++, 1024,
> > >             (get_systop() - (1 * 1024 * 1024)) / 1024);
> > >             (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
> > > }
>
> As the comment say. 0-640K is claimed prior to the 1MB to systop.

Sorry, I still don't understand although I'm really trying hard. Maybe 
my comment was misplaced and thus misunderstood.
The first ram_resource claims 0 - 640k.
The second ram_resource claims 1M - (systop-1M). Where's that hardcoded 
missing 1 MB at the top of RAM going?
> > > /* SoftVG initialization */
> >
> > That's scary. Do we really need VSA support even if there is a native
> > geodelx framebuffer/X driver?
>
> Not scary. Geode needs VSA for the PCI headers. VSA needs to know how 
> much memory is claimed to report it in the BAR.

Thanks for the info.
> ...
> > > /*
> > > * 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.
>
> Maybe that needs a better explanation. Bit 0 is used but video_mb is 
> not shifted. Or you could look at it as video_mb/2 << 1.

Suggestion:
Value in 7:0 looks like (video_mb & ~1)
> ...
> > > +#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.
>
> These need to be marked const. I will think about if this is the right 
> way to do this.

Thanks.

One more complaint (probably a genuine bug):
> +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 =

systop is the address of top of usable RAM in Bytes.
>     ((sizeram() - nb_pci->geode_video_mb) * 1024) - SMM_SIZE - 1024;

sizeram() and geode_video_mb are in MBytes, yet they only get multiplied by 1024.
A factor of 1024 is missing here.
> }
> return systop;
>
> +}

Regards,
Carl-Daniel

[ron minnich]

I have committed the patch on which this patch depends, I am not going
to test this yet as there seems to be a little more work needed. But I
am ready when you all are.

Thanks again

ron

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> On 21.02.2008 02:54, Marc Jones wrote:
> > Carl-Daniel Hailfinger wrote:
> > > 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
> > > > +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.
> >
> > I don't know if any other subarchs will implement this but I like the 
> > idea. I will make it u64.
>
> Great. Please do note that I forgot to mention the somewhat grim 
> reality of 64bit machines with more than 4 GB of RAM where we have two 
> different notions of top of RAM: The address where the 32bit hardware 
> decoded space begins and the real top of RAM, which is not equal to 
> the size of installed RAM due to the hole below 4 GB.

Right, this will give you the size of RAM in this case for Geode. Other 
architectures would have to do the right thing.
> > ...
> > > > @@ -127,7 +162,7 @@
> > > >          ram_resource(dev, idx++, 0, 640);
> > > >          /* 1 MB .. (Systop - 1 MB) (converted to KB) */
> > >
> > > You subtract only 1 MB here.
>
> My comment above referred to your comment above in conjunction with 
> your code below.
> > > >      ram_resource(dev, idx++, 1024,
> > > >             (get_systop() - (1 * 1024 * 1024)) / 1024);
> > > >             (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
> > > > }
> >
> > As the comment say. 0-640K is claimed prior to the 1MB to systop.
>
> Sorry, I still don't understand although I'm really trying hard. Maybe 
> my comment was misplaced and thus misunderstood.
> The first ram_resource claims 0 - 640k.
> The second ram_resource claims 1M - (systop-1M). Where's that 
> hardcoded missing 1 MB at the top of RAM going?

systop-1M is the size.
static void ram_resource(device_t dev, unsigned long index,
             unsigned long basek, unsigned long sizek)
> > > > /* SoftVG initialization */
> > >
> > > That's scary. Do we really need VSA support even if there is a native
> > > geodelx framebuffer/X driver?
> >
> > Not scary. Geode needs VSA for the PCI headers. VSA needs to know how 
> > much memory is claimed to report it in the BAR.
>
> Thanks for the info.
> > ...
> > > > /*
> > > > * 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.
> >
> > Maybe that needs a better explanation. Bit 0 is used but video_mb is 
> > not shifted. Or you could look at it as video_mb/2 << 1.
>
> Suggestion:
> Value in 7:0 looks like (video_mb & ~1)

ok.
> > ...
> > > > +#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.
> >
> > These need to be marked const. I will think about if this is the 
> > right way to do this.
>
> Thanks.
>
> One more complaint (probably a genuine bug):
> > +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 =
>
> systop is the address of top of usable RAM in Bytes.
> >        ((sizeram() - nb_pci->geode_video_mb) * 1024) - SMM_SIZE 
> > 1024;
>
> sizeram() and geode_video_mb are in MBytes, yet they only get 
> multiplied by 1024.
> A factor of 1024 is missing here.

Real bug. Fortunately we never take that path. I will fix it in this 
patch for v3 and submit a patch for v2.

Thanks,
Marc

[Marc Jones]

Marc Jones wrote:
> Carl-Daniel Hailfinger wrote:
> > On 21.02.2008 02:54, Marc Jones wrote:
> > > Carl-Daniel Hailfinger wrote:
> > > > On 21.02.2008 01:09, Marc Jones wrote:
> > > > > This patch assumes the vsa/idt patch is in place.

...
> > > > > @@ -127,7 +162,7 @@
> > > > >          ram_resource(dev, idx++, 0, 640);
> > > > >          /* 1 MB .. (Systop - 1 MB) (converted to KB) */
> > > >
> > > > You subtract only 1 MB here.
> >
> > My comment above referred to your comment above in conjunction with 
> > your code below.
> > > > >      ram_resource(dev, idx++, 1024,
> > > > >             (get_systop() - (1 * 1024 * 1024)) / 1024);
> > > > >             (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
> > > > > }
> > >
> > > As the comment say. 0-640K is claimed prior to the 1MB to systop.
> >
> > Sorry, I still don't understand although I'm really trying hard. Maybe 
> > my comment was misplaced and thus misunderstood.
> > The first ram_resource claims 0 - 640k.
> > The second ram_resource claims 1M - (systop-1M). Where's that 
> > hardcoded missing 1 MB at the top of RAM going?
>
> systop-1M is the size.
> static void ram_resource(device_t dev, unsigned long index,
>              unsigned long basek, unsigned long sizek)

There is a bug here. get_systop returns KB. It is already in KB so it 
doesn't need the /1024 and the 1MB doesn't need * 1024.
 The correct line is :

ram_resource(dev, idx++, 1024,
                             (get_systop(nb_pci) - 1024);

I will check v2.
> > > +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 =
> >
> > systop is the address of top of usable RAM in Bytes.
> > >        ((sizeram() - nb_pci->geode_video_mb) * 1024) - SMM_SIZE 
> > > 1024;
> >
> > sizeram() and geode_video_mb are in MBytes, yet they only get 
> > multiplied by 1024.
> > A factor of 1024 is missing here.
>
> Real bug. Fortunately we never take that path. I will fix it in this 
> patch for v3 and submit a patch for v2.

The bug here isn't MB->KB. get_systop returns KB. The bug is the 
additional 1024 taken off the end.

Marc

[Carl-Daniel Hailfinger]

On 21.02.2008 23:06, Marc Jones wrote:
> Marc Jones wrote:
> > Carl-Daniel Hailfinger wrote:
> > > On 21.02.2008 02:54, Marc Jones wrote:
> > > > Carl-Daniel Hailfinger wrote:
> > > > > On 21.02.2008 01:09, Marc Jones wrote:
> > > > > >      ram_resource(dev, idx++, 1024,
> > > > > >             (get_systop() - (1 * 1024 * 1024)) / 1024);
> > > > > >             (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
> > > > > > }
> > > >
> > > > As the comment say. 0-640K is claimed prior to the 1MB to systop.
> > >
> > > Sorry, I still don't understand although I'm really trying hard. Maybe 
> > > my comment was misplaced and thus misunderstood.
> > > The first ram_resource claims 0 - 640k.
> > > The second ram_resource claims 1M - (systop-1M). Where's that 
> > > hardcoded missing 1 MB at the top of RAM going?
> >
> > systop-1M is the size.
> > static void ram_resource(device_t dev, unsigned long index,
> >              unsigned long basek, unsigned long sizek)
>
> There is a bug here. get_systop returns KB. It is already in KB so it 
> doesn't need the /1024 and the 1MB doesn't need * 1024.
>  The correct line is :
>
> ram_resource(dev, idx++, 1024,
>                              (get_systop(nb_pci) - 1024);
>
> I will check v2.

Thanks.
> > > > +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 */

If get_systop really returns kBytes, the comment above (/* 4K */) does 
not match the code which adds 4*1024 K = 4 M.
> > > > } else {
> > > >    systop =
> > >
> > > systop is the address of top of usable RAM in Bytes.
> > > >        ((sizeram() - nb_pci->geode_video_mb) * 1024) - SMM_SIZE 
> > > > 1024;
> > >
> > > sizeram() and geode_video_mb are in MBytes, yet they only get 
> > > multiplied by 1024.
> > > A factor of 1024 is missing here.
> >
> > Real bug. Fortunately we never take that path. I will fix it in this 
> > patch for v3 and submit a patch for v2.
>
> The bug here isn't MB->KB. get_systop returns KB. The bug is the 
> additional 1024 taken off the end.

And the new bug I discovered above.

Regards,
Carl-Daniel

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> On 21.02.2008 23:06, Marc Jones wrote:
> > Marc Jones wrote:
> > > Carl-Daniel Hailfinger wrote:
> > > > On 21.02.2008 02:54, Marc Jones wrote:
> > > > > Carl-Daniel Hailfinger wrote:
> > > > > > On 21.02.2008 01:09, Marc Jones wrote:
> > > > > >
> > > > > > >          ram_resource(dev, idx++, 1024,
> > > > > > > -                 (get_systop() - (1 * 1024 * 1024)) / 1024);
> > > > > > > +                 (get_systop(nb_pci) - (1 * 1024 * 1024)) / 1024);
> > > > > > >      }
> > > > > > >
> > > > >
> > > > > As the comment say. 0-640K is claimed prior to the 1MB to systop.
> > > >
> > > > Sorry, I still don't understand although I'm really trying hard. 
> > > > Maybe my comment was misplaced and thus misunderstood.
> > > > The first ram_resource claims 0 - 640k.
> > > > The second ram_resource claims 1M - (systop-1M). Where's that 
> > > > hardcoded missing 1 MB at the top of RAM going?
> > >
> > > systop-1M is the size.
> > > static void ram_resource(device_t dev, unsigned long index,
> > >              unsigned long basek, unsigned long sizek)
> >
> > There is a bug here. get_systop returns KB. It is already in KB so it 
> > doesn't need the /1024 and the 1MB doesn't need * 1024.
> >  The correct line is :
> >    ram_resource(dev, idx++, 1024,
> >                              (get_systop(nb_pci) - 1024);
> >
> > I will check v2.
>
> Thanks.
> > > > > +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 */
>
> If get_systop really returns kBytes, the comment above (/* 4K */) does 
> not match the code which adds 4*1024 K = 4 M.

Argh! This is all messed up. Something happened from v2 to v3. 
get_systop should return bytes. Not KB. I'll get this sorted out.

Marc

[Marc Jones]

I think that this addresses all the issues. Ron can you boot this. I'm 
remote today.

Marc

[ron minnich]

Bad news. It is hanging (locking up) in vsm. I interpret this to mean
it maybe took an interrupt? Post code is 0x10.

output is attached.

ron

[ron minnich]

On Thu, Feb 21, 2008 at 5:18 PM, ron minnich  wrote:
> Bad news. It is hanging (locking up) in vsm. I interpret this to mean
>  it maybe took an interrupt? Post code is 0x10.
>
> output is attached.

Just to double-test I backed it out and rebuilt, and everything works again.

Gee, this is gonna be fun :-)

ron

[ron minnich]

On Thu, Feb 21, 2008 at 5:23 PM, ron minnich  wrote:
> On Thu, Feb 21, 2008 at 5:18 PM, ron minnich  wrote:
> > Bad news. It is hanging (locking up) in vsm. I interpret this to mean
> >  it maybe took an interrupt? Post code is 0x10.
> >
> > output is attached.
>
> Just to double-test I backed it out and rebuilt, and everything works again.
>
> Gee, this is gonna be fun :-)

I spoke to soon. OK, something has introduced another regression.

Sigh.

Here is the pattern.

You flash. Linux starts to boot. It thinks it finds an ACPI table. It
coughs chunks and dies.

The next time you boot, it hangs in vsm. Oh, yes, and power cycling
does no help.

I just tried leaving it off for one minute and ... it hangs in vsm.

!@#$#@!$#@.

I'm a little concerned that we broke the lar zeroing code again. that
would explain the bogus ACPI bits.

we're breaking things too often. I think we may need to stop
committing until we get a test. So we may need a signoff, "Tested-by",
and no commits until that is seen.

ron

[Marc Jones]

ron minnich wrote:
> On Thu, Feb 21, 2008 at 5:23 PM, ron minnich  wrote:
> > On Thu, Feb 21, 2008 at 5:18 PM, ron minnich  wrote:
> > > Bad news. It is hanging (locking up) in vsm. I interpret this to mean
> > >  it maybe took an interrupt? Post code is 0x10.
> > >
> > > output is attached.
> >
> > Just to double-test I backed it out and rebuilt, and everything works again.
> >
> > Gee, this is gonna be fun :-)
>
> I spoke to soon. OK, something has introduced another regression.
>
> Sigh.
>
> Here is the pattern.
>
> You flash. Linux starts to boot. It thinks it finds an ACPI table. It
> coughs chunks and dies.
>
> The next time you boot, it hangs in vsm. Oh, yes, and power cycling
> does no help.
>
> I just tried leaving it off for one minute and ... it hangs in vsm.
>
> !@#$#@!$#@.
>
> I'm a little concerned that we broke the lar zeroing code again. that
> would explain the bogus ACPI bits.
>
> we're breaking things too often. I think we may need to stop
> committing until we get a test. So we may need a signoff, "Tested-by",
> and no commits until that is seen.
>
> ron

I can look at this in the morning.
Marc

[ron minnich]

I must have grabbed an older vsa somehow. I am running 612 and it's
calling bios interrupts. I thought it had stopped. weird.

So, I will try to get the new one and try again. Did you update vsa in
the blob repo?

ron

[Carl-Daniel Hailfinger]

On 22.02.2008 02:42, ron minnich wrote:
> I must have grabbed an older vsa somehow. I am running 612 and it's
> calling bios interrupts. I thought it had stopped. weird.
>
> So, I will try to get the new one and try again. Did you update vsa in
> the blob repo?

The one at 
http://www.amd.com/files/connectivitysolutions/geode/geode_lx/amd_vsa_lx_1.0... 
should be OK. Depending on which browser or download program you use, 
the file may already have been gunzipped and you just have to rename it. 
(That's a bug of the AMD web server and I promised to write something up 
to help their webmaster fixing it.)

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 22.02.2008 02:30, ron minnich wrote:
> On Thu, Feb 21, 2008 at 5:23 PM, ron minnich  wrote:
> > On Thu, Feb 21, 2008 at 5:18 PM, ron minnich  wrote:
> > > Bad news. It is hanging (locking up) in vsm. I interpret this to mean
> > >  it maybe took an interrupt? Post code is 0x10.
> > >
> > > output is attached.
> >
> > Just to double-test I backed it out and rebuilt, and everything works again.
> >
> > Gee, this is gonna be fun :-)
>
> I spoke to soon. OK, something has introduced another regression.
>
> Sigh.
>
> Here is the pattern.
>
> You flash. Linux starts to boot. It thinks it finds an ACPI table. It
> coughs chunks and dies.
>
> The next time you boot, it hangs in vsm. Oh, yes, and power cycling
> does no help.
>
> I just tried leaving it off for one minute and ... it hangs in vsm.
>
> !@#$#@!$#@.

Can you try to find out if r613 is already broken? You tested that 
revision and it worked fine. I'm not trying to point blame, I just want 
to figure out how this could slip through reviews and tests.
> I'm a little concerned that we broke the lar zeroing code again. that
> would explain the bogus ACPI bits.
>
> we're breaking things too often. I think we may need to stop
> committing until we get a test. So we may need a signoff, "Tested-by",
> and no commits until that is seen.

Indeed. Stefan has a test rig. Maybe he could make that accessible to 
developers with commit rights to test new images.

Regards,
Carl-Daniel

[ron minnich]

OK, I think I got it, somehow I ended up with the old VSA in there.
Why'd it work last night? Don't know.

OK, it's ok. Whew.

I am calling it good and committing. Thanks Marc, sorry for the panic
button. It was too much like last week for a minute.

Committed revision 616.

ron

[ron minnich]

so how do I get the amd frame buffer to light up now ... anybody got a
.config for me? My attempt just gives me akernel that locks up

ron

[Carl-Daniel Hailfinger]

On 22.02.2008 03:00, ron minnich wrote:
> so how do I get the amd frame buffer to light up now ... anybody got a
> .config for me? My attempt just gives me akernel that locks up

AFAIK you explicitly have to compile out any legacy VGA console stuff. 
The .config OLPC is using might be very close to the desired 
configuration. I have no idea about any patches you might need to grab 
from their tree, though.

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> Indeed. Stefan has a test rig. Maybe he could make that accessible to 
> developers with commit rights to test new images.

It is available to all developers.

See http://www.coreboot.org/Distributed_and_Automated_Testsystem

[Carl-Daniel Hailfinger]

On 22.02.2008 13:06, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > Indeed. Stefan has a test rig. Maybe he could make that accessible to 
> > developers with commit rights to test new images.
>
> It is available to all developers.
>
> See http://www.coreboot.org/Distributed_and_Automated_Testsystem

Now we just need a alix1c and a dbe62 connected to the test system.

Regards,
Carl-Daniel

[Stefan Reinauer]

ron minnich wrote:
> I'm a little concerned that we broke the lar zeroing code again. that
> would explain the bogus ACPI bits.
>
> we're breaking things too often. I think we may need to stop
> committing until we get a test. So we may need a signoff, "Tested-by",
> and no commits until that is seen.

We don't even need to do manual testing. We have a fully automated test 
system for that:

http://www.coreboot.org/Distributed_and_Automated_Testsystem

v3 is not supported yet, but it should be simpler than v2 was.

You can browse the code here: http://coreboot.org/viewvc/?root=Testsystem

All we need is someone with the hardware, possibly an X10 power switch, 
a bios savior and another machine with connection to the internet to 
fetch and execute the tests. More information on the requirements are in 
the papers on above site.

[Myles Watson]

> we're breaking things too often. I think we may need to stop
> committing until we get a test. So we may need a signoff, "Tested-by",
> and no commits until that is seen.

Even though I test on Qemu, it's no substitute for code review.  Testing
generally only tests one path through the code, and for me only one board (I
don't have any others supported by v3.)

Myles

[Carl-Daniel Hailfinger]

On 21.02.2008 17:29, Marc Jones wrote:
> Carl-Daniel Hailfinger wrote:
> > On 21.02.2008 02:54, Marc Jones wrote:
> > > Carl-Daniel Hailfinger wrote:
> > > > As the comment say. 0-640K is claimed prior to the 1MB to systop.
> > >
> > > Sorry, I still don't understand although I'm really trying hard. 
> > > Maybe my comment was misplaced and thus misunderstood.
> > > The first ram_resource claims 0 - 640k.
> > > The second ram_resource claims 1M - (systop-1M). Where's that 
> > > hardcoded missing 1 MB at the top of RAM going?
>
> systop-1M is the size.
> static void ram_resource(device_t dev, unsigned long index,
>             unsigned long basek, unsigned long sizek)

OK, thanks.
> > One more complaint (probably a genuine bug):
> > > +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 =
> >
> > systop is the address of top of usable RAM in Bytes.
> > >        ((sizeram() - nb_pci->geode_video_mb) * 1024) - 
> > >
> > > SMM_SIZE - 1024;
> >
> > sizeram() and geode_video_mb are in MBytes, yet they only get 
> > multiplied by 1024.
> > A factor of 1024 is missing here.
>
> Real bug. Fortunately we never take that path. I will fix it in this 
> patch for v3 and submit a patch for v2.

Thanks for fixing.

Regards,
Carl-Daniel