2016-12-18 3:19 GMT+09:00 Tobias Jakobi <tjak...@math.uni-bielefeld.de>:
> Hey Chanwoo,
>
>
> Chanwoo Choi wrote:
>> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi <tjak...@math.uni-bielefeld.de>:
>>> Hey guys,
>>>
>>> Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi <cw00.c...@samsung.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
