Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus

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

On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote:
>On 08/20/2014 12:15 AM, Lina Iyer wrote:
>>Add cpuidle driver interface to allow cpus to go into C-States.
>>Use the cpuidle DT interfacecommon across ARM architectures to provide
>
>                         ^^^^^^^^
>
Thanks, will fix.

>>the C-State information to the cpuidle framework.
>>
>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>---
>>  drivers/cpuidle/Kconfig.arm    |   7 +++
>>  drivers/cpuidle/Makefile       |   1 +
>>  drivers/cpuidle/cpuidle-qcom.c | 119 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 127 insertions(+)
>>  create mode 100644 drivers/cpuidle/cpuidle-qcom.c
>>
>>diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>>index e339c7f..26e31bd 100644
>>--- a/drivers/cpuidle/Kconfig.arm
>>+++ b/drivers/cpuidle/Kconfig.arm
>>@@ -63,3 +63,10 @@ config ARM_MVEBU_V7_CPUIDLE
>>  	depends on ARCH_MVEBU
>>  	help
>>  	  Select this to enable cpuidle on Armada 370, 38x and XP processors.
>>+
>>+config ARM_QCOM_CPUIDLE
>>+	bool "CPU Idle drivers for Qualcomm processors"
>>+	depends on QCOM_PM
>>+	select DT_IDLE_STATES
>>+	help
>>+	  Select this to enable cpuidle for QCOM processors
>>diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>>index 4d177b9..6c222d5 100644
>>--- a/drivers/cpuidle/Makefile
>>+++ b/drivers/cpuidle/Makefile
>>@@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
>>  obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
>>  obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
>>  obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
>>+obj-$(CONFIG_ARM_QCOM_CPUIDLE)		+= cpuidle-qcom.o
>>
>>  ###############################################################################
>>  # MIPS drivers
>>diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
>>new file mode 100644
>>index 0000000..4aae672
>>--- /dev/null
>>+++ b/drivers/cpuidle/cpuidle-qcom.c
>>@@ -0,0 +1,119 @@
>>+/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>+ * Copyright (c) 2014, Linaro Limited.
>>+ *
>>+ * 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/platform_device.h>
>>+#include <linux/of.h>
>>+#include <linux/cpuidle.h>
>>+#include <linux/cpumask.h>
>>+#include <linux/slab.h>
>
>See below, cpumask is not needed, you can remove slab.h and cpumask.h.
>
>>+#include <linux/of_device.h>
>>+
>>+#include <soc/qcom/pm.h>
>>+#include "dt_idle_states.h"
>>+
>>+static enum msm_pm_sleep_mode modes[CPUIDLE_STATE_MAX];
>>+
>>+static int qcom_lpm_enter(struct cpuidle_device *dev,
>>+				struct cpuidle_driver *drv, int index)
>>+{
>>+	return msm_cpu_pm_enter_sleep(modes[index], true);
>
>You should return the index here.
>
Argh. yes.

>>+}
>>+
>>+static struct cpuidle_driver qcom_cpuidle_driver = {
>>+	.name	= "qcom_cpuidle",
>>+	.owner	= THIS_MODULE,
>>+};
>>+
>>+static void parse_state_translations(struct cpuidle_driver *drv)
>>+{
>>+	struct device_node *state_node, *cpu_node;
>>+	const char *mode_name;
>>+	int i, j;
>>+
>>+	struct name_map {
>>+		enum msm_pm_sleep_mode mode;
>>+		char *name;
>>+	};
>>+	static struct name_map c_states[] = {
>>+		{ MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
>>+						"standalone-pc" },
>
>What is standalone-pc and collapse ? Core power down ?
>
I had explained it in the cover letter. But let me add some comments
here or the commit text

>>+		{ MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT, "wfi" },
>>+	};
>>+
>>+	cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
>>+	if (!cpu_node)
>>+		return;
>>+
>>+	/**
>>+	 * Get the state description from idle-state node entry-method
>>+	 * First state is always WFI, per spec.
>>+	 */
>>+	modes[0] = MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT;
>>+	for (i = 1; i < drv->state_count; i++) {
>>+		mode_name = NULL;
>>+		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>>+		of_property_read_string(state_node, "entry-method",
>>+						&mode_name);
>>+		for (j = 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) {
>>+			if (!strcmp(mode_name, c_states[j].name)) {
>>+				modes[i] = c_states[j].mode;
>>+				break;
>>+			}
>>+		}
>>+	}
>>+}
>
>For the function above, I believe we can do better with the 
>idle_dt_init function to prevent to have to reparse the infos.
>
>I had the opportunity to discuss with Lorenzo privately and we found a 
>solution he will submit to solve that.
>
Hmm. Thanks, I was asked to use the entry-method. FWIW, I dont like this
either :)

>>+static int qcom_cpuidle_init(void)
>>+{
>>+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
>>+	int ret;
>>+	int i;
>>+
>>+	drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
>>+	if (!drv->cpumask)
>>+		return -ENOMEM;
>>+	cpumask_copy(drv->cpumask, cpu_possible_mask);
>
>You can get rid of this because the driver does not rely on the 
>multiple driver support. If the drv->cpumask is NULL it will be 
>automatically set to cpu_possible_mask by the cpuidle framework.
>
Ok.

>>+
>>+	/**
>>+	 * Default to standard for WFI (index = 0)
>>+	 * Probe only for other states
>>+	 */
>>+	ret = dt_init_idle_driver(drv, 1);
>
>So IIUC, if you specify the index 1, that means the state[0] will be 
>the default WFI. But you override the callback below in the loop.
>
>I recommend you use the default arm wfi callback but you implement the 
>cpu_do_idle for your platform.
>
Yes, it was intended. I dont want to define two WFI states. The
architectural WFI does not buy us enough compared to WFI that SoC can
do. L2 can go into low power modes when the core is in WFI and for that
I would like to have all WFI's enter SoC framework.

>>+	if (ret < 0) {
>>+		pr_err("%s: failed to initialize idle states\n", __func__);
>>+		goto failed;
>>+	}
>>+
>>+	/* Parse the idle states for C-States on this cpu */
>>+	parse_state_translations(drv);
>>+
>>+	/* Register entry point for all low states */
>>+	for (i = 0; i < drv->state_count; i++)
>>+		drv->states[i].enter = qcom_lpm_enter;
>>+	drv->safe_state_index = 0;
>
>The safe_state index is only used for the couple c-states and as the 
>driver is statically defined, it is pointless to initialize this 
>variable.
>
Will remove.
>>+
>>+	ret = cpuidle_register(drv, NULL);
>>+	if (ret) {
>>+		pr_err("%s: failed to register cpuidle driver\n", __func__);
>>+		goto failed;
>>+	}
>>+
>>+	return 0;
>>+
>>+failed:
>>+	kfree(drv->cpumask);
>>+	return ret;
>>+}
>>+module_init(qcom_cpuidle_init);
>
>Can you stick to the platform_driver paradigm like the other drivers ?
>
I do not have any platform data for the driver to use at this point. I
would not prefer to initialize a platform device from board file and
there isnt a configuration available at this point for me to create a DT
node for this driver. Hence after some discussions, I decided to use the
module_init(). If its just a matter of preference, I would request you
to wait until the entire CPUIdle patch stream plays out. I have a
feeling that I might need to add device properties and would need a
platform driver, legitimately.

>Thanks
>  -- Daniel

Thanks Daniel, for your time between the summits.

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