[RFC 0/4] PM / devfreq: draft for OPP suspend impl

[RFC 0/4] PM / devfreq: draft for OPP suspend impl

[Tobias Jakobi]

Hello everyone,

this is a (rought) draft for the idea discussed here:
http://www.spinics.net/lists/linux-samsung-soc/msg56311.html

The series is not tested, I don't know if it's even doing what it's supposed to. It's just to show what I have in mind.

Code is obviously inspired by CPUFreq.

With best wishes,
Tobias

Tobias Jakobi (4):
  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

 arch/arm/boot/dts/exynos4x12.dtsi |  2 ++
 drivers/base/power/main.c         |  3 ++
 drivers/devfreq/devfreq.c         | 75 +++++++++++++++++++++++++++++++++++++++
 drivers/devfreq/exynos-bus.c      |  8 +++++
 include/linux/devfreq.h           | 10 ++++++
 5 files changed, 98 insertions(+)

-- 
2.7.3

[RFC 1/4] 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 | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq.h   | 10 +++++++
 2 files changed, 85 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3ea76..2f1aa83 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -785,6 +785,81 @@ 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;
+	unsigned long freq;
+	int ret;
+
+	mutex_lock(&devfreq_list_lock);
+
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		if (!devfreq->suspend_freq)
+			continue;
+
+		ret = devfreq_suspend_device(devfreq);
+		if (ret < 0) {
+			dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
+			continue;
+		}
+
+		devfreq->resume_freq = 0;
+		if (devfreq->profile->get_cur_freq) {
+			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
+			if (ret >= 0)
+				devfreq->resume_freq = freq;
+		}
+
+		freq = devfreq->suspend_freq;
+		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
+
+		if (ret < 0)
+			dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
+	}
+
+	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;
+	unsigned long freq;
+	int ret;
+
+	mutex_lock(&devfreq_list_lock);
+
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		if (!devfreq->suspend_freq)
+			continue;
+
+		freq = devfreq->resume_freq;
+		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
+
+		if (ret < 0) {
+			dev_warn(&devfreq->dev, "%s: setting resume frequency failed\n", __func__);
+			continue;
+		}
+
+		ret = devfreq_resume_device(devfreq);
+		if (ret < 0)
+			dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
+	}
+
+	mutex_unlock(&devfreq_list_lock);
+}
+
+/**
  * 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 2de4e2e..555d024 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -171,6 +171,9 @@ struct devfreq {
 	struct notifier_block nb;
 	struct delayed_work work;
 
+	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;
 
@@ -211,6 +214,10 @@ extern void devm_devfreq_remove_device(struct device *dev,
 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);
@@ -410,6 +417,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 2/4] PM / devfreq: suspend subsystem on system suspend/hibernate

[Tobias Jakobi]

On the Exynos4412 SoC the DevFreq subsystem adjusts frequency
of the various internal busses and 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.

If the system is restarted or goes into a suspend/resume-cycle,
the first-stage (BL0) bootloader takes over, which also
initializes clocks to default values. Since the PMIC is an
external component and not part of the SoC, the BL0 doesn't
set any default voltages. Upon setting the default clocks
for the DMC bus, the BL0 hangs because the corresponding
voltage is too low.

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

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 3/4] PM / devfreq: exynos-bus: add support for suspend OPP

[Tobias Jakobi]

TODO: write desc
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..8a91970 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -335,6 +335,7 @@ static int exynos_bus_parse_of(struct device_node *np,
 			      struct exynos_bus *bus)
 {
 	struct device *dev = bus->dev;
+	struct dev_pm_opp *suspend_opp;
 	unsigned long rate;
 	int ret;
 
@@ -368,6 +369,13 @@ static int exynos_bus_parse_of(struct device_node *np,
 		ret = PTR_ERR(bus->curr_opp);
 		goto err_opp;
 	}
+
+	suspend_opp = dev_pm_opp_get_suspend_opp(dev);
+	if (suspend_opp) {
+		bus->devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
+		printk("DEBUG: suspend opp found!\n");
+	}
+
 	rcu_read_unlock();
 
 	return 0;
-- 
2.7.3

[RFC 4/4] ARM: dts: exynos4x12: add suspend OPP for DMC and leftbus

[Tobias Jakobi]

TODO: write desc
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 d5cb433..a00a8b0 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -375,6 +375,7 @@
 		opp@400000000 {
 			opp-hz = /bits/ 64 <400000000>;
 			opp-microvolt = <1050000>;
+			opp-suspend;
 		};
 	};
 
@@ -463,6 +464,7 @@
 		opp@200000000 {
 			opp-hz = /bits/ 64 <200000000>;
 			opp-microvolt = <1000000>;
+			opp-suspend;
 		};
 	};
 
-- 
2.7.3

Re: [RFC 1/4] PM / devfreq: Add DevFreq subsystem suspend/resume

[Chanwoo Choi]

Hi Tobias,

I like your suggestion. But I have some comment on below.

On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
> 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 | 75 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   | 10 +++++++
>  2 files changed, 85 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..2f1aa83 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -785,6 +785,81 @@ 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;
> +	unsigned long freq;
> +	int ret;
> +
> +	mutex_lock(&devfreq_list_lock);
> +
> +	list_for_each_entry(devfreq, &devfreq_list, node) {
> +		if (!devfreq->suspend_freq)
> +			continue;
> +
> +		ret = devfreq_suspend_device(devfreq);
> +		if (ret < 0) {
> +			dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
> +			continue;
> +		}
> +
> +		devfreq->resume_freq = 0;
> +		if (devfreq->profile->get_cur_freq) {
> +			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
> +			if (ret >= 0)
> +				devfreq->resume_freq = freq;
> +		}
> +
> +		freq = devfreq->suspend_freq;
> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);


DEVFREQ subsystem has the passive governor[2] and devfreq device using passive[1]
governor depends on the parent devfreq device. The passive governor uses
'DEVFREQ_TRANSITION_NOTIFIER' notifier to track the changed state of
parent devfreq device.

When changing the clock/voltage of parent devfreq device,
DEVFREQ subsystem have to notify the changed frequency
to the passive devfreq device.

So, you should call the devfreq_notifiy_transition()
before/after 'devfreq->profile->target' as following:
You can refer the update_devfreq() function 
that is how to use devfreq_notifiy_transition().

For example,
		struct devfreq_freq freqs;
 
		if (devfreq->profile->get_cur_freq) {
			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
			if (ret >= 0)
				devfreq->resume_freq = freq;
		} else {
			devfreq->resume_freq = devfreq_previous_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);
 
		freqs.new = freq;
		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);

                if (ret < 0)
                        dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
        }

[1] DEVFREQ_TRANSITION_NOTIFIER notifier
- https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0fe3a66410a3ba96679be903f1e287d7a0a264a9
[2] DEVFREQ passive governor
- https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=996133119f57334c38b020dbfaaac5b5eb127e29

> +
> +		if (ret < 0)
> +			dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
> +	}
> +
> +	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;
> +	unsigned long freq;
> +	int ret;
> +
> +	mutex_lock(&devfreq_list_lock);
> +
> +	list_for_each_entry(devfreq, &devfreq_list, node) {
> +		if (!devfreq->suspend_freq)
> +			continue;
> +
> +		freq = devfreq->resume_freq;
> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);

ditto.

> +
> +		if (ret < 0) {
> +			dev_warn(&devfreq->dev, "%s: setting resume frequency failed\n", __func__);
> +			continue;
> +		}
> +
> +		ret = devfreq_resume_device(devfreq);
> +		if (ret < 0)
> +			dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
> +	}
> +
> +	mutex_unlock(&devfreq_list_lock);
> +}
> +
> +/**
>   * 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 2de4e2e..555d024 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -171,6 +171,9 @@ struct devfreq {
>  	struct notifier_block nb;
>  	struct delayed_work work;
>  
> +	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;
>  
> @@ -211,6 +214,10 @@ extern void devm_devfreq_remove_device(struct device *dev,
>  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);
> @@ -410,6 +417,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__ */
> 


