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: ga...@kernel.crashing.org; linuxppc-dev@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.w...@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@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 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
