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

[Wang Dongsheng-B40534]

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