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