From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lina Iyer Subject: Re: [PATCH v2 07/10] qcom: msm-pm: Add cpu low power mode functions Date: Thu, 14 Aug 2014 19:02:38 -0600 Message-ID: <20140815010238.GA43455@ilina-mac.local> References: <1407872640-6732-1-git-send-email-lina.iyer@linaro.org> <1407872640-6732-8-git-send-email-lina.iyer@linaro.org> <53EB4969.3030204@linaro.org> <20140813141650.GC26905@ilina-mac> <53ECDFBF.5060908@linaro.org> <20140814192246.GA36004@ilina-mac.local> <53ED4DEB.8080308@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-pd0-f176.google.com ([209.85.192.176]:52087 "EHLO mail-pd0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750869AbaHOBCz (ORCPT ); Thu, 14 Aug 2014 21:02:55 -0400 Received: by mail-pd0-f176.google.com with SMTP id y10so2476815pdj.7 for ; Thu, 14 Aug 2014 18:02:55 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53ED4DEB.8080308@linaro.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Daniel Lezcano Cc: khilman@linaro.org, amit.kucheria@linaro.org, sboyd@codeaurora.org, davidb@codeaurora.org, galak@codeaurora.org, linux-arm-msm@vger.kernel.org, msivasub@codeaurora.org, Venkat Devarasetty , Nicolas Pitre On Fri, Aug 15, 2014 at 02:01:47AM +0200, Daniel Lezcano wrote: >On 08/14/2014 09:22 PM, Lina Iyer wrote: >>On Thu, Aug 14, 2014 at 06:11:43PM +0200, Daniel Lezcano wrote: >>>On 08/13/2014 04:16 PM, Lina Iyer wrote: >>>>On Wed, Aug 13, 2014 at 01:18:01PM +0200, Daniel Lezcano wrote: >>>>>On 08/12/2014 09:43 PM, Lina Iyer wrote: >>>>>>Add interface layer to abstract and handle hardware specific >>>>>>functionality for executing various cpu low power modes in QCOM >>>>>>chipsets. >>>>>> >>>>>>Signed-off-by: Venkat Devarasetty >>>>>>Signed-off-by: Mahesh Sivasubramanian >>>>>>Signed-off-by: Lina Iyer >>>>>>--- > >[ ... ] > >>>>>>+static bool msm_pm_retention(bool from_idle) >>>>>>+{ >>>>>>+ int ret =3D 0; >>>>>>+ >>>>>>+ ret =3D msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, f= alse); >>>>>>+ WARN_ON(ret); >>>>>>+ >>>>>>+ msm_arch_idle(); >>>>>>+ >>>>>>+ ret =3D msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING= , >>>>>>false); >>>>>>+ WARN_ON(ret); >>>>> >>>>>Why do you need to set the clock gating mode each time you exit th= e >>>>>retention mode ? >>>>So if the SPM did not reset to clockgating, we would not do retenti= on >>>>when we intended to do clockgating. Btw, we dont set clockgating >>>>everytime we do clockgating, helps reduce the latency in doing WFI. >>> >>>Thanks for the explanation in the other email. So IIUC, the SCM keep= s >>>the last state configuration and we have to set it back to clock >>>gating, right ? >>Correct. >>> >>>I don't think it is up to this function to do this but the clock >>>gating function. >>> >>>Also, this function prototype looks a bit weird. Just for the sake o= f >>>using callbacks. >>> >>>And finally, the WARN_ON is not desirable here, except if the goal i= s >>>to flood the terminal :) >>Was debating the use of it myself. Will remove it. >>> >>>What not using first simple functions ? >>> >>>void qcom_do_idle(void) >>>{ >>> myfirmware_call(MSM_SPM_MODE_CLOCK_GATING); >>> wfi(); >>>} >>> >>>void qcom_cpu_retention(void) >>>{ >>> myfirmware_call(MSM_SPM_MODE_RETENTION); >>> dsb(); >>> wfi(); >>>} >>> >>>void qcom_cpu_powerdown(int flags) >>>{ >>> scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag); >>>} >>> >>>and then you build on top of that the cpuidle driver. >>Okay. Will do this >>> >>>The patchset adds all the features in one shot and for someone not >>>used with the platform it is really hard to follow all the code. >>> >>>I suggest you write a simple cpuidle driver based on the DT Lorenzo >>>patches bringing the clock gating, then another patchset with the >>>retention mode, etc ... >>> > >Do you agree with this approach ? Yes, have a working patch with the code cleaned out as you suggested. Looking at Lorenzo's DT changes and a few SPM driver clean ups. Will submit a patch revision soon. > >>>>>>+ return true; >>>>>>+} >>>>>>+ >>>>>>+static int msm_pm_collapse(unsigned long from_idle) >>>>>>+{ >>>>>>+ enum msm_pm_l2_scm_flag flag =3D MSM_SCM_L2_ON; >>>>>>+ >>>>>>+ /** >>>>>>+ * Single core processors need to have L2 >>>>>>+ * flushed when powering down the core. >>>>>>+ * Notify SCM to flush secure L2 lines. >>>>>>+ */ >>>>>>+ if (num_possible_cpus() =3D=3D 1) >>>>>>+ flag =3D MSM_SCM_L2_OFF; >>>>> >>>>>I am wondering if this shouldn't be handle by a mcpm driver. >>>>> >>>>>Cc nico. >>>> >>>>Well, possibly, sorry, not sure what features of the mcpm driver yo= u >>>>think I need here? >>> >>>Please correct me if I am wrong. IIUC, this function is checking the >>>number of the cpus of the cluster in order to flush the L2 cache >>>because the SCM will power down the cluster if it is the last one, >>>right ? >>Nope. Some QCOM variants which have a single CPU, cannot be powered d= own >>without flushing the caches. Warm boot of the cpu resets the L2 >>logic as well. The cluster core is lot more complex than this :) > >Ok, probably we can discuss this later when we reach this state in the= =20 >incremental implementation. Hmm, yeah. > >Thanks > > -- Daniel > >>>-- >>> Linaro.org =E2=94=82 Open source software f= or ARM SoCs >>> >>>Follow Linaro: Facebook | >>> Twitter | >>> Blog >>> > > >--=20 > Linaro.org =E2=94=82 Open source software fo= r ARM SoCs > >Follow Linaro: Facebook | > Twitter | > Blog >