[RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[Eugeny S. Mints]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

PowerOP support for OMAP1 platforms.  Currently handles these power
parameters:

  v                 voltage (in mV)
  dpll              dpll frequency in kHz
  cpu              cpu frequency in kHz
  tc                 traffic controller frequency in kHz
  per               peripherial frequency in kHz
  dsp              dsp frequency in kHz
  dspmmu      dsp mmu frequency in kHz
  lcd               LCD frequency in kHz

Example usage will be shown with a follow-on OMAP1 PM Core and cpufreq
patches.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: OMAP1.arch.PowerOP.support.patch --]
[-- Type: text/x-patch; name="OMAP1.arch.PowerOP.support.patch", Size: 1816 bytes --]

diff --git a/include/asm-arm/arch-omap/powerop.h b/include/asm-arm/arch-omap/powerop.h
new file mode 100644
index 0000000..391e165
--- /dev/null
+++ b/include/asm-arm/arch-omap/powerop.h
@@ -0,0 +1,34 @@
+/*
+ * PowerOP support for OMAP1
+ *
+ * Based on DPM OMAP code by Matthew Locke, Dmitry Chigirev, Vladimir
+ * Barinov, and Todd Poynor.
+ *
+ * 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 __ASM_ARCH_OMAP_POWEROP_H__
+#define __ASM_ARCH_OMAP_POWEROP_H__
+
+/* 
+ * Instances of this structure define operating points
+ *
+ * -1 for any following fields indicates no change from current op 
+ */
+
+struct powerop_point {
+	unsigned int v;         /* voltage in mV */
+	unsigned int dpll;	/* in KHz */
+	unsigned int cpu;	/* CPU frequency in KHz */
+	unsigned int tc;	/* in KHz */
+	unsigned int per;	/* in KHz */
+	unsigned int dsp;	/* in KHz */
+	unsigned int dspmmu;	/* in KHz */
+	unsigned int lcd;	/* in KHz */
+};
+
+#endif /* __ASM_ARCH_OMAP_POWEROP_H__ */
+
diff --git a/include/asm-arm/powerop.h b/include/asm-arm/powerop.h
new file mode 100644
index 0000000..c1090e6
--- /dev/null
+++ b/include/asm-arm/powerop.h
@@ -0,0 +1,19 @@
+/*
+ * include/asm-arm/powerop.h
+ *
+ * Copyright 2006 (c) Eugeny S. Mints <eugeny.mints@gmail.com>. 
+ * 
+ * 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 __ASM_POWEROP_H__
+#define __ASM_POWEROP_H__
+
+/*
+ * include sub-arch specific bits
+ */
+#include <asm/arch/powerop.h>
+
+#endif /* __ASM_POWEROP_H__ */
+

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[David Brownell]

On Thursday 20 July 2006 1:01 pm, Eugeny S. Mints wrote:
> +struct powerop_point {
> +       unsigned int v;         /* voltage in mV */
> +       unsigned int dpll;      /* in KHz */
> +       unsigned int cpu;       /* CPU frequency in KHz */
> +       unsigned int tc;        /* in KHz */
> +       unsigned int per;       /* in KHz */
> +       unsigned int dsp;       /* in KHz */
> +       unsigned int dspmmu;    /* in KHz */
> +       unsigned int lcd;       /* in KHz */
> +};

A few comments:

 - This should be part of patch #4; it's not truly separate.

 - I take it "v" is CPU voltage rather than some random component?
   Either way, there seems to be an omission here since boards
   could have multiple voltages to care about ...

 - In general, shouldn't an operating point be board-specific, so
   that the parts of the system outside the SOC can be included?

 - I'd still rather see operating points be identified by a name
   string of some kind so that the userspace API resembles that
   of /sys/power/state:  just write the state name to that file.

Still looking at the patches, otherwise.

- Dave
  

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[Eugeny S. Mints]

