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

login
register
mail settings
Submitter Dongsheng Wang
Date Sept. 11, 2013, 5:56 a.m.
Message ID <1378879004-2446-4-git-send-email-dongsheng.wang@freescale.com>
Download mbox | patch
Permalink /patch/274120/
State Superseded
Headers show

Comments

Dongsheng Wang - Sept. 11, 2013, 5:56 a.m.
From: Wang Dongsheng <dongsheng.wang@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

Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
---
--
1.8.0
Scott Wood - Sept. 11, 2013, 11:04 p.m.
On Wed, 2013-09-11 at 13:56 +0800, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@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.

Better yet, this interface should take real time units rather than a
timebase bit.

Also, you disable the power saving mode if the maximum interval is
selected, 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.

> Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> ---
> diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> index 7389d49..7395d79 100644
> --- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> +++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> @@ -53,6 +53,21 @@ _GLOBAL(__e500_dcache_setup)
>  	isync
>  	blr
> 
> +_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?

> +static void query_pwrmgtcr0(void *val)
> +{
> +	u32 *value = val;
> +
> +	*value = mfspr(SPRN_PWRMGTCR0);
> +}
> +
> +static ssize_t show_pw20_state(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	u32 value;
> +	unsigned int cpu = dev->id;
> +
> +	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
> +
> +	value &= PWRMGTCR0_PW20_WAIT;
> +
> +	return sprintf(buf, "%u\n", value ? 1 : 0);
> +}
> +
> +static void control_pw20_state(void *val)
> +{
> +	u32 *value = val;
> +	u32 pw20_state;
> +
> +	pw20_state = mfspr(SPRN_PWRMGTCR0);
> +
> +	if (*value)
> +		pw20_state |= PWRMGTCR0_PW20_WAIT;
> +	else
> +		pw20_state &= ~PWRMGTCR0_PW20_WAIT;
> +
> +	mtspr(SPRN_PWRMGTCR0, pw20_state);
> +}
> +
> +static ssize_t store_pw20_state(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)

The difference between query/show and control/store is a bit awkward...
I'd s/query/do_show/ and s/control/do_store/ (or local_ or other
appropriate prefix).

> +static ssize_t show_altivec_idle_wait_entry_bit(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	u32 value;
> +	unsigned int cpu = dev->id;
> +
> +	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
> +
> +	value = MAX_BIT - ((value & PWRMGTCR0_AV_IDLE_CNT) >>
> +				PWRMGTCR0_AV_IDLE_CNT_SHIFT);
> +
> +	return sprintf(buf, "wait entry bit is %u\n", value);
> +}

Reading a sysfs value should just return the value, not a human-readable
string.

> +static DEVICE_ATTR(pw20_state, 0644, show_pw20_state, store_pw20_state);
> +static DEVICE_ATTR(pw20_wait_entry_bit, 0644, show_pw20_wait_entry_bit,
> +						store_pw20_wait_entry_bit);
> +
> +static DEVICE_ATTR(altivec_idle, 0644, show_altivec_idle, store_altivec_idle);
> +static DEVICE_ATTR(altivec_idle_wait_entry_bit, 0644,
> +			show_altivec_idle_wait_entry_bit,
> +			store_altivec_idle_wait_entry_bit);

I'd make these 0600, given their ability to spam other CPUs with IPIs
even on read.

> +static int __init create_pw20_altivec_sysfs(void)
> +{
> +	int i;
> +	struct device *cpu_dev;
> +
> +	if (!has_pw20_altivec_idle())
> +		return -ENODEV;
> +
> +	for_each_possible_cpu(i) {
> +		cpu_dev = get_cpu_device(i);
> +		device_create_file(cpu_dev, &dev_attr_pw20_state);
> +		device_create_file(cpu_dev, &dev_attr_pw20_wait_entry_bit);
> +
> +		device_create_file(cpu_dev, &dev_attr_altivec_idle);
> +		device_create_file(cpu_dev,
> +					&dev_attr_altivec_idle_wait_entry_bit);
> +	}
> +
> +	return 0;
> +}
> +device_initcall(create_pw20_altivec_sysfs);

I think I said this earlier, but it'd be nice to have a "late cpu setup"
cputable callback -- but failing that, this should be called from
register_cpu_online() which is where other CPU sysfs attributes are
created.

