| Message ID | 1408486537-6358-8-git-send-email-lina.iyer@linaro.org |
|---|---|
| State | New |
| Headers | show |
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@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. > +} > + > +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. > + > + /** > + * 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. > + 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. > + > + 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 ? Thanks -- Daniel
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 21, 2014 at 03:36:43PM +0100, Lina Iyer wrote: [...] > >>+ 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 :) You weren't asked to use entry-method, you were asked to do what psci does, namely defining a per-state parameter. Anyway, we will rely on compatible string to initialize the state enter pointer so that we are all happy again. The driver will pass an array of pairs {compatible, enter_function}, the dt init will initialize the idle state enter pointer accordingly. I still have a feeling that DT parsing code should also return an array to the CPUidle driver corresponding to DT nodes that were used to initialize idle states, this way, if it is needed, the CPUidle driver can parse for each index the platform specific idle state properties. Otherwise we will end up with a function pointer per-state even if two states are almost identical and just differ in the parameter passed. It is more a matter of taste than anything else, I will implement the code discussed with Daniel that will be on the lists at -rc3. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. > >>+} >>+ >>+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. >>+ >>+ /** >>+ * 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. > >>+ 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. > >>+ >>+ 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 ? > >Thanks > -- Daniel > >-- > <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 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. > > > >>+} > >>+ > >>+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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Lina Iyer <lina.iyer@linaro.org> writes: > On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote: >>On 08/20/2014 12:15 AM, Lina Iyer wrote: [...] >> >> 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 the L2 is going into low-power, that means there will be higher latency coming out compared to the architectural WFI, correct? If you have both, and the latency/residency numbers are accurate, the governor is then left to pick the right one. So, what's wrong with having both? Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 27, 2014 at 10:31:26AM -0700, Kevin Hilman wrote: >Lina Iyer <lina.iyer@linaro.org> writes: > >> On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote: >>>On 08/20/2014 12:15 AM, Lina Iyer wrote: > >[...] > >>> >>> 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 the L2 is going into low-power, that means there will be higher >latency coming out compared to the architectural WFI, correct? > Thats right. Architecturally L2 deeper sleep modes (which include L2 logic off) may reset the core. So L2 deeper sleep modes with cores in WFI is not a suggested possibility. >If you have both, and the latency/residency numbers are accurate, the >governor is then left to pick the right one. So, what's wrong with >having both? I agree, but there is very little benefit to doing architectural WFI vs platform WFI in most cases. The residencies are too close and almost the same, doesnt warrant a separate functions for that. There is no thing wrong, just dint see a lot of benefit, thats all. What do you think? > >Kevin > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> +#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); +} + +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" }, + { 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; + } + } + } +} + +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); + + /** + * Default to standard for WFI (index = 0) + * Probe only for other states + */ + ret = dt_init_idle_driver(drv, 1); + 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; + + 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);
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@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