[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