The noop functions code is not necessary because the header file is included in files which are compiled when CONFIG_CPU_IDLE is on.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/include/asm/cpuidle.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h index 2fca60a..7367787 100644 --- a/arch/arm/include/asm/cpuidle.h +++ b/arch/arm/include/asm/cpuidle.h @@ -1,13 +1,8 @@ #ifndef __ASM_ARM_CPUIDLE_H #define __ASM_ARM_CPUIDLE_H
-#ifdef CONFIG_CPU_IDLE extern int arm_cpuidle_simple_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index); -#else -static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) { return -ENODEV; } -#endif + struct cpuidle_driver *drv, int index);
/* Common ARM WFI state */ #define ARM_CPUIDLE_WFI_STATE_PWR(p) {\
Add the __init section for the functions which are called at init time.
Signed-off-by: Daniel Lezcano <daniel.linaro.org> Acked-by: Simon Horman horms+renesas@verge.net.au --- arch/arm/mach-shmobile/cpuidle.c | 4 ++-- arch/arm/mach-shmobile/pm-sh7372.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 9e05026..068f9ca 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -36,12 +36,12 @@ static struct cpuidle_driver shmobile_cpuidle_default_driver = {
static struct cpuidle_driver *cpuidle_drv = &shmobile_cpuidle_default_driver;
-void shmobile_cpuidle_set_driver(struct cpuidle_driver *drv) +void __init shmobile_cpuidle_set_driver(struct cpuidle_driver *drv) { cpuidle_drv = drv; }
-int shmobile_cpuidle_init(void) +int __init shmobile_cpuidle_init(void) { struct cpuidle_device *dev = &shmobile_cpuidle_dev;
diff --git a/arch/arm/mach-shmobile/pm-sh7372.c b/arch/arm/mach-shmobile/pm-sh7372.c index a0826a4..f6b14ca 100644 --- a/arch/arm/mach-shmobile/pm-sh7372.c +++ b/arch/arm/mach-shmobile/pm-sh7372.c @@ -450,12 +450,12 @@ static struct cpuidle_driver sh7372_cpuidle_driver = { }, };
-static void sh7372_cpuidle_init(void) +static void __init sh7372_cpuidle_init(void) { shmobile_cpuidle_set_driver(&sh7372_cpuidle_driver); } #else -static void sh7372_cpuidle_init(void) {} +static void __init sh7372_cpuidle_init(void) {} #endif
#ifdef CONFIG_SUSPEND
Remove the shmobile_enter_wfi function which is the same as the common WFI enter function from the arm cpuidle driver defined with the ARM_CPUIDLE_WFI_STATE macro.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Acked-by: Simon Horman horms+renesas@verge.net.au --- arch/arm/mach-shmobile/cpuidle.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/arch/arm/mach-shmobile/cpuidle.c b/arch/arm/mach-shmobile/cpuidle.c index 068f9ca..c872ae8 100644 --- a/arch/arm/mach-shmobile/cpuidle.c +++ b/arch/arm/mach-shmobile/cpuidle.c @@ -16,20 +16,12 @@ #include <asm/cpuidle.h> #include <asm/io.h>
-int shmobile_enter_wfi(struct cpuidle_device *dev, struct cpuidle_driver *drv, - int index) -{ - cpu_do_idle(); - return 0; -} - static struct cpuidle_device shmobile_cpuidle_dev; static struct cpuidle_driver shmobile_cpuidle_default_driver = { .name = "shmobile_cpuidle", .owner = THIS_MODULE, .en_core_tk_irqen = 1, .states[0] = ARM_CPUIDLE_WFI_STATE, - .states[0].enter = shmobile_enter_wfi, .safe_state_index = 0, /* C1 */ .state_count = 1, };
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle44xx.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index f4b1b23..3d33b8a 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -210,6 +210,7 @@ int __init omap4_idle_init(void) { struct cpuidle_device *dev; unsigned int cpu_id = 0; + int ret;
mpu_pd = pwrdm_lookup("mpu_pwrdm"); cpu_pd[0] = pwrdm_lookup("cpu0_pwrdm"); @@ -222,14 +223,18 @@ int __init omap4_idle_init(void) if (!cpu_clkdm[0] || !cpu_clkdm[1]) return -ENODEV;
+ ret = cpuidle_register_driver(&omap4_idle_driver); + if (ret) { + printk(KERN_ERR "failed to register the idle driver\n"); + return ret; + } + for_each_cpu(cpu_id, cpu_online_mask) { dev = &per_cpu(omap4_idle_dev, cpu_id); dev->cpu = cpu_id; #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED dev->coupled_cpus = *cpu_online_mask; #endif - cpuidle_register_driver(&omap4_idle_driver); - if (cpuidle_register_device(dev)) { pr_err("%s: CPUidle register failed\n", __func__); return -EIO;
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Regards, Santosh
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Thanks -- Daniel
On Friday 29 March 2013 04:15 PM, Daniel Lezcano wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Fisrtly patches get posted to right mailing list based on where the code resides. So one must keep a watch on LAKML for the patches.
Talking specific to OMAP idle code, there is plan to move to drivers/idle/* but for that to happen there are some PRM/CM dependency for which also driver movement is planned. Once that happen, OMAP idle will find its way in drivers/idle/*
On 03/29/2013 11:53 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:15 PM, Daniel Lezcano wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Fisrtly patches get posted to right mailing list based on where the code resides. So one must keep a watch on LAKML for the patches.
Yes, I agree.
The main issue is the multiple tree for the different drivers making hard to track, modify and improve the drivers in one shot.
It is not the first time, a modification of the cpuidle framework implied to modify all the drivers.
When Rob introduced the first code consolidation, that took months to add a simple flag in the drivers because we had to wait for the merge before the changes in drivers/cpuidle/cpuidle.c were visible.
Talking specific to OMAP idle code, there is plan to move to drivers/idle/* but for that to happen there are some PRM/CM dependency for which also driver movement is planned. Once that happen, OMAP idle will find its way in drivers/idle/*
That would be *really* great. If we can do that for all the drivers, that will solve the multi-location / multi-tree problem.
The u8500 driver will be moved soon to this directory also.
I did some modifications around the at91 some months ago to encapsulate the code more, maybe it could be also a good candidate. Nicolas ?
For OMAP3 that could be a bit more difficult. Who is maintaining the driver now ?
I Cc'ed the different maintainers for the other boards, may be they can react ?
Thanks -- Daniel
On Friday 29 March 2013 04:53 PM, Daniel Lezcano wrote:
On 03/29/2013 11:53 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:15 PM, Daniel Lezcano wrote:
[..]
For OMAP3 that could be a bit more difficult. Who is maintaining the driver now ?
The driver is still maintained by Kevin Hilman and quite a few us keep that driver upto date. OMAP3 idle is movable as well. A while back I have assessed move of OMAP3 and OMAP4 idle drivers, and the only issue was the other PRM/CM dependency.
Regards, Santosh
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
/Amit
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers).
Having said that, its always good to get more reviews and acks so that better code gets merged.
This just my personal opinion.
Regards, Santosh
On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers).
Having said that, its always good to get more reviews and acks so that better code gets merged.
This just my personal opinion.
I'm not asking for special treatment here. :) I'm requesting one set of maintainers (arm-soc maintainers) to push back on changes that don't get platform idle drivers in sync with the consolidation work that is currently ongoing.
This will speed up the process since it is hard to track every SoC-specific list for these changes. Some platform maintainers might not even be aware of it (those that Daniel hasn't modified yet). A similar approach seems to have worked for common clock, DT, pinmux, etc.
/Amit
On Friday 29 March 2013 06:20 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL.
Move this initialization out of the loop.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers).
Having said that, its always good to get more reviews and acks so that better code gets merged.
This just my personal opinion.
I'm not asking for special treatment here. :) I'm requesting one set of maintainers (arm-soc maintainers) to push back on changes that don't get platform idle drivers in sync with the consolidation work that is currently ongoing.
This will speed up the process since it is hard to track every SoC-specific list for these changes. Some platform maintainers might not even be aware of it (those that Daniel hasn't modified yet). A similar approach seems to have worked for common clock, DT, pinmux, etc.
Every patch gets pulled into arm-soc/arm-core has to be posted on LAKML. So as long as everybody follows that rule, there is no need to track every SoC lists. And what I have seen so far every this rule has been followed well.
Regards, Santosh
On 03/29/2013 04:10 PM, Santosh Shilimkar wrote:
On Friday 29 March 2013 06:20 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: > The driver is initialized several times. This is wrong and if the > return code of the function was checked, it will return -EINVAL. > > Move this initialization out of the loop. > > Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org > --- Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers).
Having said that, its always good to get more reviews and acks so that better code gets merged.
This just my personal opinion.
I'm not asking for special treatment here. :) I'm requesting one set of maintainers (arm-soc maintainers) to push back on changes that don't get platform idle drivers in sync with the consolidation work that is currently ongoing.
This will speed up the process since it is hard to track every SoC-specific list for these changes. Some platform maintainers might not even be aware of it (those that Daniel hasn't modified yet). A similar approach seems to have worked for common clock, DT, pinmux, etc.
Every patch gets pulled into arm-soc/arm-core has to be posted on LAKML. So as long as everybody follows that rule, there is no need to track every SoC lists. And what I have seen so far every this rule has been followed well.
(Added Benjamin, Deepthi and Paul)
I don't think everybody is following this rule, patches go to the SoC maintainer's tree without sometimes going through lakml.
Furthermore, there is not only ARM, there is also acpi_idle, intel_idle, pseries_idle and sh_idle, respectively located in:
drivers/acpi/processor_idle.c drivers/acpi/processor_driver.c drivers/idle/intel_idle.c
These ones above are under linux-pm, that is Rafael, like cpuidle, even if it is not marked in the MAINTAINERS file, so that should be ok.
And there is also:
arch/sh/kernel/cpu/shmobile/cpuidle.c arch/powerpc/platforms/pseries/processor_idle.c
And hopefully, some others in the right place, calxeda_idle and kirwood_idle located in drivers/cpuidle.
In the maintainer file, there is no information about cpuidle at all.
For example, if someone modify the cpuidle framework allowing to consolidate the code across the different drivers, we have to wait for the merge before using the new api into the different drivers. If we follow strictly the path of the merge tree we fall into a scenario where consolidating the drivers takes a loooooong time.
From my POV, *all* the cpuidle drivers must go under drivers/cpuidle,
like cpufreq and pass through a single entry point to apply the patches, so the cpuidle framework and the drivers are always synced.
If everyone agree and we reach this consensus, then we can work to move these drivers to a single place.
I think Amit was suggesting to Cc me in the meantime, so while we are moving these drivers to this place, I can help to ensure we go to the same direction.
For example, Arnd Cc'ed me about the zynq cpuidle driver when it has been posted and, after review, it appeared it was totally obsolete wrt the modifications we did this year.
I propose first to add an entry in MAINTAINERS:
diff --git a/MAINTAINERS b/MAINTAINERS index 4cf5fd3..5b5ab87 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2206,6 +2206,15 @@ S: Maintained F: drivers/cpufreq/ F: include/linux/cpufreq.h
+CPU IDLE DRIVERS +M: Rafael J. Wysocki rjw@sisk.pl +L: cpuidle@vger.kernel.org +L: linux-pm@vger.kernel.org +S: Maintained +F: drivers/cpuidle/ +F: include/linux/cpuidle.h + CPUID/MSR DRIVER M: "H. Peter Anvin" hpa@zytor.com S: Maintained
Does it make sense ?
Thanks -- Daniel
On Friday, March 29, 2013 04:50:55 PM Daniel Lezcano wrote:
On 03/29/2013 04:10 PM, Santosh Shilimkar wrote:
On Friday 29 March 2013 06:20 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: > On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >> The driver is initialized several times. This is wrong and if the >> return code of the function was checked, it will return -EINVAL. >> >> Move this initialization out of the loop. >> >> Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org >> --- > Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers).
Having said that, its always good to get more reviews and acks so that better code gets merged.
This just my personal opinion.
I'm not asking for special treatment here. :) I'm requesting one set of maintainers (arm-soc maintainers) to push back on changes that don't get platform idle drivers in sync with the consolidation work that is currently ongoing.
This will speed up the process since it is hard to track every SoC-specific list for these changes. Some platform maintainers might not even be aware of it (those that Daniel hasn't modified yet). A similar approach seems to have worked for common clock, DT, pinmux, etc.
Every patch gets pulled into arm-soc/arm-core has to be posted on LAKML. So as long as everybody follows that rule, there is no need to track every SoC lists. And what I have seen so far every this rule has been followed well.
(Added Benjamin, Deepthi and Paul)
I don't think everybody is following this rule, patches go to the SoC maintainer's tree without sometimes going through lakml.
Furthermore, there is not only ARM, there is also acpi_idle, intel_idle, pseries_idle and sh_idle, respectively located in:
drivers/acpi/processor_idle.c drivers/acpi/processor_driver.c drivers/idle/intel_idle.c
These ones above are under linux-pm, that is Rafael, like cpuidle, even if it is not marked in the MAINTAINERS file, so that should be ok.
And there is also:
arch/sh/kernel/cpu/shmobile/cpuidle.c arch/powerpc/platforms/pseries/processor_idle.c
And hopefully, some others in the right place, calxeda_idle and kirwood_idle located in drivers/cpuidle.
In the maintainer file, there is no information about cpuidle at all.
For example, if someone modify the cpuidle framework allowing to consolidate the code across the different drivers, we have to wait for the merge before using the new api into the different drivers. If we follow strictly the path of the merge tree we fall into a scenario where consolidating the drivers takes a loooooong time.
From my POV, *all* the cpuidle drivers must go under drivers/cpuidle, like cpufreq and pass through a single entry point to apply the patches, so the cpuidle framework and the drivers are always synced.
If everyone agree and we reach this consensus, then we can work to move these drivers to a single place.
I think Amit was suggesting to Cc me in the meantime, so while we are moving these drivers to this place, I can help to ensure we go to the same direction.
For example, Arnd Cc'ed me about the zynq cpuidle driver when it has been posted and, after review, it appeared it was totally obsolete wrt the modifications we did this year.
I propose first to add an entry in MAINTAINERS:
diff --git a/MAINTAINERS b/MAINTAINERS index 4cf5fd3..5b5ab87 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2206,6 +2206,15 @@ S: Maintained F: drivers/cpufreq/ F: include/linux/cpufreq.h
+CPU IDLE DRIVERS +M: Rafael J. Wysocki rjw@sisk.pl +L: cpuidle@vger.kernel.org +L: linux-pm@vger.kernel.org +S: Maintained +F: drivers/cpuidle/ +F: include/linux/cpuidle.h
CPUID/MSR DRIVER M: "H. Peter Anvin" hpa@zytor.com S: Maintained
Does it make sense ?
Yes, it does to me. :-)
That said, I'm not sure if moving the existing cpuidle drivers to that directory solves any problems. If somebody wants to keep their cpudile driver in his/her arch or platform, and there may be reasons for that, it's basically fine by me.
Thanks, Rafael
On 03/29/2013 09:20 PM, Daniel Lezcano wrote:
On 03/29/2013 04:10 PM, Santosh Shilimkar wrote:
On Friday 29 March 2013 06:20 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote:
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: > On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >> The driver is initialized several times. This is wrong and if the >> return code of the function was checked, it will return -EINVAL. >> >> Move this initialization out of the loop. >> >> Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org >> --- > Fix for this is already and v2 of the patch is here [1]
Ah, ok. Thanks for reviewing the patch.
Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ?
Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code.
Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ?
Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers).
Having said that, its always good to get more reviews and acks so that better code gets merged.
This just my personal opinion.
I'm not asking for special treatment here. :) I'm requesting one set of maintainers (arm-soc maintainers) to push back on changes that don't get platform idle drivers in sync with the consolidation work that is currently ongoing.
This will speed up the process since it is hard to track every SoC-specific list for these changes. Some platform maintainers might not even be aware of it (those that Daniel hasn't modified yet). A similar approach seems to have worked for common clock, DT, pinmux, etc.
Every patch gets pulled into arm-soc/arm-core has to be posted on LAKML. So as long as everybody follows that rule, there is no need to track every SoC lists. And what I have seen so far every this rule has been followed well.
(Added Benjamin, Deepthi and Paul)
I don't think everybody is following this rule, patches go to the SoC maintainer's tree without sometimes going through lakml.
Furthermore, there is not only ARM, there is also acpi_idle, intel_idle, pseries_idle and sh_idle, respectively located in:
drivers/acpi/processor_idle.c drivers/acpi/processor_driver.c drivers/idle/intel_idle.c
These ones above are under linux-pm, that is Rafael, like cpuidle, even if it is not marked in the MAINTAINERS file, so that should be ok.
And there is also:
arch/sh/kernel/cpu/shmobile/cpuidle.c arch/powerpc/platforms/pseries/processor_idle.c
And hopefully, some others in the right place, calxeda_idle and kirwood_idle located in drivers/cpuidle.
In the maintainer file, there is no information about cpuidle at all.
For example, if someone modify the cpuidle framework allowing to consolidate the code across the different drivers, we have to wait for the merge before using the new api into the different drivers. If we follow strictly the path of the merge tree we fall into a scenario where consolidating the drivers takes a loooooong time.
From my POV, *all* the cpuidle drivers must go under drivers/cpuidle, like cpufreq and pass through a single entry point to apply the patches, so the cpuidle framework and the drivers are always synced.
I can very much relate to the above mentioned problem, where code modifications done as a part of cpuidle framework needs to be reflected in every arch back-end cpuidle driver. We do have added advantages in moving all the back-end drivers into drivers/cpuidle. This would help us achieve better reviews, easier consolidations and more importantly maintaining sync btw drivers and framework and the up-streaming process.
But then, this means we get all the arch specific code out under drivers/cpuidle which can be very messy. Also instances where the changes are specifically tied only to the architecture of the back-end driver (SoC specific), it is absolutely necessary to get SoC maintainer's review.
Cheers, Deepthi
If everyone agree and we reach this consensus, then we can work to move these drivers to a single place.
I think Amit was suggesting to Cc me in the meantime, so while we are moving these drivers to this place, I can help to ensure we go to the same direction.
For example, Arnd Cc'ed me about the zynq cpuidle driver when it has been posted and, after review, it appeared it was totally obsolete wrt the modifications we did this year.
I propose first to add an entry in MAINTAINERS:
diff --git a/MAINTAINERS b/MAINTAINERS index 4cf5fd3..5b5ab87 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2206,6 +2206,15 @@ S: Maintained F: drivers/cpufreq/ F: include/linux/cpufreq.h
+CPU IDLE DRIVERS +M: Rafael J. Wysocki rjw@sisk.pl +L: cpuidle@vger.kernel.org +L: linux-pm@vger.kernel.org +S: Maintained +F: drivers/cpuidle/ +F: include/linux/cpuidle.h
CPUID/MSR DRIVER M: "H. Peter Anvin" hpa@zytor.com S: Maintained
Does it make sense ?
Thanks -- Daniel
On 04/01/2013 08:05 AM, Deepthi Dharwar wrote:
On 03/29/2013 09:20 PM, Daniel Lezcano wrote:
On 03/29/2013 04:10 PM, Santosh Shilimkar wrote:
On Friday 29 March 2013 06:20 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar santosh.shilimkar@ti.com wrote:
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote:
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano daniel.lezcano@linaro.org wrote: > On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: >> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >>> The driver is initialized several times. This is wrong and if the >>> return code of the function was checked, it will return -EINVAL. >>> >>> Move this initialization out of the loop. >>> >>> Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org >>> --- >> Fix for this is already and v2 of the patch is here [1] > > Ah, ok. Thanks for reviewing the patch. > > Can we find a solution to have a single entry point to sumbit patches > for all the cpuidle drivers ? > > Otherwise, consolidating them is a pain: a patch for the samsung tree, > another one for the at91 tree, etc ... and wait for all the trees to > sync before continuing to consolidate the code. > > Wouldn't be worth to move these drivers under the PM umbrella instead of > the SoC specific code ? > > Any idea to simplify the cpuidle consolidation and maintenance ?
Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree.
Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you?
Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers).
Having said that, its always good to get more reviews and acks so that better code gets merged.
This just my personal opinion.
I'm not asking for special treatment here. :) I'm requesting one set of maintainers (arm-soc maintainers) to push back on changes that don't get platform idle drivers in sync with the consolidation work that is currently ongoing.
This will speed up the process since it is hard to track every SoC-specific list for these changes. Some platform maintainers might not even be aware of it (those that Daniel hasn't modified yet). A similar approach seems to have worked for common clock, DT, pinmux, etc.
Every patch gets pulled into arm-soc/arm-core has to be posted on LAKML. So as long as everybody follows that rule, there is no need to track every SoC lists. And what I have seen so far every this rule has been followed well.
(Added Benjamin, Deepthi and Paul)
I don't think everybody is following this rule, patches go to the SoC maintainer's tree without sometimes going through lakml.
Furthermore, there is not only ARM, there is also acpi_idle, intel_idle, pseries_idle and sh_idle, respectively located in:
drivers/acpi/processor_idle.c drivers/acpi/processor_driver.c drivers/idle/intel_idle.c
These ones above are under linux-pm, that is Rafael, like cpuidle, even if it is not marked in the MAINTAINERS file, so that should be ok.
And there is also:
arch/sh/kernel/cpu/shmobile/cpuidle.c arch/powerpc/platforms/pseries/processor_idle.c
And hopefully, some others in the right place, calxeda_idle and kirwood_idle located in drivers/cpuidle.
In the maintainer file, there is no information about cpuidle at all.
For example, if someone modify the cpuidle framework allowing to consolidate the code across the different drivers, we have to wait for the merge before using the new api into the different drivers. If we follow strictly the path of the merge tree we fall into a scenario where consolidating the drivers takes a loooooong time.
From my POV, *all* the cpuidle drivers must go under drivers/cpuidle, like cpufreq and pass through a single entry point to apply the patches, so the cpuidle framework and the drivers are always synced.
I can very much relate to the above mentioned problem, where code modifications done as a part of cpuidle framework needs to be reflected in every arch back-end cpuidle driver. We do have added advantages in moving all the back-end drivers into drivers/cpuidle. This would help us achieve better reviews, easier consolidations and more importantly maintaining sync btw drivers and framework and the up-streaming process.
But then, this means we get all the arch specific code out under drivers/cpuidle which can be very messy. Also instances where the changes are specifically tied only to the architecture of the back-end driver (SoC specific), it is absolutely necessary to get SoC maintainer's review.
Yes, I agree.
This is why an important work of separating the PM code from the cpuidle driver must be done in order to achieve this migration. Some of this has been done for the at91 and u8500 driver.
Thanks -- Daniel
Cheers, Deepthi
If everyone agree and we reach this consensus, then we can work to move these drivers to a single place.
I think Amit was suggesting to Cc me in the meantime, so while we are moving these drivers to this place, I can help to ensure we go to the same direction.
For example, Arnd Cc'ed me about the zynq cpuidle driver when it has been posted and, after review, it appeared it was totally obsolete wrt the modifications we did this year.
I propose first to add an entry in MAINTAINERS:
diff --git a/MAINTAINERS b/MAINTAINERS index 4cf5fd3..5b5ab87 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2206,6 +2206,15 @@ S: Maintained F: drivers/cpufreq/ F: include/linux/cpufreq.h
+CPU IDLE DRIVERS +M: Rafael J. Wysocki rjw@sisk.pl +L: cpuidle@vger.kernel.org +L: linux-pm@vger.kernel.org +S: Maintained +F: drivers/cpuidle/ +F: include/linux/cpuidle.h
CPUID/MSR DRIVER M: "H. Peter Anvin" hpa@zytor.com S: Maintained
Does it make sense ?
Thanks -- Daniel
On Mon, 2013-04-01 at 11:35 +0530, Deepthi Dharwar wrote:
But then, this means we get all the arch specific code out under drivers/cpuidle which can be very messy.
Not really no. We already have that here or there in other drivers, it's not necessarily messy and the stuff like that can generally be made reasonably self contained.
The main issue is that if I (powerpc) wants a fix in my some_ppc_box_idle.c driver, especially if it needs to sync with other arch changes, having to sync/ack with Rafael might complicate things a bit (though not necessarily a lot).
I would probably keep the liberty of sending to Linus directly urgent bug/regression fixes to individual cpuidle drivers relating to our archs without waiting every now and then if for example Rafael is on vacation :-)
Also instances where the changes are specifically tied only to the architecture of the back-end driver (SoC specific), it is absolutely necessary to get SoC maintainer's review.
Ben.
On Mon, Apr 01, 2013 at 10:29:48AM +0200, Benjamin Herrenschmidt wrote:
On Mon, 2013-04-01 at 11:35 +0530, Deepthi Dharwar wrote:
But then, this means we get all the arch specific code out under drivers/cpuidle which can be very messy.
Not really no. We already have that here or there in other drivers, it's not necessarily messy and the stuff like that can generally be made reasonably self contained.
The main issue is that if I (powerpc) wants a fix in my some_ppc_box_idle.c driver, especially if it needs to sync with other arch changes, having to sync/ack with Rafael might complicate things a bit (though not necessarily a lot).
I would probably keep the liberty of sending to Linus directly urgent bug/regression fixes to individual cpuidle drivers relating to our archs without waiting every now and then if for example Rafael is on vacation :-)
Merging them all over sounds like a good idea to me as well.
This isn't too different from how we handle other subsystems; as architecutre maintainer you just use your judgement on what needs an ack vs cc. Some smaller details about how the backend of the driver works on a platform is quite different from refactoring portions of the framework.
-Olof
Initialize the idle states directly in the driver structure.
That prevents extra structure declaration and memcpy at init time.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-tegra/cpuidle-tegra20.c | 40 ++++++++++++++++----------------- 1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index 825ced4..1ad1a67 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -43,28 +43,32 @@ static atomic_t abort_barrier; static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); +#define TEGRA20_MAX_STATES 2 +#else +#define TEGRA20_MAX_STATES 1 #endif
-static struct cpuidle_state tegra_idle_states[] = { - [0] = ARM_CPUIDLE_WFI_STATE_PWR(600), -#ifdef CONFIG_PM_SLEEP - [1] = { - .enter = tegra20_idle_lp2_coupled, - .exit_latency = 5000, - .target_residency = 10000, - .power_usage = 0, - .flags = CPUIDLE_FLAG_TIME_VALID | - CPUIDLE_FLAG_COUPLED, - .name = "powered-down", - .desc = "CPU power gated", - }, -#endif -}; - static struct cpuidle_driver tegra_idle_driver = { .name = "tegra_idle", .owner = THIS_MODULE, .en_core_tk_irqen = 1, + .states = { + ARM_CPUIDLE_WFI_STATE_PWR(600), +#ifdef CONFIG_PM_SLEEP + { + .enter = tegra20_idle_lp2_coupled, + .exit_latency = 5000, + .target_residency = 10000, + .power_usage = 0, + .flags = CPUIDLE_FLAG_TIME_VALID | + CPUIDLE_FLAG_COUPLED, + .name = "powered-down", + .desc = "CPU power gated", + }, +#endif + }, + .state_count = TEGRA20_MAX_STATES, + .safe_state_index = 0, };
static DEFINE_PER_CPU(struct cpuidle_device, tegra_idle_device); @@ -226,10 +230,6 @@ int __init tegra20_cpuidle_init(void) tegra_tear_down_cpu = tegra20_tear_down_cpu; #endif
- drv->state_count = ARRAY_SIZE(tegra_idle_states); - memcpy(drv->states, tegra_idle_states, - drv->state_count * sizeof(drv->states[0])); - ret = cpuidle_register_driver(&tegra_idle_driver); if (ret) { pr_err("CPUidle driver registration failed\n");
On 03/29/2013 04:31 AM, Daniel Lezcano wrote:
Initialize the idle states directly in the driver structure.
That prevents extra structure declaration and memcpy at init time.
Joseph, can you please review/ack patch 5/9 and patch 6/9. Thanks.
On 03/29/2013 05:02 PM, Stephen Warren wrote:
On 03/29/2013 04:31 AM, Daniel Lezcano wrote:
Initialize the idle states directly in the driver structure.
That prevents extra structure declaration and memcpy at init time.
Joseph, can you please review/ack patch 5/9 and patch 6/9. Thanks.
Thanks Joseph to review the patches.
Rafael is expecting an Acked-by.
If you are ok, with these patches, is it possible to ack them please ?
Thanks -- Daniel
On Wed, 2013-04-03 at 19:18 +0800, Daniel Lezcano wrote:
On 03/29/2013 05:02 PM, Stephen Warren wrote:
On 03/29/2013 04:31 AM, Daniel Lezcano wrote:
Initialize the idle states directly in the driver structure.
That prevents extra structure declaration and memcpy at init time.
Joseph, can you please review/ack patch 5/9 and patch 6/9. Thanks.
Thanks Joseph to review the patches.
Rafael is expecting an Acked-by.
If you are ok, with these patches, is it possible to ack them please ?
OK.
The 5/9 and 6/9 of this series. Acked-by: Joseph Lo josephl@nvidia.com
Thanks, Joseph
On 04/03/2013 01:23 PM, Joseph Lo wrote:
On Wed, 2013-04-03 at 19:18 +0800, Daniel Lezcano wrote:
On 03/29/2013 05:02 PM, Stephen Warren wrote:
On 03/29/2013 04:31 AM, Daniel Lezcano wrote:
Initialize the idle states directly in the driver structure.
That prevents extra structure declaration and memcpy at init time.
Joseph, can you please review/ack patch 5/9 and patch 6/9. Thanks.
Thanks Joseph to review the patches.
Rafael is expecting an Acked-by.
If you are ok, with these patches, is it possible to ack them please ?
OK.
The 5/9 and 6/9 of this series. Acked-by: Joseph Lo josephl@nvidia.com
Thanks Joseph.
On Fri, 2013-03-29 at 18:31 +0800, Daniel Lezcano wrote:
Initialize the idle states directly in the driver structure.
That prevents extra structure declaration and memcpy at init time.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Hi Daniel,
Thank you.
Both the 5th and 6th patch,
Reviewed-by: Joseph Lo josephl@nvidia.com
On 03/29/2013 08:22 PM, Joseph Lo wrote:
On Fri, 2013-03-29 at 18:31 +0800, Daniel Lezcano wrote:
Initialize the idle states directly in the driver structure.
That prevents extra structure declaration and memcpy at init time.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Hi Daniel,
Thank you.
Both the 5th and 6th patch,
Reviewed-by: Joseph Lo josephl@nvidia.com
Thanks. Those same two patches therefore,
Acked-by: Stephen Warren swarren@nvidia.com
dev->state_count is initialized automatically by cpuidle_register_device.
When drv->state_count is equal to dev->state_count, no need to init this field, so removing it.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-tegra/cpuidle-tegra114.c | 1 - arch/arm/mach-tegra/cpuidle-tegra20.c | 1 - arch/arm/mach-tegra/cpuidle-tegra30.c | 1 - 3 files changed, 3 deletions(-)
diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c index 0f4e8c4..c527cff 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra114.c +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c @@ -49,7 +49,6 @@ int __init tegra114_cpuidle_init(void) dev = &per_cpu(tegra_idle_device, cpu); dev->cpu = cpu;
- dev->state_count = drv->state_count; ret = cpuidle_register_device(dev); if (ret) { pr_err("CPU%u: CPUidle device registration failed\n", diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index 1ad1a67..b94d76a 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -243,7 +243,6 @@ int __init tegra20_cpuidle_init(void) dev->coupled_cpus = *cpu_possible_mask; #endif
- dev->state_count = drv->state_count; ret = cpuidle_register_device(dev); if (ret) { pr_err("CPU%u: CPUidle device registration failed\n", diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c index 8b50cf4..c4e01fe 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra30.c +++ b/arch/arm/mach-tegra/cpuidle-tegra30.c @@ -176,7 +176,6 @@ int __init tegra30_cpuidle_init(void) dev = &per_cpu(tegra_idle_device, cpu); dev->cpu = cpu;
- dev->state_count = drv->state_count; ret = cpuidle_register_device(dev); if (ret) { pr_err("CPU%u: CPUidle device registration failed\n",
The davinci_enter_idle is called from the cpuidle with the cpuidle_wrap_enter function. This one does the time compution for entering and exiting the idle function and then we call again cpuidle_wrap_enter for cpu_do_idle. This is wrong, we are calling recursively cpuidle_wrap_enter for nothing and furthermore reenabling the local irq.
Remove this and replace it by the cpu_do_idle function.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index 5ac9e93..22d6d4a 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -50,14 +50,10 @@ static void davinci_save_ddr_power(int enter, bool pdown)
/* Actual code that puts the SoC in different idle states */ static int davinci_enter_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index) + struct cpuidle_driver *drv, int index) { davinci_save_ddr_power(1, ddr2_pdown); - - index = cpuidle_wrap_enter(dev, drv, index, - arm_cpuidle_simple_enter); - + cpu_do_idle(); davinci_save_ddr_power(0, ddr2_pdown);
return index;
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The davinci_enter_idle is called from the cpuidle with the cpuidle_wrap_enter function. This one does the time compution for entering and exiting the idle function and then we call again cpuidle_wrap_enter for cpu_do_idle. This is wrong, we are calling recursively cpuidle_wrap_enter for nothing and furthermore reenabling the local irq.
Remove this and replace it by the cpu_do_idle function.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Looks right to me. Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
The start/stop_critical_timings are called from arch/x86/kernel/process.c in the cpu_idle loop function.
Remove the ones in the cpuidle driver.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- drivers/idle/intel_idle.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index 5d66750..c99c31e 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -339,7 +339,6 @@ static int intel_idle(struct cpuidle_device *dev, if (!(lapic_timer_reliable_states & (1 << (cstate)))) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
- stop_critical_timings(); if (!need_resched()) {
__monitor((void *)¤t_thread_info()->flags, 0, 0); @@ -348,8 +347,6 @@ static int intel_idle(struct cpuidle_device *dev, __mwait(eax, ecx); }
- start_critical_timings(); - if (!(lapic_timer_reliable_states & (1 << (cstate)))) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
The TIME_VALID flag is specified for the different states but the time residency computation is not done, no tk flag, no time computation in the idle function.
Set the en_core_tk_irqen flag to activate it.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-omap2/cpuidle34xx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c index 80392fc..4f67a5b 100644 --- a/arch/arm/mach-omap2/cpuidle34xx.c +++ b/arch/arm/mach-omap2/cpuidle34xx.c @@ -274,8 +274,9 @@ static int omap3_enter_idle_bm(struct cpuidle_device *dev, static DEFINE_PER_CPU(struct cpuidle_device, omap3_idle_dev);
static struct cpuidle_driver omap3_idle_driver = { - .name = "omap3_idle", - .owner = THIS_MODULE, + .name = "omap3_idle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, .states = { { .enter = omap3_enter_idle_bm,
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The TIME_VALID flag is specified for the different states but the time residency computation is not done, no tk flag, no time computation in the idle function.
Set the en_core_tk_irqen flag to activate it.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Acked-by: Santosh Shilimkar santosh.shilimkar@ti.com
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The noop functions code is not necessary because the header file is included in files which are compiled when CONFIG_CPU_IDLE is on.
Well the inline function was to avoid buid breaks when !CONFIG_CPU_IDLE.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Function without definition will break the build, No? Just declaration isn't won't help to get the build through.
Ofcourse if all idle drivers are build *only when* CONFIG_CPU_IDLE=y, then the patch is should be fine.
Regards, Santosh
On 03/29/2013 12:40 PM, Santosh Shilimkar wrote:
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote:
The noop functions code is not necessary because the header file is included in files which are compiled when CONFIG_CPU_IDLE is on.
Well the inline function was to avoid buid breaks when !CONFIG_CPU_IDLE.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org
Function without definition will break the build, No? Just declaration isn't won't help to get the build through.
Ofcourse if all idle drivers are build *only when* CONFIG_CPU_IDLE=y, then the patch is should be fine.
Yes, it is case AFAICT.
linaro-kernel@lists.linaro.org
-
Amit Kucheria -
Benjamin Herrenschmidt -
Daniel Lezcano -
Deepthi Dharwar -
Joseph Lo -
Olof Johansson -
Rafael J. Wysocki -
Santosh Shilimkar -
Stephen Warren