Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Mark Brown]

On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:

> +struct opp_def {
> +	unsigned long freq;
> +	unsigned long u_volt;
> +
> +	bool enabled;
> +};

It might be clearer to use some term other than enabled in the code -
when reading I wasn't immediately sure if enabled meant that it was
available to be selected or if it was the active operating point.  How
about 'allowed' (though I'm not 100% happy with that)?

> +static inline int opp_add(struct device *dev, const struct opp_def *opp_def)
> +{
> +	return ERR_PTR(-EINVAL);
> +}

Mismatch with the return type and value here.

> +/**
> + * opp_enable() - Enable a specific OPP
> + * @opp:	Pointer to opp
> + *
> + * Enables a provided opp. If the operation is valid, this returns 0, else the
> + * corresponding error value.
> + *
> + * OPP used here is from the the opp_is_valid/opp_has_freq or other search
> + * functions
> + */
> +int opp_enable(struct opp *opp)
> +{
> +	if (unlikely(!opp || IS_ERR(opp))) {
> +		pr_err("%s: Invalid parameters being passed\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!opp->enabled && opp->dev_opp)
> +		opp->dev_opp->enabled_opp_count++;
> +
> +	opp->enabled = true;
> +
> +	return 0;
> +}

When reading the description I'd expected to see some facility to
trigger selection of an active operating point in the library (possibly
as a separate call since you might have a bunch of operating points
being updated in quick succession) but it looks like that needs to be
supplied externally at the minute?

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Nishanth Menon]

Mark Brown had written, on 09/17/2010 10:36 AM, the following:
> On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
> 
>> +struct opp_def {
>> +	unsigned long freq;
>> +	unsigned long u_volt;
>> +
>> +	bool enabled;
>> +};
> 
> It might be clearer to use some term other than enabled in the code -
> when reading I wasn't immediately sure if enabled meant that it was
> available to be selected or if it was the active operating point.  How
> about 'allowed' (though I'm not 100% happy with that)?
;).. The opp is enabled or disabled if it is populated, it is implicit 
as being available but not enabled- how about active? this would change 
the opp_enable/disable functions to opp_activate, opp_deactivate..

Recommendations folks?

> 
>> +static inline int opp_add(struct device *dev, const struct opp_def *opp_def)
>> +{
>> +	return ERR_PTR(-EINVAL);
>> +}
> 
> Mismatch with the return type and value here.
/me kicks himself.. ouch.. thanks.. will fix in v2.

> 
>> +/**
>> + * opp_enable() - Enable a specific OPP
>> + * @opp:	Pointer to opp
>> + *
>> + * Enables a provided opp. If the operation is valid, this returns 0, else the
>> + * corresponding error value.
>> + *
>> + * OPP used here is from the the opp_is_valid/opp_has_freq or other search
>> + * functions
>> + */
>> +int opp_enable(struct opp *opp)
>> +{
>> +	if (unlikely(!opp || IS_ERR(opp))) {
>> +		pr_err("%s: Invalid parameters being passed\n", __func__);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!opp->enabled && opp->dev_opp)
>> +		opp->dev_opp->enabled_opp_count++;
>> +
>> +	opp->enabled = true;
>> +
>> +	return 0;
>> +}
> 
> When reading the description I'd expected to see some facility to
> trigger selection of an active operating point in the library (possibly
> as a separate call since you might have a bunch of operating points
> being updated in quick succession) but it looks like that needs to be
> supplied externally at the minute?
The intent is we use the opp_search* functions to pick up the opp and 
enable/activate it and disable/deactivate it.

-- 
Regards,
Nishanth Menon

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Mark Brown]

On Fri, Sep 17, 2010 at 10:53:06AM -0500, Nishanth Menon wrote:
> Mark Brown had written, on 09/17/2010 10:36 AM, the following:

> >It might be clearer to use some term other than enabled in the code -
> >when reading I wasn't immediately sure if enabled meant that it was
> >available to be selected or if it was the active operating point.  How
> >about 'allowed' (though I'm not 100% happy with that)?

> ;).. The opp is enabled or disabled if it is populated, it is
> implicit as being available but not enabled- how about active? this
> would change the opp_enable/disable functions to opp_activate,
> opp_deactivate..

> Recommendations folks?

The enable/disable thing wasn't so noticable in the API itself, it was
in the data structures that I found it confusing - the core has a
different idea about what's going on with the system as a whole compared
to the decision that an individual device is taking.

> >When reading the description I'd expected to see some facility to
> >trigger selection of an active operating point in the library (possibly
> >as a separate call since you might have a bunch of operating points
> >being updated in quick succession) but it looks like that needs to be
> >supplied externally at the minute?

> The intent is we use the opp_search* functions to pick up the opp
> and enable/activate it and disable/deactivate it.

Sure, I get that bit.  What I meant was that I was expecting something
that would say that changes had been made to the enabled/disabled sets
and that it'd be a good idea to rescan, especially for cases where the
devices change their requirements but the OPPs need to be done over a
larger block.

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Phil Carmody]

