On 2016?11?22? 20:48, MyungJoo Ham wrote: > On Thu, Nov 3, 2016 at 4:03 PM, hl <hl at rock-chips.com> wrote: >> Hi MyungJoo Ham, >> >> >> On 2016?11?02? 14:35, MyungJoo Ham 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). >>>> >>>> Change-Id: Iaa0d3848d63d9ce03f65ea76f263e4685a4c295e >>>> Signed-off-by: Lin Huang <hl at rock-chips.com> >>>> --- >>>> drivers/devfreq/devfreq.c | 17 ++++++++++++++++- >>>> include/linux/devfreq.h | 9 +++++++++ >>>> 2 files changed, 25 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index bf3ea76..d1152eb 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -364,7 +364,10 @@ void devfreq_monitor_suspend(struct devfreq >>>> *devfreq) >>>> return; >>>> } >>>> - devfreq_update_status(devfreq, devfreq->previous_freq); >>>> + if (devfreq->suspend_freq) >>>> + devfreq_update_status(devfreq, devfreq->suspend_freq); >>>> + else >>>> + devfreq_update_status(devfreq, devfreq->previous_freq); >>> This is incorrect. >>> >>> devfreq_update_status is to record the statistics. >>> This code intends to record the current status before entering suspend; >>> thus you should not record "suspend_freq" here. >>> >>> If you really need to record the frequency during suspended state >>> for the statistics, you need to do that at resume; however, I object to >>> that idea either. >>> >>> If you really want to set a predefined suspend-to-RAM mode frequency, >>> (probably because of HW instability during resume process) >>> you need to update, not udpate_status (statistics). >>> >> Thanks for pointing it, but if i use update_devfreq(), it can not specify >> the >> frequency, it call the get_target_freq() to get the frequency and setting, >> use now devfreq framework, it seems hard to set the specific freqeuency >> when suspend. Do you have any idea, thanks. > > It appears that we need to set the frequency directly with target() function > at suspend/resume after it is assured that related routines are > already suspended. > > Plus, you might need to consider using devfreq_suspend/resume_device instead > of monitor. monitor functions are for the governor-specific behaviors. > You'll need such capabilities regardless of governors. (even userspace-governor > needs this) We still need to sync the all status even i call target() in devfreq_suspend/resume_device directly, so still need update_devfreq() other setp except devfreq->governor->get_target_freq(devfreq, &freq); > > Cheers, > MyungJoo > >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >> the body of a message to majordomo at vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Lin Huang
