From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Lezcano Subject: Re: [PATCH v2 07/10] qcom: msm-pm: Add cpu low power mode functions Date: Fri, 15 Aug 2014 02:01:47 +0200 Message-ID: <53ED4DEB.8080308@linaro.org> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-wi0-f171.google.com ([209.85.212.171]:58917 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751251AbaHOABt (ORCPT ); Thu, 14 Aug 2014 20:01:49 -0400 Received: by mail-wi0-f171.google.com with SMTP id hi2so199030wib.10 for ; Thu, 14 Aug 2014 17:01:47 -0700 (PDT) In-Reply-To: <20140814192246.GA36004@ilina-mac.local> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Lina Iyer 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 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 ? >>>>> + 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. 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 =46ollow Linaro: Facebook | Twitter | Blog