Message-ID: <CAGTfZH34MvFGGuPgt=qUM6FMmi+Z80akY5ZH1k6gXdzg19fh9Q@mail.gmail.com>
Date:   Sat, 17 Dec 2016 23:50:03 +0900
From:   Chanwoo Choi <cwchoi00@...il.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>
Cc:     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>,
        Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>
Subject: Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support

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.

 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

Powered by blists - more mailing lists