Re: [PATCH v2 07/10] qcom: msm-pm: Add cpu low power mode functions

[Lina Iyer <lina.iyer@linaro.org>]

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 <vdevaras@codeaurora.org>
>>>>Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
>>>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>>---
>>>> 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)    +=    qcom_gsbi.o
>>>>-obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o
>>>>+obj-$(CONFIG_QCOM_PM) +=    spm-devices.o spm.o msm-pm.o
>>>>
>>>> CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
>>>> obj-$(CONFIG_QCOM_SCM) += 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 reserved.
>>>>+ *
>>>>+ * This program is free software; you can redistribute it and/or modify
>>>>+ * 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 <linux/module.h>
>>>>+#include <linux/kernel.h>
>>>>+#include <linux/init.h>
>>>>+#include <linux/io.h>
>>>>+#include <linux/smp.h>
>>>>+#include <linux/tick.h>
>>>>+#include <linux/platform_device.h>
>>>>+#include <linux/cpu_pm.h>
>>>>+#include <linux/uaccess.h>
>>>>+
>>>>+#include <soc/qcom/spm.h>
>>>>+#include <soc/qcom/pm.h>
>>>>+#include <soc/qcom/scm.h>
>>>>+#include <soc/qcom/scm-boot.h>
>>>>+
>>>>+#include <asm/suspend.h>
>>>>+#include <asm/cacheflush.h>
>>>>+#include <asm/cputype.h>
>>>>+#include <asm/system_misc.h>
>>>>+
>>>>+#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 = 0;
>>>>+
>>>>+#if defined(CONFIG_CPU_V7)
>>>>+    u32 sel = 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]"=r" (cache_id)
>>>>+              :[ccselr]"r" (sel)
>>>>+             );
>>>>+    return cache_id & BIT(30);
>>>>+#elif defined(CONFIG_ARM64)
>>>>+    u32 sel = 0;
>>>>+    asm volatile("msr csselr_el1, %[ccselr]\n\t"
>>>>+             "isb\n\t"
>>>>+             "mrs %[ccsidr],ccsidr_el1\n\t"
>>>>+             :[ccsidr]"=r" (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. 
>Regarding the kernel option, it seems mb() is as dsb(), so I am 
>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 = 0;
>>>>+
>>>>+    ret = msm_spm_set_low_power_mode(MSM_SPM_MODE_RETENTION, false);
>>>>+    WARN_ON(ret);
>>>>+
>>>>+    msm_arch_idle();
>>>>+
>>>>+    ret = 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 
>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 of 
>using callbacks.
>
>And finally, the WARN_ON is not desirable here, except if the goal is 
>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 ...
>
>>>>+    return true;
>>>>+}
>>>>+
>>>>+static int msm_pm_collapse(unsigned long from_idle)
>>>>+{
>>>>+    enum msm_pm_l2_scm_flag flag = 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() == 1)
>>>>+        flag = 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 down
without flushing the caches. Warm boot of the cpu resets the L2
logic as well. The cluster core is lot more complex than this :)

>
>
>-- 
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>
>Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
><http://twitter.com/#!/linaroorg> Twitter |
><http://www.linaro.org/linaro-blog/> Blog
>