[coreboot] [patch][v2] AMD Fam10 rev B3 CPU

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> On 22.07.2008 01:02, Marc Jones wrote:
>> Add AMD Fam10 B3 default settings to match AMD example code.
>> Includes setting for most resent errata.
>>
>> Signed-off-by: Marc Jones <marc.jones at amd.com>
>>
>>  	{ 3, 0x44, AMD_DR_ALL, AMD_PTYPE_ALL,
>> -	  0x0A100044, 0x0A300044 },	/* [27] NB MCA to CPU0 Enable,
>> -					   [25] DisPciCfgCpuErrRsp,
>> -					   [21] SyncOnErr=0,
>> +	  0x4A30005C, 0x4A30005C },	/* [30] SyncOnDramAdrParErrEn=1,
>> +					   [27] NbMcaToMstCpuEn=1,
>> +					   [25] DisPciCfgCpuErrRsp=1,
>> +					   [21] SyncOnAnyErrEn=1,
>>  					   [20] SyncOnWDTEn=1,
>> -					   [6] CpuErrDis,
>> +					   [6] CpuErrDis=1,
>> +					   [4] SyncPktPropDis=1,
>> +					   [3] SyncPktGenDis=1,
>>  					   [2] SyncOnUcEccEn=1 */
>>   
> 
> This is not a complaint about the patch, but I'd like to understand why
> the value pair had internal differences before (0x0A100044, 0x0A300044)
> and is identical now. The comments didn't make the old situation so
> obvious, however the new situation conforms nciely with the comments.
> 

The second value is a mask, so it was clearing SyncOnAnyErrEn and not 
setting it. I think it was intentional at the time.


>>  
>>  	/* Extended NB MCA Config Register */
>>  	{ 3, 0x180, AMD_DR_ALL, AMD_PTYPE_ALL,
>> -	  0x00700022, 0x00700022 },	/* [5]     = DisPciCfgCpuMstAbtRsp
>> -					   [22:20] = SyncFloodOn_Err = 7,
>> -					   [1] = SyncFloodOnUsPwDataErr = 1 */
>> +	  0x007003E2, 0x007003E2 },	/* [22:20] = SyncFloodOn_Err = 7,
>> +					   [9] SyncOnUncNbAryEn = 1 ,
>> +					   [8] SyncOnProtEn = 1,
>> +					   [7] SyncFloodOnTgtAbtErr] = 1,
>> +					   [6] SyncFloodOnDatErr] = 1,
>>   
> 
> Both lines above have a superfluous closing square bracket.
> 

fixed.

>> +					   [5] DisPciCfgCpuMstAbtRsp=1,
>> +					   [1] SyncFloodOnUsPwDataErr = 1 */
>>   
> 
> The style of the datasheet-referencing comments is not entirely
> consistent: Sometimes it's "a=1", sometimes "a = 1". I don't care about
> this, but Uwe may.
> 

fixed.

>>  
>>  	/* L3 Control Register */
>>  	{ 3, 0x1B8, AMD_DR_ALL, AMD_PTYPE_ALL,
>> Index: coreboot-v2/src/cpu/amd/model_10xxx/init_cpus.c
>> ===================================================================
>> --- coreboot-v2.orig/src/cpu/amd/model_10xxx/init_cpus.c	2008-07-21 15:39:58.000000000 -0600
>> +++ coreboot-v2/src/cpu/amd/model_10xxx/init_cpus.c	2008-07-21 16:45:47.000000000 -0600
>> @@ -669,6 +669,35 @@
>>  }
>>  
>>  
>> +void AMD_SetupPSIVID_d (u32 platform_type, u8 node)
>> +{
>> +	u32 dword;
>> +	int i;
>> +	msr_t msr;
>> +
>> +	if (platform_type & (AMD_PTYPE_MOB | AMD_PTYPE_DSK)) {
>> +
>> +	/* The following code sets the PSIVID to the highest supported P state
>> +	 * assuming that the VID for the highest power state is below
>> +	 * the VDD voltage regulator threshold. (This also assumes that there
>> +	 * is a Pstate higher than P0)
>> +	 */
>> +
>> +		for( i = 4; i >= 0; i--) {
>> +			msr = rdmsr(0xC0010064 + i);
>>   
> 
> Having a #define for 0xC0010064 would be nice.

used #define PS_REG_BASE
> 
>> +			/*  Pstate valid? */
>> +			if ((msr.hi >> 31) & 1) {
>>   
> 
> I guess this is personal preference, but I'd like to suggest an alternative:
> 
> #define MSR_FOO_PSTATE_VALID	(1 << 31)
> 			if (msr.hi & MSR_FOO_PSTATE_VALID) {
> 
> 
> 

There was a define PS_EN_MASK



>>  	for(i = 0; i < sizeof(fam10_pci_default)/sizeof(fam10_pci_default[0]); i++) {
>>   
> 
> I admit this was not part of your current patch, but please use the
> ARRAY_SIZE() macro for such calculations.
> 

This is done in a few places and could use a separate patch to clean 
them all up.



>>  		if ((fam10_pci_default[i].revision & revision) &&
>>  		    (fam10_pci_default[i].platform & platform)) {
>>   
>>   
> 
> With the comments above answered or addressed (I trust your judgement):
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

r 3435

thanks



-- 
Marc Jones
Senior Firmware Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors