[RFC v2 0/7] PM / devfreq: draft for OPP suspend impl (even draftier)

[RFC v2 0/7] PM / devfreq: draft for OPP suspend impl (even draftier)

[Tobias Jakobi]

Hello everyone,

this is v2 of the RFC. You can find the first version here:
https://www.spinics.net/lists/linux-samsung-soc/msg56331.html

I'm still unhappy with this approach, for multiple reasons:
- the global boolean -- cpufreq also has it, but I have the feeling that we don't need it here.
- keeping track of which device was suspended looks convoluted -- some reference counting would definitely help here.
- the bus driver (here exynos_bus) sets suspend_freq, but devfreq core actually uses it.
- returning -EBUSY from devfreq_{suspend,resume}_device() requires handling this in the device drivers -- again refcounting could solve this.
- rather a question: do we need the restore on resume -- or should be just wait for the next update through the governor?
- like Chanwoo already suggested, move setting suspend freq to devfreq_suspend_device().

Concerning refcounting, I'm a bit unsure what we wanna count here? Usually we count usage. If usage goes from zero to one, we do something (alloc), and when it goes back from one to zero (free).
If we keep the current semantics however we have to count number of suspends. That's kinda unintuitive if you ask me.

With best wishes,
Tobias

Tobias Jakobi (7):
  PM / devfreq: Replace 'stop_polling' boolean with flags
  PM / devfreq: Track suspend state of DevFreq devices
  PM / devfreq: Add DevFreq subsystem suspend/resume
  PM / devfreq: Suspend subsystem on system suspend/hibernate
  PM / devfreq: exynos-bus: add support for suspend OPP
  ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus
  ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus for Prime

 arch/arm/boot/dts/exynos4412-prime.dtsi |   2 +
 arch/arm/boot/dts/exynos4x12.dtsi       |   2 +
 drivers/base/power/main.c               |   3 +
 drivers/devfreq/devfreq.c               | 170 ++++++++++++++++++++++++++++++--
 drivers/devfreq/exynos-bus.c            |   8 ++
 include/linux/devfreq.h                 |  14 ++-
 6 files changed, 188 insertions(+), 11 deletions(-)

-- 
2.7.3

[RFC v2 1/7] PM / devfreq: Replace 'stop_polling' boolean with flags

[Tobias Jakobi]

This is preparation for introducing some additional flags
to struct devfreq.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/devfreq/devfreq.c | 18 +++++++++++-------
 include/linux/devfreq.h   |  4 ++--
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 614738d..82d8052 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -28,6 +28,10 @@
 #include <linux/of.h>
 #include "governor.h"
 
+enum devfreq_flag_bits {
+	DEVFREQ_BIT_STOP_POLLING,
+};
+
 static struct class *devfreq_class;
 
 /*
@@ -366,13 +370,13 @@ EXPORT_SYMBOL(devfreq_monitor_stop);
 void devfreq_monitor_suspend(struct devfreq *devfreq)
 {
 	mutex_lock(&devfreq->lock);
-	if (devfreq->stop_polling) {
+	if (test_bit(DEVFREQ_BIT_STOP_POLLING, &devfreq->flags)) {
 		mutex_unlock(&devfreq->lock);
 		return;
 	}
 
 	devfreq_update_status(devfreq, devfreq->previous_freq);
-	devfreq->stop_polling = true;
+	__set_bit(DEVFREQ_BIT_STOP_POLLING, &devfreq->flags);
 	mutex_unlock(&devfreq->lock);
 	cancel_delayed_work_sync(&devfreq->work);
 }
@@ -391,7 +395,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 	unsigned long freq;
 
 	mutex_lock(&devfreq->lock);
-	if (!devfreq->stop_polling)
+	if (!test_bit(DEVFREQ_BIT_STOP_POLLING, &devfreq->flags))
 		goto out;
 
 	if (!delayed_work_pending(&devfreq->work) &&
@@ -400,7 +404,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 
 	devfreq->last_stat_updated = jiffies;
-	devfreq->stop_polling = false;
+	__clear_bit(DEVFREQ_BIT_STOP_POLLING, &devfreq->flags);
 
 	if (devfreq->profile->get_cur_freq &&
 		!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
@@ -427,7 +431,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 	mutex_lock(&devfreq->lock);
 	devfreq->profile->polling_ms = new_delay;
 
-	if (devfreq->stop_polling)
+	if (test_bit(DEVFREQ_BIT_STOP_POLLING, &devfreq->flags))
 		goto out;
 
 	/* if new delay is zero, stop polling */
@@ -449,7 +453,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
 		mutex_unlock(&devfreq->lock);
 		cancel_delayed_work_sync(&devfreq->work);
 		mutex_lock(&devfreq->lock);
-		if (!devfreq->stop_polling)
+		if (!test_bit(DEVFREQ_BIT_STOP_POLLING, &devfreq->flags))
 			queue_delayed_work(devfreq_wq, &devfreq->work,
 			      msecs_to_jiffies(devfreq->profile->polling_ms));
 	}
