[coreboot] fam10 AP CAR cleanup

Jordan Crouse jordan.crouse at amd.com
Thu Apr 24 18:21:53 CEST 2008


On 24/04/08 10:09 -0600, Marc Jones wrote:
> Peter Stuge wrote:
> > On Wed, Apr 23, 2008 at 05:44:27PM -0600, Marc Jones wrote:
> >   
> >> On APs the ClLinesToNbDis was being left enabled from CAR setup.
> >> Disabling it should help performance.
> >>
> >> Signed-off-by: Marc Jones <marc.jones at amd.com>
> >>     
> >
> > Acked-by: Peter Stuge <peter at stuge.se>
> >
> >
> >   
> >> +	msr_t msr;
> >> +
> >> +	/* Disable L2 IC to L3 connection (Only for CAR) */
> >> +	msr = rdmsr(BU_CFG2);
> >> +	msr.lo &= ~(1 << ClLinesToNbDis);
> >> +	wrmsr(BU_CFG2, msr);
> >>     
> >
> > Maybe add some functions? Or macro?
> >
> > void msr_set(const unsigned index, const msr_t set) {
> >   msr_t msr;
> >   msr=rdmsr(index);
> >   msr.lo|=set.lo;
> >   msr.hi|=set.hi;
> >   wrmsr(index,msr);
> > }
> >
> > void msr_set_lo(const unsigned index, const unsigned set_lo) {
> >   msr_t msr;
> >   msr=rdmsr(index);
> >   msr.lo|=set_lo;
> >   wrmsr(index,msr);
> > }
> >
> > void msr_set_hi(const unsigned index, const unsigned set_hi) {
> >   msr_t msr;
> >   msr=rdmsr(index);
> >   msr.hi|=set_hi;
> >   wrmsr(index,msr);
> > }
> >
> > void msr_clear(const unsigned index, const msr_t clear) {
> >   msr_t msr;
> >   msr=rdmsr(index);
> >   msr.lo&=~clear.lo;
> >   msr.hi&=~clear.hi;
> >   wrmsr(index,msr);
> > }
> >
> > void msr_clear_lo(const unsigned index, const unsigned clear_lo) {
> >   msr_t msr;
> >   msr=rdmsr(index);
> >   msr.lo&=~clear_lo;
> >   wrmsr(index,msr);
> > }
> >
> > void msr_clear_hi(const unsigned index, const unsigned clear_hi) {
> >   msr_t msr;
> >   msr=rdmsr(index);
> >   msr.hi|=~clear_hi;
> >   wrmsr(index,msr);
> > }
> >
> > I can make a patch, msr.h right?
> >
> >
> > //Peter
> >
> >   
> Personally, I prefer seeing the & and | values inline so I know exactly 
> what happened as I read the code. Not having to remember if the function 
> ~ the mask, BUT I could be convinced that the functions are the right 
> way to go. I think msr.h would be the correct place. Would you do a 
> massive search and replace on the entire v2 tree? That would be a lot of 
> changes. Maybe start in the v3 tree?

I agree with Marc - the readmsr / modify / writemsr cycle inline is far
easier to understand and debug.  See the Geode X driver code for multiple
examples of MSR functions gone silly. 

Jordan

-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.





More information about the coreboot mailing list