[coreboot] [PATCH 4/6] Geode GX2 cleanup patch

[Nils]

Hi Peter.
Thanks for the thorough review!

As this patches are part from a half year old patch of mine and i did not 
remember everything in detail i spend a few hours to study these confusing 
registers again.

Peter wrote:
>> Remove wrong GX2 processor IIOC mode setting on CS5535 southbridge
>> code and fix CIS mode comments.
>
>Hm, please talk a little about this?
>
>> +++ src/southbridge/amd/cs5535/early_setup.c  (working copy)
>> @@ -107,15 +107,11 @@
>..
>> -     //Only do this if we are building for 5535
>> -     msr.lo = 0x2;
>> -     msr.hi = 0x0;
>> -     wrmsr(VIP_GIO_MSR_SEL, msr);
>
>This code is not added to another place by this patch. Is it simply
>completely bogus, but harmless, for 5535? And harmful for GX2? Please
>explain a little?

I removed this code because it is duplicating code.
This register is  set in cpureginit and does not have to be set here.
This duplication is already removed  from the newer CS5536 code in rev2631.
 
>
>> +++ src/include/cpu/amd/gx2def.h      (working copy)
>..
>> +#define      FG_GIO_MSR_SEL          (MSR_FG + 0x2010)
>..
>
>> -#define VIP_GIO_MSR_SEL              (MSR_VIP + 0x2010)
>
>(Why remove this?)

This is part of a change that i made because i think it is confusing that 
GLIU1 port 5 has two names in gx2def.h .(VIP and FooGlue FG)
The GX2 does not have a VIP Video Input Port.
The gx2def.h is clobbered with LX registernames from before it was splitup in 
a seperate LX tree.(see also AES registers witch the GX2 does not have)
The rest of the conversion to one name for the other GLIU1 port 5 registers
are in a follow up patch that i didn't send yet.
(This splitting up work is a major pain in the ....)

>
>> +++ src/cpu/amd/model_gx2/cpureginit.c        (working copy)
>..
>> -     msrnum = MSR_FG + 0x10;
>..
>> +     msrnum = FG_GIO_MSR_SEL;
>
>This is not the same MSR. Please explain? 

That is good observed. :)
Register MSR_FG + 0x10 (54000010) does not exist according to the datasheet.
Register address 54000010 and 54002010 are mapped to the same physical 
register.
So i think it is less confusing to use the documented register address.
On top of this confusion the register values in the databook are wrong.
This is stated in a OFW comment.
I will send an updated patch with this extra comment for future readers.

>FG_GIO_MSR_SEL is defined
>to MSR_FG + 0x2010 above, so this particular change changes which MSR
>is being accessed. Is on purpose?

Yes.(see above)


Thanks, Nils.