@@ -1203,7 +1207,7 @@ static ssize_t trans_stat_show(struct device *dev,
 	int i, j;
 	unsigned int max_state = devfreq->profile->max_state;
 
-	if (!devfreq->stop_polling &&
+	if (!test_bit(DEVFREQ_BIT_STOP_POLLING, &devfreq->flags) &&
 			devfreq_update_status(devfreq, devfreq->previous_freq))
 		return 0;
 	if (max_state == 0)
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 3bd15ae..d6bf9a3 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -145,7 +145,7 @@ struct devfreq_governor {
  *		touch this.
  * @min_freq:	Limit minimum frequency requested by user (0: none)
  * @max_freq:	Limit maximum frequency requested by user (0: none)
- * @stop_polling:	 devfreq polling status of a device.
+ * @flags:	Internal DevFreq status of a device.
  * @total_trans:	Number of devfreq transitions
  * @trans_table:	Statistics of devfreq transitions
  * @time_in_state:	Statistics of devfreq states
@@ -180,7 +180,7 @@ struct devfreq {
 
 	unsigned long min_freq;
 	unsigned long max_freq;
-	bool stop_polling;
+	unsigned long flags;
 
 	/* information for device frequency transition */
 	unsigned int total_trans;
-- 
2.7.3

[RFC v2 2/7] PM / devfreq: Track suspend state of DevFreq devices

[Tobias Jakobi]

In preparation to suspend the whole DevFreq subsystem.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/devfreq/devfreq.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 82d8052..3904ebc 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -30,6 +30,8 @@
 
 enum devfreq_flag_bits {
 	DEVFREQ_BIT_STOP_POLLING,
+	/* Bit set if DevFreq device was suspended in devfreq_suspend_device(). */
+	DEVFREQ_BIT_SUSPENDED,
 };
 
 static struct class *devfreq_class;
@@ -822,14 +824,21 @@ EXPORT_SYMBOL(devfreq_turbo_put);
  */
 int devfreq_suspend_device(struct devfreq *devfreq)
 {
+	int ret;
+
 	if (!devfreq)
 		return -EINVAL;
 
 	if (!devfreq->governor)
 		return 0;
 
-	return devfreq->governor->event_handler(devfreq,
+	ret = devfreq->governor->event_handler(devfreq,
 				DEVFREQ_GOV_SUSPEND, NULL);
+
+	if (ret == 0)
+		__set_bit(DEVFREQ_BIT_SUSPENDED, &devfreq->flags);
+
+	return ret;
 }
 EXPORT_SYMBOL(devfreq_suspend_device);
 
@@ -843,14 +852,21 @@ EXPORT_SYMBOL(devfreq_suspend_device);
  */
 int devfreq_resume_device(struct devfreq *devfreq)
 {
+	int ret;
+
 	if (!devfreq)
 		return -EINVAL;
 
 	if (!devfreq->governor)
 		return 0;
 
-	return devfreq->governor->event_handler(devfreq,
+	ret = devfreq->governor->event_handler(devfreq,
 				DEVFREQ_GOV_RESUME, NULL);
+
+	if (ret == 0)
+		__clear_bit(DEVFREQ_BIT_SUSPENDED, &devfreq->flags);
+
+	return ret;
 }
 EXPORT_SYMBOL(devfreq_resume_device);
 
-- 
2.7.3

[RFC v2 3/7] PM / devfreq: Add DevFreq subsystem suspend/resume

[Tobias Jakobi]

This suspend/resume operations works analogous to the
cpufreq_{suspend,resume}() calls in the CPUFreq subsystem.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/devfreq/devfreq.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq.h   |  10 ++++
 2 files changed, 142 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 3904ebc..7e71c19 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -32,6 +32,8 @@ enum devfreq_flag_bits {
 	DEVFREQ_BIT_STOP_POLLING,
 	/* Bit set if DevFreq device was suspended in devfreq_suspend_device(). */
 	DEVFREQ_BIT_SUSPENDED,
+	/* Bit set if DevFreq device should be resumed in devfreq_resume(). */
+	DEVFREQ_BIT_RESUME,
 };
 
 static struct class *devfreq_class;
@@ -49,6 +51,8 @@ static LIST_HEAD(devfreq_governor_list);
 static LIST_HEAD(devfreq_list);
 static DEFINE_MUTEX(devfreq_list_lock);
 
+static bool devfreq_suspended = false;
+
 /**
  * find_device_devfreq() - find devfreq struct using device pointer
  * @dev:	device pointer used to lookup device devfreq.
@@ -832,6 +836,9 @@ int devfreq_suspend_device(struct devfreq *devfreq)
 	if (!devfreq->governor)
 		return 0;
 
+	if (devfreq_suspended)
+		return -EBUSY;
+
 	ret = devfreq->governor->event_handler(devfreq,
 				DEVFREQ_GOV_SUSPEND, NULL);
 
@@ -860,6 +867,9 @@ int devfreq_resume_device(struct devfreq *devfreq)
 	if (!devfreq->governor)
 		return 0;
 
+	if (devfreq_suspended)
+		return -EBUSY;
+
 	ret = devfreq->governor->event_handler(devfreq,
 				DEVFREQ_GOV_RESUME, NULL);
 
@@ -871,6 +881,128 @@ int devfreq_resume_device(struct devfreq *devfreq)
 EXPORT_SYMBOL(devfreq_resume_device);
 
 /**
+ * devfreq_suspend() - Suspend DevFreq governors
+ *
+ * Called during system wide Suspend/Hibernate cycles for suspending governors
+ * in the same fashion as cpufreq_suspend().
+ */
+void devfreq_suspend(void)
+{
+	struct devfreq *devfreq;
+	struct devfreq_freqs freqs;
+	unsigned long freq;
+	int ret;
+
+	devfreq_suspended = true;
+
+	mutex_lock(&devfreq_list_lock);
+
+	/*
+	 * Suspend all the devices that were not previously suspended through
+	 * devfreq_suspend_device(). In devfreq_resume() we then resume exactly
+	 * these devices.
+	 */
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		if (test_bit(DEVFREQ_BIT_SUSPENDED, &devfreq->flags))
+			continue;
+
+		ret = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL);
+		if (ret < 0) {
+			dev_err(&devfreq->dev, "%s: governor suspend failed\n", __func__);
+			continue;
+		}
+
+		__set_bit(DEVFREQ_BIT_RESUME, &devfreq->flags);
+	}
+
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		if (!devfreq->suspend_freq)
+			continue;
+
+		if (devfreq->profile->get_cur_freq)
+			devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
+		else
+			freq = devfreq->previous_freq;
+
+		devfreq->resume_freq = freq;
+
+		freqs.old = devfreq->resume_freq;
+		freqs.new = devfreq->suspend_freq;
+		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
+
+		freq = devfreq->suspend_freq;
+		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
+
+		if (ret < 0) {
+			dev_err(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
+			freqs.new = devfreq->resume_freq;
+			devfreq->resume_freq = 0;
+		} else
+			freqs.new = freq;
+
+		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
+	}
+
+	mutex_unlock(&devfreq_list_lock);
+}
+
+/**
+ * devfreq_resume() - Resume DevFreq governors
+ *
+ * Called during system wide Suspend/Hibernate cycle for resuming governors that
+ * are suspended with devfreq_suspend().
+ */
+void devfreq_resume(void)
+{
+	struct devfreq *devfreq;
+	struct devfreq_freqs freqs;
+	unsigned long freq;
+	int ret;
+
+	mutex_lock(&devfreq_list_lock);
+
+	/*
+	 * If a suspend OPP was set during devfreq_suspend(), then try to
+	 * restore the DevFreq here to the original OPP.
+	 */
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		if (!devfreq->resume_freq)
+			continue;
+
+		freqs.old = devfreq->suspend_freq;
+		freqs.new = devfreq->resume_freq;
+		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
+
+		freq = devfreq->resume_freq;
+		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
+
+		if (ret < 0) {
+			dev_err(&devfreq->dev, "%s: setting resume frequency failed\n", __func__);
+			freqs.new = devfreq->suspend_freq;
+		} else
+			freqs.new =  freq;
+
+		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
+		devfreq->resume_freq = 0;
+	}
+
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		if (!test_bit(DEVFREQ_BIT_RESUME, &devfreq->flags))
+			continue;
+
+		ret = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_RESUME, NULL);
+		if (ret < 0)
+			dev_err(&devfreq->dev, "%s: governor resume failed\n", __func__);
+
+		__clear_bit(DEVFREQ_BIT_RESUME, &devfreq->flags);
+	}
+
+	mutex_unlock(&devfreq_list_lock);
+
+	devfreq_suspended = false;
+}
+
+/**
  * devfreq_add_governor() - Add devfreq governor
  * @governor:	the devfreq governor to be added
  */
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index d6bf9a3..a98fa0f 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -173,6 +173,9 @@ struct devfreq {
 
 	unsigned int turbo_refcount;
 
+	unsigned long suspend_freq; /* freq during devfreq suspend */
+	unsigned long resume_freq; /* freq restored after suspend cycle */
+
 	unsigned long previous_freq;
 	struct devfreq_dev_status last_status;
 
@@ -216,6 +219,10 @@ extern int devfreq_turbo_put(struct devfreq *devfreq);
 extern int devfreq_suspend_device(struct devfreq *devfreq);
 extern int devfreq_resume_device(struct devfreq *devfreq);
 
+/* Suspend/resume the entire Devfreq subsystem. */
+void devfreq_suspend(void);
+void devfreq_resume(void);
+
 /* Helper functions for devfreq user device driver with OPP. */
 extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
 					   unsigned long *freq, u32 flags);
@@ -425,6 +432,9 @@ static inline int devfreq_update_stats(struct devfreq *df)
 {
 	return -EINVAL;
 }
+
+static inline void devfreq_suspend(void) {}
+static inline void devfreq_resume(void) {}
 #endif /* CONFIG_PM_DEVFREQ */
 
 #endif /* __LINUX_DEVFREQ_H__ */
-- 
2.7.3

[RFC v2 4/7] PM / devfreq: Suspend subsystem on system suspend/hibernate

[Tobias Jakobi]

On the Exynos4412 SoC the DevFreq subsystem adjusts the frequency
of the various internal busses and the corresponding voltages.

E.g. the clock of the DMC (dynamic memory controller) bus
together with the voltage of the MIF regulator are controlled
by this.

If DMC activity is low and DevFreq has set a lower OPP, the
following can happen.

The transition to the lower OPP has set a voltage V_low. If the
system now is restarted or goes into a suspend/resume-cycle,
the first-stage (BL0) bootloader takes over, which also
initializes clocks to default values, say freq_0 for the DMC.
Since the PMIC is an external component and not part of the SoC,
the BL0 doesn't set any default voltages. The BL0 now hangs
because the required voltage V_0 (corresponding to freq_0) is
higher than the currently set one V_low.

To fix this, we make sure to only go into suspend with a 'safe'
DevFreq OPP selected.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/base/power/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 5a94dfa..9cd7e06 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -32,6 +32,7 @@
 #include <trace/events/power.h>
 #include <linux/cpufreq.h>
 #include <linux/cpuidle.h>
+#include <linux/devfreq.h>
 #include <linux/timer.h>
 
 #include "../base.h"
@@ -943,6 +944,7 @@ void dpm_resume(pm_message_t state)
 	dpm_show_time(starttime, state, NULL);
 
 	cpufreq_resume();
+	devfreq_resume();
 	trace_suspend_resume(TPS("dpm_resume"), state.event, false);
 }
 
