[PATCH v2 0/2] PM / devfreq: Add support frequency support

[PATCH v2 0/2] PM / devfreq: Add support frequency support

[Chanwoo Choi]

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

These patches support the suspend frequency on devfreq subsystem for case 1.
Lin Huang posted the patch[1] to support the case1. So, I rework Lin Huang's
patch[1] to consider the passive devfreq device using passive governor.

And Tobias would support the following two features on third patches.
He already posted the patches[2] to support the case 2.
- Add new devfreq_{suspend|resume} function for all registered devfreq devices
- Support the reference count to prevent the duplicate call of
  devfreq_{suspend|resume}_device. Tobias and me already discussed it on
  patch[3].

[1] https://patchwork.kernel.org/patch/9445007/
- "[v7] PM/devfreq: add suspend frequency support"
[2] https://www.spinics.net/lists/linux-samsung-soc/msg56602.html
- [RFC v2 0/7] PM / devfreq: draft for OPP suspend impl (even draftier)
[3] https://www.spinics.net/lists/linux-samsung-soc/msg56632.html

Also, I tested these patches on Exynos3250-Rinato board and made the
patch[4][5]. But, the patches[4][5] would be posted after posting the third
patches by Tobias Jakobi.
[4] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-test&id=cf2893454b126380e5513261ec8e3aa94d898e51
[5] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-test&id=fb75ab14e7fbd5b72784dea2c45f7cb171490b6e


Depends on:
- These patches depend on v4.1-rc1 and patches[6][7]

[6] https://lkml.org/lkml/2016/12/28/91
- [PATCH v2 0/3] PM / devfreq: Fix the bug and add reviewer for devfreq support
[7] https://lkml.org/lkml/2016/12/28/102
- [PATCH v2 0/8] PM / devfreq: Update the devfreq and devfreq-event device

Chanwoo Choi (1):
  PM / devfreq: Add separate target function

Lin Huang (1):
  PM / devfreq: Add suspend frequency support

 drivers/devfreq/devfreq.c | 96 +++++++++++++++++++++++++++++++++++------------
 include/linux/devfreq.h   |  2 +
 2 files changed, 73 insertions(+), 25 deletions(-)

-- 
1.9.1

[PATCH v2 1/2] PM / devfreq: Add separate target function

[Chanwoo Choi]

This patch adds the separate target function to set the freq/voltage
because the devfreq have to handle the passive devfreq device using passive
governor when chaning the freq/voltage of parent devfreq device.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 65 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4bd7a8f71b07..e386f14d91c3 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -216,6 +216,43 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
 	return 0;
 }
 
+static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
+				u32 flags)
+{
+	struct devfreq_freqs freqs;
+	unsigned long freq, cur_freq;
+	int ret;
+
+	freq = new_freq;
+	if (devfreq->profile->get_cur_freq)
+		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
+	else
+		cur_freq = devfreq->previous_freq;
+
+	freqs.old = cur_freq;
+	freqs.new = freq;
+	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
+
+	ret = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
+	if (ret < 0) {
+		freqs.new = cur_freq;
+		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
+		return ret;
+	}
+
+	freqs.new = freq;
+	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
+
+	if (devfreq->profile->freq_table
+		&& (devfreq_update_status(devfreq, freq)))
+		dev_err(&devfreq->dev,
+			"Couldn't update frequency transition information.\n");
+
+	devfreq->previous_freq = freq;
+
+	return 0;
+}
+
 /* Load monitoring helper functions for governors use */
 
 /**
@@ -227,8 +264,7 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
  */
 int update_devfreq(struct devfreq *devfreq)
 {
-	struct devfreq_freqs freqs;
-	unsigned long freq, cur_freq;
+	unsigned long freq;
 	int err = 0;
 	u32 flags = 0;
 
@@ -262,31 +298,12 @@ int update_devfreq(struct devfreq *devfreq)
 		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
 	}
 