-- 
Best Regards,
Chanwoo Choi

Re: [RFC 2/4] PM / devfreq: suspend subsystem on system suspend/hibernate

[Chanwoo Choi]

Hi Tobias,

Looks good to me.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Best Regards,
Chanwoo Choi

On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
> On the Exynos4412 SoC the DevFreq subsystem adjusts frequency
> of the various internal busses and 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.
> 
> If the system is restarted or goes into a suspend/resume-cycle,
> the first-stage (BL0) bootloader takes over, which also
> initializes clocks to default values. Since the PMIC is an
> external component and not part of the SoC, the BL0 doesn't
> set any default voltages. Upon setting the default clocks
> for the DMC bus, the BL0 hangs because the corresponding
> voltage is too low.
> 
> To fix this, we make sure to only go into suspend with a 'safe'
> DevFreq configuration.
> 
> 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);
> 

Re: [RFC 3/4] PM / devfreq: exynos-bus: add support for suspend OPP

[Chanwoo Choi]

Hi Tobias,

On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
> TODO: write desc
> 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..8a91970 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -335,6 +335,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>  			      struct exynos_bus *bus)
>  {
>  	struct device *dev = bus->dev;
> +	struct dev_pm_opp *suspend_opp;
>  	unsigned long rate;
>  	int ret;
>  
> @@ -368,6 +369,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>  		ret = PTR_ERR(bus->curr_opp);
>  		goto err_opp;
>  	}
> +
> +	suspend_opp = dev_pm_opp_get_suspend_opp(dev);
> +	if (suspend_opp) {
> +		bus->devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
> +		printk("DEBUG: suspend opp found!\n");
> +	}

Looks good to me. But, you have to move this code
in the exynos_bus_parent_parse_of() because the passive devfreq
don't need to get the suspend_opp. The suspend_opp is necessary
only for parent devfreq device.

If parent devfreq device change the frequency/voltage,
the passive devfreq device will change the their frequency/voltage
according to parent devfreq.

> +
>  	rcu_read_unlock();
>  
>  	return 0;
> 

-- 
Best Regards,
Chanwoo Choi

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

[Chanwoo Choi]

Hi Tobias,

Looks good to me.
Because the bus_dmc (MIF) and bus_leftbus (INT) are parent devfreq device.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Best Regards,
Chanwoo Choi

On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
> TODO: write desc
> 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 d5cb433..a00a8b0 100644
> --- a/arch/arm/boot/dts/exynos4x12.dtsi
> +++ b/arch/arm/boot/dts/exynos4x12.dtsi
> @@ -375,6 +375,7 @@
>  		opp@400000000 {
>  			opp-hz = /bits/ 64 <400000000>;
>  			opp-microvolt = <1050000>;
> +			opp-suspend;
>  		};
>  	};
>  
> @@ -463,6 +464,7 @@
>  		opp@200000000 {
>  			opp-hz = /bits/ 64 <200000000>;
>  			opp-microvolt = <1000000>;
> +			opp-suspend;
>  		};
>  	};
>  
> 

Re: [RFC 1/4] PM / devfreq: Add DevFreq subsystem suspend/resume

[Chanwoo Choi]

Hi Tobias,

On 2016년 11월 24일 10:34, Chanwoo Choi wrote:
> Hi Tobias,
> 
> I like your suggestion. But I have some comment on below.
> 
> On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
>> 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 | 75 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/devfreq.h   | 10 +++++++
>>  2 files changed, 85 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..2f1aa83 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -785,6 +785,81 @@ 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;
>> +	unsigned long freq;
>> +	int ret;
>> +
>> +	mutex_lock(&devfreq_list_lock);
>> +
>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>> +		if (!devfreq->suspend_freq)
>> +			continue;
>> +
>> +		ret = devfreq_suspend_device(devfreq);

The devfreq_suspend_device() stop the monitoring of governors.
But, this function is exported. It means that each devfreq device.
can call the devfreq_suspend/resume_device() (e.g., int drivers/scsi/ufs/ufshcd.c)

So, you should consider following case:
- case :Before calling the devfreq_suspend() and specific devfreq already call
  the devfreq_suspend_device(), how to support it?
  In this case, the specific devfreq device don't want to call
  the devfreq_resume_device() in the devfreq_resume().

