<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=iso-8859-1">
<META NAME="Generator" CONTENT="MS Exchange Server version 6.5.7654.12">
<TITLE>Re: [coreboot] [PATCH] Convert Geode GX2 boards to CAR</TITLE>
</HEAD>
<BODY>
<!-- Converted from text/plain format -->

<P><FONT SIZE=2>Hi Peter,<BR>
Thanks for the super fast review!<BR>
<BR>
You wrote:<BR>
>> +++ src/southbridge/amd/cs5535/cs5535_early_setup.c   (working copy)<BR>
>..<BR>
>> +static void cs5535_setup_iobase(void)<BR>
>>  {<BR>
>>       msr_t msr;<BR>
>> +     /* setup LBAR for SMBus controller */<BR>
>> +     msr.hi = 0x0000f001;<BR>
>> +     msr.lo = SMBUS_IO_BASE;<BR>
>> +     wrmsr(MDD_LBAR_SMB, msr);<BR>
><BR>
>I'd really like if these weren't just #defines, but configured on a<BR>
>slightly higher level. Maybe devicetree.cb - is there something there<BR>
>that could fit well?<BR>
???? I don`t know :) , i adapted the code to be simmilar to cs5536.<BR>
The reason is that GCC complained about "__builtin_wrmsr".<BR>
The data writen to the MSR`s is not changed.<BR>
Maybe you could send a follow up patch to change CS5535 and CS5536 behavior?<BR>
I`l be glad to test it.<BR>
<BR>
>> -static void cs5535_setup_power_bottun(void)<BR>
>> -{<BR>
>>       /* not implemented yet */<BR>
>>  #if 0<BR>
>> +static void cs5535_setup_power_button(void)<BR>
>> +{<BR>
><BR>
>If noone is using it then maybe drop the code? Why #if 0 anyway,<BR>
>linker should just not include it if not called?<BR>
><BR>
>I know that Ron needed some power button code for ALIX.1C.<BR>
I got a GCC warning about "unused fuction" or so.<BR>
The Alix.1C should not be affected because it uses the CS5536.<BR>
Should i remove it entirely?(i would like that)<BR>
<BR>
>> +++ src/southbridge/amd/cs5535/cs5535_smbus.h (working copy)<BR>
>> @@ -45,6 +45,7 @@<BR>
> <BR>
>>  #define SMBUS_IO_BASE 0x6000<BR>
> <BR>
>> +#if 0<BR>
>>  static void smbus_delay(void)<BR>
><BR>
>Same here, why keep it?<BR>
I don`t know.<BR>
Should i remove it entirely?<BR>
<BR>
>> +++ src/southbridge/amd/cs5535/cs5535_early_smbus.c   (working copy)<BR>
>..<BR>
>> @@ -21,12 +21,12 @@<BR>
>>       outb(val, SMBUS_IO_BASE + SMB_ADD);<BR>
>>  }<BR>
> <BR>
>> +#if 0<BR>
>>  static int smbus_read_byte(unsigned device, unsigned address)<BR>
>>  {<BR>
>>          return do_smbus_read_byte(SMBUS_IO_BASE, device, address-1);<BR>
>>  }<BR>
> <BR>
>> -#if 0<BR>
>>  static int smbus_recv_byte(unsigned device)<BR>
>>  {<BR>
><BR>
>And here?<BR>
I don`t know.<BR>
Should i remove it entirely?<BR>
<BR>
>> +++ src/cpu/amd/model_gx2/Makefile.inc        (working copy)<BR>
>..<BR>
>> \ No newline at end of file<BR>
><BR>
>Please add one.<BR>
OK.<BR>
<BR>
>> +++ src/cpu/amd/model_gx2/cpureginit.c        (working copy)<BR>
>..<BR>
>> -BIST(void){<BR>
><BR>
>Hmm? I guess report_bist_failure(bist); does some of the same, but I<BR>
>guess it does not check the MSRs?<BR>
I don`t know.<BR>
I got a GCC warning about "unused fuction" or so.<BR>
I addapted the code to match Geode LX witch doesn`t seem to use it.<BR>
<BR>
>> +void cpuRegInit (void)<BR>
>..<BR>
>> -     /*if (getnvram( TOKEN_BIST_ENABLE) & == TVALUE_DISABLE) {*/<BR>
> -     {<BR>
> -//           BIST();<BR>
> -     }<BR>
><BR>
>Hehe, it was never called. Do you know why? Is the code wrong?<BR>
I don`t know.<BR>
Ron disabled it the beginning in r2249 (04-10-06) and commented it: "add cpureginit to romcc code".<BR>
Maybe Ron can explain it?<BR>
<BR>
>> -MTestPinCheckBX (void){<BR>
><BR>
>Also never called? Ok.<BR>
Exact.<BR>
<BR>
>> +++ src/cpu/amd/model_gx2/cache_as_ram.inc    (revision 0)<BR>
>..<BR>
>> +     post_code(0xc5)<BR>
>> +DCacheSetupBad:<BR>
>> +     hlt             /* issues */<BR>
>> +     jmp DCacheSetupBad<BR>
><BR>
>Should this maybe fail more loudly than a POST code, which can be<BR>
>disbled completely. Would be nice to say something on serial.<BR>
I would like that but that`s how it is in Geode LX.<BR>
I try if can do a follow up patch for that after this is in. OK?<BR>
<BR>
>> +++ src/mainboard/wyse/s50/romstage.c (working copy)<BR>
>..<BR>
>> -static void msr_init(void)<BR>
>> +void main(unsigned long bist)<BR>
>>  {<BR>
>> -     /* Setup access to cache under 1MB.<BR>
>> -     __builtin_wrmsr(CPU_RCONF_DEFAULT,  0x1000a000, 0x24fffc02); /* Rom Properties: Write Serialize, WriteProtect.<BR>
>> -                                                                   * RomBase: 0xFFFC0<BR>
..<BR>
>> -     __builtin_wrmsr(MSR_GLIU1_SHADOW, 0xffff0003, 0x2000ffff); /*   0xC0000-0xFFFFF */<BR>
>> -<BR>
>> -     /* put code in northbridge[init].c here */<BR>
>> -}<BR>
><BR>
>Where is this setup done now? Was it identical for all boards?<BR>
This is now still done in "msr_init" only it now lives in +#include "cpu/amd/model_lx/msrinit.c" just like for LX.<BR>
I think all boards should have the same (working)ram region setup.<BR>
I am sure the Rumba,btest and rev_a boards, and presumably Frontrunner also, don`t boot at the current state.<BR>
<BR>
Thanks,Nils</FONT>
</P>

</BODY>
</HTML>