Message-ID: <CAGTfZH2OchOPFa=kEd0VMxuLicjyKJv-tiT7BrmBKE9gLnvU9Q@mail.gmail.com>
Date:   Sun, 18 Dec 2016 07:03:18 +0900
From:   Chanwoo Choi <cwchoi00@...il.com>
To:     Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>
Cc:     Chanwoo Choi <cw00.choi@...sung.com>, hl <hl@...k-chips.com>,
        "myungjoo.ham@...sung.com" <myungjoo.ham@...sung.com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "dbasehore@...omium.org" <dbasehore@...omium.org>,
        "dianders@...omium.org" <dianders@...omium.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support

2016-12-18 3:19 GMT+09:00 Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>:
> Hey Chanwoo,
>
>
> Chanwoo Choi wrote:
>> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>:
>>> Hey guys,
>>>
>>> Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.choi@...sung.com>:
>>>>> Hi Lin,
>>>>>
>>>>> On 2016년 11월 24일 18:28, Chanwoo Choi wrote:
>>>>>> Hi Lin,
>>>>>>
>>>>>> On 2016년 11월 24일 17:34, hl wrote:
>>>>>>> Hi Chanwoo Choi,
>>>>>>>
>>>>>>>
>>>>>>> On 2016年11月24日 16:16, Chanwoo Choi wrote:
>>>>>>>> Hi Lin,
>>>>>>>>
>>>>>>>> On 2016년 11월 24일 16:34, hl wrote:
>>>>>>>>> Hi Chanwoo Choi,
>>>>>>>>>
>>>>>>>>>      I think the dev_pm_opp_get_suspend_opp() have implement most of
>>>>>>>>> the funtion, all we need is just define the node in dts, like following:
>>>>>>>>>
>>>>>>>>> &dmc_opp_table {
>>>>>>>>>      opp06 {
>>>>>>>>>          opp-suspend;
>>>>>>>>>      };
>>>>>>>>> };
>>>>>>>> Two approaches use the 'opp-suspend' property.
>>>>>>>>
>>>>>>>> I think that the method to support suspend-opp have to
>>>>>>>> guarantee following conditions:
>>>>>>>> - Support the all of devfreq's governors.
>>>>>>>     As MyungJoo Ham suggestion, i will set the suspend frequency in devfreq_suspend_device(),
>>>>>>> which will ingore governor.
>>>>>>
>>>>>> Other approach already support the all of governors.
>>>>>> Before calling the mail, I discussed with Myungjoo Ham.
>>>>>> Myungjoo prefer to use the devfreq_suspend/devfreq_resume().
>>>>>
>>>>> It is not correct expression. We need to wait the reply from Myungjoo
>>>>> to clarify this.
>>>>>
>>>>>>
>>>>>> To Myungjoo,
>>>>>> Please add your opinion how to support the suspend frequency.
>>>>>
>>>>>>
>>>>>>>> - Devfreq framework have the responsibility to change the
>>>>>>>>    frequency/voltage for suspend-opp. If we uses the
>>>>>>>>    new devfreq_suspend(), each devfreq device don't care
>>>>>>>>    how to support the suspend-opp. Just the developer of each
>>>>>>>>    devfreq device need to add 'opp-suspend' propet to OPP entry in DT file.
>>>>>>> Why should support change the voltage in devfreq framework, i think it shuold be handle in
>>>>>>> specific driver, i think the devfreq only handle it can get the right frequency, then pass it to
>>>>>>
>>>>>> No, the frequency should be handled by governor or framework.
>>>>>> The each devfreq device has no any responsibility of next frequency/voltage.
>>>>>> The governor and core of devfreq can decide the next frequency/voltage.
>>>>>> You can refer to the cpufreq subsystem.
>>>>>>
>>>>>>> specific driver, i think the voltage should handle in the devfreq->profile->target();
>>>>>>
>>>>>> The call of devfreq->profile->target() have to be handled by devfreq framework.
>>>>>> If user want to set the suspend frequency, user can add the 'suspend-opp' property.
>>>>>> It think this way is easy.
>>>>>>
>>>>>> But,
>>>>>> If the each devfreq device want to decide the next frequency/voltage only for
>>>>>> suspend state. We can check the cpufreq subsystem.
>>>>>>
>>>>>> If specific devfreq device want to handle the suspend frequency,
>>>>>> each devfreq will add the own suspend/resume functions as following:
>>>>>>
>>>>>>       struct devfreq_dev_profile {
>>>>>>               int (*suspend)(struct devfreq *dev);    // new function pointer
>>>>>>               int (*resume)(struct devfreq *dev);     // new function pointer
>>>>>>       } a_profile;
>>>>>>
>>>>>>       a_profile = devfreq_generic_suspend;
>>>>>>
>>>>>>       The devfreq framework will provide the devfreq_generic_suspend() funticon.
>>>>>>               int devfreq_generic_suspend(struce devfreq *dev) {
>>>>>>                       ...
>>>>>>                       devfreq->profile->target(..., devfreq->suspend_freq);
>>>>>>                       ...
>>>>>>               }
>>>>>>
>>>>>>       or
>>>>>>
>>>>>>       a_profile = a_devfreq_suspend; // specific function of each devfreq device
>>>>>>
>>>>>>       The devfreq_suspend() will call 'devfreq->profile->suspend()' function
>>>>>>       instead of devfreq->profile->target();
>>>>>>
>>>>>>       The devfreq call the 'devfreq->profile->suspend()'
>>>>>>       to support the suspend frequency.
>>>>>>
>>>>>> Regards,
>>>>>> Chanwoo Choi
>>>>>
>>>>> The key difference between two approaches:
>>>>>
>>>>> Your approach:
>>>>> - The each developer should add the 'opp-suspend' property to the dts file.
>>>>> - The each devfreq should call the devfreq_suspend_device()
>>>>>   to support the suspend frequency.
>>>>>
>>>>>   If each devfreq doesn't call the devfreq_suspend_device(), devfreq framework
>>>>>   can support the suspend frequency.
>>>>>
>>>>> Other approach:
>>>>> - The each developer only should add the 'opp-suspend' property to the dts file
>>>>>   without the additional behavior.
>>>>>
>>>>> In the cpufreq subsystem,
>>>>> When support the suspend frequency of cpufreq, we just add 'opp-suspend' property
>>>>> without the additional behavior.
>>>>
>>>> I'm missing the use-case when using the devfreq_suspend_device()
>>>> before entering the suspend mode. We should consider the case when
>>>> devfreq device
>>>> calls the devfreq_suspend_device() directly. Because devfreq_suspend_device()
>>>> is exported function, each devfreq device call this function on the fly
>>>> without entering the suspend mode.
>>>>
>>>> I correct my opinion. Your approach is necessary. I'm sorry to confuse you.
>>>> So, I make the following patch. This patch set the suspend frequency
>>>> in devfreq_suspend_device() after stoping the governor.
>>>> It consider the all governors of devfreq.
>>>>
>>>> What do you think?
>>>> If you are ok, I'll send this patch with your author.
>>> The problem I see here is that we need to keep track of the suspended
>>> state when suspending the (entire) devfreq subsystem. When doing that,
>>> we don't know if any device driver has previously called
>>> devfreq_suspend_device() and might end up calling it twice.
>>>
>>> Same thing on devfreq subsystem resume.
>>>
>>> I've prepared a new RFC of my series (going to send it shortly), but I'm
>>> not so happy with the current design. I think it would be much cleaner
>>> to keep some suspend_refcount in struct devfreq so that I can call
>>> devfreq_suspend_device() multiple times, while keeping a sane internal
>>> state.
>>
>> I agree the devfreq need  reference count for devfreq_suspend/resume_device.
>> This patch focus on when changing the suspend frequency.
>>
>>>
>>> Something like devfreq_device_runtime_{put,get}() perhaps?
>>
>> Why do devfreq need new additional functions?
>> I think the devfreq_suspend/resume_device are enough.
> Just thinking out loud here. I would prefer a naming that implies that
> some refcounting is going on. When I see a pair of function with
> put/get, then I usually know what is going on.

The suspend/resume name are already used as pair function name.
I think that devfreq_suspend/resume_device() are appropriate.

Usually, '_runtime_put/get' naming means the 'runtime PM' callback function
which handle the all of resource (e.g., clock, regulator, register and so on).

>
> Here I would have to look at the actual implementation to realize, at
> the moment, that I have to be careful calling these functions twice.

Sure. I'm waiting your patch.

[snip]

Thanks,
Chanwoo Choi

Powered by blists - more mailing lists