[coreboot] [PATCH try 2] Reduce duplicate definition in CAR code.

Warren Turkal 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:

*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




More information about the coreboot mailing list