Patchwork AMD cache setup is broken

login
register
about
Submitter Arne Georg Gleditsch
Date 2010-09-13 08:51:08
Message ID <8762yarpqb.fsf@taniquetil.gledits.ch>
Download mbox | patch
Permalink /patch/1921/
State Accepted
Headers show

Comments

Arne Georg Gleditsch - 2010-09-13 08:51:08
"Scott Duplichan" <scott@notabs.org> writes:
> I think it would be best to clear bit 35 of msr c001_102a in the AP
> cores as well as the BSP core. Otherwise, the OS might see AP cores
> having slightly lower performance than the BSP core. This bit affects
> family 10h revC and newer (45 nm).

Ok, so here's a patch adding this.  Clearing bit 35 is done
unconditionally for all fam10 cpus, is that ok?  Setting is done based
on processor type in defaults.h, as before.

Signed-off-by: Arne Georg Gleditsch <arne.gleditsch@numascale.com>
Scott - 2010-09-14 03:15:32
]-----Original Message-----
]From: coreboot-bounces+scott=notabs.org@coreboot.org [mailto:coreboot-]bounces+scott=notabs.org@coreboot.org] On Behalf Of Arne
Georg Gleditsch
]Sent: Monday, September 13, 2010 03:51 AM
]To: Scott Duplichan
]Cc: 'Marc Jones'; coreboot@coreboot.org
]Subject: Re: [coreboot] AMD cache setup is broken
]
]"Scott Duplichan" <scott@notabs.org> writes:
]> I think it would be best to clear bit 35 of msr c001_102a in the AP
]> cores as well as the BSP core. Otherwise, the OS might see AP cores
]> having slightly lower performance than the BSP core. This bit affects
]> family 10h revC and newer (45 nm).
]
]Ok, so here's a patch adding this.  Clearing bit 35 is done
]unconditionally for all fam10 cpus, is that ok?  Setting is done based
]on processor type in defaults.h, as before.

Thanks. This looks correct to me. I used simnow/tilapia to confirm bit
35 gets cleared in all cores. I found bit 35 never actually gets set.
I submitted a patch to correct that. Once that patch is applied, I can
see bit 35 gets set in all cores, then gets cleared in all cores.

Thanks,
Scott

]
]Signed-off-by: Arne Georg Gleditsch <arne.gleditsch@numascale.com>
]
]-- 
]							Arne.
Marc Jones - 2010-09-15 18:32:21
On Mon, Sep 13, 2010 at 9:15 PM, Scott Duplichan <scott@notabs.org> wrote:
> ]-----Original Message-----
> ]From: coreboot-bounces+scott=notabs.org@coreboot.org [mailto:coreboot-]bounces+scott=notabs.org@coreboot.org] On Behalf Of Arne
> Georg Gleditsch
> ]Sent: Monday, September 13, 2010 03:51 AM
> ]To: Scott Duplichan
> ]Cc: 'Marc Jones'; coreboot@coreboot.org
> ]Subject: Re: [coreboot] AMD cache setup is broken
> ]
> ]"Scott Duplichan" <scott@notabs.org> writes:
> ]> I think it would be best to clear bit 35 of msr c001_102a in the AP
> ]> cores as well as the BSP core. Otherwise, the OS might see AP cores
> ]> having slightly lower performance than the BSP core. This bit affects
> ]> family 10h revC and newer (45 nm).
> ]
> ]Ok, so here's a patch adding this.  Clearing bit 35 is done
> ]unconditionally for all fam10 cpus, is that ok?  Setting is done based
> ]on processor type in defaults.h, as before.
>
> Thanks. This looks correct to me. I used simnow/tilapia to confirm bit
> 35 gets cleared in all cores. I found bit 35 never actually gets set.
> I submitted a patch to correct that. Once that patch is applied, I can
> see bit 35 gets set in all cores, then gets cleared in all cores.
>
> Thanks,
> Scott

Hi Scott,

Can you Acked-by: if this is working for you.

Marc
Scott - 2010-09-15 18:51:38
-----Original Message-----
]From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Marc Jones
]Sent: Wednesday, September 15, 2010 01:32 PM
]To: Scott Duplichan
]Cc: Arne Georg Gleditsch; coreboot@coreboot.org
]Subject: Re: [coreboot] AMD cache setup is broken
]
]On Mon, Sep 13, 2010 at 9:15 PM, Scott Duplichan <scott@notabs.org> wrote:
]> ]-----Original Message-----
]> ]From: coreboot-bounces+scott=notabs.org@coreboot.org [mailto:coreboot-]]bounces+scott=notabs.org@coreboot.org] On Behalf Of Arne
]> Georg Gleditsch
]> ]Sent: Monday, September 13, 2010 03:51 AM
]> ]To: Scott Duplichan
]> ]Cc: 'Marc Jones'; coreboot@coreboot.org
]> ]Subject: Re: [coreboot] AMD cache setup is broken
]> ]
]> ]"Scott Duplichan" <scott@notabs.org> writes:
]> ]> I think it would be best to clear bit 35 of msr c001_102a in the AP
]> ]> cores as well as the BSP core. Otherwise, the OS might see AP cores
]> ]> having slightly lower performance than the BSP core. This bit affects
]> ]> family 10h revC and newer (45 nm).
]> ]
]> ]Ok, so here's a patch adding this.  Clearing bit 35 is done
]> ]unconditionally for all fam10 cpus, is that ok?  Setting is done based
]> ]on processor type in defaults.h, as before.
]>
]> Thanks. This looks correct to me. I used simnow/tilapia to confirm bit
]> 35 gets cleared in all cores. I found bit 35 never actually gets set.
]> I submitted a patch to correct that. Once that patch is applied, I can
]> see bit 35 gets set in all cores, then gets cleared in all cores.
]>
]> Thanks,
]> Scott
]
]Hi Scott,
]
]Can you Acked-by: if this is working for you.

