Re: [coreboot] [patch][v2] fam10 AP CAR cleanup

[Marc Jones]

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
>
> Acked-by: Peter Stuge
> > 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?

Marc