On 17/09/10 17:36 +0200, ext Mark Brown wrote:
> On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
> 
> > +struct opp_def {
> > +	unsigned long freq;
> > +	unsigned long u_volt;
> > +
> > +	bool enabled;
> > +};
> 
> It might be clearer to use some term other than enabled in the code -
> when reading I wasn't immediately sure if enabled meant that it was
> available to be selected or if it was the active operating point.  How
  ^^^^^^^^^
> about 'allowed' (though I'm not 100% happy with that)?

I think your query contains its own answer.

Phil

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Rafael J. Wysocki]

On Friday, September 17, 2010, Nishanth Menon wrote:
> Mark Brown had written, on 09/17/2010 10:36 AM, the following:
> > On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
> > 
> >> +struct opp_def {
> >> +	unsigned long freq;
> >> +	unsigned long u_volt;
> >> +
> >> +	bool enabled;
> >> +};
> > 
> > It might be clearer to use some term other than enabled in the code -
> > when reading I wasn't immediately sure if enabled meant that it was
> > available to be selected or if it was the active operating point.  How
> > about 'allowed' (though I'm not 100% happy with that)?
> ;).. The opp is enabled or disabled if it is populated, it is implicit 
> as being available but not enabled- how about active? this would change 
> the opp_enable/disable functions to opp_activate, opp_deactivate..

Would that mean that "active" is the one currently in use?

Rafael

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Nishanth Menon]

Rafael J. Wysocki had written, on 09/17/2010 05:22 PM, the following:
> On Friday, September 17, 2010, Nishanth Menon wrote:
>> Mark Brown had written, on 09/17/2010 10:36 AM, the following:
>>> On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
>>>
>>>> +struct opp_def {
>>>> +	unsigned long freq;
>>>> +	unsigned long u_volt;
>>>> +
>>>> +	bool enabled;
>>>> +};
>>> It might be clearer to use some term other than enabled in the code -
>>> when reading I wasn't immediately sure if enabled meant that it was
>>> available to be selected or if it was the active operating point.  How
>>> about 'allowed' (though I'm not 100% happy with that)?
>> ;).. The opp is enabled or disabled if it is populated, it is implicit 
>> as being available but not enabled- how about active? this would change 
>> the opp_enable/disable functions to opp_activate, opp_deactivate..
> 
> Would that mean that "active" is the one currently in use?

I like the idea Phil pointed out[1] on using "available" instead.. 
opp_enable and disable will make the OPP available or not. does this 
sound better?

[1] http://marc.info/?l=linux-arm-kernel&m=128474217132058&w=2
-- 
Regards,
Nishanth Menon

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Rafael J. Wysocki]

On Saturday, September 18, 2010, Nishanth Menon wrote:
> Rafael J. Wysocki had written, on 09/17/2010 05:22 PM, the following:
> > On Friday, September 17, 2010, Nishanth Menon wrote:
> >> Mark Brown had written, on 09/17/2010 10:36 AM, the following:
> >>> On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
> >>>
> >>>> +struct opp_def {
> >>>> +	unsigned long freq;
> >>>> +	unsigned long u_volt;
> >>>> +
> >>>> +	bool enabled;
> >>>> +};
> >>> It might be clearer to use some term other than enabled in the code -
> >>> when reading I wasn't immediately sure if enabled meant that it was
> >>> available to be selected or if it was the active operating point.  How
> >>> about 'allowed' (though I'm not 100% happy with that)?
> >> ;).. The opp is enabled or disabled if it is populated, it is implicit 
> >> as being available but not enabled- how about active? this would change 
> >> the opp_enable/disable functions to opp_activate, opp_deactivate..
> > 
> > Would that mean that "active" is the one currently in use?
> 
> I like the idea Phil pointed out[1] on using "available" instead.. 
> opp_enable and disable will make the OPP available or not. does this 
> sound better?

Yes, it does.

Rafael

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Kevin Hilman]

[trimmed Cc list a bit, as vger bounced my last reply due to header too long]

Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

> On Fri, Sep 17, 2010 at 10:53:06AM -0500, Nishanth Menon wrote:
>> Mark Brown had written, on 09/17/2010 10:36 AM, the following:
>
>> >It might be clearer to use some term other than enabled in the code -
>> >when reading I wasn't immediately sure if enabled meant that it was
>> >available to be selected or if it was the active operating point.  How
>> >about 'allowed' (though I'm not 100% happy with that)?
>
>> ;).. The opp is enabled or disabled if it is populated, it is
>> implicit as being available but not enabled- how about active? this
>> would change the opp_enable/disable functions to opp_activate,
>> opp_deactivate..
>
>> Recommendations folks?
>
> The enable/disable thing wasn't so noticable in the API itself, it was
> in the data structures that I found it confusing - the core has a
> different idea about what's going on with the system as a whole compared
> to the decision that an individual device is taking.

Can you clarify your confusion here?  I don't follow the problem you're
raising.

The enabled flag in the internal data structure is set to true when
opp_enable() is called and set to false when opp_disable() is called.
I'm not sure

