[coreboot] [PATCH 4/6] Geode GX2 cleanup patch
njacobs8 at hetnet.nl
Mon Dec 27 14:56:42 CET 2010
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
>> 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
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?
More information about the coreboot