From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753708AbcKHJ6K (ORCPT ); Tue, 8 Nov 2016 04:58:10 -0500 Received: from lucky1.263xmail.com ([211.157.147.136]:53848 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753434AbcKHJ4I (ORCPT ); Tue, 8 Nov 2016 04:56:08 -0500 X-263anti-spam: KSV:0;BIG:0;ABS:1;DNS:0;ATT:0;SPF:S; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ADDR-CHECKED: 0 X-RL-SENDER: hl@rock-chips.com X-FST-TO: linux-arm-kernel@lists.infradead.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: hl@rock-chips.com X-UNIQUE-TAG: <20e02516742eb6923ce5e477f8af0cba> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v4] PM/devfreq: add suspend frequency support To: Chanwoo Choi , myungjoo.ham@samsung.com References: <1478596289-7442-1-git-send-email-hl@rock-chips.com> <58219B96.6060106@samsung.com> Cc: linux-pm@vger.kernel.org, dbasehore@chromium.org, linux-kernel@vger.kernel.org, dianders@chromium.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org From: hl Message-ID: <5821A125.10308@rock-chips.com> Date: Tue, 8 Nov 2016 17:55:49 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <58219B96.6060106@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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