Patchwork CONFIG_LB_MEM_TOPK -> CONFIG_RAMTOP

login
register
about
Submitter Myles Watson
Date 2009-10-16 15:59:45
Message ID <2831fecf0910160859o657fc405j9caa49e19f2a6e05@mail.gmail.com>
Download mbox | patch
Permalink /patch/424/
State Accepted
Headers show

Comments

Myles Watson - 2009-10-16 15:59:45
LB_MEM_TOPK is making it hard for me to modify linker scripts because
sometimes the shifting breaks it.

RAMTOP is much nicer:
- No need to shift it
- Matches with RAMBASE
- Shows up in the correct place in coreboot_ram.map

topk_defaults.diff & topk_shifts.diff are almost trivial, and were done 99%
by scripts.
topk_fixups.diff was done by hand and verified with meld, so I'm pretty sure
but I'd like another pair of eyes.

There was only one place where I had to do (RAMBASE >> 10)!

Abuild tested.

Signed-off-by: Myles Watson <mylesgw@gmail.com>

Thanks,
Myles
Peter Stuge - 2009-10-16 16:12:11
Myles Watson wrote:
> There was only one place where I had to do (RAMBASE >> 10)!

Maybe change that one to / 1024 instead?

In a few places CONFIG_RAMTOP is used without (), but none of them
looked bad. Maybe also search for ",CONFIG_RAMTOP" and add a space
there.

> Abuild tested.
> 
> Signed-off-by: Myles Watson <mylesgw@gmail.com>

Acked-by: Peter Stuge <peter@stuge.se>
Myles Watson - 2009-10-16 16:20:55
On Fri, Oct 16, 2009 at 10:12 AM, Peter Stuge <peter@stuge.se> wrote:

> Myles Watson wrote:
> > There was only one place where I had to do (RAMBASE >> 10)!
>

> Maybe change that one to / 1024 instead?
>
Do you think it's clearer to use /1024?  I just meant it was surprising to
multiply the value by 1024 and only have to put it back once.

In a few places CONFIG_RAMTOP is used without (), but none of them
> looked bad. Maybe also search for ",CONFIG_RAMTOP" and add a space
> there.
>
Fixed.

Thanks,
Myles
ron minnich - 2009-10-16 16:25:11
On Fri, Oct 16, 2009 at 9:20 AM, Myles Watson <mylesgw@gmail.com> wrote:

> Do you think it's clearer to use /1024?  I just meant it was surprising to
> multiply the value by 1024 and only have to put it back once.

I prefer >>10 because it more clearly expresses what you want. I've
been burned from time to time by people using
an arithmetic op where a bit op was intended. (+ instead of | for example)

ron
Myles Watson - 2009-10-16 16:34:07
On Fri, Oct 16, 2009 at 10:25 AM, ron minnich <rminnich@gmail.com> wrote:

> On Fri, Oct 16, 2009 at 9:20 AM, Myles Watson <mylesgw@gmail.com> wrote:
>
> > Do you think it's clearer to use /1024?  I just meant it was surprising
> to
> > multiply the value by 1024 and only have to put it back once.
>
> I prefer >>10 because it more clearly expresses what you want. I've
> been burned from time to time by people using
> an arithmetic op where a bit op was intended. (+ instead of | for example)
>
OK.

Rev 4788.

Thanks,
Myles
Peter Stuge - 2009-10-16 16:42:22
Myles Watson wrote:
> > > There was only one place where I had to do (RAMBASE >> 10)!
> >
> > Maybe change that one to / 1024 instead?
> 
> Do you think it's clearer to use /1024?

Yes, very much so.


> I just meant it was surprising to multiply the value by 1024 and
> only have to put it back once.

I agree with that.


> > In a few places CONFIG_RAMTOP is used without (), but none of them
> > looked bad. Maybe also search for ",CONFIG_RAMTOP" and add a space
> > there.
> 
> Fixed.

Great!


ron minnich wrote:
> I prefer >>10 because it more clearly expresses what you want.

I strongly prefer to get kilobytes from bytes using 1024 rather than 10.

I don't think the bit operator is more clear - this isn't a bit
operation, it's arithmetic.

