On Wed, Oct 6, 2010 at 5:51 AM, Myles Watson <[email protected]> wrote: > On Tue, Oct 5, 2010 at 4:02 PM, Warren Turkal <[email protected]> wrote:
*snip* > 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. They are used in at least 3 places, each of the vendor car implementations. They also should be used in the other intel car implementations, but those use a slightly different algorithm for clearing the registers, and I didn't want that change to be wrapped up in this one as well. > I don't like having to look up END_MTRR_MSRS_TABLE_ENTRY_ASM to find > that it is just a .long 0 It just becomes a magic number if I don't give it a symbolic name. I would like to change the way that the vendor CAR implementations work to work like the implementation in src/cpu/intel/model_106cx/cache_as_ram.inc. That file's implementation doesn't depend on a sentinel value at the end of the list. That would eliminate the need for the END_MTRR_MSRS_TABLE_ENTRY_ASM macro. >> +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. I definitely agree. wt -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