@@ -1582,6 +1584,7 @@ int dpm_suspend(pm_message_t state)
 	trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
 	might_sleep();
 
+	devfreq_suspend();
 	cpufreq_suspend();
 
 	mutex_lock(&dpm_list_mtx);
-- 
2.7.3

[RFC v2 5/7] PM / devfreq: exynos-bus: add support for suspend OPP

[Tobias Jakobi]

Set the frequency of the suspend OPP, so that DevFreq
core can use it during system sleep/reboot/power-off.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/devfreq/exynos-bus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 29866f7..16fb37a 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -389,6 +389,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	struct devfreq_passive_data *passive_data;
 	struct devfreq *parent_devfreq;
 	struct exynos_bus *bus;
+	struct dev_pm_opp *suspend_opp;
 	int ret, max_state;
 	unsigned long min_freq, max_freq;
 
@@ -449,6 +450,13 @@ static int exynos_bus_probe(struct platform_device *pdev)
 		goto err;
 	}
 
+	/* Check for suspend OPP and store the corresponding frequency. */
+	rcu_read_lock();
+	suspend_opp = dev_pm_opp_get_suspend_opp(dev);
+	if (suspend_opp)
+		bus->devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
+	rcu_read_unlock();
+
 	/* Register opp_notifier to catch the change of OPP  */
 	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
 	if (ret < 0) {
-- 
2.7.3

[RFC v2 6/7] ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus

[Tobias Jakobi]

Both DMC and leftbus adjust regulator voltages (MIF and INT
respectively), which have to be in a safe range to allow the
BL0 to operate correctly.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 arch/arm/boot/dts/exynos4x12.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 46c2ada..35cbe40 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -378,6 +378,7 @@
 		opp@400000000 {
 			opp-hz = /bits/ 64 <400000000>;
 			opp-microvolt = <1050000>;
+			opp-suspend;
 		};
 	};
 
