[coreboot] Patch set updated for coreboot: 1c810f1 Fixes and Sandybridge support for lapic cpu init

[Peter Stuge]

Hi,

Thanks for your effort on the code!

Stefan Reinauer wrote:
> commit 1c810f17a1a3e9c5433e9eee4a18175d64698be2
> Author: Stefan Reinauer <reinauer at chromium.org>
> Date:   Tue Apr 3 16:17:11 2012 -0700
> 
>     Fixes and Sandybridge support for lapic cpu init
> 
>     - preprocessor macros should not use defined(CONFIG_*) but
>       just CONFIG_*
>     - drop AMD CPU model 14XXX config variable use. Those do not exist.
>     - skip some delays on Sandybridge systems
>     - Count how long we're waiting for each AP to stop
>     - Skip speedstep specific CPU entries

I would really have liked it if this had been split into one commit
for each change. It would not have been much work, but it would have
looked oh so much prettier.


> @@ -269,7 +270,7 @@ int start_cpu(device_t cpu)
>  				break;
>  			}
>  			udelay(10);
> -		}
> +	}
>  	}
>  	secondary_stack = 0;
>  	spin_unlock(&start_cpu_lock);

The above whitespace change simply can not be right. Please review
*carefully* before pushing patches.


> @@ -446,6 +447,8 @@ static void wait_other_cpus_stop(struct bus *cpu_bus)
>  {
>  	device_t cpu;
>  	int old_active_count, active_count;
> +	long loopcount = 0;

We like stdint types, but in throwaway cases like this I think word
length is fine. However, I believe this variable should have been
unsigned since it starts at 0

> @@ -456,17 +459,21 @@ static void wait_other_cpus_stop(struct bus *cpu_bus)
>  		}
>  		udelay(10);
>  		active_count = atomic_read(&active_cpus);
> +		loopcount++;

..gets increased


> -	printk(BIOS_DEBUG, "All AP CPUs stopped\n");
> +	printk(BIOS_DEBUG, "All AP CPUs stopped (%ld loops)\n", loopcount);

..and is finally just printed. It could theoretically wrap, and
there's just no point in making coreboot print a completely bogus
negative value.


//Peter