* [PATCH v9 0/4] Devfreq, DVFS Framework for Non-CPU Devices
@ 2011-08-31 7:29 MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 1/4] PM / OPP: Add OPP availability change notifier MyungJoo Ham
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: MyungJoo Ham @ 2011-08-31 7:29 UTC (permalink / raw)
To: linux-pm; +Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, Thomas Gleixner
The main update from the patchset v8:
- Add per-devfreq-device locking mechanism (devfreq->lock)
- Provide the per-devfreq-device locking mechanism to governors
- Merged 4/5 patch to 2/5 (devfreq internal interface for governors)
Patch 1/4 has no changes.
Patch 2/4 has major update on synchronization and merged another patch; thus, dropped "Reviewed-By".
Patch 3/4 has minor udpate (affected by the update on 2/4: mutex added)
Patch 4/4 has minor update (affected by the update on 2/4: mutex added) + removed unused variable.
For a usage example, please look at
http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
In the above git tree, DVFS (dynamic voltage and frequency scaling) mechanism
is applied to the memory bus of Exynos4210 for Exynos4210-NURI boards.
In the example, the LPDDR2 DRAM frequency changes between 133, 266, and 400MHz
and other related clocks simply follow the determined DDR RAM clock.
The devfreq driver for Exynos4210 memory bus is at
/drivers/devfreq/exynos4210_memorybus.c in the git tree.
In the dd (writing and reading 360MiB) test with NURI board, the memory
throughput was not changed (the performance is not deteriorated) while
the SoC power consumption has been reduced by 1%. When the memory access
is not that intense while the CPU is heavily used, the SoC power consumption
has been reduced by 6%. The power consumption has been compared with the
case using the conventional Exynos4210 cpufreq driver, which sets memory
bus frequency according to the CPU core frequency. Besides, when the CPU core
running slow and the memory access is intense, the performance (memory
throughput) has been increased by 11% (with higher SoC power consumption of
5%). The tested governor is "simple-ondemand".
MyungJoo Ham (4):
PM / OPP: Add OPP availability change notifier.
PM: Introduce devfreq: generic DVFS framework with device-specific
OPPs
PM / devfreq: add common sysfs interfaces
PM / devfreq: add basic governors
Documentation/ABI/testing/sysfs-devices-power | 46 ++
drivers/Kconfig | 2 +
drivers/Makefile | 2 +
drivers/base/power/opp.c | 29 ++
drivers/devfreq/Kconfig | 75 ++++
drivers/devfreq/Makefile | 5 +
drivers/devfreq/devfreq.c | 567 +++++++++++++++++++++++++
drivers/devfreq/governor.h | 22 +
drivers/devfreq/governor_performance.c | 24 +
drivers/devfreq/governor_powersave.c | 24 +
drivers/devfreq/governor_simpleondemand.c | 88 ++++
drivers/devfreq/governor_userspace.c | 126 ++++++
include/linux/devfreq.h | 160 +++++++
include/linux/opp.h | 12 +
14 files changed, 1182 insertions(+), 0 deletions(-)
create mode 100644 drivers/devfreq/Kconfig
create mode 100644 drivers/devfreq/Makefile
create mode 100644 drivers/devfreq/devfreq.c
create mode 100644 drivers/devfreq/governor.h
create mode 100644 drivers/devfreq/governor_performance.c
create mode 100644 drivers/devfreq/governor_powersave.c
create mode 100644 drivers/devfreq/governor_simpleondemand.c
create mode 100644 drivers/devfreq/governor_userspace.c
create mode 100644 include/linux/devfreq.h
--
1.7.4.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v9 1/4] PM / OPP: Add OPP availability change notifier.
2011-08-31 7:29 [PATCH v9 0/4] Devfreq, DVFS Framework for Non-CPU Devices MyungJoo Ham
@ 2011-08-31 7:29 ` MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs MyungJoo Ham
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: MyungJoo Ham @ 2011-08-31 7:29 UTC (permalink / raw)
To: linux-pm; +Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, Thomas Gleixner
The patch enables to register notifier_block for an OPP-device in order
to get notified for any changes in the availability of OPPs of the
device. For example, if a new OPP is inserted or enable/disable status
of an OPP is changed, the notifier is executed.
This enables the usage of opp_add, opp_enable, and opp_disable to
directly take effect with any connected entities such as cpufreq or
devfreq.
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Reviewed-by: Mike Turquette <mturquette@ti.com>
---
No changes since v7
Added at devfreq patch set v6 replacing devfreq_update calls at OPP.
---
drivers/base/power/opp.c | 29 +++++++++++++++++++++++++++++
include/linux/opp.h | 12 ++++++++++++
2 files changed, 41 insertions(+), 0 deletions(-)
diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index b23de18..e6b4c89 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -73,6 +73,7 @@ struct opp {
* RCU usage: nodes are not modified in the list of device_opp,
* however addition is possible and is secured by dev_opp_list_lock
* @dev: device pointer
+ * @head: notifier head to notify the OPP availability changes.
* @opp_list: list of opps
*
* This is an internal data structure maintaining the link to opps attached to
@@ -83,6 +84,7 @@ struct device_opp {
struct list_head node;
struct device *dev;
+ struct srcu_notifier_head head;
struct list_head opp_list;
};
@@ -404,6 +406,7 @@ int opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
}
dev_opp->dev = dev;
+ srcu_init_notifier_head(&dev_opp->head);
INIT_LIST_HEAD(&dev_opp->opp_list);
/* Secure the device list modification */
@@ -428,6 +431,11 @@ int opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
list_add_rcu(&new_opp->node, head);
mutex_unlock(&dev_opp_list_lock);
+ /*
+ * Notify the changes in the availability of the operable
+ * frequency/voltage list.
+ */
+ srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ADD, new_opp);
return 0;
}
@@ -504,6 +512,14 @@ static int opp_set_availability(struct device *dev, unsigned long freq,
mutex_unlock(&dev_opp_list_lock);
synchronize_rcu();
+ /* Notify the change of the OPP availability */
+ if (availability_req)
+ srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_ENABLE,
+ new_opp);
+ else
+ srcu_notifier_call_chain(&dev_opp->head, OPP_EVENT_DISABLE,
+ new_opp);
+
/* clean up old opp */
new_opp = opp;
goto out;
@@ -643,3 +659,16 @@ void opp_free_cpufreq_table(struct device *dev,
*table = NULL;
}
#endif /* CONFIG_CPU_FREQ */
+
+/** opp_get_notifier() - find notifier_head of the device with opp
+ * @dev: device pointer used to lookup device OPPs.
+ */
+struct srcu_notifier_head *opp_get_notifier(struct device *dev)
+{
+ struct device_opp *dev_opp = find_device_opp(dev);
+
+ if (IS_ERR(dev_opp))
+ return ERR_PTR(PTR_ERR(dev_opp)); /* matching type */
+
+ return &dev_opp->head;
+}
diff --git a/include/linux/opp.h b/include/linux/opp.h
index 7020e97..87a9208 100644
--- a/include/linux/opp.h
+++ b/include/linux/opp.h
@@ -16,9 +16,14 @@
#include <linux/err.h>
#include <linux/cpufreq.h>
+#include <linux/notifier.h>
struct opp;
+enum opp_event {
+ OPP_EVENT_ADD, OPP_EVENT_ENABLE, OPP_EVENT_DISABLE,
+};
+
#if defined(CONFIG_PM_OPP)
unsigned long opp_get_voltage(struct opp *opp);
@@ -40,6 +45,8 @@ int opp_enable(struct device *dev, unsigned long freq);
int opp_disable(struct device *dev, unsigned long freq);
+struct srcu_notifier_head *opp_get_notifier(struct device *dev);
+
#else
static inline unsigned long opp_get_voltage(struct opp *opp)
{
@@ -89,6 +96,11 @@ static inline int opp_disable(struct device *dev, unsigned long freq)
{
return 0;
}
+
+struct srcu_notifier_head *opp_get_notifier(struct device *dev)
+{
+ return ERR_PTR(-EINVAL);
+}
#endif /* CONFIG_PM */
#if defined(CONFIG_CPU_FREQ) && defined(CONFIG_PM_OPP)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs
2011-08-31 7:29 [PATCH v9 0/4] Devfreq, DVFS Framework for Non-CPU Devices MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 1/4] PM / OPP: Add OPP availability change notifier MyungJoo Ham
@ 2011-08-31 7:29 ` MyungJoo Ham
2011-08-31 20:05 ` Turquette, Mike
2011-08-31 7:29 ` [PATCH v9 3/4] PM / devfreq: add common sysfs interfaces MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 4/4] PM / devfreq: add basic governors MyungJoo Ham
3 siblings, 1 reply; 11+ messages in thread
From: MyungJoo Ham @ 2011-08-31 7:29 UTC (permalink / raw)
To: linux-pm; +Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, Thomas Gleixner
With OPPs, a device may have multiple operable frequency and voltage
sets. However, there can be multiple possible operable sets and a system
will need to choose one from them. In order to reduce the power
consumption (by reducing frequency and voltage) without affecting the
performance too much, a Dynamic Voltage and Frequency Scaling (DVFS)
scheme may be used.
This patch introduces the DVFS capability to non-CPU devices with OPPs.
DVFS is a techique whereby the frequency and supplied voltage of a
device is adjusted on-the-fly. DVFS usually sets the frequency as low
as possible with given conditions (such as QoS assurance) and adjusts
voltage according to the chosen frequency in order to reduce power
consumption and heat dissipation.
The generic DVFS for devices, devfreq, may appear quite similar with
/drivers/cpufreq. However, cpufreq does not allow to have multiple
devices registered and is not suitable to have multiple heterogenous
devices with different (but simple) governors.
Normally, DVFS mechanism controls frequency based on the demand for
the device, and then, chooses voltage based on the chosen frequency.
devfreq also controls the frequency based on the governor's frequency
recommendation and let OPP pick up the pair of frequency and voltage
based on the recommended frequency. Then, the chosen OPP is passed to
device driver's "target" callback.
When PM QoS is going to be used with the devfreq device, the device
driver should enable OPPs that are appropriate with the current PM QoS
requests. In order to do so, the device driver may call opp_enable and
opp_disable at the notifier callback of PM QoS so that PM QoS's
update_target() call enables the appropriate OPPs. Note that at least
one of OPPs should be enabled at any time; be careful when there is a
transition.
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
The test code with board support for Exynos4-NURI is at
http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/devfreq
---
Thank you for your valuable comments, Rafael, Greg, Pavel, Colin, Mike,
and Kevin.
Changed from v8
- Merged patch 4/5 of v8 (internal interfaces for governors)
- Added lock (mutex) to struct devfreq
- Uses devfreq->lock to access elements in devfreq.
- Added kerneldoc entries for init/exit callbacks of governors.
- The caller of update_devfreq() in governor.h should lock
devfreq->lock before calling it.
- Added comment on the usage of lock in struct devfreq.
- Revised devfreq_add_device error handling
At v8, there is no changes since v7
Changes from v6
- Type revised for timing variables
- Removed unnecessary code and variable
Changes at v6-resubmit from v6
- Use jiffy directly instead of ktime
- Be prepared for profile->polling_ms changes (not supported fully at
this stage)
Changes from v5
- Uses OPP availability change notifier
- Removed devfreq_interval. Uses one jiffy instead. DEVFREQ adjusts
polling interval based on the interval requirement of DEVFREQ
devices.
- Moved devfreq to /drivers/devfreq to accomodate devfreq-related files
including governors and devfreq drivers.
- Coding style revised.
- Updated devfreq_add_device interface to get tunable values.
Changed from v4
- Removed tickle, which is a duplicated feature; PM QoS can do the same.
- Allow to extend polling interval if devices have longer polling intervals.
- Relocated private data of governors.
- Removed system-wide sysfs
Changed from v3
- In kerneldoc comments, DEVFREQ has ben replaced by devfreq
- Revised removing devfreq entries with error mechanism
- Added and revised comments
- Removed unnecessary codes
- Allow to give a name to a governor
- Bugfix: a tickle call may cancel an older tickle call that is still in
effect.
Changed from v2
- Code style revised and cleaned up.
- Remove DEVFREQ entries that incur errors except for EAGAIN
- Bug fixed: tickle for devices without polling governors
Changes from v1(RFC)
- Rename: DVFS --> DEVFREQ
- Revised governor design
. Governor receives the whole struct devfreq
. Governor should gather usage information (thru get_dev_status) itself
- Periodic monitoring runs only when needed.
- DEVFREQ no more deals with voltage information directly
- Removed some printks.
- Some cosmetics update
- Use freezable_wq.
---
drivers/Kconfig | 2 +
drivers/Makefile | 2 +
drivers/devfreq/Kconfig | 39 +++++
drivers/devfreq/Makefile | 1 +
drivers/devfreq/devfreq.c | 364 ++++++++++++++++++++++++++++++++++++++++++++
drivers/devfreq/governor.h | 22 +++
include/linux/devfreq.h | 123 +++++++++++++++
7 files changed, 553 insertions(+), 0 deletions(-)
create mode 100644 drivers/devfreq/Kconfig
create mode 100644 drivers/devfreq/Makefile
create mode 100644 drivers/devfreq/devfreq.c
create mode 100644 drivers/devfreq/governor.h
create mode 100644 include/linux/devfreq.h
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 95b9e7e..a1efd75 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -130,4 +130,6 @@ source "drivers/iommu/Kconfig"
source "drivers/virt/Kconfig"
+source "drivers/devfreq/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 7fa433a..97c957b 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -127,3 +127,5 @@ obj-$(CONFIG_IOMMU_SUPPORT) += iommu/
# Virtualization drivers
obj-$(CONFIG_VIRT_DRIVERS) += virt/
+
+obj-$(CONFIG_PM_DEVFREQ) += devfreq/
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
new file mode 100644
index 0000000..1fb42de
--- /dev/null
+++ b/drivers/devfreq/Kconfig
@@ -0,0 +1,39 @@
+config ARCH_HAS_DEVFREQ
+ bool
+ depends on ARCH_HAS_OPP
+ help
+ Denotes that the architecture supports DEVFREQ. If the architecture
+ supports multiple OPP entries per device and the frequency of the
+ devices with OPPs may be altered dynamically, the architecture
+ supports DEVFREQ.
+
+menuconfig PM_DEVFREQ
+ bool "Generic Dynamic Voltage and Frequency Scaling (DVFS) support"
+ depends on PM_OPP && ARCH_HAS_DEVFREQ
+ help
+ With OPP support, a device may have a list of frequencies and
+ voltages available. DEVFREQ, a generic DVFS framework can be
+ registered for a device with OPP support in order to let the
+ governor provided to DEVFREQ choose an operating frequency
+ based on the OPP's list and the policy given with DEVFREQ.
+
+ Each device may have its own governor and policy. DEVFREQ can
+ reevaluate the device state periodically and/or based on the
+ OPP list changes (each frequency/voltage pair in OPP may be
+ disabled or enabled).
+
+ Like some CPUs with CPUFREQ, a device may have multiple clocks.
+ However, because the clock frequencies of a single device are
+ determined by the single device's state, an instance of DEVFREQ
+ is attached to a single device and returns a "representative"
+ clock frequency from the OPP of the device, which is also attached
+ to a device by 1-to-1. The device registering DEVFREQ takes the
+ responsiblity to "interpret" the frequency listed in OPP and
+ to set its every clock accordingly with the "target" callback
+ given to DEVFREQ.
+
+if PM_DEVFREQ
+
+comment "DEVFREQ Drivers"
+
+endif # PM_DEVFREQ
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
new file mode 100644
index 0000000..168934a
--- /dev/null
+++ b/drivers/devfreq/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_PM_DEVFREQ) += devfreq.o
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
new file mode 100644
index 0000000..621b863
--- /dev/null
+++ b/drivers/devfreq/devfreq.c
@@ -0,0 +1,364 @@
+/*
+ * devfreq: Generic Dynamic Voltage and Frequency Scaling (DVFS) Framework
+ * for Non-CPU Devices Based on OPP.
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/opp.h>
+#include <linux/devfreq.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+#include <linux/list.h>
+#include <linux/printk.h>
+#include <linux/hrtimer.h>
+
+/*
+ * devfreq_work periodically monitors every registered device.
+ * The minimum polling interval is one jiffy. The polling interval is
+ * determined by the minimum polling period among all polling devfreq
+ * devices. The resolution of polling interval is one jiffy.
+ */
+static bool polling;
+static struct workqueue_struct *devfreq_wq;
+static struct delayed_work devfreq_work;
+
+/* The list of all device-devfreq */
+static LIST_HEAD(devfreq_list);
+static DEFINE_MUTEX(devfreq_list_lock);
+
+/**
+ * find_device_devfreq() - find devfreq struct using device pointer
+ * @dev: device pointer used to lookup device devfreq.
+ *
+ * Search the list of device devfreqs and return the matched device's
+ * devfreq info. devfreq_list_lock should be held by the caller.
+ */
+static struct devfreq *find_device_devfreq(struct device *dev)
+{
+ struct devfreq *tmp_devfreq;
+
+ if (unlikely(IS_ERR_OR_NULL(dev))) {
+ pr_err("DEVFREQ: %s: Invalid parameters\n", __func__);
+ return ERR_PTR(-EINVAL);
+ }
+
+ list_for_each_entry(tmp_devfreq, &devfreq_list, node) {
+ if (tmp_devfreq->dev == dev)
+ return tmp_devfreq;
+ }
+
+ return ERR_PTR(-ENODEV);
+}
+
+/**
+ * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
+ * with mutex protection. exported for governors
+ * @dev: device pointer used to lookup device devfreq.
+ */
+struct devfreq *get_devfreq(struct device *dev)
+{
+ struct devfreq *ret;
+
+ mutex_lock(&devfreq_list_lock);
+ ret = find_device_devfreq(dev);
+ mutex_unlock(&devfreq_list_lock);
+
+ return ret;
+}
+
+/**
+ * devfreq_do() - Check the usage profile of a given device and configure
+ * frequency and voltage accordingly
+ * @devfreq: devfreq info of the given device
+ */
+static int devfreq_do(struct devfreq *devfreq)
+{
+ struct opp *opp;
+ unsigned long freq;
+ int err;
+
+ err = devfreq->governor->get_target_freq(devfreq, &freq);
+ if (err)
+ return err;
+
+ opp = opp_find_freq_ceil(devfreq->dev, &freq);
+ if (opp == ERR_PTR(-ENODEV))
+ opp = opp_find_freq_floor(devfreq->dev, &freq);
+
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+
+ if (devfreq->previous_freq == freq)
+ return 0;
+
+ err = devfreq->profile->target(devfreq->dev, opp);
+ if (err)
+ return err;
+
+ devfreq->previous_freq = freq;
+ return 0;
+}
+
+/**
+ * update_devfreq() - Notify that the device OPP or frequency requirement
+ * has been changed. This function is exported for governors.
+ * @devfreq: the devfreq instance.
+ *
+ * Note: lock devfreq->lock before calling update_devfreq
+ */
+int update_devfreq(struct devfreq *devfreq)
+{
+ int err = 0;
+
+ if (!mutex_is_locked(&devfreq->lock)) {
+ WARN(true, "devfreq->lock must be locked by the caller.\n");
+ return -EINVAL;
+ }
+
+ /* Reevaluate the proper frequency */
+ err = devfreq_do(devfreq);
+ return err;
+}
+
+/**
+ * devfreq_update() - Notify that the device OPP has been changed.
+ * @dev: the device whose OPP has been changed.
+ *
+ * Called by OPP notifier.
+ */
+static int devfreq_update(struct notifier_block *nb, unsigned long type,
+ void *devp)
+{
+ struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
+ int ret;
+
+ mutex_lock(&devfreq->lock);
+ ret = update_devfreq(devfreq);
+ mutex_unlock(&devfreq->lock);
+
+ return ret;
+}
+
+/**
+ * devfreq_monitor() - Periodically run devfreq_do()
+ * @work: the work struct used to run devfreq_monitor periodically.
+ *
+ */
+static void devfreq_monitor(struct work_struct *work)
+{
+ static unsigned long last_polled_at;
+ struct devfreq *devfreq, *tmp;
+ int error;
+ unsigned long jiffies_passed;
+ unsigned long next_jiffies = ULONG_MAX, now = jiffies;
+
+ /* Initially last_polled_at = 0, polling every device at bootup */
+ jiffies_passed = now - last_polled_at;
+ last_polled_at = now;
+ if (jiffies_passed == 0)
+ jiffies_passed = 1;
+
+ mutex_lock(&devfreq_list_lock);
+
+ list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
+ mutex_lock(&devfreq->lock);
+
+ if (devfreq->next_polling == 0) {
+ mutex_unlock(&devfreq->lock);
+ continue;
+ }
+
+ /*
+ * Reduce more next_polling if devfreq_wq took an extra
+ * delay. (i.e., CPU has been idled.)
+ */
+ if (devfreq->next_polling <= jiffies_passed) {
+ error = devfreq_do(devfreq);
+
+ /* Remove a devfreq with an error. */
+ if (error && error != -EAGAIN) {
+ dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
+ error, devfreq->governor->name);
+
+ list_del(&devfreq->node);
+ mutex_unlock(&devfreq->lock);
+ kfree(devfreq);
+ continue;
+ }
+ devfreq->next_polling = devfreq->polling_jiffies;
+
+ /* No more polling required (polling_ms changed) */
+ if (devfreq->next_polling == 0) {
+ mutex_unlock(&devfreq->lock);
+ continue;
+ }
+ } else {
+ devfreq->next_polling -= jiffies_passed;
+ }
+
+ next_jiffies = (next_jiffies > devfreq->next_polling) ?
+ devfreq->next_polling : next_jiffies;
+
+ mutex_unlock(&devfreq->lock);
+ }
+
+ if (next_jiffies > 0 && next_jiffies < ULONG_MAX) {
+ polling = true;
+ queue_delayed_work(devfreq_wq, &devfreq_work, next_jiffies);
+ } else {
+ polling = false;
+ }
+
+ mutex_unlock(&devfreq_list_lock);
+}
+
+/**
+ * devfreq_add_device() - Add devfreq feature to the device
+ * @dev: the device to add devfreq feature.
+ * @profile: device-specific profile to run devfreq.
+ * @governor: the policy to choose frequency.
+ * @data: private data for the governor. The devfreq framework does not
+ * touch this value.
+ */
+int devfreq_add_device(struct device *dev, struct devfreq_dev_profile *profile,
+ struct devfreq_governor *governor, void *data)
+{
+ struct devfreq *devfreq;
+ struct srcu_notifier_head *nh;
+ int err = 0;
+
+ if (!dev || !profile || !governor) {
+ dev_err(dev, "%s: Invalid parameters.\n", __func__);
+ return -EINVAL;
+ }
+
+ mutex_lock(&devfreq_list_lock);
+
+ devfreq = find_device_devfreq(dev);
+ if (!IS_ERR(devfreq)) {
+ dev_err(dev, "%s: Unable to create devfreq for the device. It already has one.\n", __func__);
+ err = -EINVAL;
+ goto out;
+ }
+
+ devfreq = kzalloc(sizeof(struct devfreq), GFP_KERNEL);
+ if (!devfreq) {
+ dev_err(dev, "%s: Unable to create devfreq for the device\n",
+ __func__);
+ err = -ENOMEM;
+ goto out;
+ }
+
+ mutex_init(&devfreq->lock);
+ mutex_lock(&devfreq->lock);
+ devfreq->dev = dev;
+ devfreq->profile = profile;
+ devfreq->governor = governor;
+ devfreq->next_polling = devfreq->polling_jiffies
+ = msecs_to_jiffies(devfreq->profile->polling_ms);
+ devfreq->previous_freq = profile->initial_freq;
+ devfreq->data = data;
+
+ devfreq->nb.notifier_call = devfreq_update;
+
+ nh = opp_get_notifier(dev);
+ if (IS_ERR(nh)) {
+ err = PTR_ERR(nh);
+ goto err_opp;
+ }
+ err = srcu_notifier_chain_register(nh, &devfreq->nb);
+ if (err)
+ goto err_opp;
+
+ if (governor->init)
+ err = governor->init(devfreq);
+ if (err)
+ goto err_init;
+
+ list_add(&devfreq->node, &devfreq_list);
+
+ if (devfreq_wq && devfreq->next_polling && !polling) {
+ polling = true;
+ queue_delayed_work(devfreq_wq, &devfreq_work,
+ devfreq->next_polling);
+ }
+ mutex_unlock(&devfreq->lock);
+ goto out;
+err_init:
+ srcu_notifier_chain_unregister(nh, &devfreq->nb);
+err_opp:
+ mutex_unlock(&devfreq->lock);
+ kfree(devfreq);
+out:
+ mutex_unlock(&devfreq_list_lock);
+ return err;
+}
+
+/**
+ * devfreq_remove_device() - Remove devfreq feature from a device.
+ * @device: the device to remove devfreq feature.
+ */
+int devfreq_remove_device(struct device *dev)
+{
+ struct devfreq *devfreq;
+ struct srcu_notifier_head *nh;
+ int err = 0;
+
+ if (!dev)
+ return -EINVAL;
+
+ mutex_lock(&devfreq_list_lock);
+ devfreq = find_device_devfreq(dev);
+ if (IS_ERR(devfreq)) {
+ err = PTR_ERR(devfreq);
+ goto out;
+ }
+
+ mutex_lock(&devfreq->lock);
+ nh = opp_get_notifier(dev);
+ if (IS_ERR(nh)) {
+ err = PTR_ERR(nh);
+ mutex_unlock(&devfreq->lock);
+ goto out;
+ }
+
+ list_del(&devfreq->node);
+
+ if (devfreq->governor->exit)
+ devfreq->governor->exit(devfreq);
+
+ srcu_notifier_chain_unregister(nh, &devfreq->nb);
+ mutex_unlock(&devfreq->lock);
+ kfree(devfreq);
+out:
+ mutex_unlock(&devfreq_list_lock);
+ return 0;
+}
+
+/**
+ * devfreq_init() - Initialize data structure for devfreq framework and
+ * start polling registered devfreq devices.
+ */
+static int __init devfreq_init(void)
+{
+ mutex_lock(&devfreq_list_lock);
+ polling = false;
+ devfreq_wq = create_freezable_workqueue("devfreq_wq");
+ INIT_DELAYED_WORK_DEFERRABLE(&devfreq_work, devfreq_monitor);
+ mutex_unlock(&devfreq_list_lock);
+
+ devfreq_monitor(&devfreq_work.work);
+ return 0;
+}
+late_initcall(devfreq_init);
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
new file mode 100644
index 0000000..9122090
--- /dev/null
+++ b/drivers/devfreq/governor.h
@@ -0,0 +1,22 @@
+/*
+ * governor.h - internal header for governors.
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This header is for devfreq governors in drivers/devfreq/
+ */
+
+#ifndef _GOVERNOR_H
+#define _GOVERNOR_H
+
+extern struct devfreq *get_devfreq(struct device *dev);
+
+/* (Mandatory) Lock devfreq->lock before calling update_devfreq */
+extern int update_devfreq(struct devfreq *devfreq);
+
+#endif /* _GOVERNOR_H */
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
new file mode 100644
index 0000000..f14b57d
--- /dev/null
+++ b/include/linux/devfreq.h
@@ -0,0 +1,123 @@
+/*
+ * devfreq: Generic Dynamic Voltage and Frequency Scaling (DVFS) Framework
+ * for Non-CPU Devices Based on OPP.
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_DEVFREQ_H__
+#define __LINUX_DEVFREQ_H__
+
+#include <linux/device.h>
+#include <linux/notifier.h>
+#include <linux/opp.h>
+
+#define DEVFREQ_NAME_LEN 16
+
+struct devfreq;
+struct devfreq_dev_status {
+ /* both since the last measure */
+ unsigned long total_time;
+ unsigned long busy_time;
+ unsigned long current_frequency;
+};
+
+struct devfreq_dev_profile {
+ unsigned long max_freq; /* may be larger than the actual value */
+ unsigned long initial_freq;
+ unsigned int polling_ms; /* 0 for at opp change only */
+
+ int (*target)(struct device *dev, struct opp *opp);
+ int (*get_dev_status)(struct device *dev,
+ struct devfreq_dev_status *stat);
+};
+
+/**
+ * struct devfreq_governor - Devfreq policy governor
+ * @name Governor's name
+ * @get_target_freq Returns desired operating frequency for the device.
+ * Basically, get_target_freq will run
+ * devfreq_dev_profile.get_dev_status() to get the
+ * status of the device (load = busy_time / total_time).
+ * @init Called when the devfreq is being attached to a device
+ * @exit Called when the devfreq is being removed from a device
+ *
+ * Note that the callbacks are called with devfreq->lock locked by devfreq.
+ */
+struct devfreq_governor {
+ char name[DEVFREQ_NAME_LEN];
+ int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
+ int (*init)(struct devfreq *this);
+ void (*exit)(struct devfreq *this);
+};
+
+/**
+ * struct devfreq - Device devfreq structure
+ * @node list node - contains the devices with devfreq that have been
+ * registered.
+ * @lock a mutex to protect accessing devfreq.
+ * @dev device pointer
+ * @profile device-specific devfreq profile
+ * @governor method how to choose frequency based on the usage.
+ * @nb notifier block registered to the corresponding OPP to get
+ * notified for frequency availability updates.
+ * @polling_jiffies interval in jiffies.
+ * @previous_freq previously configured frequency value.
+ * @next_polling the number of remaining jiffies to poll with
+ * "devfreq_monitor" executions to reevaluate
+ * frequency/voltage of the device. Set by
+ * profile's polling_ms interval.
+ * @data Private data of the governor. The devfreq framework does not
+ * touch this.
+ *
+ * This structure stores the devfreq information for a give device.
+ *
+ * Note that when a governor accesses entries in struct devfreq in its
+ * functions except for the context of callbacks defined in struct
+ * devfreq_governor, the governor should protect its access with the
+ * struct mutex lock in struct devfreq. A governor may use this mutex
+ * to protect its own private data in void *data as well.
+ */
+struct devfreq {
+ struct list_head node;
+
+ struct mutex lock;
+ struct device *dev;
+ struct devfreq_dev_profile *profile;
+ struct devfreq_governor *governor;
+ struct notifier_block nb;
+
+ unsigned long polling_jiffies;
+ unsigned long previous_freq;
+ unsigned int next_polling;
+
+ void *data; /* private data for governors */
+};
+
+#if defined(CONFIG_PM_DEVFREQ)
+extern int devfreq_add_device(struct device *dev,
+ struct devfreq_dev_profile *profile,
+ struct devfreq_governor *governor,
+ void *data);
+extern int devfreq_remove_device(struct device *dev);
+#else /* !CONFIG_PM_DEVFREQ */
+static int devfreq_add_device(struct device *dev,
+ struct devfreq_dev_profile *profile,
+ struct devfreq_governor *governor,
+ void *data)
+{
+ return 0;
+}
+
+static int devfreq_remove_device(struct device *dev)
+{
+ return 0;
+}
+#endif /* CONFIG_PM_DEVFREQ */
+
+#endif /* __LINUX_DEVFREQ_H__ */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v9 3/4] PM / devfreq: add common sysfs interfaces
2011-08-31 7:29 [PATCH v9 0/4] Devfreq, DVFS Framework for Non-CPU Devices MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 1/4] PM / OPP: Add OPP availability change notifier MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs MyungJoo Ham
@ 2011-08-31 7:29 ` MyungJoo Ham
2011-08-31 21:27 ` Turquette, Mike
2011-08-31 7:29 ` [PATCH v9 4/4] PM / devfreq: add basic governors MyungJoo Ham
3 siblings, 1 reply; 11+ messages in thread
From: MyungJoo Ham @ 2011-08-31 7:29 UTC (permalink / raw)
To: linux-pm; +Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, Thomas Gleixner
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
- polling_interval R: polling interval in ms given with devfreq profile
W: update polling interval.
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
--
Changes from v8
- applied per-devfreq locking mechanism
Changes from v7
- removed set_freq from the common devfreq interface
- added get_devfreq, a mutex-protected wrapper for find_device_devfreq
(for sysfs interfaces and later with governor-support)
- corrected ABI documentation.
Changes from v6
- poling_interval is writable.
Changes from v5
- updated devferq_update usage.
Changes 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)
Changes from v3
- corrected sysfs API usage
- corrected error messages
- moved sysfs entry location
- added sysfs entries
Changes from v2
- add ABI entries for devfreq sysfs interface
---
Documentation/ABI/testing/sysfs-devices-power | 37 +++++
drivers/devfreq/devfreq.c | 203 +++++++++++++++++++++++++
2 files changed, 240 insertions(+), 0 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
index 8ffbc25..57f4591 100644
--- a/Documentation/ABI/testing/sysfs-devices-power
+++ b/Documentation/ABI/testing/sysfs-devices-power
@@ -165,3 +165,40 @@ 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@samsung.com>
+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@samsung.com>
+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@samsung.com>
+Description:
+ The /sys/devices/.../power/devfreq_max_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@samsung.com>
+Description:
+ The /sys/devices/.../power/devfreq_min_freq shows the
+ minimum operable frequency of the corresponding device.
+
+What: /sys/devices/.../power/devfreq_polling_interval
+Date: July 2011
+Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
+Description:
+ The /sys/devices/.../power/devfreq_polling_interval sets and
+ shows the requested polling interval of the corresponding
+ device. The values are represented in ms. If the value is less
+ than 1 jiffy, it is considered to be 0, which means no polling.
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 621b863..1c46052 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.
@@ -191,6 +193,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);
mutex_unlock(&devfreq->lock);
kfree(devfreq);
@@ -293,6 +297,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);
mutex_unlock(&devfreq->lock);
goto out;
err_init:
@@ -333,6 +339,8 @@ int devfreq_remove_device(struct device *dev)
goto out;
}
+ sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
+
list_del(&devfreq->node);
if (devfreq->governor->exit)
@@ -346,6 +354,201 @@ out:
return 0;
}
+static ssize_t show_governor(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct devfreq *df;
+ ssize_t ret;
+
+ mutex_lock(&devfreq_list_lock);
+ df = find_device_devfreq(dev);
+ if (IS_ERR(df)) {
+ ret = PTR_ERR(df);
+ goto out;
+ }
+
+ mutex_lock(&df->lock);
+ if (!df->governor) {
+ ret = -EINVAL;
+ goto out_l;
+ }
+
+ ret = sprintf(buf, "%s\n", df->governor->name);
+out_l:
+ mutex_unlock(&df->lock);
+out:
+ mutex_unlock(&devfreq_list_lock);
+ return ret;
+}
+
+static ssize_t show_freq(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct devfreq *df;
+ ssize_t ret;
+
+ mutex_lock(&devfreq_list_lock);
+ df = find_device_devfreq(dev);
+ if (IS_ERR(df)) {
+ ret = PTR_ERR(df);
+ goto out;
+ }
+
+ ret = sprintf(buf, "%lu\n", df->previous_freq);
+out:
+ mutex_unlock(&devfreq_list_lock);
+ return ret;
+}
+
+static ssize_t show_max_freq(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct devfreq *df;
+ ssize_t ret;
+ unsigned long freq = ULONG_MAX;
+ struct opp *opp;
+
+ mutex_lock(&devfreq_list_lock);
+ df = find_device_devfreq(dev);
+ if (IS_ERR(df)) {
+ ret = PTR_ERR(df);
+ goto out;
+ }
+
+ mutex_lock(&df->lock);
+ opp = opp_find_freq_floor(df->dev, &freq);
+ if (IS_ERR(opp)) {
+ ret = PTR_ERR(opp);
+ goto out_l;
+ }
+
+ ret = sprintf(buf, "%lu\n", freq);
+out_l:
+ mutex_unlock(&df->lock);
+out:
+ mutex_unlock(&devfreq_list_lock);
+ return ret;
+}
+
+static ssize_t show_min_freq(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct devfreq *df;
+ ssize_t ret;
+ unsigned long freq = 0;
+ struct opp *opp;
+
+ mutex_lock(&devfreq_list_lock);
+ df = find_device_devfreq(dev);
+ if (IS_ERR(df)) {
+ ret = PTR_ERR(df);
+ goto out;
+ }
+
+ mutex_lock(&df->lock);
+ opp = opp_find_freq_ceil(df->dev, &freq);
+ if (IS_ERR(opp)) {
+ ret = PTR_ERR(opp);
+ goto out_l;
+ }
+
+ ret = sprintf(buf, "%lu\n", freq);
+out_l:
+ mutex_unlock(&df->lock);
+out:
+ mutex_unlock(&devfreq_list_lock);
+ return ret;
+}
+
+static ssize_t show_polling_interval(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct devfreq *df;
+ ssize_t ret;
+
+ mutex_lock(&devfreq_list_lock);
+ df = find_device_devfreq(dev);
+ if (IS_ERR(df)) {
+ ret = PTR_ERR(df);
+ goto out;
+ }
+
+ mutex_lock(&df->lock);
+ if (!df->profile) {
+ ret = -EINVAL;
+ goto out_l;
+ }
+
+ ret = sprintf(buf, "%d\n", df->profile->polling_ms);
+out_l:
+ mutex_unlock(&df->lock);
+out:
+ mutex_unlock(&devfreq_list_lock);
+ return ret;
+}
+
+static ssize_t store_polling_interval(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct devfreq *df;
+ unsigned int value;
+ int ret;
+
+ mutex_lock(&devfreq_list_lock);
+ df = find_device_devfreq(dev);
+ if (IS_ERR(df)) {
+ count = PTR_ERR(df);
+ goto out;
+ }
+ mutex_lock(&df->lock);
+ if (!df->profile) {
+ count = -EINVAL;
+ goto out_l;
+ }
+
+ ret = sscanf(buf, "%u", &value);
+ if (ret != 1) {
+ count = -EINVAL;
+ goto out_l;
+ }
+
+ df->profile->polling_ms = value;
+ df->next_polling = df->polling_jiffies
+ = msecs_to_jiffies(value);
+
+ if (df->next_polling > 0 && !polling) {
+ polling = true;
+ queue_delayed_work(devfreq_wq, &devfreq_work,
+ df->next_polling);
+ }
+out_l:
+ mutex_unlock(&df->lock);
+out:
+ mutex_unlock(&devfreq_list_lock);
+
+ return count;
+}
+
+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, 0644, show_polling_interval,
+ store_polling_interval);
+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,
+ 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v9 4/4] PM / devfreq: add basic governors
2011-08-31 7:29 [PATCH v9 0/4] Devfreq, DVFS Framework for Non-CPU Devices MyungJoo Ham
` (2 preceding siblings ...)
2011-08-31 7:29 ` [PATCH v9 3/4] PM / devfreq: add common sysfs interfaces MyungJoo Ham
@ 2011-08-31 7:29 ` MyungJoo Ham
3 siblings, 0 replies; 11+ messages in thread
From: MyungJoo Ham @ 2011-08-31 7:29 UTC (permalink / raw)
To: linux-pm; +Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, Thomas Gleixner
Four cpufreq-like governors are provided as examples.
powersave: use the lowest frequency possible. The user (device) should
set the polling_ms as 0 because polling is useless for this governor.
performance: use the highest freqeuncy possible. The user (device)
should set the polling_ms as 0 because polling is useless for this
governor.
userspace: use the user specified frequency stored at
devfreq.user_set_freq. With sysfs support in the following patch, a user
may set the value with the sysfs interface.
simple_ondemand: simplified version of cpufreq's ondemand governor.
When a user updates OPP entries (enable/disable/add), OPP framework
automatically notifies devfreq to update operating frequency
accordingly. Thus, devfreq users (device drivers) do not need to update
devfreq manually with OPP entry updates or set polling_ms for powersave
, performance, userspace, or any other "static" governors.
Note that these are given only as basic examples for governors and any
devices with devfreq may implement their own governors with the drivers
and use them.
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changed from v8
- Removed unnecessary header entries
Changed from v7
- Userspace uses its own sysfs interface.
Changed from v5
- Seperated governor files from devfreq.c
- Allow simple ondemand to be tuned for each device
---
Documentation/ABI/testing/sysfs-devices-power | 9 ++
drivers/devfreq/Kconfig | 36 +++++++
drivers/devfreq/Makefile | 4 +
drivers/devfreq/governor_performance.c | 24 +++++
drivers/devfreq/governor_powersave.c | 24 +++++
drivers/devfreq/governor_simpleondemand.c | 88 +++++++++++++++++
drivers/devfreq/governor_userspace.c | 126 +++++++++++++++++++++++++
include/linux/devfreq.h | 37 +++++++
8 files changed, 348 insertions(+), 0 deletions(-)
create mode 100644 drivers/devfreq/governor_performance.c
create mode 100644 drivers/devfreq/governor_powersave.c
create mode 100644 drivers/devfreq/governor_simpleondemand.c
create mode 100644 drivers/devfreq/governor_userspace.c
diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
index 57f4591..c7f6977 100644
--- a/Documentation/ABI/testing/sysfs-devices-power
+++ b/Documentation/ABI/testing/sysfs-devices-power
@@ -202,3 +202,12 @@ Description:
shows the requested polling interval of the corresponding
device. The values are represented in ms. If the value is less
than 1 jiffy, it is considered to be 0, which means no polling.
+
+What: /sys/devices/.../power/devfreq_userspace_set_freq
+Date: August 2011
+Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
+Description:
+ The /sys/devices/.../power/devfreq_userspace_set_freq sets
+ and shows the user specified frequency in kHz. This sysfs
+ entry is created and managed by userspace DEVFREQ governor.
+ If other governors are used, it won't be supported.
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 1fb42de..643b055 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -34,6 +34,42 @@ menuconfig PM_DEVFREQ
if PM_DEVFREQ
+comment "DEVFREQ Governors"
+
+config DEVFREQ_GOV_SIMPLE_ONDEMAND
+ bool "Simple Ondemand"
+ help
+ Chooses frequency based on the recent load on the device. Works
+ similar as ONDEMAND governor of CPUFREQ does. A device with
+ Simple-Ondemand should be able to provide busy/total counter
+ values that imply the usage rate. A device may provide tuned
+ values to the governor with data field at devfreq_add_device().
+
+config DEVFREQ_GOV_PERFORMANCE
+ bool "Performance"
+ help
+ Sets the frequency at the maximum available frequency.
+ This governor always returns UINT_MAX as frequency so that
+ the DEVFREQ framework returns the highest frequency available
+ at any time.
+
+config DEVFREQ_GOV_POWERSAVE
+ bool "Powersave"
+ help
+ Sets the frequency at the minimum available frequency.
+ This governor always returns 0 as frequency so that
+ the DEVFREQ framework returns the lowest frequency available
+ at any time.
+
+config DEVFREQ_GOV_USERSPACE
+ bool "Userspace"
+ help
+ Sets the frequency at the user specified one.
+ This governor returns the user configured frequency if there
+ has been an input to /sys/devices/.../power/devfreq_set_freq.
+ Otherwise, the governor does not change the frequnecy
+ given at the initialization.
+
comment "DEVFREQ Drivers"
endif # PM_DEVFREQ
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 168934a..4564a89 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -1 +1,5 @@
obj-$(CONFIG_PM_DEVFREQ) += devfreq.o
+obj-$(CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND) += governor_simpleondemand.o
+obj-$(CONFIG_DEVFREQ_GOV_PERFORMANCE) += governor_performance.o
+obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
+obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
new file mode 100644
index 0000000..c47eff8
--- /dev/null
+++ b/drivers/devfreq/governor_performance.c
@@ -0,0 +1,24 @@
+/*
+ * linux/drivers/devfreq/governor_performance.c
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/devfreq.h>
+
+static int devfreq_performance_func(struct devfreq *df,
+ unsigned long *freq)
+{
+ *freq = UINT_MAX; /* devfreq_do will run "floor" */
+ return 0;
+}
+
+struct devfreq_governor devfreq_performance = {
+ .name = "performance",
+ .get_target_freq = devfreq_performance_func,
+};
diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c
new file mode 100644
index 0000000..4f128d8
--- /dev/null
+++ b/drivers/devfreq/governor_powersave.c
@@ -0,0 +1,24 @@
+/*
+ * linux/drivers/devfreq/governor_powersave.c
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/devfreq.h>
+
+static int devfreq_powersave_func(struct devfreq *df,
+ unsigned long *freq)
+{
+ *freq = 0; /* devfreq_do will run "ceiling" to 0 */
+ return 0;
+}
+
+struct devfreq_governor devfreq_powersave = {
+ .name = "powersave",
+ .get_target_freq = devfreq_powersave_func,
+};
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
new file mode 100644
index 0000000..18fe8be
--- /dev/null
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -0,0 +1,88 @@
+/*
+ * linux/drivers/devfreq/governor_simpleondemand.c
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/errno.h>
+#include <linux/devfreq.h>
+#include <linux/math64.h>
+
+/* Default constants for DevFreq-Simple-Ondemand (DFSO) */
+#define DFSO_UPTHRESHOLD (90)
+#define DFSO_DOWNDIFFERENCTIAL (5)
+static int devfreq_simple_ondemand_func(struct devfreq *df,
+ unsigned long *freq)
+{
+ struct devfreq_dev_status stat;
+ int err = df->profile->get_dev_status(df->dev, &stat);
+ unsigned long long a, b;
+ unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
+ unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
+ struct devfreq_simple_ondemand_data *data = df->data;
+
+ if (err)
+ return err;
+
+ if (data) {
+ if (data->upthreshold)
+ dfso_upthreshold = data->upthreshold;
+ if (data->downdifferential)
+ dfso_downdifferential = data->downdifferential;
+ }
+ if (dfso_upthreshold > 100 ||
+ dfso_upthreshold < dfso_downdifferential)
+ return -EINVAL;
+
+ /* Assume MAX if it is going to be divided by zero */
+ if (stat.total_time == 0) {
+ *freq = UINT_MAX;
+ return 0;
+ }
+
+ /* Prevent overflow */
+ if (stat.busy_time >= (1 << 24) || stat.total_time >= (1 << 24)) {
+ stat.busy_time >>= 7;
+ stat.total_time >>= 7;
+ }
+
+ /* Set MAX if it's busy enough */
+ if (stat.busy_time * 100 >
+ stat.total_time * dfso_upthreshold) {
+ *freq = UINT_MAX;
+ return 0;
+ }
+
+ /* Set MAX if we do not know the initial frequency */
+ if (stat.current_frequency == 0) {
+ *freq = UINT_MAX;
+ return 0;
+ }
+
+ /* Keep the current frequency */
+ if (stat.busy_time * 100 >
+ stat.total_time * (dfso_upthreshold - dfso_downdifferential)) {
+ *freq = stat.current_frequency;
+ return 0;
+ }
+
+ /* Set the desired frequency based on the load */
+ a = stat.busy_time;
+ a *= stat.current_frequency;
+ b = div_u64(a, stat.total_time);
+ b *= 100;
+ b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
+ *freq = (unsigned long) b;
+
+ return 0;
+}
+
+struct devfreq_governor devfreq_simple_ondemand = {
+ .name = "simple_ondemand",
+ .get_target_freq = devfreq_simple_ondemand_func,
+};
diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
new file mode 100644
index 0000000..490167f
--- /dev/null
+++ b/drivers/devfreq/governor_userspace.c
@@ -0,0 +1,126 @@
+/*
+ * linux/drivers/devfreq/governor_simpleondemand.c
+ *
+ * Copyright (C) 2011 Samsung Electronics
+ * MyungJoo Ham <myungjoo.ham@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/devfreq.h>
+#include <linux/pm.h>
+#include <linux/mutex.h>
+#include "governor.h"
+
+struct userspace_data {
+ unsigned long user_frequency;
+ bool valid;
+};
+
+static int devfreq_userspace_func(struct devfreq *df, unsigned long *freq)
+{
+ struct userspace_data *data = df->data;
+
+ if (!data->valid)
+ *freq = df->previous_freq; /* No user freq specified yet */
+ else
+ *freq = data->user_frequency;
+ return 0;
+}
+
+static ssize_t store_freq(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct devfreq *devfreq = get_devfreq(dev);
+ struct userspace_data *data;
+ unsigned long wanted;
+ int err = 0;
+
+ if (IS_ERR(devfreq)) {
+ err = PTR_ERR(devfreq);
+ goto out;
+ }
+
+ mutex_lock(&devfreq->lock);
+ data = devfreq->data;
+
+ sscanf(buf, "%lu", &wanted);
+ data->user_frequency = wanted;
+ data->valid = true;
+ err = update_devfreq(devfreq);
+ if (err == 0)
+ err = count;
+ mutex_unlock(&devfreq->lock);
+out:
+ return err;
+}
+
+static ssize_t show_freq(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct devfreq *devfreq = get_devfreq(dev);
+ struct userspace_data *data;
+ int err = 0;
+
+ if (IS_ERR(devfreq)) {
+ err = PTR_ERR(devfreq);
+ goto out;
+ }
+
+ mutex_lock(&devfreq->lock);
+ data = devfreq->data;
+
+ if (data->valid)
+ err = sprintf(buf, "%lu\n", data->user_frequency);
+ else
+ err = sprintf(buf, "undefined\n");
+ mutex_unlock(&devfreq->lock);
+out:
+ return err;
+}
+
+static DEVICE_ATTR(devfreq_userspace_set_freq, 0644, show_freq, store_freq);
+static struct attribute *dev_entries[] = {
+ &dev_attr_devfreq_userspace_set_freq.attr,
+ NULL,
+};
+static struct attribute_group dev_attr_group = {
+ .name = power_group_name,
+ .attrs = dev_entries,
+};
+
+static int userspace_init(struct devfreq *devfreq)
+{
+ int err = 0;
+ struct userspace_data *data = kzalloc(sizeof(struct userspace_data),
+ GFP_KERNEL);
+
+ if (!data) {
+ err = -ENOMEM;
+ goto out;
+ }
+ data->valid = false;
+ devfreq->data = data;
+
+ sysfs_merge_group(&devfreq->dev->kobj, &dev_attr_group);
+out:
+ return err;
+}
+
+static void userspace_exit(struct devfreq *devfreq)
+{
+ sysfs_unmerge_group(&devfreq->dev->kobj, &dev_attr_group);
+ kfree(devfreq->data);
+ devfreq->data = NULL;
+}
+
+struct devfreq_governor devfreq_userspace = {
+ .name = "userspace",
+ .get_target_freq = devfreq_userspace_func,
+ .init = userspace_init,
+ .exit = userspace_exit,
+};
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index f14b57d..5b802a6 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -105,6 +105,37 @@ extern int devfreq_add_device(struct device *dev,
struct devfreq_governor *governor,
void *data);
extern int devfreq_remove_device(struct device *dev);
+
+#ifdef CONFIG_DEVFREQ_GOV_POWERSAVE
+extern struct devfreq_governor devfreq_powersave;
+#endif
+#ifdef CONFIG_DEVFREQ_GOV_PERFORMANCE
+extern struct devfreq_governor devfreq_performance;
+#endif
+#ifdef CONFIG_DEVFREQ_GOV_USERSPACE
+extern struct devfreq_governor devfreq_userspace;
+#endif
+#ifdef CONFIG_DEVFREQ_GOV_SIMPLE_ONDEMAND
+extern struct devfreq_governor devfreq_simple_ondemand;
+/**
+ * struct devfreq_simple_ondemand_data - void *data fed to struct devfreq
+ * and devfreq_add_device
+ * @ upthreshold If the load is over this value, the frequency jumps.
+ * Specify 0 to use the default. Valid value = 0 to 100.
+ * @ downdifferential If the load is under upthreshold - downdifferential,
+ * the governor may consider slowing the frequency down.
+ * Specify 0 to use the default. Valid value = 0 to 100.
+ * downdifferential < upthreshold must hold.
+ *
+ * If the fed devfreq_simple_ondemand_data pointer is NULL to the governor,
+ * the governor uses the default values.
+ */
+struct devfreq_simple_ondemand_data {
+ unsigned int upthreshold;
+ unsigned int downdifferential;
+};
+#endif
+
#else /* !CONFIG_PM_DEVFREQ */
static int devfreq_add_device(struct device *dev,
struct devfreq_dev_profile *profile,
@@ -118,6 +149,12 @@ static int devfreq_remove_device(struct device *dev)
{
return 0;
}
+
+#define devfreq_powersave NULL
+#define devfreq_performance NULL
+#define devfreq_userspace NULL
+#define devfreq_simple_ondemand NULL
+
#endif /* CONFIG_PM_DEVFREQ */
#endif /* __LINUX_DEVFREQ_H__ */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs
2011-08-31 7:29 ` [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs MyungJoo Ham
@ 2011-08-31 20:05 ` Turquette, Mike
2011-09-01 4:51 ` MyungJoo Ham
0 siblings, 1 reply; 11+ messages in thread
From: Turquette, Mike @ 2011-08-31 20:05 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm, Thomas Gleixner
On Wed, Aug 31, 2011 at 12:29 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
[snip]
> +/**
> + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
> + * with mutex protection. exported for governors
> + * @dev: device pointer used to lookup device devfreq.
> + */
> +struct devfreq *get_devfreq(struct device *dev)
> +{
> + struct devfreq *ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + ret = find_device_devfreq(dev);
> + mutex_unlock(&devfreq_list_lock);
You prevent changes to the devfreq list while searching (good) but
after returning the pointer there is no protection from that item
being removed from the list. Generally "get" and "put" functions do
more than just return a pointer: get functions often increment a
refcount, or hold a lock. The put function will decrement the
refcount or release the lock. Maybe you want something like the
following:
mutex_lock(&devfreq_list_lock);
ret = find_device_devfreq(dev);
mutex_lock(&devfreq->lock);
mutex_unlock(&devfreq_list_lock);
Then you need a corresponding put which does the mutex_unlock(&devfreq->lock).
It looks like the only consumers of get_devfreq are the sysfs
show/store interfaces, which immediately hold devfreq->lock, so the
above proposal certainly makes fits the existing use case.
Also CPUfreq's "cpufreq_get" function does a nice job of protecting
the object from getting freed with a rw_semaphore. It is a bit more
"complicated" but makes for good reading.
> +
> + return ret;
> +}
> +
> +/**
> + * devfreq_do() - Check the usage profile of a given device and configure
> + * frequency and voltage accordingly
> + * @devfreq: devfreq info of the given device
> + */
> +static int devfreq_do(struct devfreq *devfreq)
> +{
> + struct opp *opp;
> + unsigned long freq;
> + int err;
> +
Put the mutex_is_locked check here? See more below.
> + err = devfreq->governor->get_target_freq(devfreq, &freq);
> + if (err)
> + return err;
> +
> + opp = opp_find_freq_ceil(devfreq->dev, &freq);
> + if (opp == ERR_PTR(-ENODEV))
> + opp = opp_find_freq_floor(devfreq->dev, &freq);
> +
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> +
> + if (devfreq->previous_freq == freq)
> + return 0;
> +
> + err = devfreq->profile->target(devfreq->dev, opp);
> + if (err)
> + return err;
> +
> + devfreq->previous_freq = freq;
> + return 0;
> +}
> +
> +/**
> + * update_devfreq() - Notify that the device OPP or frequency requirement
> + * has been changed. This function is exported for governors.
> + * @devfreq: the devfreq instance.
> + *
> + * Note: lock devfreq->lock before calling update_devfreq
> + */
> +int update_devfreq(struct devfreq *devfreq)
> +{
> + int err = 0;
> +
> + if (!mutex_is_locked(&devfreq->lock)) {
> + WARN(true, "devfreq->lock must be locked by the caller.\n");
> + return -EINVAL;
> + }
> +
> + /* Reevaluate the proper frequency */
> + err = devfreq_do(devfreq);
> + return err;
> +}
> +
> +/**
> + * devfreq_update() - Notify that the device OPP has been changed.
> + * @dev: the device whose OPP has been changed.
> + *
> + * Called by OPP notifier.
> + */
> +static int devfreq_update(struct notifier_block *nb, unsigned long type,
> + void *devp)
> +{
> + struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
> + int ret;
> +
> + mutex_lock(&devfreq->lock);
> + ret = update_devfreq(devfreq);
> + mutex_unlock(&devfreq->lock);
The whole devfreq_update/update_devfreq pairing is redundant.
update_devfreq's purpose is to make sure the lock is held before going
further, and the only caller of update_devfreq is devfreq_update which
always holds the lock.
This still doesn't stop a bad driver writer from just calling
devfreq_do with an extern. Perhaps the lock detection should be moved
into devfreq_do and update_devfreq should go away?
> +
> + return ret;
> +}
> +
> +/**
> + * devfreq_monitor() - Periodically run devfreq_do()
> + * @work: the work struct used to run devfreq_monitor periodically.
> + *
> + */
> +static void devfreq_monitor(struct work_struct *work)
> +{
> + static unsigned long last_polled_at;
> + struct devfreq *devfreq, *tmp;
> + int error;
> + unsigned long jiffies_passed;
> + unsigned long next_jiffies = ULONG_MAX, now = jiffies;
> +
> + /* Initially last_polled_at = 0, polling every device at bootup */
> + jiffies_passed = now - last_polled_at;
> + last_polled_at = now;
> + if (jiffies_passed == 0)
> + jiffies_passed = 1;
> +
> + mutex_lock(&devfreq_list_lock);
Should not lock the list here. If we lock the list for all major
operations, it nullifies the performance benefit of having a mutex in
struct devfreq.
> +
> + list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
> + mutex_lock(&devfreq->lock);
> +
> + if (devfreq->next_polling == 0) {
> + mutex_unlock(&devfreq->lock);
> + continue;
> + }
> +
> + /*
> + * Reduce more next_polling if devfreq_wq took an extra
> + * delay. (i.e., CPU has been idled.)
> + */
> + if (devfreq->next_polling <= jiffies_passed) {
> + error = devfreq_do(devfreq);
> +
> + /* Remove a devfreq with an error. */
> + if (error && error != -EAGAIN) {
> + dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
> + error, devfreq->governor->name);
> +
> + list_del(&devfreq->node);
> + mutex_unlock(&devfreq->lock);
> + kfree(devfreq);
> + continue;
Should this error handling also unregister the OPP notifier? This
code duplicates portions of devfreq_remove_device. I propose instead
here we do the following:
mutex_unlock(&devfreq->lock);
_devfreq_remove_lock(devfreq);
/* this locks the list first,
* then locks devfreq->lock,
* then does the house cleaning
*/
continue;
> + }
> + devfreq->next_polling = devfreq->polling_jiffies;
> +
> + /* No more polling required (polling_ms changed) */
> + if (devfreq->next_polling == 0) {
> + mutex_unlock(&devfreq->lock);
> + continue;
> + }
> + } else {
> + devfreq->next_polling -= jiffies_passed;
> + }
> +
> + next_jiffies = (next_jiffies > devfreq->next_polling) ?
> + devfreq->next_polling : next_jiffies;
> +
> + mutex_unlock(&devfreq->lock);
> + }
> +
> + if (next_jiffies > 0 && next_jiffies < ULONG_MAX) {
> + polling = true;
> + queue_delayed_work(devfreq_wq, &devfreq_work, next_jiffies);
> + } else {
> + polling = false;
> + }
> +
> + mutex_unlock(&devfreq_list_lock);
Again, list should not be locked over this whole function. It blocks
other unrelated devfreq devices from scaling.
> +}
[snip]
> +/**
> + * devfreq_remove_device() - Remove devfreq feature from a device.
> + * @device: the device to remove devfreq feature.
> + */
> +int devfreq_remove_device(struct device *dev)
Why does this take a struct device*? Shouldn't it be a struct devfreq*?
If there is a case for removing a devfreq device with only struct
device* as input, how about:
int devfreq_remove_device(struct device *dev)
{
struct devfreq *devfreq;
mutex_lock(&devfreq_list_lock);
devfreq = find_device_devfreq(dev);
mutex_unlock(&devfreq_list_lock);
return _devfreq_remove_device(struct devfreq *df);
/* _devfreq_remove_device does the real work and can also be
called from devfreq_monitor */
}
Regards,
Mike
> +{
> + struct devfreq *devfreq;
> + struct srcu_notifier_head *nh;
> + int err = 0;
> +
> + if (!dev)
> + return -EINVAL;
> +
> + mutex_lock(&devfreq_list_lock);
> + devfreq = find_device_devfreq(dev);
> + if (IS_ERR(devfreq)) {
> + err = PTR_ERR(devfreq);
> + goto out;
> + }
> +
> + mutex_lock(&devfreq->lock);
> + nh = opp_get_notifier(dev);
> + if (IS_ERR(nh)) {
> + err = PTR_ERR(nh);
> + mutex_unlock(&devfreq->lock);
> + goto out;
> + }
> +
> + list_del(&devfreq->node);
> +
> + if (devfreq->governor->exit)
> + devfreq->governor->exit(devfreq);
> +
> + srcu_notifier_chain_unregister(nh, &devfreq->nb);
> + mutex_unlock(&devfreq->lock);
> + kfree(devfreq);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> + return 0;
> +}
[snip]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v9 3/4] PM / devfreq: add common sysfs interfaces
2011-08-31 7:29 ` [PATCH v9 3/4] PM / devfreq: add common sysfs interfaces MyungJoo Ham
@ 2011-08-31 21:27 ` Turquette, Mike
0 siblings, 0 replies; 11+ messages in thread
From: Turquette, Mike @ 2011-08-31 21:27 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm, Thomas Gleixner
On Wed, Aug 31, 2011 at 12:29 AM, MyungJoo Ham <myungjoo.ham@samsung.com> 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
> - polling_interval R: polling interval in ms given with devfreq profile
> W: update polling interval.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
> --
> Changes from v8
> - applied per-devfreq locking mechanism
>
> Changes from v7
> - removed set_freq from the common devfreq interface
> - added get_devfreq, a mutex-protected wrapper for find_device_devfreq
> (for sysfs interfaces and later with governor-support)
> - corrected ABI documentation.
>
> Changes from v6
> - poling_interval is writable.
>
> Changes from v5
> - updated devferq_update usage.
>
> Changes 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)
>
> Changes from v3
> - corrected sysfs API usage
> - corrected error messages
> - moved sysfs entry location
> - added sysfs entries
>
> Changes from v2
> - add ABI entries for devfreq sysfs interface
> ---
> Documentation/ABI/testing/sysfs-devices-power | 37 +++++
> drivers/devfreq/devfreq.c | 203 +++++++++++++++++++++++++
> 2 files changed, 240 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-power b/Documentation/ABI/testing/sysfs-devices-power
> index 8ffbc25..57f4591 100644
> --- a/Documentation/ABI/testing/sysfs-devices-power
> +++ b/Documentation/ABI/testing/sysfs-devices-power
> @@ -165,3 +165,40 @@ 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@samsung.com>
> +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@samsung.com>
> +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@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_max_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@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_min_freq shows the
> + minimum operable frequency of the corresponding device.
> +
> +What: /sys/devices/.../power/devfreq_polling_interval
> +Date: July 2011
> +Contact: MyungJoo Ham <myungjoo.ham@samsung.com>
> +Description:
> + The /sys/devices/.../power/devfreq_polling_interval sets and
> + shows the requested polling interval of the corresponding
> + device. The values are represented in ms. If the value is less
> + than 1 jiffy, it is considered to be 0, which means no polling.
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 621b863..1c46052 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.
> @@ -191,6 +193,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);
> mutex_unlock(&devfreq->lock);
> kfree(devfreq);
> @@ -293,6 +297,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);
> mutex_unlock(&devfreq->lock);
> goto out;
> err_init:
> @@ -333,6 +339,8 @@ int devfreq_remove_device(struct device *dev)
> goto out;
> }
>
> + sysfs_unmerge_group(&dev->kobj, &dev_attr_group);
> +
> list_del(&devfreq->node);
>
> if (devfreq->governor->exit)
> @@ -346,6 +354,201 @@ out:
> return 0;
> }
>
> +static ssize_t show_governor(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df;
> + ssize_t ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + df = find_device_devfreq(dev);
> + if (IS_ERR(df)) {
> + ret = PTR_ERR(df);
> + goto out;
> + }
> +
> + mutex_lock(&df->lock);
> + if (!df->governor) {
> + ret = -EINVAL;
> + goto out_l;
> + }
> +
> + ret = sprintf(buf, "%s\n", df->governor->name);
> +out_l:
> + mutex_unlock(&df->lock);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> + return ret;
> +}
> +
> +static ssize_t show_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df;
> + ssize_t ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + df = find_device_devfreq(dev);
> + if (IS_ERR(df)) {
> + ret = PTR_ERR(df);
> + goto out;
> + }
> +
> + ret = sprintf(buf, "%lu\n", df->previous_freq);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> + return ret;
> +}
> +
> +static ssize_t show_max_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df;
> + ssize_t ret;
> + unsigned long freq = ULONG_MAX;
> + struct opp *opp;
> +
> + mutex_lock(&devfreq_list_lock);
> + df = find_device_devfreq(dev);
> + if (IS_ERR(df)) {
> + ret = PTR_ERR(df);
> + goto out;
> + }
> +
> + mutex_lock(&df->lock);
> + opp = opp_find_freq_floor(df->dev, &freq);
> + if (IS_ERR(opp)) {
> + ret = PTR_ERR(opp);
> + goto out_l;
> + }
> +
> + ret = sprintf(buf, "%lu\n", freq);
> +out_l:
> + mutex_unlock(&df->lock);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> + return ret;
> +}
> +
> +static ssize_t show_min_freq(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df;
> + ssize_t ret;
> + unsigned long freq = 0;
> + struct opp *opp;
> +
> + mutex_lock(&devfreq_list_lock);
> + df = find_device_devfreq(dev);
> + if (IS_ERR(df)) {
> + ret = PTR_ERR(df);
> + goto out;
> + }
> +
> + mutex_lock(&df->lock);
> + opp = opp_find_freq_ceil(df->dev, &freq);
> + if (IS_ERR(opp)) {
> + ret = PTR_ERR(opp);
> + goto out_l;
> + }
> +
> + ret = sprintf(buf, "%lu\n", freq);
> +out_l:
> + mutex_unlock(&df->lock);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> + return ret;
> +}
> +
> +static ssize_t show_polling_interval(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct devfreq *df;
> + ssize_t ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + df = find_device_devfreq(dev);
> + if (IS_ERR(df)) {
> + ret = PTR_ERR(df);
> + goto out;
> + }
> +
> + mutex_lock(&df->lock);
> + if (!df->profile) {
> + ret = -EINVAL;
> + goto out_l;
> + }
> +
> + ret = sprintf(buf, "%d\n", df->profile->polling_ms);
> +out_l:
> + mutex_unlock(&df->lock);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> + return ret;
> +}
> +
> +static ssize_t store_polling_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct devfreq *df;
> + unsigned int value;
> + int ret;
> +
> + mutex_lock(&devfreq_list_lock);
> + df = find_device_devfreq(dev);
> + if (IS_ERR(df)) {
> + count = PTR_ERR(df);
> + goto out;
> + }
> + mutex_lock(&df->lock);
> + if (!df->profile) {
> + count = -EINVAL;
> + goto out_l;
> + }
> +
> + ret = sscanf(buf, "%u", &value);
> + if (ret != 1) {
> + count = -EINVAL;
> + goto out_l;
> + }
> +
> + df->profile->polling_ms = value;
> + df->next_polling = df->polling_jiffies
> + = msecs_to_jiffies(value);
> +
> + if (df->next_polling > 0 && !polling) {
> + polling = true;
> + queue_delayed_work(devfreq_wq, &devfreq_work,
> + df->next_polling);
> + }
> +out_l:
> + mutex_unlock(&df->lock);
> +out:
> + mutex_unlock(&devfreq_list_lock);
> +
> + return count;
> +}
> +
> +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, 0644, show_polling_interval,
Instead of DEVICE_ATTR, why don't you create your own ktype specific
to devfreq? That would also mean that you don't have to do the struct
device * conversions to get struct devfreq * everytime (which requires
locking and walking the list).
Regards,
Mike
> + store_polling_interval);
> +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,
> + 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
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs
2011-08-31 20:05 ` Turquette, Mike
@ 2011-09-01 4:51 ` MyungJoo Ham
2011-09-01 16:57 ` Turquette, Mike
0 siblings, 1 reply; 11+ messages in thread
From: MyungJoo Ham @ 2011-09-01 4:51 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm, Thomas Gleixner
On Thu, Sep 1, 2011 at 5:05 AM, Turquette, Mike <mturquette@ti.com> wrote:
> On Wed, Aug 31, 2011 at 12:29 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> [snip]
>> +/**
>> + * get_devfreq() - find devfreq struct. a wrapped find_device_devfreq()
>> + * with mutex protection. exported for governors
>> + * @dev: device pointer used to lookup device devfreq.
>> + */
>> +struct devfreq *get_devfreq(struct device *dev)
>> +{
>> + struct devfreq *ret;
>> +
>> + mutex_lock(&devfreq_list_lock);
>> + ret = find_device_devfreq(dev);
>> + mutex_unlock(&devfreq_list_lock);
>
> You prevent changes to the devfreq list while searching (good) but
> after returning the pointer there is no protection from that item
> being removed from the list. Generally "get" and "put" functions do
> more than just return a pointer: get functions often increment a
> refcount, or hold a lock. The put function will decrement the
> refcount or release the lock. Maybe you want something like the
> following:
>
> mutex_lock(&devfreq_list_lock);
> ret = find_device_devfreq(dev);
> mutex_lock(&devfreq->lock);
> mutex_unlock(&devfreq_list_lock);
>
> Then you need a corresponding put which does the mutex_unlock(&devfreq->lock).
>
> It looks like the only consumers of get_devfreq are the sysfs
> show/store interfaces, which immediately hold devfreq->lock, so the
> above proposal certainly makes fits the existing use case.
>
> Also CPUfreq's "cpufreq_get" function does a nice job of protecting
> the object from getting freed with a rw_semaphore. It is a bit more
> "complicated" but makes for good reading.
>
Thank you. The possibility that someone may do something on struct
devfreq after mutex_unlock(&devfreq_list_lock) and before
mutex_lock(&devfreq->lock) bothered me and it appears that I need to
add devfreq_put() anyway. I hesitated it because users might forget
using devfreq_put() after calling devfreq_get(); however, it is just
same as forgetting mutex_unlock after mutex_lock. So I wouldn't mind
that much.
The next devfreq_get() will do
> mutex_lock(&devfreq_list_lock);
> ret = find_device_devfreq(dev);
> mutex_lock(&devfreq->lock);
> mutex_unlock(&devfreq_list_lock);
and devfreq_put() will do
> mutex_unlock(&devfreq->lock);
as you've suggested.
>> +static int devfreq_do(struct devfreq *devfreq)
>> +{
>> + struct opp *opp;
>> + unsigned long freq;
>> + int err;
>> +
>
> Put the mutex_is_locked check here? See more below.
>
[]
>> +static int devfreq_update(struct notifier_block *nb, unsigned long type,
>> + void *devp)
>> +{
>> + struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
>> + int ret;
>> +
>> + mutex_lock(&devfreq->lock);
>> + ret = update_devfreq(devfreq);
>> + mutex_unlock(&devfreq->lock);
>
> The whole devfreq_update/update_devfreq pairing is redundant.
> update_devfreq's purpose is to make sure the lock is held before going
> further, and the only caller of update_devfreq is devfreq_update which
> always holds the lock.
>
> This still doesn't stop a bad driver writer from just calling
> devfreq_do with an extern. Perhaps the lock detection should be moved
> into devfreq_do and update_devfreq should go away?
>
- update_devfreq: extern for governors
- devfreq_update: notifier callback for OPP
- devfreq_do: internal function of devfreq.
Anyway, as you've mentioned, it seems I'd better rename devfreq_do as
update_devfreq and make it exported with mutex check.
[]
>> +static void devfreq_monitor(struct work_struct *work)
>> +{
>> + static unsigned long last_polled_at;
>> + struct devfreq *devfreq, *tmp;
>> + int error;
>> + unsigned long jiffies_passed;
>> + unsigned long next_jiffies = ULONG_MAX, now = jiffies;
>> +
>> + /* Initially last_polled_at = 0, polling every device at bootup */
>> + jiffies_passed = now - last_polled_at;
>> + last_polled_at = now;
>> + if (jiffies_passed == 0)
>> + jiffies_passed = 1;
>> +
>> + mutex_lock(&devfreq_list_lock);
>
> Should not lock the list here. If we lock the list for all major
> operations, it nullifies the performance benefit of having a mutex in
> struct devfreq.
>
Ok... then.. how about locking like this? :
mutex_lock(&devfreq_list_lock);
list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
mutex_lock(&devfreq->lock);
mutex_unlock(&devfreq_list_lock);
blahblah
mutex_unlock(&devfreq->lock);
mutex_lock(&devfreq_list_lock);
}
mutex_unlock(&devfreq_list_lock);
Anyway, there is one more problem with allowing
unlocked-devfreq_list_lock in the loop.
list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) is safe
for the removal of devfreq from the list in the loop.
However, it is not safe against the removal of devfreq's next member
in the loop and while devfreq_list_lock is unlocked,
devfreq_remove_device may remove that one; thus, breaking the
list_for_each_entry_safe loop.
Such break is prevent by adding one more mutex_lock/mutex_unlock to
the loop for tmp->lock. However, if we do mutex_unlock(&tmp->lock)
before mutex_lock(&devfreq_list_lock), we still have the same
breaking-the-loop issue and if we do it after
mutex_lock(&devfreq_list_lock), we have a deadlock issue (someone
might have locked devfreq_list_lock and waiting to lock tmp->lock).
Thus, we will need to block devfreq_remove_device at devfreq_monitor
whlie unlocking devfreq_list in the loop. Other operations (add /
list) on the list are fine for it.
So, the loop will be:
mutex_lock(&devfreq_list_lock);
prohibit_devfreq_remove = true;
list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
mutex_lock(&devfreq->lock);
mutex_unlock(&devfreq_list_lock);
blahblah
mutex_unlock(&devfreq->lock);
mutex_lock(&devfreq_list_lock);
}
prohibit_devfreq_remove = false;
mutex_unlock(&devfreq_list_lock);
[]
>> + /* Remove a devfreq with an error. */
>> + if (error && error != -EAGAIN) {
>> + dev_err(devfreq->dev, "Due to devfreq_do error(%d), devfreq(%s) is removed from the device\n",
>> + error, devfreq->governor->name);
>> +
>> + list_del(&devfreq->node);
>> + mutex_unlock(&devfreq->lock);
>> + kfree(devfreq);
>> + continue;
>
> Should this error handling also unregister the OPP notifier? This
> code duplicates portions of devfreq_remove_device. I propose instead
> here we do the following:
>
> mutex_unlock(&devfreq->lock);
> _devfreq_remove_lock(devfreq);
> /* this locks the list first,
> * then locks devfreq->lock,
> * then does the house cleaning
> */
> continue;
Ah.. that was missing. Thanks!
I'll make a _devfreq_remove_device(struct device *dev, bool
call_by_monitor) and let devfreq_remove_device use it.
>
>> + }
>> + devfreq->next_polling = devfreq->polling_jiffies;
>> +
>> + /* No more polling required (polling_ms changed) */
>> + if (devfreq->next_polling == 0) {
>> + mutex_unlock(&devfreq->lock);
>> + continue;
>> + }
>> + } else {
>> + devfreq->next_polling -= jiffies_passed;
>> + }
>> +
>> + next_jiffies = (next_jiffies > devfreq->next_polling) ?
>> + devfreq->next_polling : next_jiffies;
>> +
>> + mutex_unlock(&devfreq->lock);
>> + }
>> +
>> + if (next_jiffies > 0 && next_jiffies < ULONG_MAX) {
>> + polling = true;
>> + queue_delayed_work(devfreq_wq, &devfreq_work, next_jiffies);
>> + } else {
>> + polling = false;
>> + }
>> +
>> + mutex_unlock(&devfreq_list_lock);
>
> Again, list should not be locked over this whole function. It blocks
> other unrelated devfreq devices from scaling.
>
>> +}
>
> [snip]
>
>> +/**
>> + * devfreq_remove_device() - Remove devfreq feature from a device.
>> + * @device: the device to remove devfreq feature.
>> + */
>> +int devfreq_remove_device(struct device *dev)
>
> Why does this take a struct device*? Shouldn't it be a struct devfreq*?
It is because devfreq_add_device() does not return struct devfreq.
struct devfreq is not visible to device drivers. It is visible to
governors.
>
> If there is a case for removing a devfreq device with only struct
> device* as input, how about:
>
> int devfreq_remove_device(struct device *dev)
> {
> struct devfreq *devfreq;
> mutex_lock(&devfreq_list_lock);
> devfreq = find_device_devfreq(dev);
> mutex_unlock(&devfreq_list_lock);
>
> return _devfreq_remove_device(struct devfreq *df);
> /* _devfreq_remove_device does the real work and can also be
> called from devfreq_monitor */
> }
Sure, I'll do so and reduce the redundancy.
>
> Regards,
> Mike
>
[]
Thank you so much!
Cheers,
MyungJoo
--
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@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs
2011-09-01 4:51 ` MyungJoo Ham
@ 2011-09-01 16:57 ` Turquette, Mike
2011-09-02 4:38 ` MyungJoo Ham
0 siblings, 1 reply; 11+ messages in thread
From: Turquette, Mike @ 2011-09-01 16:57 UTC (permalink / raw)
To: myungjoo.ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm, Thomas Gleixner
On Wed, Aug 31, 2011 at 9:51 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> On Thu, Sep 1, 2011 at 5:05 AM, Turquette, Mike <mturquette@ti.com> wrote:
>> On Wed, Aug 31, 2011 at 12:29 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
[snip]
>>> +static void devfreq_monitor(struct work_struct *work)
>>> +{
>>> + static unsigned long last_polled_at;
>>> + struct devfreq *devfreq, *tmp;
>>> + int error;
>>> + unsigned long jiffies_passed;
>>> + unsigned long next_jiffies = ULONG_MAX, now = jiffies;
>>> +
>>> + /* Initially last_polled_at = 0, polling every device at bootup */
>>> + jiffies_passed = now - last_polled_at;
>>> + last_polled_at = now;
>>> + if (jiffies_passed == 0)
>>> + jiffies_passed = 1;
>>> +
>>> + mutex_lock(&devfreq_list_lock);
>>
>> Should not lock the list here. If we lock the list for all major
>> operations, it nullifies the performance benefit of having a mutex in
>> struct devfreq.
>>
>
> Ok... then.. how about locking like this? :
>
> mutex_lock(&devfreq_list_lock);
> list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
> mutex_lock(&devfreq->lock);
> mutex_unlock(&devfreq_list_lock);
>
> blahblah
>
> mutex_unlock(&devfreq->lock);
> mutex_lock(&devfreq_list_lock);
> }
> mutex_unlock(&devfreq_list_lock);
I took a step back from the code to rethink the big picture, and I've
come back to the same conclusion conclusion that I had in the V5
patchset. (https://patchwork.kernel.org/patch/1043442/)
Firstly, there is no reason to walk the list here. All of the locking
discussion here could just go away if we didn't walk the list of
devfreq devices every time the delay_work gets fired. If each device
programmed it's own delayed work then this problem simply goes away.
If you don't mind I'd like to post an RFC of devfreq with these
changes implemented so we can review them and discuss the ideas around
some concrete code.
Best regards,
Mike
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs
2011-09-01 16:57 ` Turquette, Mike
@ 2011-09-02 4:38 ` MyungJoo Ham
2011-09-02 6:55 ` Turquette, Mike
0 siblings, 1 reply; 11+ messages in thread
From: MyungJoo Ham @ 2011-09-02 4:38 UTC (permalink / raw)
To: Turquette, Mike
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm, Thomas Gleixner
On Fri, Sep 2, 2011 at 1:57 AM, Turquette, Mike <mturquette@ti.com> wrote:
> On Wed, Aug 31, 2011 at 9:51 PM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>> On Thu, Sep 1, 2011 at 5:05 AM, Turquette, Mike <mturquette@ti.com> wrote:
>>> On Wed, Aug 31, 2011 at 12:29 AM, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
> [snip]
>>>> +static void devfreq_monitor(struct work_struct *work)
>>>> +{
>>>> + static unsigned long last_polled_at;
>>>> + struct devfreq *devfreq, *tmp;
>>>> + int error;
>>>> + unsigned long jiffies_passed;
>>>> + unsigned long next_jiffies = ULONG_MAX, now = jiffies;
>>>> +
>>>> + /* Initially last_polled_at = 0, polling every device at bootup */
>>>> + jiffies_passed = now - last_polled_at;
>>>> + last_polled_at = now;
>>>> + if (jiffies_passed == 0)
>>>> + jiffies_passed = 1;
>>>> +
>>>> + mutex_lock(&devfreq_list_lock);
>>>
>>> Should not lock the list here. If we lock the list for all major
>>> operations, it nullifies the performance benefit of having a mutex in
>>> struct devfreq.
>>>
>>
>> Ok... then.. how about locking like this? :
>>
>> mutex_lock(&devfreq_list_lock);
>> list_for_each_entry_safe(devfreq, tmp, &devfreq_list, node) {
>> mutex_lock(&devfreq->lock);
>> mutex_unlock(&devfreq_list_lock);
>>
>> blahblah
>>
>> mutex_unlock(&devfreq->lock);
>> mutex_lock(&devfreq_list_lock);
>> }
>> mutex_unlock(&devfreq_list_lock);
>
> I took a step back from the code to rethink the big picture, and I've
> come back to the same conclusion conclusion that I had in the V5
> patchset. (https://patchwork.kernel.org/patch/1043442/)
>
> Firstly, there is no reason to walk the list here. All of the locking
> discussion here could just go away if we didn't walk the list of
> devfreq devices every time the delay_work gets fired. If each device
> programmed it's own delayed work then this problem simply goes away.
>
> If you don't mind I'd like to post an RFC of devfreq with these
> changes implemented so we can review them and discuss the ideas around
> some concrete code.
>
> Best regards,
> Mike
>
I'm considering to allow a governor to declare that it will run its own loop
.
Because we have .init/.exit callbacks and an update in OPP triggers to
call devfreq->profile->target anyway, we only need to add one bit in
struct devfreq_governor, "bool own_loop". Then, we just need to modify
devfreq_add_device to omit from the list or mark them not to be looped
if the "own_loop" bit is true for the given governor.
That way, we can keep most governors simple as cpufreq's drivers and
as not complex as cpufreq's governors. For those governors that really
need to run their own loop, we can give the option. I want most
devfreq governors to be general enough for devfreq devices so that the
device drivers may use any of them without altering the parameters
much and easy and short enough for subsystems to have their own
governors. Thus, common things such as looping and getting usage
statistics are moved from governors to devfreq framework.
You are welcome to post an RFC patch to allow governors to have their
own loop implemented in governors; however, I think that should be
optional, not mandatory for governors. And, that degree of
synchronization issue in devfreq ain't that badly complex, isn't it?
Thank you.
Cheers! It's Friday.
MyungJoo
ps. Ah.. and for the kobject thing that you've mentioned in the other
thread, I'm experimenting it (a kobject class "devfreq"). However, it
will relocate devfreq sysfs entries from /sys/devices/.../power/* to
/sys/devices/.../devfreq/*.
To Rafael:
Would it be fine for the devfreq sys entries to move to such a
location by creating "devfreq" class? I remember you've objected to an
independent per-device sysfs directory for devfreq entries. However,
it is sort of "sideeffect" in reducing overhead of searching struct
devfreq again and again for sysfs callbacks.
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs
2011-09-02 4:38 ` MyungJoo Ham
@ 2011-09-02 6:55 ` Turquette, Mike
0 siblings, 0 replies; 11+ messages in thread
From: Turquette, Mike @ 2011-09-02 6:55 UTC (permalink / raw)
To: MyungJoo Ham
Cc: Len Brown, Greg Kroah-Hartman, Kyungmin Park, linux-pm, Thomas Gleixner
On Thu, Sep 1, 2011 at 9:38 PM, MyungJoo Ham <myungjoo.ham@gmail.com> wrote:
> I'm considering to allow a governor to declare that it will run its own loop
> .
> Because we have .init/.exit callbacks and an update in OPP triggers to
> call devfreq->profile->target anyway, we only need to add one bit in
> struct devfreq_governor, "bool own_loop". Then, we just need to modify
> devfreq_add_device to omit from the list or mark them not to be looped
> if the "own_loop" bit is true for the given governor.
>
> That way, we can keep most governors simple as cpufreq's drivers and
> as not complex as cpufreq's governors. For those governors that really
> need to run their own loop, we can give the option. I want most
> devfreq governors to be general enough for devfreq devices so that the
> device drivers may use any of them without altering the parameters
> much and easy and short enough for subsystems to have their own
> governors. Thus, common things such as looping and getting usage
> statistics are moved from governors to devfreq framework.
I can understand wanting to keep the governors simpler than their
CPUfreq counterparts. However I also disagree with polling boolean
and the use of devfreq for non-polling purposes (with the exception of
powersave & performance, which are mostly for testing, and for
userspace which is a special case).
Also my version of the patch removes the dependency on the opp library
and abstracts those details away in a frequency table, which might use
OPPs as a backing store, or might accept arbitrary rates given min/max
limitations (such as for a device connected to a clock with a very
wide divider). There are probably devices out there than can simply
change rates with clk_set_rate that would benefit from devfreq, but do
not have OPPs.
> You are welcome to post an RFC patch to allow governors to have their
> own loop implemented in governors; however, I think that should be
> optional, not mandatory for governors. And, that degree of
> synchronization issue in devfreq ain't that badly complex, isn't it?
Thanks for being open to the idea. I don't think that the list walk
is wildly complex but some of the differences run deeper than that.
I'm at LPC next week so I'm not sure when my RFC will hit the list.
Hopefully the week after LPC. Until then I'll continue to review your
devfreq patches as usual.
>
> Thank you.
>
>
> Cheers! It's Friday.
> MyungJoo
>
> ps. Ah.. and for the kobject thing that you've mentioned in the other
> thread, I'm experimenting it (a kobject class "devfreq"). However, it
> will relocate devfreq sysfs entries from /sys/devices/.../power/* to
> /sys/devices/.../devfreq/*.
Just my $0.02, but my patch does the same thing and I think it's a
good thing. The code lives drivers/devfreq/ so placing it under
/sys/device/.../power/ wasn't a perfect fit anyways.
Regards,
Mike
> To Rafael:
> Would it be fine for the devfreq sys entries to move to such a
> location by creating "devfreq" class? I remember you've objected to an
> independent per-device sysfs directory for devfreq entries. However,
> it is sort of "sideeffect" in reducing overhead of searching struct
> devfreq again and again for sysfs callbacks.
>
> --
> MyungJoo Ham, Ph.D.
> Mobile Software Platform Lab, DMC Business, Samsung Electronics
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-09-02 6:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-31 7:29 [PATCH v9 0/4] Devfreq, DVFS Framework for Non-CPU Devices MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 1/4] PM / OPP: Add OPP availability change notifier MyungJoo Ham
2011-08-31 7:29 ` [PATCH v9 2/4] PM: Introduce devfreq: generic DVFS framework with device-specific OPPs MyungJoo Ham
2011-08-31 20:05 ` Turquette, Mike
2011-09-01 4:51 ` MyungJoo Ham
2011-09-01 16:57 ` Turquette, Mike
2011-09-02 4:38 ` MyungJoo Ham
2011-09-02 6:55 ` Turquette, Mike
2011-08-31 7:29 ` [PATCH v9 3/4] PM / devfreq: add common sysfs interfaces MyungJoo Ham
2011-08-31 21:27 ` Turquette, Mike
2011-08-31 7:29 ` [PATCH v9 4/4] PM / devfreq: add basic governors MyungJoo Ham
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.