At least as we're currently using it, this API has know knowlege of what
OPP is currently active on the system.  IOW, it has no idea what the
current frequency/voltage a given device is set to.  It's intended more
like an OPP database with some convience functions to search through the
OPPs and to make OPPs available (or not.)  The users of this API (in our
case, CPUfreq and voltage scaling code) are the ones which
keep track of which OPP is actually in use.

As I write this, I agree with what Phil pointed out; that 'available' is
probably the right name for this flag, instead of 'enabled.'

>> >When reading the description I'd expected to see some facility to
>> >trigger selection of an active operating point in the library (possibly
>> >as a separate call since you might have a bunch of operating points
>> >being updated in quick succession) but it looks like that needs to be
>> >supplied externally at the minute?
>
>> The intent is we use the opp_search* functions to pick up the opp
>> and enable/activate it and disable/deactivate it.
>
> Sure, I get that bit.  What I meant was that I was expecting something
> that would say that changes had been made to the enabled/disabled sets
> and that it'd be a good idea to rescan, especially for cases where the
> devices change their requirements but the OPPs need to be done over a
> larger block.

I guess you're thinking of notifiers, so if the list of available OPPs
changes, a driver could (re)search to see if a more optimal OPP was
available?

Certainly sounds possible, but not sure how useful in practice.  OPP
change decisions are not very often, so any OPP database updates may not
affect a device OPP change currently in progress, but would take effect
the next time that device makes an OPP change.

Either way, this is something that could easily be added if it seems
useful.

Kevin

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Mark Brown]

On Fri, Sep 17, 2010 at 05:37:06PM -0700, Kevin Hilman wrote:
> [trimmed Cc list a bit, as vger bounced my last reply due to header too long]
> Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

> > The enable/disable thing wasn't so noticable in the API itself, it was
> > in the data structures that I found it confusing - the core has a
> > different idea about what's going on with the system as a whole compared
> > to the decision that an individual device is taking.

> Can you clarify your confusion here?  I don't follow the problem you're
> raising.

The confusion is between the enable flag meaning that the operating
point is on the list that can be chosen from and it being the currently
active one.  It's clear once you understand what the API does but the
current naming makes that harder to grasp.

> As I write this, I agree with what Phil pointed out; that 'available' is
> probably the right name for this flag, instead of 'enabled.'

Yup, me too.

> > Sure, I get that bit.  What I meant was that I was expecting something
> > that would say that changes had been made to the enabled/disabled sets
> > and that it'd be a good idea to rescan, especially for cases where the
> > devices change their requirements but the OPPs need to be done over a
> > larger block.

> I guess you're thinking of notifiers, so if the list of available OPPs
> changes, a driver could (re)search to see if a more optimal OPP was
> available?

Yup, or at least some suggestion in the API for how this should be done.

> Certainly sounds possible, but not sure how useful in practice.  OPP
> change decisions are not very often, so any OPP database updates may not
> affect a device OPP change currently in progress, but would take effect
> the next time that device makes an OPP change.

Like I say, the main use case would be when the device itself is not
making the actual operating point decision but is feeding in to a wider
block - if the device implements the decision then it really shouldn't
need to notify itself about it, though I guess it might be handy.

> Either way, this is something that could easily be added if it seems
> useful.

My concern there would be divergent idioms for working with the API.
It'd seem better to start everyone off down the same path if we can
rather than have differences between platforms which make life harder
when moving between mulitple platforms.

Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs

[Mark Brown]

On Fri, Sep 17, 2010 at 07:45:43PM +0300, Phil Carmody wrote:
> On 17/09/10 17:36 +0200, ext Mark Brown wrote:

