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