[PATCH 0/6] devfreq: handle suspend/resume

[PATCH 0/6] devfreq: handle suspend/resume

[Lukasz Luba]

Hi all,

This patch set aims to address the issue with devfreq devices' frequency
during suspend/resume. It extends suspend/resume by calls to Devfreq
framework. In the devfreq framework there is a small refactoring to avoid
code duplication in changging frequency (patch 2) and there are extensions
for suspending devices.

It has been tested on Odroid u3 with Exynos 4412.

The patch set draws on Tobias Jakobi's work posted ~2 years ago, who tried
to solve issue with devfreq device's frequency during suspend/resume.
During the discussion on LKML some corner cases and comments appeared
related to the design. This patch set address them keeping in mind
suggestions from Chanwoo Choi.
Tobias's paches:
https://www.spinics.net/lists/linux-samsung-soc/msg56602.html 

Regards,
Lukasz Luba

Lukasz Luba (6):
  devfreq: add basic fileds supporting suspend functionality
  devfreq: refactor set_target frequency function
  devfreq: add support for suspend/resume of a devfreq device
  devfreq: add devfreq_suspend/resume() functions
  drivers: power: suspend: call devfreq suspend/resume
  arm: dts: exynos4: set opp-suspend for DMC and leftbus

 arch/arm/boot/dts/exynos4210.dtsi |   2 +
 arch/arm/boot/dts/exynos4412.dtsi |   2 +
 drivers/base/power/main.c         |   3 +
 drivers/devfreq/devfreq.c         | 159 ++++++++++++++++++++++++++++++--------
 include/linux/devfreq.h           |  11 +++
 5 files changed, 146 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH 1/6] devfreq: add basic fileds supporting suspend functionality

[Lukasz Luba]

The patch prepares devfreq device for handling suspend/resume functionality.
The new fields will store needed information during this process.
Devfreq framework handles opp-suspend DT entry and there is no need of
modyfications in the drivers code.

The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
solve issue with devfreq device's frequency during suspend/resume.
During the discussion on LKML some corner cases and comments appeared
related to the design. This patch address them keeping in mind suggestions
from Chanwoo Choi.

Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/devfreq.c | 3 +++
 include/linux/devfreq.h   | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1414130..e20e7e4 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -657,6 +657,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	}
 	devfreq->max_freq = devfreq->scaling_max_freq;
 
+	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
+	atomic_set(&devfreq->suspend_count, 0);
+
 	dev_set_name(&devfreq->dev, "devfreq%d",
 				atomic_inc_return(&devfreq_no));
 	err = device_register(&devfreq->dev);
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index e4963b0..7fe96f9 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -167,6 +167,10 @@ struct devfreq {
 	unsigned long scaling_max_freq;
 	bool stop_polling;
 
+	unsigned long suspend_freq;
+	unsigned long resume_freq;
+	atomic_t suspend_count;
+
 	/* information for device frequency transition */
 	unsigned int total_trans;
 	unsigned int *trans_table;
-- 
2.7.4

[PATCH 2/6] devfreq: refactor set_target frequency function

[Lukasz Luba]

The refactoring is needed for the new client in devfreq: suspend.
To avoid code duplication, move it to the new local function
devfreq_set_target.

The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
solve issue with devfreq device's frequency during suspend/resume.
During the discussion on LKML some corner cases and comments appeared
related to the design. This patch address them keeping in mind suggestions
from Chanwoo Choi.

Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/devfreq.c | 62 ++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index e20e7e4..cf9c643 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -285,6 +285,42 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
 	return 0;
 }
 
+static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
+			      unsigned long *prev_freq, u32 flags)
+{
+	struct devfreq_freqs freqs;
+	unsigned long cur_freq;
+	int err = 0;
+
+	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 = new_freq;
+	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
+
+	err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags);
+	if (err) {
+		freqs.new = cur_freq;
+		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
+		return err;
+	}
+
+	freqs.new = new_freq;
+	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
+
+	if (devfreq_update_status(devfreq, new_freq))
+		dev_err(&devfreq->dev,
+			"Couldn't update frequency transition information.\n");
+
+	devfreq->previous_freq = new_freq;
+	*prev_freq = cur_freq;
+
+	return err;
+}
+
 /* Load monitoring helper functions for governors use */
 
 /**
@@ -296,7 +332,6 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
  */
 int update_devfreq(struct devfreq *devfreq)
 {
-	struct devfreq_freqs freqs;
 	unsigned long freq, cur_freq, min_freq, max_freq;
 	int err = 0;
 	u32 flags = 0;
@@ -333,31 +368,8 @@ 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);
+	return devfreq_set_target(devfreq, freq, &cur_freq, flags);
 
-	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
-	if (err) {
-		freqs.new = cur_freq;
-		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
-		return err;
-	}
-
-	freqs.new = freq;
-	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
-
-	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);
 
-- 
2.7.4

[PATCH 3/6] devfreq: add support for suspend/resume of a devfreq device

[Lukasz Luba]

The patch adds support for handling suspend/resume process.
It uses atomic variables to make sure no race condition
affects the process.

The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
solve issue with devfreq device's frequency during suspend/resume.
During the discussion on LKML some corner cases and comments appeared
related to the design. This patch address them keeping in mind suggestions
from Chanwoo Choi.

Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/devfreq.c | 45 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index cf9c643..7e09de8 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -872,14 +872,33 @@ EXPORT_SYMBOL(devm_devfreq_remove_device);
  */
 int devfreq_suspend_device(struct devfreq *devfreq)
 {
-	if (!devfreq)
-		return -EINVAL;
+        int ret;
+        unsigned long prev_freq;
+        u32 flags = 0;
+
+        if (!devfreq)
+                return -EINVAL;
+
+        if (devfreq->governor) {
+		ret = devfreq->governor->event_handler(devfreq,
+					DEVFREQ_GOV_SUSPEND, NULL);
+		if (ret)
+			return ret;
+	}
 
-	if (!devfreq->governor)
-		return 0;
+	if (devfreq->suspend_freq) {
+		if (atomic_inc_return(&devfreq->suspend_count) > 1)
+			return 0;
 
-	return devfreq->governor->event_handler(devfreq,
-				DEVFREQ_GOV_SUSPEND, NULL);
+		ret = devfreq_set_target(devfreq, devfreq->suspend_freq,
+					 &prev_freq, flags);
+		if (ret)
+			return ret;
+
+		devfreq->resume_freq = prev_freq;
+	}
+
+        return 0;
 }
 EXPORT_SYMBOL(devfreq_suspend_device);
 
@@ -893,9 +912,23 @@ EXPORT_SYMBOL(devfreq_suspend_device);
  */
 int devfreq_resume_device(struct devfreq *devfreq)
 {
+        int ret;
+        unsigned long prev_freq;
+        u32 flags = 0;
+
 	if (!devfreq)
 		return -EINVAL;
 
+	if (devfreq->suspend_freq) {
+		if (atomic_dec_return(&devfreq->suspend_count) >= 1)
+			return 0;
+
+		ret = devfreq_set_target(devfreq, devfreq->resume_freq,
+					 &prev_freq, flags);
+		if (ret)
+			return ret;
+	}
+
 	if (!devfreq->governor)
 		return 0;
 
-- 
2.7.4

[PATCH 4/6] devfreq: add devfreq_suspend/resume() functions

[Lukasz Luba]

This patch adds implementation for global suspend/resume for
devfreq framework. System suspend will next use these functions.

The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
solve issue with devfreq device's frequency during suspend/resume.
During the discussion on LKML some corner cases and comments appeared
related to the design. This patch address them keeping in mind suggestions
from Chanwoo Choi.

Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/devfreq.h   |  7 +++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7e09de8..2f4391c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -44,6 +44,9 @@ static LIST_HEAD(devfreq_governor_list);
 static LIST_HEAD(devfreq_list);
 static DEFINE_MUTEX(devfreq_list_lock);
 
+/* Flag showing state of suspend/resume */
+static bool devfreq_suspended;
+
 /**
  * find_device_devfreq() - find devfreq struct using device pointer
  * @dev:	device pointer used to lookup device devfreq.
@@ -938,6 +941,52 @@ int devfreq_resume_device(struct devfreq *devfreq)
 EXPORT_SYMBOL(devfreq_resume_device);
 
 /**
+ * devfreq_suspend() - Suspend devfreq governors and devices
+ *
+ * Called during system wide Suspend/Hibernate cycles for suspending governors
+ * and devices preserving the state for resume. On some platforms the devfreq
+ * device must have precise state (frequency) after resume in order to provide
+ * fully operating setup.
+ */
+void devfreq_suspend(void)
+{
+	struct devfreq *devfreq;
+	int ret;
+
+	mutex_lock(&devfreq_list_lock);
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		ret = devfreq_suspend_device(devfreq);
+		if (ret)
+			dev_warn(&devfreq->dev, "device suspend failed\n");
+	}
+	mutex_unlock(&devfreq_list_lock);
+
+	devfreq_suspended = true;
+}
+
+/**
+ * devfreq_resume() - Resume devfreq governors and devices
+ *
+ * Called during system wide Suspend/Hibernate cycle for resuming governors and
+ * devices that are suspended with devfreq_suspend().
+ */
+void devfreq_resume(void)
+{
+	struct devfreq *devfreq;
+	int ret;
+
+	devfreq_suspended = false;
+
+	mutex_lock(&devfreq_list_lock);
+	list_for_each_entry(devfreq, &devfreq_list, node) {
+		ret = devfreq_resume_device(devfreq);
+		if (ret)
+			dev_warn(&devfreq->dev, "device resume failed\n");
+	}
+	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 7fe96f9..4f0fea8 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -202,6 +202,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);
 