> > It might be clearer to use some term other than enabled in the code -
> > when reading I wasn't immediately sure if enabled meant that it was
> > available to be selected or if it was the active operating point.  How
>   ^^^^^^^^^
> > about 'allowed' (though I'm not 100% happy with that)?

> I think your query contains its own answer.

Yeah, I didn't pick that because all of them are available in the sense
that they could in the future be enabled but I do think it's the best
option.

Re: [PATCH] opp: introduce library for device-specific OPPs

[Rafael J. Wysocki]

[Trimming the CC list]

On Saturday, September 18, 2010, Nishanth Menon wrote:
> Rafael J. Wysocki had written, on 09/17/2010 05:45 PM, the following:
> 
> Thanks for your review. few views below..
> 
> > On Friday, September 17, 2010, Nishanth Menon wrote:
> >> Nishanth Menon had written, on 09/17/2010 10:05 AM, the following:
> >>> Linus Walleij had written, on 09/17/2010 08:41 AM, the following:
> >>>> 2010/9/17 Nishanth Menon <nm@ti.com>:
> >>>>
> >>>>> diff --git a/Documentation/power/00-INDEX b/Documentation/power/00-INDEX
> >>>>> index fb742c2..45e9d4a 100644
> >>>>> --- a/Documentation/power/00-INDEX
> >>>>> +++ b/Documentation/power/00-INDEX
> >>>>> @@ -14,6 +14,8 @@ interface.txt
> >>>>>        - Power management user interface in /sys/power
> >>>>>  notifiers.txt
> >>>>>        - Registering suspend notifiers in device drivers
> >>>>> +opp.txt
> >>>>> +       - Operating Performance Point library
> >>>>>  pci.txt
> >>>>>        - How the PCI Subsystem Does Power Management
> >>>> Hm you add the documentation to the index but not the documentation
> >>>> itself (easy slip) would you mind posting it?
> >>> ouch.. Sorry.. I realized that I missed adding MAINTAINER entry as 
> >>> well.. v2 coming up..
> >>>
> >> while the review goes on, I will till end of the day before posting a v2 
> >> will collated comments, here is the opp.txt documentation for the same.. 
> >> apologies on missing in my v1..
> > 
> > OK, I'm not generally familiar with these things, so a couple of questions.
> > 
> >> OPP Layer
> >> ==============
> >> SOCs have a standard set of tuples consisting of frequency and
> >> voltage pairs that the device will support per voltage domain. This
> >> is called Operating Performance Point or OPP. The actual definitions
> >> of OPP varies over silicon within the same family of devices.
> >> For a specific domain, you can have a set of {frequency, voltage}
> >> pairs. As the kernel boots and more information is available, a set
> >> of these are activated based on the precise nature of device the kernel
> >> boots up on.
> > 
> > Does it mean that the full set of possible OPPs is already known from the
> > hardware configuration and the ones that are actually useable are found
> > during boot?
> 
> yes. example - https://patchwork.kernel.org/patch/183742/ (based on 
> original patch posted to l-arm, this will need some tweaks with the 
> latest evolution, the concepts remain the same).
> 
> For example, consider in the patch above where we have a opp definitions 
> we can choose from omap3430 and 3630,
> 
> when using 3630 silicon as a specific, certain boards wish to enable 
> 1GHz, this allows us to do something as follows:
> in the generic omap code, we do init of all opps
> in the board specific file, we enable 1Ghz
> 
> The selection of oppset and actual availability is done on the fly.

OK, so why don't you simply drop the OPPs you're not going to use from the
list in the board-specific file?

...
> >> OPP layer organizes the data internally using device pointers representing
> >> individual voltage domains.
> > 
> > Can you please describe these data structures in a bit more detail?
> probably a dumb question: Is'nt it enough to give detailed verbose 
> information in the  struct comment header? why duplicate here as well.. 
> other than giving an overview?

I meant "explain it to me" rather than "explain it in the doc". :-)  Sorry for
not being clear enough.

> >> NOTE:
> >> a) the OPP layer implementation expects to be used in multiple contexts
> >> based on SOC implementation and recommends locking schemes appropriate to
> >> the usage of SOC.
> >> b) All pointer returns are expected to be handled by standard error checks
> >> such as IS_ERR() and appropriate actions taken by the caller.
> > 
> > I noticed that in a few places it isn't really necessary to return a pointer.
> > I think you can simply return int in such cases.
> could you help and point these to me. opp_find_freq_exact, 
> opp_find_freq_ceil, opp_find_freq_floor are the only ones that return 
> pointers and they need to return the pointer to the opp they found to 
> operate on them such as add_freq etc..

Hmm, OK.  I must have made a mistake, sorry.

...
> >> Data Structures:
> >> ---------------
> >> struct opp * is a handle structure whose internals are known only
> >> to the OPP layer and is meant to hide the complexity away from users of
> >> opp layer.
> >>
> >> struct opp_def * is the definitions that users can interface with
> >> opp layer and is meant to define one OPP for a domain device.
> > 
> > The data structures require some more description IMHO.  They aren't completely
> > intuitive.
> 
> ok, a repeat question -> why duplicate the information defined in the 
> structure comment header?

Because it doesn't imply the specific implementation, AFAICS.

Like what the list heads in your structures are used for etc.

Thanks,
Rafael

Re: [PATCH] opp: introduce library for device-specific OPPs

[Rafael J. Wysocki]

