From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Lezcano Subject: Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus Date: Thu, 21 Aug 2014 03:24:44 +0200 Message-ID: <53F54A5C.3060002@linaro.org> References: <1408486537-6358-1-git-send-email-lina.iyer@linaro.org> <1408486537-6358-8-git-send-email-lina.iyer@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-ig0-f172.google.com ([209.85.213.172]:63989 "EHLO mail-ig0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750809AbaHUBYr (ORCPT ); Wed, 20 Aug 2014 21:24:47 -0400 Received: by mail-ig0-f172.google.com with SMTP id h15so12582889igd.11 for ; Wed, 20 Aug 2014 18:24:46 -0700 (PDT) In-Reply-To: <1408486537-6358-8-git-send-email-lina.iyer@linaro.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Lina Iyer , khilman@linaro.org, sboyd@codeaurora.org, davidb@codeaurora.org, galak@codeaurora.org, linux-arm-msm@vger.kernel.org, lorenzo.pieralisi@arm.com Cc: msivasub@codeaurora.org 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; > + } > + } > + } > +} =46or the function above, I believe we can do better with the idle_dt_i= nit=20 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 multipl= e=20 driver support. If the drv->cpumask is NULL it will be automatically se= t=20 to cpu_possible_mask by the cpuidle framework. > + > + /** > + * 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 th= e=20 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 variab= le. > + > + 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 =46ollow Linaro: Facebook | Twitter | Blog