Patchwork k8 uma and high tables

login
register
about
Submitter Stefan Reinauer
Date 2009-10-24 01:14:39
Message ID <4AE254FF.2060701@coresystems.de>
Download mbox | patch
Permalink /patch/472/
State Accepted
Commit r4835
Headers show

Comments

Stefan Reinauer - 2009-10-24 01:14:39
See patch. Carl Daniel sent something very similar before... This one is
the same but adds some preprocessing sugar to it so it does the right
thing both for UMA and non-UMA cases
Carl-Daniel Hailfinger - 2009-10-24 09:08:20
On 24.10.2009 03:14, Stefan Reinauer wrote:
> See patch. Carl Daniel sent something very similar before... This one is
> the same but adds some preprocessing sugar to it so it does the right
> thing both for UMA and non-UMA cases
>   

Yay! One less patch to carry around.
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

Regards,
Carl-Daniel
Myles Watson - 2009-10-24 13:07:42
>
> +#if CONFIG_GFXUMA == 1
> +               printk_debug("node %d : uma_memory_base/1024=0x%08x,
> mmio_basek=0x%08x, basek=0x%08x, limitk=0x%08x\n", i, uma_memory_base >> 10,
> mmio_basek, basek, limitk);
> +               if ((uma_memory_base >> 10) < mmio_basek)
> +                       printk_alert("node %d: UMA memory starts below
> mmio_basek\n", i);
>
Isn't this always true?  It seems like if uma_memory_base > (mmio_basek -
uma_memory_size) it's an error.

As a side note, I think that calculation should be done here, and I don't
think it should depend on how much RAM is installed, but should be user
configurable.

Then we can unify GFXUMA and VGA_MEM_MB -> UMA_MEM_MB.  Then uma_memory_base
can go away too, since it will be mmio_base - VGA_MEM_MB.


