Please ignore the patch attached to the previous email and
consider current patch attached.
This patch contains complete PowerOP Core layer rework.
Other patches follow shortly.
My comments below remain valid.
Thanks,
Eugeny
2006/8/2, Eugeny S. Mints <eugeny.mints at gmail.com>:
> David Brownell wrote:
> > On Sunday 30 July 2006 12:32 pm, Eugeny S. Mints wrote:
> >
> >
> >>> Whereas I say that arch dependent is arch dependent ... unless
> >>> that "core" is indirecting through "struct powerop_point *", the
> >>> interface doesn't include the struct at all. Ergo my comment.
> >>>
> >>>
> >>>
> >> Let me try to set it this way.
> >> struct powerop_point is an arch independent piece in the sense that any
> >> platform
> >> which leverages PorewOP concept should implement struct powerop_point.
> >> struct powerop_point is fundamental component that has to be defined by
> >> a platfrom.
> >>
> >
> > We're not communicating here ... if the contents are arch-specific,
> > it doesn't matter to the interface except that it exist. A better
> > way to define it would be:
> >
> > struct powerop_point {
> > struct kobject kobj;
> > void *arch_hook;
> > // presumably there will be method hooks too, like
> > int (*enter_prepare)(struct powerop_point *);
> > int (*enter)(struct powerop_point *);
> > int (*enter_complete)(struct powerop_point *);
> > };
> >
> > where that "void *" is the entire arch hook, and the kobj holds the
> > name and represents the /sys/power/... directory for that arch.
> >
> > (Those methods are just placeholders for what might be needed; the
> > prepare might suspend certain devices, the complete might resume
> > them with different underlying clock or voltage availability, and
> > the enter would change voltage, clocks, and whatever else.)
> >
> >
> >
> Please find proposal of updated interface attached. The update
> addresses "void *" approach but does not take hooks outlined
> above. Updated approach allows to handle lists of named
> operating points inside as well as outside the PowerOP Core
> layer.
>
> The main terms and assumptions are:
> - set of a platform power parameters: platform parameters which
> affect platform power consumption
>
> - operating point: _fixed_length_ array of power parameters.
> length varies from platform to platform. For more details
> and discussion of fixed length approach see
> http://lists.osdl.org/pipermail/linux-pm/2006-August/003156.html
>
> (the following explains why there are no hooks outlined
> by David in my updated interface proposal)
> - assuming we deferred discussion of whether sleep operating
> points should be handled in the same way with others (seems
> we agree here) and consider all except sleep ops.
>
> -I guess you're talking about drivers notifications in your
> explanation about arch dependent hooks to allow a certain driver to
> adjust something (stop and restart DMA for ex) it needs in regard to
> changing to new operating point.
>
> Although the question which layer in PM framework should
> be responsible for driver notification in the pre/post change
> manner is for further discussion I consider that it's up to
> device driver to define what are steps to handle a transition.
> There is just no sense to extract that [essentially a driver's]
> functionality to any other layer. Thus, there indeed should
> be three phase for any transition from an operating point
> to an operating point but all we need is to notify interested
> party of the system about upcoming/completed change and
> this has nothing to do with something per operating point
> specific.
>
> The summary is that I don't see any reason to have that
> hooks to be defined per operating point.
>
> IMO clock/voltage framework layer should be responsible
> for issuing pre/post drivers notifications since a certain
> driver is tied with a particular clock(s)/voltage(s) and we
> know about the mapping at the point when a certain driver
> requests those resources. Such approach could allow to
> minimize overhead even comparing to the current separated
> pre/post lists of notifiers approach [in cpufreq for ex]
> because different drivers will monitor different and only
> chosen clock(s)/voltage(s) changes. Again I feel this like
> a topic for a separate discussion.
>
> Thanks,
> Eugeny
> >> In this way struct powerop_point _is_ part of PowerOP interface .
> >> Let me refer to the picture of proposed framework as well. PowerOP is
> >> interface between PM Core and any upper layer in the framework. Pushing
> >> power parameters definition down to PM Core interface (into some patch with
> >> a name related to pm core rather than to PowerOP) makes understanding
> >> of the framework layers much harder IMO.
> >>
> >
> > If the arch wants to expose parameters for a given operating point,
> > that'd be its own responsibility ... and trivial, there's lots of
> > utility code to do that.
> >
> >
> >
> >> If you are referring to the certain implementation let assume that I put
> >> "struct powerop_point;" instead of inclusion of 'asm/powrop.h' in
> >> include/linux/powerop.h file.
> >>
> >
> > I'd rather assume something as shown above ... something where it's
> > reasonable for the core to access the struct, if its declaration
> > must for whatever reason be visible to the core. :)
> >
> >
> >
> >> This way you get a compilable arch independent powerop core
> >> piece but let me ask what for? To allow arch independent powerop core
> >> to be standalone compilable?
> >>
> >
> > Call it "information hiding" or "clean interface design". There's
> > no reason for the core to know _anything_ about the arch-specific
> > details. You've set it up so they will, which means that the code
> > will probably evolve to try using that information. This should be
> > a loosely coupled interface, not a tightly coupled one.
> >
> > One artifact of an effective loosely coupled interface design is
> > that it's easy to completely revamp the implementation of one of
> > the coupled components without changing the other. In this case,
> > one component is (minimal) core code, the other is platform specific
> > code implementing each operating point.
> >
> >
> >
> >> It's just useless: if the arch you are
> >> building for
> >> does not implement definition of struct powerop_point you just defer
> >> compilation error to the link phase. Currently you can chose PowerOP
> >> core only if you chose an arch which implements arch dependent piece
> >> of PowerOP.
> >>
> >
> > It's very useful. How could you have modules defining new operating
> > points, with new parameters, with tight coupling? Surely it should
> > be possible to link every operating point except the initial "system
> > startup" point dynamically, using kernel modules?
> >
> > (That bootstrap issue needs looking at too. I think there may well
> > need to be an arch independent initial operating point. That's a
> > topic for a different thread though.)
> >
> >
> >
> >
> >>>>> - In general, shouldn't an operating point be board-specific, so
> >>>>> that the parts of the system outside the SOC can be included
> >>>>>
> >>>>>
> >>>>>
> >>>> good question. Basically current assumption is that definition is for an SoC
> >>>> and the values are board specific. While definition will most likely be the
> >>>> same for every board based on a certain SoC I can imaging for example
> >>>> that we can have an external clock source for an external hw on a board.
> >>>>
> >>>>
> >>> I agree that parts of an OP will merit that approach. But ... the SOC
> >>> is not the only system component that needs managing, and it won't always
> >>> be practical to shuffle the others under the "device-specfic PM" tent.
> >>>
> >>>
> >>
> >> OK, I am almost ready to buy this per SoC and per baord-specific OP
> >> definition approach.
> >>
> >
> > Code can come later. :)
> >
> >
> >
> >> But let me ask first whether you have at least one example of
> >> a platform which fits into this model nowadays?
> >>
> >
> > Certainly. Any two boards using the same SOC but different
> > external circuitry would naturally fit that model ... be they
> > OMAP boards, or PXA ones, or Atmel ones, etc.
> >
> >
> >
> >>>> Since that powerop_point structure definition could be board specific
> >>>> but that's where I'd prefer to get some input from the community to
> >>>> decide whether we have to move to board specific operating point
> >>>> structure definition.
> >>>>
> >>>>
> >>> My input: make it easy to partition things into components. One way
> >>> to do that might be to have an SOC component, multiple device components,
> >>> and a board-specific glue component that connects them in the right way.
> >>>
> >>>
> >>>
> >> please elaborate multiple device components.
> >>
> >
> > Considering only OMAP boards ... there are a variety of different
> > power management chips, audio chips, touchscreen controllers, and
> > backlight arrangements. It's reasonable to expect that two points
> > differ in which of those may be active.
> >
> > - Dave
> >
> >
> >
> >
>
>
>
> _______________________________________________
> linux-pm mailing list
> linux-pm at lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/linux-pm
>
>
>
>
-------------- next part --------------
diff --git a/drivers/Makefile b/drivers/Makefile
index fc2d744..f8eaf31 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_ISDN) += isdn/
obj-$(CONFIG_EDAC) += edac/
obj-$(CONFIG_MCA) += mca/
obj-$(CONFIG_EISA) += eisa/
+obj-$(CONFIG_POWEROP) += powerop/
obj-$(CONFIG_CPU_FREQ) += cpufreq/
obj-$(CONFIG_MMC) += mmc/
obj-$(CONFIG_NEW_LEDS) += leds/
diff --git a/drivers/powerop/Kconfig b/drivers/powerop/Kconfig
new file mode 100644
index 0000000..94d2459
--- /dev/null
+++ b/drivers/powerop/Kconfig
@@ -0,0 +1,12 @@
+#
+# powerop
+#
+
+menu "PowerOP (Power Management)"
+
+config POWEROP
+ bool "PowerOP Core"
+ help
+
+endmenu
+
diff --git a/drivers/powerop/Makefile b/drivers/powerop/Makefile
new file mode 100644
index 0000000..131b983
--- /dev/null
+++ b/drivers/powerop/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_POWEROP) += powerop.o
+
diff --git a/drivers/powerop/powerop.c b/drivers/powerop/powerop.c
new file mode 100644
index 0000000..e75a14d
--- /dev/null
+++ b/drivers/powerop/powerop.c
@@ -0,0 +1,315 @@
+/*
+ * PowerOP Core routines
+ *
+ * Author: Todd Poynor <tpoynor at mvista.com>
+ * Interface update by Eugeny S. Mints <eugeny.mints at gmail.com>
+ *
+ * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/powerop.h>
+
+static struct powerop_driver *powerop_driver;
+
+/* list of named operating points maintained by PowerOP Core layer */
+static struct list_head named_opt_list;
+static DECLARE_MUTEX(named_opt_list_mutex);
+static int powerop_init;
+
+struct namedop {
+ struct powerop_point point;
+ struct list_head node;
+};
+
+/* hw access serialization */
+static DECLARE_MUTEX(powerop_mutex);
+
+/* Forward declaration */
+int powerop_set_point(struct powerop_point *point);
+
+int
+powerop_driver_register(struct powerop_driver *p)
+{
+ int error = 0;
+
+ if (! powerop_driver) {
+ printk(KERN_INFO "PowerOP registering driver %s.\n", p->name);
+ powerop_driver = p;
+
+ } else
+ error = -EBUSY;
+
+ return error;
+}
+
+void
+powerop_driver_unregister(struct powerop_driver *p)
+{
+ if (powerop_driver == p) {
+ printk(KERN_INFO "PowerOP unregistering driver %s.\n", p->name);
+ powerop_driver = NULL;
+ }
+}
+
+EXPORT_SYMBOL_GPL(powerop_driver_register);
+EXPORT_SYMBOL_GPL(powerop_driver_unregister);
+
+/*
+ * powerop_register_named_point - add new operating point with a given name to
+ * operating points list
+ *
+ * INPUT
+ * id - operating point name
+ * md_opt - set of power parameters value
+ *
+ * OUTPUT
+ * none
+ *
+ * RETURN
+ * zero on success, error code otherwise
+ *
+ */
+int
+powerop_register_named_point(const char *id, void *md_opt)
+{
+ struct namedop *opt;
+ int error;
+
+ if ((!powerop_init) || (id == NULL) || (md_opt == NULL))
+ return -EINVAL;
+
+ if ((opt = kmalloc(sizeof(struct namedop), GFP_KERNEL)) == NULL)
+ return -ENOMEM;
+
+ memset(op, 0, sizeof(struct namedop));
+ kobject_set_name(&opt->point.kobj, id);
+ opt->point.md_opt = md_opt;
+
+ down(&named_opt_list_mutex);
+ list_add_tail(&opt->node, &named_opt_list);
+ up(&named_opt_list_mutex);
+
+ return 0;
+}
+
+/*
+ * powerop_unregister_named_point - search for operating point with specified
+ * name and remove it from operating points list
+ *
+ * INPUT
+ * id - name of operating point
+ *
+ * OUTPUT
+ * none
+ *
+ * RETURN
+ * zero on success, -EINVAL if no operating point is found
+ *
+ */
+int
+powerop_unregister_named_point(const char *id)
+{
+ struct namedop *opt, *tmpopt;
+ int ret = -EINVAL;
+
+ if ((!powerop_init) || (id == NULL))
+ return ret;
+
+ down(&named_opt_list_mutex);
+
+ list_for_each_entry_safe(opt, tmpopt, &named_opt_list, node) {
+ if (strcmp(op->kobj.name, id) == 0) {
+ list_del(&opt->node);
+ kfree(opt);
+ ret = 0;
+ break;
+ }
+ }
+
+ up(&named_opt_list_mutex);
+
+ return ret;
+}
+
+/*
+ * powerop_set_named_point - search for operating point with specified name
+ * and switch the system to the specified operating point
+ *
+ * INPUT
+ * id - name of operating point
+ *
+ * OUTPUT
+ * none
+ *
+ * RETURN
+ * zero on success
+ * -EINVAL if no operating point is found or error code otherwise
+ */
+int
+powerop_set_named_point(const char *id)
+{
+ struct namedop *opt, *selected_opt = NULL;
+ int ret;
+
+ if ((!powerop_init) || (id == NULL))
+ return ret;
+
+ down(&named_opt_list_mutex);
+
+ list_for_each_entry(opt, &named_opt_list, node) {
+ if (strcmp(opt->kobj.name, id) == 0) {
+ selected_opt = opt;
+ break;
+ }
+ }
+
+ ret = (selected_opt == NULL) ?
+ -EINVAL : powerop_set_point(opt->point);
+
+ up(&named_opt_list_mutex);
+
+ return ret;
+}
+
+/*
+ * powerop_get_named_point - search for operating point with specified name
+ * and return value of power parameters corresponding to the operating point
+ *
+ * INPUT
+ * id - name of operating point
+ *
+ * OUTPUT
+ * md_opt - power parameter values
+ *
+ * RETURN
+ * zero on success, -EINVAL if no operating point is found
+ */
+int
+powerop_get_named_point(const char *id, void *md_opt)
+{
+ int ret = -EINVAL;
+ struct namedop *opt;
+
+ if ((!powerop_init) || (id == NULL) || (md_opt == NULL))
+ return ret;
+
+ down(&named_opt_list_mutex);
+
+ list_for_each_entry(opt, &named_opt_list, node) {
+ if (strcmp(opt->kobj.name, id) == 0) {
+ md_opt = opt->point->md_opt;
+ ret = 0;
+ break;
+ }
+ }
+
+ up(&named_opt_list_mutex);
+
+ return ret;
+}
+
+EXPORT_SYMBOL_GPL(powerop_register_named_point);
+EXPORT_SYMBOL_GPL(powerop_unregister_named_point);
+EXPORT_SYMBOL_GPL(powerop_set_named_point);
+EXPORT_SYMBOL_GPL(powerop_get_named_point);
+
+/*
+ * powerop_set_point - switch the system to new operating point
+ *
+ * INPUT:
+ * point - operating point to set
+ *
+ * OUTPUT:
+ * none
+ *
+ * RETURN:
+ * zero on success, error code otherwise
+ *
+ * NOTES:
+ * kobj field of 'point' is ignored
+ */
+int
+powerop_set_point(struct powerop_point *point)
+{
+ int rc;
+
+ down(&powerop_mutex);
+ rc = point && point->md_opt && powerop_driver &&
+ powerop_driver->set_point ?
+ powerop_driver->set_point(point->md_opt) : -EINVAL;
+ up(&powerop_mutex);
+
+ return rc;
+}
+
+/*
+ * powerop_get_point - get current operating point the systems is
+ * running at
+ *
+ * INPUT:
+ * none
+ *
+ * OUTPUT:
+ * point - filled in power parameter values
+ *
+ * RETURN:
+ * zero on success, error code otherwise
+ *
+ * NOTES:
+ * 1) caller must allocate memory for md_opt field of 'point'
+ * 2) kobj field of 'point' is uninitialized on return
+ */
+int
+powerop_get_point(struct powerop_point *point)
+{
+ int rc;
+
+ if (point->md_opt == NULL)
+ return -EINVAL;
+
+ down(&powerop_mutex);
+ rc = point && point->md_opt && powerop_driver &&
+ powerop_driver->get_point ?
+ powerop_driver->get_point(point->md_opt) : -EINVAL;
+ up(&powerop_mutex);
+
+ return rc;
+}
+
+EXPORT_SYMBOL_GPL(powerop_set_point);
+EXPORT_SYMBOL_GPL(powerop_get_point);
+
+static int __init powerop_init(void)
+{
+ INIT_LIST_HEAD(&named_opt_list);
+ powerop_init = 1;
+
+ return 0;
+}
+
+static void __exit powerop_exit(void)
+{
+ struct namedop *opt, *tmp_opt;
+
+ down(&named_opt_list_mutex);
+
+ list_for_each_entry_safe(opt, tmp_opt, &named_opt_list, node) {
+ list_del(&opt->node);
+ kfree(opt);
+ }
+
+ up(named_opt_list_mutex);
+}
+
+module_init(powerop_init);
+module_exit(powerop_exit);
+
+MODULE_DESCRIPTION("PowerOP Core");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/powerop.h b/include/linux/powerop.h
new file mode 100644
index 0000000..1b1c233
--- /dev/null
+++ b/include/linux/powerop.h
@@ -0,0 +1,43 @@
+/*
+ * PowerOP core definitions
+ *
+ * Author: Todd Poynor <tpoynor at mvista.com>
+ * Interface update by Eugeny S. Mints <eugeny.mints at gmail.com>
+ *
+ * 2005 (c) MontaVista Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#ifndef __POWEROP_H__
+#define __POWEROP_H__
+
+struct powerop_point {
+ struct kobject kobj; /* hook to reference an operating point in
+ * some arch independent way
+ */
+ void *md_opt; /* arch dependent set of power parameters */
+};
+
+struct powerop_driver {
+ char *name;
+ int (*set_point)(void *md_opt);
+ int (*get_point)(void *md_opt);
+};
+
+/* Interface to an arch PM Core layer */
+int powerop_driver_register(struct powerop_driver *p);
+void powerop_driver_unregister(struct powerop_driver *p);
+
+/* Interface to control/access operating points by name */
+int powerop_register_named_point(const char *id, void *md_opt);
+int powerop_unregister_named_point(const char *id);
+int powerop_set_named_point(const char *id);
+int powerop_get_named_point(const char *id, void *md_opt);
+
+/* Direct interface to set/get operating points */
+int powerop_set_point(powerop_point *opt);
+int powerop_get_point(powerop_point *opt);
+
+#endif /* __POWEROP_H__ */
+