@@ -466,6 +467,7 @@
 		opp@200000000 {
 			opp-hz = /bits/ 64 <200000000>;
 			opp-microvolt = <1000000>;
+			opp-suspend;
 		};
 	};
 
-- 
2.7.3

[RFC v2 7/7] ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus for Prime

[Tobias Jakobi]

Both DMC and leftbus adjust regulator voltages (MIF and INT
respectively), which have to be in a safe range to allow the
BL0 to operate correctly.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 arch/arm/boot/dts/exynos4412-prime.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412-prime.dtsi b/arch/arm/boot/dts/exynos4412-prime.dtsi
index 97d1b9c..18e411f 100644
--- a/arch/arm/boot/dts/exynos4412-prime.dtsi
+++ b/arch/arm/boot/dts/exynos4412-prime.dtsi
@@ -123,6 +123,7 @@
 		opp@400000000 {
 			opp-hz = /bits/ 64 <400000000>;
 			opp-microvolt = <1100000>;
+			opp-suspend;
 		};
 	};
 
@@ -145,6 +146,7 @@
 		opp@200000000 {
 			opp-hz = /bits/ 64 <200000000>;
 			opp-microvolt = <1150000>;
+			opp-suspend;
 		};
 	};
 
-- 
2.7.3

Re: [RFC v2 0/7] PM / devfreq: draft for OPP suspend impl (even draftier)

[Chanwoo Choi]

Hi Tobias,

On 2016년 12월 19일 11:16, Tobias Jakobi wrote:
> Hello everyone,
> 
> this is v2 of the RFC. You can find the first version here:
> https://www.spinics.net/lists/linux-samsung-soc/msg56331.html
> 
> I'm still unhappy with this approach, for multiple reasons:
> - the global boolean -- cpufreq also has it, but I have the feeling that we don't need it here.