>> +		if (ret < 0) {
>> +			dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
>> +			continue;
>> +		}
>> +
>> +		devfreq->resume_freq = 0;
>> +		if (devfreq->profile->get_cur_freq) {
>> +			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
>> +			if (ret >= 0)
>> +				devfreq->resume_freq = freq;
>> +		}
>> +
>> +		freq = devfreq->suspend_freq;
>> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
> 
> 
> DEVFREQ subsystem has the passive governor[2] and devfreq device using passive[1]
> governor depends on the parent devfreq device. The passive governor uses
> 'DEVFREQ_TRANSITION_NOTIFIER' notifier to track the changed state of
> parent devfreq device.
> 
> When changing the clock/voltage of parent devfreq device,
> DEVFREQ subsystem have to notify the changed frequency
> to the passive devfreq device.
> 
> So, you should call the devfreq_notifiy_transition()
> before/after 'devfreq->profile->target' as following:
> You can refer the update_devfreq() function 
> that is how to use devfreq_notifiy_transition().
> 
> For example,
> 		struct devfreq_freq freqs;
>  
> 		if (devfreq->profile->get_cur_freq) {
> 			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
> 			if (ret >= 0)
> 				devfreq->resume_freq = freq;
> 		} else {
> 			devfreq->resume_freq = devfreq_previous_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);
>  
> 		freqs.new = freq;
> 		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
> 
>                 if (ret < 0)
>                         dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
>         }
> 
> [1] DEVFREQ_TRANSITION_NOTIFIER notifier
> - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0fe3a66410a3ba96679be903f1e287d7a0a264a9
> [2] DEVFREQ passive governor
> - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=996133119f57334c38b020dbfaaac5b5eb127e29
> 
>> +
>> +		if (ret < 0)
>> +			dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
>> +	}
>> +
>> +	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;
>> +	unsigned long freq;
>> +	int ret;
>> +
>> +	mutex_lock(&devfreq_list_lock);
>> +
>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>> +		if (!devfreq->suspend_freq)
>> +			continue;
>> +
>> +		freq = devfreq->resume_freq;
>> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
> 
> ditto.
> 
>> +
>> +		if (ret < 0) {
>> +			dev_warn(&devfreq->dev, "%s: setting resume frequency failed\n", __func__);
>> +			continue;
>> +		}
>> +
>> +		ret = devfreq_resume_device(devfreq);
>> +		if (ret < 0)
>> +			dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
>> +	}
>> +
>> +	mutex_unlock(&devfreq_list_lock);
>> +}
>> +
>> +/**
>>   * 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 2de4e2e..555d024 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -171,6 +171,9 @@ struct devfreq {
>>  	struct notifier_block nb;
>>  	struct delayed_work work;
>>  
>> +	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;
>>  
>> @@ -211,6 +214,10 @@ extern void devm_devfreq_remove_device(struct device *dev,
>>  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);
>> @@ -410,6 +417,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__ */
>>
> 
> 
Best Regards,
Chanwoo Choi

Re: [RFC 1/4] PM / devfreq: Add DevFreq subsystem suspend/resume

[Tobias Jakobi]

Hey Chanwoo,

first of all, thanks for the help!


Chanwoo Choi wrote:
> Hi Tobias,
> 
> On 2016년 11월 24일 10:34, Chanwoo Choi wrote:
>> Hi Tobias,
>>
>> I like your suggestion. But I have some comment on below.
>>
>> On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
>>> 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 | 75 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/devfreq.h   | 10 +++++++
>>>  2 files changed, 85 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index bf3ea76..2f1aa83 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -785,6 +785,81 @@ 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;
>>> +	unsigned long freq;
>>> +	int ret;
>>> +
>>> +	mutex_lock(&devfreq_list_lock);
>>> +
>>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>>> +		if (!devfreq->suspend_freq)
>>> +			continue;
>>> +
>>> +		ret = devfreq_suspend_device(devfreq);
> 
> The devfreq_suspend_device() stop the monitoring of governors.
> But, this function is exported. It means that each devfreq device.
> can call the devfreq_suspend/resume_device() (e.g., int drivers/scsi/ufs/ufshcd.c)
> 
> So, you should consider following case:
> - case :Before calling the devfreq_suspend() and specific devfreq already call
>   the devfreq_suspend_device(), how to support it?
>   In this case, the specific devfreq device don't want to call
>   the devfreq_resume_device() in the devfreq_resume().
How about this idea?

Introduce a boolean 'devfreq_suspended' (again similar to CPUFreq) and
set it to true (atomically?) once we have entered devfreq_suspend().

At the end of devfreq_suspend() we set it to false again.

We just need to check in devfreq_{suspend,resume}_device() for
'devfreq_suspended' and return some error code (-EBUSY maybe?) to the
caller.