-Scott
Wang Dongsheng-B40534 - Sept. 12, 2013, 3:48 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, September 12, 2013 7:04 AM
> To: Wang Dongsheng-B40534
> Cc: galak@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.wang@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.
 
> 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.

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

> > Signed-off-by: Wang Dongsheng <dongsheng.wang@freescale.com>
> > ---
> > diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> > b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> > index 7389d49..7395d79 100644
> > --- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> > +++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
> > @@ -53,6 +53,21 @@ _GLOBAL(__e500_dcache_setup)
> >  	isync
> >  	blr
> >
> > +_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...
I see that, we can use cpu_spec->pvr_value in c code.

> > +static void query_pwrmgtcr0(void *val) {
> > +	u32 *value = val;
> > +
> > +	*value = mfspr(SPRN_PWRMGTCR0);
> > +}
> > +
> > +static ssize_t show_pw20_state(struct device *dev,
> > +				struct device_attribute *attr, char *buf) {
> > +	u32 value;
> > +	unsigned int cpu = dev->id;
> > +
> > +	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
> > +
> > +	value &= PWRMGTCR0_PW20_WAIT;
> > +
> > +	return sprintf(buf, "%u\n", value ? 1 : 0); }
> > +
> > +static void control_pw20_state(void *val) {
> > +	u32 *value = val;
> > +	u32 pw20_state;
> > +
> > +	pw20_state = mfspr(SPRN_PWRMGTCR0);
> > +
> > +	if (*value)
> > +		pw20_state |= PWRMGTCR0_PW20_WAIT;
> > +	else
> > +		pw20_state &= ~PWRMGTCR0_PW20_WAIT;
> > +
> > +	mtspr(SPRN_PWRMGTCR0, pw20_state);
> > +}
> > +
> > +static ssize_t store_pw20_state(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> 
> The difference between query/show and control/store is a bit awkward...
> I'd s/query/do_show/ and s/control/do_store/ (or local_ or other
> appropriate prefix).
> 
do_show/do_store looks better than others.

> > +static ssize_t show_altivec_idle_wait_entry_bit(struct device *dev,
> > +				struct device_attribute *attr, char *buf) {
> > +	u32 value;
> > +	unsigned int cpu = dev->id;
> > +
> > +	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
> > +
> > +	value = MAX_BIT - ((value & PWRMGTCR0_AV_IDLE_CNT) >>
> > +				PWRMGTCR0_AV_IDLE_CNT_SHIFT);
> > +
> > +	return sprintf(buf, "wait entry bit is %u\n", value); }
> 
> Reading a sysfs value should just return the value, not a human-readable
> string.
> 
Thanks.

> > +static DEVICE_ATTR(pw20_state, 0644, show_pw20_state,
> > +store_pw20_state); static DEVICE_ATTR(pw20_wait_entry_bit, 0644,
> show_pw20_wait_entry_bit,
> > +						store_pw20_wait_entry_bit);
> > +
> > +static DEVICE_ATTR(altivec_idle, 0644, show_altivec_idle,
> > +store_altivec_idle); static DEVICE_ATTR(altivec_idle_wait_entry_bit,
> 0644,
> > +			show_altivec_idle_wait_entry_bit,
> > +			store_altivec_idle_wait_entry_bit);
> 
> I'd make these 0600, given their ability to spam other CPUs with IPIs
> even on read.
> 
Thanks.

> > +static int __init create_pw20_altivec_sysfs(void) {
> > +	int i;
> > +	struct device *cpu_dev;
> > +
> > +	if (!has_pw20_altivec_idle())
> > +		return -ENODEV;
> > +
> > +	for_each_possible_cpu(i) {
> > +		cpu_dev = get_cpu_device(i);
> > +		device_create_file(cpu_dev, &dev_attr_pw20_state);
> > +		device_create_file(cpu_dev, &dev_attr_pw20_wait_entry_bit);
> > +
> > +		device_create_file(cpu_dev, &dev_attr_altivec_idle);
> > +		device_create_file(cpu_dev,
> > +					&dev_attr_altivec_idle_wait_entry_bit);
> > +	}
> > +
> > +	return 0;
> > +}
> > +device_initcall(create_pw20_altivec_sysfs);
> 
> I think I said this earlier, but it'd be nice to have a "late cpu setup"
> cputable callback -- but failing that, this should be called from
> register_cpu_online() which is where other CPU sysfs attributes are
> created.

