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