+
+extern void devfreq_suspend(void);
+extern void devfreq_resume(void);
+
 /**
  * update_devfreq() - Reevaluate the device and configure frequency
  * @devfreq:	the devfreq device
@@ -396,6 +400,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.4

[PATCH 5/6] drivers: power: suspend: call devfreq suspend/resume

[Lukasz Luba]

Devfreq framework supports suspend of its devices.
Call the the devfreq interface and allow devfreq devices preserve/restore
their states during suspend/resume.

The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
solve issue with devfreq device's frequency during suspend/resume.
During the discussion on LKML some corner cases and comments appeared
related to the design. This patch address them keeping in mind suggestions
from Chanwoo Choi.

Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 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 a690fd4..0992e67 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"
@@ -1078,6 +1079,7 @@ void dpm_resume(pm_message_t state)
 	dpm_show_time(starttime, state, 0, NULL);
 
 	cpufreq_resume();
+	devfreq_resume();
 	trace_suspend_resume(TPS("dpm_resume"), state.event, false);
 }
 
@@ -1852,6 +1854,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.4

[PATCH 6/6] arm: dts: exynos4: set opp-suspend for DMC and leftbus

[Lukasz Luba]

Mark the state for devfreq device while entring suspend/resume process.

The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
solve issue with devfreq device's frequency during suspend/resume.
During the discussion on LKML some corner cases and comments appeared
related to the design. This patch address them keeping in mind suggestions
from Chanwoo Choi.

Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 arch/arm/boot/dts/exynos4210.dtsi | 2 ++
 arch/arm/boot/dts/exynos4412.dtsi | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index b6091c2..4429b72 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -298,6 +298,7 @@
 			opp-400000000 {
 				opp-hz = /bits/ 64 <400000000>;
 				opp-microvolt = <1150000>;
+				opp-suspend;
 			};
 		};
 
@@ -367,6 +368,7 @@
 			};
 			opp-200000000 {
 				opp-hz = /bits/ 64 <200000000>;
+				opp-suspend;
 			};
 		};
 	};
diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index 51f72f0..908c0c4 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -432,6 +432,7 @@
 			opp-400000000 {
 				opp-hz = /bits/ 64 <400000000>;
 				opp-microvolt = <1050000>;
+				opp-suspend;
 			};
 		};
 
@@ -520,6 +521,7 @@
 			opp-200000000 {
 				opp-hz = /bits/ 64 <200000000>;
 				opp-microvolt = <1000000>;
+				opp-suspend;
 			};
 		};
 
-- 
2.7.4

Re: [PATCH 1/6] devfreq: add basic fileds supporting suspend functionality

[Chanwoo Choi]

Hi,

On 2018년 11월 22일 03:01, Lukasz Luba wrote:
> The patch prepares devfreq device for handling suspend/resume functionality.
> The new fields will store needed information during this process.
> Devfreq framework handles opp-suspend DT entry and there is no need of
> modyfications in the drivers code.
> 
> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
> solve issue with devfreq device's frequency during suspend/resume.
> During the discussion on LKML some corner cases and comments appeared
> related to the design. This patch address them keeping in mind suggestions
> from Chanwoo Choi.

You already explain the patch history on cover letter. It is enough.
Please remove the duplicate history description from all patches except for cover letter.

> 
> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 3 +++
>  include/linux/devfreq.h   | 4 ++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1414130..e20e7e4 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -657,6 +657,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	}
>  	devfreq->max_freq = devfreq->scaling_max_freq;
>  
> +	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
> +	atomic_set(&devfreq->suspend_count, 0);
> +
>  	dev_set_name(&devfreq->dev, "devfreq%d",
>  				atomic_inc_return(&devfreq_no));
>  	err = device_register(&devfreq->dev);
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index e4963b0..7fe96f9 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -167,6 +167,10 @@ struct devfreq {
>  	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
> +	unsigned long suspend_freq;
> +	unsigned long resume_freq;
> +	atomic_t suspend_count;
> +
>  	/* information for device frequency transition */
>  	unsigned int total_trans;
>  	unsigned int *trans_table;
> 

You don't need to make the separate patch for this. You can squash patch1
into patch3 because the newly added variables are used on patch3.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/6] devfreq: refactor set_target frequency function

[Chanwoo Choi]

On 2018년 11월 22일 03:01, Lukasz Luba wrote:
> The refactoring is needed for the new client in devfreq: suspend.
> To avoid code duplication, move it to the new local function
> devfreq_set_target.
> 
> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
> solve issue with devfreq device's frequency during suspend/resume.
> During the discussion on LKML some corner cases and comments appeared
> related to the design. This patch address them keeping in mind suggestions
> from Chanwoo Choi.

As I commented on patch1, please remove it.

> 
> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 62 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index e20e7e4..cf9c643 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -285,6 +285,42 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>  	return 0;
>  }
>  
> +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
> +			      unsigned long *prev_freq, u32 flags)

Please remove the unused space in front of 'unsigned long *prev_freq'.
Use tab only for indentation.

> +{
> +	struct devfreq_freqs freqs;
> +	unsigned long cur_freq;
> +	int err = 0;
> +
> +	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 = new_freq;
> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
> +
> +	err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags);
> +	if (err) {
> +		freqs.new = cur_freq;
> +		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
> +		return err;
> +	}
> +
> +	freqs.new = new_freq;
> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
> +
> +	if (devfreq_update_status(devfreq, new_freq))
> +		dev_err(&devfreq->dev,
> +			"Couldn't update frequency transition information.\n");
> +
> +	devfreq->previous_freq = new_freq;
> +	*prev_freq = cur_freq;
> +
> +	return err;
> +}
> +
>  /* Load monitoring helper functions for governors use */
>  
>  /**
> @@ -296,7 +332,6 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>   */
>  int update_devfreq(struct devfreq *devfreq)
>  {
> -	struct devfreq_freqs freqs;
>  	unsigned long freq, cur_freq, min_freq, max_freq;


cur_freq is not used after modification. Remove it.

>  	int err = 0;
>  	u32 flags = 0;
> @@ -333,31 +368,8 @@ 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);
> +	return devfreq_set_target(devfreq, freq, &cur_freq, flags);

You get the 'cur_freq' from devfreq_set_taget() for devfreq_suspend_device() on patch3.
But, update_devfreq() and devfreq_resume_device() don't use the 'cur_freq' value
from devfreq_set_target().

Instead, getting 'cur_freq' for 'devfreq->resume_freq' in the devfreq_set_target().
Please remove the 'cur_freq' parameter from devfreq_set_target.

>  
> -	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
> -	if (err) {
> -		freqs.new = cur_freq;
> -		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
> -		return err;
> -	}
> -
> -	freqs.new = freq;
> -	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
> -
> -	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);
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/6] devfreq: refactor set_target frequency function

[Chanwoo Choi]

On 2018년 11월 22일 11:52, Chanwoo Choi wrote:
> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>> The refactoring is needed for the new client in devfreq: suspend.
>> To avoid code duplication, move it to the new local function
>> devfreq_set_target.
>>
>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>> solve issue with devfreq device's frequency during suspend/resume.
>> During the discussion on LKML some corner cases and comments appeared
>> related to the design. This patch address them keeping in mind suggestions
>> from Chanwoo Choi.
> 
> As I commented on patch1, please remove it.
> 
>>
>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 62 ++++++++++++++++++++++++++++-------------------
>>  1 file changed, 37 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index e20e7e4..cf9c643 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -285,6 +285,42 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>  	return 0;
>>  }
>>  
>> +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>> +			      unsigned long *prev_freq, u32 flags)
> 
> Please remove the unused space in front of 'unsigned long *prev_freq'.
> Use tab only for indentation.
> 
>> +{
>> +	struct devfreq_freqs freqs;
>> +	unsigned long cur_freq;
>> +	int err = 0;
>> +
>> +	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 = new_freq;
>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
>> +
>> +	err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags);
>> +	if (err) {
>> +		freqs.new = cur_freq;
>> +		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> +		return err;
>> +	}
>> +
>> +	freqs.new = new_freq;
>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> +
>> +	if (devfreq_update_status(devfreq, new_freq))
>> +		dev_err(&devfreq->dev,
>> +			"Couldn't update frequency transition information.\n");
>> +
>> +	devfreq->previous_freq = new_freq;
>> +	*prev_freq = cur_freq;
>> +
>> +	return err;
>> +}

You can change the devfreq_set_target as following:

 static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
-                             unsigned long *prev_freq, u32 flags)
+                       u32 flags)
 {
        struct devfreq_freqs freqs;
        unsigned long cur_freq;
@@ -319,7 +319,9 @@ static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
                        "Couldn't update frequency transition information.\n");
 
        devfreq->previous_freq = new_freq;
-       *prev_freq = cur_freq;


But, you have to add following new code on patch3 instead of patch2.

+
+       if (devfreq->suspend_freq)
+               devfreq->resume_freq = cur_freq;

 
        return err;
 }



>> +
>>  /* Load monitoring helper functions for governors use */
>>  
>>  /**
>> @@ -296,7 +332,6 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>   */
>>  int update_devfreq(struct devfreq *devfreq)
>>  {
>> -	struct devfreq_freqs freqs;
>>  	unsigned long freq, cur_freq, min_freq, max_freq;
> 
> 
> cur_freq is not used after modification. Remove it.
> 
>>  	int err = 0;
>>  	u32 flags = 0;
>> @@ -333,31 +368,8 @@ 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);
>> +	return devfreq_set_target(devfreq, freq, &cur_freq, flags);
> 
> You get the 'cur_freq' from devfreq_set_taget() for devfreq_suspend_device() on patch3.
> But, update_devfreq() and devfreq_resume_device() don't use the 'cur_freq' value
> from devfreq_set_target().
> 
> Instead, getting 'cur_freq' for 'devfreq->resume_freq' in the devfreq_set_target().
> Please remove the 'cur_freq' parameter from devfreq_set_target.
> 
>>  
>> -	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>> -	if (err) {
>> -		freqs.new = cur_freq;
>> -		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> -		return err;
>> -	}
>> -
>> -	freqs.new = freq;
>> -	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> -
>> -	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);
>>  
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 3/6] devfreq: add support for suspend/resume of a devfreq device