> - keeping track of which device was suspended looks convoluted -- some reference counting would definitely help here.
> - the bus driver (here exynos_bus) sets suspend_freq, but devfreq core actually uses it.
> - returning -EBUSY from devfreq_{suspend,resume}_device() requires handling this in the device drivers -- again refcounting could solve this.
> - rather a question: do we need the restore on resume -- or should be just wait for the next update through the governor?
> - like Chanwoo already suggested, move setting suspend freq to devfreq_suspend_device().

I want to separate the two subject as following:
- subject1 : Where should devfreq set the suspend-freq for device.
- subject2 : How can devfreq support the duplicate call of devfreq_{suspend,resume}_device().

For subject1:
I explain why devfreq need to set the supend-freq in devfreq_suspend_device().
Following explanation do not include the refcount of suspend and bit operation.

The devfreq has two case to enter the suspend mode for devfreq instance as following:
case 1. Some device driver call the 'devfreq_suspend/resume_device()' directly regardless the sequence of suspend mode (echo mem > /sys/power/state).
case 2. The system enter the suspend mode by using 'echo mem > /sys/power/state' command.

When setting suspend frequency in the devfreq_suspend/resume_device(),
this suggestion can cover all cases for suspend freq setting.

I make the separate function[1] to set the frequency.
[1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-test&id=8851e989ccaf0ee23a4278724227e879f8752625

	devfreq_suspend_device(struct devfreq *devfreq) {
		/* Enter the suspend mode */
		devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL);

		/* Set the suspend-freq */
		devfreq_set_target(devfreq, devfreq->suspend_freq, 0);
	}

	devfreq_resume_device(struct devfreq *devfreq) {
		/* Set the resume freq */
		devfreq_set_target(devfreq, devfreq->resume_freq, 0);

		/* Wakeup from suspend mode */
		devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_RESUME, NULL);
	}

And,
devfreq_suspend() call the devfreq_suspend_device() for all registered devfreq instance.

	void devfreq_suspend(void)
	{
		mutex_lock(&devfreq_list_lock);

		list_for_each_entry(devfreq, &devfreq_list, node) {
			ret = devfreq_suspend_device(devfreq);
			if (ret < 0)
				dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
		}

		mutex_unlock(&devfreq_list_lock);
	}

	void devfreq_resume(void)
	{
		mutex_lock(&devfreq_list_lock);

		list_for_each_entry(devfreq, &devfreq_list, node) {
			ret = devfreq_resume_device(devfreq);
			if (ret < 0)
				dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
		}

		mutex_unlock(&devfreq_list_lock);
	}


For subject2:
- subject2 : How can devfreq support the duplicate call of devfreq_{suspend,resume}_device().
devfreq_{suspend,resume}_device() have to check whether devfreq device is already suspended or not
with refcount or enum devfreq_flag_bits.

If devfreq_{suspend,resume}_device() support it, devfreq_suspend/resume() don't need to consider duplicate call.

> 
> Concerning refcounting, I'm a bit unsure what we wanna count here? Usually we count usage. If usage goes from zero to one, we do something (alloc), and when it goes back from one to zero (free).
> If we keep the current semantics however we have to count number of suspends. That's kinda unintuitive if you ask me.
> 
> With best wishes,
> Tobias
> 
> Tobias Jakobi (7):
>   PM / devfreq: Replace 'stop_polling' boolean with flags
>   PM / devfreq: Track suspend state of DevFreq devices
>   PM / devfreq: Add DevFreq subsystem suspend/resume
>   PM / devfreq: Suspend subsystem on system suspend/hibernate
>   PM / devfreq: exynos-bus: add support for suspend OPP
>   ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus
>   ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus for Prime
> 
>  arch/arm/boot/dts/exynos4412-prime.dtsi |   2 +
>  arch/arm/boot/dts/exynos4x12.dtsi       |   2 +
>  drivers/base/power/main.c               |   3 +
>  drivers/devfreq/devfreq.c               | 170 ++++++++++++++++++++++++++++++--
>  drivers/devfreq/exynos-bus.c            |   8 ++
>  include/linux/devfreq.h                 |  14 ++-
>  6 files changed, 188 insertions(+), 11 deletions(-)
> 


-- 
Regards,
Chanwoo Choi

Re: [RFC v2 7/7] ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus for Prime

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, December 19, 2016 03:16:27 AM Tobias Jakobi wrote:
> Both DMC and leftbus adjust regulator voltages (MIF and INT
> respectively), which have to be in a safe range to allow the
> BL0 to operate correctly.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  arch/arm/boot/dts/exynos4412-prime.dtsi | 2 ++

Could you please post patch(es) adding this file?  I'm planning to fix
OPPs for Exynos4412-Prime and don't wont to duplicate efforts.

