Patchwork [try,4] Reduce duplicate definition in CAR code.

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(-)

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

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

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
>