-	if (devfreq->profile->get_cur_freq)
-		devfreq->profile->get_cur_freq(devfreq->dev.parent, &cur_freq);
-	else
-		cur_freq = devfreq->previous_freq;
-
-	freqs.old = cur_freq;
-	freqs.new = freq;
-	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
-
-	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
-	if (err) {
-		freqs.new = cur_freq;
-		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
+	err = devfreq_set_target(devfreq, freq, flags);
+	if (err < 0) {
+		dev_err(devfreq->dev.parent, "failed to set frequency\n");
 		return err;
 	}
 
-	freqs.new = freq;
-	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
-
-	if (devfreq->profile->freq_table)
-		if (devfreq_update_status(devfreq, freq))
-			dev_err(&devfreq->dev,
-				"Couldn't update frequency transition information.\n");
-
-	devfreq->previous_freq = freq;
 	return err;
 }
 EXPORT_SYMBOL(update_devfreq);
-- 
1.9.1

[PATCH v2 2/2] PM / devfreq: Add suspend frequency support

[Chanwoo Choi]

From: Lin Huang <hl@rock-chips.com>

Add suspend frequency support and if needed set it to
the frequency obtained from the suspend opp (can be defined
using opp-v2 bindings and is optional).

Signed-off-by: Lin Huang <hl@rock-chips.com>
[cw00.choi: Support the passive governor and use separate devfreq_set_target()]
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/devfreq/devfreq.c | 31 ++++++++++++++++++++++++++++++-
 include/linux/devfreq.h   |  2 ++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index e386f14d91c3..8109fb71177b 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -620,6 +620,23 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		goto err_init;
 	}
 
+	/*
+	 * Get the suspend frequency from OPP table. But the devfreq device
+	 * using passive governor don't need to get the suspend frequency
+	 * because the passive devfreq device depend on the parent devfreq
+	 * device.
+	 */
+	devfreq->suspend_freq = 0L;
+	if (strncmp(devfreq->governor_name, "passive", 7)) {
+		struct dev_pm_opp *opp;
+
+		rcu_read_lock();
+		opp = dev_pm_opp_get_suspend_opp(dev);
+		if (opp)
+			devfreq->suspend_freq = dev_pm_opp_get_freq(opp);
+		rcu_read_unlock();
+	}
+
 	devfreq->governor = governor;
 	err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START,
 						NULL);
@@ -777,14 +794,26 @@ void devm_devfreq_remove_device(struct device *dev, struct devfreq *devfreq)
  */
 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)
