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 13:22:46 -0600 Message-ID: <20140814192246.GA36004@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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-pa0-f51.google.com ([209.85.220.51]:56016 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752007AbaHNTWy (ORCPT ); Thu, 14 Aug 2014 15:22:54 -0400 Received: by mail-pa0-f51.google.com with SMTP id ey11so2148705pad.24 for ; Thu, 14 Aug 2014 12:22:53 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53ECDFBF.5060908@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 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 >>>>--- >>>> drivers/soc/qcom/Makefile | 2 +- >>>> drivers/soc/qcom/msm-pm.c | 219 >>>>++++++++++++++++++++++++++++++++++++++++++++++ >>>> include/soc/qcom/pm.h | 39 +++++++++ >>>> 3 files changed, 259 insertions(+), 1 deletion(-) >>>> create mode 100644 drivers/soc/qcom/msm-pm.c >>>> create mode 100644 include/soc/qcom/pm.h >>>> >>>>diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile >>>>index d7ae93b..7925f83 100644 >>>>--- a/drivers/soc/qcom/Makefile >>>>+++ b/drivers/soc/qcom/Makefile >>>>@@ -1,5 +1,5 @@ >>>> obj-$(CONFIG_QCOM_GSBI) +=3D qcom_gsbi.o >>>>-obj-$(CONFIG_QCOM_PM) +=3D spm-devices.o spm.o >>>>+obj-$(CONFIG_QCOM_PM) +=3D spm-devices.o spm.o msm-pm.o >>>> >>>> CFLAGS_scm.o :=3D$(call as-instr,.arch_extension sec,-DREQUIRES_SE= C=3D1) >>>> obj-$(CONFIG_QCOM_SCM) +=3D scm.o scm-boot.o >>>>diff --git a/drivers/soc/qcom/msm-pm.c b/drivers/soc/qcom/msm-pm.c >>>>new file mode 100644 >>>>index 0000000..f2f15b8 >>>>--- /dev/null >>>>+++ b/drivers/soc/qcom/msm-pm.c >>>>@@ -0,0 +1,219 @@ >>>>+/* Copyright (c) 2010-2014, The Linux Foundation. All rights reser= ved. >>>>+ * >>>>+ * This program is free software; you can redistribute it and/or m= odify >>>>+ * it under the terms of the GNU General Public License version 2 = and >>>>+ * only version 2 as published by the Free Software Foundation. >>>>+ * >>>>+ * This program is distributed in the hope that it will be useful, >>>>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>+ * GNU General Public License for more details. >>>>+ * >>>>+ */ >>>>+ >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+ >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+ >>>>+#include >>>>+#include >>>>+#include >>>>+#include >>>>+ >>>>+#define SCM_CMD_TERMINATE_PC (0x2) >>>>+#define SCM_CMD_CORE_HOTPLUGGED (0x10) >>>>+#define SCM_FLUSH_FLAG_MASK (0x3) >>>>+ >>>>+static bool msm_pm_is_L1_writeback(void) >>>>+{ >>>>+ u32 cache_id =3D 0; >>>>+ >>>>+#if defined(CONFIG_CPU_V7) >>>>+ u32 sel =3D 0; >>>>+ >>>>+ asm volatile ("mcr p15, 2, %[ccselr], c0, c0, 0\n\t" >>>>+ "isb\n\t" >>>>+ "mrc p15, 1, %[ccsidr], c0, c0, 0\n\t" >>>>+ :[ccsidr]"=3Dr" (cache_id) >>>>+ :[ccselr]"r" (sel) >>>>+ ); >>>>+ return cache_id & BIT(30); >>>>+#elif defined(CONFIG_ARM64) >>>>+ u32 sel =3D 0; >>>>+ asm volatile("msr csselr_el1, %[ccselr]\n\t" >>>>+ "isb\n\t" >>>>+ "mrs %[ccsidr],ccsidr_el1\n\t" >>>>+ :[ccsidr]"=3Dr" (cache_id) >>>>+ :[ccselr]"r" (sel) >>>>+ ); >>>>+ return cache_id & BIT(30); >>>>+#else >>>>+#error No valid CPU arch selected >>>>+#endif >>>>+} >>>>+ >>>>+static inline void msm_arch_idle(void) >>>>+{ >>>>+ /* Flush and clock-gate */ >>>>+ mb(); >>> >>>Why is needed this memory barrier ? >>Some QCOM SoCs needed this. I am not sure which one anymore. :( > >I guess this is to flush the L1 cache when the core is going down.=20 >Regarding the kernel option, it seems mb() is as dsb(), so I am=20 >wondering if this function could be simply replaced by cpu_do_idle(). > Possibly could. I will do that. >>>>+ wfi(); >>>>+} >>>>+ >>>>+static bool msm_pm_swfi(bool from_idle) >>>>+{ >>>>+ msm_arch_idle(); >>>>+ return true; >>>>+} > >Same here, could be replaced by cpu_do_idle(), I think. > >>>>+static bool msm_pm_retention(bool from_idle) >>>>+{ >>>>+ int ret =3D 0; >>>>+ >>>>+ ret =3D msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, fal= se); >>>>+ 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 the >>>retention mode ? >>So if the SPM did not reset to clockgating, we would not do retention >>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 keeps=20 >the last state configuration and we have to set it back to clock=20 >gating, right ? Correct. > >I don't think it is up to this function to do this but the clock=20 >gating function. > >Also, this function prototype looks a bit weird. Just for the sake of=20 >using callbacks. > >And finally, the WARN_ON is not desirable here, except if the goal is=20 >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=20 >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=20 >patches bringing the clock gating, then another patchset with the=20 >retention mode, etc ... > >>>>+ 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 you >>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 dow= n without flushing the caches. Warm boot of the cpu resets the L2 logic as well. The cluster core is lot more complex than this :) > > >--=20 > Linaro.org =E2=94=82 Open source software fo= r ARM SoCs > >Follow Linaro: Facebook | > Twitter | > Blog >