Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932507AbcLQWDl (ORCPT ); Sat, 17 Dec 2016 17:03:41 -0500 Received: from mail-qk0-f193.google.com ([209.85.220.193]:35093 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752724AbcLQWDj (ORCPT ); Sat, 17 Dec 2016 17:03:39 -0500 MIME-Version: 1.0 Reply-To: cwchoi00@gmail.com In-Reply-To: <58558195.20907@math.uni-bielefeld.de> References: <20161124061416epcms1p44a0152bca14312f1229cab835ea0297f@epcms1p4> <58368C91.8030502@rock-chips.com> <5836927B.9010205@samsung.com> <5836980F.6050006@rock-chips.com> <5836A1E0.1070707@samsung.com> <5836A622.20007@rock-chips.com> <5836B2C2.6090308@samsung.com> <5836B8D1.9050707@samsung.com> <5855560E.4050609@math.uni-bielefeld.de> <58558195.20907@math.uni-bielefeld.de> From: Chanwoo Choi Date: Sun, 18 Dec 2016 07:03:18 +0900 Message-ID: Subject: Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support To: Tobias Jakobi Cc: Chanwoo Choi , hl , "myungjoo.ham@samsung.com" , "linux-pm@vger.kernel.org" , "dbasehore@chromium.org" , "dianders@chromium.org" , "linux-kernel@vger.kernel.org" , "linux-rockchip@lists.infradead.org" , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uBHM3lh4021083 Content-Length: 7362 Lines: 181 2016-12-18 3:19 GMT+09:00 Tobias Jakobi : > Hey Chanwoo, > > > Chanwoo Choi wrote: >> 2016-12-18 0:13 GMT+09:00 Tobias Jakobi : >>> Hey guys, >>> >>> Chanwoo Choi wrote: >>>> Hi Lin, >>>> >>>> 2016-11-24 18:54 GMT+09:00 Chanwoo Choi : >>>>> 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