[coreboot] [PATCH] Convert Geode GX2 boards to CAR

njacobs8 at hetnet.nl njacobs8 at hetnet.nl
Mon Jun 28 14:06:23 CEST 2010


Hi Peter,
Thanks for the super fast review!

You wrote:
>> +++ src/southbridge/amd/cs5535/cs5535_early_setup.c   (working copy)
>..
>> +static void cs5535_setup_iobase(void)
>>  {
>>       msr_t msr;
>> +     /* setup LBAR for SMBus controller */
>> +     msr.hi = 0x0000f001;
>> +     msr.lo = SMBUS_IO_BASE;
>> +     wrmsr(MDD_LBAR_SMB, msr);
>
>I'd really like if these weren't just #defines, but configured on a
>slightly higher level. Maybe devicetree.cb - is there something there
>that could fit well?
???? I don`t know :) , i adapted the code to be simmilar to cs5536.
The reason is that GCC complained about "__builtin_wrmsr".
The data writen to the MSR`s is not changed. 
Maybe you could send a follow up patch to change CS5535 and CS5536 behavior?
I`l be glad to test it.

>> -static void cs5535_setup_power_bottun(void)
>> -{
>>       /* not implemented yet */
>>  #if 0
>> +static void cs5535_setup_power_button(void)
>> +{
>
>If noone is using it then maybe drop the code? Why #if 0 anyway,
>linker should just not include it if not called?
>
>I know that Ron needed some power button code for ALIX.1C.
I got a GCC warning about "unused fuction" or so.
The Alix.1C should not be affected because it uses the CS5536.
Should i remove it entirely?(i would like that)

>> +++ src/southbridge/amd/cs5535/cs5535_smbus.h (working copy)
>> @@ -45,6 +45,7 @@
>  
>>  #define SMBUS_IO_BASE 0x6000
>  
>> +#if 0
>>  static void smbus_delay(void)
>
>Same here, why keep it?
I don`t know.
Should i remove it entirely?

>> +++ src/southbridge/amd/cs5535/cs5535_early_smbus.c   (working copy)
>..
>> @@ -21,12 +21,12 @@
>>       outb(val, SMBUS_IO_BASE + SMB_ADD);
>>  }
>  
>> +#if 0
>>  static int smbus_read_byte(unsigned device, unsigned address)
>>  {
>>          return do_smbus_read_byte(SMBUS_IO_BASE, device, address-1);
>>  }
>  
>> -#if 0
>>  static int smbus_recv_byte(unsigned device)
>>  {
>
>And here?
I don`t know.
Should i remove it entirely?

>> +++ src/cpu/amd/model_gx2/Makefile.inc        (working copy)
>..
>> \ No newline at end of file
>
>Please add one.
OK.

>> +++ src/cpu/amd/model_gx2/cpureginit.c        (working copy)
>..
>> -BIST(void){
>
>Hmm? I guess report_bist_failure(bist); does some of the same, but I
>guess it does not check the MSRs?
I don`t know.
I got a GCC warning about "unused fuction" or so.
I addapted the code to match Geode LX witch doesn`t seem to use it.

>> +void cpuRegInit (void)
>..
>> -     /*if (getnvram( TOKEN_BIST_ENABLE) & == TVALUE_DISABLE) {*/
> -     {
> -//           BIST();
> -     }
>
>Hehe, it was never called. Do you know why? Is the code wrong?
I don`t know.
Ron disabled it the beginning in r2249 (04-10-06) and commented it: "add cpureginit to romcc code".
Maybe Ron can explain it? 

>> -MTestPinCheckBX (void){
>
>Also never called? Ok.
Exact.

>> +++ src/cpu/amd/model_gx2/cache_as_ram.inc    (revision 0)
>..
>> +     post_code(0xc5)
>> +DCacheSetupBad:
>> +     hlt             /* issues */
>> +     jmp DCacheSetupBad
>
>Should this maybe fail more loudly than a POST code, which can be
>disbled completely. Would be nice to say something on serial.
I would like that but that`s how it is in Geode LX.
I try if can do a follow up patch for that after this is in. OK?

>> +++ src/mainboard/wyse/s50/romstage.c (working copy)
>..
>> -static void msr_init(void)
>> +void main(unsigned long bist)
>>  {
>> -     /* Setup access to cache under 1MB.
>> -     __builtin_wrmsr(CPU_RCONF_DEFAULT,  0x1000a000, 0x24fffc02); /* Rom Properties: Write Serialize, WriteProtect.
>> -                                                                   * RomBase: 0xFFFC0
..
>> -     __builtin_wrmsr(MSR_GLIU1_SHADOW, 0xffff0003, 0x2000ffff); /*   0xC0000-0xFFFFF */
>> -
>> -     /* put code in northbridge[init].c here */
>> -}
>
>Where is this setup done now? Was it identical for all boards?
This is now still done in "msr_init" only it now lives in +#include "cpu/amd/model_lx/msrinit.c" just like for LX.
I think all boards should have the same (working)ram region setup.
I am sure the Rumba,btest and rev_a boards, and presumably Frontrunner also, don`t boot at the current state.

Thanks,Nils
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20100628/46642855/attachment.html>


More information about the coreboot mailing list