On Monday, September 20, 2010, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Monday, September 20, 2010, Kevin Hilman wrote:
> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >> 
> >> [...]
> >> 
> >> >> >> In terms of the lifetime rules on the nodes in the list:
> >> >> >> The list is expected to be maintained once created, entries are expected 
> >> >> >> to be added optimally and not expected to be destroyed, the choice of 
> >> >> >> list implementation was for reducing the complexity of the code itself 
> >> >> >> and not yet meant as a mechanism to dynamically add and delete nodes on 
> >> >> >> the fly.. Essentially, it was intended for the SOC framework to ensure 
> >> >> >> it plugs in the OPP entries optimally and not create a humongous list of 
> >> >> >> all possible OPPs for all families of the vendor SOCs - even though it 
> >> >> >> is possible to use the OPP layer so - it just wont be smart to do so 
> >> >> >> considering list scan latencies on hot paths such as cpufreq transitions 
> >> >> >> or idle transitions.
> >> >> > 
> >> >> > If the list nodes are not supposed to be added and removed dynamically,
> >> >> > it probably would make sense to create data structures containing
> >> >> > the "available" OPPs only, once they are known, and simply free the object
> >> >> > representing the other ones.
> >> >> I covered the usage in my reply here: 
> >> >> http://marc.info/?l=linux-arm-kernel&m=128476570300466&w=2
> >> >> but to repeat, the list is dynamic during initialization but remains 
> >> >> static after initialization based on SOC framework implementation - this 
> >> >> is best implemented with a list (we had started with an original array 
> >> >> implementation which evolved to the current list implementation 
> >> >> http://marc.info/?l=linux-omap&m=125912217718770&w=2)
> >> >
> >> > Well, my point is, since the _final_ set of OPPs doesn't really
> >> > change, there's no need to use a list for storing it in principle.
> >> >
> >> > Your current algorithm seems to be:
> >> > (1) Create a list of all _possible_ OPPs.
> >> > (2) Mark the ones that can actually be used on the given hardware as
> >> >     "available".
> >> > (3) Whenever we need to find an OPP to use, browse the entire list.
> >> > This isn't optimal, because the OPPs that are not marked as "available" in (2)
> >> > will never be used, although they _will_ be inspected while browsing the list.
> >> 
> >> A little clarificaion about "will never be used" below...
> >> 
> >> > So, I think a better algorithm would be:
> >> > (1) Create a list of all possible OPPs.
> >> > (2) Drop the nonavailable OPPs from the list.
> >> > (3) Whenever we need to find an OPP to use, browse the entire list.
> >> >
> >> > But then, it may be better to simply move the list we get in (2) into an
> >> > array, because the browsing is going to require fewer memory accesses in
> >> > that case (also, an array would use less memory than the list).  So, perhaps,
> >> > it's better to change the algorithm even further:
> >> > (1) Create a list of all possible OPPs.
> >> > (2) Drop the nonavailable OPPs from the list.
> >> > (3) Move the list we got in (2) into a sorted array.
> >> > (4) Whenever we need to find an OPP to use, browse the array
> >> >      (perhaps using binary search).
> >> 
> >> Just a little clarification on "available."  The intended use of this flag
> >> was not just a one-time "available on hardware X."  It was also intended
> >> to be able to add/remove availbale OPPs dynamically at run-time.
> >> 
> >> More specifically, it's intended for use to *temporarily* remove an OPP
> >> from being selected.  The production usage of this would primarily for
> >> thermal considerations (e.g. don't use OPPx until the temperature drops)
> >> 
> >> However, for PM development & debug, we also use this to temporarily
> >> take a class of OPPs out of the running for test/debug purposes
> >> (e.g. driver X runs great at OPPx and OPPy, but not OPPz.)  So the
> >> ability to temporarily be selective about OPPs at runtime for
> >> debug/development is extremely useful.
> >> 
> >> So, to summarize, "most of the time", all the OPPs that were added (via
> >> opp_add()) will be "available".  Ones that are !availble will likely
> >> only be so temporarily, so I'm not sure that the overhead of keeping a
> >> separate structure for the available and !available OPPs is worth it.
> >> Especially, since OPP changes are relatively infrequent.
> >
> > Well, the Nishanth's description doesn't match this, so thanks for the
> > clarification.
> 
> Agreed, we need to update the doc file to reflect this.
> 
> > In that case you might consider using a red-black tree for storing the
> > "available" OPPs, so that you can add-remove them dynamically, but
> > you can avoid a linear search through the entire list every time you need to
> > find and available OPP.  Since we have standard helpers for handling rbtrees,
> > that shouldn't be a big deal.
> 
> That's a possibility, but do you think rbtrees are worth it for a
> relatively small number of OPPs for any given device?  We're using this
> to track a list of OPPs for any struct device, so there may be multiple
> independent OPP lists, but each would have a small number of OPPs.
> 
> For example, on OMAP, while the CPU might have a larger number of OPPs
> (5-6), most devices will have a small number of OPPs (1-3.)  I gather
> this is similar for many of the embedded SoCs available today.
> 
> Considering such a small number of OPPs, is the extra complexity of an
> rbtree worth it?

OK, probably not.  If there's so few of them, I agree that using lists is
probably better, but please put the numbers information into the doc too. :-)

Thanks,
Rafael

Re: [PATCH] opp: introduce library for device-specific OPPs

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Monday, September 20, 2010, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> [...]
>> 
>> >> >> In terms of the lifetime rules on the nodes in the list:
>> >> >> The list is expected to be maintained once created, entries are expected 
>> >> >> to be added optimally and not expected to be destroyed, the choice of 
>> >> >> list implementation was for reducing the complexity of the code itself 
>> >> >> and not yet meant as a mechanism to dynamically add and delete nodes on 
>> >> >> the fly.. Essentially, it was intended for the SOC framework to ensure 
>> >> >> it plugs in the OPP entries optimally and not create a humongous list of 
>> >> >> all possible OPPs for all families of the vendor SOCs - even though it 
>> >> >> is possible to use the OPP layer so - it just wont be smart to do so 
>> >> >> considering list scan latencies on hot paths such as cpufreq transitions 
>> >> >> or idle transitions.
>> >> > 
>> >> > If the list nodes are not supposed to be added and removed dynamically,
>> >> > it probably would make sense to create data structures containing
>> >> > the "available" OPPs only, once they are known, and simply free the object
>> >> > representing the other ones.
>> >> I covered the usage in my reply here: 
>> >> http://marc.info/?l=linux-arm-kernel&m=128476570300466&w=2
>> >> but to repeat, the list is dynamic during initialization but remains 
>> >> static after initialization based on SOC framework implementation - this 
>> >> is best implemented with a list (we had started with an original array 
>> >> implementation which evolved to the current list implementation 
>> >> http://marc.info/?l=linux-omap&m=125912217718770&w=2)
>> >
>> > Well, my point is, since the _final_ set of OPPs doesn't really
>> > change, there's no need to use a list for storing it in principle.
>> >
>> > Your current algorithm seems to be:
>> > (1) Create a list of all _possible_ OPPs.
>> > (2) Mark the ones that can actually be used on the given hardware as
>> >     "available".
>> > (3) Whenever we need to find an OPP to use, browse the entire list.
>> > This isn't optimal, because the OPPs that are not marked as "available" in (2)
>> > will never be used, although they _will_ be inspected while browsing the list.
>> 
>> A little clarificaion about "will never be used" below...
>> 
>> > So, I think a better algorithm would be:
>> > (1) Create a list of all possible OPPs.
>> > (2) Drop the nonavailable OPPs from the list.
>> > (3) Whenever we need to find an OPP to use, browse the entire list.
>> >
>> > But then, it may be better to simply move the list we get in (2) into an
>> > array, because the browsing is going to require fewer memory accesses in
>> > that case (also, an array would use less memory than the list).  So, perhaps,
>> > it's better to change the algorithm even further:
>> > (1) Create a list of all possible OPPs.
>> > (2) Drop the nonavailable OPPs from the list.
>> > (3) Move the list we got in (2) into a sorted array.
>> > (4) Whenever we need to find an OPP to use, browse the array
>> >      (perhaps using binary search).
>> 
>> Just a little clarification on "available."  The intended use of this flag
>> was not just a one-time "available on hardware X."  It was also intended
>> to be able to add/remove availbale OPPs dynamically at run-time.
>> 
>> More specifically, it's intended for use to *temporarily* remove an OPP
>> from being selected.  The production usage of this would primarily for
>> thermal considerations (e.g. don't use OPPx until the temperature drops)
>> 
>> However, for PM development & debug, we also use this to temporarily
>> take a class of OPPs out of the running for test/debug purposes
>> (e.g. driver X runs great at OPPx and OPPy, but not OPPz.)  So the
>> ability to temporarily be selective about OPPs at runtime for
>> debug/development is extremely useful.
>> 
>> So, to summarize, "most of the time", all the OPPs that were added (via
>> opp_add()) will be "available".  Ones that are !availble will likely
>> only be so temporarily, so I'm not sure that the overhead of keeping a
>> separate structure for the available and !available OPPs is worth it.
>> Especially, since OPP changes are relatively infrequent.
>
> Well, the Nishanth's description doesn't match this, so thanks for the
> clarification.

Agreed, we need to update the doc file to reflect this.

> In that case you might consider using a red-black tree for storing the
> "available" OPPs, so that you can add-remove them dynamically, but
> you can avoid a linear search through the entire list every time you need to
> find and available OPP.  Since we have standard helpers for handling rbtrees,
> that shouldn't be a big deal.

That's a possibility, but do you think rbtrees are worth it for a
relatively small number of OPPs for any given device?  We're using this
to track a list of OPPs for any struct device, so there may be multiple
independent OPP lists, but each would have a small number of OPPs.

For example, on OMAP, while the CPU might have a larger number of OPPs
(5-6), most devices will have a small number of OPPs (1-3.)  I gather
this is similar for many of the embedded SoCs available today.

Considering such a small number of OPPs, is the extra complexity of an
rbtree worth it?

Kevin

Re: [PATCH] opp: introduce library for device-specific OPPs

[Rafael J. Wysocki]

On Monday, September 20, 2010, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> [...]
> 
> >> >> In terms of the lifetime rules on the nodes in the list:
> >> >> The list is expected to be maintained once created, entries are expected 
> >> >> to be added optimally and not expected to be destroyed, the choice of 
> >> >> list implementation was for reducing the complexity of the code itself 
> >> >> and not yet meant as a mechanism to dynamically add and delete nodes on 
> >> >> the fly.. Essentially, it was intended for the SOC framework to ensure 
> >> >> it plugs in the OPP entries optimally and not create a humongous list of 
> >> >> all possible OPPs for all families of the vendor SOCs - even though it 
> >> >> is possible to use the OPP layer so - it just wont be smart to do so 
> >> >> considering list scan latencies on hot paths such as cpufreq transitions 
> >> >> or idle transitions.
> >> > 
> >> > If the list nodes are not supposed to be added and removed dynamically,
> >> > it probably would make sense to create data structures containing
> >> > the "available" OPPs only, once they are known, and simply free the object
> >> > representing the other ones.
> >> I covered the usage in my reply here: 
> >> http://marc.info/?l=linux-arm-kernel&m=128476570300466&w=2
> >> but to repeat, the list is dynamic during initialization but remains 
> >> static after initialization based on SOC framework implementation - this 
> >> is best implemented with a list (we had started with an original array 
> >> implementation which evolved to the current list implementation 
> >> http://marc.info/?l=linux-omap&m=125912217718770&w=2)
> >
> > Well, my point is, since the _final_ set of OPPs doesn't really
> > change, there's no need to use a list for storing it in principle.
> >
> > Your current algorithm seems to be:
> > (1) Create a list of all _possible_ OPPs.
> > (2) Mark the ones that can actually be used on the given hardware as
> >     "available".
> > (3) Whenever we need to find an OPP to use, browse the entire list.
> > This isn't optimal, because the OPPs that are not marked as "available" in (2)
> > will never be used, although they _will_ be inspected while browsing the list.
> 
> A little clarificaion about "will never be used" below...
> 
> > So, I think a better algorithm would be:
> > (1) Create a list of all possible OPPs.
> > (2) Drop the nonavailable OPPs from the list.
> > (3) Whenever we need to find an OPP to use, browse the entire list.
> >
> > But then, it may be better to simply move the list we get in (2) into an
> > array, because the browsing is going to require fewer memory accesses in
> > that case (also, an array would use less memory than the list).  So, perhaps,
> > it's better to change the algorithm even further:
> > (1) Create a list of all possible OPPs.
> > (2) Drop the nonavailable OPPs from the list.
> > (3) Move the list we got in (2) into a sorted array.
> > (4) Whenever we need to find an OPP to use, browse the array
> >      (perhaps using binary search).
> 
> Just a little clarification on "available."  The intended use of this flag
> was not just a one-time "available on hardware X."  It was also intended
> to be able to add/remove availbale OPPs dynamically at run-time.
> 
> More specifically, it's intended for use to *temporarily* remove an OPP
> from being selected.  The production usage of this would primarily for
> thermal considerations (e.g. don't use OPPx until the temperature drops)
> 
> However, for PM development & debug, we also use this to temporarily
> take a class of OPPs out of the running for test/debug purposes
> (e.g. driver X runs great at OPPx and OPPy, but not OPPz.)  So the
> ability to temporarily be selective about OPPs at runtime for
> debug/development is extremely useful.
> 
> So, to summarize, "most of the time", all the OPPs that were added (via
> opp_add()) will be "available".  Ones that are !availble will likely
> only be so temporarily, so I'm not sure that the overhead of keeping a
> separate structure for the available and !available OPPs is worth it.
> Especially, since OPP changes are relatively infrequent.

Well, the Nishanth's description doesn't match this, so thanks for the
clarification.

In that case you might consider using a red-black tree for storing the
"available" OPPs, so that you can add-remove them dynamically, but
you can avoid a linear search through the entire list every time you need to
find and available OPP.  Since we have standard helpers for handling rbtrees,
that shouldn't be a big deal.

Thanks,
Rafael

Re: [PATCH] opp: introduce library for device-specific OPPs

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

[...]

>> >> In terms of the lifetime rules on the nodes in the list:
>> >> The list is expected to be maintained once created, entries are expected 
>> >> to be added optimally and not expected to be destroyed, the choice of 
>> >> list implementation was for reducing the complexity of the code itself 
>> >> and not yet meant as a mechanism to dynamically add and delete nodes on 
>> >> the fly.. Essentially, it was intended for the SOC framework to ensure 
>> >> it plugs in the OPP entries optimally and not create a humongous list of 
>> >> all possible OPPs for all families of the vendor SOCs - even though it 
>> >> is possible to use the OPP layer so - it just wont be smart to do so 
>> >> considering list scan latencies on hot paths such as cpufreq transitions 
>> >> or idle transitions.
>> > 
>> > If the list nodes are not supposed to be added and removed dynamically,
>> > it probably would make sense to create data structures containing
>> > the "available" OPPs only, once they are known, and simply free the object
>> > representing the other ones.
>> I covered the usage in my reply here: 
>> http://marc.info/?l=linux-arm-kernel&m=128476570300466&w=2
>> but to repeat, the list is dynamic during initialization but remains 
>> static after initialization based on SOC framework implementation - this 
>> is best implemented with a list (we had started with an original array 
>> implementation which evolved to the current list implementation 
>> http://marc.info/?l=linux-omap&m=125912217718770&w=2)
>
> Well, my point is, since the _final_ set of OPPs doesn't really
> change, there's no need to use a list for storing it in principle.
>
> Your current algorithm seems to be:
> (1) Create a list of all _possible_ OPPs.
> (2) Mark the ones that can actually be used on the given hardware as
>     "available".
> (3) Whenever we need to find an OPP to use, browse the entire list.
> This isn't optimal, because the OPPs that are not marked as "available" in (2)
> will never be used, although they _will_ be inspected while browsing the list.

A little clarificaion about "will never be used" below...

> So, I think a better algorithm would be:
> (1) Create a list of all possible OPPs.
> (2) Drop the nonavailable OPPs from the list.
> (3) Whenever we need to find an OPP to use, browse the entire list.
>
> But then, it may be better to simply move the list we get in (2) into an
> array, because the browsing is going to require fewer memory accesses in
> that case (also, an array would use less memory than the list).  So, perhaps,
> it's better to change the algorithm even further:
> (1) Create a list of all possible OPPs.
> (2) Drop the nonavailable OPPs from the list.
> (3) Move the list we got in (2) into a sorted array.
> (4) Whenever we need to find an OPP to use, browse the array
>      (perhaps using binary search).

Just a little clarification on "available."  The intended use of this flag
was not just a one-time "available on hardware X."  It was also intended
to be able to add/remove availbale OPPs dynamically at run-time.

More specifically, it's intended for use to *temporarily* remove an OPP
from being selected.  The production usage of this would primarily for
thermal considerations (e.g. don't use OPPx until the temperature drops)

However, for PM development & debug, we also use this to temporarily
take a class of OPPs out of the running for test/debug purposes
(e.g. driver X runs great at OPPx and OPPy, but not OPPz.)  So the
ability to temporarily be selective about OPPs at runtime for
debug/development is extremely useful.

So, to summarize, "most of the time", all the OPPs that were added (via
opp_add()) will be "available".  Ones that are !availble will likely
only be so temporarily, so I'm not sure that the overhead of keeping a
separate structure for the available and !available OPPs is worth it.
Especially, since OPP changes are relatively infrequent.

Kevin

Re: [PATCH] opp: introduce library for device-specific OPPs

[Rafael J. Wysocki]

[Trimming the CC list.]

On Saturday, September 18, 2010, Nishanth Menon wrote:
> Rafael J. Wysocki had written, on 09/17/2010 06:07 PM, the following:
...
> > 
> > Apart from this, it might be a good idea to help callers a bit and actually
> > introduce some sort of locking into the framework.
> in OMAP implementation we have need to use these apis from irq_locked 
> contexts as well..  lock implementation will prevent thier necessary 
> usage.

Why so?  You can use spin_lock_irqsave/spin_unclock_irqrestore  to
avoid that.

> instead we have implemented lock mechanisms in higher layers and 
> ensured that the opp table does not change once created - the rest of 
> them are query functions - the risk is on opp_enable/disable.

OK, but this way you practically make the users of the framework duplicate
some code for the locking purposes.  This is kind of suboptimal, and error
prone too.

> >> In terms of the lifetime rules on the nodes in the list:
> >> The list is expected to be maintained once created, entries are expected 
> >> to be added optimally and not expected to be destroyed, the choice of 
> >> list implementation was for reducing the complexity of the code itself 
> >> and not yet meant as a mechanism to dynamically add and delete nodes on 
> >> the fly.. Essentially, it was intended for the SOC framework to ensure 
> >> it plugs in the OPP entries optimally and not create a humongous list of 
> >> all possible OPPs for all families of the vendor SOCs - even though it 
> >> is possible to use the OPP layer so - it just wont be smart to do so 
> >> considering list scan latencies on hot paths such as cpufreq transitions 
> >> or idle transitions.
> > 
> > If the list nodes are not supposed to be added and removed dynamically,
> > it probably would make sense to create data structures containing
> > the "available" OPPs only, once they are known, and simply free the object
> > representing the other ones.
> I covered the usage in my reply here: 
> http://marc.info/?l=linux-arm-kernel&m=128476570300466&w=2
> but to repeat, the list is dynamic during initialization but remains 
> static after initialization based on SOC framework implementation - this 
> is best implemented with a list (we had started with an original array 
> implementation which evolved to the current list implementation 
> http://marc.info/?l=linux-omap&m=125912217718770&w=2)

Well, my point is, since the _final_ set of OPPs doesn't really change, there's
no need to use a list for storing it in principle.

Your current algorithm seems to be:
(1) Create a list of all _possible_ OPPs.
(2) Mark the ones that can actually be used on the given hardware as
    "available".
(3) Whenever we need to find an OPP to use, browse the entire list.

This isn't optimal, because the OPPs that are not marked as "available" in (2)
will never be used, although they _will_ be inspected while browsing the list.
So, I think a better algorithm would be:
(1) Create a list of all possible OPPs.
(2) Drop the nonavailable OPPs from the list.
(3) Whenever we need to find an OPP to use, browse the entire list.

But then, it may be better to simply move the list we get in (2) into an
array, because the browsing is going to require fewer memory accesses in
that case (also, an array would use less memory than the list).  So, perhaps,
it's better to change the algorithm even further:
(1) Create a list of all possible OPPs.
(2) Drop the nonavailable OPPs from the list.
(3) Move the list we got in (2) into a sorted array.
(4) Whenever we need to find an OPP to use, browse the array
     (perhaps using binary search).

Thanks,
Rafael