Message ID | 1369645193-3595-11-git-send-email-horms+renesas@verge.net.au (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 05/27/2013 10:59 AM, Simon Horman wrote: > From: Bastian Hecht <hechtb@gmail.com> > > We make use of the r8a7740 Suspend To Ram code to plug together a > CPUIdle driver. > > Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- Shouldn't it go through Rafael's tree ? Or does the patch contains some dependencies on a code only visible in the ARM tree ? > arch/arm/mach-shmobile/Makefile | 2 +- > arch/arm/mach-shmobile/cpuidle-r8a7740.c | 62 +++++++++++++++++++++++++ > arch/arm/mach-shmobile/include/mach/r8a7740.h | 1 + > arch/arm/mach-shmobile/pm-r8a7740.c | 1 + > 4 files changed, 65 insertions(+), 1 deletion(-) > create mode 100644 arch/arm/mach-shmobile/cpuidle-r8a7740.c > > diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile > index 0568894..2852dcf 100644 > --- a/arch/arm/mach-shmobile/Makefile > +++ b/arch/arm/mach-shmobile/Makefile > @@ -30,7 +30,7 @@ obj-$(CONFIG_SUSPEND) += suspend.o > obj-$(CONFIG_CPU_IDLE) += cpuidle.o > obj-$(CONFIG_ARCH_SHMOBILE) += pm-rmobile.o > obj-$(CONFIG_ARCH_SH7372) += pm-sh7372.o sleep-sh7372.o > -obj-$(CONFIG_ARCH_R8A7740) += pm-r8a7740.o sleep-r8a7740.o > +obj-$(CONFIG_ARCH_R8A7740) += pm-r8a7740.o sleep-r8a7740.o cpuidle-r8a7740.o > obj-$(CONFIG_ARCH_R8A7779) += pm-r8a7779.o > obj-$(CONFIG_ARCH_SH73A0) += pm-sh73a0.o > > diff --git a/arch/arm/mach-shmobile/cpuidle-r8a7740.c b/arch/arm/mach-shmobile/cpuidle-r8a7740.c > new file mode 100644 > index 0000000..48c7a6c > --- /dev/null > +++ b/arch/arm/mach-shmobile/cpuidle-r8a7740.c > @@ -0,0 +1,62 @@ > +/* > + * CPUIdle code for SoC r8a7740 > + * > + * Copyright (C) 2013 Bastian Hecht > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details. > + */ > +#include <linux/cpuidle.h> > +#include <linux/module.h> > +#include <asm/cpuidle.h> > +#include <mach/common.h> > +#include <mach/r8a7740.h> > + > +#if defined(CONFIG_SUSPEND) && defined(CONFIG_CPU_IDLE) > +static int r8a7740_enter_a3sm_pll_on(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + r8a7740_enter_a3sm_common(1); > + return 1; > +} > + > +static int r8a7740_enter_a3sm_pll_off(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + r8a7740_enter_a3sm_common(0); > + return 2; > +} > + > +static struct cpuidle_driver r8a7740_cpuidle_driver = { > + .name = "r8a7740_cpuidle", > + .owner = THIS_MODULE, > + .en_core_tk_irqen = 1, > + .state_count = 3, > + .safe_state_index = 0, /* C1 */ > + .states[0] = ARM_CPUIDLE_WFI_STATE, > + .states[1] = { > + .name = "C2", > + .desc = "A3SM PLL ON", > + .exit_latency = 40, > + .target_residency = 30 + 40, > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .enter = r8a7740_enter_a3sm_pll_on, > + }, > + .states[2] = { > + .name = "C3", > + .desc = "A3SM PLL OFF", > + .exit_latency = 120, > + .target_residency = 30 + 120, > + .flags = CPUIDLE_FLAG_TIME_VALID, > + .enter = r8a7740_enter_a3sm_pll_off, > + }, > +}; > + > +void r8a7740_cpuidle_init(void) > +{ > + shmobile_cpuidle_set_driver(&r8a7740_cpuidle_driver); > +} > +#else > +void r8a7740_cpuidle_init(void) {} > +#endif > diff --git a/arch/arm/mach-shmobile/include/mach/r8a7740.h b/arch/arm/mach-shmobile/include/mach/r8a7740.h > index 5cfe5d9..3b538e3 100644 > --- a/arch/arm/mach-shmobile/include/mach/r8a7740.h > +++ b/arch/arm/mach-shmobile/include/mach/r8a7740.h > @@ -545,6 +545,7 @@ extern void r8a7740_pinmux_init(void); > extern void r8a7740_pm_init(void); > extern void r8a7740_resume(void); > extern void r8a7740_shutdown(void); > +extern void r8a7740_cpuidle_init(void); > extern void r8a7740_enter_a3sm_common(int); > > #ifdef CONFIG_PM > diff --git a/arch/arm/mach-shmobile/pm-r8a7740.c b/arch/arm/mach-shmobile/pm-r8a7740.c > index fe3c867..2f196bd 100644 > --- a/arch/arm/mach-shmobile/pm-r8a7740.c > +++ b/arch/arm/mach-shmobile/pm-r8a7740.c > @@ -237,4 +237,5 @@ static void r8a7740_suspend_init(void) {} > void __init r8a7740_pm_init(void) > { > r8a7740_suspend_init(); > + r8a7740_cpuidle_init(); > } >
On Mon, May 27, 2013 at 01:03:31PM +0200, Daniel Lezcano wrote: > On 05/27/2013 10:59 AM, Simon Horman wrote: > > From: Bastian Hecht <hechtb@gmail.com> > > > > We make use of the r8a7740 Suspend To Ram code to plug together a > > CPUIdle driver. > > > > Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com> > > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > --- > > Shouldn't it go through Rafael's tree ? Or does the patch contains some > dependencies on a code only visible in the ARM tree ? Missing S-o-b from Simon. But this patch clearly builds on the preceding one in the series, so merging them independently might not make much sense. Getting an ack from Rafael would be nice though. I was going to say that it should probably go under drivers/cpuidle as well, but that just seems silly -- there is practically no code to share with any other platform in this small driver, AND there's not really any subsystem-internal data exposed. So it might just make more sense to keep it under arch/arm instead. Likewise, looking at the kirkwood and calxeda drivers under drivers/cpuidle, I'm wondering why we thought it was a good idea to merge them there, besides getting caught up in the "nothing can live under arch/arm any more" frenzy. -Olof
On 05/28/2013 05:54 AM, Olof Johansson wrote: > On Mon, May 27, 2013 at 01:03:31PM +0200, Daniel Lezcano wrote: >> On 05/27/2013 10:59 AM, Simon Horman wrote: >>> From: Bastian Hecht <hechtb@gmail.com> >>> >>> We make use of the r8a7740 Suspend To Ram code to plug together a >>> CPUIdle driver. >>> >>> Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com> >>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> --- >> >> Shouldn't it go through Rafael's tree ? Or does the patch contains some >> dependencies on a code only visible in the ARM tree ? > > Missing S-o-b from Simon. But this patch clearly builds on the preceding > one in the series, so merging them independently might not make much > sense. Getting an ack from Rafael would be nice though. I was not suggesting to put the driver in the drivers/cpuidle directory but to merge the driver through Rafael's tree as we decided some weeks ago [1]. Although having the drivers all over the place does not help to consolidate the code, so moving them little by little to drivers/cpuidle makes sense but this is part of another work. > I was going to say that it should probably go under drivers/cpuidle as > well, but that just seems silly -- there is practically no code to share > with any other platform in this small driver, AND there's not really > any subsystem-internal data exposed. So it might just make more sense > to keep it under arch/arm instead. > > Likewise, looking at the kirkwood and calxeda drivers under drivers/cpuidle, > I'm wondering why we thought it was a good idea to merge them there, besides > getting caught up in the "nothing can live under arch/arm any more" frenzy. Having the drivers in the drivers/cpuidle directory like drivers/cpufreq will help to keep a consistency with the code and a single entry point for upstream and review. Thanks. -- Daniel [1] https://patchwork.kernel.org/patch/2492841/
On Mon, May 27, 2013 at 08:54:46PM -0700, Olof Johansson wrote: > On Mon, May 27, 2013 at 01:03:31PM +0200, Daniel Lezcano wrote: > > On 05/27/2013 10:59 AM, Simon Horman wrote: > > > From: Bastian Hecht <hechtb@gmail.com> > > > > > > We make use of the r8a7740 Suspend To Ram code to plug together a > > > CPUIdle driver. > > > > > > Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com> > > > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > > --- > > > > Shouldn't it go through Rafael's tree ? Or does the patch contains some > > dependencies on a code only visible in the ARM tree ? > > Missing S-o-b from Simon. But this patch clearly builds on the preceding > one in the series, so merging them independently might not make much > sense. Getting an ack from Rafael would be nice though. > > I was going to say that it should probably go under drivers/cpuidle as > well, but that just seems silly -- there is practically no code to share > with any other platform in this small driver, AND there's not really > any subsystem-internal data exposed. So it might just make more sense > to keep it under arch/arm instead. > > Likewise, looking at the kirkwood and calxeda drivers under drivers/cpuidle, > I'm wondering why we thought it was a good idea to merge them there, besides > getting caught up in the "nothing can live under arch/arm any more" frenzy. After discussion with Magnus I have decided to drop these changes. We can revisit them for a future release.
diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile index 0568894..2852dcf 100644 --- a/arch/arm/mach-shmobile/Makefile +++ b/arch/arm/mach-shmobile/Makefile @@ -30,7 +30,7 @@ obj-$(CONFIG_SUSPEND) += suspend.o obj-$(CONFIG_CPU_IDLE) += cpuidle.o obj-$(CONFIG_ARCH_SHMOBILE) += pm-rmobile.o obj-$(CONFIG_ARCH_SH7372) += pm-sh7372.o sleep-sh7372.o -obj-$(CONFIG_ARCH_R8A7740) += pm-r8a7740.o sleep-r8a7740.o +obj-$(CONFIG_ARCH_R8A7740) += pm-r8a7740.o sleep-r8a7740.o cpuidle-r8a7740.o obj-$(CONFIG_ARCH_R8A7779) += pm-r8a7779.o obj-$(CONFIG_ARCH_SH73A0) += pm-sh73a0.o diff --git a/arch/arm/mach-shmobile/cpuidle-r8a7740.c b/arch/arm/mach-shmobile/cpuidle-r8a7740.c new file mode 100644 index 0000000..48c7a6c --- /dev/null +++ b/arch/arm/mach-shmobile/cpuidle-r8a7740.c @@ -0,0 +1,62 @@ +/* + * CPUIdle code for SoC r8a7740 + * + * Copyright (C) 2013 Bastian Hecht + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + */ +#include <linux/cpuidle.h> +#include <linux/module.h> +#include <asm/cpuidle.h> +#include <mach/common.h> +#include <mach/r8a7740.h> + +#if defined(CONFIG_SUSPEND) && defined(CONFIG_CPU_IDLE) +static int r8a7740_enter_a3sm_pll_on(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + r8a7740_enter_a3sm_common(1); + return 1; +} + +static int r8a7740_enter_a3sm_pll_off(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + r8a7740_enter_a3sm_common(0); + return 2; +} + +static struct cpuidle_driver r8a7740_cpuidle_driver = { + .name = "r8a7740_cpuidle", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, + .state_count = 3, + .safe_state_index = 0, /* C1 */ + .states[0] = ARM_CPUIDLE_WFI_STATE, + .states[1] = { + .name = "C2", + .desc = "A3SM PLL ON", + .exit_latency = 40, + .target_residency = 30 + 40, + .flags = CPUIDLE_FLAG_TIME_VALID, + .enter = r8a7740_enter_a3sm_pll_on, + }, + .states[2] = { + .name = "C3", + .desc = "A3SM PLL OFF", + .exit_latency = 120, + .target_residency = 30 + 120, + .flags = CPUIDLE_FLAG_TIME_VALID, + .enter = r8a7740_enter_a3sm_pll_off, + }, +}; + +void r8a7740_cpuidle_init(void) +{ + shmobile_cpuidle_set_driver(&r8a7740_cpuidle_driver); +} +#else +void r8a7740_cpuidle_init(void) {} +#endif diff --git a/arch/arm/mach-shmobile/include/mach/r8a7740.h b/arch/arm/mach-shmobile/include/mach/r8a7740.h index 5cfe5d9..3b538e3 100644 --- a/arch/arm/mach-shmobile/include/mach/r8a7740.h +++ b/arch/arm/mach-shmobile/include/mach/r8a7740.h @@ -545,6 +545,7 @@ extern void r8a7740_pinmux_init(void); extern void r8a7740_pm_init(void); extern void r8a7740_resume(void); extern void r8a7740_shutdown(void); +extern void r8a7740_cpuidle_init(void); extern void r8a7740_enter_a3sm_common(int); #ifdef CONFIG_PM diff --git a/arch/arm/mach-shmobile/pm-r8a7740.c b/arch/arm/mach-shmobile/pm-r8a7740.c index fe3c867..2f196bd 100644 --- a/arch/arm/mach-shmobile/pm-r8a7740.c +++ b/arch/arm/mach-shmobile/pm-r8a7740.c @@ -237,4 +237,5 @@ static void r8a7740_suspend_init(void) {} void __init r8a7740_pm_init(void) { r8a7740_suspend_init(); + r8a7740_cpuidle_init(); }