Hi,
Thanks for your effort on the code!
Stefan Reinauer wrote:
commit 1c810f17a1a3e9c5433e9eee4a18175d64698be2 Author: Stefan Reinauer reinauer@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