| Message ID | 1378879004-2446-4-git-send-email-dongsheng.wang@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
> -----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
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
> -----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. :)
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
> -----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
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);