Yes, thanks. 

-dongsheng
Scott Wood - Sept. 12, 2013, 6:06 p.m.
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@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.wang@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
Wang Dongsheng-B40534 - Sept. 13, 2013, 2:53 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, September 13, 2013 2:07 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; galak@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 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@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.wang@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.

> > > 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. :)
Scott Wood - Sept. 16, 2013, 9:09 p.m.
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@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 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@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.wang@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.

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

-Scott
Wang Dongsheng-B40534 - Sept. 18, 2013, 3:35 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, September 17, 2013 5:09 AM
> To: Wang Dongsheng-B40534
> Cc: Wood Scott-B07421; galak@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 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@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 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@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.wang@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

Patch

diff --git a/arch/powerpc/kernel/cpu_setup_fsl_booke.S b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
index 7389d49..7395d79 100644
--- a/arch/powerpc/kernel/cpu_setup_fsl_booke.S
+++ b/arch/powerpc/kernel/cpu_setup_fsl_booke.S
@@ -53,6 +53,21 @@  _GLOBAL(__e500_dcache_setup)
 	isync
 	blr

+_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
+
 /*
  * FIXME - We don't know, what time should we let the core into PW20 state.
  * because we don't know the current state of the cpu load. And threads are
diff --git a/arch/powerpc/platforms/85xx/common.c b/arch/powerpc/platforms/85xx/common.c
index d0861a0..fe4d3a7 100644
--- a/arch/powerpc/platforms/85xx/common.c
+++ b/arch/powerpc/platforms/85xx/common.c
@@ -5,12 +5,16 @@ 
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
+#include <linux/cpu.h>
 #include <linux/of_platform.h>

 #include <sysdev/cpm2_pic.h>

 #include "mpc85xx.h"

+#define MAX_BIT		63
+
 static struct of_device_id __initdata mpc85xx_common_ids[] = {
 	{ .type = "soc", },
 	{ .compatible = "soc", },
@@ -80,3 +84,234 @@  void __init mpc85xx_cpm2_pic_init(void)
 	irq_set_chained_handler(irq, cpm2_cascade);
 }
 #endif
+
+static void query_pwrmgtcr0(void *val)
+{
+	u32 *value = val;
+
+	*value = mfspr(SPRN_PWRMGTCR0);
+}
+
+static ssize_t show_pw20_state(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
+
+	value &= PWRMGTCR0_PW20_WAIT;
+
+	return sprintf(buf, "%u\n", value ? 1 : 0);
+}
+
+static void control_pw20_state(void *val)
+{
+	u32 *value = val;
+	u32 pw20_state;
+
+	pw20_state = mfspr(SPRN_PWRMGTCR0);
+
+	if (*value)
+		pw20_state |= PWRMGTCR0_PW20_WAIT;
+	else
+		pw20_state &= ~PWRMGTCR0_PW20_WAIT;
+
+	mtspr(SPRN_PWRMGTCR0, pw20_state);
+}
+
+static ssize_t store_pw20_state(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	if (kstrtou32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value > 1)
+		return -EINVAL;
+
+	smp_call_function_single(cpu, control_pw20_state, &value, 1);
+
+	return count;
+}
+
+static ssize_t show_pw20_wait_entry_bit(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
+
+	value = MAX_BIT - ((value & PWRMGTCR0_PW20_ENT) >>
+				PWRMGTCR0_PW20_ENT_SHIFT);
+
+	return sprintf(buf, "wait entry bit is %u\n", value);
+}
+
+static void set_pw20_wait_entry_bit(void *val)
+{
+	u32 *value = val;
+	u32 pw20_idle;
+
+	pw20_idle = mfspr(SPRN_PWRMGTCR0);
+
+	/* Set Automatic PW20 Core Idle Count */
+	/* clear count */
+	pw20_idle &= ~PWRMGTCR0_PW20_ENT;
+
+	/* set count */
+	pw20_idle |= ((MAX_BIT - *value) << PWRMGTCR0_PW20_ENT_SHIFT);
+
+	mtspr(SPRN_PWRMGTCR0, pw20_idle);
+}
+
+static ssize_t store_pw20_wait_entry_bit(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	if (kstrtou32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value > MAX_BIT)
+		return -EINVAL;
+
+	smp_call_function_single(cpu, set_pw20_wait_entry_bit,
+				&value, 1);
+
+	return count;
+}
+
+static ssize_t show_altivec_idle(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
+
+	value &= PWRMGTCR0_AV_IDLE_PD_EN;
+
+	return sprintf(buf, "%u\n", value ? 1 : 0);
+}
+
+static void control_altivec_idle(void *val)
+{
+	u32 *value = val;
+	u32 altivec_idle;
+
+	altivec_idle = mfspr(SPRN_PWRMGTCR0);
+
+	if (*value)
+		altivec_idle |= PWRMGTCR0_AV_IDLE_PD_EN;
+	else
+		altivec_idle &= ~PWRMGTCR0_AV_IDLE_PD_EN;
+
+	mtspr(SPRN_PWRMGTCR0, altivec_idle);
+}
+
+static ssize_t store_altivec_idle(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	if (kstrtou32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value > 1)
+		return -EINVAL;
+
+	smp_call_function_single(cpu, control_altivec_idle, &value, 1);
+
+	return count;
+}
+
+static ssize_t show_altivec_idle_wait_entry_bit(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	smp_call_function_single(cpu, query_pwrmgtcr0, &value, 1);
+
+	value = MAX_BIT - ((value & PWRMGTCR0_AV_IDLE_CNT) >>
+				PWRMGTCR0_AV_IDLE_CNT_SHIFT);
+
+	return sprintf(buf, "wait entry bit is %u\n", value);
+}
+
+static void set_altivec_idle_wait_entry_bit(void *val)
+{
+	u32 *value = val;
+	u32 altivec_idle;
+
+	altivec_idle = mfspr(SPRN_PWRMGTCR0);
+
+	/* Set Automatic AltiVec Idle Count */
+	/* clear count */
+	altivec_idle &= ~PWRMGTCR0_AV_IDLE_CNT;
+
+	/* set count */
+	altivec_idle |=
+		((MAX_BIT - *value) << PWRMGTCR0_AV_IDLE_CNT_SHIFT);
+
+	mtspr(SPRN_PWRMGTCR0, altivec_idle);
+}
+
+static ssize_t store_altivec_idle_wait_entry_bit(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	u32 value;
+	unsigned int cpu = dev->id;
+
+	if (kstrtou32(buf, 0, &value))
+		return -EINVAL;
+
+	if (value > MAX_BIT)
+		return -EINVAL;
+
+	smp_call_function_single(cpu, set_altivec_idle_wait_entry_bit,
+				&value, 1);
+
+	return count;
+}
+
+static DEVICE_ATTR(pw20_state, 0644, show_pw20_state, store_pw20_state);
+static DEVICE_ATTR(pw20_wait_entry_bit, 0644, show_pw20_wait_entry_bit,
+						store_pw20_wait_entry_bit);
+
+static DEVICE_ATTR(altivec_idle, 0644, show_altivec_idle, store_altivec_idle);
+static DEVICE_ATTR(altivec_idle_wait_entry_bit, 0644,
+			show_altivec_idle_wait_entry_bit,
+			store_altivec_idle_wait_entry_bit);
+
+static int __init create_pw20_altivec_sysfs(void)
+{
+	int i;
+	struct device *cpu_dev;
+
+	if (!has_pw20_altivec_idle())
+		return -ENODEV;
+
+	for_each_possible_cpu(i) {
+		cpu_dev = get_cpu_device(i);
+		device_create_file(cpu_dev, &dev_attr_pw20_state);
+		device_create_file(cpu_dev, &dev_attr_pw20_wait_entry_bit);
+
+		device_create_file(cpu_dev, &dev_attr_altivec_idle);
+		device_create_file(cpu_dev,
+					&dev_attr_altivec_idle_wait_entry_bit);
+	}
+
+	return 0;
+}
+device_initcall(create_pw20_altivec_sysfs);
diff --git a/arch/powerpc/platforms/85xx/mpc85xx.h b/arch/powerpc/platforms/85xx/mpc85xx.h
index 2aa7c5d..e454d4d 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx.h
+++ b/arch/powerpc/platforms/85xx/mpc85xx.h
@@ -1,6 +1,7 @@ 
 #ifndef MPC85xx_H
 #define MPC85xx_H
 extern int mpc85xx_common_publish_devices(void);
+extern bool has_pw20_altivec_idle(void);

 #ifdef CONFIG_CPM2
 extern void mpc85xx_cpm2_pic_init(void);