Also please remember to mention in the cover letter the tree that your
patchset is based on (if it is different than the mainline).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4412-prime.dtsi b/arch/arm/boot/dts/exynos4412-prime.dtsi
> index 97d1b9c..18e411f 100644
> --- a/arch/arm/boot/dts/exynos4412-prime.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-prime.dtsi
> @@ -123,6 +123,7 @@
>  		opp@400000000 {
>  			opp-hz = /bits/ 64 <400000000>;
>  			opp-microvolt = <1100000>;
> +			opp-suspend;
>  		};
>  	};
>  
> @@ -145,6 +146,7 @@
>  		opp@200000000 {
>  			opp-hz = /bits/ 64 <200000000>;
>  			opp-microvolt = <1150000>;
> +			opp-suspend;
>  		};
>  	};

Re: [RFC v2 7/7] ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus for Prime

[Tobias Jakobi]

Hi Bartlomiej,


Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Monday, December 19, 2016 03:16:27 AM Tobias Jakobi wrote:
>> Both DMC and leftbus adjust regulator voltages (MIF and INT
>> respectively), which have to be in a safe range to allow the
>> BL0 to operate correctly.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  arch/arm/boot/dts/exynos4412-prime.dtsi | 2 ++
> 
> Could you please post patch(es) adding this file?  I'm planning to fix
> OPPs for Exynos4412-Prime and don't wont to duplicate efforts.
I can do it, but I rather not. My patches are based on the vendor kernel
by Hardkernel. But I'm not affiliated with with either Hardkernel nor
Samsung, so I have no access to the reference manual and no idea if the
frequencies and voltages are actually correct. I don't want to sign off
something which I can't verify to be correct...



> Also please remember to mention in the cover letter the tree that your
> patchset is based on (if it is different than the mainline).
I usually base patches on current torvalds/master, but I didn't bother
with this series since it's RFC and very much WIP. I don't think even
the devfreq bits cleanly apply. Anyway, I'm doing it properly once this
is out of RFC.



With best wishes,
Tobias


> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos4412-prime.dtsi b/arch/arm/boot/dts/exynos4412-prime.dtsi
>> index 97d1b9c..18e411f 100644
>> --- a/arch/arm/boot/dts/exynos4412-prime.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412-prime.dtsi
>> @@ -123,6 +123,7 @@
>>  		opp@400000000 {
>>  			opp-hz = /bits/ 64 <400000000>;
>>  			opp-microvolt = <1100000>;
>> +			opp-suspend;
>>  		};
>>  	};
>>  
>> @@ -145,6 +146,7 @@
>>  		opp@200000000 {
>>  			opp-hz = /bits/ 64 <200000000>;
>>  			opp-microvolt = <1150000>;
>> +			opp-suspend;
>>  		};
>>  	};
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC v2 0/7] PM / devfreq: draft for OPP suspend impl (even draftier)

[Tobias Jakobi]

Hello Chanwoo,

thanks for the comments!


Chanwoo Choi wrote:
> Hi Tobias,
> 
> On 2016년 12월 19일 11:16, Tobias Jakobi wrote:
>> Hello everyone,
>>
>> this is v2 of the RFC. You can find the first version here:
>> https://www.spinics.net/lists/linux-samsung-soc/msg56331.html
>>
>> I'm still unhappy with this approach, for multiple reasons:
>> - the global boolean -- cpufreq also has it, but I have the feeling that we don't need it here.
> 
>> - keeping track of which device was suspended looks convoluted -- some reference counting would definitely help here.
>> - the bus driver (here exynos_bus) sets suspend_freq, but devfreq core actually uses it.
>> - returning -EBUSY from devfreq_{suspend,resume}_device() requires handling this in the device drivers -- again refcounting could solve this.
>> - rather a question: do we need the restore on resume -- or should be just wait for the next update through the governor?
>> - like Chanwoo already suggested, move setting suspend freq to devfreq_suspend_device().
> 
> I want to separate the two subject as following:
> - subject1 : Where should devfreq set the suspend-freq for device.
> - subject2 : How can devfreq support the duplicate call of devfreq_{suspend,resume}_device().
> 
> For subject1:
> I explain why devfreq need to set the supend-freq in devfreq_suspend_device().
> Following explanation do not include the refcount of suspend and bit operation.
> 
> The devfreq has two case to enter the suspend mode for devfreq instance as following:
> case 1. Some device driver call the 'devfreq_suspend/resume_device()' directly regardless the sequence of suspend mode (echo mem > /sys/power/state).
> case 2. The system enter the suspend mode by using 'echo mem > /sys/power/state' command.
You forgot normal system reboot and shutdown, but I guess we can sort
this into category 2.

