| Message ID | 1411779495-39724-6-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27-09-2014 06:28 AM, Lina Iyer wrote: .. > +} > + > +static struct platform_driver qcom_cpuidle_plat_driver = { > + .probe = qcom_cpuidle_probe, > + .driver = { > + .name = "qcom_cpuidle", > + .owner = THIS_MODULE, .owner field can be removed here, as for drivers which use the module_platform_driver API, this is overriden in platform_driver_register anyway. > + }, > +}; > + > +module_platform_driver(qcom_cpuidle_plat_driver); >
Hi Lina One sections mismatch warning here as well: WARNING: drivers/cpuidle/built-in.o(.text+0x2740): Section mismatch in reference from the function qcom_cpuidle_probe() to the (unknown reference) .init.rodata:(unknown) The function qcom_cpuidle_probe() references the (unknown reference) __initconst (unknown). This is often because qcom_cpuidle_probe lacks a __initconst annotation or the annotation of (unknown) is wrong. On Saturday 27 September 2014 06:28 AM, Lina Iyer wrote: > Add cpuidle driver interface to allow cpus to go into C-States. Use the > cpuidle DT interface common across ARM architectures to provide the > C-State information to the cpuidle framework. > > .. > +}; > + > +static const struct of_device_id qcom_idle_state_match[] __initconst = { This is causing it. > + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, > + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, > + { }, > +}; > + Thanks Pramod
On Mon, Sep 29 2014 at 04:27 -0600, Pramod Gurav wrote: >Hi Lina > >One sections mismatch warning here as well: > >WARNING: drivers/cpuidle/built-in.o(.text+0x2740): Section mismatch in >reference from the function qcom_cpuidle_probe() to the (unknown >reference) .init.rodata:(unknown) >The function qcom_cpuidle_probe() references >the (unknown reference) __initconst (unknown). >This is often because qcom_cpuidle_probe lacks a __initconst >annotation or the annotation of (unknown) is wrong. > Thanks, will fix it. >On Saturday 27 September 2014 06:28 AM, Lina Iyer wrote: >> Add cpuidle driver interface to allow cpus to go into C-States. Use the >> cpuidle DT interface common across ARM architectures to provide the >> C-State information to the cpuidle framework. >> >> > >.. > >> +}; >> + >> +static const struct of_device_id qcom_idle_state_match[] __initconst = { >This is causing it. > >> + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, >> + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, >> + { }, >> +}; >> + >Thanks >Pramod >
Hi Lina, On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote: > Add cpuidle driver interface to allow cpus to go into C-States. Use the > cpuidle DT interface common across ARM architectures to provide the > C-State information to the cpuidle framework. > > Supported modes at this time are clock gating (wfi) and cpu power down > (Standalone PC or spc). > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > .../bindings/arm/msm/qcom,idle-state.txt | 72 +++++++++++++++++ > drivers/cpuidle/Kconfig.arm | 7 ++ > drivers/cpuidle/Makefile | 1 + > drivers/cpuidle/cpuidle-qcom.c | 89 ++++++++++++++++++++++ > 4 files changed, 169 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > create mode 100644 drivers/cpuidle/cpuidle-qcom.c > > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > new file mode 100644 > index 0000000..47095b9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > @@ -0,0 +1,72 @@ > +QCOM Idle States for cpuidle driver > + > +ARM provides idle-state node to define the cpuidle states, as defined in [1]. > +cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle > +states. Idle states have different enter/exit latency and residency values. > +The idle states supported by the QCOM SoC are defined as - > + > + * WFI > + * Retention > + * Standalone Power Collapse (Standalone PC or SPC) > + * Power Collapse (PC) > + > +WFI: WFI does a little more in addition to architectural clock gating. ARM This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is. > +processors when execute the wfi instruction will gate their internal clocks. > +QCOM cpus use this instruction as a trigger for the SPM state machine. Usually > +with a cpu entering WFI, the SPM is configured to do clock-gating as well. The > +SPM state machine waits for the interrrupt to trigger the core back in to s/interrrupt/interrupt/ > +active. When all CPUs in the SoC, clock gate using the ARM wfi instruction, the > +second level cache usually can also clock gate sensing no cpu activity. When a > +cpu is ready to run, it needs the cache to be active before starting execution. > +Allowing the SPM to execute the clock gating statemachine and waiting for s/statemachine/state machine/ You are defining a generic binding for Qualcomm idle states, so it should not be tied to a specific cache level (ie L2 gating), otherwise if the same state shows up in a future system with L3 you are back to square one. "Platform WFI" or something like that ? You got what I mean. > +interrupt on behalf of the processor has a benefit of guaranteeing that the > +system state is conducive for the core to resume execution. > + > +Retention: Retention is a low power state where the core is clockgated and the > +memory and the registers associated with the core are retained. The voltage > +may be reduced to the minimum value needed to keep the processor registers > +active. Retention is triggered when the core executes wfi instruction. The SPM I think that saying "..is triggered when the core executes wfi instruction" is not necessary. I am not aware of any other _proper_ way of entering an ARM idle state other than executing wfi and I hope I will never have to to become aware of one. > +should be configured to execute the retention sequence and would wait for > +interrupt, before restoring the cpu to execution state. Retention may have a > +slightly higher latency than WFI. > + > +Standalone PC: A cpu can power down and warmboot if there is a sufficient time > +between now and the next know wake up. SPC mode is used to indicate a core s/know/known/ > +entering a power down state without consulting any other cpu or the system > +resources. This helps save power only on that core. Like WFI and Retention, the > +core executes wfi and the SPM programmed to do SPC would use the cpu control > +logic to power down the core's supply and restore it back when woken up by an > +interrupt. Applying power and reseting the core causes the core to warmboot s/reseting/resetting/ > +back into secure mode which trampolines the control back to the kernel. To > +enter a power down state the kernel needs to call into the secure layer which > +would then execute the ARM wfi instruction. Failing to do so, would result in a > +crash enforced by the warm boot code in the secure layer. On a SoC with > +write-back L1 cache, the cache would need to be flushed. > +Power Collapse: This state is similiar to the SPC mode, but distinguishes s/similiar/similar/ > +itself in the fact that the cpu acknowledges and permits the SoC to enter s/in the fact that/in that/ > +deeper sleep modes. In a hierarchical power domain SoC, this means L2 and other > +caches can be flushed, system bus, clocks - lowered, and SoC main XO turned off > +and voltages reduced, provided all cpus enter this state. In other words, it is > +a coupled idle state. Since the span of low power modes possible at this state Careful with the wording here. "Coupled" idle states are defined in the kernel for systems where the CPUs must enter power down with a specific ordering. I do not think "Power Collapse" is a "coupled" state from this perspective, it seems to me more like a "cluster" state, a state that is entered when all (not necessarily coordinated) CPUs in the cluster enter that state. Feel free to call it a hierarchical state, if it makes sense to you. > +is vast, the exit latency and the residency of this low power mode would be > +considered high even though at a cpu level, this essentially is cpu power down. > +The SPM in this state also may handshake with the Resource power manager > +processor in the SoC to indicate a complete subsystem shut down. > + > +The idle-state for QCOM SoCs are distinguished by the compatible property of > +the node. They indicate to the cpuidle driver the entry point to use for What node ? Please be specific. Moreover, the compatible string has a standard DT meaning and it does not indicate anything. This is a DT idle states binding and that's where it should stop, anything else is a kernel implementation detail, or put it differently, it must not define how the kernel translates that compatible property into data structures/control code. > +cpuidle. The devicetree representation of the idle state should be - > + > +Required properties: > + > +- compatible: Must be "arm,idle-state" > + and one of - > + "qcom,idle-state-wfi", > + "qcom,idle-state-ret", > + "qcom,idle-state-spc", > + "qcom,idle-state-pc", > + > +Other required and optional properties are specified in [1]. > + > +[1]. Documentation/devicetree/bindings/arm/idle-states.txt > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm > index 38cff69..6a9ee12 100644 > --- a/drivers/cpuidle/Kconfig.arm > +++ b/drivers/cpuidle/Kconfig.arm > @@ -62,3 +62,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..2fcf79a > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-qcom.c > @@ -0,0 +1,89 @@ > +/* > + * 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/cpu_pm.h> > +#include <linux/cpuidle.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > + > +#include <soc/qcom/pm.h> > +#include "dt_idle_states.h" > + > +static void (*qcom_idle_enter)(enum pm_sleep_mode); > + > +static int qcom_lpm_enter_wfi(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + qcom_idle_enter(PM_SLEEP_MODE_WFI); > + > + return index; > +} > + > +static int qcom_lpm_enter_spc(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + cpu_pm_enter(); > + qcom_idle_enter(PM_SLEEP_MODE_SPC); > + cpu_pm_exit(); > + > + return index; > +} > + > +static struct cpuidle_driver qcom_cpuidle_driver = { > + .name = "qcom_cpuidle", > + .owner = THIS_MODULE, > +}; > + > +static const struct of_device_id qcom_idle_state_match[] __initconst = { If it can be built as a module, __initconst should be removed (and that's true for my Exynos patch too). > + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, > + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, > + { }, > +}; > + > +static int qcom_cpuidle_probe(struct platform_device *pdev) > +{ > + struct cpuidle_driver *drv = &qcom_cpuidle_driver; > + int ret; > + > + qcom_idle_enter = (void *)(pdev->dev.platform_data); Casting a void* to a void* does not seem that useful to me, and that's valid for other CPUidle drivers too, the cast can be removed unless you explain to me what it is for. Lorenzo > + if (!qcom_idle_enter) > + return -EFAULT; > + > + /* Probe for other states including platform WFI */ > + ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0); > + if (ret <= 0) { > + pr_err("%s: No cpuidle state found.\n", __func__); > + return ret; > + } > + > + ret = cpuidle_register(drv, NULL); > + if (ret) { > + pr_err("%s: failed to register cpuidle driver\n", __func__); > + return ret; > + } > + > + return 0; > +} > + > +static struct platform_driver qcom_cpuidle_plat_driver = { > + .probe = qcom_cpuidle_probe, > + .driver = { > + .name = "qcom_cpuidle", > + .owner = THIS_MODULE, > + }, > +}; > + > +module_platform_driver(qcom_cpuidle_plat_driver); > -- > 1.9.1 > >
On Mon, Sep 29 2014 at 09:31 -0600, Lorenzo Pieralisi wrote: >Hi Lina, > >On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote: >> +The idle states supported by the QCOM SoC are defined as - >> + >> + * WFI >> + * Retention >> + * Standalone Power Collapse (Standalone PC or SPC) >> + * Power Collapse (PC) >> + >> +WFI: WFI does a little more in addition to architectural clock gating. ARM > >This may be misleading. Call it PlatformWFI or something like that, not WFI if >that's not what it is. > Hmm.. Not elegant. Let me see what I can do. >> +processors when execute the wfi instruction will gate their internal clocks. >> +QCOM cpus use this instruction as a trigger for the SPM state machine. Usually >> +with a cpu entering WFI, the SPM is configured to do clock-gating as well. The >> +SPM state machine waits for the interrrupt to trigger the core back in to > >s/interrrupt/interrupt/ > >> +active. When all CPUs in the SoC, clock gate using the ARM wfi instruction, the >> +second level cache usually can also clock gate sensing no cpu activity. When a >> +cpu is ready to run, it needs the cache to be active before starting execution. >> +Allowing the SPM to execute the clock gating statemachine and waiting for > >s/statemachine/state machine/ > >You are defining a generic binding for Qualcomm idle states, so it should >not be tied to a specific cache level (ie L2 gating), otherwise if the >same state shows up in a future system with L3 you are back to square >one. > >"Platform WFI" or something like that ? You got what I mean. > I am not, I am just explaining the difference between Architectural and Platform WFI and how the WFI on the core, can help L2 enter shallower idle states, which is true for L3 as well (provided there is enough time and we are not allowed to do power down states). >> +interrupt on behalf of the processor has a benefit of guaranteeing that the >> +system state is conducive for the core to resume execution. >> + >> +Retention: Retention is a low power state where the core is clockgated and the >> +memory and the registers associated with the core are retained. The voltage >> +may be reduced to the minimum value needed to keep the processor registers >> +active. Retention is triggered when the core executes wfi instruction. The SPM > >I think that saying "..is triggered when the core executes wfi instruction" >is not necessary. I am not aware of any other _proper_ way of entering >an ARM idle state other than executing wfi and I hope I will never have to >to become aware of one. > Hopefully so :) >> +should be configured to execute the retention sequence and would wait for >> +interrupt, before restoring the cpu to execution state. Retention may have a >> +slightly higher latency than WFI. >> + >> +Standalone PC: A cpu can power down and warmboot if there is a sufficient time >> +between now and the next know wake up. SPC mode is used to indicate a core > >s/know/known/ > >> +entering a power down state without consulting any other cpu or the system >> +resources. This helps save power only on that core. Like WFI and Retention, the >> +core executes wfi and the SPM programmed to do SPC would use the cpu control >> +logic to power down the core's supply and restore it back when woken up by an >> +interrupt. Applying power and reseting the core causes the core to warmboot > >s/reseting/resetting/ > >> +back into secure mode which trampolines the control back to the kernel. To >> +enter a power down state the kernel needs to call into the secure layer which >> +would then execute the ARM wfi instruction. Failing to do so, would result in a >> +crash enforced by the warm boot code in the secure layer. On a SoC with >> +write-back L1 cache, the cache would need to be flushed. >> +Power Collapse: This state is similiar to the SPC mode, but distinguishes > >s/similiar/similar/ > >> +itself in the fact that the cpu acknowledges and permits the SoC to enter > >s/in the fact that/in that/ > >> +deeper sleep modes. In a hierarchical power domain SoC, this means L2 and other >> +caches can be flushed, system bus, clocks - lowered, and SoC main XO turned off >> +and voltages reduced, provided all cpus enter this state. In other words, it is >> +a coupled idle state. Since the span of low power modes possible at this state > >Careful with the wording here. "Coupled" idle states are defined in the >kernel for systems where the CPUs must enter power down with a specific >ordering. I do not think "Power Collapse" is a "coupled" state from this >perspective, it seems to me more like a "cluster" state, a state that is >entered when all (not necessarily coordinated) CPUs in the cluster enter >that state. Feel free to call it a hierarchical state, if it makes sense >to you. > Okay. I thought I changed it from coupled to cluster.. I will change it to cluster. >> +is vast, the exit latency and the residency of this low power mode would be >> +considered high even though at a cpu level, this essentially is cpu power down. >> +The SPM in this state also may handshake with the Resource power manager >> +processor in the SoC to indicate a complete subsystem shut down. >> + >> +The idle-state for QCOM SoCs are distinguished by the compatible property of >> +the node. They indicate to the cpuidle driver the entry point to use for > >What node ? Please be specific. Moreover, the compatible string has a >standard DT meaning and it does not indicate anything. This is a DT idle states >binding and that's where it should stop, anything else is a kernel >implementation detail, or put it differently, it must not define how the >kernel translates that compatible property into data structures/control >code. > Hmm. I'll take it out. >> +cpuidle. The devicetree representation of the idle state should be - >> + >> +Required properties: >> + >> +- compatible: Must be "arm,idle-state" >> + and one of - >> + "qcom,idle-state-wfi", >> + "qcom,idle-state-ret", >> + "qcom,idle-state-spc", >> + "qcom,idle-state-pc", >> + >> +static struct cpuidle_driver qcom_cpuidle_driver = { >> + .name = "qcom_cpuidle", >> + .owner = THIS_MODULE, >> +}; >> + >> +static const struct of_device_id qcom_idle_state_match[] __initconst = { > >If it can be built as a module, __initconst should be removed (and >that's true for my Exynos patch too). > Yes, fixing it. >> + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, >> + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, >> + { }, >> +}; >> + >> +static int qcom_cpuidle_probe(struct platform_device *pdev) >> +{ >> + struct cpuidle_driver *drv = &qcom_cpuidle_driver; >> + int ret; >> + >> + qcom_idle_enter = (void *)(pdev->dev.platform_data); > >Casting a void* to a void* does not seem that useful to me, and that's valid >for other CPUidle drivers too, the cast can be removed unless you explain >to me what it is for. > Sure, I dont need it. Thanks for your time, Lorenzo. Lina
On Mon, Sep 29, 2014 at 05:16:25PM +0100, Lina Iyer wrote: [...] > >> +processors when execute the wfi instruction will gate their internal clocks. > >> +QCOM cpus use this instruction as a trigger for the SPM state machine. Usually > >> +with a cpu entering WFI, the SPM is configured to do clock-gating as well. The > >> +SPM state machine waits for the interrrupt to trigger the core back in to > > > >s/interrrupt/interrupt/ > > > >> +active. When all CPUs in the SoC, clock gate using the ARM wfi instruction, the > >> +second level cache usually can also clock gate sensing no cpu activity. When a > >> +cpu is ready to run, it needs the cache to be active before starting execution. > >> +Allowing the SPM to execute the clock gating statemachine and waiting for > > > >s/statemachine/state machine/ > > > >You are defining a generic binding for Qualcomm idle states, so it should > >not be tied to a specific cache level (ie L2 gating), otherwise if the > >same state shows up in a future system with L3 you are back to square > >one. > > > >"Platform WFI" or something like that ? You got what I mean. > > > I am not, I am just explaining the difference between Architectural and > Platform WFI and how the WFI on the core, can help L2 enter shallower > idle states, which is true for L3 as well (provided there is enough time > and we are not allowed to do power down states). I wanted to say that instead of referring to L2 you can refer to the computing system as a whole, it is a computing subsystem clock gating. Instead of referring to L2, refer to cache hierarchy, generically. It is just a nitpick to make your life easier in the future. [...] > >> +static int qcom_cpuidle_probe(struct platform_device *pdev) > >> +{ > >> + struct cpuidle_driver *drv = &qcom_cpuidle_driver; > >> + int ret; > >> + > >> + qcom_idle_enter = (void *)(pdev->dev.platform_data); > > > >Casting a void* to a void* does not seem that useful to me, and that's valid > >for other CPUidle drivers too, the cast can be removed unless you explain > >to me what it is for. > > > Sure, I dont need it. > > Thanks for your time, Lorenzo. You are welcome, thanks for you patience in converting the driver to the new DT API. Lorenzo
On 09/26/14 17:58, Lina Iyer wrote: > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c > new file mode 100644 > index 0000000..2fcf79a > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-qcom.c > @@ -0,0 +1,89 @@ > +/* > + * 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/cpu_pm.h> > +#include <linux/cpuidle.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > + > +#include <soc/qcom/pm.h> > +#include "dt_idle_states.h" > + > +static void (*qcom_idle_enter)(enum pm_sleep_mode); > + > +static int qcom_lpm_enter_wfi(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + qcom_idle_enter(PM_SLEEP_MODE_WFI); Why can't we just pass index here? It would be nice to not need to include soc/qcom/pm.h in this file. > + > + return index; > +} > + > +static int qcom_lpm_enter_spc(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + cpu_pm_enter(); > + qcom_idle_enter(PM_SLEEP_MODE_SPC); > + cpu_pm_exit(); > + > + return index; > +} > + > +static struct cpuidle_driver qcom_cpuidle_driver = { > + .name = "qcom_cpuidle", > + .owner = THIS_MODULE, > +}; > + > +static const struct of_device_id qcom_idle_state_match[] __initconst = { > + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, > + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, > + { }, > +}; > + > +static int qcom_cpuidle_probe(struct platform_device *pdev) > +{ > + struct cpuidle_driver *drv = &qcom_cpuidle_driver; > + int ret; > + > + qcom_idle_enter = (void *)(pdev->dev.platform_data); > + if (!qcom_idle_enter) > + return -EFAULT; Error code looks a little wrong. -ENODEV? > + > + /* Probe for other states including platform WFI */ > + ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0); > + if (ret <= 0) { > + pr_err("%s: No cpuidle state found.\n", __func__); This would be true if ret == 0, but if it's < 0 that isn't true. Drop the print? > + return ret; > + } > + > + ret = cpuidle_register(drv, NULL); > + if (ret) { > + pr_err("%s: failed to register cpuidle driver\n", __func__); This seems redundant given that cpuidle_register() already prints an error when it fails. > + return ret; > + } > + > + return 0; Could just be return cpuidle_register(drv, NULL);
On Tue, Sep 30, 2014 at 12:18:46AM +0100, Stephen Boyd wrote: > On 09/26/14 17:58, Lina Iyer wrote: > > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c > > new file mode 100644 > > index 0000000..2fcf79a > > --- /dev/null > > +++ b/drivers/cpuidle/cpuidle-qcom.c > > @@ -0,0 +1,89 @@ > > +/* > > + * 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/cpu_pm.h> > > +#include <linux/cpuidle.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > + > > +#include <soc/qcom/pm.h> > > +#include "dt_idle_states.h" > > + > > +static void (*qcom_idle_enter)(enum pm_sleep_mode); > > + > > +static int qcom_lpm_enter_wfi(struct cpuidle_device *dev, > > + struct cpuidle_driver *drv, int index) > > +{ > > + qcom_idle_enter(PM_SLEEP_MODE_WFI); > > Why can't we just pass index here? It would be nice to not need to > include soc/qcom/pm.h in this file. I think this is definitely worth exploring. > > + return index; > > +} > > + > > +static int qcom_lpm_enter_spc(struct cpuidle_device *dev, > > + struct cpuidle_driver *drv, int index) > > +{ > > + cpu_pm_enter(); > > + qcom_idle_enter(PM_SLEEP_MODE_SPC); > > + cpu_pm_exit(); > > + > > + return index; > > +} > > + > > +static struct cpuidle_driver qcom_cpuidle_driver = { > > + .name = "qcom_cpuidle", > > + .owner = THIS_MODULE, > > +}; > > + > > +static const struct of_device_id qcom_idle_state_match[] __initconst = { > > + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, > > + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, > > + { }, > > +}; > > + > > +static int qcom_cpuidle_probe(struct platform_device *pdev) > > +{ > > + struct cpuidle_driver *drv = &qcom_cpuidle_driver; > > + int ret; > > + > > + qcom_idle_enter = (void *)(pdev->dev.platform_data); > > + if (!qcom_idle_enter) > > + return -EFAULT; > > Error code looks a little wrong. -ENODEV? > > > + > > + /* Probe for other states including platform WFI */ > > + ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0); > > + if (ret <= 0) { > > + pr_err("%s: No cpuidle state found.\n", __func__); > > This would be true if ret == 0, but if it's < 0 that isn't true. Drop > the print? I think we can safely drop the print, DT parsing code already barfs on error. Maybe you want to keep the (ret == 0) case to signal that driver was probed but no idle states were found. > > + return ret; > > + } > > + > > + ret = cpuidle_register(drv, NULL); > > + if (ret) { > > + pr_err("%s: failed to register cpuidle driver\n", __func__); > > This seems redundant given that cpuidle_register() already prints an > error when it fails. Yes, I will drop it from arm64 driver too as part of a minor clean-up when the merge window closes (also other drivers do that, and as you say it is redundant). Lorenzo
On Mon, Sep 29 2014 at 17:18 -0600, Stephen Boyd wrote: >On 09/26/14 17:58, Lina Iyer wrote: >> diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c >> new file mode 100644 >> index 0000000..2fcf79a >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-qcom.c >> @@ -0,0 +1,89 @@ >> +/* >> + * 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/cpu_pm.h> >> +#include <linux/cpuidle.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> + >> +#include <soc/qcom/pm.h> >> +#include "dt_idle_states.h" >> + >> +static void (*qcom_idle_enter)(enum pm_sleep_mode); >> + >> +static int qcom_lpm_enter_wfi(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + qcom_idle_enter(PM_SLEEP_MODE_WFI); > >Why can't we just pass index here? It would be nice to not need to >include soc/qcom/pm.h in this file. > This is the right place, the translation of QCOM's idle states is done while registering and needs to be translated back to QCOM's idle states. The interpretation of idle state bindings to the QCOM is hence contained in one place. >> + >> + return index; >> +} >> + >> +static int qcom_lpm_enter_spc(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, int index) >> +{ >> + cpu_pm_enter(); >> + qcom_idle_enter(PM_SLEEP_MODE_SPC); >> + cpu_pm_exit(); >> + >> + return index; >> +} >> + >> +static struct cpuidle_driver qcom_cpuidle_driver = { >> + .name = "qcom_cpuidle", >> + .owner = THIS_MODULE, >> +}; >> + >> +static const struct of_device_id qcom_idle_state_match[] __initconst = { >> + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, >> + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, >> + { }, >> +}; >> + >> +static int qcom_cpuidle_probe(struct platform_device *pdev) >> +{ >> + struct cpuidle_driver *drv = &qcom_cpuidle_driver; >> + int ret; >> + >> + qcom_idle_enter = (void *)(pdev->dev.platform_data); >> + if (!qcom_idle_enter) >> + return -EFAULT; > >Error code looks a little wrong. -ENODEV? > The dev is there, not just the expected platform data. I can change. >> + >> + /* Probe for other states including platform WFI */ >> + ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0); >> + if (ret <= 0) { >> + pr_err("%s: No cpuidle state found.\n", __func__); > >This would be true if ret == 0, but if it's < 0 that isn't true. Drop >the print? > Okay >> + return ret; >> + } >> + >> + ret = cpuidle_register(drv, NULL); >> + if (ret) { >> + pr_err("%s: failed to register cpuidle driver\n", __func__); > >This seems redundant given that cpuidle_register() already prints an >error when it fails. > >> + return ret; >> + } >> + >> + return 0; > >Could just be return cpuidle_register(drv, NULL); > Sure. >-- >Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, >hosted by The Linux Foundation >
On Tue, Sep 30 2014 at 02:58 -0600, Lorenzo Pieralisi wrote: >On Tue, Sep 30, 2014 at 12:18:46AM +0100, Stephen Boyd wrote: >> On 09/26/14 17:58, Lina Iyer wrote: >> > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c [...] > > +static int qcom_lpm_enter_wfi(struct cpuidle_device *dev, >> > + struct cpuidle_driver *drv, int index) >> > +{ >> > + qcom_idle_enter(PM_SLEEP_MODE_WFI); >> >> Why can't we just pass index here? It would be nice to not need to >> include soc/qcom/pm.h in this file. > >I think this is definitely worth exploring. > Sorry, I explained in the other thread. I feel that we are dispersing the translation information all over the place in doing so. The reason being, the compatible flag is not passed over to pm.c and I believe this is where it should be. >> > + return index; >> > +} >> > + >> > +static int qcom_lpm_enter_spc(struct cpuidle_device *dev, >> > + struct cpuidle_driver *drv, int index) >> > +{ >> > + cpu_pm_enter(); >> > + qcom_idle_enter(PM_SLEEP_MODE_SPC); >> > + cpu_pm_exit(); >> > + >> > + return index; >> > +} >> > + >> > +static struct cpuidle_driver qcom_cpuidle_driver = { >> > + .name = "qcom_cpuidle", >> > + .owner = THIS_MODULE, >> > +}; >> > + >> > +static const struct of_device_id qcom_idle_state_match[] __initconst = { >> > + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, >> > + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, >> > + { }, >> > +}; This is the onward translation from QCOM's idle states to cpuidle's states and passing cpuidle index value to SoC layer, opens up possibility for errors. >> > + >> > +static int qcom_cpuidle_probe(struct platform_device *pdev) >> > +{ >> > + struct cpuidle_driver *drv = &qcom_cpuidle_driver; >> > + int ret; >> > + >> > + qcom_idle_enter = (void *)(pdev->dev.platform_data); >> > + if (!qcom_idle_enter) >> > + return -EFAULT; >> >> Error code looks a little wrong. -ENODEV? >> >> > + >> > + /* Probe for other states including platform WFI */ >> > + ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0); >> > + if (ret <= 0) { >> > + pr_err("%s: No cpuidle state found.\n", __func__); >> >> This would be true if ret == 0, but if it's < 0 that isn't true. Drop >> the print? > >I think we can safely drop the print, DT parsing code already barfs on >error. Maybe you want to keep the (ret == 0) case to signal that driver >was probed but no idle states were found. > OK >> > + return ret; >> > + } >> > + >> > + ret = cpuidle_register(drv, NULL); >> > + if (ret) { >> > + pr_err("%s: failed to register cpuidle driver\n", __func__); >> >> This seems redundant given that cpuidle_register() already prints an >> error when it fails. > >Yes, I will drop it from arm64 driver too as part of a minor clean-up >when the merge window closes (also other drivers do that, and as you say >it is redundant). > OK >
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes: > Hi Lina, > > On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote: >> Add cpuidle driver interface to allow cpus to go into C-States. Use the >> cpuidle DT interface common across ARM architectures to provide the >> C-State information to the cpuidle framework. >> >> Supported modes at this time are clock gating (wfi) and cpu power down >> (Standalone PC or spc). >> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >> --- >> .../bindings/arm/msm/qcom,idle-state.txt | 72 +++++++++++++++++ >> drivers/cpuidle/Kconfig.arm | 7 ++ >> drivers/cpuidle/Makefile | 1 + >> drivers/cpuidle/cpuidle-qcom.c | 89 ++++++++++++++++++++++ >> 4 files changed, 169 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt >> create mode 100644 drivers/cpuidle/cpuidle-qcom.c >> >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt >> new file mode 100644 >> index 0000000..47095b9 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt >> @@ -0,0 +1,72 @@ >> +QCOM Idle States for cpuidle driver >> + >> +ARM provides idle-state node to define the cpuidle states, as defined in [1]. >> +cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle >> +states. Idle states have different enter/exit latency and residency values. >> +The idle states supported by the QCOM SoC are defined as - >> + >> + * WFI >> + * Retention >> + * Standalone Power Collapse (Standalone PC or SPC) >> + * Power Collapse (PC) >> + >> +WFI: WFI does a little more in addition to architectural clock gating. ARM > > This may be misleading. Call it PlatformWFI or something like that, not WFI if > that's not what it is. This gets at a little pet peeve of mine: IMO, naming any state with "WFI" is a bit confusing, because typically *every* idle state is entered by one (or more) CPU executing WFI, no? Kevin
On Tue, 30 Sep 2014, Kevin Hilman wrote: > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes: > > > Hi Lina, > > > > On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote: > >> Add cpuidle driver interface to allow cpus to go into C-States. Use the > >> cpuidle DT interface common across ARM architectures to provide the > >> C-State information to the cpuidle framework. > >> > >> Supported modes at this time are clock gating (wfi) and cpu power down > >> (Standalone PC or spc). > >> > >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > >> --- > >> .../bindings/arm/msm/qcom,idle-state.txt | 72 +++++++++++++++++ > >> drivers/cpuidle/Kconfig.arm | 7 ++ > >> drivers/cpuidle/Makefile | 1 + > >> drivers/cpuidle/cpuidle-qcom.c | 89 ++++++++++++++++++++++ > >> 4 files changed, 169 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > >> create mode 100644 drivers/cpuidle/cpuidle-qcom.c > >> > >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > >> new file mode 100644 > >> index 0000000..47095b9 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > >> @@ -0,0 +1,72 @@ > >> +QCOM Idle States for cpuidle driver > >> + > >> +ARM provides idle-state node to define the cpuidle states, as defined in [1]. > >> +cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle > >> +states. Idle states have different enter/exit latency and residency values. > >> +The idle states supported by the QCOM SoC are defined as - > >> + > >> + * WFI > >> + * Retention > >> + * Standalone Power Collapse (Standalone PC or SPC) > >> + * Power Collapse (PC) > >> + > >> +WFI: WFI does a little more in addition to architectural clock gating. ARM > > > > This may be misleading. Call it PlatformWFI or something like that, not WFI if > > that's not what it is. > > This gets at a little pet peeve of mine: > > IMO, naming any state with "WFI" is a bit confusing, because typically > *every* idle state is entered by one (or more) CPU executing WFI, no? Agreed. The only state called "WFI" should be the one that only executes the WFI instruction without any other hardware setup around it. Nicolas
On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > On Tue, 30 Sep 2014, Kevin Hilman wrote: > >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes: >> >> > Hi Lina, >> > >> > On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote: >> >> Add cpuidle driver interface to allow cpus to go into C-States. Use the >> >> cpuidle DT interface common across ARM architectures to provide the >> >> C-State information to the cpuidle framework. >> >> >> >> Supported modes at this time are clock gating (wfi) and cpu power down >> >> (Standalone PC or spc). >> >> >> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >> >> --- >> >> .../bindings/arm/msm/qcom,idle-state.txt | 72 +++++++++++++++++ >> >> drivers/cpuidle/Kconfig.arm | 7 ++ >> >> drivers/cpuidle/Makefile | 1 + >> >> drivers/cpuidle/cpuidle-qcom.c | 89 ++++++++++++++++++++++ >> >> 4 files changed, 169 insertions(+) >> >> create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt >> >> create mode 100644 drivers/cpuidle/cpuidle-qcom.c >> >> >> >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt >> >> new file mode 100644 >> >> index 0000000..47095b9 >> >> --- /dev/null >> >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt >> >> @@ -0,0 +1,72 @@ >> >> +QCOM Idle States for cpuidle driver >> >> + >> >> +ARM provides idle-state node to define the cpuidle states, as defined in [1]. >> >> +cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle >> >> +states. Idle states have different enter/exit latency and residency values. >> >> +The idle states supported by the QCOM SoC are defined as - >> >> + >> >> + * WFI >> >> + * Retention >> >> + * Standalone Power Collapse (Standalone PC or SPC) >> >> + * Power Collapse (PC) >> >> + >> >> +WFI: WFI does a little more in addition to architectural clock gating. ARM >> > >> > This may be misleading. Call it PlatformWFI or something like that, not WFI if >> > that's not what it is. >> >> This gets at a little pet peeve of mine: >> >> IMO, naming any state with "WFI" is a bit confusing, because typically >> *every* idle state is entered by one (or more) CPU executing WFI, no? > > Agreed. > > The only state called "WFI" should be the one that only executes the WFI > instruction without any other hardware setup around it. Well, I would go even further in that none of the states should be called WFI, because WFI is used to enter all of them. Kevin
On Tue, 30 Sep 2014, Kevin Hilman wrote: > On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre > <nicolas.pitre@linaro.org> wrote: > > On Tue, 30 Sep 2014, Kevin Hilman wrote: > > > >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes: > >> > >> > Hi Lina, > >> > > >> > On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote: > >> >> Add cpuidle driver interface to allow cpus to go into C-States. Use the > >> >> cpuidle DT interface common across ARM architectures to provide the > >> >> C-State information to the cpuidle framework. > >> >> > >> >> Supported modes at this time are clock gating (wfi) and cpu power down > >> >> (Standalone PC or spc). > >> >> > >> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > >> >> --- > >> >> .../bindings/arm/msm/qcom,idle-state.txt | 72 +++++++++++++++++ > >> >> drivers/cpuidle/Kconfig.arm | 7 ++ > >> >> drivers/cpuidle/Makefile | 1 + > >> >> drivers/cpuidle/cpuidle-qcom.c | 89 ++++++++++++++++++++++ > >> >> 4 files changed, 169 insertions(+) > >> >> create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > >> >> create mode 100644 drivers/cpuidle/cpuidle-qcom.c > >> >> > >> >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > >> >> new file mode 100644 > >> >> index 0000000..47095b9 > >> >> --- /dev/null > >> >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt > >> >> @@ -0,0 +1,72 @@ > >> >> +QCOM Idle States for cpuidle driver > >> >> + > >> >> +ARM provides idle-state node to define the cpuidle states, as defined in [1]. > >> >> +cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle > >> >> +states. Idle states have different enter/exit latency and residency values. > >> >> +The idle states supported by the QCOM SoC are defined as - > >> >> + > >> >> + * WFI > >> >> + * Retention > >> >> + * Standalone Power Collapse (Standalone PC or SPC) > >> >> + * Power Collapse (PC) > >> >> + > >> >> +WFI: WFI does a little more in addition to architectural clock gating. ARM > >> > > >> > This may be misleading. Call it PlatformWFI or something like that, not WFI if > >> > that's not what it is. > >> > >> This gets at a little pet peeve of mine: > >> > >> IMO, naming any state with "WFI" is a bit confusing, because typically > >> *every* idle state is entered by one (or more) CPU executing WFI, no? > > > > Agreed. > > > > The only state called "WFI" should be the one that only executes the WFI > > instruction without any other hardware setup around it. > > Well, I would go even further in that none of the states should be > called WFI, because WFI is used to enter all of them. Fair enough. So let's fix this by finding a name for that state that consists of only executing WFI and that every SOC has. Suggestions? Nicolas
Nicolas Pitre <nicolas.pitre@linaro.org> writes: > On Tue, 30 Sep 2014, Kevin Hilman wrote: > >> On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre >> <nicolas.pitre@linaro.org> wrote: >> > On Tue, 30 Sep 2014, Kevin Hilman wrote: >> > >> >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes: [...] >> >> > This may be misleading. Call it PlatformWFI or something like that, not WFI if >> >> > that's not what it is. >> >> >> >> This gets at a little pet peeve of mine: >> >> >> >> IMO, naming any state with "WFI" is a bit confusing, because typically >> >> *every* idle state is entered by one (or more) CPU executing WFI, no? >> > >> > Agreed. >> > >> > The only state called "WFI" should be the one that only executes the WFI >> > instruction without any other hardware setup around it. >> >> Well, I would go even further in that none of the states should be >> called WFI, because WFI is used to enter all of them. > > Fair enough. > > So let's fix this by finding a name for that state that consists of only > executing WFI and that every SOC has. > > Suggestions? The DT idle-states binding doc (though seemingly written more with arm64 and SBSA in mind) uses "standby" for the shallowest idle. Kevin
On Tue, Sep 30 2014 at 12:41 -0600, Kevin Hilman wrote: >Nicolas Pitre <nicolas.pitre@linaro.org> writes: > >> On Tue, 30 Sep 2014, Kevin Hilman wrote: >> >>> On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre >>> <nicolas.pitre@linaro.org> wrote: >>> > On Tue, 30 Sep 2014, Kevin Hilman wrote: >>> > >>> >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes: > >[...] > >>> >> > This may be misleading. Call it PlatformWFI or something like that, not WFI if >>> >> > that's not what it is. >>> >> >>> >> This gets at a little pet peeve of mine: >>> >> >>> >> IMO, naming any state with "WFI" is a bit confusing, because typically >>> >> *every* idle state is entered by one (or more) CPU executing WFI, no? >>> > >>> > Agreed. >>> > >>> > The only state called "WFI" should be the one that only executes the WFI >>> > instruction without any other hardware setup around it. >>> >>> Well, I would go even further in that none of the states should be >>> called WFI, because WFI is used to enter all of them. >> >> Fair enough. >> >> So let's fix this by finding a name for that state that consists of only >> executing WFI and that every SOC has. >> >> Suggestions? > >The DT idle-states binding doc (though seemingly written more with >arm64 and SBSA in mind) uses "standby" for the shallowest idle. > Standby - looks good to me.
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file mode 100644 index 0000000..47095b9 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt @@ -0,0 +1,72 @@ +QCOM Idle States for cpuidle driver + +ARM provides idle-state node to define the cpuidle states, as defined in [1]. +cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle +states. Idle states have different enter/exit latency and residency values. +The idle states supported by the QCOM SoC are defined as - + + * WFI + * Retention + * Standalone Power Collapse (Standalone PC or SPC) + * Power Collapse (PC) + +WFI: WFI does a little more in addition to architectural clock gating. ARM +processors when execute the wfi instruction will gate their internal clocks. +QCOM cpus use this instruction as a trigger for the SPM state machine. Usually +with a cpu entering WFI, the SPM is configured to do clock-gating as well. The +SPM state machine waits for the interrrupt to trigger the core back in to +active. When all CPUs in the SoC, clock gate using the ARM wfi instruction, the +second level cache usually can also clock gate sensing no cpu activity. When a +cpu is ready to run, it needs the cache to be active before starting execution. +Allowing the SPM to execute the clock gating statemachine and waiting for +interrupt on behalf of the processor has a benefit of guaranteeing that the +system state is conducive for the core to resume execution. + +Retention: Retention is a low power state where the core is clockgated and the +memory and the registers associated with the core are retained. The voltage +may be reduced to the minimum value needed to keep the processor registers +active. Retention is triggered when the core executes wfi instruction. The SPM +should be configured to execute the retention sequence and would wait for +interrupt, before restoring the cpu to execution state. Retention may have a +slightly higher latency than WFI. + +Standalone PC: A cpu can power down and warmboot if there is a sufficient time +between now and the next know wake up. SPC mode is used to indicate a core +entering a power down state without consulting any other cpu or the system +resources. This helps save power only on that core. Like WFI and Retention, the +core executes wfi and the SPM programmed to do SPC would use the cpu control +logic to power down the core's supply and restore it back when woken up by an +interrupt. Applying power and reseting the core causes the core to warmboot +back into secure mode which trampolines the control back to the kernel. To +enter a power down state the kernel needs to call into the secure layer which +would then execute the ARM wfi instruction. Failing to do so, would result in a +crash enforced by the warm boot code in the secure layer. On a SoC with +write-back L1 cache, the cache would need to be flushed. + +Power Collapse: This state is similiar to the SPC mode, but distinguishes +itself in the fact that the cpu acknowledges and permits the SoC to enter +deeper sleep modes. In a hierarchical power domain SoC, this means L2 and other +caches can be flushed, system bus, clocks - lowered, and SoC main XO turned off +and voltages reduced, provided all cpus enter this state. In other words, it is +a coupled idle state. Since the span of low power modes possible at this state +is vast, the exit latency and the residency of this low power mode would be +considered high even though at a cpu level, this essentially is cpu power down. +The SPM in this state also may handshake with the Resource power manager +processor in the SoC to indicate a complete subsystem shut down. + +The idle-state for QCOM SoCs are distinguished by the compatible property of +the node. They indicate to the cpuidle driver the entry point to use for +cpuidle. The devicetree representation of the idle state should be - + +Required properties: + +- compatible: Must be "arm,idle-state" + and one of - + "qcom,idle-state-wfi", + "qcom,idle-state-ret", + "qcom,idle-state-spc", + "qcom,idle-state-pc", + +Other required and optional properties are specified in [1]. + +[1]. Documentation/devicetree/bindings/arm/idle-states.txt diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm index 38cff69..6a9ee12 100644 --- a/drivers/cpuidle/Kconfig.arm +++ b/drivers/cpuidle/Kconfig.arm @@ -62,3 +62,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..2fcf79a --- /dev/null +++ b/drivers/cpuidle/cpuidle-qcom.c @@ -0,0 +1,89 @@ +/* + * 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/cpu_pm.h> +#include <linux/cpuidle.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> + +#include <soc/qcom/pm.h> +#include "dt_idle_states.h" + +static void (*qcom_idle_enter)(enum pm_sleep_mode); + +static int qcom_lpm_enter_wfi(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + qcom_idle_enter(PM_SLEEP_MODE_WFI); + + return index; +} + +static int qcom_lpm_enter_spc(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + cpu_pm_enter(); + qcom_idle_enter(PM_SLEEP_MODE_SPC); + cpu_pm_exit(); + + return index; +} + +static struct cpuidle_driver qcom_cpuidle_driver = { + .name = "qcom_cpuidle", + .owner = THIS_MODULE, +}; + +static const struct of_device_id qcom_idle_state_match[] __initconst = { + { .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi }, + { .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc }, + { }, +}; + +static int qcom_cpuidle_probe(struct platform_device *pdev) +{ + struct cpuidle_driver *drv = &qcom_cpuidle_driver; + int ret; + + qcom_idle_enter = (void *)(pdev->dev.platform_data); + if (!qcom_idle_enter) + return -EFAULT; + + /* Probe for other states including platform WFI */ + ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0); + if (ret <= 0) { + pr_err("%s: No cpuidle state found.\n", __func__); + return ret; + } + + ret = cpuidle_register(drv, NULL); + if (ret) { + pr_err("%s: failed to register cpuidle driver\n", __func__); + return ret; + } + + return 0; +} + +static struct platform_driver qcom_cpuidle_plat_driver = { + .probe = qcom_cpuidle_probe, + .driver = { + .name = "qcom_cpuidle", + .owner = THIS_MODULE, + }, +}; + +module_platform_driver(qcom_cpuidle_plat_driver);
Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework. Supported modes at this time are clock gating (wfi) and cpu power down (Standalone PC or spc). Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- .../bindings/arm/msm/qcom,idle-state.txt | 72 +++++++++++++++++ drivers/cpuidle/Kconfig.arm | 7 ++ drivers/cpuidle/Makefile | 1 + drivers/cpuidle/cpuidle-qcom.c | 89 ++++++++++++++++++++++ 4 files changed, 169 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt create mode 100644 drivers/cpuidle/cpuidle-qcom.c