Patchwork Reduce duplicate definition in CAR code.

login
register
about
Submitter Warren Turkal
Date 2010-10-05 10:00:36
Message ID <1286272836-1767-1-git-send-email-wt@penguintechs.org>
Download mbox | patch
Permalink /patch/2039/
State Superseded
Headers show

Comments

Warren Turkal - 2010-10-05 10:00:36
Here is a patch that I feel improves the readability of the CAR code a
little bit. What do you all think?

For the record, the intel and amd implementations have the same lists.
These could probably be pulled into a common macro used by all three
implementations.

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.

Signed-off-by: Warren Turkal <wt@penguintechs.org>
---
 src/cpu/via/car/cache_as_ram.inc |   35 ++++++++++++++++++++++++-----------
 1 files changed, 24 insertions(+), 11 deletions(-)
ron minnich - 2010-10-05 15:36:24
On Tue, Oct 5, 2010 at 3:00 AM, Warren Turkal <wt@penguintechs.org> wrote:
> Here is a patch that I feel improves the readability of the CAR code a
> little bit. What do you all think?


I think it's quite nice, as you replace mysterious numbers with a
meaningful name.

ron
Warren Turkal - 2010-10-05 19:49:57
Is that an Ack? If so, I will work on a more serious patch that does
this for all car implementations and moves the lists to a header.

I am thinking of using a macro like MTRR_ADDR_LIST_ASM in the header.
What do you think about that? I don't want it to look like a function
call. What do you think about that?

Thanks,
wt

On Tue, Oct 5, 2010 at 8:36 AM, ron minnich <rminnich@gmail.com> wrote:
> On Tue, Oct 5, 2010 at 3:00 AM, Warren Turkal <wt@penguintechs.org> wrote:
>> Here is a patch that I feel improves the readability of the CAR code a
>> little bit. What do you all think?
>
>
> I think it's quite nice, as you replace mysterious numbers with a
> meaningful name.
>
> ron
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>
ron minnich - 2010-10-05 20:15:36
I think I'm happier if other newer commiters ack this stuff. I don't
work with the code frequently enough to feel comfortable acking some
of these things.

I'm very happy with macros that make data definitions clearer. I'm
very worried about assembly code macros because this is tricky
assembly and people fall into certain habits in terms of thinking
macros are side-effect-free, which they are not in this case.

thanks!

ron
Warren Turkal - 2010-10-05 20:18:57
On Tue, Oct 5, 2010 at 1:15 PM, ron minnich <rminnich@gmail.com> wrote:
> I think I'm happier if other newer commiters ack this stuff. I don't
> work with the code frequently enough to feel comfortable acking some
> of these things.

Fair enough. Anyone else have any comments?

> I'm very happy with macros that make data definitions clearer. I'm
> very worried about assembly code macros because this is tricky
> assembly and people fall into certain habits in terms of thinking
> macros are side-effect-free, which they are not in this case.

Let me throw together a patch and see what you and others think.

Thanks,
wt

Patch

diff --git a/src/cpu/via/car/cache_as_ram.inc b/src/cpu/via/car/cache_as_ram.inc
index d6df4a9..bbd4420 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,17 +60,30 @@  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
+all_mtrr_msrs:
+	/* 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), MTRRphysBase_MSR(1)
+	.long	MTRRphysBase_MSR(2), MTRRphysBase_MSR(3)
+	.long	MTRRphysBase_MSR(4), MTRRphysBase_MSR(5)
+	.long	MTRRphysBase_MSR(6), MTRRphysBase_MSR(7)
+	.long	MTRRphysBase_MSR(8), MTRRphysBase_MSR(9)
+	.long	MTRRphysBase_MSR(10), MTRRphysBase_MSR(11)
+	.long	MTRRphysBase_MSR(12), MTRRphysBase_MSR(13)
+	.long	MTRRphysBase_MSR(14), MTRRphysBase_MSR(15)
+
 	.long	0x000 /* NULL, end of table */
 
 clear_fixed_var_mtrr_out: