On Mon, Sep 29 2014 at 17:37 -0600, Stephen Boyd wrote:

On 09/26/14 17:58, Lina Iyer wrote:

quoted

Based on work by many authors, available at codeaurora.org

quoted

Signed-off-by: Lina Iyer <redacted>
[lina: simplify the driver for an initial submission, add commit text
description of idle states]

Maintainer tags don't really make sense unless there is another author.

Hmm.. Since this patch is a derivative work, I wanted to clarify, what
changed seems important. The work was done by many authors. Adding
signed-off from everybody who could have contributed to the patch
downstream is confusing.
I would be okay removing it.

+

[...]

quoted

+static int qcom_pm_collapse(unsigned long int unused)
+{
+	int ret;
+	u32 flag;
+
+	ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());

Preemption better be off here, so why are we using raw_smp_processor_id()?

True, so raw_ returns without premeption disable, no?

quoted

+	if (ret) {
+		pr_err("Failed to set warm boot address for cpu %d\n",
+				raw_smp_processor_id());

Stuff cpu into a local variable?

Yeah

quoted

+		return ret;
+	}
+
+	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
+	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
+
+	return 0;
+}
+
+/**
+ * qcom_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
+ *
+ * @mode - sleep mode to enter
+ *
+ * The code should be called with interrupts disabled and on the core on
+ * which the low power mode is to be executed.
+ *
+ */
+static int qcom_cpu_pm_enter_sleep(enum pm_sleep_mode mode)
+{
+	int ret;
+
+	switch (mode) {
+	case PM_SLEEP_MODE_SPC:
+		qcom_spm_set_low_power_mode(SPM_MODE_POWER_COLLAPSE);

Isn't it a one-to-one between PM_SLEEP_MODE_SPC and
SPM_MODE_POWER_COLLAPSE? The enum to enum map seems overly complicated.

Not really. SPM modes, differ when the idle state has to notify RPM. It
does not have 2 enums for those modes, but uses an overloaded enum with
an additional flag.

quoted

+		ret = cpu_suspend(0, qcom_pm_collapse);
+		break;
+	default:
+	case PM_SLEEP_MODE_WFI:
+		qcom_spm_set_low_power_mode(SPM_MODE_CLOCK_GATING);
+		ret = cpu_do_idle();
+		break;
+	}
+
+	local_irq_enable();
+
+	return ret;
+}
+
+static struct platform_device qcom_cpuidle_device = {
+	.name              = "qcom_cpuidle",
+	.id                = -1,
+	.dev.platform_data = qcom_cpu_pm_enter_sleep,
+};

This doesn't need to be static. Use platform_device_register_full() instead.

Okay

quoted

+
+static int __init qcom_pm_device_init(void)
+{
+	platform_device_register(&qcom_cpuidle_device);
+
+	return 0;
+}
+device_initcall(qcom_pm_device_init);

modules?

Why? An earlier initialization helps with power saving

quoted

diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
new file mode 100644
index 0000000..563b9c3
--- /dev/null
+++ b/include/soc/qcom/pm.h

@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ */
+
+#ifndef __QCOM_PM_H
+#define __QCOM_PM_H
+
+enum pm_sleep_mode {
+	PM_SLEEP_MODE_WFI,
+	PM_SLEEP_MODE_RET,
+	PM_SLEEP_MODE_SPC,
+	PM_SLEEP_MODE_PC,
+	PM_SLEEP_MODE_NR,
+};
+
+#endif  /* __QCOM_PM_H */

Hopefully this header is not necessary.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation