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

From: Scott Wood <hidden>
Date: 2013-11-04 21:51:54

On Sun, 2013-11-03 at 22:04 -0600, Wang Dongsheng-B40534 wrote:

quoted

-----Original Message-----
From: Wang Dongsheng-B40534
Sent: Monday, October 21, 2013 11:11 AM
To: Wood Scott-B07421
Cc: Bhushan Bharat-R65777; linuxppc-dev@lists.ozlabs.org
Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20 state and
altivec idle

quoted

-----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@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:

quoted

-----Original Message-----
From: Bhushan Bharat-R65777
Sent: Thursday, October 17, 2013 2:46 PM
To: Wang Dongsheng-B40534; Wood Scott-B07421
Cc: linuxppc-dev@lists.ozlabs.org
Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20 state
and altivec idle

quoted

-----Original Message-----
From: Wang Dongsheng-B40534
Sent: Thursday, October 17, 2013 11:22 AM
To: Bhushan Bharat-R65777; Wood Scott-B07421
Cc: linuxppc-dev@lists.ozlabs.org
Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for pw20
state and altivec idle

quoted

-----Original Message-----
From: Bhushan Bharat-R65777
Sent: Thursday, October 17, 2013 11:20 AM
To: Wang Dongsheng-B40534; Wood Scott-B07421
Cc: linuxppc-dev@lists.ozlabs.org
Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for
pw20 state and altivec idle

quoted

-----Original Message-----
From: Wang Dongsheng-B40534
Sent: Thursday, October 17, 2013 8:16 AM
To: Bhushan Bharat-R65777; Wood Scott-B07421
Cc: linuxppc-dev@lists.ozlabs.org
Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs for
pw20 state and altivec idle

quoted

-----Original Message-----
From: Bhushan Bharat-R65777
Sent: Thursday, October 17, 2013 1:01 AM
To: Wang Dongsheng-B40534; Wood Scott-B07421
Cc: linuxppc-dev@lists.ozlabs.org
Subject: RE: [PATCH v5 4/4] powerpc/85xx: add sysfs
for
pw20 state and altivec idle

quoted

-----Original Message-----
From: Wang Dongsheng-B40534
Sent: Tuesday, October 15, 2013 2:51 PM
To: Wood Scott-B07421
Cc: Bhushan Bharat-R65777;
linuxppc-dev@lists.ozlabs.org; Wang

Dongsheng-B40534

quoted

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

altivec idle

quoted

From: Wang Dongsheng <redacted>

Add a sys interface to enable/diable pw20 state or
altivec idle, and

control the

quoted

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_time
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
[off-list ref]
---
*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

quoted

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);

quoted

 #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;

quoted

quoted
quoted
quoted
quoted
quoted
quoted
+	else
+		cycle = div_u64(ns * tb_ticks_per_usec,

1000);

quoted

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

quoted

*buf) {

quoted

+	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)

quoted

+	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,

quoted

quoted
quoted
quoted
quoted
quoted
quoted
quoted
quoted
+				struct device_attribute *attr,

char

quoted

*buf) {

quoted

+	u32 value;
+	u64 tb_cycle;
+	s64 time;
+
+	unsigned int cpu = dev->id;
+
+	if (!pw20_wt) {
+		smp_call_function_single(cpu,

do_show_pwrmgtcr0,

quoted

quoted
quoted
+&value,

1);

quoted

+		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,

quoted

+		time = div_u64(tb_cycle * 1000,

tb_ticks_per_usec)

quoted

- 1;

quoted

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.

quoted

quoted
quoted
quoted
quoted
Because if the "value = 0" means this feature will be

"disable".

quoted

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.

quoted

Sorry for the late to response the mail. If it caused confusion, we
can

add a comment.

quoted

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.

quoted

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

-Scott

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help