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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx>
Signed-off-by: Mahesh Sivasubramanian <msivasub@xxxxxxxxxxxxxx>
Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
---
 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().

+    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 ?

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 :)

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.

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 ?


--
 <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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux