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

[Wang Dongsheng-B40534]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, September 17, 2013 5:09 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; 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 Thu, 2013-09-12 at 21:53 -0500, Wang Dongsheng-B40534 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Friday, September 13, 2013 2:07 AM
> > > To: Wang Dongsheng-B40534
> > > Cc: Wood Scott-B07421; 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 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.
> > >
> > But there has a problem, we can't convert bit to the real time when
> user read this sysfs.
> > Like:
> > echo 1000(us) > /sys/*/pw20_wait_entry_bit, after convert we get bit is
> 49.
> > cat /sys/*/pw20_wait_entry_bit, after convert the time is 1598(us).
> >
> > The read out of the time is not real time. Unless we define a variable
> to save the real time.
> 
> It's not the end of the world if the value is different when read back.
> It just gets rounded up when you write it.
> 
Ok, make a variable to save it.

> > > > > 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).
> > >
> > There has pw20_state/altivec_idle sys interface to control
> > "enable/disable", There is only to control wait bit. Did you mean
> remove "pw20_state/altivec_idle"
> > sys interface, and reuse "pw20_wait_entry_bit/altivec_idle*" sys
> interface?
> > When echo zero into "pw20_wait_entry_bit" we just to disable pw20
> > state, I think that is reasonable. :)
> 
> Sorry, I misread the patch and didn't realize these were separate
> interfaces.
Keep the "pw20_state/altivec_idle" interfaces.

-dongsheng