I added names for the AMD registers and separated each .long onto it's own line to avoid any bugs of the type the caldani mentioned.
Comments? Acks? :)
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. I also added a few macros to the amd mtrr.h to make the MSR naming more consistent.
Signed-off-by: Warren Turkal wt@penguintechs.org --- src/cpu/amd/car/cache_as_ram.inc | 24 ++++----------------- src/cpu/intel/car/cache_as_ram.inc | 17 +++------------ src/cpu/via/car/cache_as_ram.inc | 17 +++------------ src/include/cpu/amd/mtrr.h | 27 +++++++++++++++++++----- src/include/cpu/x86/mtrr.h | 39 ++++++++++++++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 51 deletions(-)
diff --git a/src/cpu/amd/car/cache_as_ram.inc b/src/cpu/amd/car/cache_as_ram.inc index 5318272..4f692b2 100644 --- a/src/cpu/amd/car/cache_as_ram.inc +++ b/src/cpu/amd/car/cache_as_ram.inc @@ -155,7 +155,7 @@ enable_fixed_mtrr_dram_modify:
/* Clear all MTRRs. */ xorl %edx, %edx - movl $fixed_mtrr_msr, %esi + movl $all_mtrr_msrs, %esi
clear_fixed_var_mtrr: lodsl (%esi), %eax @@ -396,23 +396,9 @@ CAR_FAM10_ap_out:
post_code(0xaf) /* Should never see this POST code. */
-fixed_mtrr_msr: - .long 0x250, 0x258, 0x259 - .long 0x268, 0x269, 0x26A - .long 0x26B, 0x26C, 0x26D - .long 0x26E, 0x26F - -var_mtrr_msr: - .long 0x200, 0x201, 0x202, 0x203 - .long 0x204, 0x205, 0x206, 0x207 - .long 0x208, 0x209, 0x20A, 0x20B - .long 0x20C, 0x20D, 0x20E, 0x20F - -var_iorr_msr: - .long 0xC0010016, 0xC0010017, 0xC0010018, 0xC0010019 - -mem_top: - .long 0xC001001A, 0xC001001D - .long 0x000 /* NULL, end of table */ +all_mtrr_msrs: +X86_MTRR_MSRS_TABLE_ENTRIES +AMD_MTRR_MSRS_TABLE_ENTRIES +END_MTRR_MSRS_TABLE_ENTRY
cache_as_ram_setup_out: diff --git a/src/cpu/intel/car/cache_as_ram.inc b/src/cpu/intel/car/cache_as_ram.inc index d8465f4..29a9c95 100644 --- a/src/cpu/intel/car/cache_as_ram.inc +++ b/src/cpu/intel/car/cache_as_ram.inc @@ -115,7 +115,7 @@ NotHtProcessor:
/* Clear all MTRRs. */ xorl %edx, %edx - movl $fixed_mtrr_msr, %esi + movl $all_mtrr_msrs, %esi
clear_fixed_var_mtrr: lodsl (%esi), %eax @@ -128,18 +128,9 @@ clear_fixed_var_mtrr:
jmp clear_fixed_var_mtrr
-fixed_mtrr_msr: - .long 0x250, 0x258, 0x259 - .long 0x268, 0x269, 0x26A - .long 0x26B, 0x26C, 0x26D - .long 0x26E, 0x26F - -var_mtrr_msr: - .long 0x200, 0x201, 0x202, 0x203 - .long 0x204, 0x205, 0x206, 0x207 - .long 0x208, 0x209, 0x20A, 0x20B - .long 0x20C, 0x20D, 0x20E, 0x20F - .long 0x000 /* NULL, end of table */ +all_mtrr_msrs: +X86_MTRR_MSRS_TABLE_ENTRIES +END_MTRR_MSRS_TABLE_ENTRY
clear_fixed_var_mtrr_out:
diff --git a/src/cpu/via/car/cache_as_ram.inc b/src/cpu/via/car/cache_as_ram.inc index d6df4a9..6394110 100644 --- a/src/cpu/via/car/cache_as_ram.inc +++ b/src/cpu/via/car/cache_as_ram.inc @@ -47,7 +47,7 @@ CacheAsRam:
/* Clear all MTRRs. */ xorl %edx, %edx - movl $fixed_mtrr_msr, %esi + movl $all_mtrr_msrs, %esi
clear_fixed_var_mtrr: lodsl (%esi), %eax @@ -60,18 +60,9 @@ clear_fixed_var_mtrr:
jmp clear_fixed_var_mtrr
-fixed_mtrr_msr: - .long 0x250, 0x258, 0x259 - .long 0x268, 0x269, 0x26A - .long 0x26B, 0x26C, 0x26D - .long 0x26E, 0x26F - -var_mtrr_msr: - .long 0x200, 0x201, 0x202, 0x203 - .long 0x204, 0x205, 0x206, 0x207 - .long 0x208, 0x209, 0x20A, 0x20B - .long 0x20C, 0x20D, 0x20E, 0x20F - .long 0x000 /* NULL, end of table */ +all_mtrr_msrs: +X86_MTRR_MSRS_TABLE_ENTRIES +END_MTRR_MSRS_TABLE_ENTRY
clear_fixed_var_mtrr_out: movl $MTRRphysBase_MSR(0), %ecx diff --git a/src/include/cpu/amd/mtrr.h b/src/include/cpu/amd/mtrr.h index c7b3fca..ec56c06 100644 --- a/src/include/cpu/amd/mtrr.h +++ b/src/include/cpu/amd/mtrr.h @@ -21,16 +21,31 @@ #define SYSCFG_MSR_SysVicLimitMask ((1 << 8) - (1 << 5)) #define SYSCFG_MSR_SysAckLimitMask ((1 << 5) - (1 << 0))
-#define IORR0_BASE 0xC0010016 -#define IORR0_MASK 0xC0010017 -#define IORR1_BASE 0xC0010018 -#define IORR1_MASK 0xC0010019 -#define TOP_MEM 0xC001001A -#define TOP_MEM2 0xC001001D +#define IORRBase_MSR(reg) (0xC0010016 + 2 * (reg)) +#define IORRMask_MSR(reg) (0xC0010016 + 2 * (reg) + 1) + +#define TOP_MEM_MSR 0xC001001A +#define TOP_MEM2_MSR 0xC001001D +#define TOP_MEM TOP_MEM_MSR +#define TOP_MEM2 TOP_MEM2_MSR
#define TOP_MEM_MASK 0x007fffff #define TOP_MEM_MASK_KB (TOP_MEM_MASK >> 10)
+#if defined(ASSEMBLY) +.macro AMD_MTRR_MSRS_TABLE_ENTRIES + /* Variable IORR MTRR MSRs */ + .long IORRBase_MSR(0) + .long IORRMask_MSR(0) + .long IORRBase_MSR(1) + .long IORRMask_MSR(1) + + /* Top of memory MTRR MSRs */ + .long TOP_MEM_MSR + .long TOP_MEM2_MSR +.endm /* AMD_MTRR_MSRS_TABLE_ENTRIES */ +#endif /* defined(ASSEMBLY) */ + #if !defined(__PRE_RAM__) && !defined(ASSEMBLY) void amd_setup_mtrrs(void); #endif diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h index e79c90e..826a46d 100644 --- a/src/include/cpu/x86/mtrr.h +++ b/src/include/cpu/x86/mtrr.h @@ -34,6 +34,45 @@ #define MTRRfix4K_F0000_MSR 0x26e #define MTRRfix4K_F8000_MSR 0x26f
+#if defined (ASSEMBLY) +.macro X86_MTRR_MSRS_TABLE_ENTRIES + /* fixed mtrr MSRs */ + .long MTRRfix64K_00000_MSR + .long MTRRfix16K_80000_MSR + .long MTRRfix16K_A0000_MSR + .long MTRRfix4K_C0000_MSR + .long MTRRfix4K_C8000_MSR + .long MTRRfix4K_D0000_MSR + .long MTRRfix4K_D8000_MSR + .long MTRRfix4K_E0000_MSR + .long MTRRfix4K_E8000_MSR + .long MTRRfix4K_F0000_MSR + .long MTRRfix4K_F8000_MSR + + /* var mtrr MSRs */ + .long MTRRphysBase_MSR(0) + .long MTRRphysMask_MSR(0) + .long MTRRphysBase_MSR(1) + .long MTRRphysMask_MSR(1) + .long MTRRphysBase_MSR(2) + .long MTRRphysMask_MSR(2) + .long MTRRphysBase_MSR(3) + .long MTRRphysMask_MSR(3) + .long MTRRphysBase_MSR(4) + .long MTRRphysMask_MSR(4) + .long MTRRphysBase_MSR(5) + .long MTRRphysMask_MSR(5) + .long MTRRphysBase_MSR(6) + .long MTRRphysMask_MSR(6) + .long MTRRphysBase_MSR(7) + .long MTRRphysMask_MSR(7) +.endm /* X86_MTRR_MSRS_TABLE_ENTRIES */ + +.macro END_MTRR_MSRS_TABLE_ENTRY + .long 0x000 /* NULL, end of table */ +.endm /* END_MTRR_MSRS_TABLE_ENTRY */ +#endif /* defined (ASSEMBLY) */ + #if !defined (ASSEMBLY) && !defined(__PRE_RAM__) #include <device/device.h> void enable_fixed_mtrr(void);
On Thu, Oct 07, 2010 at 04:35:04AM -0700, Warren Turkal wrote:
I added names for the AMD registers and separated each .long onto it's own line to avoid any bugs of the type the caldani mentioned.
Comments? Acks? :)
Just my $0.02 - I like how you've replaced the magic numbers with names. I don't like how you've moved the lists to another file.
My suggestion would be to replace all the magic numbers in the assembler files. Then once that's complete, send patches with your proposal to change the assembler layout.
BTW, the list concept doesn't make much sense anyway - at least on Via, there is no need to clear the fixed mtrrs, and you don't need a list to clear the variable mtrrs (a simple iterator would suffice).
-Kevin
On Thu, Oct 7, 2010 at 5:58 AM, Kevin O'Connor kevin@koconnor.net wrote:
Just my $0.02 - I like how you've replaced the magic numbers with names. I don't like how you've moved the lists to another file.
My suggestion would be to replace all the magic numbers in the assembler files. Then once that's complete, send patches with your proposal to change the assembler layout.
BTW, the list concept doesn't make much sense anyway - at least on Via, there is no need to clear the fixed mtrrs, and you don't need a list to clear the variable mtrrs (a simple iterator would suffice).
Agree on all points ...
ron
On 10/7/10 5:58 AM, Kevin O'Connor wrote:
BTW, the list concept doesn't make much sense anyway - at least on Via, there is no need to clear the fixed mtrrs, and you don't need a list to clear the variable mtrrs (a simple iterator would suffice).
The list seemed more comprehensible than writing down linear code. It's simple for the variable mtrrs, but the fixed ones are not linearly spread.
Why do you assume it's not needed to clear the fixed MTRRs on VIA systems? I don't think we should assume they didn't get set to "bad" values by the OS running prior to a reset, for example.
Stefan
Actually, one more comment here, as if we need one :-)
It's actually been easiest for me over the years to define initialized data in C, even if used in assembly code, i.e.:
u32 *allmtrr[] = {fixedmtrr, varmtrr, amdmtrr, NULL};
u32 fixedmtrr[] = {0xthis, 0xthat, 0xother, 0};
Then reference these from assembly code. In fact, I frequently write the C code to walk this double loop, generate asm, and use that (I do this more commonly on Plan 9 than linux for several reasons but the idea is the same).
The way to actually use these structs is left as an exercise for the reader :-)
It's really best to use the C compiler as often as you can for things, including static initialized data. At least it is for me.
ron
If the code was already setup to do that, I certainly would, but it's really not setup that way for the CAR code. That would be a much bigger change to the structure of the code IMO.
wt
On Thu, Oct 7, 2010 at 10:19 AM, ron minnich rminnich@gmail.com wrote:
Actually, one more comment here, as if we need one :-)
It's actually been easiest for me over the years to define initialized data in C, even if used in assembly code, i.e.:
u32 *allmtrr[] = {fixedmtrr, varmtrr, amdmtrr, NULL};
u32 fixedmtrr[] = {0xthis, 0xthat, 0xother, 0};
Then reference these from assembly code. In fact, I frequently write the C code to walk this double loop, generate asm, and use that (I do this more commonly on Plan 9 than linux for several reasons but the idea is the same).
The way to actually use these structs is left as an exercise for the reader :-)
It's really best to use the C compiler as often as you can for things, including static initialized data. At least it is for me.
ron
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On Thu, Oct 07, 2010 at 09:16:28AM -0700, Stefan Reinauer wrote:
On 10/7/10 5:58 AM, Kevin O'Connor wrote:
BTW, the list concept doesn't make much sense anyway - at least on Via, there is no need to clear the fixed mtrrs, and you don't need a list to clear the variable mtrrs (a simple iterator would suffice).
The list seemed more comprehensible than writing down linear code. It's simple for the variable mtrrs, but the fixed ones are not linearly spread.
Why do you assume it's not needed to clear the fixed MTRRs on VIA systems? I don't think we should assume they didn't get set to "bad" values by the OS running prior to a reset, for example.
The via code runs with fixed mtrrs disabled in MTRRdefType_MSR. (It writes 0x800 there instead of 0xc00.) Because fixed MTRRs aren't enabled during CAR, they don't need to be cleared in the CAR setup phase.
-Kevin
Even with that, the CAR code for via currently has the following: /* Clear all MTRRs. */ xorl %edx, %edx movl $fixed_mtrr_msr, %esi
clear_fixed_var_mtrr: lodsl (%esi), %eax testl %eax, %eax jz clear_fixed_var_mtrr_out
movl %eax, %ecx xorl %eax, %eax wrmsr
jmp clear_fixed_var_mtrr
fixed_mtrr_msr: .long 0x250, 0x258, 0x259 .long 0x268, 0x269, 0x26A .long 0x26B, 0x26C, 0x26D .long 0x26E, 0x26F
var_mtrr_msr: .long 0x200, 0x201, 0x202, 0x203 .long 0x204, 0x205, 0x206, 0x207 .long 0x208, 0x209, 0x20A, 0x20B .long 0x20C, 0x20D, 0x20E, 0x20F .long 0x000 /* NULL, end of table */
That fixed_mtrr_msr list is identical to the intel fixed_mtrr_msr list, so the registers are being cleared whether they need to be or not.
Speaking of which, is the via bios writer's guide accessible by mere mortals?
wt
On Thu, Oct 7, 2010 at 5:13 PM, Kevin O'Connor kevin@koconnor.net wrote:
On Thu, Oct 07, 2010 at 09:16:28AM -0700, Stefan Reinauer wrote:
On 10/7/10 5:58 AM, Kevin O'Connor wrote:
BTW, the list concept doesn't make much sense anyway - at least on Via, there is no need to clear the fixed mtrrs, and you don't need a list to clear the variable mtrrs (a simple iterator would suffice).
The list seemed more comprehensible than writing down linear code. It's simple for the variable mtrrs, but the fixed ones are not linearly spread.
Why do you assume it's not needed to clear the fixed MTRRs on VIA systems? I don't think we should assume they didn't get set to "bad" values by the OS running prior to a reset, for example.
The via code runs with fixed mtrrs disabled in MTRRdefType_MSR. (It writes 0x800 there instead of 0xc00.) Because fixed MTRRs aren't enabled during CAR, they don't need to be cleared in the CAR setup phase.
-Kevin
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
On Thu, Oct 7, 2010 at 9:16 AM, Stefan Reinauer stefan.reinauer@coresystems.de wrote:
On 10/7/10 5:58 AM, Kevin O'Connor wrote:
BTW, the list concept doesn't make much sense anyway - at least on Via, there is no need to clear the fixed mtrrs, and you don't need a list to clear the variable mtrrs (a simple iterator would suffice).
The list seemed more comprehensible than writing down linear code. It's simple for the variable mtrrs, but the fixed ones are not linearly spread.
Why do you assume it's not needed to clear the fixed MTRRs on VIA systems? I don't think we should assume they didn't get set to "bad" values by the OS running prior to a reset, for example.
Are you saying that the list doesn't make sense for variable MTRRs because that happen to be even spaced and can be calculated?
wt
-
Kevin O'Connor -
ron minnich -
Stefan Reinauer -
Warren Turkal