> When setting suspend frequency in the devfreq_suspend/resume_device(),
> this suggestion can cover all cases for suspend freq setting.
> 
> I make the separate function[1] to set the frequency.
> [1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/linux.git/commit/?h�vfreq-test&id�51e989ccaf0ee23a4278724227e879f8752625
Looks like I good idea to encapsulate this. Do you think it makes sense
to base my series on your branch? I think the soonest when this series
is going to land is 4.11 anyway.


> 	devfreq_suspend_device(struct devfreq *devfreq) {
> 		/* Enter the suspend mode */
> 		devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL);
> 
> 		/* Set the suspend-freq */
> 		devfreq_set_target(devfreq, devfreq->suspend_freq, 0);
> 	}
> 
> 	devfreq_resume_device(struct devfreq *devfreq) {
> 		/* Set the resume freq */
> 		devfreq_set_target(devfreq, devfreq->resume_freq, 0);
> 
> 		/* Wakeup from suspend mode */
> 		devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_RESUME, NULL);
> 	}
> 
> And,
> devfreq_suspend() call the devfreq_suspend_device() for all registered devfreq instance.
> 
> 	void devfreq_suspend(void)
> 	{
> 		mutex_lock(&devfreq_list_lock);
> 
> 		list_for_each_entry(devfreq, &devfreq_list, node) {
> 			ret =evfreq_suspend_device(devfreq);
> 			if (ret < 0)
> 				dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
> 		}
> 
> 		mutex_unlock(&devfreq_list_lock);
> 	}
> 
> 	void devfreq_resume(void)
> 	{
> 		mutex_lock(&devfreq_list_lock);
> 
> 		list_for_each_entry(devfreq, &devfreq_list, node) {
> 			ret =evfreq_resume_device(devfreq);
> 			if (ret < 0)
> 				dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
> 		}
> 
> 		mutex_unlock(&devfreq_list_lock);
> 	}
OK, so you rather see the main logic moved into
devfreq_{resume,suspend}_device().

What about my other questions?
- resume freq, yes or no?
- is the core/driver mismatch with respect to setting/using suspend_freq
an issue? (not functional, but for the design)



> For subject2:
> - subject2 : How can devfreq support the duplicate call of devfreq_{suspend,resume}_device().
> devfreq_{suspend,resume}_device() have to check whether devfreq device is already suspended or not
> with refcount or enum devfreq_flag_bits.
I think that with a bitflag we cannot avoid having some extra logic in
the device drivers to handle whatever error code we return from
devfreq_{resume,suspend}_device().

I'll take the refcount route for now.


> If devfreq_{suspend,resume}_device() support it, devfreq_suspend/resume() don't need to consider duplicate call.
> 
>>
>> Concerning refcounting, I'm a bit unsure what we wanna count here? Usually we count usage. If usage goes from zero to one, we do something (alloc), and when it goes back from one to zero (free).
>> If we keep the current semantics however we have to count number of suspends. That's kinda unintuitive if you ask me.
What about this question? So you want me to refcount number of suspend?

Or should we refcount usage, i.e. we start with refcount=1 and allow
refcounts<=1 (also negative).

Again, this is more a design than a functional issue.


I'll try to post a v3 in the next days. Not sure when though, with
Christmas upcoming and all :)


- Tobias

>>
>> With best wishes,
>> Tobias
>>
>> Tobias Jakobi (7):
>>   PM / devfreq: Replace 'stop_polling' boolean with flags
>>   PM / devfreq: Track suspend state of DevFreq devices
>>   PM / devfreq: Add DevFreq subsystem suspend/resume
>>   PM / devfreq: Suspend subsystem on system suspend/hibernate
>>   PM / devfreq: exynos-bus: add support for suspend OPP
>>   ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus
>>   ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus for Prime
>>
>>  arch/arm/boot/dts/exynos4412-prime.dtsi |   2 +
>>  arch/arm/boot/dts/exynos4x12.dtsi       |   2 +
>>  drivers/base/power/main.c               |   3 +
>>  drivers/devfreq/devfreq.c               | 170 ++++++++++++++++++++++++++++++--
>>  drivers/devfreq/exynos-bus.c            |   8 ++
>>  include/linux/devfreq.h                 |  14 ++-
>>  6 files changed, 188 insertions(+), 11 deletions(-)
>>
> 
> 

Re: [RFC v2 0/7] PM / devfreq: draft for OPP suspend impl (even draftier)

[Chanwoo Choi]

Hi Tobias,