[Chanwoo Choi]

On 2018년 11월 22일 03:01, Lukasz Luba wrote:
> The patch adds support for handling suspend/resume process.
> It uses atomic variables to make sure no race condition
> affects the process.
> 
> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
> solve issue with devfreq device's frequency during suspend/resume.
> During the discussion on LKML some corner cases and comments appeared
> related to the design. This patch address them keeping in mind suggestions
> from Chanwoo Choi.

Please remove the duplicate information about patch history.

> 
> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 45 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index cf9c643..7e09de8 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -872,14 +872,33 @@ EXPORT_SYMBOL(devm_devfreq_remove_device);
>   */
>  int devfreq_suspend_device(struct devfreq *devfreq)
>  {
> -	if (!devfreq)
> -		return -EINVAL;
> +        int ret;

Move 'ret' definition under 'if (devfreq->suspend_freq) {'
because 'ret' is used if suspend_freq isn't zero.

> +        unsigned long prev_freq;
> +        u32 flags = 0;
> +
> +        if (!devfreq)
> +                return -EINVAL;
> +
> +        if (devfreq->governor) {
> +		ret = devfreq->governor->event_handler(devfreq,
> +					DEVFREQ_GOV_SUSPEND, NULL);
> +		if (ret)
> +			return ret;
> +	}
>  
> -	if (!devfreq->governor)
> -		return 0;
> +	if (devfreq->suspend_freq) {
> +		if (atomic_inc_return(&devfreq->suspend_count) > 1)
> +			return 0;
>  
> -	return devfreq->governor->event_handler(devfreq,
> -				DEVFREQ_GOV_SUSPEND, NULL);
> +		ret = devfreq_set_target(devfreq, devfreq->suspend_freq,
> +					 &prev_freq, flags);

Remove the 'prev_freq' parameter.

> +		if (ret)
> +			return ret;
> +
> +		devfreq->resume_freq = prev_freq;

As I commented on patch2, if devfreq_set_taget save the current frequency
to 'devfreq->resume_freq', this line is not needed.


> +	}
> +
> +        return 0;
>  }
>  EXPORT_SYMBOL(devfreq_suspend_device);
>  
> @@ -893,9 +912,23 @@ EXPORT_SYMBOL(devfreq_suspend_device);
>   */
>  int devfreq_resume_device(struct devfreq *devfreq)
>  {
> +        int ret;

Move 'ret' definition under 'if (devfreq->suspend_freq) {'
because 'ret' is used if suspend_freq isn't zero.

> +        unsigned long prev_freq;

Remove prev_freq variable which is not used on this function.
After calling devfreq_set_target, prev_freq is not used.

> +        u32 flags = 0;
> +
>  	if (!devfreq)
>  		return -EINVAL;
>  
> +	if (devfreq->suspend_freq) {
> +		if (atomic_dec_return(&devfreq->suspend_count) >= 1)
> +			return 0;
> +
> +		ret = devfreq_set_target(devfreq, devfreq->resume_freq,
> +					 &prev_freq, flags);

Remove the 'prev_freq' parameter.

> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (!devfreq->governor)
>  		return 0;
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 4/6] devfreq: add devfreq_suspend/resume() functions

[Chanwoo Choi]

Hi,

On 2018년 11월 22일 03:01, Lukasz Luba wrote:
> This patch adds implementation for global suspend/resume for
> devfreq framework. System suspend will next use these functions.
> 
> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
> solve issue with devfreq device's frequency during suspend/resume.
> During the discussion on LKML some corner cases and comments appeared
> related to the design. This patch address them keeping in mind suggestions
> from Chanwoo Choi.

Please remove the duplicate information about patch history.

> 
> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/devfreq.h   |  7 +++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 7e09de8..2f4391c 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -44,6 +44,9 @@ static LIST_HEAD(devfreq_governor_list);
>  static LIST_HEAD(devfreq_list);
>  static DEFINE_MUTEX(devfreq_list_lock);
>  
> +/* Flag showing state of suspend/resume */
> +static bool devfreq_suspended;

Is it necessary? This patch just saves the 'true' or 'false' to this variable.
And there are no any point to check the value of this variable.
Please remove it.

> +
>  /**
>   * find_device_devfreq() - find devfreq struct using device pointer
>   * @dev:	device pointer used to lookup device devfreq.
> @@ -938,6 +941,52 @@ int devfreq_resume_device(struct devfreq *devfreq)
>  EXPORT_SYMBOL(devfreq_resume_device);
>  
>  /**
> + * devfreq_suspend() - Suspend devfreq governors and devices
> + *
> + * Called during system wide Suspend/Hibernate cycles for suspending governors
> + * and devices preserving the state for resume. On some platforms the devfreq
> + * device must have precise state (frequency) after resume in order to provide
> + * fully operating setup.
> + */
> +void devfreq_suspend(void)
> +{
> +	struct devfreq *devfreq;
> +	int ret;
> +
> +	mutex_lock(&devfreq_list_lock);
> +	list_for_each_entry(devfreq, &devfreq_list, node) {
> +		ret = devfreq_suspend_device(devfreq);
> +		if (ret)
> +			dev_warn(&devfreq->dev, "device suspend failed\n");
> +	}
> +	mutex_unlock(&devfreq_list_lock);
> +
> +	devfreq_suspended = true;

Remove it.

> +}
> +
> +/**
> + * devfreq_resume() - Resume devfreq governors and devices
> + *
> + * Called during system wide Suspend/Hibernate cycle for resuming governors and
> + * devices that are suspended with devfreq_suspend().
> + */
> +void devfreq_resume(void)
> +{
> +	struct devfreq *devfreq;
> +	int ret;
> +
> +	devfreq_suspended = false;

Remove it.

> +
> +	mutex_lock(&devfreq_list_lock);
> +	list_for_each_entry(devfreq, &devfreq_list, node) {
> +		ret = devfreq_resume_device(devfreq);
> +		if (ret)
> +			dev_warn(&devfreq->dev, "device resume failed\n");
> +	}
> +	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 7fe96f9..4f0fea8 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -202,6 +202,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);
>  
> +

Remove the blank line.

> +extern void devfreq_suspend(void);
> +extern void devfreq_resume(void);
> +
>  /**
>   * update_devfreq() - Reevaluate the device and configure frequency
>   * @devfreq:	the devfreq device
> @@ -396,6 +400,9 @@ static inline int devfreq_update_stats(struct devfreq *df)
>  {
>  	return -EINVAL;
>  }
> +
> +static inline void devfreq_suspend(void) {}
> +static inline void devfreq_resume(void) {}

You better to move the inline definitions
under 'devfreq_resume_device' inline function.

>  #endif /* CONFIG_PM_DEVFREQ */
>  
>  #endif /* __LINUX_DEVFREQ_H__ */
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 5/6] drivers: power: suspend: call devfreq suspend/resume

[Chanwoo Choi]

Hi,

On 2018년 11월 22일 03:01, Lukasz Luba wrote:
> Devfreq framework supports suspend of its devices.
> Call the the devfreq interface and allow devfreq devices preserve/restore
> their states during suspend/resume.
> 
> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
> solve issue with devfreq device's frequency during suspend/resume.
> During the discussion on LKML some corner cases and comments appeared
> related to the design. This patch address them keeping in mind suggestions
> from Chanwoo Choi.

Please remove the duplicate information about patch history
because you already explained it on cover-letter.

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

> 
> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  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 a690fd4..0992e67 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"
> @@ -1078,6 +1079,7 @@ void dpm_resume(pm_message_t state)
>  	dpm_show_time(starttime, state, 0, NULL);
>  
>  	cpufreq_resume();
> +	devfreq_resume();
>  	trace_suspend_resume(TPS("dpm_resume"), state.event, false);
>  }
>  
> @@ -1852,6 +1854,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);
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 6/6] arm: dts: exynos4: set opp-suspend for DMC and leftbus

[Chanwoo Choi]

Hi,

On 2018년 11월 22일 03:01, Lukasz Luba wrote:
> Mark the state for devfreq device while entring suspend/resume process.
> 
> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
> solve issue with devfreq device's frequency during suspend/resume.
> During the discussion on LKML some corner cases and comments appeared
> related to the design. This patch address them keeping in mind suggestions
> from Chanwoo Choi.
> 
> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  arch/arm/boot/dts/exynos4210.dtsi | 2 ++
>  arch/arm/boot/dts/exynos4412.dtsi | 2 ++
>  2 files changed, 4 insertions(+)

Looks good to me.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

> 
> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> index b6091c2..4429b72 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -298,6 +298,7 @@
>  			opp-400000000 {
>  				opp-hz = /bits/ 64 <400000000>;
>  				opp-microvolt = <1150000>;
> +				opp-suspend;
>  			};
>  		};
>  
> @@ -367,6 +368,7 @@
>  			};
>  			opp-200000000 {
>  				opp-hz = /bits/ 64 <200000000>;
> +				opp-suspend;
>  			};
>  		};
>  	};
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> index 51f72f0..908c0c4 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -432,6 +432,7 @@
>  			opp-400000000 {
>  				opp-hz = /bits/ 64 <400000000>;
>  				opp-microvolt = <1050000>;
> +				opp-suspend;
>  			};
>  		};
>  
> @@ -520,6 +521,7 @@
>  			opp-200000000 {
>  				opp-hz = /bits/ 64 <200000000>;
>  				opp-microvolt = <1000000>;
> +				opp-suspend;
>  			};
>  		};
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/6] devfreq: refactor set_target frequency function

[Lukasz Luba]

Hi Chanwoo Choi

On 11/22/18 3:52 AM, Chanwoo Choi wrote:
> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>> The refactoring is needed for the new client in devfreq: suspend.
>> To avoid code duplication, move it to the new local function
>> devfreq_set_target.
>>
>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>> solve issue with devfreq device's frequency during suspend/resume.
>> During the discussion on LKML some corner cases and comments appeared
>> related to the design. This patch address them keeping in mind suggestions
>> from Chanwoo Choi.
> 
> As I commented on patch1, please remove it.
OK
> 
>>
>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/devfreq/devfreq.c | 62 ++++++++++++++++++++++++++++-------------------
>>   1 file changed, 37 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index e20e7e4..cf9c643 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -285,6 +285,42 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>   	return 0;
>>   }
>>   
>> +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>> +			      unsigned long *prev_freq, u32 flags)
> 
> Please remove the unused space in front of 'unsigned long *prev_freq'.
> Use tab only for indentation.
OK
> 
>> +{
>> +	struct devfreq_freqs freqs;
>> +	unsigned long cur_freq;
>> +	int err = 0;
>> +
>> +	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 = new_freq;
>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
>> +
>> +	err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags);
>> +	if (err) {
>> +		freqs.new = cur_freq;
>> +		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> +		return err;
>> +	}
>> +
>> +	freqs.new = new_freq;
>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> +
>> +	if (devfreq_update_status(devfreq, new_freq))
>> +		dev_err(&devfreq->dev,
>> +			"Couldn't update frequency transition information.\n");
>> +
>> +	devfreq->previous_freq = new_freq;
>> +	*prev_freq = cur_freq;
>> +
>> +	return err;
>> +}
>> +
>>   /* Load monitoring helper functions for governors use */
>>   
>>   /**
>> @@ -296,7 +332,6 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>    */
>>   int update_devfreq(struct devfreq *devfreq)
>>   {
>> -	struct devfreq_freqs freqs;
>>   	unsigned long freq, cur_freq, min_freq, max_freq;
> 
> 
> cur_freq is not used after modification. Remove it.
> 
>>   	int err = 0;
>>   	u32 flags = 0;
>> @@ -333,31 +368,8 @@ 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);
>> +	return devfreq_set_target(devfreq, freq, &cur_freq, flags);
> 
> You get the 'cur_freq' from devfreq_set_taget() for devfreq_suspend_device() on patch3.
> But, update_devfreq() and devfreq_resume_device() don't use the 'cur_freq' value
> from devfreq_set_target().
> 
> Instead, getting 'cur_freq' for 'devfreq->resume_freq' in the devfreq_set_target().
> Please remove the 'cur_freq' parameter from devfreq_set_target.

I can remove the 3rd parameter from devfreq_set_target(),
but it implies that patch 1 and 3 cannot be merged. The function 
devfreq_set_target will use 'devfreq->resume_freq' so it must be in 
devfreq struct.
So, in v2 version there will be still 6 patches, with the 1st patch 
defining needed fields in devfreq struct.
Do you agree for that?

> 
>>   
>> -	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>> -	if (err) {
>> -		freqs.new = cur_freq;
>> -		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> -		return err;
>> -	}
>> -
>> -	freqs.new = freq;
>> -	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>> -
>> -	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);
>>   
>>
> 
> 

Regards,
Lukasz Luba

Re: [PATCH 3/6] devfreq: add support for suspend/resume of a devfreq device

[Lukasz Luba]

On 11/22/18 3:58 AM, Chanwoo Choi wrote:
> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>> The patch adds support for handling suspend/resume process.
>> It uses atomic variables to make sure no race condition
>> affects the process.
>>
>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>> solve issue with devfreq device's frequency during suspend/resume.
>> During the discussion on LKML some corner cases and comments appeared
>> related to the design. This patch address them keeping in mind suggestions
>> from Chanwoo Choi.
> 
> Please remove the duplicate information about patch history.
> 
>>
>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/devfreq/devfreq.c | 45 +++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index cf9c643..7e09de8 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -872,14 +872,33 @@ EXPORT_SYMBOL(devm_devfreq_remove_device);
>>    */
>>   int devfreq_suspend_device(struct devfreq *devfreq)
>>   {
>> -	if (!devfreq)
>> -		return -EINVAL;
>> +        int ret;
> 
> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
> because 'ret' is used if suspend_freq isn't zero.
Not only there, 6 lines above 'ret' is used for 
devfreq->governor->event_handler().
> 
>> +        unsigned long prev_freq;
>> +        u32 flags = 0;
>> +
>> +        if (!devfreq)
>> +                return -EINVAL;
>> +
>> +        if (devfreq->governor) {
>> +		ret = devfreq->governor->event_handler(devfreq,
>> +					DEVFREQ_GOV_SUSPEND, NULL);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>> -	if (!devfreq->governor)
>> -		return 0;
>> +	if (devfreq->suspend_freq) {
>> +		if (atomic_inc_return(&devfreq->suspend_count) > 1)
>> +			return 0;
>>   
>> -	return devfreq->governor->event_handler(devfreq,
>> -				DEVFREQ_GOV_SUSPEND, NULL);
>> +		ret = devfreq_set_target(devfreq, devfreq->suspend_freq,
>> +					 &prev_freq, flags);
> 
> Remove the 'prev_freq' parameter.
OK
> 
>> +		if (ret)
>> +			return ret;
>> +
>> +		devfreq->resume_freq = prev_freq;
> 
> As I commented on patch2, if devfreq_set_taget save the current frequency
> to 'devfreq->resume_freq', this line is not needed.
OK
> 
> 
>> +	}
>> +
>> +        return 0;
>>   }
>>   EXPORT_SYMBOL(devfreq_suspend_device);
>>   
>> @@ -893,9 +912,23 @@ EXPORT_SYMBOL(devfreq_suspend_device);
>>    */
>>   int devfreq_resume_device(struct devfreq *devfreq)
>>   {
>> +        int ret;
> 
> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
> because 'ret' is used if suspend_freq isn't zero.
OK
> 
>> +        unsigned long prev_freq;
> 
> Remove prev_freq variable which is not used on this function.
> After calling devfreq_set_target, prev_freq is not used.
OK
> 
>> +        u32 flags = 0;
>> +
>>   	if (!devfreq)
>>   		return -EINVAL;
>>   
>> +	if (devfreq->suspend_freq) {
>> +		if (atomic_dec_return(&devfreq->suspend_count) >= 1)
>> +			return 0;
>> +
>> +		ret = devfreq_set_target(devfreq, devfreq->resume_freq,
>> +					 &prev_freq, flags);
> 
> Remove the 'prev_freq' parameter.
OK
> 
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	if (!devfreq->governor)
>>   		return 0;
>>   
>>
> 
> 

Regards,
Lukasz Luba

Re: [PATCH 4/6] devfreq: add devfreq_suspend/resume() functions

[Lukasz Luba]

On 11/22/18 4:07 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>> This patch adds implementation for global suspend/resume for
>> devfreq framework. System suspend will next use these functions.
>>
>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>> solve issue with devfreq device's frequency during suspend/resume.
>> During the discussion on LKML some corner cases and comments appeared
>> related to the design. This patch address them keeping in mind suggestions
>> from Chanwoo Choi.
> 
> Please remove the duplicate information about patch history.
> 
>>
>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/devfreq/devfreq.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/devfreq.h   |  7 +++++++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 7e09de8..2f4391c 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -44,6 +44,9 @@ static LIST_HEAD(devfreq_governor_list);
>>   static LIST_HEAD(devfreq_list);
>>   static DEFINE_MUTEX(devfreq_list_lock);
>>   
>> +/* Flag showing state of suspend/resume */
>> +static bool devfreq_suspended;
> 
> Is it necessary? This patch just saves the 'true' or 'false' to this variable.
> And there are no any point to check the value of this variable.
> Please remove it.
> 
>> +
>>   /**
>>    * find_device_devfreq() - find devfreq struct using device pointer
>>    * @dev:	device pointer used to lookup device devfreq.
>> @@ -938,6 +941,52 @@ int devfreq_resume_device(struct devfreq *devfreq)
>>   EXPORT_SYMBOL(devfreq_resume_device);
>>   
>>   /**
>> + * devfreq_suspend() - Suspend devfreq governors and devices
>> + *
>> + * Called during system wide Suspend/Hibernate cycles for suspending governors
>> + * and devices preserving the state for resume. On some platforms the devfreq
>> + * device must have precise state (frequency) after resume in order to provide
>> + * fully operating setup.
>> + */
>> +void devfreq_suspend(void)
>> +{
>> +	struct devfreq *devfreq;
>> +	int ret;
>> +
>> +	mutex_lock(&devfreq_list_lock);
>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>> +		ret = devfreq_suspend_device(devfreq);
>> +		if (ret)
>> +			dev_warn(&devfreq->dev, "device suspend failed\n");
>> +	}
>> +	mutex_unlock(&devfreq_list_lock);
>> +
>> +	devfreq_suspended = true;
> 
> Remove it.
> 
>> +}
>> +
>> +/**
>> + * devfreq_resume() - Resume devfreq governors and devices
>> + *
>> + * Called during system wide Suspend/Hibernate cycle for resuming governors and
>> + * devices that are suspended with devfreq_suspend().
>> + */
>> +void devfreq_resume(void)
>> +{
>> +	struct devfreq *devfreq;
>> +	int ret;
>> +
>> +	devfreq_suspended = false;
> 
> Remove it.
> 
>> +
>> +	mutex_lock(&devfreq_list_lock);
>> +	list_for_each_entry(devfreq, &devfreq_list, node) {
>> +		ret = devfreq_resume_device(devfreq);
>> +		if (ret)
>> +			dev_warn(&devfreq->dev, "device resume failed\n");
>> +	}
>> +	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 7fe96f9..4f0fea8 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -202,6 +202,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);
>>   
>> +
> 
> Remove the blank line.
> 
>> +extern void devfreq_suspend(void);
>> +extern void devfreq_resume(void);
>> +
>>   /**
>>    * update_devfreq() - Reevaluate the device and configure frequency
>>    * @devfreq:	the devfreq device
>> @@ -396,6 +400,9 @@ static inline int devfreq_update_stats(struct devfreq *df)
>>   {
>>   	return -EINVAL;
>>   }
>> +
>> +static inline void devfreq_suspend(void) {}
>> +static inline void devfreq_resume(void) {}
> 
> You better to move the inline definitions
> under 'devfreq_resume_device' inline function.
OK, I will move it there.
> 
>>   #endif /* CONFIG_PM_DEVFREQ */
>>   
>>   #endif /* __LINUX_DEVFREQ_H__ */
>>
> 
>

Regards,
Lukasz Luba

Re: [PATCH 6/6] arm: dts: exynos4: set opp-suspend for DMC and leftbus

[Lukasz Luba]

Hi Chanwoo Choi,

Thank you for the review of the patch set.
I will add your suggestions in v2 and add 'Reviewed-by' for the last to 
patches.

On 11/22/18 4:09 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>> Mark the state for devfreq device while entring suspend/resume process.
>>
>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>> solve issue with devfreq device's frequency during suspend/resume.
>> During the discussion on LKML some corner cases and comments appeared
>> related to the design. This patch address them keeping in mind suggestions
>> from Chanwoo Choi.
>>
>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   arch/arm/boot/dts/exynos4210.dtsi | 2 ++
>>   arch/arm/boot/dts/exynos4412.dtsi | 2 ++
>>   2 files changed, 4 insertions(+)
> 
> Looks good to me.
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> 
>>
>> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
>> index b6091c2..4429b72 100644
>> --- a/arch/arm/boot/dts/exynos4210.dtsi
>> +++ b/arch/arm/boot/dts/exynos4210.dtsi
>> @@ -298,6 +298,7 @@
>>   			opp-400000000 {
>>   				opp-hz = /bits/ 64 <400000000>;
>>   				opp-microvolt = <1150000>;
>> +				opp-suspend;
>>   			};
>>   		};
>>   
>> @@ -367,6 +368,7 @@
>>   			};
>>   			opp-200000000 {
>>   				opp-hz = /bits/ 64 <200000000>;
>> +				opp-suspend;
>>   			};
>>   		};
>>   	};
>> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
>> index 51f72f0..908c0c4 100644
>> --- a/arch/arm/boot/dts/exynos4412.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412.dtsi
>> @@ -432,6 +432,7 @@
>>   			opp-400000000 {
>>   				opp-hz = /bits/ 64 <400000000>;
>>   				opp-microvolt = <1050000>;
>> +				opp-suspend;
>>   			};
>>   		};
>>   
>> @@ -520,6 +521,7 @@
>>   			opp-200000000 {
>>   				opp-hz = /bits/ 64 <200000000>;
>>   				opp-microvolt = <1000000>;
>> +				opp-suspend;
>>   			};
>>   		};
>>   
>>
> 
> 

Regards,
Lukasz Luba

Re: [PATCH 0/6] devfreq: handle suspend/resume

[Tobias Jakobi]

Hey Lukasz,

just wanted to say hi and thanks for picking this up. Sadly my work no longer
permits me to spend time working on the kernel.

Anyway, great that this issue finally gets solved! :)

With best wishes,
Tobias


Lukasz Luba wrote:
> Hi all,
> 
> This patch set aims to address the issue with devfreq devices' frequency
> during suspend/resume. It extends suspend/resume by calls to Devfreq
> framework. In the devfreq framework there is a small refactoring to avoid
> code duplication in changging frequency (patch 2) and there are extensions
> for suspending devices.
> 
> It has been tested on Odroid u3 with Exynos 4412.
> 
> The patch set draws on Tobias Jakobi's work posted ~2 years ago, who tried
> to solve issue with devfreq device's frequency during suspend/resume.
> During the discussion on LKML some corner cases and comments appeared
> related to the design. This patch set address them keeping in mind
> suggestions from Chanwoo Choi.
> Tobias's paches:
> https://www.spinics.net/lists/linux-samsung-soc/msg56602.html 
> 
> Regards,
> Lukasz Luba
> 
> Lukasz Luba (6):
>   devfreq: add basic fileds supporting suspend functionality
>   devfreq: refactor set_target frequency function
>   devfreq: add support for suspend/resume of a devfreq device
>   devfreq: add devfreq_suspend/resume() functions
>   drivers: power: suspend: call devfreq suspend/resume
>   arm: dts: exynos4: set opp-suspend for DMC and leftbus
> 
>  arch/arm/boot/dts/exynos4210.dtsi |   2 +
>  arch/arm/boot/dts/exynos4412.dtsi |   2 +
>  drivers/base/power/main.c         |   3 +
>  drivers/devfreq/devfreq.c         | 159 ++++++++++++++++++++++++++++++--------
>  include/linux/devfreq.h           |  11 +++
>  5 files changed, 146 insertions(+), 31 deletions(-)
> 

Re: [PATCH 0/6] devfreq: handle suspend/resume

[Lukasz Luba]

Hi Tobias,

On 11/22/18 6:24 PM, Tobias Jakobi wrote:
> Hey Lukasz,
> 
> just wanted to say hi and thanks for picking this up. Sadly my work no longer
> permits me to spend time working on the kernel.
Fingers crossed for your current work and maybe for come back to kernel
development in the future!
> 
> Anyway, great that this issue finally gets solved! :)
Thank you for your idea and development of these patches in v1 and v2.
This functionality is really needed.

Regards,
Lukasz

> 
> With best wishes,
> Tobias
> 
> 
> Lukasz Luba wrote:
>> Hi all,
>>
>> This patch set aims to address the issue with devfreq devices' frequency
>> during suspend/resume. It extends suspend/resume by calls to Devfreq
>> framework. In the devfreq framework there is a small refactoring to avoid
>> code duplication in changging frequency (patch 2) and there are extensions
>> for suspending devices.
>>
>> It has been tested on Odroid u3 with Exynos 4412.
>>
>> The patch set draws on Tobias Jakobi's work posted ~2 years ago, who tried
>> to solve issue with devfreq device's frequency during suspend/resume.
>> During the discussion on LKML some corner cases and comments appeared
>> related to the design. This patch set address them keeping in mind
>> suggestions from Chanwoo Choi.
>> Tobias's paches:
>> https://www.spinics.net/lists/linux-samsung-soc/msg56602.html
>>
>> Regards,
>> Lukasz Luba
>>
>> Lukasz Luba (6):
>>    devfreq: add basic fileds supporting suspend functionality
>>    devfreq: refactor set_target frequency function
>>    devfreq: add support for suspend/resume of a devfreq device
>>    devfreq: add devfreq_suspend/resume() functions
>>    drivers: power: suspend: call devfreq suspend/resume
>>    arm: dts: exynos4: set opp-suspend for DMC and leftbus
>>
>>   arch/arm/boot/dts/exynos4210.dtsi |   2 +
>>   arch/arm/boot/dts/exynos4412.dtsi |   2 +
>>   drivers/base/power/main.c         |   3 +
>>   drivers/devfreq/devfreq.c         | 159 ++++++++++++++++++++++++++++++--------
>>   include/linux/devfreq.h           |  11 +++
>>   5 files changed, 146 insertions(+), 31 deletions(-)
>>
> 
> 
> 

Re: [PATCH 2/6] devfreq: refactor set_target frequency function

[Chanwoo Choi]

Hi Lukasz,

On 2018년 11월 22일 19:40, Lukasz Luba wrote:
> Hi Chanwoo Choi
> 
> On 11/22/18 3:52 AM, Chanwoo Choi wrote:
>> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>>> The refactoring is needed for the new client in devfreq: suspend.
>>> To avoid code duplication, move it to the new local function
>>> devfreq_set_target.
>>>
>>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>>> solve issue with devfreq device's frequency during suspend/resume.
>>> During the discussion on LKML some corner cases and comments appeared
>>> related to the design. This patch address them keeping in mind suggestions
>>> from Chanwoo Choi.
>>
>> As I commented on patch1, please remove it.
> OK
>>
>>>
>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>> ---
>>>   drivers/devfreq/devfreq.c | 62 ++++++++++++++++++++++++++++-------------------
>>>   1 file changed, 37 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index e20e7e4..cf9c643 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -285,6 +285,42 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>>   	return 0;
>>>   }
>>>   
>>> +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>>> +			      unsigned long *prev_freq, u32 flags)
>>
>> Please remove the unused space in front of 'unsigned long *prev_freq'.
>> Use tab only for indentation.
> OK
>>
>>> +{
>>> +	struct devfreq_freqs freqs;
>>> +	unsigned long cur_freq;
>>> +	int err = 0;
>>> +
>>> +	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 = new_freq;
>>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
>>> +
>>> +	err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags);
>>> +	if (err) {
>>> +		freqs.new = cur_freq;
>>> +		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>> +		return err;
>>> +	}
>>> +
>>> +	freqs.new = new_freq;
>>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>> +
>>> +	if (devfreq_update_status(devfreq, new_freq))
>>> +		dev_err(&devfreq->dev,
>>> +			"Couldn't update frequency transition information.\n");
>>> +
>>> +	devfreq->previous_freq = new_freq;
>>> +	*prev_freq = cur_freq;
>>> +
>>> +	return err;
>>> +}
>>> +
>>>   /* Load monitoring helper functions for governors use */
>>>   
>>>   /**
>>> @@ -296,7 +332,6 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>>    */
>>>   int update_devfreq(struct devfreq *devfreq)
>>>   {
>>> -	struct devfreq_freqs freqs;
>>>   	unsigned long freq, cur_freq, min_freq, max_freq;
>>
>>
>> cur_freq is not used after modification. Remove it.
>>
>>>   	int err = 0;
>>>   	u32 flags = 0;
>>> @@ -333,31 +368,8 @@ 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);
>>> +	return devfreq_set_target(devfreq, freq, &cur_freq, flags);
>>
>> You get the 'cur_freq' from devfreq_set_taget() for devfreq_suspend_device() on patch3.
>> But, update_devfreq() and devfreq_resume_device() don't use the 'cur_freq' value
>> from devfreq_set_target().
>>
>> Instead, getting 'cur_freq' for 'devfreq->resume_freq' in the devfreq_set_target().
>> Please remove the 'cur_freq' parameter from devfreq_set_target.
> 
> I can remove the 3rd parameter from devfreq_set_target(),
> but it implies that patch 1 and 3 cannot be merged. The function 
> devfreq_set_target will use 'devfreq->resume_freq' so it must be in 
> devfreq struct.
> So, in v2 version there will be still 6 patches, with the 1st patch 
> defining needed fields in devfreq struct.
> Do you agree for that?

So, I replied again as following on my other reply[1] of patch2: 
[1] https://lkml.org/lkml/2018/11/22/507

But, you have to add following new code on patch3 instead of patch2.
patch2 doesn't contain the following codes and then
patch3 adds 'resume_freq' variable and adds the following code on patch3.

+
+       if (devfreq->suspend_freq)
+               devfreq->resume_freq = cur_freq;

> 
>>
>>>   
>>> -	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>>> -	if (err) {
>>> -		freqs.new = cur_freq;
>>> -		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>> -		return err;
>>> -	}
>>> -
>>> -	freqs.new = freq;
>>> -	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>> -
>>> -	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);
>>>   
>>>
>>
>>
> 
> Regards,
> Lukasz Luba
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 3/6] devfreq: add support for suspend/resume of a devfreq device

[Chanwoo Choi]

Hi Lukasz,

I add one more comment about devfreq_resume_device().

On 2018년 11월 22일 20:00, Lukasz Luba wrote:
> 
> 
> On 11/22/18 3:58 AM, Chanwoo Choi wrote:
>> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>>> The patch adds support for handling suspend/resume process.
>>> It uses atomic variables to make sure no race condition
>>> affects the process.
>>>
>>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>>> solve issue with devfreq device's frequency during suspend/resume.
>>> During the discussion on LKML some corner cases and comments appeared
>>> related to the design. This patch address them keeping in mind suggestions
>>> from Chanwoo Choi.
>>
>> Please remove the duplicate information about patch history.
>>
>>>
>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>> ---
>>>   drivers/devfreq/devfreq.c | 45 +++++++++++++++++++++++++++++++++++++++------
>>>   1 file changed, 39 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index cf9c643..7e09de8 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -872,14 +872,33 @@ EXPORT_SYMBOL(devm_devfreq_remove_device);
>>>    */
>>>   int devfreq_suspend_device(struct devfreq *devfreq)
>>>   {
>>> -	if (!devfreq)
>>> -		return -EINVAL;
>>> +        int ret;
>>
>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>> because 'ret' is used if suspend_freq isn't zero.
> Not only there, 6 lines above 'ret' is used for 
> devfreq->governor->event_handler().

OK.

>>
>>> +        unsigned long prev_freq;
>>> +        u32 flags = 0;
>>> +
>>> +        if (!devfreq)
>>> +                return -EINVAL;
>>> +
>>> +        if (devfreq->governor) {
>>> +		ret = devfreq->governor->event_handler(devfreq,
>>> +					DEVFREQ_GOV_SUSPEND, NULL);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>>   
>>> -	if (!devfreq->governor)
>>> -		return 0;
>>> +	if (devfreq->suspend_freq) {
>>> +		if (atomic_inc_return(&devfreq->suspend_count) > 1)
>>> +			return 0;
>>>   
>>> -	return devfreq->governor->event_handler(devfreq,
>>> -				DEVFREQ_GOV_SUSPEND, NULL);
>>> +		ret = devfreq_set_target(devfreq, devfreq->suspend_freq,
>>> +					 &prev_freq, flags);
>>
>> Remove the 'prev_freq' parameter.
> OK
>>
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		devfreq->resume_freq = prev_freq;
>>
>> As I commented on patch2, if devfreq_set_taget save the current frequency
>> to 'devfreq->resume_freq', this line is not needed.
> OK
>>
>>
>>> +	}
>>> +
>>> +        return 0;
>>>   }
>>>   EXPORT_SYMBOL(devfreq_suspend_device);
>>>   
>>> @@ -893,9 +912,23 @@ EXPORT_SYMBOL(devfreq_suspend_device);
>>>    */
>>>   int devfreq_resume_device(struct devfreq *devfreq)
>>>   {
>>> +        int ret;
>>
>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>> because 'ret' is used if suspend_freq isn't zero.
> OK

If you change the devfreq_resume_device() according to my new comment,
please ignore it.

>>
>>> +        unsigned long prev_freq;
>>
>> Remove prev_freq variable which is not used on this function.
>> After calling devfreq_set_target, prev_freq is not used.
> OK
>>
>>> +        u32 flags = 0;
>>> +
>>>   	if (!devfreq)
>>>   		return -EINVAL;
>>>   
>>> +	if (devfreq->suspend_freq) {
>>> +		if (atomic_dec_return(&devfreq->suspend_count) >= 1)
>>> +			return 0;
>>> +
>>> +		ret = devfreq_set_target(devfreq, devfreq->resume_freq,
>>> +					 &prev_freq, flags);
>>
>> Remove the 'prev_freq' parameter.
> OK
>>
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>   	if (!devfreq->governor)
>>>   		return 0;

Please change the code as following because you uses the following type
in the devfreq_suspend_device(). If there is any special issue, please
keep the same format in order to improve the readability. 

	if (devfreq->governor) {                                                
		ret = devfreq->governor->event_handler(devfreq,                 
				DEVFREQ_GOV_RESUME, NULL);             
		if (ret)                                                        
			return ret;                                             
	}        


>>>   
>>>
>>
>>
> 
> Regards,
> Lukasz Luba
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/6] devfreq: refactor set_target frequency function

[Lukasz Luba]

Hi Chanwoo Choi,

On 11/23/18 12:45 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> On 2018년 11월 22일 19:40, Lukasz Luba wrote:
>> Hi Chanwoo Choi
>>
>> On 11/22/18 3:52 AM, Chanwoo Choi wrote:
>>> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>>>> The refactoring is needed for the new client in devfreq: suspend.
>>>> To avoid code duplication, move it to the new local function
>>>> devfreq_set_target.
>>>>
>>>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>>>> solve issue with devfreq device's frequency during suspend/resume.
>>>> During the discussion on LKML some corner cases and comments appeared
>>>> related to the design. This patch address them keeping in mind suggestions
>>>> from Chanwoo Choi.
>>>
>>> As I commented on patch1, please remove it.
>> OK
>>>
>>>>
>>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 62 ++++++++++++++++++++++++++++-------------------
>>>>    1 file changed, 37 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index e20e7e4..cf9c643 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -285,6 +285,42 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static int devfreq_set_target(struct devfreq *devfreq, unsigned long new_freq,
>>>> +			      unsigned long *prev_freq, u32 flags)
>>>
>>> Please remove the unused space in front of 'unsigned long *prev_freq'.
>>> Use tab only for indentation.
>> OK
>>>
>>>> +{
>>>> +	struct devfreq_freqs freqs;
>>>> +	unsigned long cur_freq;
>>>> +	int err = 0;
>>>> +
>>>> +	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 = new_freq;
>>>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_PRECHANGE);
>>>> +
>>>> +	err = devfreq->profile->target(devfreq->dev.parent, &new_freq, flags);
>>>> +	if (err) {
>>>> +		freqs.new = cur_freq;
>>>> +		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	freqs.new = new_freq;
>>>> +	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>> +
>>>> +	if (devfreq_update_status(devfreq, new_freq))
>>>> +		dev_err(&devfreq->dev,
>>>> +			"Couldn't update frequency transition information.\n");
>>>> +
>>>> +	devfreq->previous_freq = new_freq;
>>>> +	*prev_freq = cur_freq;
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>>    /* Load monitoring helper functions for governors use */
>>>>    
>>>>    /**
>>>> @@ -296,7 +332,6 @@ static int devfreq_notify_transition(struct devfreq *devfreq,
>>>>     */
>>>>    int update_devfreq(struct devfreq *devfreq)
>>>>    {
>>>> -	struct devfreq_freqs freqs;
>>>>    	unsigned long freq, cur_freq, min_freq, max_freq;
>>>
>>>
>>> cur_freq is not used after modification. Remove it.
>>>
>>>>    	int err = 0;
>>>>    	u32 flags = 0;
>>>> @@ -333,31 +368,8 @@ 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);
>>>> +	return devfreq_set_target(devfreq, freq, &cur_freq, flags);
>>>
>>> You get the 'cur_freq' from devfreq_set_taget() for devfreq_suspend_device() on patch3.
>>> But, update_devfreq() and devfreq_resume_device() don't use the 'cur_freq' value
>>> from devfreq_set_target().
>>>
>>> Instead, getting 'cur_freq' for 'devfreq->resume_freq' in the devfreq_set_target().
>>> Please remove the 'cur_freq' parameter from devfreq_set_target.
>>
>> I can remove the 3rd parameter from devfreq_set_target(),
>> but it implies that patch 1 and 3 cannot be merged. The function
>> devfreq_set_target will use 'devfreq->resume_freq' so it must be in
>> devfreq struct.
>> So, in v2 version there will be still 6 patches, with the 1st patch
>> defining needed fields in devfreq struct.
>> Do you agree for that?
> 
> So, I replied again as following on my other reply[1] of patch2:
> [1] https://lkml.org/lkml/2018/11/22/507
> 
> But, you have to add following new code on patch3 instead of patch2.
> patch2 doesn't contain the following codes and then
> patch3 adds 'resume_freq' variable and adds the following code on patch3.
> 
> +
> +       if (devfreq->suspend_freq)
> +               devfreq->resume_freq = cur_freq;
> 
OK, I will add that change to patch 3.

Regards,
Lukasz
>>
>>>
>>>>    
>>>> -	err = devfreq->profile->target(devfreq->dev.parent, &freq, flags);
>>>> -	if (err) {
>>>> -		freqs.new = cur_freq;
>>>> -		devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>> -		return err;
>>>> -	}
>>>> -
>>>> -	freqs.new = freq;
>>>> -	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);
>>>> -
>>>> -	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);
>>>>    
>>>>
>>>
>>>
>>
>> Regards,
>> Lukasz Luba
>>
>>
> 
> 

Re: [PATCH 3/6] devfreq: add support for suspend/resume of a devfreq device

[Lukasz Luba]

Hi Chanwoo Choi,

On 11/23/18 12:54 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> I add one more comment about devfreq_resume_device().
> 
> On 2018년 11월 22일 20:00, Lukasz Luba wrote:
>>
>>
>> On 11/22/18 3:58 AM, Chanwoo Choi wrote:
>>> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>>>> The patch adds support for handling suspend/resume process.
>>>> It uses atomic variables to make sure no race condition
>>>> affects the process.
>>>>
>>>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>>>> solve issue with devfreq device's frequency during suspend/resume.
>>>> During the discussion on LKML some corner cases and comments appeared
>>>> related to the design. This patch address them keeping in mind suggestions
>>>> from Chanwoo Choi.
>>>
>>> Please remove the duplicate information about patch history.
>>>
>>>>
>>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 45 +++++++++++++++++++++++++++++++++++++++------
>>>>    1 file changed, 39 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index cf9c643..7e09de8 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -872,14 +872,33 @@ EXPORT_SYMBOL(devm_devfreq_remove_device);
>>>>     */
>>>>    int devfreq_suspend_device(struct devfreq *devfreq)
>>>>    {
>>>> -	if (!devfreq)
>>>> -		return -EINVAL;
>>>> +        int ret;
>>>
>>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>>> because 'ret' is used if suspend_freq isn't zero.
>> Not only there, 6 lines above 'ret' is used for
>> devfreq->governor->event_handler().
> 
> OK.
> 
>>>
>>>> +        unsigned long prev_freq;
>>>> +        u32 flags = 0;
>>>> +
>>>> +        if (!devfreq)
>>>> +                return -EINVAL;
>>>> +
>>>> +        if (devfreq->governor) {
>>>> +		ret = devfreq->governor->event_handler(devfreq,
>>>> +					DEVFREQ_GOV_SUSPEND, NULL);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>>    
>>>> -	if (!devfreq->governor)
>>>> -		return 0;
>>>> +	if (devfreq->suspend_freq) {
>>>> +		if (atomic_inc_return(&devfreq->suspend_count) > 1)
>>>> +			return 0;
>>>>    
>>>> -	return devfreq->governor->event_handler(devfreq,
>>>> -				DEVFREQ_GOV_SUSPEND, NULL);
>>>> +		ret = devfreq_set_target(devfreq, devfreq->suspend_freq,
>>>> +					 &prev_freq, flags);
>>>
>>> Remove the 'prev_freq' parameter.
>> OK
>>>
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		devfreq->resume_freq = prev_freq;
>>>
>>> As I commented on patch2, if devfreq_set_taget save the current frequency
>>> to 'devfreq->resume_freq', this line is not needed.
>> OK
>>>
>>>
>>>> +	}
>>>> +
>>>> +        return 0;
>>>>    }
>>>>    EXPORT_SYMBOL(devfreq_suspend_device);
>>>>    
>>>> @@ -893,9 +912,23 @@ EXPORT_SYMBOL(devfreq_suspend_device);
>>>>     */
>>>>    int devfreq_resume_device(struct devfreq *devfreq)
>>>>    {
>>>> +        int ret;
>>>
>>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>>> because 'ret' is used if suspend_freq isn't zero.
>> OK
> 
> If you change the devfreq_resume_device() according to my new comment,
> please ignore it.
> 
>>>
>>>> +        unsigned long prev_freq;
>>>
>>> Remove prev_freq variable which is not used on this function.
>>> After calling devfreq_set_target, prev_freq is not used.
>> OK
>>>
>>>> +        u32 flags = 0;
>>>> +
>>>>    	if (!devfreq)
>>>>    		return -EINVAL;
>>>>    
>>>> +	if (devfreq->suspend_freq) {
>>>> +		if (atomic_dec_return(&devfreq->suspend_count) >= 1)
>>>> +			return 0;
>>>> +
>>>> +		ret = devfreq_set_target(devfreq, devfreq->resume_freq,
>>>> +					 &prev_freq, flags);
>>>
>>> Remove the 'prev_freq' parameter.
>> OK
>>>
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>    	if (!devfreq->governor)
>>>>    		return 0;
> 
> Please change the code as following because you uses the following type
> in the devfreq_suspend_device(). If there is any special issue, please
> keep the same format in order to improve the readability.
> 
> 	if (devfreq->governor) {
> 		ret = devfreq->governor->event_handler(devfreq,
> 				DEVFREQ_GOV_RESUME, NULL);
> 		if (ret)
> 			return ret;
> 	}
> 
> 
OK, I will change the code to keep the same format.
Thank you for the review.

Regards,
Lukasz

Re: [PATCH 3/6] devfreq: add support for suspend/resume of a devfreq device

[Chanwoo Choi]

Hi Lukasz,

I add the one more comment for devfreq_resume_device()

On 2018년 11월 23일 19:01, Lukasz Luba wrote:
> Hi Chanwoo Choi,
> 
> On 11/23/18 12:54 AM, Chanwoo Choi wrote:
>> Hi Lukasz,
>>
>> I add one more comment about devfreq_resume_device().
>>
>> On 2018년 11월 22일 20:00, Lukasz Luba wrote:
>>>
>>>
>>> On 11/22/18 3:58 AM, Chanwoo Choi wrote:
>>>> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>>>>> The patch adds support for handling suspend/resume process.
>>>>> It uses atomic variables to make sure no race condition
>>>>> affects the process.
>>>>>
>>>>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>>>>> solve issue with devfreq device's frequency during suspend/resume.
>>>>> During the discussion on LKML some corner cases and comments appeared
>>>>> related to the design. This patch address them keeping in mind suggestions
>>>>> from Chanwoo Choi.
>>>>
>>>> Please remove the duplicate information about patch history.
>>>>
>>>>>
>>>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>>> ---
>>>>>    drivers/devfreq/devfreq.c | 45 +++++++++++++++++++++++++++++++++++++++------
>>>>>    1 file changed, 39 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index cf9c643..7e09de8 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -872,14 +872,33 @@ EXPORT_SYMBOL(devm_devfreq_remove_device);
>>>>>     */
>>>>>    int devfreq_suspend_device(struct devfreq *devfreq)
>>>>>    {
>>>>> -	if (!devfreq)
>>>>> -		return -EINVAL;
>>>>> +        int ret;
>>>>
>>>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>>>> because 'ret' is used if suspend_freq isn't zero.
>>> Not only there, 6 lines above 'ret' is used for
>>> devfreq->governor->event_handler().
>>
>> OK.
>>
>>>>
>>>>> +        unsigned long prev_freq;
>>>>> +        u32 flags = 0;
>>>>> +
>>>>> +        if (!devfreq)
>>>>> +                return -EINVAL;
>>>>> +
>>>>> +        if (devfreq->governor) {
>>>>> +		ret = devfreq->governor->event_handler(devfreq,
>>>>> +					DEVFREQ_GOV_SUSPEND, NULL);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>>    
>>>>> -	if (!devfreq->governor)
>>>>> -		return 0;
>>>>> +	if (devfreq->suspend_freq) {
>>>>> +		if (atomic_inc_return(&devfreq->suspend_count) > 1)
>>>>> +			return 0;
>>>>>    
>>>>> -	return devfreq->governor->event_handler(devfreq,
>>>>> -				DEVFREQ_GOV_SUSPEND, NULL);
>>>>> +		ret = devfreq_set_target(devfreq, devfreq->suspend_freq,
>>>>> +					 &prev_freq, flags);
>>>>
>>>> Remove the 'prev_freq' parameter.
>>> OK
>>>>
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +
>>>>> +		devfreq->resume_freq = prev_freq;
>>>>
>>>> As I commented on patch2, if devfreq_set_taget save the current frequency
>>>> to 'devfreq->resume_freq', this line is not needed.
>>> OK
>>>>
>>>>
>>>>> +	}
>>>>> +
>>>>> +        return 0;
>>>>>    }
>>>>>    EXPORT_SYMBOL(devfreq_suspend_device);
>>>>>    
>>>>> @@ -893,9 +912,23 @@ EXPORT_SYMBOL(devfreq_suspend_device);
>>>>>     */
>>>>>    int devfreq_resume_device(struct devfreq *devfreq)
>>>>>    {
>>>>> +        int ret;
>>>>
>>>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>>>> because 'ret' is used if suspend_freq isn't zero.
>>> OK
>>
>> If you change the devfreq_resume_device() according to my new comment,
>> please ignore it.
>>
>>>>
>>>>> +        unsigned long prev_freq;
>>>>
>>>> Remove prev_freq variable which is not used on this function.
>>>> After calling devfreq_set_target, prev_freq is not used.
>>> OK
>>>>
>>>>> +        u32 flags = 0;
>>>>> +
>>>>>    	if (!devfreq)
>>>>>    		return -EINVAL;
>>>>>    
>>>>> +	if (devfreq->suspend_freq) {

In devfreq_resume_device(), it should check the 'devfreq->resume_freq'
instead of 'devfreq->suspend_freq'.


>>>>> +		if (atomic_dec_return(&devfreq->suspend_count) >= 1)
>>>>> +			return 0;
>>>>> +
>>>>> +		ret = devfreq_set_target(devfreq, devfreq->resume_freq,
>>>>> +					 &prev_freq, flags);
>>>>
>>>> Remove the 'prev_freq' parameter.
>>> OK
>>>>
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>> +
>>>>>    	if (!devfreq->governor)
>>>>>    		return 0;
>>
>> Please change the code as following because you uses the following type
>> in the devfreq_suspend_device(). If there is any special issue, please
>> keep the same format in order to improve the readability.
>>
>> 	if (devfreq->governor) {
>> 		ret = devfreq->governor->event_handler(devfreq,
>> 				DEVFREQ_GOV_RESUME, NULL);
>> 		if (ret)
>> 			return ret;
>> 	}
>>
>>
> OK, I will change the code to keep the same format.
> Thank you for the review.
> 
> Regards,
> Lukasz
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

RE: [PATCH 1/6] devfreq: add basic fileds supporting suspend functionality

[MyungJoo Ham]

>The patch prepares devfreq device for handling suspend/resume functionality.
>The new fields will store needed information during this process.
>Devfreq framework handles opp-suspend DT entry and there is no need of
>modyfications in the drivers code.
>
>The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>solve issue with devfreq device's frequency during suspend/resume.
>During the discussion on LKML some corner cases and comments appeared
>related to the design. This patch address them keeping in mind suggestions
>from Chanwoo Choi.
>
>Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>

When you add new elements in a common struct (i.e., struct devfreq),
please describe kindly in the doxygen entries so that developers
may understand before reading all places where the new elements are
used.

You have added three new elements and there is no explanations on them.


Cheers,
MyungJoo

Re: [PATCH 1/6] devfreq: add basic fileds supporting suspend functionality

[Lukasz Luba]

Hi MyungJoo,

On 11/26/18 9:14 AM, MyungJoo Ham wrote:
>> The patch prepares devfreq device for handling suspend/resume functionality.
>> The new fields will store needed information during this process.
>> Devfreq framework handles opp-suspend DT entry and there is no need of
>> modyfications in the drivers code.
>>
>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>> solve issue with devfreq device's frequency during suspend/resume.
>> During the discussion on LKML some corner cases and comments appeared
>> related to the design. This patch address them keeping in mind suggestions
>>from Chanwoo Choi.
>>
>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> 
> When you add new elements in a common struct (i.e., struct devfreq),
> please describe kindly in the doxygen entries so that developers
> may understand before reading all places where the new elements are
> used.
> 
> You have added three new elements and there is no explanations on them.
You are right, thank you for the review.
I will fix it in the next patch set version.

Regards,
Lukasz

> 
> 
> Cheers,
> MyungJoo
> 
> 
> 

Re: [PATCH 3/6] devfreq: add support for suspend/resume of a devfreq device

[Lukasz Luba]

Hi Chanwoo Choi,

On 11/26/18 1:19 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> I add the one more comment for devfreq_resume_device()
> 
> On 2018년 11월 23일 19:01, Lukasz Luba wrote:
>> Hi Chanwoo Choi,
>>
>> On 11/23/18 12:54 AM, Chanwoo Choi wrote:
>>> Hi Lukasz,
>>>
>>> I add one more comment about devfreq_resume_device().
>>>
>>> On 2018년 11월 22일 20:00, Lukasz Luba wrote:
>>>>
>>>>
>>>> On 11/22/18 3:58 AM, Chanwoo Choi wrote:
>>>>> On 2018년 11월 22일 03:01, Lukasz Luba wrote:
>>>>>> The patch adds support for handling suspend/resume process.
>>>>>> It uses atomic variables to make sure no race condition
>>>>>> affects the process.
>>>>>>
>>>>>> The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>>>>>> solve issue with devfreq device's frequency during suspend/resume.
>>>>>> During the discussion on LKML some corner cases and comments appeared
>>>>>> related to the design. This patch address them keeping in mind suggestions
>>>>>> from Chanwoo Choi.
>>>>>
>>>>> Please remove the duplicate information about patch history.
>>>>>
>>>>>>
>>>>>> Suggested-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>>>> Suggested-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>>>> ---
>>>>>>     drivers/devfreq/devfreq.c | 45 +++++++++++++++++++++++++++++++++++++++------
>>>>>>     1 file changed, 39 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>> index cf9c643..7e09de8 100644
>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>> @@ -872,14 +872,33 @@ EXPORT_SYMBOL(devm_devfreq_remove_device);
>>>>>>      */
>>>>>>     int devfreq_suspend_device(struct devfreq *devfreq)
>>>>>>     {
>>>>>> -	if (!devfreq)
>>>>>> -		return -EINVAL;
>>>>>> +        int ret;
>>>>>
>>>>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>>>>> because 'ret' is used if suspend_freq isn't zero.
>>>> Not only there, 6 lines above 'ret' is used for
>>>> devfreq->governor->event_handler().
>>>
>>> OK.
>>>
>>>>>
>>>>>> +        unsigned long prev_freq;
>>>>>> +        u32 flags = 0;
>>>>>> +
>>>>>> +        if (!devfreq)
>>>>>> +                return -EINVAL;
>>>>>> +
>>>>>> +        if (devfreq->governor) {
>>>>>> +		ret = devfreq->governor->event_handler(devfreq,
>>>>>> +					DEVFREQ_GOV_SUSPEND, NULL);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>>     
>>>>>> -	if (!devfreq->governor)
>>>>>> -		return 0;
>>>>>> +	if (devfreq->suspend_freq) {
>>>>>> +		if (atomic_inc_return(&devfreq->suspend_count) > 1)
>>>>>> +			return 0;
>>>>>>     
>>>>>> -	return devfreq->governor->event_handler(devfreq,
>>>>>> -				DEVFREQ_GOV_SUSPEND, NULL);
>>>>>> +		ret = devfreq_set_target(devfreq, devfreq->suspend_freq,
>>>>>> +					 &prev_freq, flags);
>>>>>
>>>>> Remove the 'prev_freq' parameter.
>>>> OK
>>>>>
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +
>>>>>> +		devfreq->resume_freq = prev_freq;
>>>>>
>>>>> As I commented on patch2, if devfreq_set_taget save the current frequency
>>>>> to 'devfreq->resume_freq', this line is not needed.
>>>> OK
>>>>>
>>>>>
>>>>>> +	}
>>>>>> +
>>>>>> +        return 0;
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(devfreq_suspend_device);
>>>>>>     
>>>>>> @@ -893,9 +912,23 @@ EXPORT_SYMBOL(devfreq_suspend_device);
>>>>>>      */
>>>>>>     int devfreq_resume_device(struct devfreq *devfreq)
>>>>>>     {
>>>>>> +        int ret;
>>>>>
>>>>> Move 'ret' definition under 'if (devfreq->suspend_freq) {'
>>>>> because 'ret' is used if suspend_freq isn't zero.
>>>> OK
>>>
>>> If you change the devfreq_resume_device() according to my new comment,
>>> please ignore it.
>>>
>>>>>
>>>>>> +        unsigned long prev_freq;
>>>>>
>>>>> Remove prev_freq variable which is not used on this function.
>>>>> After calling devfreq_set_target, prev_freq is not used.
>>>> OK
>>>>>
>>>>>> +        u32 flags = 0;
>>>>>> +
>>>>>>     	if (!devfreq)
>>>>>>     		return -EINVAL;
>>>>>>     
>>>>>> +	if (devfreq->suspend_freq) {
> 
> In devfreq_resume_device(), it should check the 'devfreq->resume_freq'
> instead of 'devfreq->suspend_freq'.
OK, I will fix it.

Regards,
Lukasz

> 
> 
>>>>>> +		if (atomic_dec_return(&devfreq->suspend_count) >= 1)
>>>>>> +			return 0;
>>>>>> +
>>>>>> +		ret = devfreq_set_target(devfreq, devfreq->resume_freq,
>>>>>> +					 &prev_freq, flags);
>>>>>
>>>>> Remove the 'prev_freq' parameter.
>>>> OK
>>>>>
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +
>>>>>>     	if (!devfreq->governor)
>>>>>>     		return 0;
>>>
>>> Please change the code as following because you uses the following type
>>> in the devfreq_suspend_device(). If there is any special issue, please
>>> keep the same format in order to improve the readability.
>>>
>>> 	if (devfreq->governor) {
>>> 		ret = devfreq->governor->event_handler(devfreq,
>>> 				DEVFREQ_GOV_RESUME, NULL);
>>> 		if (ret)
>>> 			return ret;
>>> 	}
>>>
>>>
>> OK, I will change the code to keep the same format.
>> Thank you for the review.
>>
>> Regards,
>> Lukasz
>>
>>
>>
> 
>