Of course I would inline the devfreq->governor->event_handler() call
into devfreq_{suspend,resume}() first.

Does this look sane?

Also, do you see any other exported calls which we might have to
'protect' during suspended state?


With best wishes,
Tobias


>>> +		if (ret < 0) {
>>> +			dev_warn(&devfreq->dev, "%s: governor suspend failed\n", __func__);
>>> +			continue;
>>> +		}
>>> +
>>> +		devfreq->resume_freq = 0;
>>> +		if (devfreq->profile->get_cur_freq) {
>>> +			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
>>> +			if (ret >= 0)
>>> +				devfreq->resume_freq = freq;
>>> +		}
>>> +
>>> +		freq = devfreq->suspend_freq;
>>> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
>>
>>
>> DEVFREQ subsystem has the passive governor[2] and devfreq device using passive[1]
>> governor depends on the parent devfreq device. The passive governor uses
>> 'DEVFREQ_TRANSITION_NOTIFIER' notifier to track the changed state of
>> parent devfreq device.
>>
>> When changing the clock/voltage of parent devfreq device,
>> DEVFREQ subsystem have to notify the changed frequency
>> to the passive devfreq device.
>>
>> So, you should call the devfreq_notifiy_transition()
>> before/after 'devfreq->profile->target' as following:
>> You can refer the update_devfreq() function 
>> that is how to use devfreq_notifiy_transition().
>>
>> For example,
>> 		struct devfreq_freq freqs;
>>  
>> 		if (devfreq->profile->get_cur_freq) {
>> 			ret = devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq);
>> 			if (ret >= 0)
>> 				devfreq->resume_freq = freq;
>> 		} else {
>> 			devfreq->resume_freq = devfreq_previous_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);
>>  
>> 		freqs.new = freq;
>> 		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>
>>                 if (ret < 0)
>>                         dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
>>         }
>>
>> [1] DEVFREQ_TRANSITION_NOTIFIER notifier
>> - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=0fe3a66410a3ba96679be903f1e287d7a0a264a9
>> [2] DEVFREQ passive governor
>> - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=996133119f57334c38b020dbfaaac5b5eb127e29
>>
>>> +
>>> +		if (ret < 0)
>>> +			dev_warn(&devfreq->dev, "%s: setting suspend frequency failed\n", __func__);
>>> +	}
>>> +
>>> +	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;
>>> +	unsigned long freq;
>>> +	int ret;
>>> +
>>> +	mutex_lock(&devfreq_list_lock);
>>> +
>>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>>> +		if (!devfreq->suspend_freq)
>>> +			continue;
>>> +
>>> +		freq = devfreq->resume_freq;
>>> +		ret = devfreq->profile->target(devfreq->dev.parent, &freq, 0);
>>
>> ditto.
>>
>>> +
>>> +		if (ret < 0) {
>>> +			dev_warn(&devfreq->dev, "%s: setting resume frequency failed\n", __func__);
>>> +			continue;
>>> +		}
>>> +
>>> +		ret = devfreq_resume_device(devfreq);
>>> +		if (ret < 0)
>>> +			dev_warn(&devfreq->dev, "%s: governor resume failed\n", __func__);
>>> +	}
>>> +
>>> +	mutex_unlock(&devfreq_list_lock);
>>> +}
>>> +
>>> +/**
>>>   * 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 2de4e2e..555d024 100644
>>> --- a/include/linux/devfreq.h
>>> +++ b/include/linux/devfreq.h
>>> @@ -171,6 +171,9 @@ struct devfreq {
>>>  	struct notifier_block nb;
>>>  	struct delayed_work work;
>>>  
>>> +	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;
>>>  
>>> @@ -211,6 +214,10 @@ extern void devm_devfreq_remove_device(struct device *dev,
>>>  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);
>>> @@ -410,6 +417,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__ */
>>>
>>
>>
> Best Regards,
> Chanwoo Choi
> --
> 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 3/4] PM / devfreq: exynos-bus: add support for suspend OPP

[Tobias Jakobi]

Hello Chanwoo,

