> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, September 12, 2013 7:04 AM
> To: Wang Dongsheng-B40534
> Cc: ga...@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.w...@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.w...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