David Brownell wrote:
> On Thursday 20 July 2006 1:01 pm, Eugeny S. Mints wrote:
>   
>> +struct powerop_point {
>> +       unsigned int v;         /* voltage in mV */
>> +       unsigned int dpll;      /* in KHz */
>> +       unsigned int cpu;       /* CPU frequency in KHz */
>> +       unsigned int tc;        /* in KHz */
>> +       unsigned int per;       /* in KHz */
>> +       unsigned int dsp;       /* in KHz */
>> +       unsigned int dspmmu;    /* in KHz */
>> +       unsigned int lcd;       /* in KHz */
>> +};
>>     
>
> A few comments:
>
>  - This should be part of patch #4; it's not truly separate.
>   
PowerOP defines interface between an upper layer and PM core
and struct powerop_point is part of this interface - it defines what
the terms an upper layer and PM Core use to communicate to
each other are. And from this perspective I feel struct
powerop_point  logically belongs to PowerOP layer although
it is defined in arch dependent code.
>  - I take it "v" is CPU voltage rather than some random component?
>   
yes
>    Either way, there seems to be an omission here since boards
>    could have multiple voltages to care about ...
>   
see reply under the next bullet discussing board vs SoC but basically yes,
if we have multiple voltages to care about all voltages have to be
represented in the structure. Reference code structure definition may
be incomplete.
>  - 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.
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.
>  - I'd still rather see operating points be identified by a name
>    string of some kind so that the userspace API resembles that
>    of /sys/power/state:  just write the state name to that file.
>
>   
it's actully designed this way except that an operating point should be
created first to be dereferenced by name. PowerOP sysfs layer
provides two interfaces for operating points to be created. First one is
powerop_register_point() and this interface enables exactly runtime
API you outlined assuming that a kernel entity creates a set of
operating points with help of power_register_point() for example
at boot up.  Second real sysfs interface assumes that a set of operating
 points is created from user space. But since operating points initial
creation may be handled by an init script (again say at system boot up)
the PowerOP runtime API may be again seen as what you described.

Thanks,
Eugeny
> Still looking at the patches, otherwise.
>
> - Dave
>   
>
>
>   

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[David Brownell]

On Wednesday 26 July 2006 2:02 pm, Eugeny S. Mints wrote:
> David Brownell wrote:

> >  - This should be part of patch #4; it's not truly separate.
> >   
> PowerOP defines interface between an upper layer and PM core
> and struct powerop_point is part of this interface - it defines what
> the terms an upper layer and PM Core use to communicate to
> each other are. And from this perspective I feel struct
> powerop_point  logically belongs to PowerOP layer although
> it is defined in arch dependent code.

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.


> >  - I take it "v" is CPU voltage rather than some random component?
> >   
> yes
> >    Either way, there seems to be an omission here since boards
> >    could have multiple voltages to care about ...
> >   
> see reply under the next bullet discussing board vs SoC but basically yes,
> if we have multiple voltages to care about all voltages have to be
> represented in the structure. Reference code structure definition may
> be incomplete.

I don't think "complete" is achievable then.  This is exactly where
there will need to be component boundaries:  there must be OMAP1 specific
componentry (with some chip-specific nuances), and also there must be
board specific componentry.

Your patch #4 for example had #ifdeffery for specific boards.  ISTR
that was wrong -- it should have been chip specfic nuances, 17xx parts
vs 16xx ones -- but still there _would_ be a need for boar-specific
components in a real/usable/complete system.


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


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

- Dave

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[Eugeny S. Mints]

David Brownell wrote:
> On Wednesday 26 July 2006 2:02 pm, Eugeny S. Mints wrote:
>   
>> David Brownell wrote:
>>     
>
>   
>>>  - This should be part of patch #4; it's not truly separate.
>>>   
>>>       
>> PowerOP defines interface between an upper layer and PM core
>> and struct powerop_point is part of this interface - it defines what
>> the terms an upper layer and PM Core use to communicate to
>> each other are. And from this perspective I feel struct
>> powerop_point  logically belongs to PowerOP layer although
>> it is defined in arch dependent code.
>>     
>
> 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.

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

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

