| Message ID | 1392312816-17657-13-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Feb 13, 2014 at 05:33:35PM +0000, Gregory CLEMENT wrote: [...] > diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c > new file mode 100644 > index 000000000000..57c69812e79d > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c > @@ -0,0 +1,120 @@ > +/* > + * Marvell Armada 370 and Armada XP SoC cpuidle driver > + * > + * Copyright (C) 2013 Marvell > + * > + * Nadav Haklai <nadavh@marvell.com> > + * Gregory CLEMENT <gregory.clement@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + * > + * Maintainer: Gregory CLEMENT <gregory.clement@free-electrons.com> > + */ > + > +#include <linux/cpu_pm.h> > +#include <linux/cpuidle.h> > +#include <linux/module.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/suspend.h> > +#include <asm/suspend.h> > +#include <linux/smp.h> > +#include <asm/cpuidle.h> > +#include <asm/smp_plat.h> > +#include <linux/platform_device.h> > +#include <asm/cp15.h> > +#include <asm/cacheflush.h> You should order them <linux/...>, then <asm/...> > + > +#define ARMADA_370_XP_MAX_STATES 3 Is this macro really needed ? > +#define ARMADA_370_XP_FLAG_DEEP_IDLE 0x10000 > +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle); > +extern void ll_clear_cpu_coherent(void); > +extern void ll_set_cpu_coherent(void); > + > +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) > +{ > + armada_370_xp_pmsu_idle_prepare(deepidle); > + > + v7_exit_coherency_flush(all); The macro above clears the C bit in SCTLR and exit coherency (clears SMP bit in SCTLR), let's keep this in mind, see below. > + ll_clear_cpu_coherent(); And the macro above uses ldr/str exclusives, and this is done with MMU on and off (on cold-boot before jumping to secondary_startup and also before jumping to cpu_resume in armada_370_xp_cpu_resume). Can you explain to me how load/store exclusives work on this platform ? ARM ARM A3.4.5 "It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be performed to a memory region with the Device or Strongly-ordered memory attribute. Unless the implementation documentation explicitly states that LDREX and STREX operations to a memory region with the Device or Strongly-ordered attribute are permitted, the effect of such operations is UNPREDICTABLE." At least code must be commented and an explanation on why this works has to be given. > + > + dsb(); > + > + wfi(); > + > + ll_set_cpu_coherent(); > + > + asm volatile( > + "mrc p15, 0, %0, c1, c0, 0 \n\t" > + "tst %0, #(1 << 2) \n\t" > + "orreq r0, %0, #(1 << 2) \n\t" > + "mcreq p15, 0, %0, c1, c0, 0 \n\t" > + "isb " > + : : "r" (0)); First of all, complex code like this must be commented. Moreover, this sequence is wrong. If wfi completes the kernel would explode. 1) where is the SMP bit in SCTLR restored ? 2) where are tlbs flushed (ie processors run out of coherency for _some_ time, so tlbs might be stale) ? > + > + return 0; > +} > + > +static int armada_370_xp_enter_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, > + int index) > +{ > + bool deepidle = false; > + cpu_pm_enter(); > + > + if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE) > + deepidle = true; > + > + cpu_suspend(deepidle, armada_370_xp_cpu_suspend); > + > + cpu_pm_exit(); > + > + return index; You should check the cpu_suspend return value and demote the idle state accordingly, if it failed. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/13/2014 06:33 PM, Gregory CLEMENT wrote: > Add the wfi, cpu idle and cpu deep idle power states support for the > Armada XP SoCs. > > All the latencies and the power consumption values used at the > "armada_370_xp_idle_driver" structure are preliminary and will be > modified in the future after running some measurements and analysis. > > Based on the work of Nadav Haklai. > > Signed-off-by: Nadav Haklai <nadavh@marvell.com> > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > --- > drivers/cpuidle/Kconfig.arm | 5 ++ > drivers/cpuidle/Makefile | 1 + > drivers/cpuidle/cpuidle-armada-370-xp.c | 120 ++++++++++++++++++++++++++++++++ > 3 files changed, 126 insertions(+) > create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c > > diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm > index d988948a89a0..f377eb0840e3 100644 > --- a/drivers/cpuidle/Kconfig.arm > +++ b/drivers/cpuidle/Kconfig.arm > @@ -1,6 +1,11 @@ > # > # ARM CPU Idle drivers > # > +config ARM_ARMADA_370_XP_CPUIDLE > + bool "CPU Idle Driver for Armada 370/XP family processors" > + depends on ARCH_MVEBU > + help > + Select this to enable cpuidle on Armada 370/XP processors. > > config ARM_BIG_LITTLE_CPUIDLE > bool "Support for ARM big.LITTLE processors" > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile > index f71ae1b373c5..9902d052bd87 100644 > --- a/drivers/cpuidle/Makefile > +++ b/drivers/cpuidle/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o > > ################################################################################## > # ARM SoC drivers > +obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += cpuidle-armada-370-xp.o > obj-$(CONFIG_ARM_BIG_LITTLE_CPUIDLE) += cpuidle-big_little.o > obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE) += cpuidle-calxeda.o > obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE) += cpuidle-kirkwood.o > diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c > new file mode 100644 > index 000000000000..57c69812e79d > --- /dev/null > +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c > @@ -0,0 +1,120 @@ > +/* > + * Marvell Armada 370 and Armada XP SoC cpuidle driver > + * > + * Copyright (C) 2013 Marvell > + * > + * Nadav Haklai <nadavh@marvell.com> > + * Gregory CLEMENT <gregory.clement@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + * > + * Maintainer: Gregory CLEMENT <gregory.clement@free-electrons.com> > + */ Hi Gregory, > +#include <linux/cpu_pm.h> > +#include <linux/cpuidle.h> > +#include <linux/module.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/suspend.h> > +#include <asm/suspend.h> > +#include <linux/smp.h> > +#include <asm/cpuidle.h> > +#include <asm/smp_plat.h> > +#include <linux/platform_device.h> > +#include <asm/cp15.h> > +#include <asm/cacheflush.h> the convention for the drivers inside this directory is to not include <asm/*>. > +#define ARMADA_370_XP_MAX_STATES 3 > +#define ARMADA_370_XP_FLAG_DEEP_IDLE 0x10000 > +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle); > +extern void ll_clear_cpu_coherent(void); > +extern void ll_set_cpu_coherent(void); Here you have to declare three low level functions which should not be visible in this driver. I think you can go one step more in the code encapsulation by doing like the cpuidle-at91 driver: * Move armada_370_xp_cpu_suspend inside arch/arm/mach-mvebu/pm.c or whereever you want * Assign the suspend function to the platform device data file * Get the suspend function as a callback for suspend from the field above > +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) > +{ > + armada_370_xp_pmsu_idle_prepare(deepidle); > + > + v7_exit_coherency_flush(all); > + > + ll_clear_cpu_coherent(); > + > + dsb(); > + > + wfi(); > + > + ll_set_cpu_coherent(); > + > + asm volatile( > + "mrc p15, 0, %0, c1, c0, 0 \n\t" > + "tst %0, #(1 << 2) \n\t" > + "orreq r0, %0, #(1 << 2) \n\t" > + "mcreq p15, 0, %0, c1, c0, 0 \n\t" > + "isb " > + : : "r" (0)); > + > + return 0; > +} > + > +static int armada_370_xp_enter_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, > + int index) > +{ > + bool deepidle = false; > + cpu_pm_enter(); > + > + if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE) > + deepidle = true; > + > + cpu_suspend(deepidle, armada_370_xp_cpu_suspend); > + > + cpu_pm_exit(); > + > + return index; > +} > + > +static struct cpuidle_driver armada_370_xp_idle_driver = { > + .name = "armada_370_xp_idle", > + .states[0] = ARM_CPUIDLE_WFI_STATE, > + .states[1] = { > + .enter = armada_370_xp_enter_idle, > + .exit_latency = 10, > + .power_usage = 50, > + .target_residency = 100, > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .name = "MV CPU IDLE", > + .desc = "CPU power down", > + }, > + .states[2] = { > + .enter = armada_370_xp_enter_idle, > + .exit_latency = 100, > + .power_usage = 5, > + .target_residency = 1000, > + .flags = CPUIDLE_FLAG_TIME_VALID | > + ARMADA_370_XP_FLAG_DEEP_IDLE, > + .name = "MV CPU DEEP IDLE", > + .desc = "CPU and L2 Fabric power down", > + }, > + .state_count = ARMADA_370_XP_MAX_STATES, > +}; > + > +static int armada_370_xp_cpuidle_probe(struct platform_device *pdev) > +{ > + return cpuidle_register(&armada_370_xp_idle_driver, NULL); > +} > + > +static struct platform_driver armada_370_xp_cpuidle_plat_driver = { > + .driver = { > + .name = "cpuidle-armada-370-xp", > + .owner = THIS_MODULE, > + }, > + .probe = armada_370_xp_cpuidle_probe, > +}; > + > +module_platform_driver(armada_370_xp_cpuidle_plat_driver); > + > +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>"); > +MODULE_DESCRIPTION("Armada 370/XP cpu idle driver"); > +MODULE_LICENSE("GPL"); >
Dear Gregory CLEMENT, On Thu, 13 Feb 2014 18:33:35 +0100, Gregory CLEMENT wrote: > +config ARM_ARMADA_370_XP_CPUIDLE > + bool "CPU Idle Driver for Armada 370/XP family processors" Sorry to bring the naming issue, but it looks like the Armada 38x has a PMSU that looks almost identical to the Armada XP PMSU, except that it doesn't have the L2 fabric registers (probably because Armada 38x uses the PL310 and not the Marvell L2 cache). Therefore, should this cpuidle driver be named Armada 370/XP, or ARMADA_MVEBU for example? > +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) > +{ > + armada_370_xp_pmsu_idle_prepare(deepidle); > + > + v7_exit_coherency_flush(all); > + > + ll_clear_cpu_coherent(); > + > + dsb(); > + > + wfi(); > + > + ll_set_cpu_coherent(); > + > + asm volatile( > + "mrc p15, 0, %0, c1, c0, 0 \n\t" > + "tst %0, #(1 << 2) \n\t" > + "orreq r0, %0, #(1 << 2) \n\t" > + "mcreq p15, 0, %0, c1, c0, 0 \n\t" > + "isb " > + : : "r" (0)); I believe a little comment on top of this assembly block would be good to have, to at least give a quick idea on what's going on. Also, I'm a bit unsure about your choice of mixing C and assembly here. This function is already calling ll_clear_cpu_coherent() and ll_set_cpu_coherent() that are assembly functions implement in coherency_ll.S. Shouldn't we do the same for this final bit of assembly? Thomas
On 19/02/2014 17:51, Thomas Petazzoni wrote: > Dear Gregory CLEMENT, > > On Thu, 13 Feb 2014 18:33:35 +0100, Gregory CLEMENT wrote: > >> +config ARM_ARMADA_370_XP_CPUIDLE >> + bool "CPU Idle Driver for Armada 370/XP family processors" > > Sorry to bring the naming issue, but it looks like the Armada 38x has a > PMSU that looks almost identical to the Armada XP PMSU, except that it > doesn't have the L2 fabric registers (probably because Armada 38x uses > the PL310 and not the Marvell L2 cache). > > Therefore, should this cpuidle driver be named Armada 370/XP, or > ARMADA_MVEBU for example? Well most of the code is related to the coherency and the L2 cache, so a different L2 cache is a significant difference. The CPU are also different for example the PJ4B can use LDREX/STREX without MMU whereas the Cortex A9 can't. > >> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) > > > >> +{ >> + armada_370_xp_pmsu_idle_prepare(deepidle); >> + >> + v7_exit_coherency_flush(all); >> + >> + ll_clear_cpu_coherent(); >> + >> + dsb(); >> + >> + wfi(); >> + >> + ll_set_cpu_coherent(); >> + >> + asm volatile( >> + "mrc p15, 0, %0, c1, c0, 0 \n\t" >> + "tst %0, #(1 << 2) \n\t" >> + "orreq r0, %0, #(1 << 2) \n\t" >> + "mcreq p15, 0, %0, c1, c0, 0 \n\t" >> + "isb " >> + : : "r" (0)); > > I believe a little comment on top of this assembly block would be good > to have, to at least give a quick idea on what's going on. I am on it, I had the same remark from Lorenzo Pieralisi. > > Also, I'm a bit unsure about your choice of mixing C and assembly here. > This function is already calling ll_clear_cpu_coherent() and > ll_set_cpu_coherent() that are assembly functions implement in > coherency_ll.S. Shouldn't we do the same for this final bit of assembly? I made several tries when I converted most of the code in C, so I am not sure but I think that using a C function didn't work here. But as Lorenzo pointed they were mistakes in this code, so once I will have fixed them, I will try again. Thanks, Gregory > > Thomas >
Dear Gregory CLEMENT, On Wed, 19 Feb 2014 18:19:51 +0100, Gregory CLEMENT wrote: > > Sorry to bring the naming issue, but it looks like the Armada 38x has a > > PMSU that looks almost identical to the Armada XP PMSU, except that it > > doesn't have the L2 fabric registers (probably because Armada 38x uses > > the PL310 and not the Marvell L2 cache). > > > > Therefore, should this cpuidle driver be named Armada 370/XP, or > > ARMADA_MVEBU for example? > > Well most of the code is related to the coherency and the L2 cache, so > a different L2 cache is a significant difference. The CPU are also different > for example the PJ4B can use LDREX/STREX without MMU whereas the Cortex A9 > can't. Right, ok. > > Also, I'm a bit unsure about your choice of mixing C and assembly here. > > This function is already calling ll_clear_cpu_coherent() and > > ll_set_cpu_coherent() that are assembly functions implement in > > coherency_ll.S. Shouldn't we do the same for this final bit of assembly? > > I made several tries when I converted most of the code in C, so I am not > sure but I think that using a C function didn't work here. But as Lorenzo > pointed they were mistakes in this code, so once I will have fixed them, I > will try again. Having this is C would certainly be a lot better, but my comment was merely to move this tiny bit of assembly somewhere else, but keep it as assembly if it's really needed. Thomas
On 14/02/2014 18:00, Lorenzo Pieralisi wrote: > On Thu, Feb 13, 2014 at 05:33:35PM +0000, Gregory CLEMENT wrote: > > [...] > >> diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c >> new file mode 100644 >> index 000000000000..57c69812e79d >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c >> @@ -0,0 +1,120 @@ >> +/* >> + * Marvell Armada 370 and Armada XP SoC cpuidle driver >> + * >> + * Copyright (C) 2013 Marvell >> + * >> + * Nadav Haklai <nadavh@marvell.com> >> + * Gregory CLEMENT <gregory.clement@free-electrons.com> >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + * >> + * Maintainer: Gregory CLEMENT <gregory.clement@free-electrons.com> >> + */ >> + >> +#include <linux/cpu_pm.h> >> +#include <linux/cpuidle.h> >> +#include <linux/module.h> >> +#include <linux/io.h> >> +#include <linux/of.h> >> +#include <linux/suspend.h> >> +#include <asm/suspend.h> >> +#include <linux/smp.h> >> +#include <asm/cpuidle.h> >> +#include <asm/smp_plat.h> >> +#include <linux/platform_device.h> >> +#include <asm/cp15.h> >> +#include <asm/cacheflush.h> > > You should order them <linux/...>, then <asm/...> OK done in v5 > >> + >> +#define ARMADA_370_XP_MAX_STATES 3 > > Is this macro really needed ? Well most of the other driver use it. And instead of having this number directly written in the state_count field I prefer to using a name for this value. But I think it is more a matter of taste here. > >> +#define ARMADA_370_XP_FLAG_DEEP_IDLE 0x10000 >> +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle); >> +extern void ll_clear_cpu_coherent(void); >> +extern void ll_set_cpu_coherent(void); >> + >> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) >> +{ >> + armada_370_xp_pmsu_idle_prepare(deepidle); >> + >> + v7_exit_coherency_flush(all); > > The macro above clears the C bit in SCTLR and exit coherency (clears SMP > bit in SCTLR), let's keep this in mind, see below. > >> + ll_clear_cpu_coherent(); > > And the macro above uses ldr/str exclusives, and this is done with MMU > on and off (on cold-boot before jumping to secondary_startup and also > before jumping to cpu_resume in armada_370_xp_cpu_resume). > > Can you explain to me how load/store exclusives work on this platform ? > > ARM ARM A3.4.5 > > "It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be > performed to a memory region with the Device or Strongly-ordered memory > attribute. Unless the implementation documentation explicitly states that > LDREX and STREX operations to a memory region with the Device or > Strongly-ordered attribute are permitted, the effect of such operations is > UNPREDICTABLE." > Armada XP has an exclusive monitor that can track transactions to Device and/or SO and as such also when MMU is disabled the exclusive transactions will be functional. > At least code must be commented and an explanation on why this works has > to be given. I have added this information in comment for the v5. > >> + >> + dsb(); >> + >> + wfi(); >> + >> + ll_set_cpu_coherent(); >> + >> + asm volatile( >> + "mrc p15, 0, %0, c1, c0, 0 \n\t" >> + "tst %0, #(1 << 2) \n\t" >> + "orreq r0, %0, #(1 << 2) \n\t" >> + "mcreq p15, 0, %0, c1, c0, 0 \n\t" >> + "isb " >> + : : "r" (0)); > > First of all, complex code like this must be commented. > > Moreover, this sequence is wrong. If wfi completes the kernel would explode. > > 1) where is the SMP bit in SCTLR restored ? It is restored in this uncommeted piece of code. I added comment now in the v5. > 2) where are tlbs flushed (ie processors run out of coherency for _some_ > time, so tlbs might be stale) ? Right it was missing, I added it. > >> + >> + return 0; >> +} >> + >> +static int armada_370_xp_enter_idle(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, >> + int index) >> +{ >> + bool deepidle = false; >> + cpu_pm_enter(); >> + >> + if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE) >> + deepidle = true; >> + >> + cpu_suspend(deepidle, armada_370_xp_cpu_suspend); >> + >> + cpu_pm_exit(); >> + >> + return index; > > You should check the cpu_suspend return value and demote the idle state > accordingly, if it failed. Done in v5. > > Thanks, > Lorenzo >
On 17/02/2014 09:49, Daniel Lezcano wrote: > On 02/13/2014 06:33 PM, Gregory CLEMENT wrote: >> Add the wfi, cpu idle and cpu deep idle power states support for the >> Armada XP SoCs. >> >> All the latencies and the power consumption values used at the >> "armada_370_xp_idle_driver" structure are preliminary and will be >> modified in the future after running some measurements and analysis. >> >> Based on the work of Nadav Haklai. >> >> Signed-off-by: Nadav Haklai <nadavh@marvell.com> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> --- >> drivers/cpuidle/Kconfig.arm | 5 ++ >> drivers/cpuidle/Makefile | 1 + >> drivers/cpuidle/cpuidle-armada-370-xp.c | 120 ++++++++++++++++++++++++++++++++ >> 3 files changed, 126 insertions(+) >> create mode 100644 drivers/cpuidle/cpuidle-armada-370-xp.c >> >> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm >> index d988948a89a0..f377eb0840e3 100644 >> --- a/drivers/cpuidle/Kconfig.arm >> +++ b/drivers/cpuidle/Kconfig.arm >> @@ -1,6 +1,11 @@ >> # >> # ARM CPU Idle drivers >> # >> +config ARM_ARMADA_370_XP_CPUIDLE >> + bool "CPU Idle Driver for Armada 370/XP family processors" >> + depends on ARCH_MVEBU >> + help >> + Select this to enable cpuidle on Armada 370/XP processors. >> >> config ARM_BIG_LITTLE_CPUIDLE >> bool "Support for ARM big.LITTLE processors" >> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile >> index f71ae1b373c5..9902d052bd87 100644 >> --- a/drivers/cpuidle/Makefile >> +++ b/drivers/cpuidle/Makefile >> @@ -7,6 +7,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o >> >> ################################################################################## >> # ARM SoC drivers >> +obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += cpuidle-armada-370-xp.o >> obj-$(CONFIG_ARM_BIG_LITTLE_CPUIDLE) += cpuidle-big_little.o >> obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE) += cpuidle-calxeda.o >> obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE) += cpuidle-kirkwood.o >> diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c >> new file mode 100644 >> index 000000000000..57c69812e79d >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c >> @@ -0,0 +1,120 @@ >> +/* >> + * Marvell Armada 370 and Armada XP SoC cpuidle driver >> + * >> + * Copyright (C) 2013 Marvell >> + * >> + * Nadav Haklai <nadavh@marvell.com> >> + * Gregory CLEMENT <gregory.clement@free-electrons.com> >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + * >> + * Maintainer: Gregory CLEMENT <gregory.clement@free-electrons.com> >> + */ > > Hi Gregory, > >> +#include <linux/cpu_pm.h> >> +#include <linux/cpuidle.h> >> +#include <linux/module.h> >> +#include <linux/io.h> >> +#include <linux/of.h> >> +#include <linux/suspend.h> >> +#include <asm/suspend.h> >> +#include <linux/smp.h> >> +#include <asm/cpuidle.h> >> +#include <asm/smp_plat.h> >> +#include <linux/platform_device.h> >> +#include <asm/cp15.h> >> +#include <asm/cacheflush.h> > > the convention for the drivers inside this directory is to not include > <asm/*>. Well it is not the case yet: git grep asm drivers/cpuidle/cpuidle-* | wc -l 25 Most of the time it was asm/cpuidle.h which was needed for using ARM_CPUIDLE_WFI_STATE. However I try to remove most of the header from asm/ by following your other advises. > >> +#define ARMADA_370_XP_MAX_STATES 3 >> +#define ARMADA_370_XP_FLAG_DEEP_IDLE 0x10000 >> +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle); >> +extern void ll_clear_cpu_coherent(void); >> +extern void ll_set_cpu_coherent(void); > > Here you have to declare three low level functions which should not be > visible in this driver. > > I think you can go one step more in the code encapsulation by doing like > the cpuidle-at91 driver: > * Move armada_370_xp_cpu_suspend inside arch/arm/mach-mvebu/pm.c or > whereever you want > * Assign the suspend function to the platform device data file > * Get the suspend function as a callback for suspend from the field above Ok I have done it in the 5th version. > > >> +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) >> +{ >> + armada_370_xp_pmsu_idle_prepare(deepidle); >> + >> + v7_exit_coherency_flush(all); >> + >> + ll_clear_cpu_coherent(); >> + >> + dsb(); >> + >> + wfi(); >> + >> + ll_set_cpu_coherent(); >> + >> + asm volatile( >> + "mrc p15, 0, %0, c1, c0, 0 \n\t" >> + "tst %0, #(1 << 2) \n\t" >> + "orreq r0, %0, #(1 << 2) \n\t" >> + "mcreq p15, 0, %0, c1, c0, 0 \n\t" >> + "isb " >> + : : "r" (0)); >> + >> + return 0; >> +} >> + >> +static int armada_370_xp_enter_idle(struct cpuidle_device *dev, >> + struct cpuidle_driver *drv, >> + int index) >> +{ >> + bool deepidle = false; >> + cpu_pm_enter(); >> + >> + if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE) >> + deepidle = true; >> + >> + cpu_suspend(deepidle, armada_370_xp_cpu_suspend); >> + >> + cpu_pm_exit(); >> + >> + return index; >> +} >> + >> +static struct cpuidle_driver armada_370_xp_idle_driver = { >> + .name = "armada_370_xp_idle", >> + .states[0] = ARM_CPUIDLE_WFI_STATE, >> + .states[1] = { >> + .enter = armada_370_xp_enter_idle, >> + .exit_latency = 10, >> + .power_usage = 50, >> + .target_residency = 100, >> + .flags = CPUIDLE_FLAG_TIME_VALID, >> + .name = "MV CPU IDLE", >> + .desc = "CPU power down", >> + }, >> + .states[2] = { >> + .enter = armada_370_xp_enter_idle, >> + .exit_latency = 100, >> + .power_usage = 5, >> + .target_residency = 1000, >> + .flags = CPUIDLE_FLAG_TIME_VALID | >> + ARMADA_370_XP_FLAG_DEEP_IDLE, >> + .name = "MV CPU DEEP IDLE", >> + .desc = "CPU and L2 Fabric power down", >> + }, >> + .state_count = ARMADA_370_XP_MAX_STATES, >> +}; >> + >> +static int armada_370_xp_cpuidle_probe(struct platform_device *pdev) >> +{ >> + return cpuidle_register(&armada_370_xp_idle_driver, NULL); >> +} >> + >> +static struct platform_driver armada_370_xp_cpuidle_plat_driver = { >> + .driver = { >> + .name = "cpuidle-armada-370-xp", >> + .owner = THIS_MODULE, >> + }, >> + .probe = armada_370_xp_cpuidle_probe, >> +}; >> + >> +module_platform_driver(armada_370_xp_cpuidle_plat_driver); >> + >> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>"); >> +MODULE_DESCRIPTION("Armada 370/XP cpu idle driver"); >> +MODULE_LICENSE("GPL"); >> > >
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm index d988948a89a0..f377eb0840e3 100644 --- a/drivers/cpuidle/Kconfig.arm +++ b/drivers/cpuidle/Kconfig.arm @@ -1,6 +1,11 @@ # # ARM CPU Idle drivers # +config ARM_ARMADA_370_XP_CPUIDLE + bool "CPU Idle Driver for Armada 370/XP family processors" + depends on ARCH_MVEBU + help + Select this to enable cpuidle on Armada 370/XP processors. config ARM_BIG_LITTLE_CPUIDLE bool "Support for ARM big.LITTLE processors" diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile index f71ae1b373c5..9902d052bd87 100644 --- a/drivers/cpuidle/Makefile +++ b/drivers/cpuidle/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o ################################################################################## # ARM SoC drivers +obj-$(CONFIG_ARM_ARMADA_370_XP_CPUIDLE) += cpuidle-armada-370-xp.o obj-$(CONFIG_ARM_BIG_LITTLE_CPUIDLE) += cpuidle-big_little.o obj-$(CONFIG_ARM_HIGHBANK_CPUIDLE) += cpuidle-calxeda.o obj-$(CONFIG_ARM_KIRKWOOD_CPUIDLE) += cpuidle-kirkwood.o diff --git a/drivers/cpuidle/cpuidle-armada-370-xp.c b/drivers/cpuidle/cpuidle-armada-370-xp.c new file mode 100644 index 000000000000..57c69812e79d --- /dev/null +++ b/drivers/cpuidle/cpuidle-armada-370-xp.c @@ -0,0 +1,120 @@ +/* + * Marvell Armada 370 and Armada XP SoC cpuidle driver + * + * Copyright (C) 2013 Marvell + * + * Nadav Haklai <nadavh@marvell.com> + * Gregory CLEMENT <gregory.clement@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + * + * Maintainer: Gregory CLEMENT <gregory.clement@free-electrons.com> + */ + +#include <linux/cpu_pm.h> +#include <linux/cpuidle.h> +#include <linux/module.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/suspend.h> +#include <asm/suspend.h> +#include <linux/smp.h> +#include <asm/cpuidle.h> +#include <asm/smp_plat.h> +#include <linux/platform_device.h> +#include <asm/cp15.h> +#include <asm/cacheflush.h> + +#define ARMADA_370_XP_MAX_STATES 3 +#define ARMADA_370_XP_FLAG_DEEP_IDLE 0x10000 +extern void armada_370_xp_pmsu_idle_prepare(bool deepidle); +extern void ll_clear_cpu_coherent(void); +extern void ll_set_cpu_coherent(void); + +noinline static int armada_370_xp_cpu_suspend(unsigned long deepidle) +{ + armada_370_xp_pmsu_idle_prepare(deepidle); + + v7_exit_coherency_flush(all); + + ll_clear_cpu_coherent(); + + dsb(); + + wfi(); + + ll_set_cpu_coherent(); + + asm volatile( + "mrc p15, 0, %0, c1, c0, 0 \n\t" + "tst %0, #(1 << 2) \n\t" + "orreq r0, %0, #(1 << 2) \n\t" + "mcreq p15, 0, %0, c1, c0, 0 \n\t" + "isb " + : : "r" (0)); + + return 0; +} + +static int armada_370_xp_enter_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) +{ + bool deepidle = false; + cpu_pm_enter(); + + if (drv->states[index].flags & ARMADA_370_XP_FLAG_DEEP_IDLE) + deepidle = true; + + cpu_suspend(deepidle, armada_370_xp_cpu_suspend); + + cpu_pm_exit(); + + return index; +} + +static struct cpuidle_driver armada_370_xp_idle_driver = { + .name = "armada_370_xp_idle", + .states[0] = ARM_CPUIDLE_WFI_STATE, + .states[1] = { + .enter = armada_370_xp_enter_idle, + .exit_latency = 10, + .power_usage = 50, + .target_residency = 100, + .flags = CPUIDLE_FLAG_TIME_VALID, + .name = "MV CPU IDLE", + .desc = "CPU power down", + }, + .states[2] = { + .enter = armada_370_xp_enter_idle, + .exit_latency = 100, + .power_usage = 5, + .target_residency = 1000, + .flags = CPUIDLE_FLAG_TIME_VALID | + ARMADA_370_XP_FLAG_DEEP_IDLE, + .name = "MV CPU DEEP IDLE", + .desc = "CPU and L2 Fabric power down", + }, + .state_count = ARMADA_370_XP_MAX_STATES, +}; + +static int armada_370_xp_cpuidle_probe(struct platform_device *pdev) +{ + return cpuidle_register(&armada_370_xp_idle_driver, NULL); +} + +static struct platform_driver armada_370_xp_cpuidle_plat_driver = { + .driver = { + .name = "cpuidle-armada-370-xp", + .owner = THIS_MODULE, + }, + .probe = armada_370_xp_cpuidle_probe, +}; + +module_platform_driver(armada_370_xp_cpuidle_plat_driver); + +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@free-electrons.com>"); +MODULE_DESCRIPTION("Armada 370/XP cpu idle driver"); +MODULE_LICENSE("GPL");