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.
int devfreq_suspend_device(struct devfreq *devfreq)
{
+ int ret = 0;
+
if (!devfreq)
return -EINVAL;
if (!devfreq->governor)
return 0;
- return devfreq->governor->event_handler(devfreq,
+ ret = devfreq->governor->event_handler(devfreq,
DEVFREQ_GOV_SUSPEND, NULL);
+ if (ret < 0) {
+ dev_err(devfreq->dev.parent, "failed to suspend governor\n");
+ return ret;
+ }
+
+ if (devfreq->suspend_freq) {
+ ret = devfreq->profile->target(devfreq->dev.parent,
+ &devfreq->suspend_freq, 0);
+ if (ret < 0) {
+ dev_err(devfreq->dev.parent,
+ "failed to set suspend-freq\n");
+ return ret;
+ }
+ dev_dbg(devfreq->dev.parent, "Setting suspend-freq: %lu\n",
+ devfreq->suspend_freq);
+ }
+
+ return 0;
}
EXPORT_SYMBOL(devfreq_suspend_device);
--
Best Regards,
Chanwoo Choi
