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

[Dongsheng Wang]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, November 05, 2013 7:48 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-10-20 at 22:27 -0500, Wang Dongsheng-B40534 wrote:
> >
> > > -----Original Message-----
> > > From: Wood Scott-B07421
> > > Sent: Saturday, October 19, 2013 3:23 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 Thu, 2013-10-17 at 21:36 -0500, Wang Dongsheng-B40534 wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Wood Scott-B07421
> > > > > Sent: Friday, October 18, 2013 12:52 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 00:51 -0500, Wang Dongsheng-B40534 wrote:
> > > > > >
> > > > > > > -----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
> > > > > > > > > >
> > > > > > > > > > +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,
> > > > >
> > > > > Actually, tb_cycle will be undefined because you shifted a
> > > > > 32-bit value
> > > > > (1) by more than 31 bits.  s/1/1ULL/
> > > > >
> > > > Actually, we have been discussing this situation that could not
> > > > have
> > > happened.
> > > > See !pw20_wt branch, this branch is read default wait bit.
> > > > The default wait bit is 50, the time is about 1ms.
> > > > The default wait bit cannot less than 50, means the wait entry
> > > > time
> > > cannot greater than 1ms.
> > > > We have already begun benchmark test, and we got a preliminary
> results.
> > > > 55, 56, 57bit looks good, but we need more benchmark to get the
> > > > default
> > > bit.
> > >
> > > What does the default have to do with it?  The user could have set a
> > > different value, and then read it back.
> > >
> > > Plus, how much time corresponds to bit 50 depends on the actual
> > > timebase frequency which could vary.
> > >
> > 	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;
> > 		time = tb_cycle * (1000 / tb_ticks_per_usec) - 1;
> > 	} else {
> > 		time = pw20_wt;
> > 	}
> >
> > As we have discussed before we need a variable to save To save the
> users to set wait-entry-time value.
> >
> > See the code, if user have set a value, and the value will be set in
> pw20_wt.
> > When the user read it back, the code will do "time = pw20_wt" branch.
> >
> > When the user not set the wait-entry-time value and read this sys
> interface, the code will do the following branch.
> 
> Oh, so it's not that you "need" this, you already have it.
> 
Yes, we already have a variable. :)