+		return ret;
+
+	if (devfreq->suspend_freq) {
+		ret = devfreq_set_target(devfreq, devfreq->suspend_freq, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(devfreq_suspend_device);
 
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2de4e2eea180..926bef5a6332 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -141,6 +141,7 @@ struct devfreq_governor {
  *		devfreq.nb to the corresponding register notifier call chain.
  * @work:	delayed work for load monitoring.
  * @previous_freq:	previously configured frequency value.
+ * @suspend_freq:	frequency to set during suspend mode.
  * @data:	Private data of the governor. The devfreq framework does not
  *		touch this.
  * @min_freq:	Limit minimum frequency requested by user (0: none)
@@ -172,6 +173,7 @@ struct devfreq {
 	struct delayed_work work;
 
 	unsigned long previous_freq;
+	unsigned long suspend_freq;
 	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
-- 
1.9.1

Re: [PATCH v2 0/2] PM / devfreq: Add support frequency support

[Chanwoo Choi]

Dear all,

I'm sorry for the version of these patches.
These patches is v1 instead of v2. It is my mistake.

But, if I should send next version, I'll use 'v3' to reduce
the confusion for the duplicate version.

Best Regards,
Chanwoo Choi

On 2016년 12월 28일 21:07, Chanwoo Choi wrote:
> The devfreq has two case to enter the suspend mode for devfreq dev as following:
> case 1: Some devfreq device call the 'devfreq_suspend/resume_device()'
>         directly on the fly regardless of 'echo mem > /sys/power/state'.
> case 2: The system enter the suspend mode by using 'echo mem > sys/power/state'.
> 
> These patches support the suspend frequency on devfreq subsystem for case 1.
> Lin Huang posted the patch[1] to support the case1. So, I rework Lin Huang's
> patch[1] to consider the passive devfreq device using passive governor.
> 
> And Tobias would support the following two features on third patches.
> He already posted the patches[2] to support the case 2.
> - Add new devfreq_{suspend|resume} function for all registered devfreq devices
> - Support the reference count to prevent the duplicate call of
>   devfreq_{suspend|resume}_device. Tobias and me already discussed it on
>   patch[3].
> 
> [1] https://patchwork.kernel.org/patch/9445007/
> - "[v7] PM/devfreq: add suspend frequency support"
> [2] https://www.spinics.net/lists/linux-samsung-soc/msg56602.html
> - [RFC v2 0/7] PM / devfreq: draft for OPP suspend impl (even draftier)
> [3] https://www.spinics.net/lists/linux-samsung-soc/msg56632.html
> 
> Also, I tested these patches on Exynos3250-Rinato board and made the
> patch[4][5]. But, the patches[4][5] would be posted after posting the third
> patches by Tobias Jakobi.
> [4] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-test&id=cf2893454b126380e5513261ec8e3aa94d898e51
> [5] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-test&id=fb75ab14e7fbd5b72784dea2c45f7cb171490b6e
> 
> 
> Depends on:
> - These patches depend on v4.1-rc1 and patches[6][7]
> 
> [6] https://lkml.org/lkml/2016/12/28/91
> - [PATCH v2 0/3] PM / devfreq: Fix the bug and add reviewer for devfreq support
> [7] https://lkml.org/lkml/2016/12/28/102
> - [PATCH v2 0/8] PM / devfreq: Update the devfreq and devfreq-event device
> 
> Chanwoo Choi (1):
>   PM / devfreq: Add separate target function
> 
> Lin Huang (1):
>   PM / devfreq: Add suspend frequency support
> 
>  drivers/devfreq/devfreq.c | 96 +++++++++++++++++++++++++++++++++++------------
>  include/linux/devfreq.h   |  2 +
>  2 files changed, 73 insertions(+), 25 deletions(-)
> 

Re: [PATCH v2 2/2] PM / devfreq: Add suspend frequency support

[Viresh Kumar]

On Wed, Dec 28, 2016 at 5:37 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> +++ b/drivers/devfreq/devfreq.c
> @@ -620,6 +620,23 @@ struct devfreq *devfreq_add_device(struct device *dev,
>                 goto err_init;
>         }
>
> +       /*
> +        * Get the suspend frequency from OPP table. But the devfreq device
> +        * using passive governor don't need to get the suspend frequency
> +        * because the passive devfreq device depend on the parent devfreq
> +        * device.
> +        */
> +       devfreq->suspend_freq = 0L;
> +       if (strncmp(devfreq->governor_name, "passive", 7)) {
> +               struct dev_pm_opp *opp;
> +
> +               rcu_read_lock();
> +               opp = dev_pm_opp_get_suspend_opp(dev);
> +               if (opp)
> +                       devfreq->suspend_freq = dev_pm_opp_get_freq(opp);

The interface has changed a bit recently. Can you please use below
function instead ?

dev_pm_opp_get_suspend_opp_freq()

Re: [PATCH v2 2/2] PM / devfreq: Add suspend frequency support

[Chanwoo Choi]

Hi Viresh,

On 2017년 01월 30일 13:50, Viresh Kumar wrote:
> On Wed, Dec 28, 2016 at 5:37 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -620,6 +620,23 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>                 goto err_init;
>>         }
>>
>> +       /*
>> +        * Get the suspend frequency from OPP table. But the devfreq device
>> +        * using passive governor don't need to get the suspend frequency
>> +        * because the passive devfreq device depend on the parent devfreq
>> +        * device.
>> +        */
>> +       devfreq->suspend_freq = 0L;
>> +       if (strncmp(devfreq->governor_name, "passive", 7)) {
>> +               struct dev_pm_opp *opp;
>> +
>> +               rcu_read_lock();
>> +               opp = dev_pm_opp_get_suspend_opp(dev);
>> +               if (opp)
>> +                       devfreq->suspend_freq = dev_pm_opp_get_freq(opp);
> 
> The interface has changed a bit recently. Can you please use below
> function instead ?

I knew. This patch posted before applying your patch.

> 
> dev_pm_opp_get_suspend_opp_freq()

This patch has not yet reviewed by devfreq maintainer.
So, I'm just waiting the review.

But, this patch is wrong on latest opp patches.
I'll fix it and resend it.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics