On Fri, Aug 19, 2011 at 12:28 AM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
> On Fri, Aug 19, 2011 at 12:15 PM, Turquette, Mike <mturquette@xxxxxx> wrote:
>> On Thu, Aug 18, 2011 at 3:27 AM, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
>>> Device specific sysfs interface /sys/devices/.../power/devfreq_*
>>> - governor R: name of governor
>>> - cur_freq R: current frequency
>>> - max_freq R: maximum operable frequency
>>> - min_freq R: minimum operable frequency
>>> - set_freq R: read user specified frequency (0 if not specified yet)
>>> W: set user specified frequency
>>> - polling_interval R: polling interval in ms given with devfreq profile
>>
>> This code still does not export up_threshold/down_threshold entries
>> for simple-ondemand which I requested in V5. However there is a much
>> bigger issue here brought out in patch 3. Those values shouldn't be
>> global, since devices might want different thresholds.
>>
>> The solution is not to put those tunables in struct devfreq.
>> up_threshold and down_threshold must be parameters of the governor,
>> and there must be a per-device instance of the governor.
>>
>> Regards,
>> Mike
>
> As in the example in the reply to the thread of patch 3/4 of v6
> patchset, we do not need to create another per-device object in
> DEVFREQ in order to allow governors to be tuned per device. The global
> values you've mentioned are only used as default and each instance of
> devfreq (per device) can specify its own tuneable values and they can
> alter the values in run-time.
>
> For the sysfs interfaces of governor-specific tunable values, I'd like
> to see that as a "future work" and that would be implemented
> per-governor basis; i.e., probably under
> /sysfs/devices/.../power/NAME_OF_GOVERNOR/variable_name. Or should I
> regard the sysfs interface to tune governors at userland is somewhat
> essential and critical for the initial release?
I guess it is not critical but I feel that implementing all of the
sysfs bits will reveal some designs issues.
E.g. by pushing threshold data into the device's private_data you're
crossing data paths with the governors: the private data tunable must
be associated per-device in sysfs, but the read/write functions must
come from the governor. If the core code assumes too much about what
private data looks like (up_threshold, down_threshold, etc) then it
might be limiting to future governor authors.
The patches are shaping up however and I won't consider sysfs support
a blocker. Rafael's point about revisiting design at a later date is
good guidance here.
Regards,
Mike
> Cheers!
>
> MyungJoo
>
>>
>>>
>>> Signed-off-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>>
>>> --
>>> Changed from v5
>>> - updated devferq_update usage.
>>>
>>> Changed from v4
>>> - removed system-wide sysfs interface
>>> - removed tickling sysfs interface
>>> - added set_freq for userspace governor (and any other governors that
>>> require user input)
>>>
>>> Changed from v3
>>> - corrected sysfs API usage
>>> - corrected error messages
>>> - moved sysfs entry location
>>> - added sysfs entries
>>>
>>> Changed from v2
>>> - add ABI entries for devfreq sysfs interface
>>> ---
>>> Documentation/ABI/testing/sysfs-devices-power | 45 +++++++
>>> drivers/devfreq/devfreq.c | 152 +++++++++++++++++++++++++
>>> 2 files changed, 197 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
>>> index 8ffbc25..6e77604 100644
>>> --- a/Documentation/ABI/testing/sysfs-devices-power
>>> +++ b/Documentation/ABI/testing/sysfs-devices-power
>>> @@ -165,3 +165,48 @@ Description:
>>>
>>> Not all drivers support this attribute. If it isn't supported,
>>> attempts to read or write it will yield I/O errors.
>>> +
>>> +What: /sys/devices/.../power/devfreq_governor
>>> +Date: July 2011
>>> +Contact: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> +Description:
>>> + The /sys/devices/.../power/devfreq_governor shows the name
>>> + of the governor used by the corresponding device.
>>> +
>>> +What: /sys/devices/.../power/devfreq_cur_freq
>>> +Date: July 2011
>>> +Contact: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> +Description:
>>> + The /sys/devices/.../power/devfreq_cur_freq shows the current
>>> + frequency of the corresponding device.
>>> +
>>> +What: /sys/devices/.../power/devfreq_max_freq
>>> +Date: July 2011
>>> +Contact: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> +Description:
>>> + The /sys/devices/.../power/devfreq_cur_freq shows the
>>> + maximum operable frequency of the corresponding device.
>>> +
>>> +What: /sys/devices/.../power/devfreq_min_freq
>>> +Date: July 2011
>>> +Contact: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> +Description:
>>> + The /sys/devices/.../power/devfreq_cur_freq shows the
>>> + minimum operable frequency of the corresponding device.
>>> +
>>> +What: /sys/devices/.../power/devfreq_set_freq
>>> +Date: August 2011
>>> +Contact: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> +Description:
>>> + The /sys/devices/.../power/devfreq_set_freq sets and shows
>>> + the user specified desired frequency of the device. The
>>> + governor may and may not use the value. With the basic
>>> + governors given with devfreq.c, userspace governor is
>>> + using the value.
>>> +
>>> +What: /sys/devices/.../power/devfreq_polling_interval
>>> +Date: July 2011
>>> +Contact: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
>>> +Description:
>>> + The /sys/devices/.../power/devfreq_polling_interval shows the
>>> + requested polling interval of the corresponding device.
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 2036f2c..13b422f 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -37,6 +37,8 @@ static struct delayed_work devfreq_work;
>>> static LIST_HEAD(devfreq_list);
>>> static DEFINE_MUTEX(devfreq_list_lock);
>>>
>>> +static struct attribute_group dev_attr_group;
>>> +
>>> /**
>>> * find_device_devfreq() - find devfreq struct using device pointer
>>> * @dev: device pointer used to lookup device devfreq.
>>> @@ -154,6 +156,8 @@ static void devfreq_monitor(struct work_struct *work)
>>> dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
>>> error, devfreq->governor->name);
>>>
>>> + sysfs_unmerge_group(&devfreq->dev->kobj,
>>> + &dev_attr_group);
>>> list_del(&devfreq->node);
>>> kfree(devfreq);
>>>
>>> @@ -244,6 +248,8 @@ int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
>>> queue_delayed_work(devfreq_wq, &devfreq_work,
>>> devfreq->next_polling);
>>> }
>>> +
>>> + sysfs_merge_group(&dev->kobj, &dev_attr_group);
>>> out:
>>> mutex_unlock(&devfreq_list_lock);
>>>
>>> @@ -276,6 +282,8 @@ int devfreq_remove_device(struct device *dev)
>>> goto out;
>>> }
>>>
>>> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
>>> +
>>> list_del(&devfreq->node);
>>> srcu_notifier_chain_unregister(nh, &devfreq->nb);
>>> kfree(devfreq);
>>> @@ -284,6 +292,150 @@ out:
>>> return 0;
>>> }
>>>
>>> +static ssize_t show_governor(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = find_device_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->governor)
>>> + return -EINVAL;
>>> +
>>> + return sprintf(buf, "%s\n", df->governor->name);
>>> +}
>>> +
>>> +static ssize_t show_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = find_device_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> +
>>> + return sprintf(buf, "%lu\n", df->previous_freq);
>>> +}
>>> +
>>> +static ssize_t show_max_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = find_device_devfreq(dev);
>>> + unsigned long freq = ULONG_MAX;
>>> + struct opp *opp;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->dev)
>>> + return -EINVAL;
>>> +
>>> + opp = opp_find_freq_floor(df->dev, &freq);
>>> + if (IS_ERR(opp))
>>> + return PTR_ERR(opp);
>>> +
>>> + return sprintf(buf, "%lu\n", freq);
>>> +}
>>> +
>>> +static ssize_t show_min_freq(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = find_device_devfreq(dev);
>>> + unsigned long freq = 0;
>>> + struct opp *opp;
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->dev)
>>> + return -EINVAL;
>>> +
>>> + opp = opp_find_freq_ceil(df->dev, &freq);
>>> + if (IS_ERR(opp))
>>> + return PTR_ERR(opp);
>>> +
>>> + return sprintf(buf, "%lu\n", freq);
>>> +}
>>> +
>>> +static ssize_t show_polling_interval(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *df = find_device_devfreq(dev);
>>> +
>>> + if (IS_ERR(df))
>>> + return PTR_ERR(df);
>>> + if (!df->profile)
>>> + return -EINVAL;
>>> +
>>> + return sprintf(buf, "%d\n", df->profile->polling_ms);
>>> +}
>>> +
>>> +static ssize_t set_user_frequency(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct devfreq *devfreq;
>>> + unsigned long wanted;
>>> + int err = 0;
>>> +
>>> + sscanf(buf, "%lu", &wanted);
>>> +
>>> + mutex_lock(&devfreq_list_lock);
>>> + devfreq = find_device_devfreq(dev);
>>> +
>>> + if (IS_ERR(devfreq)) {
>>> + err = PTR_ERR(devfreq);
>>> + goto out;
>>> + }
>>> +
>>> + devfreq->user_set_freq = wanted;
>>> + err = count;
>>> +out:
>>> + mutex_unlock(&devfreq_list_lock);
>>> + if (err >= 0)
>>> + devfreq_update(&devfreq->nb, 0, NULL);
>>> + return err;
>>> +}
>>> +
>>> +static ssize_t show_user_frequency(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct devfreq *devfreq;
>>> + int err = 0;
>>> +
>>> + mutex_lock(&devfreq_list_lock);
>>> + devfreq = find_device_devfreq(dev);
>>> +
>>> + if (IS_ERR(devfreq)) {
>>> + err = PTR_ERR(devfreq);
>>> + goto out;
>>> + }
>>> +
>>> + err = sprintf(buf, "%lu\n", devfreq->user_set_freq);
>>> +out:
>>> + mutex_unlock(&devfreq_list_lock);
>>> + return err;
>>> +
>>> +}
>>> +
>>> +static DEVICE_ATTR(devfreq_governor, 0444, show_governor, NULL);
>>> +static DEVICE_ATTR(devfreq_cur_freq, 0444, show_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_max_freq, 0444, show_max_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_min_freq, 0444, show_min_freq, NULL);
>>> +static DEVICE_ATTR(devfreq_polling_interval, 0444, show_polling_interval, NULL);
>>> +static DEVICE_ATTR(devfreq_set_freq, 0644, show_user_frequency,
>>> + set_user_frequency);
>>> +static struct attribute *dev_entries[] = {
>>> + &dev_attr_devfreq_governor.attr,
>>> + &dev_attr_devfreq_cur_freq.attr,
>>> + &dev_attr_devfreq_max_freq.attr,
>>> + &dev_attr_devfreq_min_freq.attr,
>>> + &dev_attr_devfreq_polling_interval.attr,
>>> + &dev_attr_devfreq_set_freq.attr,
>>> + NULL,
>>> +};
>>> +static struct attribute_group dev_attr_group = {
>>> + .name = power_group_name,
>>> + .attrs = dev_entries,
>>> +};
>>> +
>>> /**
>>> * devfreq_init() - Initialize data structure for devfreq framework and
>>> * start polling registered devfreq devices.
>>> --
>>> 1.7.4.1
>>>
>>>
>>
>
>
>
> --
> MyungJoo Ham (함명주), Ph.D.
> Mobile Software Platform Lab,
> Digital Media and Communications (DMC) Business
> Samsung Electronics
> cell: 82-10-6714-2858
>
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
