Re: [PATCH v1 1/2] PM/devfreq: add suspend frequency support

From: MyungJoo Ham <myungjoo.ham@samsung.com>
To: hl <hl@rock-chips.com>
Cc: Chanwoo Choi <cw00.choi@samsung.com>,
	"dianders@chromium.org" <dianders@chromium.org>,
	"linux-rockchip@lists.infradead.org"
	<linux-rockchip@lists.infradead.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"dbasehore@chromium.org" <dbasehore@chromium.org>
Subject: Re: [PATCH v1 1/2] PM/devfreq: add suspend frequency support
Date: Tue, 22 Nov 2016 21:48:14 +0900	[thread overview]
Message-ID: <CAJ0PZbSYZYF6xJ-ut23G+tTXYLCDNpQ8_AJpjTdDGO7VnECMvg@mail.gmail.com> (raw)
In-Reply-To: <581AE13C.9070706@rock-chips.com>

On Thu, Nov 3, 2016 at 4:03 PM, hl <hl@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@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)

Cheers,
MyungJoo

>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
MyungJoo Ham, Ph.D.
Frontier CS Lab, S/W Center, Samsung Electronics