[LinuxBIOS] Latest commit - clarification

[yhlu]

>
>
> > Index: src/cpu/amd/car/clear_1m_ram.c
> > ===================================================================
> > --- src/cpu/amd/car/clear_1m_ram.c (revision 0)
> > +++ src/cpu/amd/car/clear_1m_ram.c (revision 0)
>
> I'm not real comfortable with C include files that are simply
> one big assembly statement. Something just seems wrong.
>
> I can understand the need for control when you are turning cache
> as ram on and off though.


YH: that is called after disable cache as ram.


> Testing for CONFIG_USE_INIT in any file that depends on cache_as_ram
> seems the unnecessary. Am I missing something here?


YH: cache_as_ram, there is two way to use it: cache_as_ram_auto.c--->
auto.inc
or cache_as_ram_auto.c --> auto.o and ....

> > ===================================================================
> > --- src/cpu/amd/car/cache_as_ram.inc (revision 2079)
> > +++ src/cpu/amd/car/cache_as_ram.inc (working copy)
> > @@ -8,10 +8,17 @@
> >
> > /* Save the BIST result */
> > movl %eax, %ebp
> > +
> > + // for normal part %ebx already contain cpu_init_detected from
> fallback call
> >
> > CacheAsRam:
> > /* hope we can skip the double set for normal part */
> > #if USE_FALLBACK_IMAGE == 1
>
> This looks like a testing/validation problem only enabling cache_as_ram
> in the fallback image. I would rather continue to compile fallback.c
> with romcc than to have weird special cases like this.


YH: because it could cause section conflicts that compiling with gcc..

> ===================================================================
> > --- src/cpu/amd/dualcore/dualcore.c (revision 2079)
> > +++ src/cpu/amd/dualcore/dualcore.c (working copy)
> > @@ -2,6 +2,31 @@
> >
> > #include "cpu/amd/dualcore/dualcore_id.c"
> >
> > +static inline unsigned get_core_num_in_bsp(unsigned nodeid)
> > +{
> > + return ((pci_read_config32(PCI_DEV(0, 0x18+nodeid, 3), 0xe8)>>12) &
> 3);
> > +}
> > +
> > +static inline uint8_t set_apicid_cpuid_lo(void)
> > +{
> > + if(is_cpu_pre_e0()) return 0; // pre_e0 can not be set
> > +
> > +
> > + if(read_option(CMOS_VSTART_dual_core, CMOS_VLEN_dual_core, 0) != 0) {
> // disable dual_core
> > + return 0;
> > + }
> > +
> > + // set the NB_CFG[54]=1; why the OS will be happy with that ???
> > + msr_t msr;
> > + msr = rdmsr(NB_CFG_MSR);
> > + msr.hi |= (1<<(54-32)); // InitApicIdCpuIdLo
> > + wrmsr(NB_CFG_MSR, msr);
> > +
> > + return 1;
> > +
> > +}
>
> Our code is battling here. The goal of do_k8_init_and_stop_secondaries
> was to have code that could be shared between the two cases.
> Even if it isn't so and we need to duplicate the logic it should
> be as close as possible between the two cases.
>
> Right now we seem to have to totally different ways at looking at
> the world.



YH: I use init_cpus for cache_as_ram in cpu/amd/model_fxx/init_cpus.c
but it take several function, instead of ..., so it is readable...

> }
>
> I'm not yet comfortable with the concept of having the equivalent
> of failover.c in the primary file. Mostly because using cache_as_ram
> this early means we can't test changes to it in the normal image.



YH: ???

> - stop_this_cpu(); // it will stop all cores except core0 of cpu0
> > - }
> > + init_cpus(cpu_init_detectedx, sizeof(cpu)/sizeof(cpu[0]), cpu);
>
> This looks like a good start, there is a lot less duplicate code here.
>
> Why do we need to pass in the cpus?
> Where is init_cpus defined?
>
> I think we need a little harmonization between this and
> k8_init_and_stop_secondaries.


YH: cpu/amd/model_fxx/init_cpus.c, I miss it?

> }
> >
>
> Eric
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20051025/d223aaa2/attachment.html>