On Fri, Aug 22, 2014 at 04:36:20PM +0100, Lina Iyer wrote:
> 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
> >
> > ^^^^^^^^
> >
> >>the C-State information to the cpuidle framework.
> >>
> >>Signed-off-by: Lina Iyer <lina.iyer@xxxxxxxxxx>
> >>---
> >> 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.
> >
> >>+}
> >>+
> >>+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 ?
> >
> >>+ { 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.
> >
> >>+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.
> >
> Lorenzo: Seems like the dt_init_idle_driver expects to see this mask
> set.
> May be you can check if its NULL and revert to cpu_possible_mask in that
> case. That would avoid drivers creating memory and copying
> cpu_possible_mask in their init functions.
I will do, thanks.
Lorenzo
--
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
