From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lina Iyer Subject: Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus Date: Fri, 22 Aug 2014 09:36:20 -0600 Message-ID: <20140822153620.GB65503@ilina-mac.local> References: <1408486537-6358-1-git-send-email-lina.iyer@linaro.org> <1408486537-6358-8-git-send-email-lina.iyer@linaro.org> <53F54A5C.3060002@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-pa0-f44.google.com ([209.85.220.44]:35735 "EHLO mail-pa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932097AbaHVPgZ (ORCPT ); Fri, 22 Aug 2014 11:36:25 -0400 Received: by mail-pa0-f44.google.com with SMTP id eu11so16743246pac.31 for ; Fri, 22 Aug 2014 08:36:25 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53F54A5C.3060002@linaro.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Daniel Lezcano Cc: khilman@linaro.org, sboyd@codeaurora.org, davidb@codeaurora.org, galak@codeaurora.org, linux-arm-msm@vger.kernel.org, lorenzo.pieralisi@arm.com, msivasub@codeaurora.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 provid= e > > ^^^^^^^^ > >>the C-State information to the cpuidle framework. >> >>Signed-off-by: Lina Iyer >>--- >> 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.ar= m >>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 processo= rs. >>+ >>+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) +=3D cpuidle-zynq.o >> obj-$(CONFIG_ARM_U8500_CPUIDLE) +=3D cpuidle-ux500.o >> obj-$(CONFIG_ARM_AT91_CPUIDLE) +=3D cpuidle-at91.o >> obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) +=3D cpuidle-exynos.o >>+obj-$(CONFIG_ARM_QCOM_CPUIDLE) +=3D 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 mod= ify >>+ * it under the terms of the GNU General Public License version 2 an= d >>+ * 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 >>+#include >>+#include >>+#include >>+#include >>+#include > >See below, cpumask is not needed, you can remove slab.h and cpumask.h. > >>+#include >>+ >>+#include >>+#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 =3D { >>+ .name =3D "qcom_cpuidle", >>+ .owner =3D 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[] =3D { >>+ { 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 =3D 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] =3D MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT; >>+ for (i =3D 1; i < drv->state_count; i++) { >>+ mode_name =3D NULL; >>+ state_node =3D of_parse_phandle(cpu_node, "cpu-idle-states", i); >>+ of_property_read_string(state_node, "entry-method", >>+ &mode_name); >>+ for (j =3D 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) { >>+ if (!strcmp(mode_name, c_states[j].name)) { >>+ modes[i] =3D c_states[j].mode; >>+ break; >>+ } >>+ } >>+ } >>+} > >For the function above, I believe we can do better with the=20 >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= =20 >solution he will submit to solve that. > >>+static int qcom_cpuidle_init(void) >>+{ >>+ struct cpuidle_driver *drv =3D &qcom_cpuidle_driver; >>+ int ret; >>+ int i; >>+ >>+ drv->cpumask =3D 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=20 >multiple driver support. If the drv->cpumask is NULL it will be=20 >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 tha= t case. That would avoid drivers creating memory and copying cpu_possible_mask in their init functions. >>+ >>+ /** >>+ * Default to standard for WFI (index =3D 0) >>+ * Probe only for other states >>+ */ >>+ ret =3D dt_init_idle_driver(drv, 1); > >So IIUC, if you specify the index 1, that means the state[0] will be=20 >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= =20 >cpu_do_idle for your platform. > >>+ 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 =3D 0; i < drv->state_count; i++) >>+ drv->states[i].enter =3D qcom_lpm_enter; >>+ drv->safe_state_index =3D 0; > >The safe_state index is only used for the couple c-states and as the=20 >driver is statically defined, it is pointless to initialize this=20 >variable. > >>+ >>+ ret =3D 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 ? > >Thanks > -- Daniel > >--=20 > Linaro.org =E2=94=82 Open source software fo= r ARM SoCs > >Follow Linaro: Facebook | > Twitter | > Blog >