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

[Stefan Reinauer]

Stefan Reinauer ([e-mail]) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/871

-gerrit

commit 1c810f17a1a3e9c5433e9eee4a18175d64698be2
Author: Stefan Reinauer 
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

Change-Id: I13db384ba4e28acbe7f0f8c9cd169954b39f167d
    Signed-off-by: Stefan Reinauer 
    Signed-off-by: Duncan Laurie 
---
 src/cpu/x86/lapic/lapic_cpu_init.c |   15 +++++++++++----
 src/include/cpu/intel/speedstep.h  |    3 +++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/cpu/x86/lapic/lapic_cpu_init.c b/src/cpu/x86/lapic/lapic_cpu_init.c
index ed9940c..2ac9093 100644
--- a/src/cpu/x86/lapic/lapic_cpu_init.c
+++ b/src/cpu/x86/lapic/lapic_cpu_init.c
@@ -14,6 +14,7 @@
 #include <smp/atomic.h>
 #include <smp/spinlock.h>
 #include <cpu/cpu.h>
+#include <cpu/intel/speedstep.h>

#if CONFIG_SMP == 1
 /* This is a lot more paranoid now, since Linux can NOT handle
@@ -108,7 +109,7 @@ static int lapic_start_cpu(unsigned long apicid)
    	}
    	return 0;
    }
-#if !defined (CONFIG_CPU_AMD_MODEL_10XXX) && !defined (CONFIG_CPU_AMD_MODEL_14XXX)
+#if !CONFIG_CPU_AMD_MODEL_10XXX && !CONFIG_CPU_INTEL_MODEL_206AX
    mdelay(10);
 #endif

@@ -136,7 +137,7 @@ static int lapic_start_cpu(unsigned long apicid)

start_eip = get_valid_start_eip((unsigned long)_secondary_start);

-#if !defined (CONFIG_CPU_AMD_MODEL_10XXX) && !defined (CONFIG_CPU_AMD_MODEL_14XXX)
+#if !CONFIG_CPU_AMD_MODEL_10XXX
    num_starts = 2;
 #else
    num_starts = 1;
@@ -269,7 +270,7 @@ int start_cpu(device_t cpu)
    			break;
    		}
    		udelay(10);
-		}
+	}
    }
    secondary_stack = 0;
    spin_unlock(&start_cpu_lock);
@@ -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;
+
    /* Now loop until the other cpus have finished initializing */
    old_active_count = 1;
    active_count = atomic_read(&active_cpus);
@@ -456,17 +459,21 @@ static void wait_other_cpus_stop(struct bus *cpu_bus)
    	}
    	udelay(10);
    	active_count = atomic_read(&active_cpus);
+		loopcount++;
    }
    for(cpu = cpu_bus->children; cpu; cpu = cpu->sibling) {
    	if (cpu->path.type != DEVICE_PATH_APIC) {
    		continue;
    	}
+		if (cpu->path.apic.apic_id == SPEEDSTEP_APIC_MAGIC) {
+			continue;
+		}
    	if (!cpu->initialized) {
    		printk(BIOS_ERR, "CPU 0x%02x did not initialize!\n",
    			cpu->path.apic.apic_id);
    	}
    }
-	printk(BIOS_DEBUG, "All AP CPUs stopped\n");
+	printk(BIOS_DEBUG, "All AP CPUs stopped (%ld loops)\n", loopcount);
 }

#else /* CONFIG_SMP */
diff --git a/src/include/cpu/intel/speedstep.h b/src/include/cpu/intel/speedstep.h
index 0fa5244..00a5b9b 100644
--- a/src/include/cpu/intel/speedstep.h
+++ b/src/include/cpu/intel/speedstep.h
@@ -19,6 +19,9 @@
  * MA 02110-1301 USA
  */

+/* Magic value used to locate speedstep configuration in the device tree */
+#define SPEEDSTEP_APIC_MAGIC 0xACAC
+
 /* MWAIT coordination I/O base address. This must match
  * the _PR_.CPU0 PM base address.
  */

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 
> 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