[coreboot] patch: working SMP startup for kontron/core2

Peter Stuge peter at stuge.se
Sat Mar 7 11:05:33 CET 2009


ron minnich wrote:
> comments welcome. Please see notes in the patch.

Short version: Very nice!


> I am not as interested in comments on this specific code (it needs
> cleanup) as I am in two questions:

I'll mention a few things that I would like to be included in that
cleanup.


> 1. can artec please test the current svn to make sure there is
> nothing I have broken

Good point. Could be tested also on ALIX.


> 2. Are the changes to lib/stage2.c ok

I think so.


> Anyway, take a look. With luck, we have SMP on the kontron within
> the week; SMI follows, then ACPI, then maybe we can make v3 the
> preferred kontron software base. 

It would be awesome!


> +++ lib/stage2.c	(working copy)
..
> @@ -85,6 +104,11 @@
>  	dev_phase6();
>  	show_all_devs(BIOS_DEBUG, "After phase 6.");
>  
> +	/* final cleanup: do any remaining CPU setup. This can include memory 
> +	 * init, or not, depending on the CPU; it may have been done in phase 1. 
> + 	 */
> +	cpu_phase2(is_coldboot(), sysinfo);
> +

The comment mentions phase 1 - but which phase 1 is that? Would help
if it said cpu_phase1() or stage2_phase1() instead.


> +	movw	$0x11, 0

Just curious, what do these movws to 0 do?


> +++ arch/x86/intel/core2/init_cpus.c	(working copy)
..
> @@ -73,6 +72,7 @@
>  	int nodes, siblings;
>  	result = cpuid(1);
>  	/* See how many sibling cpus we have */
> +	printk(BIOS_DEBUG, "cpuid(1) %x\n", result.ebx);

Please make this debug output say ebx=%08x because even if that is
redundant for the people looking at it right now, output like this
can linger and anyone else may be confused.


> @@ -377,18 +384,25 @@
>  	stackmem->stacks[index].post   = 0;
>  	stackmem->stacks[index].active_cpus  = active_cpus;
>  	stackmem->stacks[index].start_cpu_lock = start_cpu_lock;
> +	printk(BIOS_SPEW, "stack[index, apicid, post, active_cpus, start_cpu_lock = [%lx, %x, %d, %p, %p]\n", index, apicid, 0, active_cpus, start_cpu_lock);

I guess this is missing ] after start_cpu_lock.


Overall great improvements!


//Peter




More information about the coreboot mailing list