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

From: "MyungJoo Ham" <myungjoo.ham@samsung.com>
To: "hl@rock-chips.com" <hl@rock-chips.com>
Cc: "Chanwoo Choi" <cw00.choi@samsung.com>,
	"dbasehore@chromium.org" <dbasehore@chromium.org>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"dianders@chromium.org" <dianders@chromium.org>,
	"linux-rockchip@lists.infradead.org"
	<linux-rockchip@lists.infradead.org>
Subject: Re: [PATCH v1 1/2] PM/devfreq: add suspend frequency support
Date: Thu, 24 Nov 2016 05:19:03 +0000	[thread overview]
Message-ID: <20161124051903epcms1p75ab1450eff0361338592c5cb659104f3@epcms1p7> (raw)
In-Reply-To: CGME20161124051903epcms1p75ab1450eff0361338592c5cb659104f3@epcms1p7

[-- Attachment #1: Type: text/plain, Size: 2803 bytes --]

On Wed, Nov 23, 2016 at 11:01 AM, hl <hl@rock-chips.com> wrote:
>
>
> On 2016年11月22日 20:48, MyungJoo Ham wrote:
>>
>> 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).
>>>>>
>>>>> +       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);

The devfreq_update() there, which is to invoke corresponding
governor to decide next frequency (get_target_freq).

In order to change the frequency ignoring whatever governors might say
(i.e., we are going to suspend / resume now), you need to talk to
the driver directly without invoking governors.

You only need to be careful on the sequence of notification chains and
status updating (you will probably be writing a much simpler version of
update_devfreq() for suspend/resume)

Cheers,
MyungJoo