Message-ID: <58558195.20907@math.uni-bielefeld.de>
Date:   Sat, 17 Dec 2016 19:19:01 +0100
From:   Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>
To:     cwchoi00@...il.com
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

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.

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.

-Tobias


> 
> Thanks,
> Chanwoo Choi
> 
>>
>> - Tobias
>>
>>
>>
>>>
>>>  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);
>>>
>>

Powered by blists - more mailing lists