[patch][v2] fam10 AP CAR cleanup

[Peter Stuge]

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 <[e-mail]>

Acked-by: Peter Stuge <[e-mail]>
> +	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

[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 <[e-mail]>
>
> Acked-by: Peter Stuge <[e-mail]>
> > +	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


-- 
Marc Jones
Senior Firmware Engineer
[phone] Office
mailto:[e-mail]
http://www.amd.com/embeddedprocessors

fam10 AP CAR cleanup

[Jordan Crouse]

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 <[e-mail]>
> >
> > Acked-by: Peter Stuge <[e-mail]>
> > > +	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.

fam10 AP CAR cleanup

[ron minnich]

On Thu, Apr 24, 2008 at 9:21 AM, Jordan Crouse <[e-mail]> wrote:
> On 24/04/08 10:09 -0600, Marc Jones wrote:

> > 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.

I'm with Jordan and Marc on this one.

Also, we might want to stop dropping code in .h files. Unless you've
really measured it, we can not assume that inline/macro is somehow
always better than a function call. People have gone out of control
with inlines in other projects :-)

thanks

ron

[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 <[e-mail]>
>
> Acked-by: Peter Stuge <[e-mail]>

r3262
Thanks,
Marc


-- 
Marc Jones
Senior Firmware Engineer
[phone] Office
mailto:[e-mail]
http://www.amd.com/embeddedprocessors

msr_set(), msr_clear() and friends

[Peter Stuge]

On Thu, Apr 24, 2008 at 10:09:38AM -0600, Marc Jones wrote:
> Peter Stuge wrote:
> > Maybe add some functions? Or macro?
> >
> > void msr_set(const unsigned index, const msr_t set) {
> > void msr_set_lo(const unsigned index, const unsigned set_lo) {
> > void msr_set_hi(const unsigned index, const unsigned set_hi) {
> >
> > void msr_clear(const unsigned index, const msr_t clear) {
> > void msr_clear_lo(const unsigned index, const unsigned clear_lo) {
> > void msr_clear_hi(const unsigned index, const unsigned clear_hi) {
> >
> > I can make a patch, msr.h right?
>
> 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,

_clear() would clear bits set both in hardware MSR and the input MSR.

My motivation is to make code easier and thus faster to read and
write by using function names and operations that are easily
understood in a spoken language. (set and clear in this case)

No doubt anyone using them must still understand what &= and |= do,
but there's no point in reading or writing more code than is
neccessary IMO. (Not everyone agrees.)
> BUT I could be convinced that the functions are the right way to
> go.

Great! :)
> I think msr.h would be the correct place.

Hm.
> Would you do a massive search and replace on the entire v2 tree?
> That would be a lot of changes.

But a mighty good one I think. :)
> Maybe start in the v3 tree?

Yep, that's an excellent idea.


On Thu, Apr 24, 2008 at 10:21:53AM -0600, Jordan Crouse wrote:
> I agree with Marc - the readmsr / modify / writemsr cycle inline is
> far easier to understand and debug.

Even though it is 3+ lines per operation and always requires a
local variable in whatever scope one might be when wanting to
touch an MSR? :\

Maybe you're jaded by now(?) but I really like putting common things
like this (and this is quite common, plus I expect it to become even
more common in the future) in a single place.

Another argument is that it eliminates the risk of a missing & or |
causing problems. And did I mention that less typing is needed?
> See the Geode X driver code for multiple examples of MSR functions
> gone silly.

I've never seen the code before, grep -ri msr returns every single
line of code in the driver (no, but many) and {cim/cim,geode}_msr.c
actually looked pretty good to me. Where are the mistakes so I can
learn from them?


On Thu, Apr 24, 2008 at 09:26:36AM -0700, ron minnich wrote:
> I'm with Jordan and Marc on this one.

I seem to be going uphill, but I do think this is, and will be,
important.

If everyone will stand fast I will not waste time, but I would like
the change.

Maybe v3 is the perfect sandbox?
> Also, we might want to stop dropping code in .h files.

Yes, agreed. What is a better place? Create arch/i386/lib/msr.c ?
> Unless you've really measured it, we can not assume that
> inline/macro is somehow always better than a function call.
> People have gone out of control with inlines in other projects :-)

I have believed that the inline keyword means that the compiler will
not make a function of the code but will replicate the function
contents at every call site. Conversely I assumed that functions
were not replicated otherwise.

Seems I am already old school. :p

I prefer functions over macros, but on rare occasions I've done nice
syntax tricks in macros that just were not possible with functions.
Not neccessary here though, so I favor functions and let others
decide if they should be inline or not.


//Peter