* [PATCH v4] PM/devfreq: add suspend frequency support
@ 2016-11-08 9:11 ` Lin Huang
0 siblings, 0 replies; 10+ messages in thread
From: Lin Huang @ 2016-11-08 9:11 UTC (permalink / raw)
To: myungjoo.ham
Cc: cw00.choi, dianders, linux-rockchip, linux-pm, dbasehore,
linux-arm-kernel, linux-kernel, Lin Huang
Add suspend frequency support and if needed set it to
the frequency obtained from the suspend opp (can be defined
using opp-v2 bindings and is optional).
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v2:
- use update_devfreq() instead devfreq_update_status()
Changes in v3:
- fix build error
Changes in v4:
- move dev_pm_opp_get_suspend_opp() to devfreq_add_device()
drivers/devfreq/devfreq.c | 15 +++++++++++++--
drivers/devfreq/governor_simpleondemand.c | 9 +++++++++
include/linux/devfreq.h | 9 +++++++++
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3ea76..d9d56e1 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -363,7 +363,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
mutex_unlock(&devfreq->lock);
return;
}
-
+ if (devfreq->suspend_freq) {
+ update_devfreq(devfreq);
+ devfreq->suspend_flag = true;
+ }
devfreq_update_status(devfreq, devfreq->previous_freq);
devfreq->stop_polling = true;
mutex_unlock(&devfreq->lock);
@@ -394,7 +397,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
devfreq->last_stat_updated = jiffies;
devfreq->stop_polling = false;
-
+ if (devfreq->suspend_freq)
+ devfreq->suspend_flag = false;
if (devfreq->profile->get_cur_freq &&
!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
devfreq->previous_freq = freq;
@@ -528,6 +532,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
struct devfreq *devfreq;
struct devfreq_governor *governor;
int err = 0;
+ struct dev_pm_opp *suspend_opp;
if (!dev || !profile || !governor_name) {
dev_err(dev, "%s: Invalid parameters.\n", __func__);
@@ -563,6 +568,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
devfreq->data = data;
devfreq->nb.notifier_call = devfreq_notifier_call;
+ rcu_read_lock();
+ suspend_opp = dev_pm_opp_get_suspend_opp(dev);
+ if (suspend_opp)
+ devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
+ rcu_read_unlock();
+
if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
mutex_unlock(&devfreq->lock);
devfreq_set_freq_table(devfreq);
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index ae72ba5..84b3ce1 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
struct devfreq_simple_ondemand_data *data = df->data;
unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
+ /*
+ * if devfreq in suspend status and have suspend_freq,
+ * the frequency need to set to suspend_freq
+ */
+ if (df->suspend_flag) {
+ *freq = df->suspend_freq;
+ return 0;
+ }
+
err = devfreq_update_stats(df);
if (err)
return err;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2de4e2e..c463ae1 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -172,6 +172,7 @@ struct devfreq {
struct delayed_work work;
unsigned long previous_freq;
+ unsigned long suspend_freq;
struct devfreq_dev_status last_status;
void *data; /* private data for governors */
@@ -179,6 +180,7 @@ struct devfreq {
unsigned long min_freq;
unsigned long max_freq;
bool stop_polling;
+ bool suspend_flag;
/* information for device frequency transition */
unsigned int total_trans;
@@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
/* Helper functions for devfreq user device driver with OPP. */
extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
unsigned long *freq, u32 flags);
+extern void devfreq_opp_get_suspend_opp(struct device *dev,
+ struct devfreq *devfreq);
extern int devfreq_register_opp_notifier(struct device *dev,
struct devfreq *devfreq);
extern int devfreq_unregister_opp_notifier(struct device *dev,
@@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
return ERR_PTR(-EINVAL);
}
+static inline void devfreq_opp_get_suspend_opp(struct device *dev,
+ struct devfreq *devfreq)
+{
+}
+
static inline int devfreq_register_opp_notifier(struct device *dev,
struct devfreq *devfreq)
{
--
2.6.6
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4] PM/devfreq: add suspend frequency support
@ 2016-11-08 9:11 ` Lin Huang
0 siblings, 0 replies; 10+ messages in thread
From: Lin Huang @ 2016-11-08 9:11 UTC (permalink / raw)
To: linux-arm-kernel
Add suspend frequency support and if needed set it to
the frequency obtained from the suspend opp (can be defined
using opp-v2 bindings and is optional).
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v2:
- use update_devfreq() instead devfreq_update_status()
Changes in v3:
- fix build error
Changes in v4:
- move dev_pm_opp_get_suspend_opp() to devfreq_add_device()
drivers/devfreq/devfreq.c | 15 +++++++++++++--
drivers/devfreq/governor_simpleondemand.c | 9 +++++++++
include/linux/devfreq.h | 9 +++++++++
3 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3ea76..d9d56e1 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -363,7 +363,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
mutex_unlock(&devfreq->lock);
return;
}
-
+ if (devfreq->suspend_freq) {
+ update_devfreq(devfreq);
+ devfreq->suspend_flag = true;
+ }
devfreq_update_status(devfreq, devfreq->previous_freq);
devfreq->stop_polling = true;
mutex_unlock(&devfreq->lock);
@@ -394,7 +397,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
devfreq->last_stat_updated = jiffies;
devfreq->stop_polling = false;
-
+ if (devfreq->suspend_freq)
+ devfreq->suspend_flag = false;
if (devfreq->profile->get_cur_freq &&
!devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
devfreq->previous_freq = freq;
@@ -528,6 +532,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
struct devfreq *devfreq;
struct devfreq_governor *governor;
int err = 0;
+ struct dev_pm_opp *suspend_opp;
if (!dev || !profile || !governor_name) {
dev_err(dev, "%s: Invalid parameters.\n", __func__);
@@ -563,6 +568,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
devfreq->data = data;
devfreq->nb.notifier_call = devfreq_notifier_call;
+ rcu_read_lock();
+ suspend_opp = dev_pm_opp_get_suspend_opp(dev);
+ if (suspend_opp)
+ devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
+ rcu_read_unlock();
+
if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
mutex_unlock(&devfreq->lock);
devfreq_set_freq_table(devfreq);
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index ae72ba5..84b3ce1 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
struct devfreq_simple_ondemand_data *data = df->data;
unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
+ /*
+ * if devfreq in suspend status and have suspend_freq,
+ * the frequency need to set to suspend_freq
+ */
+ if (df->suspend_flag) {
+ *freq = df->suspend_freq;
+ return 0;
+ }
+
err = devfreq_update_stats(df);
if (err)
return err;
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2de4e2e..c463ae1 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -172,6 +172,7 @@ struct devfreq {
struct delayed_work work;
unsigned long previous_freq;
+ unsigned long suspend_freq;
struct devfreq_dev_status last_status;
void *data; /* private data for governors */
@@ -179,6 +180,7 @@ struct devfreq {
unsigned long min_freq;
unsigned long max_freq;
bool stop_polling;
+ bool suspend_flag;
/* information for device frequency transition */
unsigned int total_trans;
@@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
/* Helper functions for devfreq user device driver with OPP. */
extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
unsigned long *freq, u32 flags);
+extern void devfreq_opp_get_suspend_opp(struct device *dev,
+ struct devfreq *devfreq);
extern int devfreq_register_opp_notifier(struct device *dev,
struct devfreq *devfreq);
extern int devfreq_unregister_opp_notifier(struct device *dev,
@@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
return ERR_PTR(-EINVAL);
}
+static inline void devfreq_opp_get_suspend_opp(struct device *dev,
+ struct devfreq *devfreq)
+{
+}
+
static inline int devfreq_register_opp_notifier(struct device *dev,
struct devfreq *devfreq)
{
--
2.6.6
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4] PM/devfreq: add suspend frequency support
2016-11-08 9:11 ` Lin Huang
@ 2016-11-08 9:32 ` Chanwoo Choi
-1 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2016-11-08 9:32 UTC (permalink / raw)
To: Lin Huang, myungjoo.ham
Cc: dianders, linux-rockchip, linux-pm, dbasehore, linux-arm-kernel,
linux-kernel
Hi Lin,
On 2016년 11월 08일 18:11, Lin Huang wrote:
> Add suspend frequency support and if needed set it to
> the frequency obtained from the suspend opp (can be defined
> using opp-v2 bindings and is optional).
>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - use update_devfreq() instead devfreq_update_status()
> Changes in v3:
> - fix build error
> Changes in v4:
> - move dev_pm_opp_get_suspend_opp() to devfreq_add_device()
>
> drivers/devfreq/devfreq.c | 15 +++++++++++++--
> drivers/devfreq/governor_simpleondemand.c | 9 +++++++++
> include/linux/devfreq.h | 9 +++++++++
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..d9d56e1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -363,7 +363,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
> mutex_unlock(&devfreq->lock);
> return;
> }
> -
> + if (devfreq->suspend_freq) {
> + update_devfreq(devfreq);
> + devfreq->suspend_flag = true;
You don't need the additional variable (devfreq->suspend_flag).
When adding the devfreq on devfreq_add_device(),
you can initialize the devfreq->suspend_freq as zero(0).
You can check whether devfreq->suspend_freq is 0 or not
without the new suspend_flag.
> + }
> devfreq_update_status(devfreq, devfreq->previous_freq);
> devfreq->stop_polling = true;
> mutex_unlock(&devfreq->lock);
> @@ -394,7 +397,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>
> devfreq->last_stat_updated = jiffies;
> devfreq->stop_polling = false;
> -
> + if (devfreq->suspend_freq)
> + devfreq->suspend_flag = false;
ditto. You don't need to add this code.
> if (devfreq->profile->get_cur_freq &&
> !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
> devfreq->previous_freq = freq;
> @@ -528,6 +532,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
> struct devfreq *devfreq;
> struct devfreq_governor *governor;
> int err = 0;
> + struct dev_pm_opp *suspend_opp;
>
> if (!dev || !profile || !governor_name) {
> dev_err(dev, "%s: Invalid parameters.\n", __func__);
> @@ -563,6 +568,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->data = data;
> devfreq->nb.notifier_call = devfreq_notifier_call;
>
> + rcu_read_lock();
> + suspend_opp = dev_pm_opp_get_suspend_opp(dev);
> + if (suspend_opp)
> + devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
> + rcu_read_unlock();
> +
> if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
> mutex_unlock(&devfreq->lock);
> devfreq_set_freq_table(devfreq);
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> index ae72ba5..84b3ce1 100644
> --- a/drivers/devfreq/governor_simpleondemand.c
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> struct devfreq_simple_ondemand_data *data = df->data;
> unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
>
> + /*
> + * if devfreq in suspend status and have suspend_freq,
> + * the frequency need to set to suspend_freq
> + */
> + if (df->suspend_flag) {
> + *freq = df->suspend_freq;
> + return 0;
> + }
You can check it as following:
if (df->suspend_freq != 0)
*freq = df->suspend_freq;
> +
> err = devfreq_update_stats(df);
> if (err)
> return err;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2de4e2e..c463ae1 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -172,6 +172,7 @@ struct devfreq {
> struct delayed_work work;
>
> unsigned long previous_freq;
> + unsigned long suspend_freq;
> struct devfreq_dev_status last_status;
>
> void *data; /* private data for governors */
> @@ -179,6 +180,7 @@ struct devfreq {
> unsigned long min_freq;
> unsigned long max_freq;
> bool stop_polling;
> + bool suspend_flag;
You don't need to add new variable.
>
> /* information for device frequency transition */
> unsigned int total_trans;
> @@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
> /* Helper functions for devfreq user device driver with OPP. */
> extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
> unsigned long *freq, u32 flags);
> +extern void devfreq_opp_get_suspend_opp(struct device *dev,
> + struct devfreq *devfreq);
Why do you need this function? Also, this patch don't include the body
of devfreq_opp_get_suspend_opp() function.
> extern int devfreq_register_opp_notifier(struct device *dev,
> struct devfreq *devfreq);
> extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
> return ERR_PTR(-EINVAL);
> }
>
> +static inline void devfreq_opp_get_suspend_opp(struct device *dev,
> + struct devfreq *devfreq)
> +{
> +}
> +
> static inline int devfreq_register_opp_notifier(struct device *dev,
> struct devfreq *devfreq)
> {
>
--
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] PM/devfreq: add suspend frequency support
@ 2016-11-08 9:32 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2016-11-08 9:32 UTC (permalink / raw)
To: linux-arm-kernel
Hi Lin,
On 2016? 11? 08? 18:11, Lin Huang wrote:
> Add suspend frequency support and if needed set it to
> the frequency obtained from the suspend opp (can be defined
> using opp-v2 bindings and is optional).
>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - use update_devfreq() instead devfreq_update_status()
> Changes in v3:
> - fix build error
> Changes in v4:
> - move dev_pm_opp_get_suspend_opp() to devfreq_add_device()
>
> drivers/devfreq/devfreq.c | 15 +++++++++++++--
> drivers/devfreq/governor_simpleondemand.c | 9 +++++++++
> include/linux/devfreq.h | 9 +++++++++
> 3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index bf3ea76..d9d56e1 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -363,7 +363,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
> mutex_unlock(&devfreq->lock);
> return;
> }
> -
> + if (devfreq->suspend_freq) {
> + update_devfreq(devfreq);
> + devfreq->suspend_flag = true;
You don't need the additional variable (devfreq->suspend_flag).
When adding the devfreq on devfreq_add_device(),
you can initialize the devfreq->suspend_freq as zero(0).
You can check whether devfreq->suspend_freq is 0 or not
without the new suspend_flag.
> + }
> devfreq_update_status(devfreq, devfreq->previous_freq);
> devfreq->stop_polling = true;
> mutex_unlock(&devfreq->lock);
> @@ -394,7 +397,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>
> devfreq->last_stat_updated = jiffies;
> devfreq->stop_polling = false;
> -
> + if (devfreq->suspend_freq)
> + devfreq->suspend_flag = false;
ditto. You don't need to add this code.
> if (devfreq->profile->get_cur_freq &&
> !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
> devfreq->previous_freq = freq;
> @@ -528,6 +532,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
> struct devfreq *devfreq;
> struct devfreq_governor *governor;
> int err = 0;
> + struct dev_pm_opp *suspend_opp;
>
> if (!dev || !profile || !governor_name) {
> dev_err(dev, "%s: Invalid parameters.\n", __func__);
> @@ -563,6 +568,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
> devfreq->data = data;
> devfreq->nb.notifier_call = devfreq_notifier_call;
>
> + rcu_read_lock();
> + suspend_opp = dev_pm_opp_get_suspend_opp(dev);
> + if (suspend_opp)
> + devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
> + rcu_read_unlock();
> +
> if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
> mutex_unlock(&devfreq->lock);
> devfreq_set_freq_table(devfreq);
> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
> index ae72ba5..84b3ce1 100644
> --- a/drivers/devfreq/governor_simpleondemand.c
> +++ b/drivers/devfreq/governor_simpleondemand.c
> @@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
> struct devfreq_simple_ondemand_data *data = df->data;
> unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
>
> + /*
> + * if devfreq in suspend status and have suspend_freq,
> + * the frequency need to set to suspend_freq
> + */
> + if (df->suspend_flag) {
> + *freq = df->suspend_freq;
> + return 0;
> + }
You can check it as following:
if (df->suspend_freq != 0)
*freq = df->suspend_freq;
> +
> err = devfreq_update_stats(df);
> if (err)
> return err;
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2de4e2e..c463ae1 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -172,6 +172,7 @@ struct devfreq {
> struct delayed_work work;
>
> unsigned long previous_freq;
> + unsigned long suspend_freq;
> struct devfreq_dev_status last_status;
>
> void *data; /* private data for governors */
> @@ -179,6 +180,7 @@ struct devfreq {
> unsigned long min_freq;
> unsigned long max_freq;
> bool stop_polling;
> + bool suspend_flag;
You don't need to add new variable.
>
> /* information for device frequency transition */
> unsigned int total_trans;
> @@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
> /* Helper functions for devfreq user device driver with OPP. */
> extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
> unsigned long *freq, u32 flags);
> +extern void devfreq_opp_get_suspend_opp(struct device *dev,
> + struct devfreq *devfreq);
Why do you need this function? Also, this patch don't include the body
of devfreq_opp_get_suspend_opp() function.
> extern int devfreq_register_opp_notifier(struct device *dev,
> struct devfreq *devfreq);
> extern int devfreq_unregister_opp_notifier(struct device *dev,
> @@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
> return ERR_PTR(-EINVAL);
> }
>
> +static inline void devfreq_opp_get_suspend_opp(struct device *dev,
> + struct devfreq *devfreq)
> +{
> +}
> +
> static inline int devfreq_register_opp_notifier(struct device *dev,
> struct devfreq *devfreq)
> {
>
--
Best Regards,
Chanwoo Choi
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] PM/devfreq: add suspend frequency support
2016-11-08 9:32 ` Chanwoo Choi
(?)
@ 2016-11-08 9:55 ` hl
-1 siblings, 0 replies; 10+ messages in thread
From: hl @ 2016-11-08 9:55 UTC (permalink / raw)
To: Chanwoo Choi, myungjoo.ham
Cc: linux-pm, dbasehore, dianders, linux-kernel, linux-rockchip,
linux-arm-kernel
Hi Chanwoo Choi,
On 2016年11月08日 17:32, Chanwoo Choi wrote:
> Hi Lin,
>
> On 2016년 11월 08일 18:11, Lin Huang wrote:
>> Add suspend frequency support and if needed set it to
>> the frequency obtained from the suspend opp (can be defined
>> using opp-v2 bindings and is optional).
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>> Changes in v2:
>> - use update_devfreq() instead devfreq_update_status()
>> Changes in v3:
>> - fix build error
>> Changes in v4:
>> - move dev_pm_opp_get_suspend_opp() to devfreq_add_device()
>>
>> drivers/devfreq/devfreq.c | 15 +++++++++++++--
>> drivers/devfreq/governor_simpleondemand.c | 9 +++++++++
>> include/linux/devfreq.h | 9 +++++++++
>> 3 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..d9d56e1 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -363,7 +363,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>> mutex_unlock(&devfreq->lock);
>> return;
>> }
>> -
>> + if (devfreq->suspend_freq) {
>> + update_devfreq(devfreq);
>> + devfreq->suspend_flag = true;
> You don't need the additional variable (devfreq->suspend_flag).
> When adding the devfreq on devfreq_add_device(),
> you can initialize the devfreq->suspend_freq as zero(0).
>
> You can check whether devfreq->suspend_freq is 0 or not
> without the new suspend_flag.
>
>> + }
>> devfreq_update_status(devfreq, devfreq->previous_freq);
>> devfreq->stop_polling = true;
>> mutex_unlock(&devfreq->lock);
>> @@ -394,7 +397,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>
>> devfreq->last_stat_updated = jiffies;
>> devfreq->stop_polling = false;
>> -
>> + if (devfreq->suspend_freq)
>> + devfreq->suspend_flag = false;
> ditto. You don't need to add this code.
>
>> if (devfreq->profile->get_cur_freq &&
>> !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
>> devfreq->previous_freq = freq;
>> @@ -528,6 +532,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> struct devfreq *devfreq;
>> struct devfreq_governor *governor;
>> int err = 0;
>> + struct dev_pm_opp *suspend_opp;
>>
>> if (!dev || !profile || !governor_name) {
>> dev_err(dev, "%s: Invalid parameters.\n", __func__);
>> @@ -563,6 +568,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> devfreq->data = data;
>> devfreq->nb.notifier_call = devfreq_notifier_call;
>>
>> + rcu_read_lock();
>> + suspend_opp = dev_pm_opp_get_suspend_opp(dev);
>> + if (suspend_opp)
>> + devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
>> + rcu_read_unlock();
>> +
>> if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
>> mutex_unlock(&devfreq->lock);
>> devfreq_set_freq_table(devfreq);
>> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
>> index ae72ba5..84b3ce1 100644
>> --- a/drivers/devfreq/governor_simpleondemand.c
>> +++ b/drivers/devfreq/governor_simpleondemand.c
>> @@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>> struct devfreq_simple_ondemand_data *data = df->data;
>> unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
>>
>> + /*
>> + * if devfreq in suspend status and have suspend_freq,
>> + * the frequency need to set to suspend_freq
>> + */
>> + if (df->suspend_flag) {
>> + *freq = df->suspend_freq;
>> + return 0;
>> + }
> You can check it as following:
>
> if (df->suspend_freq != 0)
> *freq = df->suspend_freq;
If i do like this, it will always return suspend frequency, since
devfreq->suspend_freq will be assigned value
if we define it on dts. But what we want is only in suspend status we
return the suspend frequency.
>
>> +
>> err = devfreq_update_stats(df);
>> if (err)
>> return err;
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 2de4e2e..c463ae1 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -172,6 +172,7 @@ struct devfreq {
>> struct delayed_work work;
>>
>> unsigned long previous_freq;
>> + unsigned long suspend_freq;
>> struct devfreq_dev_status last_status;
>>
>> void *data; /* private data for governors */
>> @@ -179,6 +180,7 @@ struct devfreq {
>> unsigned long min_freq;
>> unsigned long max_freq;
>> bool stop_polling;
>> + bool suspend_flag;
> You don't need to add new variable.
>
>>
>> /* information for device frequency transition */
>> unsigned int total_trans;
>> @@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
>> /* Helper functions for devfreq user device driver with OPP. */
>> extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>> unsigned long *freq, u32 flags);
>> +extern void devfreq_opp_get_suspend_opp(struct device *dev,
>> + struct devfreq *devfreq);
> Why do you need this function? Also, this patch don't include the body
> of devfreq_opp_get_suspend_opp() function.
Oh, forget to delete it, sorry about that.
>
>> extern int devfreq_register_opp_notifier(struct device *dev,
>> struct devfreq *devfreq);
>> extern int devfreq_unregister_opp_notifier(struct device *dev,
>> @@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>> return ERR_PTR(-EINVAL);
>> }
>>
>> +static inline void devfreq_opp_get_suspend_opp(struct device *dev,
>> + struct devfreq *devfreq)
>> +{
>> +}
>> +
>> static inline int devfreq_register_opp_notifier(struct device *dev,
>> struct devfreq *devfreq)
>> {
>>
--
Lin Huang
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4] PM/devfreq: add suspend frequency support
@ 2016-11-08 9:55 ` hl
0 siblings, 0 replies; 10+ messages in thread
From: hl @ 2016-11-08 9:55 UTC (permalink / raw)
To: Chanwoo Choi, myungjoo.ham
Cc: linux-pm, dbasehore, linux-kernel, dianders, linux-rockchip,
linux-arm-kernel
Hi Chanwoo Choi,
On 2016年11月08日 17:32, Chanwoo Choi wrote:
> Hi Lin,
>
> On 2016년 11월 08일 18:11, Lin Huang wrote:
>> Add suspend frequency support and if needed set it to
>> the frequency obtained from the suspend opp (can be defined
>> using opp-v2 bindings and is optional).
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>> Changes in v2:
>> - use update_devfreq() instead devfreq_update_status()
>> Changes in v3:
>> - fix build error
>> Changes in v4:
>> - move dev_pm_opp_get_suspend_opp() to devfreq_add_device()
>>
>> drivers/devfreq/devfreq.c | 15 +++++++++++++--
>> drivers/devfreq/governor_simpleondemand.c | 9 +++++++++
>> include/linux/devfreq.h | 9 +++++++++
>> 3 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..d9d56e1 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -363,7 +363,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>> mutex_unlock(&devfreq->lock);
>> return;
>> }
>> -
>> + if (devfreq->suspend_freq) {
>> + update_devfreq(devfreq);
>> + devfreq->suspend_flag = true;
> You don't need the additional variable (devfreq->suspend_flag).
> When adding the devfreq on devfreq_add_device(),
> you can initialize the devfreq->suspend_freq as zero(0).
>
> You can check whether devfreq->suspend_freq is 0 or not
> without the new suspend_flag.
>
>> + }
>> devfreq_update_status(devfreq, devfreq->previous_freq);
>> devfreq->stop_polling = true;
>> mutex_unlock(&devfreq->lock);
>> @@ -394,7 +397,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>
>> devfreq->last_stat_updated = jiffies;
>> devfreq->stop_polling = false;
>> -
>> + if (devfreq->suspend_freq)
>> + devfreq->suspend_flag = false;
> ditto. You don't need to add this code.
>
>> if (devfreq->profile->get_cur_freq &&
>> !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
>> devfreq->previous_freq = freq;
>> @@ -528,6 +532,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> struct devfreq *devfreq;
>> struct devfreq_governor *governor;
>> int err = 0;
>> + struct dev_pm_opp *suspend_opp;
>>
>> if (!dev || !profile || !governor_name) {
>> dev_err(dev, "%s: Invalid parameters.\n", __func__);
>> @@ -563,6 +568,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> devfreq->data = data;
>> devfreq->nb.notifier_call = devfreq_notifier_call;
>>
>> + rcu_read_lock();
>> + suspend_opp = dev_pm_opp_get_suspend_opp(dev);
>> + if (suspend_opp)
>> + devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
>> + rcu_read_unlock();
>> +
>> if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
>> mutex_unlock(&devfreq->lock);
>> devfreq_set_freq_table(devfreq);
>> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
>> index ae72ba5..84b3ce1 100644
>> --- a/drivers/devfreq/governor_simpleondemand.c
>> +++ b/drivers/devfreq/governor_simpleondemand.c
>> @@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>> struct devfreq_simple_ondemand_data *data = df->data;
>> unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
>>
>> + /*
>> + * if devfreq in suspend status and have suspend_freq,
>> + * the frequency need to set to suspend_freq
>> + */
>> + if (df->suspend_flag) {
>> + *freq = df->suspend_freq;
>> + return 0;
>> + }
> You can check it as following:
>
> if (df->suspend_freq != 0)
> *freq = df->suspend_freq;
If i do like this, it will always return suspend frequency, since
devfreq->suspend_freq will be assigned value
if we define it on dts. But what we want is only in suspend status we
return the suspend frequency.
>
>> +
>> err = devfreq_update_stats(df);
>> if (err)
>> return err;
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 2de4e2e..c463ae1 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -172,6 +172,7 @@ struct devfreq {
>> struct delayed_work work;
>>
>> unsigned long previous_freq;
>> + unsigned long suspend_freq;
>> struct devfreq_dev_status last_status;
>>
>> void *data; /* private data for governors */
>> @@ -179,6 +180,7 @@ struct devfreq {
>> unsigned long min_freq;
>> unsigned long max_freq;
>> bool stop_polling;
>> + bool suspend_flag;
> You don't need to add new variable.
>
>>
>> /* information for device frequency transition */
>> unsigned int total_trans;
>> @@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
>> /* Helper functions for devfreq user device driver with OPP. */
>> extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>> unsigned long *freq, u32 flags);
>> +extern void devfreq_opp_get_suspend_opp(struct device *dev,
>> + struct devfreq *devfreq);
> Why do you need this function? Also, this patch don't include the body
> of devfreq_opp_get_suspend_opp() function.
Oh, forget to delete it, sorry about that.
>
>> extern int devfreq_register_opp_notifier(struct device *dev,
>> struct devfreq *devfreq);
>> extern int devfreq_unregister_opp_notifier(struct device *dev,
>> @@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>> return ERR_PTR(-EINVAL);
>> }
>>
>> +static inline void devfreq_opp_get_suspend_opp(struct device *dev,
>> + struct devfreq *devfreq)
>> +{
>> +}
>> +
>> static inline int devfreq_register_opp_notifier(struct device *dev,
>> struct devfreq *devfreq)
>> {
>>
--
Lin Huang
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4] PM/devfreq: add suspend frequency support
@ 2016-11-08 9:55 ` hl
0 siblings, 0 replies; 10+ messages in thread
From: hl @ 2016-11-08 9:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi Chanwoo Choi,
On 2016?11?08? 17:32, Chanwoo Choi wrote:
> Hi Lin,
>
> On 2016? 11? 08? 18:11, Lin Huang wrote:
>> Add suspend frequency support and if needed set it to
>> the frequency obtained from the suspend opp (can be defined
>> using opp-v2 bindings and is optional).
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>> Changes in v2:
>> - use update_devfreq() instead devfreq_update_status()
>> Changes in v3:
>> - fix build error
>> Changes in v4:
>> - move dev_pm_opp_get_suspend_opp() to devfreq_add_device()
>>
>> drivers/devfreq/devfreq.c | 15 +++++++++++++--
>> drivers/devfreq/governor_simpleondemand.c | 9 +++++++++
>> include/linux/devfreq.h | 9 +++++++++
>> 3 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index bf3ea76..d9d56e1 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -363,7 +363,10 @@ void devfreq_monitor_suspend(struct devfreq *devfreq)
>> mutex_unlock(&devfreq->lock);
>> return;
>> }
>> -
>> + if (devfreq->suspend_freq) {
>> + update_devfreq(devfreq);
>> + devfreq->suspend_flag = true;
> You don't need the additional variable (devfreq->suspend_flag).
> When adding the devfreq on devfreq_add_device(),
> you can initialize the devfreq->suspend_freq as zero(0).
>
> You can check whether devfreq->suspend_freq is 0 or not
> without the new suspend_flag.
>
>> + }
>> devfreq_update_status(devfreq, devfreq->previous_freq);
>> devfreq->stop_polling = true;
>> mutex_unlock(&devfreq->lock);
>> @@ -394,7 +397,8 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>
>> devfreq->last_stat_updated = jiffies;
>> devfreq->stop_polling = false;
>> -
>> + if (devfreq->suspend_freq)
>> + devfreq->suspend_flag = false;
> ditto. You don't need to add this code.
>
>> if (devfreq->profile->get_cur_freq &&
>> !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq))
>> devfreq->previous_freq = freq;
>> @@ -528,6 +532,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> struct devfreq *devfreq;
>> struct devfreq_governor *governor;
>> int err = 0;
>> + struct dev_pm_opp *suspend_opp;
>>
>> if (!dev || !profile || !governor_name) {
>> dev_err(dev, "%s: Invalid parameters.\n", __func__);
>> @@ -563,6 +568,12 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> devfreq->data = data;
>> devfreq->nb.notifier_call = devfreq_notifier_call;
>>
>> + rcu_read_lock();
>> + suspend_opp = dev_pm_opp_get_suspend_opp(dev);
>> + if (suspend_opp)
>> + devfreq->suspend_freq = dev_pm_opp_get_freq(suspend_opp);
>> + rcu_read_unlock();
>> +
>> if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
>> mutex_unlock(&devfreq->lock);
>> devfreq_set_freq_table(devfreq);
>> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
>> index ae72ba5..84b3ce1 100644
>> --- a/drivers/devfreq/governor_simpleondemand.c
>> +++ b/drivers/devfreq/governor_simpleondemand.c
>> @@ -29,6 +29,15 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>> struct devfreq_simple_ondemand_data *data = df->data;
>> unsigned long max = (df->max_freq) ? df->max_freq : UINT_MAX;
>>
>> + /*
>> + * if devfreq in suspend status and have suspend_freq,
>> + * the frequency need to set to suspend_freq
>> + */
>> + if (df->suspend_flag) {
>> + *freq = df->suspend_freq;
>> + return 0;
>> + }
> You can check it as following:
>
> if (df->suspend_freq != 0)
> *freq = df->suspend_freq;
If i do like this, it will always return suspend frequency, since
devfreq->suspend_freq will be assigned value
if we define it on dts. But what we want is only in suspend status we
return the suspend frequency.
>
>> +
>> err = devfreq_update_stats(df);
>> if (err)
>> return err;
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index 2de4e2e..c463ae1 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -172,6 +172,7 @@ struct devfreq {
>> struct delayed_work work;
>>
>> unsigned long previous_freq;
>> + unsigned long suspend_freq;
>> struct devfreq_dev_status last_status;
>>
>> void *data; /* private data for governors */
>> @@ -179,6 +180,7 @@ struct devfreq {
>> unsigned long min_freq;
>> unsigned long max_freq;
>> bool stop_polling;
>> + bool suspend_flag;
> You don't need to add new variable.
>
>>
>> /* information for device frequency transition */
>> unsigned int total_trans;
>> @@ -214,6 +216,8 @@ extern int devfreq_resume_device(struct devfreq *devfreq);
>> /* Helper functions for devfreq user device driver with OPP. */
>> extern struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>> unsigned long *freq, u32 flags);
>> +extern void devfreq_opp_get_suspend_opp(struct device *dev,
>> + struct devfreq *devfreq);
> Why do you need this function? Also, this patch don't include the body
> of devfreq_opp_get_suspend_opp() function.
Oh, forget to delete it, sorry about that.
>
>> extern int devfreq_register_opp_notifier(struct device *dev,
>> struct devfreq *devfreq);
>> extern int devfreq_unregister_opp_notifier(struct device *dev,
>> @@ -348,6 +352,11 @@ static inline struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
>> return ERR_PTR(-EINVAL);
>> }
>>
>> +static inline void devfreq_opp_get_suspend_opp(struct device *dev,
>> + struct devfreq *devfreq)
>> +{
>> +}
>> +
>> static inline int devfreq_register_opp_notifier(struct device *dev,
>> struct devfreq *devfreq)
>> {
>>
--
Lin Huang
^ permalink raw reply [flat|nested] 10+ messages in thread