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

[ron minnich]

attached. We've been faking stop_ap in stage1 since the beginning.

This is a proposed option that builds

ron

[Peter Stuge]

ron minnich wrote:
> support for node id and core id. We've been faking this for two
> years now.

Good improvement.
> +++ mainboard/amd/serengeti/Makefile	(working copy)
> @@ -27,6 +27,8 @@
>    		$(src)/southbridge/amd/amd8111/stage1_smbus.c \
>    		$(src)/southbridge/amd/amd8111/stage1_ctrl.c \
>    		$(src)/southbridge/amd/amd8111/stage1_enable_rom.c \
> 	$(src)/arch/x86/amd/model_fxx/dualcore_id.c \
> 	$(src)/arch/x86/amd/model_fxx/stage1.c \
> $(src)/northbridge/amd/k8/coherent_ht.c \
> $(src)/northbridge/amd/k8/libstage1.c \
>
> @@ -38,10 +40,9 @@
>    		$(src)/arch/x86/pci_ops_conf1.c \
>    		$(src)/arch/x86/stage1_mtrr.c \
>    		$(src)/southbridge/amd/amd8111/stage1_smbus.c \
> 	$(src)/arch/x86/amd/model_fxx/init_cpus.c \
> $(src)/arch/x86/amd/model_fxx/dualcore.c \
> 	$(src)/arch/x86/amd/model_fxx/dualcore_id.c \
> $(src)/arch/x86/amd/model_fxx/fidvid.c \
> 	$(src)/arch/x86/amd/model_fxx/init_cpus.c \
> $(src)/lib/clog2.c

Hm, is the order important?
> /**
> Return core id/node id info. Always 0.
> */
>
> +struct node_core_id get_node_core_id(void)
> +{
> struct node_core_id id;
> id.nodeid = 0;
> id.coreid = 0;
>
> +}

Except there is no return. :)
> me = get_node_core_id();
>
> /* before we do anything, we want to stop if we dont run
> * on the bootstrap processor.
> * stop_ap is responsible for NOT stopping the BSP
> */
>
> stop_ap();

stop_ap() and comment could go before get_node_core_id().
> /* Initialize global variables before we can think of using them.
>     * NEVER run this on an AP!
>     */
> global_vars_init(&globvars);
> globvars.init_detected = init_detected;
> if (me.nodeid == 0) {
> global_vars_init(&globvars);
> globvars.init_detected = init_detected;
> }

And in any case this check shouldn't be needed - then stop_ap() will
have failed and if that can happen it should be handled by stop_ap().

//Peter

[ron minnich]

Thanks for the review Peter, here is try 2.

Also, yes, the order of some includes had to change.

ron

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

[ron minnich]

ok, I'm on travel for the next 10 days, so this won't get done :-)

On Sat, Sep 20, 2008 at 3:59 AM, Carl-Daniel Hailfinger
 wrote:
> 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.

The include thing is a difference of opinion, I sometimes get in Plan
9 habits. I'm not concerned too much, sure I'll include it in
global_variables.h in 10 days.
> >
>
> 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?

Let's leave bist as a parameter. Just because we do not yet use it
does not mean we never will.

it's a hard problem. if BIST is non zero things are bad. But we need
to do a lot of work to even print a message.
> 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.

v2 runs raminit on all nodes on some machines now. But they will never
run this particular code. They will be given an
IPI with the address of the AP code to be run for starting their ram.
So checks for a non-zero core id are not needed here.

The most we should do:

stop_ap();
if (me.nodeid || me.coreid)
  die("Things look dark");

The k8 raminit is a very interesting piece of code, worth learning
from. We've got some smart people out there :-)

If somebody wants to take this patch and improve it would be a big
help. I think we know what we need.

thanks

ron

[ron minnich]

On Sat, Sep 20, 2008 at 7:36 AM, ron minnich  wrote:
> If somebody wants to take this patch and improve it would be a big
> help. I think we know what we need.

OK, I am back from travel and patches are going to start up again.

Acked-by: Stefan Reinauer

r872.

Thanks

ron