Chanwoo Choi wrote:
> Hi Tobias,
> 
> On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
>> TODO: write desc
>> 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..8a91970 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -335,6 +335,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>  			      struct exynos_bus *bus)
>>  {
>>  	struct device *dev = bus->dev;
>> +	struct dev_pm_opp *suspend_opp;
>>  	unsigned long rate;
>>  	int ret;
>>  
>> @@ -368,6 +369,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>  		ret = PTR_ERR(bus->curr_opp);
>>  		goto err_opp;
>>  	}
>> +
>> +	suspend_opp = dev_pm_opp_get_suspend_opp(dev);
>> +	if (suspend_opp) {
>> +		bus->devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
>> +		printk("DEBUG: suspend opp found!\n");
>> +	}
> 
> Looks good to me. But, you have to move this code
> in the exynos_bus_parent_parse_of() because the passive devfreq
> don't need to get the suspend_opp. The suspend_opp is necessary
> only for parent devfreq device.
right, that makes sense!

Thanks again for the review!

- Tobias


> If parent devfreq device change the frequency/voltage,
> the passive devfreq device will change the their frequency/voltage
> according to parent devfreq.
> 
>> +
>>  	rcu_read_unlock();
>>  
>>  	return 0;
>>
> 

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

[Tobias Jakobi]

Hello Chanwoo,

since exynos-bus is not only used for the Exynos4 platform, could you
maybe check if we need to apply this elsewhere?

I see in the exynos5422-odroidxu3-common.dtsi that bus_wcore adjusts the
INT voltage. But MyungJoo mentioned that on newer platform the BL0 might
be "smarter", so we don't need this.

With best wishes,
Tobias


Chanwoo Choi wrote:
> Hi Tobias,
> 
> Looks good to me.
> Because the bus_dmc (MIF) and bus_leftbus (INT) are parent devfreq device.
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
> Best Regards,
> Chanwoo Choi
> 
> On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
>> TODO: write desc
>> 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 d5cb433..a00a8b0 100644
>> --- a/arch/arm/boot/dts/exynos4x12.dtsi
>> +++ b/arch/arm/boot/dts/exynos4x12.dtsi
>> @@ -375,6 +375,7 @@
>>  		opp@400000000 {
>>  			opp-hz = /bits/ 64 <400000000>;
>>  			opp-microvolt = <1050000>;
>> +			opp-suspend;
>>  		};
>>  	};
>>  
>> @@ -463,6 +464,7 @@
>>  		opp@200000000 {
>>  			opp-hz = /bits/ 64 <200000000>;
>>  			opp-microvolt = <1000000>;
>> +			opp-suspend;
>>  		};
>>  	};
>>  
>>
> 

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

[Chanwoo Choi]

Hi Tobias,

On 2016년 11월 24일 23:27, Tobias Jakobi wrote:
> Hello Chanwoo,
> 
> since exynos-bus is not only used for the Exynos4 platform, could you
> maybe check if we need to apply this elsewhere?

Sure, I'll review and test your patches on following boards:
- exynos4412-u3/exynos5422-xu3/exynos3250-rinato.

Regards,
Chanwoo Choi

> 
> I see in the exynos5422-odroidxu3-common.dtsi that bus_wcore adjusts the
> INT voltage. But MyungJoo mentioned that on newer platform the BL0 might
> be "smarter", so we don't need this.
> 
> With best wishes,
> Tobias
> 
> 
> Chanwoo Choi wrote:
>> Hi Tobias,
>>
>> Looks good to me.
>> Because the bus_dmc (MIF) and bus_leftbus (INT) are parent devfreq device.
>>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>> Best Regards,
>> Chanwoo Choi
>>
>> On 2016년 11월 23일 22:51, Tobias Jakobi wrote:
>>> TODO: write desc
>>> 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 d5cb433..a00a8b0 100644
>>> --- a/arch/arm/boot/dts/exynos4x12.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4x12.dtsi
>>> @@ -375,6 +375,7 @@
>>>  		opp@400000000 {
>>>  			opp-hz = /bits/ 64 <400000000>;
>>>  			opp-microvolt = <1050000>;
>>> +			opp-suspend;
>>>  		};
>>>  	};
>>>  
>>> @@ -463,6 +464,7 @@
>>>  		opp@200000000 {
>>>  			opp-hz = /bits/ 64 <200000000>;
>>>  			opp-microvolt = <1000000>;
>>> +			opp-suspend;
>>>  		};
>>>  	};
>>>  
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi