[PATCH v3 4/4] powerpc/85xx: add sysfs for pw20 state and altivec idle

[Scott Wood]

On Wed, 2013-09-11 at 22:48 -0500, Wang Dongsheng-B40534 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, September 12, 2013 7:04 AM
> > To: Wang Dongsheng-B40534
> > Cc: galak at kernel.crashing.org; linuxppc-dev at lists.ozlabs.org
> > Subject: Re: [PATCH v3 4/4] powerpc/85xx: add sysfs for pw20 state and
> > altivec idle
> > 
> > On Wed, 2013-09-11 at 13:56 +0800, Dongsheng Wang wrote:
> > > From: Wang Dongsheng <dongsheng.wang at freescale.com>
> > >
> > > Add a sys interface to enable/diable pw20 state or altivec idle, and
> > > control the wait entry time.
> > >
> > > Enable/Disable interface:
> > > 0, disable. 1, enable.
> > > /sys/devices/system/cpu/cpuX/pw20_state
> > > /sys/devices/system/cpu/cpuX/altivec_idle
> > >
> > > Set wait entry bit interface:
> > > bit value range 0~63, 0 bit is Mintime, 63 bit is Maxtime.
> > > /sys/devices/system/cpu/cpuX/pw20_wait_entry_bit
> > > /sys/devices/system/cpu/cpuX/altivec_idle_wait_entry_bit
> > 
> > I'm no fan of the way powerpc does bit numbering, but don't flip it
> > around here -- you'll just cause confusion.
> > 
> OK. 0 bit is maxtime, 63 bit is mintime.
> 
> > Better yet, this interface should take real time units rather than a
> > timebase bit.
> > 
> I think the real time is not suitable, because timebase bit does not correspond with
> real time.

It's a bit sloppy due to how the hardware works, but you could convert
it like you did in earlier patches.  Semantically it should probably be
the minimum time to wait before entering the low power state.
 
> > Also, you disable the power saving mode if the maximum interval is
> > selected, 
> It's not disable the pw20 state or altivec idle, just max-delay entry time.

No, the code checks for zero to set or clear the enabling bit (e.g.
PW20_WAIT).

> >but the documentation doesn't say that -- and the documentation
> > should be in the code (or other easily findable place), not just in the
> > commit message.
> > 
> Also add a comment in the 85xx/common.c ?

Yes.

> > > +_GLOBAL(has_pw20_altivec_idle)
> > > +	/* 0 false, 1 true */
> > > +	li	r3, 0
> > > +
> > > +	/* PW20 & AltiVec idle feature only exists for E6500 */
> > > +	mfspr	r0, SPRN_PVR
> > > +	rlwinm	r4, r0, 16, 16, 31
> > > +	lis	r12, 0
> > > +	ori	r12, r12, PVR_VER_E6500 at l
> > > +	cmpw	r4, r12
> > > +	bne	2f
> > > +	li	r3, 1
> > > +2:
> > > +	blr
> > 
> > Why is this in asm?  And shouldn't this go in the cputable somehow?
> > 
> Not a special reason for this, just asm...

Asm shouldn't be used without a good reason.

-Scott