> +#else
>  //             printk_debug("node %d : mmio_basek=%08x, basek=%08x,
> limitk=%08x\n", i, mmio_basek, basek, limitk); //yhlu
> +#endif
>
>                /* See if I need to split the region to accomodate pci
> memory space */
>                if ( (basek < 4*1024*1024 ) && (limitk > mmio_basek) ) {
> @@ -1015,7 +1024,11 @@
>  #if CONFIG_WRITE_HIGH_TABLES==1
>                                        if (i==0 && high_tables_base==0) {
>                                        /* Leave some space for ACPI, PIRQ
> and MP tables */
> +#if CONFIG_GFXUMA == 1
> +                                               high_tables_base =
> ((uma_memory_base >> 10) - HIGH_TABLES_SIZE) *

1024;
>
high_tables_base = uma_memory_base - (HIGH_TABLES_SIZE * 1024);


> +#else
>                                                high_tables_base =
> (mmio_basek - HIGH_TABLES_SIZE) * 1024;
>


> @@ -1051,7 +1064,11 @@
>                             i, mmio_basek, basek, limitk);
>                if (i==0 && high_tables_base==0)
>                /* Leave some space for ACPI, PIRQ and MP tables */
> +#if CONFIG_GFXUMA == 1
> +                       high_tables_base = ((uma_memory_base >> 10) -
> HIGH_TABLES_SIZE) * 1024;
>
same thing.

Thanks,
Myles
Stefan Reinauer - 2009-10-24 13:10:05
Carl-Daniel Hailfinger wrote:
> On 24.10.2009 03:14, Stefan Reinauer wrote:
>   
>> See patch. Carl Daniel sent something very similar before... This one is
>> the same but adds some preprocessing sugar to it so it does the right
>> thing both for UMA and non-UMA cases
>>   
>>     
>
> Yay! One less patch to carry around.
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
>
> Regards,
> Carl-Daniel
>
>   
Thanks.. r4835
Stefan Reinauer - 2009-10-24 13:20:00
Myles Watson wrote:
>
>
>     +#if CONFIG_GFXUMA == 1
>     +               printk_debug("node %d :
>     uma_memory_base/1024=0x%08x, mmio_basek=0x%08x, basek=0x%08x,
>     limitk=0x%08x\n", i, uma_memory_base >> 10, mmio_basek, basek,
>     limitk);
>     +               if ((uma_memory_base >> 10) < mmio_basek)
>     +                       printk_alert("node %d: UMA memory starts
>     below mmio_basek\n", i);
>
> Isn't this always true?  It seems like if uma_memory_base >
> (mmio_basek - uma_memory_size) it's an error.
Can this happen? I have no idea,.. Carl-Daniel came up with the equation
and it worked great for me on the KT690

> As a side note, I think that calculation should be done here, and I
> don't think it should depend on how much RAM is installed, but should
> be user configurable.
I think it depends on the installed RAM because there is a hole below 4G
for PCI devices etc.


> Then we can unify GFXUMA and VGA_MEM_MB -> UMA_MEM_MB.  Then
> uma_memory_base can go away too, since it will be mmio_base - VGA_MEM_MB.
unless uma memory can live above 4G on some chipsets...?


>  
>
>     +#else
>      //             printk_debug("node %d : mmio_basek=%08x,
>     basek=%08x, limitk=%08x\n", i, mmio_basek, basek, limitk); //yhlu
>     +#endif
>
>                    /* See if I need to split the region to accomodate
>     pci memory space */
>                    if ( (basek < 4*1024*1024 ) && (limitk >
>     mmio_basek) ) {
>     @@ -1015,7 +1024,11 @@
>      #if CONFIG_WRITE_HIGH_TABLES==1
>                                            if (i==0 &&
>     high_tables_base==0) {
>                                            /* Leave some space for
>     ACPI, PIRQ and MP tables */
>     +#if CONFIG_GFXUMA == 1
>     +                                               high_tables_base =
>     ((uma_memory_base >> 10) - HIGH_TABLES_SIZE) * 
>
>     1024;
>
> high_tables_base = uma_memory_base - (HIGH_TABLES_SIZE * 1024);
d'uh, yes. Can you commit?

>  
>
>     +#else
>                                                    high_tables_base =
>     (mmio_basek - HIGH_TABLES_SIZE) * 1024;
>
>  
>
>     @@ -1051,7 +1064,11 @@
>                                 i, mmio_basek, basek, limitk);
>                    if (i==0 && high_tables_base==0)
>                    /* Leave some space for ACPI, PIRQ and MP tables */
>     +#if CONFIG_GFXUMA == 1
>     +                       high_tables_base = ((uma_memory_base >>
>     10) - HIGH_TABLES_SIZE) * 1024;
>
> same thing.

Thanks for catching this.
Myles Watson - 2009-10-24 13:51:09
> -----Original Message-----
> From: Stefan Reinauer [mailto:stepan@coresystems.de]
> Sent: Saturday, October 24, 2009 7:20 AM
> To: Myles Watson
> Cc: coreboot_mailing list
> Subject: Re: [coreboot] [PATCH] k8 uma and high tables
> 
> Myles Watson wrote:
> >
> >
> >     +#if CONFIG_GFXUMA == 1
> >     +               printk_debug("node %d :
> >     uma_memory_base/1024=0x%08x, mmio_basek=0x%08x, basek=0x%08x,
> >     limitk=0x%08x\n", i, uma_memory_base >> 10, mmio_basek, basek,
> >     limitk);
> >     +               if ((uma_memory_base >> 10) < mmio_basek)
> >     +                       printk_alert("node %d: UMA memory starts
> >     below mmio_basek\n", i);
> >
> > Isn't this always true?  It seems like if uma_memory_base >
> > (mmio_basek - uma_memory_size) it's an error.
> Can this happen? I have no idea,.. Carl-Daniel came up with the equation
> and it worked great for me on the KT690
So you don't see that message?  It seems like you'd have to have more than
4G of RAM and have UMA live above 4G for that to happen.

> 
> > As a side note, I think that calculation should be done here, and I
> > don't think it should depend on how much RAM is installed, but should
> > be user configurable.
> I think it depends on the installed RAM because there is a hole below 4G
> for PCI devices etc.
> 
> 
> > Then we can unify GFXUMA and VGA_MEM_MB -> UMA_MEM_MB.  Then
> > uma_memory_base can go away too, since it will be mmio_base -
> VGA_MEM_MB.
> unless uma memory can live above 4G on some chipsets...?
I don't think we have to worry about that with K8.

> >     +                                               high_tables_base =
> >     ((uma_memory_base >> 10) - HIGH_TABLES_SIZE) *
> >
> >     1024;
> >
> > high_tables_base = uma_memory_base - (HIGH_TABLES_SIZE * 1024);
> d'uh, yes. Can you commit?
Rev 4389.

Thanks,
Myles

Patch

Index: src/northbridge/amd/amdk8/northbridge.c
===================================================================
--- src/northbridge/amd/amdk8/northbridge.c	(revision 4831)
+++ src/northbridge/amd/amdk8/northbridge.c	(working copy)
@@ -837,7 +837,10 @@ 
 #if CONFIG_WRITE_HIGH_TABLES==1
 #define HIGH_TABLES_SIZE 64	// maximum size of high tables in KB
 extern uint64_t high_tables_base, high_tables_size;
+#if CONFIG_GFXUMA == 1
+extern uint64_t uma_memory_base, uma_memory_size;
 #endif
+#endif
 
 static void amdk8_domain_set_resources(device_t dev)
 {
@@ -1001,7 +1004,13 @@ 
 		}
 
 
+#if CONFIG_GFXUMA == 1
+		printk_debug("node %d : uma_memory_base/1024=0x%08x, mmio_basek=0x%08x, basek=0x%08x, limitk=0x%08x\n", i, uma_memory_base >> 10, mmio_basek, basek, limitk);
+		if ((uma_memory_base >> 10) < mmio_basek)
+			printk_alert("node %d: UMA memory starts below mmio_basek\n", i);
+#else
 //		printk_debug("node %d : mmio_basek=%08x, basek=%08x, limitk=%08x\n", i, mmio_basek, basek, limitk); //yhlu
+#endif
 
 		/* See if I need to split the region to accomodate pci memory space */
 		if ( (basek < 4*1024*1024 ) && (limitk > mmio_basek) ) {
@@ -1015,7 +1024,11 @@ 
 #if CONFIG_WRITE_HIGH_TABLES==1
 					if (i==0 && high_tables_base==0) {
 					/* Leave some space for ACPI, PIRQ and MP tables */
+#if CONFIG_GFXUMA == 1
+						high_tables_base = ((uma_memory_base >> 10) - HIGH_TABLES_SIZE) * 1024;
+#else
 						high_tables_base = (mmio_basek - HIGH_TABLES_SIZE) * 1024;
+#endif
 						high_tables_size = HIGH_TABLES_SIZE * 1024;
 						printk_debug(" split: %dK table at =%08llx\n", HIGH_TABLES_SIZE,
 							     high_tables_base);
@@ -1051,7 +1064,11 @@ 
 			     i, mmio_basek, basek, limitk);
 		if (i==0 && high_tables_base==0) {
 		/* Leave some space for ACPI, PIRQ and MP tables */
+#if CONFIG_GFXUMA == 1
+			high_tables_base = ((uma_memory_base >> 10) - HIGH_TABLES_SIZE) * 1024;
+#else
 			high_tables_base = (limitk - HIGH_TABLES_SIZE) * 1024;
+#endif
 			high_tables_size = HIGH_TABLES_SIZE * 1024;
 		}
 #endif