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

Wed Nov 6 16:25:06 EST 2013

> -----Original Message-----
> From: Wang Dongsheng-B40534
> Sent: Tuesday, November 05, 2013 8:40 AM
> To: Wood Scott-B07421
> Cc: Bhushan Bharat-R65777; linuxppc-dev at lists.ozlabs.org
> Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20 state and altivec
> idle
> 
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, November 05, 2013 5:52 AM
> > To: Wang Dongsheng-B40534
> > Cc: Wood Scott-B07421; Bhushan Bharat-R65777; linuxppc-
> > dev at lists.ozlabs.org
> > Subject: Re: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20 state and
> > altivec idle
> >
> > On Sun, 2013-11-03 at 22:04 -0600, Wang Dongsheng-B40534 wrote:
> > > > -----Original Message-----
> > > > From: Wang Dongsheng-B40534
> > > > Sent: Monday, October 21, 2013 11:11 AM
> > > > To: Wood Scott-B07421
> > > > Cc: Bhushan Bharat-R65777; linuxppc-dev at lists.ozlabs.org
> > > > Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20 state
> > > > and altivec idle
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Saturday, October 19, 2013 3:22 AM
> > > > > To: Wang Dongsheng-B40534
> > > > > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; linuxppc-
> > > > > dev at lists.ozlabs.org
> > > > > Subject: Re: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20
> > > > > state and altivec idle
> > > > >
> > > > > On Thu, 2013-10-17 at 22:02 -0500, Wang Dongsheng-B40534 wrote:
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Bhushan Bharat-R65777
> > > > > > > Sent: Thursday, October 17, 2013 2:46 PM
> > > > > > > To: Wang Dongsheng-B40534; Wood Scott-B07421
> > > > > > > Cc: linuxppc-dev at lists.ozlabs.org
> > > > > > > Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20
> > > > > > > state and altivec idle
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > > -----Original Message-----
> > > > > > > > > > From: Wang Dongsheng-B40534
> > > > > > > > > > Sent: Thursday, October 17, 2013 11:22 AM
> > > > > > > > > > To: Bhushan Bharat-R65777; Wood Scott-B07421
> > > > > > > > > > Cc: linuxppc-dev at lists.ozlabs.org
> > > > > > > > > > Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs
> > > > > > > > > > for
> > > > > > > > > > pw20 state and altivec idle
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > From: Bhushan Bharat-R65777
> > > > > > > > > > > Sent: Thursday, October 17, 2013 11:20 AM
> > > > > > > > > > > To: Wang Dongsheng-B40534; Wood Scott-B07421
> > > > > > > > > > > Cc: linuxppc-dev at lists.ozlabs.org
> > > > > > > > > > > Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs
> > > > > > > > > > > for
> > > > > > > > > > > pw20 state and altivec idle
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Wang Dongsheng-B40534
> > > > > > > > > > > > Sent: Thursday, October 17, 2013 8:16 AM
> > > > > > > > > > > > To: Bhushan Bharat-R65777; Wood Scott-B07421
> > > > > > > > > > > > Cc: linuxppc-dev at lists.ozlabs.org
> > > > > > > > > > > > Subject: RE: [PATCH v5 4/4] powerpc/85xx: add
> > > > > > > > > > > > sysfs for
> > > > > > > > > > > > pw20 state and altivec idle
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > From: Bhushan Bharat-R65777
> > > > > > > > > > > > > Sent: Thursday, October 17, 2013 1:01 AM
> > > > > > > > > > > > > To: Wang Dongsheng-B40534; Wood Scott-B07421
> > > > > > > > > > > > > Cc: linuxppc-dev at lists.ozlabs.org
> > > > > > > > > > > > > Subject: RE: [PATCH v5 4/4] powerpc/85xx: add
> > > > > > > > > > > > > sysfs for
> > > > > > > > > > > > > pw20 state and altivec idle
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > > > From: Wang Dongsheng-B40534
> > > > > > > > > > > > > > Sent: Tuesday, October 15, 2013 2:51 PM
> > > > > > > > > > > > > > To: Wood Scott-B07421
> > > > > > > > > > > > > > Cc: Bhushan Bharat-R65777;
> > > > > > > > > > > > > > linuxppc-dev at lists.ozlabs.org; Wang
> > > > > > > > > > > > > Dongsheng-B40534
> > > > > > > > > > > > > > Subject: [PATCH v5 4/4] powerpc/85xx: add
> > > > > > > > > > > > > > sysfs for
> > > > > > > > > > > > > > pw20 state and
> > > > > > > > > > > > > altivec idle
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 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 time interface:(Nanosecond)
> > > > > > > > > > > > > > /sys/devices/system/cpu/cpuX/pw20_wait_time
> > > > > > > > > > > > > > /sys/devices/system/cpu/cpuX/altivec_idle_wait
> > > > > > > > > > > > > > _t
> > > > > > > > > > > > > > ime
> > > > > > > > > > > > > > Example: Base on TBfreq is 41MHZ.
> > > > > > > > > > > > > > 1~48(ns): TB[63]
> > > > > > > > > > > > > > 49~97(ns): TB[62]
> > > > > > > > > > > > > > 98~195(ns): TB[61]
> > > > > > > > > > > > > > 196~390(ns): TB[60]
> > > > > > > > > > > > > > 391~780(ns): TB[59]
> > > > > > > > > > > > > > 781~1560(ns): TB[58] ...
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Signed-off-by: Wang Dongsheng
> > > > > > > > > > > > > > <dongsheng.wang at freescale.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > *v5:
> > > > > > > > > > > > > > Change get_idle_ticks_bit function implementation.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > *v4:
> > > > > > > > > > > > > > Move code from 85xx/common.c to kernel/sysfs.c.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Remove has_pw20_altivec_idle function.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Change wait "entry_bit" to wait time.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git a/arch/powerpc/kernel/sysfs.c
> > > > > > > > > > > > > > b/arch/powerpc/kernel/sysfs.c
> > > > > > > > > > > > > index
> > > > > > > > > > > > > > 27a90b9..10d1128 100644
> > > > > > > > > > > > > > --- a/arch/powerpc/kernel/sysfs.c
> > > > > > > > > > > > > > +++ b/arch/powerpc/kernel/sysfs.c
> > > > > > > > > > > > > > @@ -85,6 +85,284 @@
> > > > > > > > > > > > > > __setup("smt-snooze-delay=",
> > > > > > > > > > > > > setup_smt_snooze_delay);
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >  #endif /* CONFIG_PPC64 */
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > +#ifdef CONFIG_FSL_SOC
> > > > > > > > > > > > > > +#define MAX_BIT				63
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +static u64 pw20_wt; static u64
> > > > > > > > > > > > > > +altivec_idle_wt;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +static unsigned int get_idle_ticks_bit(u64 ns) {
> > > > > > > > > > > > > > +	u64 cycle;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	if (ns >= 10000)
> > > > > > > > > > > > > > +		cycle = div_u64(ns + 500, 1000) *
> > > > > > > tb_ticks_per_usec;
> > > > > > > > > > > > > > +	else
> > > > > > > > > > > > > > +		cycle = div_u64(ns *
> > tb_ticks_per_usec,
> > > > > 1000);
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	if (!cycle)
> > > > > > > > > > > > > > +		return 0;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	return ilog2(cycle); }
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +static void do_show_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,
> > > > > > > > > > > > > > +do_show_pwrmgtcr0, &value, 1);
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	value &= PWRMGTCR0_PW20_WAIT;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	return sprintf(buf, "%u\n", value ? 1 :
> > 0); }
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +static void do_store_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,
> > > > > > > > > > > > > > +do_store_pw20_state, &value, 1);
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	return count; }
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +static ssize_t show_pw20_wait_time(struct
> > > > > > > > > > > > > > +device
> > > > *dev,
> > > > > > > > > > > > > > +				struct device_attribute
> > *attr,
> > > > > char
> > > > > > > *buf) {
> > > > > > > > > > > > > > +	u32 value;
> > > > > > > > > > > > > > +	u64 tb_cycle;
> > > > > > > > > > > > > > +	s64 time;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	unsigned int cpu = dev->id;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +	if (!pw20_wt) {
> > > > > > > > > > > > > > +		smp_call_function_single(cpu,
> > > > > do_show_pwrmgtcr0,
> > > > > > > > > > > > > > +&value,
> > > > > > > > > > > 1);
> > > > > > > > > > > > > > +		value = (value &
> > PWRMGTCR0_PW20_ENT) >>
> > > > > > > > > > > > > > +
> > 	PWRMGTCR0_PW20_ENT_SHIFT;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +		tb_cycle = (1 << (MAX_BIT - value)) *
> > 2;
> > > > > > > > > > > > >
> > > > > > > > > > > > > Is value = 0 and value = 1 legal? These will
> > > > > > > > > > > > > make tb_cycle = 0,
> > > > > > > > > > > > >
> > > > > > > > > > > > > > +		time = div_u64(tb_cycle * 1000,
> > > > > tb_ticks_per_usec)
> > > > > > > - 1;
> > > > > > > > > > > > >
> > > > > > > > > > > > > And time = -1;
> > > > > > > > > > > > >
> > > > > > > > > > > > Please look at the end of the function, :)
> > > > > > > > > > > >
> > > > > > > > > > > > "return sprintf(buf, "%llu\n", time > 0 ? time : 0);"
> > > > > > > > > > >
> > > > > > > > > > > I know you return 0 if value = 0/1, my question was
> > > > > > > > > > > that, is this correct as per specification?
> > > > > > > > > > >
> > > > > > > > > > > Ahh, also for "value" upto 7 you will return 0, no?
> > > > > > > > > > >
> > > > > > > > > > If value = 0, MAX_BIT - value = 63 tb_cycle =
> > > > > > > > > > 0xffffffff_ffffffff, tb_cycle * 1000 will overflow,
> > > > > > > > > > but this
> > > > > situation is not possible.
> > > > > > > > > > Because if the "value = 0" means this feature will be
> > > > "disable".
> > > > > > > > > > Now The default wait bit is 50(MAX_BIT - value, value
> > > > > > > > > > = 13), the PW20/Altivec Idle wait entry time is about
> > > > > > > > > > 1ms, this time is very long for wait idle time, and
> > > > > > > > > > it's cannot be increased(means (MAX_BIT
> > > > > > > > > > - value)
> > > > > > > > > cannot greater than 50).
> > > > > > > > >
> > > > > > > > > What you said is not obvious from code and so at least
> > > > > > > > > write a comment that value will be always >= 13 or value
> > > > > > > > > will never be less than < 8 and below calculation will
> > > > > > > > > not overflow. may be error out if value is less than 8.
> > > > > > > > >
> > > > > > > > The "value" less than 10, this will overflow.
> > > > > > > > There is not error, The code I knew it could not be less
> > > > > > > > than 10, that's why I use the following code. :)
> > > > > > >
> > > > > > > I am sorry to persist but this is not about what you know,
> > > > > > > this is about how code is read and code does not say what
> > > > > > > you know, so add a comment at least and error out/warn when
> > > > > > > "value" is less than a
> > > > > certain number.
> > > > > > >
> > > > > > Sorry for the late to response the mail. If it caused
> > > > > > confusion, we can
> > > > > add a comment.
> > > > > >
> > > > > > How about the following comment?
> > > > > > /*
> > > > > >  * If the "value" less than 10, this will overflow.
> > > > > >  * From benchmark test, the default wait bit will not be set
> > > > > > less than
> > > > > 10bit.
> > > > > >  * Because 10 bit corresponds to the wait entry time is
> > > > > > 439375573401999609(ns),
> > > > > >  * for wait-entry-idle time this value looks too long, and we
> > > > > > cannot use those
> > > > > >  * "long" time as a default wait-entry time. So overflow could
> > > > > > not have happened
> > > > > >  * and we use this calculation method to get wait-entry-idle time.
> > > > > >  */
> > > > >
> > > > > If there's to be a limit on the times we accept, make it explicit.
> > > > > Check for it before doing any conversions, and return an error
> > > > > if userspace tries to set it.
> > > > >
> > > > The branch only use to read default wait-entry-time.
> > > > We have no limit the user's input, and we can't restrict. Once the
> > > > user set the wait-entry-time, the code will do another branch.
> > > >
> > >
> > > Hi scott,
> > > Do you have any comments about this patch?
> > > I will add the comment and send this patch again.
> >
> > What do you mean by "and we can't restrict"?  Why not?
> >
> > Why is it only used to read the default, and not the current value?
> >
> We already have a variable which value is set by the user, as we have discussed
> before.
> 
> When the system boot-up. Before user set the wait-entry-time, we need to return
> a default wait-entry-time, if the user read this sys-interface. The default
> wait-entry-time is converted by wait-bit.
> 
> Once the user set the sys-interface, a variable will be used to save it. And
> when the user read sys-interface we will return back the variable.

While we are not "restricting user defined value" or "define same restriction for user defined and default", can we have only one flow of calculation and setting rather than conditional based on user have set or not set? 

-Bharat

> 
> -dongsheng