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

[Carl-Daniel Hailfinger]

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
> Index: coreboot-v2/src/cpu/amd/model_10xxx/defaults.h
>
> --- coreboot-v2.orig/src/cpu/amd/model_10xxx/defaults.h	2008-07-21 15:39:58.000000000 -0600
> +++ coreboot-v2/src/cpu/amd/model_10xxx/defaults.h	2008-07-21 16:14:24.000000000 -0600
> @@ -46,10 +46,6 @@
>      0xF << 19, 0x00000000,
>      0xF << 19, 0x00000000 },	/* [RtryHt[0..3]]=1 */
> { MC4_CTL_MASK, AMD_DR_ALL, AMD_PTYPE_ALL,
>  0x1 << 10, 0x00000000,
>  0x1 << 10, 0x00000000 },	/* [GartTblWkEn]=1 */
> { DC_CFG, AMD_DR_ALL, AMD_PTYPE_SVR,
>    0x00000000, 0x00000004,
>    0x00000000, 0x0000000C },	/* [REQ_CTR] = 1 for Server */
>
> @@ -68,7 +64,7 @@
>
> { DC_CFG, AMD_DR_ALL, AMD_PTYPE_ALL,
>      1 << 24, 0x00000000,
>  1 << 24, 0x00000000 },	/* Erratum #202 [DIS_PIGGY_BACK_SCRUB]=1 */
>  1 << 24, 0x00000000 },	/* Erratum #261 [DIS_PIGGY_BACK_SCRUB]=1 */
>
> { LS_CFG, AMD_DR_GT_B0, AMD_PTYPE_ALL,
>    0 << 1, 0x00000000,
>
> @@ -160,11 +156,14 @@
>      0x00000100, 0x00000100 },	/* [8] MstrAbrtEn */
>
> { 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.
> /* XBAR buffer settings */
> @@ -222,7 +221,7 @@
>    { 3, 0x84, AMD_DR_ALL, AMD_PTYPE_ALL,
>      0xA0E641E6, 0xFFFFFFFF },
> { 3, 0xA0, AMD_DR_ALL, AMD_PTYPE_MOB,
>
> { 3, 0xA0, AMD_DR_ALL, AMD_PTYPE_MOB | AMD_PTYPE_DSK,
> 0x00000080, 0x00000080 },	/* [7] PSIVidEnable */
>
> { 3, 0xA0, AMD_DR_ALL, AMD_PTYPE_ALL,
>
> @@ -250,9 +249,13 @@
>
> /* 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.
> 			   [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.
> /* 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.
> 	/*  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) {
> 		dword = pci_read_config32(NODE_PCI(i,3), 0xA0);
> 		dword &= ~0x7F;
> 		dword |= (msr.lo >> 9) & 0x7F;
> 		pci_write_config32(NODE_PCI(i,3), 0xA0, dword);
> 		break;
> 	}
> }
> }
>
> +}
>
> /**
> AMD_CpuFindCapability - Traverse PCI capability list to find host HT links.
> HT Phy operations are not valid on links that aren't present, so this
>
> @@ -857,6 +886,8 @@
>    revision = mctGetLogicalCPUID(node);
>    platform = get_platform_type();
> AMD_SetupPSIVID_d(platform, node);	/* Set PSIVID offset which is not table driven */
> 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.
> 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

Regards,
Carl-Daniel