The particular arithmetic happens to have an efficient implementation
using a bit operator, but the compiler knows that, so that the
programmer doesn't have to care and can use arithmetic expression,
which is meant to be (I think it is) more clear.


> I've been burned from time to time by people using an arithmetic op
> where a bit op was intended. (+ instead of | for example)

That would be a bug, but it doesn't translate to "all use of
arithmetic operators should be changed to use bit operators".

Not in C. For assembly you could convince me, but not in C. :)


//Peter
Uwe Hermann - 2009-10-16 16:47:53
On Fri, Oct 16, 2009 at 06:42:22PM +0200, Peter Stuge wrote:
> > I prefer >>10 because it more clearly expresses what you want.
> 
> I strongly prefer to get kilobytes from bytes using 1024 rather than 10.
>
> I don't think the bit operator is more clear - this isn't a bit
> operation, it's arithmetic.

Full ACK.

 
Uwe.

Patch

Index: cbv2/src/config/coreboot_ram.ld
===================================================================
--- cbv2.orig/src/config/coreboot_ram.ld
+++ cbv2/src/config/coreboot_ram.ld
@@ -103,8 +103,8 @@  SECTIONS
 	_stack = .;
 	.stack . : {
 		/* Reserve a stack for each possible cpu */
-		/* the stack for ap will be put after pgtbl in 1M to CONFIG_LB_MEM_TOPK range when VGA and ROM_RUN and CONFIG_LB_MEM_TOPK>1024*/
-		. = ((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_LB_MEM_TOPK>(0x100000>>10)) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
+		/* the stack for ap will be put after pgtbl in 1M to CONFIG_RAMTOP range when VGA and ROM_RUN and CONFIG_RAMTOP>1M*/
+		. = ((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN)&&(CONFIG_RAMBASE<0x100000)&&(CONFIG_RAMTOP>0x100000) ) ? CONFIG_STACK_SIZE : (CONFIG_MAX_CPUS*CONFIG_STACK_SIZE);
 	}
 	_estack = .;
         _heap = .;
@@ -120,9 +120,9 @@  SECTIONS
 	_ram_seg = _text; 
 	_eram_seg = _eheap;
 
-	_bogus = ASSERT( ( (_eram_seg>>10) < (CONFIG_LB_MEM_TOPK)) , "please increase CONFIG_LB_MEM_TOPK");
+	_bogus = ASSERT( ( _eram_seg < (CONFIG_RAMTOP)) , "please increase CONFIG_RAMTOP");
 
-        _bogus = ASSERT( !((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN) && ((_ram_seg<0xa0000) && (_eram_seg>0xa0000))) , "please increase CONFIG_LB_MEM_TOPK and if still fail, try to set CONFIG_RAMBASE more than 1M");
+        _bogus = ASSERT( !((CONFIG_CONSOLE_VGA || CONFIG_PCI_ROM_RUN) && ((_ram_seg<0xa0000) && (_eram_seg>0xa0000))) , "please increase CONFIG_RAMTOP and if still fail, try to set CONFIG_RAMBASE more than 1M");
 
 	/DISCARD/ : {
 		*(.comment)
Index: cbv2/src/cpu/amd/car/post_cache_as_ram.c
===================================================================
--- cbv2.orig/src/cpu/amd/car/post_cache_as_ram.c
+++ cbv2/src/cpu/amd/car/post_cache_as_ram.c
@@ -56,13 +56,13 @@  static void post_cache_as_ram(void)
 	print_debug_pcar("testx = ", testx);
 
 	/* copy data from cache as ram to 
-		ram need to set CONFIG_LB_MEM_TOPK to 2048 and use var mtrr instead.
+		ram need to set CONFIG_RAMTOP to 2M and use var mtrr instead.
 	 */
-#if CONFIG_LB_MEM_TOPK <= 1024
-        #error "You need to set CONFIG_LB_MEM_TOPK greater than 1024"
+#if CONFIG_RAMTOP <= 0x100000
+        #error "You need to set CONFIG_RAMTOP greater than 0x100000"
 #endif
 	
-	set_init_ram_access(); /* So we can access RAM from [1M, CONFIG_LB_MEM_TOPK) */
+	set_init_ram_access(); /* So we can access RAM from [1M, CONFIG_RAMTOP) */
 
 //	dump_mem(CONFIG_DCACHE_RAM_BASE+CONFIG_DCACHE_RAM_SIZE-0x8000, CONFIG_DCACHE_RAM_BASE+CONFIG_DCACHE_RAM_SIZE-0x7c00);
 	print_debug("Copying data from cache to RAM -- switching to use RAM as stack... ");
Index: cbv2/src/cpu/amd/model_fxx/model_fxx_init.c
===================================================================
--- cbv2.orig/src/cpu/amd/model_fxx/model_fxx_init.c
+++ cbv2/src/cpu/amd/model_fxx/model_fxx_init.c
@@ -308,8 +308,8 @@  static void init_ecc_memory(unsigned nod
 
 	/* Don't start too early */
 	begink = startk;
-	if (begink < CONFIG_LB_MEM_TOPK) { 
-		begink = CONFIG_LB_MEM_TOPK;
+	if (begink < (CONFIG_RAMTOP >> 10)) {
+		begink = (CONFIG_RAMTOP >>10);
 	}
 
 	printk_debug("Clearing memory %luK - %luK: ", begink, endk);
Index: cbv2/src/cpu/x86/lapic/lapic_cpu_init.c
===================================================================
--- cbv2.orig/src/cpu/x86/lapic/lapic_cpu_init.c
+++ cbv2/src/cpu/x86/lapic/lapic_cpu_init.c
@@ -246,17 +246,17 @@  int start_cpu(device_t cpu)
 	index = ++last_cpu_index;
 	
 	/* Find end of the new processors stack */
-#if (CONFIG_LB_MEM_TOPK>1024) && (CONFIG_RAMBASE < 0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1))
+#if (CONFIG_RAMTOP>0x100000) && (CONFIG_RAMBASE < 0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1))
 	if(index<1) { // only keep bsp on low 
 		stack_end = ((unsigned long)_estack) - (CONFIG_STACK_SIZE*index) - sizeof(struct cpu_info);
 	} else {
 		// for all APs, let use stack after pgtbl, 20480 is the pgtbl size for every cpu
 		stack_end = 0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS - (CONFIG_STACK_SIZE*index);
 #if (0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS) > (CONFIG_RAMTOP)
-		#warning "We may need to increase CONFIG_LB_MEM_TOPK, it need to be more than (0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS)\n"
+		#warning "We may need to increase CONFIG_RAMTOP, it need to be more than (0x100000+(20480 + CONFIG_STACK_SIZE)*CONFIG_MAX_CPUS)\n"
 #endif
 		if(stack_end > (CONFIG_RAMTOP)) {
-			printk_debug("start_cpu: Please increase the CONFIG_LB_MEM_TOPK more than %luK\n", stack_end>>10);
+			printk_debug("start_cpu: Please increase the CONFIG_RAMTOP more than %luK\n", stack_end);
 			die("Can not go on\n");
 		}
 		stack_end -= sizeof(struct cpu_info);
Index: cbv2/src/cpu/x86/mtrr/earlymtrr.c
===================================================================
--- cbv2.orig/src/cpu/x86/mtrr/earlymtrr.c
+++ cbv2/src/cpu/x86/mtrr/earlymtrr.c
@@ -11,8 +11,8 @@ 
 #if defined(CONFIG_XIP_ROM_BASE) && !defined(CONFIG_XIP_ROM_SIZE)
 # error "CONFIG_XIP_ROM_BASE without CONFIG_XIP_ROM_SIZE"
 #endif
-#if !defined(CONFIG_LB_MEM_TOPK)
-# error "CONFIG_LB_MEM_TOPK not defined"
+#if !defined(CONFIG_RAMTOP)
+# error "CONFIG_RAMTOP not defined"
 #endif
 
 #if defined(CONFIG_XIP_ROM_SIZE) && ((CONFIG_XIP_ROM_SIZE & (CONFIG_XIP_ROM_SIZE -1)) != 0)
@@ -22,8 +22,8 @@ 
 # error "CONFIG_XIP_ROM_BASE is not a multiple of CONFIG_XIP_ROM_SIZE"
 #endif
 
-#if (CONFIG_LB_MEM_TOPK & (CONFIG_LB_MEM_TOPK -1)) != 0
-# error "CONFIG_LB_MEM_TOPK must be a power of 2"
+#if (CONFIG_RAMTOP & (CONFIG_RAMTOP -1)) != 0
+# error "CONFIG_RAMTOP must be a power of 2"
 #endif
 
 static void disable_var_mtrr(unsigned reg)
Index: cbv2/src/cpu/x86/pae/pgtbl.c
===================================================================
--- cbv2.orig/src/cpu/x86/pae/pgtbl.c
+++ cbv2/src/cpu/x86/pae/pgtbl.c
@@ -54,19 +54,19 @@  void *map_2M_page(unsigned long page) 
 		struct pde pdp[512];
 	} __attribute__ ((packed));
 
-#if (CONFIG_LB_MEM_TOPK>1024) && (CONFIG_RAMBASE<0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1))
+#if (CONFIG_RAMTOP>0x100000) && (CONFIG_RAMBASE<0x100000) && ((CONFIG_CONSOLE_VGA==1) || (CONFIG_PCI_ROM_RUN == 1))
 	/*
 	 pgtbl is too big, so use last one 1M before CONFIG_LB_MEM_TOP, otherwise for 8 way dual core with vga support will push stack and heap cross 0xa0000, 
-	 and that region need to be used as vga font buffer. Please make sure set CONFIG_LB_MEM_TOPK=2048 in MB Config
+	 and that region need to be used as vga font buffer. Please make sure set CONFIG_RAMTOP=0x200000 in MB Config
 	*/
 	struct pg_table *pgtbl = (struct pg_table*)0x100000; //1M
 
 	unsigned x_end = 0x100000 + sizeof(struct pg_table) * CONFIG_MAX_CPUS;
 #if (0x100000+20480*CONFIG_MAX_CPUS) > (CONFIG_RAMTOP)
-                #warning "We may need to increase CONFIG_LB_MEM_TOPK, it need to be more than (0x100000+20480*CONFIG_MAX_CPUS)\n"
+                #warning "We may need to increase CONFIG_RAMTOP, it need to be more than (0x100000+20480*CONFIG_MAX_CPUS)\n"
 #endif
 	if(x_end > (CONFIG_RAMTOP)) {
-                        printk_debug("map_2M_page: Please increase the CONFIG_LB_MEM_TOPK more than %dK\n", x_end>>10);
+                        printk_debug("map_2M_page: Please increase the CONFIG_RAMTOP more than %dK\n", x_end);
                         die("Can not go on");
 	}
 #else
Index: cbv2/src/northbridge/amd/amdk8/raminit.c
===================================================================
--- cbv2.orig/src/northbridge/amd/amdk8/raminit.c
+++ cbv2/src/northbridge/amd/amdk8/raminit.c
@@ -11,8 +11,8 @@ 
 #include "raminit.h"
 #include "amdk8.h"
 
-#if (CONFIG_LB_MEM_TOPK & (CONFIG_LB_MEM_TOPK -1)) != 0
-# error "CONFIG_LB_MEM_TOPK must be a power of 2"
+#if (CONFIG_RAMTOP & (CONFIG_RAMTOP -1)) != 0
+# error "CONFIG_RAMTOP must be a power of 2"
 #endif
 
 #ifndef QRANK_DIMM_SUPPORT
Index: cbv2/src/northbridge/amd/amdk8/raminit_f.c
===================================================================
--- cbv2.orig/src/northbridge/amd/amdk8/raminit_f.c
+++ cbv2/src/northbridge/amd/amdk8/raminit_f.c
@@ -48,8 +48,8 @@ 
 #endif
 
 
-#if (CONFIG_LB_MEM_TOPK & (CONFIG_LB_MEM_TOPK -1)) != 0
-# error "CONFIG_LB_MEM_TOPK must be a power of 2"
+#if (CONFIG_RAMTOP & (CONFIG_RAMTOP -1)) != 0
+# error "CONFIG_RAMTOP must be a power of 2"
 #endif
 
 #include "amdk8_f_pci.c"