On Tue, Oct 5, 2010 at 4:02 PM, Warren Turkal <[email protected]> wrote: > Here's another cut at this patch that is more comprehensive. I have > included all major vendor car implementations. What do you all think > about this approach? > > Thanks, > wt > 8<------------ > Macros for the register addresses for the MTRR MSRs are already defined > in include/cpu/x86/car.h. This patch uses those macros instead of > creating a second instance of that same data. > > I also combined common MTRR definitions into a macro.
> -fixed_mtrr_msr: > - .long 0x250, 0x258, 0x259 ... > + /* fixed mtrr MSRs */ > + .long MTRRfix64K_00000_MSR > + .long MTRRfix16K_80000_MSR > + .long MTRRfix16K_A0000_MSR I'm fine with replacing the magic numbers, but moving the lists to ASM macros in C header files, when they're only used in this one place, doesn't make sense to me. I don't like having to look up END_MTRR_MSRS_TABLE_ENTRY_ASM to find that it is just a .long 0 > +X86_MTRR_MSRS_TABLE_ENTRIES_ASM > +AMD_MTRR_MSRS_TABLE_ENTRIES_ASM > +END_MTRR_MSRS_TABLE_ENTRY_ASM I don't find this easier to read than the original: > +#if defined(ASSEMBLY) > +.macro AMD_MTRR_MSRS_TABLE_ENTRIES_ASM > + /* Variable IORR MTRR MSRs */ > + .long 0xC0010016, 0xC0010017, 0xC0010018, 0xC0010019 > + /* Top of memory MTRR MSRs */ > + .long 0xC001001A, 0xC001001D > +.endm /* AMD_MTRR_MSRS_TABLE_ENTRIES_ASM */ > +#endif /* defined(ASSEMBLY) */ > + This code is easy enough to break that we have to be very careful about changing it. Thanks, Myles -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
