[coreboot] Intel Eagle Height evaluation board support
Thomas JOURDAN
thomas.jourdan at gmail.com
Tue Jun 30 09:06:53 CEST 2009
Hi Myles, guys
>> /* 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.
> 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.
> 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.
> 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.
> 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. The rdmsr instruction will read
the msr specified by ecx into edx:eax (Intel Software Dev Manual,
volume 2B, 4-322).
Thanks again for taking the time to review my patch.
Thomas
More information about the coreboot
mailing list