[coreboot] patch: make node id/core id available on all arch; really implement stop_ap

[Carl-Daniel Hailfinger]

On 20.09.2008 06:51, ron minnich wrote:
> Thanks for the review Peter, here is try 2.
>
> Also, yes, the order of some includes had to change.
>   

IMO that's a bug. If one include needs another, it should #include it,
not depend on random ordering.

> ron
>   
> --- arch/x86/stage1.c	(revision 867)
> +++ arch/x86/stage1.c	(working copy)
> @@ -21,6 +21,7 @@
>  #include <types.h>
>  #include <io.h>
>  #include <console.h>
> +#include <cpu.h>
>  #include <globalvars.h>
>  #include <lar.h>
>  #include <string.h>
> @@ -43,12 +44,6 @@
>  void disable_car(void);
>  void mainboard_pre_payload(void);
>  
> -static void stop_ap(void)
> -{
> -	// nothing yet
> -	post_code(POST_STAGE1_STOP_AP);
> -}
> -
>  static void enable_rom(void)
>  {
>  	// nothing here yet
> @@ -156,6 +151,7 @@
>  	int ret;
>  	struct mem_file archive;
>  	void *entry;
> +	struct node_core_id me;
>   

Dead/uninitialized variable.

>  #ifdef CONFIG_PAYLOAD_ELF_LOADER
>  	struct mem_file result;
>  	int elfboot_mem(struct lb_memory *mem, void *where, int size);
> @@ -174,16 +170,13 @@
>  
>  	post_code(POST_STAGE1_MAIN);
>  
> -	// before we do anything, we want to stop if we dont run
> -	// on the bootstrap processor.
> -#warning We do not want to check BIST here, we want to check whether we are BSC!
> -	if (bist==0) {
> -		// stop secondaries
> -		stop_ap();
> -	}
>   

While the new code is obviously what we wanted back then, please tell me
why we pass BIST to stage1 if we totally ignore it. Should we die()
early if bist!=0 or die even earlier in stage0?

> +	/* before we do anything, we want to stop if we do not run
> +	 * on the bootstrap processor.
> +	 * stop_ap is responsible for NOT stopping the BSP
> +	 */
> +	stop_ap();
>  
>  	/* Initialize global variables before we can think of using them.
> -	 * NEVER run this on an AP!
>   

Please leave that comment in. Once we have to run raminit from each node
(family 10h), that code will be run by all cores. I'd say that your
original patch for this was better because it made execution conditional.

>  	 */
>  	global_vars_init(&globvars);
>  	globvars.init_detected = init_detected;

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/