Plus, in such arch files as pm_core.c and cpu-freq.c you will need 
explicitly
include both arch independent and arch dependent pieces of PowerOP.
While it could be fine with pm core I don't like it for layers above PowerOP
as I explained above.

The bottom line is that yes, PowerOP consists of two part -- arch 
independent
and arch dependent -- but it's still is one layer and one interface 
between PM
Core layer and any layer above PowerOP for _all_ platforms.
>   
>>>  - I take it "v" is CPU voltage rather than some random component?
>>>   
>>>       
>> yes
>>     
>>>    Either way, there seems to be an omission here since boards
>>>    could have multiple voltages to care about ...
>>>   
>>>       
>> see reply under the next bullet discussing board vs SoC but basically yes,
>> if we have multiple voltages to care about all voltages have to be
>> represented in the structure. Reference code structure definition may
>> be incomplete.
>>     
>
> I don't think "complete" is achievable then.  This is exactly where
> there will need to be component boundaries:  there must be OMAP1 specific
> componentry (with some chip-specific nuances), and also there must be
> board specific componentry.
>
> Your patch #4 for example had #ifdeffery for specific boards.  ISTR
> that was wrong -- it should have been chip specfic nuances, 17xx parts
> vs 16xx ones -- but still there _would_ be a need for boar-specific
> components in a real/usable/complete system.
>
>   

>>>  - 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. But let me ask first whether you have at least one example of 
a platform
which fits into this model nowadays?
>> 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.

Thanks,
Eugeny
> - Dave
>
>
>   

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[David Brownell]

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


> 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

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[Vitaly Wool]

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