2016-12-19 23:56 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> Hello Chanwoo,
>
> thanks for the comments!
>
>
> Chanwoo Choi wrote:
>> Hi Tobias,
>>
>> On 2016년 12월 19일 11:16, Tobias Jakobi wrote:
>>> Hello everyone,
>>>
>>> this is v2 of the RFC. You can find the first version here:
>>> https://www.spinics.net/lists/linux-samsung-soc/msg56331.html
>>>
>>> I'm still unhappy with this approach, for multiple reasons:
>>> - the global boolean -- cpufreq also has it, but I have the feeling that we don't need it here.
>>
>>> - keeping track of which device was suspended looks convoluted -- some reference counting would definitely help here.
>>> - the bus driver (here exynos_bus) sets suspend_freq, but devfreq core actually uses it.
>>> - returning -EBUSY from devfreq_{suspend,resume}_device() requires handling this in the device drivers -- again refcounting could solve this.
>>> - rather a question: do we need the restore on resume -- or should be just wait for the next update through the governor?
>>> - like Chanwoo already suggested, move setting suspend freq to devfreq_suspend_device().
>>
>> I want to separate the two subject as following:
>> - subject1 : Where should devfreq set the suspend-freq for device.
>> - subject2 : How can devfreq support the duplicate call of devfreq_{suspend,resume}_device().
>>
>> For subject1:
>> I explain why devfreq need to set the supend-freq in devfreq_suspend_device().
>> Following explanation do not include the refcount of suspend and bit operation.
>>
>> The devfreq has two case to enter the suspend mode for devfreq instance as following:
>> case 1. Some device driver call the 'devfreq_suspend/resume_device()' directly regardless the sequence of suspend mode (echo mem > /sys/power/state).
>> case 2. The system enter the suspend mode by using 'echo mem > /sys/power/state' command.
> You forgot normal system reboot and shutdown, but I guess we can sort
> this into category 2.
>
>> When setting suspend frequency in the devfreq_suspend/resume_device(),
>> this suggestion can cover all cases for suspend freq setting.
>>
>> I make the separate function[1] to set the frequency.
>> [1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/linux.git/commit/?h�vfreq-test&id�51e989ccaf0ee23a4278724227e879f8752625
> Looks like I good idea to encapsulate this. Do you think it makes sense
> to base my series on your branch? I think the soonest when this series
> is going to land is 4.11 anyway.

I'm not sure. But, I already sent these patches[1][2] to fix the bug
and update devfreq/devfreq-event driver.
If you mention the base commit[1][2] on cover-letter, I think there is
no problem.
[1] https://lkml.org/lkml/2016/12/15/119
[2] https://lkml.org/lkml/2016/12/15/122

>
>
>>       devfreq_suspend_device(struct devfreq *devfreq) {
>>               /* Enter the suspend mode */
>>               devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL);
>>
>>               /* Set the suspend-freq */
>>               devfreq_set_target(devfreq, devfreq->suspend_freq, 0);
>>       }
>>
>>       devfreq_resume_device(struct devfreq *devfreq) {
>>               /* Set the resume freq */
>>               devfreq_set_target(devfreq, devfreq->resume_freq, 0);
>>
>>               /* Wakeup from suspend mode */
>>               devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_RESUME, NULL);
>>       }
>>
>> And,
>> devfreq_suspend() call the devfreq_suspend_device() for all registered devfreq instance.
>>
>>       void devfreq_suspend(void)
>>       {
>>               mutex_lock(&devfreq_list_lock);
>>
>>               list_for_each_entry(devfreq, &devfreq_list, node) {
>>                       ret =evfreq_suspend_device(devfreq);
>>                       if (ret < 0)
>>                               dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
>>               }
>>
>>               mutex_unlock(&devfreq_list_lock);
>>       }
>>
>>       void devfreq_resume(void)
>>       {
>>               mutex_lock(&devfreq_list_lock);
>>
>>               list_for_each_entry(devfreq, &devfreq_list, node) {
>>                       ret =evfreq_resume_device(devfreq);
>>                       if (ret < 0)
>>                               dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
>>               }
>>
>>               mutex_unlock(&devfreq_list_lock);
>>       }
> OK, so you rather see the main logic moved into
> devfreq_{resume,suspend}_device().
>
> What about my other questions?
> - resume freq, yes or no?
> - is the core/driver mismatch with respect to setting/using suspend_freq
> an issue? (not functional, but for the design)

I think that suspend-freq support is enough without resume-freq. After
wakeup from suspend mode, the devfreq will decide the proper frequency
level on first time of governor.

>
>
>
>> For subject2:
>> - subject2 : How can devfreq support the duplicate call of devfreq_{suspend,resume}_device().
>> devfreq_{suspend,resume}_device() have to check whether devfreq device is already suspended or not
>> with refcount or enum devfreq_flag_bits.
> I think that with a bitflag we cannot avoid having some extra logic in
> the device drivers to handle whatever error code we return from
> devfreq_{resume,suspend}_device().
>
> I'll take the refcount route for now.

OK. I recommend that you better to implement the refcount in
devfreq_{suspend,resume}_device().

>
>
>> If devfreq_{suspend,resume}_device() support it, devfreq_suspend/resume() don't need to consider duplicate call.
>>
>>>
>>> Concerning refcounting, I'm a bit unsure what we wanna count here? Usually we count usage. If usage goes from zero to one, we do something (alloc), and when it goes back from one to zero (free).
>>> If we keep the current semantics however we have to count number of suspends. That's kinda unintuitive if you ask me.
> What about this question? So you want me to refcount number of suspend?
>
> Or should we refcount usage, i.e. we start with refcount=1 and allow
> refcounts<=1 (also negative).

I prefer that the refcount start from zero(0). When calling the
devfreq_suspend_device(), the refcount should be increased. This way
support the all of case when calling the devfreq_suspend_device() on
various time.

devfreq_suspend_device(struct devfreq *devfreq)
       if (devfreq->suspend_count > 0) {
             devfreq->suspend_count++;
             return 0;
       }
       devfreq->suspend_count++;

       // ......


devfreq_resume_device(struct devfreq *devfreq)
       if (devfreq->suspend_count >0) {
             devfreq->suspend_count--;
       } else {
             return -EINVAL;
      }

       // ......


>
> Again, this is more a design than a functional issue.
>
>
> I'll try to post a v3 in the next days. Not sure when though, with
> Christmas upcoming and all :)

OK.

[snip]

-- 
Best Regards,
Chanwoo Choi