[patch][v2] AMD Fam10 rev B3 CPU

[Marc Jones]

Update default CPU settings for B3

Marc

[Paul Menzel]

Dear Marc,

just nitpicking.

Am Montag, den 21.07.2008, 17:02 -0600 schrieb Marc Jones:
> Add AMD Fam10 B3 default settings to match AMD example code.
> Includes setting for most resent errata.

s/resent/recent/ ?
> Signed-off-by: Marc Jones

Sorry, I have no idea about the code and therefore somebody else will
have to ack your patch.

Thanks,

Paul

[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

[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
> >
> > { 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

r 3435

thanks