I do agree with David here (oh my, I'm in agreement with David on
something, it's unbelievable ;)

Vitaly

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[David Brownell]

On Sunday 30 July 2006 11:59 pm, Vitaly Wool wrote:
> > > 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.
> 
> I do agree with David here (oh my, I'm in agreement with David on
> something, it's unbelievable ;)

We're allowed to agree when we're both right.  :)

Nitpick to my explanation:  that kobject would represent the sysfs
directory for that set of operating points, not the arch.

- Dave

Core PowerOP Interface Update [Was: Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5]

[Eugeny S. Mints]

[-- Attachment #1: Type: text/plain, Size: 8563 bytes --]

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


[-- Attachment #2: powerop.h.interface.revised.patch --]
[-- Type: text/x-patch, Size: 1410 bytes --]

diff --git a/include/linux/powerop.h b/include/linux/powerop.h
new file mode 100644
index 0000000..eebff76
--- /dev/null
+++ b/include/linux/powerop.h
@@ -0,0 +1,40 @@
+/*
+ * PowerOP core definitions
+ *
+ * Author: Todd Poynor <tpoynor@mvista.com>
+ * Interface update by Eugeny S. Mints <eugeny.mints@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_named_op {
+	struct kobject  kobj;   /* hook to reference an op by a name */
+	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 upper PMF layers */
+int powerop_set_point(powerop_named_op *opt);
+int powerop_get_point(powerop_named_op *opt);
+
+/* Interface to an arch PM core */
+int powerop_driver_register(struct powerop_driver *p);
+void powerop_driver_unregister(struct powerop_driver *p);
+
+/* Interface to handle named OP by PowerOP core layer */
+int powerop_register_point(const char *id, void *md_opt);
+int powerop_unregister_point(const char *id);
+int powerop_select_point(const char *id);
+
+#endif /* __POWEROP_H__ */
+


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: Core PowerOP Interface Update [Was: Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5]

[Eugeny S. Mints]

[-- Attachment #1: Type: text/plain, Size: 9404 bytes --]

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@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@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/linux-pm
>
>
>
>

[-- Attachment #2: powerop.core.patch --]
[-- Type: text/plain, Size: 9283 bytes --]

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@mvista.com>
+ * Interface update by Eugeny S. Mints <eugeny.mints@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@mvista.com>
+ * Interface update by Eugeny S. Mints <eugeny.mints@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__ */
+

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: Core PowerOP Interface Update [Was: Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5]

[Vitaly Wool]

Eugeny,

On 8/3/06, Eugeny S. Mints <eugeny.mints@gmail.com> wrote:
> 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.

Will anyone except drivers/powerop/powerop.c be using
powerop_set_point/powerop_get_point? If yes, why?If no, then why make
them public?

Thanks,
   Vitaly

Re: Core PowerOP Interface Update [Was: Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5]

[Eugeny S. Mints]

Vitaly Wool wrote:
> Eugeny,
>
> On 8/3/06, Eugeny S. Mints <eugeny.mints@gmail.com> wrote:
>> 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.
>
> Will anyone except drivers/powerop/powerop.c be using
> powerop_set_point/powerop_get_point? If yes, why?
The main reason is that a layer above PowerOP Core may want:
a) to use it's own method to reference operating points
and
b) implement another algorithm to maintain set of operating points
rather than use simple list algorithm provided by PowerOP Core
(for example due to performance reasons).

I assume that b) may be achieved in the future by implementing a
kind of algorithm plugins for PowerOP Core but  this will not
change already existed api and therefore this improvement may
be deferred for the time being.

The other minor reason is to allow smooth evolve of existed upper
layers (cpufreq): using powerop_set/get on the first step will require
less modification of existed code than leveraging named api .

It's reasonable to have powerop_get_point() exported for development
purposes as well since reasonable implementation of
powerop_get_named_active_opint() implies returning the result without
accessing underlying hw.

Thanks,
Eugeny
> If no, then why make
> them public?
>
> Thanks,
>   Vitaly
>

Re: [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5

[Gross, Mark]

>-----Original Message-----
>From: linux-pm-bounces@lists.osdl.org [mailto:linux-pm-bounces@lists.osdl.org] On Behalf Of David
>Brownell
>Sent: Sunday, July 23, 2006 9:25 AM
>To: linux-pm@lists.osdl.org
>Cc: Matthew Locke; Mochel, Patrick; sampsa.fabritius@nokia.com; linux@dominikbrodowski.net
>Subject: Re: [linux-pm] [RFC] PowerOP Take 3, ARM OMAP1 platform support 3/5
>
>On Thursday 20 July 2006 1:01 pm, Eugeny S. Mints wrote:
>> +struct powerop_point {
>> +       unsigned int v;         /* voltage in mV */
>> +       unsigned int dpll;      /* in KHz */
>> +       unsigned int cpu;       /* CPU frequency in KHz */
>> +       unsigned int tc;        /* in KHz */
>> +       unsigned int per;       /* in KHz */
>> +       unsigned int dsp;       /* in KHz */
>> +       unsigned int dspmmu;    /* in KHz */
>> +       unsigned int lcd;       /* in KHz */
>> +};
>
>A few comments:
>
> - This should be part of patch #4; it's not truly separate.
>
> - I take it "v" is CPU voltage rather than some random component?
>   Either way, there seems to be an omission here since boards
>   could have multiple voltages to care about ...

I think the components be exposed in a stand alone manner in addition to the structure.  Also, could we have a driver module per element to expose each one to the system and user.  That way you can just load the drivers for the control knobs you care about, and add more for your platform more easily.

>
> - In general, shouldn't an operating point be board-specific, so
>   that the parts of the system outside the SOC can be included?
>
> - I'd still rather see operating points be identified by a name
>   string of some kind so that the userspace API resembles that
>   of /sys/power/state:  just write the state name to that file.
>

What would you name an n-tuple of integers?  I can't think how this would work.

--mgross

>Still looking at the patches, otherwise.
>

Me too...