> >> /* The memory is now setup, use it */ > >> +#if USE_DCACHE_RAM == 0 > >> cache_lbmem(MTRR_TYPE_WRBACK); > >> +#endif > >> } > > > > Could you explain why you had to add this? > > This board uses CAR, like the kontron. When we initialize the memory, > we are still in CAR, and cache_lbmem will setup MTRR its ways. From > what I understand, this will break CAR and we'll get stuck here. So > cache_lbmem is OK for boards using ROMCC (mtarvon, truxton...), but is > bad with CAR. OK.
> > I've refactored two of your patches so that they use svn cp from kontron > and > > model_6fx sources to show the differences. If you based your work on > > different sources, let me know. > > I'm really I wanted to do that but I did not find the time (lot of > work right now). I used a mix of 2 board : for the CAR skeleton I used > the Kontron 986LCDM mainboard, for the 3100 stuff the mt arvon board. Great. > > My comments: > > There is some #if 0 code in acpi_tables. Will it ever be enabled? If > not, > > remove it. > > Yes indeed. As I said I started from the kontron mainboard but in my > case I didn't fill the OEMB table so they can safely be removed. We > can still fill table later if needed. I also found another #if 0 / > #endif pair in auto.c. Its about the MSR_THERM2_CTL. I remember I add > to disable it on a core 2 duo L7400 otherwise I was stuck. I'll check > it today if we can re-enable this one. Thanks. > > I'm confused why we need eagleheights_fixups. Can we remove it? > > Yes indeed. I always add it in case but here I did not had to use it, so > trash. Will do. > > Index: svn/src/cpu/intel/model_1067x/cache_as_ram_post.c > > =================================================================== > > --- svn.orig/src/cpu/intel/model_1067x/cache_as_ram_post.c > > +++ svn/src/cpu/intel/model_1067x/cache_as_ram_post.c > > @@ -50,9 +50,9 @@ > > "wrmsr\n" > > "movl $MTRRphysMask_MSR(1), %ecx\n" > > "wrmsr\n" > > -#endif > > > > "movb $0x33, %al\noutb %al, $0x80\n" > > +#endif > > #ifdef CLEAR_FIRST_1M_RAM > > "movb $0x34, %al\noutb %al, $0x80\n" > > /* Enable Write Combining and Speculative Reads for the first 1MB > */ > > @@ -120,7 +120,7 @@ > > "movb $0x3b, %al\noutb %al, $0x80\n" > > > > /* Enable prefetchers */ > > - "movl $0x01a0, %eax\n" > > + "movl $0x01a0, %ecx\n" > > "rdmsr\n" > > "andl $~((1 << 9) | (1 << 19)), %eax\n" > > "andl $~((1 << 5) | (1 << 7)), %edx\n" > > > > These changes were surprising. Is there a bug in the original code? > > In the code, we execute a block a instructions to setup stuffs, then > output a post code. But if the instructions are disabled (ifdef > CLEAR_FIRST_1M_RAM), it's useless to output the post code no ? For the > last modifications (enable prefetchers : eax becomes ecx), there is > indeed a bug. I already sent a patch for this one few months ago but > it has been lost in the mailing list. Sorry about that. > The rdmsr instruction will read > the msr specified by ecx into edx:eax (Intel Software Dev Manual, > volume 2B, 4-322). So I think we should fix the original code and not duplicate it. Stefan: What do you think about unifying src/cpu/intel/model_*/cache_as_ram_post.c? There are a couple of the files that are very similar. > Thanks again for taking the time to review my patch. No problem. Could you send me the corrected Config.lb now that you removed the disabled devices? Thanks, Myles -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
