[coreboot] [PATCH try 2] Reduce duplicate definition in CAR code.
wt at penguintechs.org
Thu Oct 7 09:22:44 CEST 2010
On Wed, Oct 6, 2010 at 5:51 AM, Myles Watson <mylesgw at gmail.com> wrote:
> On Tue, Oct 5, 2010 at 4:02 PM, Warren Turkal <wt at penguintechs.org> wrote:
> 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.
> 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.
More information about the coreboot