From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756253AbcKXHex (ORCPT ); Thu, 24 Nov 2016 02:34:53 -0500 Received: from lucky1.263xmail.com ([211.157.147.130]:34161 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751625AbcKXHev (ORCPT ); Thu, 24 Nov 2016 02:34:51 -0500 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ADDR-CHECKED: 0 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-RL-SENDER: hl@rock-chips.com X-FST-TO: tjakobi@math.uni-bielefeld.de X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: hl@rock-chips.com X-UNIQUE-TAG: <583b3eb288d4b82bd903af881e8daa4f> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support To: Chanwoo Choi , myungjoo.ham@samsung.com References: <20161124061416epcms1p44a0152bca14312f1229cab835ea0297f@epcms1p4> <58368C91.8030502@rock-chips.com> <5836927B.9010205@samsung.com> Cc: "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" , Tobias Jakobi From: hl Message-ID: <5836980F.6050006@rock-chips.com> Date: Thu, 24 Nov 2016 15:34:39 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <5836927B.9010205@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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; }; }; so i think my way semm more simple. On 2016年11月24日 15:10, Chanwoo Choi wrote: > + Tobias Jakobi, > > Hi Lin, > > We need to discuss how to support the suspend-opp of devfreq device. > Now, there are two patch thread for suspend-opp of devfreq. > > The Lin's approach modify the devfreq_suspend_device() to support suspend-opp. > The Tobias's approach[1] add new devfreq_suspend() and then call it on dpm_suspend() > when entering the suspend state. > > [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl > - https://patchwork.kernel.org/patch/9443323/ > - https://patchwork.kernel.org/patch/9443325/ > - https://patchwork.kernel.org/patch/9443329/ > - https://patchwork.kernel.org/patch/9443331/ > > I think we need to discuss it together. > > Regards, > Chanwoo Choi > > On 2016년 11월 24일 15:45, hl wrote: >> Hi MyungJoo Ham, >> >> On 2016年11月24日 14:14, MyungJoo Ham wrote: >>> On Thu, Nov 24, 2016 at 11:18 AM, hl wrote: >>>> Hi MyungJoo Ham, >>> [] >>>>> We still need to sync the all status even i call target() in >>>>> devfreq_suspend/resume_device >>>>> directly, so still need update_devfreq() other setp except >>>>> devfreq->governor->get_target_freq(devfreq, &freq); >>>> And i think it better to be governor behaviors, for userspace they may not >>>> want to change >>>> the suspend frequency like other governor, the frequency should decide by >>>> the user, if they >>>> want this function, they should like other governor to rigister a >>>> devfreq_monitor_suspend(). >>>> What do you think about my rev6 patch? >>> If I understand the intention correctly, this is for the stability of >>> the device due to the behavior or bootloader/SoC-initializer, which >>> has nothing to do with governors. >>> >>> Even if users are using userspace, as long as they set the custom >>> frequencies lower than the default, they have the possibility of >>> being unstable as ondemand is going to have. >>> >>> >>> To reuse the update_devfreq() code, you may do something like: >>> >>> static int _update_freq(struct devfreq *devfreq, bool is_suspending) >>> { >>> /* original contents of update_freq with if statement with is_suspending wrapping get_target_freq */ >>> } >>> int update_freq(struct devfreq *devfreq) >>> { >>> return _update_freq(devfreq, false); >>> } >>> >>> >>> There should be other good non-invasive methods that are not governoe-specific as well. >>> >> Thanks for your suggestion, i will update the new version soon. >>> Cheers, >>> MyungJoo >>> >>> >>> >>> >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> -- >> Lin Huang >> > > > -- Lin Huang From mboxrd@z Thu Jan 1 00:00:00 1970 From: hl Subject: Re: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support Date: Thu, 24 Nov 2016 15:34:39 +0800 Message-ID: <5836980F.6050006@rock-chips.com> References: <20161124061416epcms1p44a0152bca14312f1229cab835ea0297f@epcms1p4> <58368C91.8030502@rock-chips.com> <5836927B.9010205@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: Received: from lucky1.263xmail.com ([211.157.147.130]:34161 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751625AbcKXHev (ORCPT ); Thu, 24 Nov 2016 02:34:51 -0500 In-Reply-To: <5836927B.9010205@samsung.com> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Chanwoo Choi , myungjoo.ham@samsung.com Cc: "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" , Tobias Jakobi 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; }; }; so i think my way semm more simple. On 2016年11月24日 15:10, Chanwoo Choi wrote: > + Tobias Jakobi, > > Hi Lin, > > We need to discuss how to support the suspend-opp of devfreq device. > Now, there are two patch thread for suspend-opp of devfreq. > > The Lin's approach modify the devfreq_suspend_device() to support suspend-opp. > The Tobias's approach[1] add new devfreq_suspend() and then call it on dpm_suspend() > when entering the suspend state. > > [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl > - https://patchwork.kernel.org/patch/9443323/ > - https://patchwork.kernel.org/patch/9443325/ > - https://patchwork.kernel.org/patch/9443329/ > - https://patchwork.kernel.org/patch/9443331/ > > I think we need to discuss it together. > > Regards, > Chanwoo Choi > > On 2016년 11월 24일 15:45, hl wrote: >> Hi MyungJoo Ham, >> >> On 2016年11月24日 14:14, MyungJoo Ham wrote: >>> On Thu, Nov 24, 2016 at 11:18 AM, hl wrote: >>>> Hi MyungJoo Ham, >>> [] >>>>> We still need to sync the all status even i call target() in >>>>> devfreq_suspend/resume_device >>>>> directly, so still need update_devfreq() other setp except >>>>> devfreq->governor->get_target_freq(devfreq, &freq); >>>> And i think it better to be governor behaviors, for userspace they may not >>>> want to change >>>> the suspend frequency like other governor, the frequency should decide by >>>> the user, if they >>>> want this function, they should like other governor to rigister a >>>> devfreq_monitor_suspend(). >>>> What do you think about my rev6 patch? >>> If I understand the intention correctly, this is for the stability of >>> the device due to the behavior or bootloader/SoC-initializer, which >>> has nothing to do with governors. >>> >>> Even if users are using userspace, as long as they set the custom >>> frequencies lower than the default, they have the possibility of >>> being unstable as ondemand is going to have. >>> >>> >>> To reuse the update_devfreq() code, you may do something like: >>> >>> static int _update_freq(struct devfreq *devfreq, bool is_suspending) >>> { >>> /* original contents of update_freq with if statement with is_suspending wrapping get_target_freq */ >>> } >>> int update_freq(struct devfreq *devfreq) >>> { >>> return _update_freq(devfreq, false); >>> } >>> >>> >>> There should be other good non-invasive methods that are not governoe-specific as well. >>> >> Thanks for your suggestion, i will update the new version soon. >>> Cheers, >>> MyungJoo >>> >>> >>> >>> >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> -- >> Lin Huang >> > > > -- Lin Huang From mboxrd@z Thu Jan 1 00:00:00 1970 From: hl@rock-chips.com (hl) Date: Thu, 24 Nov 2016 15:34:39 +0800 Subject: [PATCH v1 & v6 1/2] PM/devfreq: add suspend frequency support In-Reply-To: <5836927B.9010205@samsung.com> References: <20161124061416epcms1p44a0152bca14312f1229cab835ea0297f@epcms1p4> <58368C91.8030502@rock-chips.com> <5836927B.9010205@samsung.com> Message-ID: <5836980F.6050006@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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; }; }; so i think my way semm more simple. On 2016?11?24? 15:10, Chanwoo Choi wrote: > + Tobias Jakobi, > > Hi Lin, > > We need to discuss how to support the suspend-opp of devfreq device. > Now, there are two patch thread for suspend-opp of devfreq. > > The Lin's approach modify the devfreq_suspend_device() to support suspend-opp. > The Tobias's approach[1] add new devfreq_suspend() and then call it on dpm_suspend() > when entering the suspend state. > > [1] [RFC 0/4] PM / devfreq: draft for OPP suspend impl > - https://patchwork.kernel.org/patch/9443323/ > - https://patchwork.kernel.org/patch/9443325/ > - https://patchwork.kernel.org/patch/9443329/ > - https://patchwork.kernel.org/patch/9443331/ > > I think we need to discuss it together. > > Regards, > Chanwoo Choi > > On 2016? 11? 24? 15:45, hl wrote: >> Hi MyungJoo Ham, >> >> On 2016?11?24? 14:14, MyungJoo Ham wrote: >>> On Thu, Nov 24, 2016 at 11:18 AM, hl wrote: >>>> Hi MyungJoo Ham, >>> [] >>>>> We still need to sync the all status even i call target() in >>>>> devfreq_suspend/resume_device >>>>> directly, so still need update_devfreq() other setp except >>>>> devfreq->governor->get_target_freq(devfreq, &freq); >>>> And i think it better to be governor behaviors, for userspace they may not >>>> want to change >>>> the suspend frequency like other governor, the frequency should decide by >>>> the user, if they >>>> want this function, they should like other governor to rigister a >>>> devfreq_monitor_suspend(). >>>> What do you think about my rev6 patch? >>> If I understand the intention correctly, this is for the stability of >>> the device due to the behavior or bootloader/SoC-initializer, which >>> has nothing to do with governors. >>> >>> Even if users are using userspace, as long as they set the custom >>> frequencies lower than the default, they have the possibility of >>> being unstable as ondemand is going to have. >>> >>> >>> To reuse the update_devfreq() code, you may do something like: >>> >>> static int _update_freq(struct devfreq *devfreq, bool is_suspending) >>> { >>> /* original contents of update_freq with if statement with is_suspending wrapping get_target_freq */ >>> } >>> int update_freq(struct devfreq *devfreq) >>> { >>> return _update_freq(devfreq, false); >>> } >>> >>> >>> There should be other good non-invasive methods that are not governoe-specific as well. >>> >> Thanks for your suggestion, i will update the new version soon. >>> Cheers, >>> MyungJoo >>> >>> >>> >>> >>> _______________________________________________ >>> Linux-rockchip mailing list >>> Linux-rockchip at lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-rockchip >> -- >> Lin Huang >> > > > -- Lin Huang