Re: [linux-pm] OpPoint summary

Re: [linux-pm] OpPoint summary

[Vitaly Wool]

Hi Greg,

On 9/14/06, Greg KH <greg@kroah.com> wrote:
> >
> > The kernel patches are available at:
> >
> >        http://source.mvista.com/~dsingleton/2.6.1-rc6
>
> I get a 404 with that link :(

I bet David meant http://source.mvista.com/~dsingleton/2.6.18-rc6/

Vitaly

Re: [linux-pm] OpPoint summary

[Vitaly Wool]

On 9/14/06, Auke Kok <auke-jan.h.kok@intel.com> wrote:
> David Singleton wrote:
>
> > +static const struct cpu_id cpu_ids[] = {
> > +       [CPU_BANIAS]    = { 6,  9, 5 },
> > +       [CPU_DOTHAN_A1] = { 6, 13, 1 },
> > +       [CPU_DOTHAN_A2] = { 6, 13, 2 },
> > +       [CPU_DOTHAN_B0] = { 6, 13, 6 },
> > +       [CPU_MP4HT_D0]  = {15,  3, 4 },
> > +       [CPU_MP4HT_E0]  = {15,  4, 1 },
> > +};
>
>
> Any reason why { 6, 13, 8 } is missing? My lenovo T43 identifies itself as such:
>
> processor       : 0
> vendor_id       : GenuineIntel
> cpu family      : 6
> model           : 13
> model name      : Intel(R) Pentium(R) M processor 1.86GHz
> stepping        : 8
>
> I'm not sure a Dothan B1 exists, but some postings suggest even C0 and C1 are
> valid steppings. I'm sure OpPoint could work with those as well.

Heh, that shows pretty much that the approach itself is not good...
And this is only beginning.

Vitaly

Re: [linux-pm] OpPoint summary

[Matthew Locke]

Dave,

I am confused as to why you continue to duplicate work already done in 
PowerOP.  We are still waiting to hear why you need to have a different 
operating point interface.  As Eugeny and I  have pointed out before 
(refer to the thread  in my PowerOP vs OPpoint email), your interface 
doesn't make sense on a mulit cpu, SMP or otherwise, system.  I 
recommend you redo your patches to use PowerOP which already supports 
this properly.


On Sep 16, 2006, at 10:07 PM, David Singleton wrote:

> Pavel and Greg,
>
> I've incorporated Pavels suggestions and only put suspend states
> in the /sys/power/state file.  The control file for frequency and
> voltage operating
> point transitions is now in /sys/power/operating_points/current_point.
>
> The /sys/power/operating_points dirctory still contains the operating
> points themselves, with a frequency, voltage and latency file
> for each operating point.
>
> The oppointd power manager has been changed to use the
> new control file for operating points.  It has been tested on
> a centrino laptop, the 4 way Xeon server and the arm-pxa27x.
>
> I finally got SMP tested on a 4 way Xeon server.  The patch
> that supports SMP Xeon's is the oppoint-x86-p4.patch in the series.
>
> The only files in the core framework patch now are:
>
>         kernel/power/main.c
>         include/linux/pm.h
>         kernel/power/power.h
>
> The full patch set is at
>
>         http://source.mvista.com/~dsingleton/2.6.18-rc7
>
> The power manager source and patch is at:
>
>         http://source.mvista.com/~dsingleton/oppointd-1.2.3
>
> Attached is the oppoint-core.patch.
>
> David
>
> Signed-Off-by: David Singleton <dsingleton@mvista.com>
>
>  include/linux/pm.h   |   30 +++-
>  kernel/power/main.c  |  361 
> +++++++++++++++++++++++++++++++++++++++++++++------
>  kernel/power/power.h |    2
>  3 files changed, 350 insertions(+), 43 deletions(-)
>
> Index: linux-2.6.17/kernel/power/main.c
> ===================================================================
> --- linux-2.6.17.orig/kernel/power/main.c
> +++ linux-2.6.17/kernel/power/main.c
> @@ -16,6 +16,7 @@
>  #include <linux/init.h>
>  #include <linux/pm.h>
>  #include <linux/console.h>
> +#include <linux/module.h>
>
>  #include "power.h"
>
> @@ -49,7 +50,7 @@ void pm_set_ops(struct pm_ops * ops)
>   *     the platform can enter the requested state.
>   */
>
> -static int suspend_prepare(suspend_state_t state)
> +static int suspend_prepare(struct oppoint * state)
>  {
>         int error = 0;
>         unsigned int free_pages;
> @@ -82,7 +83,7 @@ static int suspend_prepare(suspend_state
>         }
>
>         if (pm_ops->prepare) {
> -               if ((error = pm_ops->prepare(state)))
> +               if ((error = pm_ops->prepare(state->type)))
>                         goto Thaw;
>         }
>
> @@ -94,7 +95,7 @@ static int suspend_prepare(suspend_state
>         return 0;
>   Finish:
>         if (pm_ops->finish)
> -               pm_ops->finish(state);
> +               pm_ops->finish(state->type);
>   Thaw:
>         thaw_processes();
>   Enable_cpu:
> @@ -104,7 +105,7 @@ static int suspend_prepare(suspend_state
>  }
>
>
> -int suspend_enter(suspend_state_t state)
> +int suspend_enter(struct oppoint * state)
>  {
>         int error = 0;
>         unsigned long flags;
> @@ -115,7 +116,7 @@ int suspend_enter(suspend_state_t state)
>                 printk(KERN_ERR "Some devices failed to power down\n");
>                 goto Done;
>         }
> -       error = pm_ops->enter(state);
> +       error = pm_ops->enter(state->type);
>         device_power_up();
>   Done:
>         local_irq_restore(flags);
> @@ -131,36 +132,82 @@ int suspend_enter(suspend_state_t state)
>   *     console that we've allocated. This is not called for 
> suspend-to-disk.
>   */
>
> -static void suspend_finish(suspend_state_t state)
> +static void suspend_finish(struct oppoint * state)
>  {
>         device_resume();
>         resume_console();
>         thaw_processes();
>         enable_nonboot_cpus();
>         if (pm_ops && pm_ops->finish)
> -               pm_ops->finish(state);
> +               pm_ops->finish(state->type);
>         pm_restore_console();
>  }
>
> +struct list_head pm_list;
> +static struct oppoint standby = {
> +       .name = "standby",
> +       .type = PM_SUSPEND_STANDBY,
> +};
>
> +static struct oppoint mem = {
> +       .name = "mem",
> +       .type = PM_SUSPEND_MEM,
> +       .frequency = 0,
> +       .voltage = 0,
> +       .latency = 150,
> +};
>
> -
> -static const char * const pm_states[PM_SUSPEND_MAX] = {
> -       [PM_SUSPEND_STANDBY]    = "standby",
> -       [PM_SUSPEND_MEM]        = "mem",
>  #ifdef CONFIG_SOFTWARE_SUSPEND
> -       [PM_SUSPEND_DISK]       = "disk",
> +struct oppoint disk = {
> +       .name = "disk",
> +       .type = PM_SUSPEND_DISK,
> +};
>  #endif
> +
> +struct oppoint pm_states = {
> +       .name = "default",
> +       .type = PM_FREQ_CHANGE,
>  };
> +struct oppoint *current_state;
> +
> +/*
> + *
> + */
> +static int pm_change_state(struct oppoint *state)
> +{
> +       int error = 0;
> +
> +       printk("OpPoint: changing from %s to %s\n", 
> current_state->name,
> +            state->name);
> +       /*
> +        * compare to current operating point.
> +        * if different change to new operating point.
> +        */
> +       if (current_state == state)
> +               goto out;
> +
> +       if ((error = state->prepare_transition(current_state, state)))
> +               goto out;
> +
> +       if ((error = state->transition(current_state, state)))
> +                state = current_state;
> +
> +       if ((error = state->finish_transition(current_state, state)) 
> == 0)
> +               current_state = state;
> +
> +out:
> +       printk("OpPoint: State change returned %d\n", error);
> +       return error;
> +}
>
> -static inline int valid_state(suspend_state_t state)
> +static inline int valid_state(struct oppoint * state)
>  {
>         /* Suspend-to-disk does not really need low-level support.
>          * It can work with reboot if needed. */
> -       if (state == PM_SUSPEND_DISK)
> +       if (state->type == PM_SUSPEND_DISK)
>                 return 1;
>
> -       if (pm_ops && pm_ops->valid && !pm_ops->valid(state))
> +       if (pm_ops && pm_ops->valid && !pm_ops->valid(state->type))
>                 return 0;
>         return 1;
>  }
> @@ -168,7 +215,7 @@ static inline int valid_state(suspend_st
>
>  /**
>   *     enter_state - Do common work of entering low-power state.
> - *     @state:         pm_state structure for state we're entering.
> + *     @state:         oppoint structure for state we're entering.
>   *
>   *     Make sure we're the only ones trying to enter a sleep state. 
> Fail
>   *     if someone has beat us to it, since we don't want anything 
> weird to
> @@ -177,7 +224,7 @@ static inline int valid_state(suspend_st
>   *     we've woken up).
>   */
>
> -static int enter_state(suspend_state_t state)
> +static int enter_state(struct oppoint *state)
>  {
>         int error;
>
> @@ -186,16 +233,21 @@ static int enter_state(suspend_state_t s
>         if (down_trylock(&pm_sem))
>                 return -EBUSY;
>
> -       if (state == PM_SUSPEND_DISK) {
> +       if (state->type == PM_SUSPEND_DISK) {
>                 error = pm_suspend_disk();
>                 goto Unlock;
>         }
>
> -       pr_debug("PM: Preparing system for %s sleep\n", 
> pm_states[state]);
> +       if (state->type == PM_FREQ_CHANGE || state->type == 
> PM_VOLT_CHANGE) {
> +               error = pm_change_state(state);
> +               goto Unlock;
> +       }
> +
> +       pr_debug("PM: Preparing system for %s sleep\n", state->name);
>         if ((error = suspend_prepare(state)))
>                 goto Unlock;
>
> -       pr_debug("PM: Entering %s sleep\n", pm_states[state]);
> +       pr_debug("PM: Entering %s sleep\n", state->name);
>         error = suspend_enter(state);
>
>         pr_debug("PM: Finishing wakeup.\n");
> @@ -211,7 +263,15 @@ static int enter_state(suspend_state_t s
>   */
>  int software_suspend(void)
>  {
> -       return enter_state(PM_SUSPEND_DISK);
> +       struct oppoint *this, *next;
> +       struct list_head *head = &mem.list;
> +       int error = 0;
> +
> +       list_for_each_entry_safe(this, next, head, list) {
> +               if (this->type == PM_SUSPEND_DISK)
> +                       error= enter_state(this);
> +       }
> +       return error;
>  }
>
>
> @@ -223,9 +283,9 @@ int software_suspend(void)
>   *     structure, and enter (above).
>   */
>
> -int pm_suspend(suspend_state_t state)
> +int pm_suspend(struct oppoint * state)
>  {
> -       if (state > PM_SUSPEND_ON && state <= PM_SUSPEND_MAX)
> +       if (state->type > PM_SUSPEND_ON && state->type <= 
> PM_SUSPEND_MAX)
>                 return enter_state(state);
>         return -EINVAL;
>  }
> @@ -248,36 +308,35 @@ decl_subsys(power,NULL,NULL);
>
>  static ssize_t state_show(struct subsystem * subsys, char * buf)
>  {
> -       int i;
> -       char * s = buf;
> +       struct oppoint *this, *next;
> +       struct list_head *head = &pm_list;
> +       char *s = buf;
> +
> +       list_for_each_entry_safe(this, next, head, list)
> +               s += sprintf(s,"%s ", this->name);
>
> -       for (i = 0; i < PM_SUSPEND_MAX; i++) {
> -               if (pm_states[i] && valid_state(i))
> -                       s += sprintf(s,"%s ", pm_states[i]);
> -       }
>         s += sprintf(s,"\n");
> +
>         return (s - buf);
>  }
>
>  static ssize_t state_store(struct subsystem * subsys, const char *
> buf, size_t n)
>  {
> -       suspend_state_t state = PM_SUSPEND_STANDBY;
> -       const char * const *s;
> +       struct oppoint *this, *next;
> +       struct list_head *head = &mem.list;
>         char *p;
> -       int error;
> +       int error = -EINVAL;
>         int len;
>
>         p = memchr(buf, '\n', n);
>         len = p ? p - buf : n;
> -
> -       for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, 
> state++) {
> -               if (*s && !strncmp(buf, *s, len))
> +       list_for_each_entry_safe(this, next, head, list) {
> +               if ((strlen(this->name) == len) &&
> +                  (!strncmp(this->name, buf, len))) {
> +                       error = enter_state(this);
>                         break;
> +               }
>         }
> -       if (state < PM_SUSPEND_MAX && *s)
> -               error = enter_state(state);
> -       else
> -               error = -EINVAL;
>         return error ? error : n;
>  }
>
> @@ -292,12 +351,234 @@ static struct attribute_group attr_group
>         .attrs = g,
>  };
>
> +static struct kobject oppoint_kobj = {
> +        .kset = &power_subsys.kset,
> +};
> +
> +struct oppoint_attribute {
> +        struct attribute        attr;
> +        ssize_t (*show)(struct kobject * kobj, char * buf);
> +        ssize_t (*store)(struct kobject * kobj, const char * buf,
> size_t count);
> +};
> +
> +#define to_oppoint(obj) container_of(obj,struct oppoint,kobj)
> +#define to_oppoint_attr(_attr) container_of(_attr,struct
> oppoint_attribute,attr)
> +/*
> + * the frequency, voltage and latency files are readonly
> + */
> +
> +static ssize_t oppoint_voltage_show(struct kobject * kobj, char * buf)
> +{
> +       ssize_t len;
> +       struct oppoint *opt = to_oppoint(kobj);
> +
> +       len = sprintf(buf, "%8d\n", opt->voltage);
> +
> +       return len;
> +}
> +
> +static ssize_t oppoint_voltage_store(struct kobject * kobj, const 
> char * buf,
> +       size_t n)
> +{
> +        return -EINVAL;
> +
> +}
> +
> +static ssize_t oppoint_frequency_show(struct kobject * kobj, char * 
> buf)
> +{
> +       ssize_t len;
> +       struct oppoint *opt = to_oppoint(kobj);
> +
> +       len = sprintf(buf, "%8d\n", opt->frequency);
> +
> +       return len;
> +}
> +
> +static ssize_t oppoint_frequency_store(struct kobject * kobj,
> +        const char * buf, size_t n)
> +{
> +        return -EINVAL;
> +
> +}
> +
> +static ssize_t oppoint_point_show(struct kobject * kobj, char * buf)
> +{
> +       ssize_t len;
> +
> +       len = sprintf(buf, "%s\n", current_state->name);
> +
> +       return len;
> +}
> +
> +static ssize_t oppoint_point_store(struct kobject * kobj, const char 
> * buf,
> +       size_t n)
> +{
> +        struct oppoint *this, *next;
> +        struct list_head *head = &pm_states.list;
> +        char *p;
> +        int error = -EINVAL;
> +        int len;
> +
> +        p = memchr(buf, '\n', n);
> +        len = p ? p - buf : n;
> +        list_for_each_entry_safe(this, next, head, list) {
> +                if ((strlen(this->name) == len) &&
> +                   (!strncmp(this->name, buf, len))) {
> +                        error = enter_state(this);
> +                        break;
> +                }
> +        }
> +        return error ? error : n;
> +}
> +
> +static ssize_t oppoint_latency_show(struct kobject * kobj, char * buf)
> +{
> +       ssize_t len;
> +       struct oppoint *opt = to_oppoint(kobj);
> +
> +       len = sprintf(buf, "%8d\n", opt->latency);
> +
> +       return len;
> +}
> +
> +static ssize_t oppoint_latency_store(struct kobject * kobj,
> +        const char * buf, size_t n)
> +{
> +        return -EINVAL;
> +
> +}
> +
> +static struct oppoint_attribute point_attr = {
> +        .attr   = {
> +                .name = "current_point",
> +                .mode = 0600,
> +        },
> +        .show   = oppoint_point_show,
> +        .store  = oppoint_point_store,
> +};
> +
> +static struct oppoint_attribute frequency_attr = {
> +        .attr   = {
> +                .name = "frequency",
> +                .mode = 0400,
> +        },
> +        .show   = oppoint_frequency_show,
> +        .store  = oppoint_frequency_store,
> +};
> +
> +static struct oppoint_attribute voltage_attr = {
> +        .attr   = {
> +                .name = "voltage",
> +                .mode = 0400,
> +        },
> +        .show   = oppoint_voltage_show,
> +        .store  = oppoint_voltage_store,
> +};
> +
> +static struct oppoint_attribute latency_attr = {
> +        .attr   = {
> +                .name = "latency",
> +                .mode = 0400,
> +        },
> +        .show   = oppoint_latency_show,
> +        .store  = oppoint_latency_store,
> +};
> +
> +static ssize_t
> +oppoint_attr_show(struct kobject * kobj, struct attribute * attr, 
> char * buf)
> +{
> +       struct oppoint_attribute * opt_attr = to_oppoint_attr(attr);
> +       ssize_t ret = 0;
> +
> +       if (opt_attr->show)
> +               ret = opt_attr->show(kobj,buf);
> +       return ret;
> +}
> +
> +static ssize_t
> +oppoint_attr_store(struct kobject * kobj, struct attribute * attr,
> +             const char * buf, size_t count)
> +{
> +       return -EINVAL;
> +}
> +
> +static void oppoint_kobj_release(struct kobject *kobj)
> +{
> +       return;
> +}
> +
> +static struct sysfs_ops oppoint_sysfs_ops = {
> +       .show   = oppoint_attr_show,
> +       .store  = oppoint_attr_store,
> +};
> +
> +static struct attribute * oppoint_default_attrs[] = {
> +       &frequency_attr.attr,
> +       &voltage_attr.attr,
> +       &latency_attr.attr,
> +       NULL,
> +};
> +
> +static struct kobj_type ktype_operating_point = {
> +        .release        = oppoint_kobj_release,
> +        .sysfs_ops      = &oppoint_sysfs_ops,
> +        .default_attrs  = oppoint_default_attrs,
> +};
> +
> +int unregister_operating_point(struct oppoint *opt)
> +{
> +       down(&pm_sem);
> +       list_del_init(&opt->list);
> +       sysfs_remove_file(&opt->kobj, &frequency_attr.attr);
> +       sysfs_remove_file(&opt->kobj, &voltage_attr.attr);
> +       sysfs_remove_file(&opt->kobj, &latency_attr.attr);
> +       up(&pm_sem);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(unregister_operating_point);
> +
> +int register_operating_point(struct oppoint *opt)
> +{
> +       down(&pm_sem);
> +       kobject_set_name(&opt->kobj, opt->name);
> +       opt->kobj.kset = &power_subsys.kset;
> +       opt->kobj.parent = &oppoint_kobj;
> +       opt->kobj.ktype = &ktype_operating_point;
> +       kobject_register(&opt->kobj);
> +
> +       sysfs_create_file(&opt->kobj, &frequency_attr.attr);
> +       sysfs_create_file(&opt->kobj, &voltage_attr.attr);
> +       sysfs_create_file(&opt->kobj, &latency_attr.attr);
> +
> +       list_add_tail(&opt->list, &pm_states.list);
> +       up(&pm_sem);
> +       return 0;
> +}
> +EXPORT_SYMBOL(register_operating_point);
>
>  static int __init pm_init(void)
>  {
> +
>         int error = subsystem_register(&power_subsys);
> -       if (!error)
> +       if (!error) {
>                 error = 
> sysfs_create_group(&power_subsys.kset.kobj,&attr_group);
> +               kobject_set_name(&oppoint_kobj, "operating_points");
> +               kobject_register(&oppoint_kobj);
> +               sysfs_create_file(&oppoint_kobj, &point_attr.attr);
> +       }
> +
> +
> +       INIT_LIST_HEAD(&pm_states.list);
> +       INIT_LIST_HEAD(&pm_list);
> +
> +#ifdef CONFIG_SOFTWARE_SUSPEND
> +       list_add(&disk.list, &pm_list);
> +#endif
> +       list_add(&standby.list, &pm_list);
> +       list_add(&mem.list, &pm_list);
> +       current_state = &pm_states;
> +
>         return error;
>  }
>
> Index: linux-2.6.17/include/linux/pm.h
> ===================================================================
> --- linux-2.6.17.orig/include/linux/pm.h
> +++ linux-2.6.17/include/linux/pm.h
> @@ -24,6 +24,7 @@
>  #ifdef __KERNEL__
>
>  #include <linux/list.h>
> +#include <linux/kobject.h>
>  #include <asm/atomic.h>
>
>  /*
> @@ -108,7 +109,32 @@ typedef int __bitwise suspend_state_t;
>  #define PM_SUSPEND_STANDBY     ((__force suspend_state_t) 1)
>  #define PM_SUSPEND_MEM         ((__force suspend_state_t) 3)
>  #define PM_SUSPEND_DISK                ((__force suspend_state_t) 4)
> -#define PM_SUSPEND_MAX         ((__force suspend_state_t) 5)
> +#define PM_FREQ_CHANGE         ((__force suspend_state_t) 5)
> +#define PM_VOLT_CHANGE         ((__force suspend_state_t) 6)
> +#define PM_SUSPEND_MAX         ((__force suspend_state_t) 7)
> +
> +struct oppoint {
> +       struct list_head list;
> +       suspend_state_t type;
> +       char *name;
> +       unsigned int flags;
> +       unsigned int frequency;         /* in KHz */
> +       unsigned int voltage;           /* mV */
> +       unsigned int latency;           /* transition latency in us */
> +       int     (*prepare_transition)(struct oppoint *cur, struct 
> oppoint *new);
> +       int     (*transition)(struct oppoint *cur, struct oppoint 
> *new);
> +       int     (*finish_transition)(struct oppoint *cur, struct 
> oppoint *new);
> +
> +       void *md_data;                  /* arch dependent data */
> +       struct kobject kobj;
> +};
> +
> +
> +extern struct oppoint pm_states;
> +extern struct oppoint *current_state;
> +extern int register_operating_point(struct oppoint *opt);
> +extern int unregister_operating_point(struct oppoint *opt);
> +struct notifier_block;
>
>  typedef int __bitwise suspend_disk_method_t;
>
> @@ -128,7 +154,7 @@ struct pm_ops {
>
>  extern void pm_set_ops(struct pm_ops *);
>  extern struct pm_ops *pm_ops;
> -extern int pm_suspend(suspend_state_t state);
> +extern int pm_suspend(struct oppoint *state);
>
>
>  /*
> Index: linux-2.6.17/kernel/power/power.h
> ===================================================================
> --- linux-2.6.17.orig/kernel/power/power.h
> +++ linux-2.6.17/kernel/power/power.h
> @@ -113,4 +113,4 @@ extern int swsusp_resume(void);
>  extern int swsusp_read(void);
>  extern int swsusp_write(void);
>  extern void swsusp_close(void);
> -extern int suspend_enter(suspend_state_t state);
> +extern int suspend_enter(struct oppoint * state);
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/linux-pm
>

Re: [linux-pm] OpPoint summary

[Scott E. Preece]

| From: Pavel Machek<pavel@ucw.cz>
| 
| Hi!
| 
| > >Care to resend your patches in the proper format, through email so that
| > >we can see them, and possibly get some testing in -mm if they look sane?
| > 
| > Greg,
| >   here's the patch that implements operating points for different 
| >   frequencies
| > for the speedstep-centrino line of processors.  Operating points are created
| > in much the same manner that cpufreq tables are.  This works for both
| > simple implementations like the centrino and more complex SoC systems
| > like the arm-pxa72x which has several clocks to control, and different clock
| > divisors and multipliers.
| 
| > +static struct oppoint lowest = {
| > +       .name = "lowest",
| > +       .type = PM_FREQ_CHANGE,
| > +       .frequency = 0,
| > +       .voltage = 0,
| > +       .latency = 15,
| > +       .prepare_transition  = cpufreq_prepare_transition,
| > +       .transition = centrino_transition,
| > +       .finish_transition = cpufreq_finish_transition,
| > +};
| 
| We had nice, descriptive interface... with numbers. Now you want to
| introduce english state names... looks like a step back to me.
---

Well, a single number is fine if you're describing a scalar abstraction,
but an operating point is a vector. You can't assume that "399" is three
times "133" in performance or energy cost, so its "numberness" is simply
misleading.

scott

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com

Re: [linux-pm] OpPoint summary

[Pavel Machek]

H!

> | > +static struct oppoint lowest = {
> | > +       .name = "lowest",
> | > +       .type = PM_FREQ_CHANGE,
> | > +       .frequency = 0,
> | > +       .voltage = 0,
> | > +       .latency = 15,
> | > +       .prepare_transition  = cpufreq_prepare_transition,
> | > +       .transition = centrino_transition,
> | > +       .finish_transition = cpufreq_finish_transition,
> | > +};
> | 
> | We had nice, descriptive interface... with numbers. Now you want to
> | introduce english state names... looks like a step back to me.
> ---
> 
> Well, a single number is fine if you're describing a scalar abstraction,
> but an operating point is a vector. You can't assume that "399" is three
> times "133" in performance or energy cost, so its "numberness" is simply
> misleading.

"lowest" can simply be mapped to "0", with "low" mapped to "1",
etc.

I believe, using english names is wrong in this case. If you want to
provide vectors... well provide the vectors. Is "medium" operating
point 1GHz on cpu 0 and 2GHz on cpu 1, or is it 1.5 ghz on cpu 0 and
1.5 ghz on cpu 1?
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] OpPoint summary

[Richard A. Griffiths]

On Sun, 2006-09-17 at 19:48 +0200, Pavel Machek wrote:
> Hi!
> 
> > >Care to resend your patches in the proper format, through email so that
> > >we can see them, and possibly get some testing in -mm if they look sane?
> > 
> > Greg,
> >   here's the patch that implements operating points for different 
> >   frequencies
> > for the speedstep-centrino line of processors.  Operating points are created
> > in much the same manner that cpufreq tables are.  This works for both
> > simple implementations like the centrino and more complex SoC systems
> > like the arm-pxa72x which has several clocks to control, and different clock
> > divisors and multipliers.
> 
> > +static struct oppoint lowest = {
> > +       .name = "lowest",
> > +       .type = PM_FREQ_CHANGE,
> > +       .frequency = 0,
> > +       .voltage = 0,
> > +       .latency = 15,
> > +       .prepare_transition  = cpufreq_prepare_transition,
> > +       .transition = centrino_transition,
> > +       .finish_transition = cpufreq_finish_transition,
> > +};
> 
> We had nice, descriptive interface... with numbers. Now you want to
> introduce english state names... looks like a step back to me.

Maybe a compromise could be reached where a defined set of numbers maps
to  string names ala Unix init states. Many people (at least me) still
invoke init 6 to reboot a system.  A defined table would satisfy both
the number and string camps.

Richard

Re: [linux-pm] OpPoint summary

[Matthew Locke]

On Sep 18, 2006, at 7:33 AM, Richard A. Griffiths wrote:

> On Sun, 2006-09-17 at 19:48 +0200, Pavel Machek wrote:
>> Hi!
>>
>>>> Care to resend your patches in the proper format, through email so 
>>>> that
>>>> we can see them, and possibly get some testing in -mm if they look 
>>>> sane?
>>>
>>> Greg,
>>>   here's the patch that implements operating points for different
>>>   frequencies
>>> for the speedstep-centrino line of processors.  Operating points are 
>>> created
>>> in much the same manner that cpufreq tables are.  This works for both
>>> simple implementations like the centrino and more complex SoC systems
>>> like the arm-pxa72x which has several clocks to control, and 
>>> different clock
>>> divisors and multipliers.
>>
>>> +static struct oppoint lowest = {
>>> +       .name = "lowest",
>>> +       .type = PM_FREQ_CHANGE,
>>> +       .frequency = 0,
>>> +       .voltage = 0,
>>> +       .latency = 15,
>>> +       .prepare_transition  = cpufreq_prepare_transition,
>>> +       .transition = centrino_transition,
>>> +       .finish_transition = cpufreq_finish_transition,
>>> +};
>>
>> We had nice, descriptive interface... with numbers. Now you want to
>> introduce english state names... looks like a step back to me.
>
> Maybe a compromise could be reached where a defined set of numbers maps
> to  string names ala Unix init states. Many people (at least me) still
> invoke init 6 to reboot a system.  A defined table would satisfy both
> the number and string camps.

PowerOP allows the platform to define the name. In our cpufreq 
integration patches, we reuse the same name that cpufreq centrino used.

>
> Richard
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/linux-pm
>