[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[PATCH v2] PM / devfreq: Use freq_table for available_frequencies

[Saravana Kannan]

On 05/08/2014 03:02 AM, MyungJoo Ham wrote:
>> On 04/29/2014 01:00 PM, Saravana Kannan wrote:
>>> On 04/27/2014 06:41 PM, MyungJoo Ham wrote:
>>>> You are hereby changing the semmantics of the original
>>>> available_frequencies node.
>>>>
>>>> When a frequency/voltage pair has been disabled (opp_disable), probably
>>>> by opp_disable(), the frequency is no more "available".
>>>> However, when the driver author supplied freq_table as well as OPP
>>>> in order to see the statistics, the node will behave differently.
>>>>
>>>> Please do not affect the current users as long as it does not give
>>>> additional benefit or fix a bug.
>>>
>>> I was actually trying to stick with the semantics as it was documented.
>>> The documentation for this file says it'll show frequencies that are not
>>> allowed by the current min/max settings either. To me, an OPP disable
>>> seems similar to some frequencies "disabled" by min/max settings.
>>>
>>> Giving preference to OPP is not a hard change to do, but it seems to go
>>> againsts the documented semantics.
>>>
>>> Thoughts?
>>
>> I'll send out another patch like you wanted -- with OPP being given
>> preference over freq_table when listing frequencies.
>>
>> But I would still like to hear your thoughts. As of today, there's no
>> clean way to get the complete list of available frequencies that would
>> give a consistent output irrespective of the temporary limits/conditions
>> imposed by thermal, current limiting, etc. The round about way is to cat
>> trans_stat and parse the frequencies from that.
>>
>> That's why I was trying to give preference to freq_table.
>>
>> Thanks,
>> Saravana
> 
> The node, available_frequencies, was suggested before freq_table concept.
> At that time, available_frequencies was supposed to show the list of
> available OPP lists for those who use OPP for devfreq device, excluding
> those disabled by OPP. (OPP lists are external to devfreq and devfreq's
> min/max are internal to devfreq)
> 
> Locally, this node has been used to debug the behavior of a devfreq device.
> With min/max nodes, we know the range while we cannot (easily at shell)
> see which OPP points are available at the moment, where we have been able
> to use available_frequencies.
> 
> We do not want to lose such capavility as long as we do not have OPP sysfs
> automatically assigned to any OPP lists. If I remember correctly, we don't
> have it, yet.
> 
> 
> A. I want to minimize semantics changes in sysfs. Adding another without
> interfering with previous usage is ok.
> B. (more importantly) I don't want to lose the debugging capabilities.
> 

Thanks for the response MyungJoo. Makes sense. I was also discussing
this internally and was considering a "possible_frequencies" similar to
available vs possible CPUs.

I'll make such a patch for that and send it out. In that case, I'll
probably leave "available_frequencies" alone.

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3] PM / devfreq: Add possible_frequencies device attribute

[Saravana Kannan]

Some devices use freq_table instead of OPP. For those devices, the
available_frequencies sysfs file shows up empty. So, add a
possible_frequencies attribute/syfs file that list all the possible
frequencies.

For devices that use OPP, the output of this file will match
available_frequencies. It may change in the future to show all OPP
frequencies -- even the disabled ones.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
---
 drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 9f90369..65eed38 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -994,6 +994,31 @@ static ssize_t available_frequencies_show(struct device *d,
 }
 static DEVICE_ATTR_RO(available_frequencies);
 
+static ssize_t possible_frequencies_show(struct device *d,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct devfreq *df = to_devfreq(d);
+	unsigned int i = 0;
+	ssize_t count = 0;
+
+	if (!df->profile->freq_table)
+		return available_frequencies_show(d, attr, buf);
+
+	for (i = 0; i < df->profile->max_state; i++)
+		count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
+				   "%u ", df->profile->freq_table[i]);
+
+	/* Truncate the trailing space */
+	if (count)
+		count--;
+
+	count += sprintf(&buf[count], "\n");
+
+	return count;
+}
+static DEVICE_ATTR_RO(possible_frequencies);
+
 static ssize_t trans_stat_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
@@ -1041,6 +1066,7 @@ static struct attribute *devfreq_attrs[] = {
 	&dev_attr_available_governors.attr,
 	&dev_attr_cur_freq.attr,
 	&dev_attr_available_frequencies.attr,
+	&dev_attr_possible_frequencies.attr,
 	&dev_attr_target_freq.attr,
 	&dev_attr_polling_interval.attr,
 	&dev_attr_min_freq.attr,
-- 
1.8.2.1

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH v3] PM / devfreq: Add possible_frequencies device attribute

[Saravana Kannan]

On 07/16/2014 05:50 PM, MyungJoo Ham wrote:
> On Wed, Jul 16, 2014 at 12:01 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> Some devices use freq_table instead of OPP. For those devices, the
>> available_frequencies sysfs file shows up empty. So, add a
>> possible_frequencies attribute/syfs file that list all the possible
>> frequencies.
>>
>> For devices that use OPP, the output of this file will match
>> available_frequencies. It may change in the future to show all OPP
>> frequencies -- even the disabled ones.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> 
> Hi,
> 
> 
> Please add a documentation entry for this new ABI having a little justification and usage included.

Will do.

> 
> Plus, I am considering to move trans_stat along with this entry to somewhere such as .../stat/*
> (you don't need to take care of this.)

Ok.

> 
> Besides, as OPP seems becoming the standard as imagined when devfreq development started,
> soon, devfreq may require OPP unless the devfreq device has continuous frequencies.

I agree. Only one caveat with OPP is that if a device isn't using OPP to
do any voltage scaling, then it forces a voltage column with 0s. Also,
even if we make OPP mandatory, we'll still want trans_stats that
currently seem to depend on freq_table being populated. I was actually
planning on sending out more patches later that'll do a lot of stuff
automatically for devices with OPP. Like creating freq_table, etc.

-Saravana

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation