2018-11-21 19:13:19

by Lukasz Luba

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

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

2018-11-21 19:13:53

by Lukasz Luba

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

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 <[email protected]>
Suggested-by: Chanwoo Choi <[email protected]>
Signed-off-by: Lukasz Luba <[email protected]>
---
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

2018-11-21 20:20:03

by Lukasz Luba

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

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 <[email protected]>
Suggested-by: Chanwoo Choi <[email protected]>
Signed-off-by: Lukasz Luba <[email protected]>
---
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

2018-11-21 20:20:03

by Lukasz Luba

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

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 <[email protected]>
Suggested-by: Chanwoo Choi <[email protected]>
Signed-off-by: Lukasz Luba <[email protected]>
---
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

2018-11-21 20:29:08

by Lukasz Luba

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

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 <[email protected]>
Suggested-by: Chanwoo Choi <[email protected]>
Signed-off-by: Lukasz Luba <[email protected]>
---
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

2018-11-21 20:29:11

by Lukasz Luba

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

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 <[email protected]>
Suggested-by: Chanwoo Choi <[email protected]>
Signed-off-by: Lukasz Luba <[email protected]>
---
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

2018-11-21 20:29:19

by Lukasz Luba

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

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 <[email protected]>
Signed-off-by: Lukasz Luba <[email protected]>
---
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

2018-11-22 14:05:56

by Chanwoo Choi

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

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 <[email protected]>
> Suggested-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> 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

2018-11-22 14:06:14

by Chanwoo Choi

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

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 <[email protected]>
> Suggested-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> 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

2018-11-22 14:06:18

by Chanwoo Choi

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

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 <[email protected]>
>> Suggested-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> 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

2018-11-22 14:06:21

by Chanwoo Choi

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

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 <[email protected]>
> Suggested-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> 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

2018-11-22 14:06:56

by Chanwoo Choi

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

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 <[email protected]>
> Suggested-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> 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 <[email protected]>

>
> 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

2018-11-22 15:51:36

by Chanwoo Choi

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

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 <[email protected]>

>
> Suggested-by: Tobias Jakobi <[email protected]>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> 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

2018-11-22 15:51:50

by Chanwoo Choi

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

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 <[email protected]>
> Suggested-by: Chanwoo Choi <[email protected]>
> Signed-off-by: Lukasz Luba <[email protected]>
> ---
> 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

2018-11-23 11:27:45

by Lukasz Luba

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

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 <[email protected]>
>> Suggested-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> 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

2018-11-23 12:03:36

by Lukasz Luba

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

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 <[email protected]>
>> Suggested-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> 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

2018-11-23 12:03:36

by Lukasz Luba

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

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 <[email protected]>
>> Suggested-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> 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

2018-11-23 12:04:14

by Lukasz Luba

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

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 <[email protected]>
>> Suggested-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Lukasz Luba <[email protected]>
>> ---
>> 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 <[email protected]>
>
>>
>> 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

2018-11-24 07:06:53

by Tobias Jakobi

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

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(-)
>

2018-11-24 07:07:39

by Lukasz Luba

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

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(-)
>>
>
>
>

2018-11-24 07:48:44

by Chanwoo Choi

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

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 <[email protected]>
>>> Suggested-by: Chanwoo Choi <[email protected]>
>>> Signed-off-by: Lukasz Luba <[email protected]>
>>> ---
>>> 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

2018-11-24 07:48:54

by Chanwoo Choi

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

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 <[email protected]>
>>> Suggested-by: Chanwoo Choi <[email protected]>
>>> Signed-off-by: Lukasz Luba <[email protected]>
>>> ---
>>> 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

2018-11-24 08:22:04

by Lukasz Luba

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

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 <[email protected]>
>>>> Suggested-by: Chanwoo Choi <[email protected]>
>>>> Signed-off-by: Lukasz Luba <[email protected]>
>>>> ---
>>>> 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
>>
>>
>
>

2018-11-24 08:22:29

by Lukasz Luba

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

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 <[email protected]>
>>>> Suggested-by: Chanwoo Choi <[email protected]>
>>>> Signed-off-by: Lukasz Luba <[email protected]>
>>>> ---
>>>> 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

2018-11-26 00:20:42

by Chanwoo Choi

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

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 <[email protected]>
>>>>> Suggested-by: Chanwoo Choi <[email protected]>
>>>>> Signed-off-by: Lukasz Luba <[email protected]>
>>>>> ---
>>>>> 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

2018-11-26 08:15:48

by MyungJoo Ham

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

>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 <[email protected]>
>Suggested-by: Chanwoo Choi <[email protected]>
>Signed-off-by: Lukasz Luba <[email protected]>

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

2018-12-03 14:06:24

by Lukasz Luba

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

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 <[email protected]>
>> Suggested-by: Chanwoo Choi <[email protected]>
>> Signed-off-by: Lukasz Luba <[email protected]>
>
> 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
>
>
>

2018-12-03 14:06:44

by Lukasz Luba

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

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 <[email protected]>
>>>>>> Suggested-by: Chanwoo Choi <[email protected]>
>>>>>> Signed-off-by: Lukasz Luba <[email protected]>
>>>>>> ---
>>>>>> 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
>>
>>
>>
>
>