Thanks Marc. Here you go..

Acked-by: Scott Duplichan <scott@notabs.org>
<http://www.coreboot.org/pipermail/coreboot/attachments/20100913/5dbed41d/attachment.bin>


]Marc

]-- 
]http://se-eng.com
Marc Jones - 2010-09-17 00:33:10
On Wed, Sep 15, 2010 at 12:51 PM, Scott Duplichan <scott@notabs.org> wrote:
> -----Original Message-----
> ]From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Marc Jones
> ]Sent: Wednesday, September 15, 2010 01:32 PM
> ]To: Scott Duplichan
> ]Cc: Arne Georg Gleditsch; coreboot@coreboot.org
> ]Subject: Re: [coreboot] AMD cache setup is broken
> ]
> ]On Mon, Sep 13, 2010 at 9:15 PM, Scott Duplichan <scott@notabs.org> wrote:
> ]> ]-----Original Message-----
> ]> ]From: coreboot-bounces+scott=notabs.org@coreboot.org [mailto:coreboot-]]bounces+scott=notabs.org@coreboot.org] On Behalf Of Arne
> ]> Georg Gleditsch
> ]> ]Sent: Monday, September 13, 2010 03:51 AM
> ]> ]To: Scott Duplichan
> ]> ]Cc: 'Marc Jones'; coreboot@coreboot.org
> ]> ]Subject: Re: [coreboot] AMD cache setup is broken
> ]> ]
> ]> ]"Scott Duplichan" <scott@notabs.org> writes:
> ]> ]> I think it would be best to clear bit 35 of msr c001_102a in the AP
> ]> ]> cores as well as the BSP core. Otherwise, the OS might see AP cores
> ]> ]> having slightly lower performance than the BSP core. This bit affects
> ]> ]> family 10h revC and newer (45 nm).
> ]> ]
> ]> ]Ok, so here's a patch adding this.  Clearing bit 35 is done
> ]> ]unconditionally for all fam10 cpus, is that ok?  Setting is done based
> ]> ]on processor type in defaults.h, as before.
> ]>
> ]> Thanks. This looks correct to me. I used simnow/tilapia to confirm bit
> ]> 35 gets cleared in all cores. I found bit 35 never actually gets set.
> ]> I submitted a patch to correct that. Once that patch is applied, I can
> ]> see bit 35 gets set in all cores, then gets cleared in all cores.
> ]>
> ]> Thanks,
> ]> Scott
> ]
> ]Hi Scott,
> ]
> ]Can you Acked-by: if this is working for you.
>
> Thanks Marc. Here you go..
>
> Acked-by: Scott Duplichan <scott@notabs.org>

r5817

Patch

diff --git a/src/cpu/amd/model_10xxx/defaults.h b/src/cpu/amd/model_10xxx/defaults.h
index 2fbfbb2..9a0e349 100644
--- a/src/cpu/amd/model_10xxx/defaults.h
+++ b/src/cpu/amd/model_10xxx/defaults.h
@@ -91,7 +91,7 @@  static const struct {
 
         { BU_CFG2, AMD_DRBH_Cx , AMD_PTYPE_ALL,
 	  0x00000000, 1 << (35-32),
-	  0x00000000, 1 << (35-32) },	/* Erratum 343 (set to 0 after CAR, in post_cache_as_ram() )  */  
+	  0x00000000, 1 << (35-32) },	/* Erratum 343 (set to 0 after CAR, in post_cache_as_ram()/model_10xxx_init() )  */  
 };
 
 
diff --git a/src/cpu/amd/model_10xxx/model_10xxx_init.c b/src/cpu/amd/model_10xxx/model_10xxx_init.c
index 6f61fc3..a0e1f6f 100644
--- a/src/cpu/amd/model_10xxx/model_10xxx_init.c
+++ b/src/cpu/amd/model_10xxx/model_10xxx_init.c
@@ -113,9 +113,11 @@  static void model_10xxx_init(device_t dev)
 	msr.hi &= ~(1 << (46 - 32));
 	wrmsr(NB_CFG_MSR, msr);
 
-	/* Clear ClLinesToNbDis */
 	msr = rdmsr(BU_CFG2_MSR);
+	/* Clear ClLinesToNbDis */
 	msr.lo &= ~(1 << 15);
+	/* Clear bit 35 as per Erratum 343 */
+	msr.hi &= ~(1 << (35-32));
 	wrmsr(BU_CFG2_MSR, msr);
 
 	/* Write protect SMM space with SMMLOCK. */