<div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>> Index: src/cpu/amd/car/clear_1m_ram.c<br>> ===================================================================
<br>> --- src/cpu/amd/car/clear_1m_ram.c    (revision 0)<br>> +++ src/cpu/amd/car/clear_1m_ram.c    (revision 0)<br><br>I'm not real comfortable with C include files that are simply<br>one big assembly statement.  Something just seems wrong.
<br><br>I can understand the need for control when you are turning cache<br>as ram on and off though.</blockquote><div><br>
YH: that is called after disable cache as ram.<br>
 </div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>Testing for CONFIG_USE_INIT in any file that depends on cache_as_ram<br>seems the unnecessary.  Am I missing something here?
</blockquote><div><br>
YH: cache_as_ram, there is two way to use it: cache_as_ram_auto.c---> auto.inc<br>
or cache_as_ram_auto.c --> auto.o and .... <br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">> ===================================================================<br>> --- src/cpu/amd/car/cache_as_ram.inc  (revision 2079)
<br>> +++ src/cpu/amd/car/cache_as_ram.inc  (working copy)<br>> @@ -8,10 +8,17 @@<br>><br>>       /* Save the BIST result */<br>>       movl    %eax, %ebp<br>> +<br>> +     // for normal part %ebx already contain cpu_init_detected from fallback call
<br>><br>>  CacheAsRam:<br>>       /* hope we can skip the double set for normal part */<br>>  #if USE_FALLBACK_IMAGE == 1<br><br>This looks like a testing/validation problem only enabling cache_as_ram<br>in the fallback image.  I would rather continue to compile 
fallback.c<br>with romcc than to have weird special cases like this.</blockquote><div><br>
YH: because it could cause section conflicts that compiling with gcc..<br>
</div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">> ===================================================================<br>> --- src/cpu/amd/dualcore/dualcore.c   (revision 2079)
<br>> +++ src/cpu/amd/dualcore/dualcore.c   (working copy)<br>> @@ -2,6 +2,31 @@<br>><br>>  #include "cpu/amd/dualcore/dualcore_id.c"<br>><br>> +static inline unsigned get_core_num_in_bsp(unsigned nodeid)
<br>> +{<br>>
+        return
((pci_read_config32(PCI_DEV(0, 0x18+nodeid, 3), 0xe8)>>12) &
3);<br>> +}<br>> +<br>> +static inline uint8_t set_apicid_cpuid_lo(void)<br>> +{<br>> +        if(is_cpu_pre_e0()) return 0; // pre_e0 can not be set<br>> +<br>> +<br>> +        if(read_option(CMOS_VSTART_dual_core, CMOS_VLEN_dual_core, 0) != 0)  { // disable dual_core
<br>> +                return 0;<br>> +        }<br>> +<br>>
+                //
set the NB_CFG[54]=1; why the OS will be happy with that ???<br>> +        msr_t msr;<br>> +        msr = rdmsr(NB_CFG_MSR);<br>> +        msr.hi |= (1<<(54-32)); // InitApicIdCpuIdLo<br>> +        wrmsr(NB_CFG_MSR, msr);
<br>> +<br>> +        return 1;<br>> +<br>> +}<br><br>Our code is battling here.  The goal of do_k8_init_and_stop_secondaries<br>was to have code that could be shared between the two cases.<br>Even if it isn't so and we need to duplicate the logic it should
<br>be as close as possible between the two cases.<br><br>Right now we seem to have to totally different ways at looking at<br>the world.</blockquote><div><br>
<br>
YH: I use init_cpus for cache_as_ram in cpu/amd/model_fxx/init_cpus.c<br>
but it take several function, instead of ..., so it is readable...<br>
</div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">>  }<br><br>I'm not yet comfortable with the concept of having the equivalent<br>
of failover.c in the primary file.  Mostly because using cache_as_ram<br>this early means we can't test changes to it in the normal image.</blockquote><div><br>
<br>
YH: ??? <br>
</div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">>
-                        stop_this_cpu();
// it will stop all cores except core0 of cpu0<br>> -                }<br>>
+              init_cpus(cpu_init_detectedx,
sizeof(cpu)/sizeof(cpu[0]), cpu);<br><br>This looks like a good start, there is a lot less duplicate code here.<br><br>Why do we need to pass in the cpus?<br>Where is init_cpus defined?<br><br>I think we need a little harmonization between this and k8_init_and_stop_secondaries.
</blockquote><div><br>
YH:  cpu/amd/model_fxx/init_cpus.c, I miss it?<br>
</div><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">>          }<br>><br><br>Eric<br></blockquote></div><br>