| Message ID | 1365166743-5940-1-git-send-email-santosh.shilimkar@ti.com |
|---|---|
| State | New |
| Headers | show |
On 16:19-20130405, Felipe Balbi wrote: > On Fri, Apr 05, 2013 at 06:29:00PM +0530, Santosh Shilimkar wrote: > > OMAP5 and future OMAP based SOCs has backward compatible MPUSS > > IP block with OMAP4. It's programming model is mostly similar. > > s/It's/Its/ > s/mostly // > > (similar already expands to 'almost the same' :-) > > > @@ -355,6 +389,12 @@ int __init omap4_mpuss_init(void) > > > > save_l2x0_context(); > > > > + if (cpu_is_omap44xx()) { > > + omap_pm_ops.finish_suspend = omap4_finish_suspend; > > + omap_pm_ops.resume = omap4_cpu_resume; > > + omap_pm_ops.scu_prepare = scu_pwrst_prepare; > > + } > > why don't you just rename omap4_* into omap_* and add cpu-based checks > there in order to handle differences between omap4 and omap5? > > If implementation will be almost the same for both, you might be able to > save on some more duplication, no ? Jeez NO! finish_suspend is assembly, further, it is the hottest path in cpuidle framework - for every WFI we invoke it. we definitely dont want to add more overhead beyond what is necessary. > > -- > balbi
On Friday 05 April 2013 06:51 PM, Felipe Balbi wrote: > On Fri, Apr 05, 2013 at 06:29:01PM +0530, Santosh Shilimkar wrote: >> OMAP5 has backward compatible PRCM block and it's programming >> model is mostly similar to OMAP4. Same is going to be maintained >> for future OMAP4 based SOCs. Hence consolidate the OMAP4 power >> management code so that it can be re-used on OMAP5 and later devices. >> >> While at it, update the kernel-doc for omap4_pm_init(). >> >> Acked-by: Nishanth Menon <nm@ti.com> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> --- >> arch/arm/mach-omap2/Makefile | 9 +-- >> arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} | 58 ++++++++++++++++---- >> .../mach-omap2/{sleep44xx.S => sleep_omap4plus.S} | 0 >> 3 files changed, 53 insertions(+), 14 deletions(-) >> rename arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} (83%) >> rename arch/arm/mach-omap2/{sleep44xx.S => sleep_omap4plus.S} (100%) >> >> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile >> index b068b7f..3e59895 100644 >> --- a/arch/arm/mach-omap2/Makefile >> +++ b/arch/arm/mach-omap2/Makefile >> @@ -35,14 +35,14 @@ obj-$(CONFIG_SOC_HAS_OMAP2_SDRC) += sdrc.o >> obj-$(CONFIG_SMP) += omap-smp.o omap-headsmp.o >> obj-$(CONFIG_HOTPLUG_CPU) += omap-hotplug.o >> omap-4-5-common = omap4-common.o omap-wakeupgen.o \ >> - sleep44xx.o >> + sleep_omap4plus.o >> obj-$(CONFIG_ARCH_OMAP4) += $(omap-4-5-common) >> obj-$(CONFIG_SOC_OMAP5) += $(omap-4-5-common) >> >> plus_sec := $(call as-instr,.arch_extension sec,+sec) >> AFLAGS_omap-headsmp.o :=-Wa,-march=armv7-a$(plus_sec) >> AFLAGS_omap-smc.o :=-Wa,-march=armv7-a$(plus_sec) >> -AFLAGS_sleep44xx.o :=-Wa,-march=armv7-a$(plus_sec) >> +AFLAGS_sleep_omap4plus.o :=-Wa,-march=armv7-a$(plus_sec) >> >> # Functions loaded to SRAM >> obj-$(CONFIG_SOC_OMAP2420) += sram242x.o >> @@ -80,11 +80,12 @@ endif >> obj-$(CONFIG_OMAP_PM_NOOP) += omap-pm-noop.o >> >> ifeq ($(CONFIG_PM),y) >> +omap4plus-common-pm = omap-mpuss-lowpower.o pm_omap4plus.o >> obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o >> obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o >> obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o >> -obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o omap-mpuss-lowpower.o >> -obj-$(CONFIG_SOC_OMAP5) += omap-mpuss-lowpower.o >> +obj-$(CONFIG_ARCH_OMAP4) += $(omap4plus-common-pm) >> +obj-$(CONFIG_SOC_OMAP5) += $(omap4plus-common-pm) >> obj-$(CONFIG_PM_DEBUG) += pm-debug.o >> >> obj-$(CONFIG_POWER_AVS_OMAP) += sr_device.o >> diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm_omap4plus.c >> similarity index 83% >> rename from arch/arm/mach-omap2/pm44xx.c >> rename to arch/arm/mach-omap2/pm_omap4plus.c >> index 5ba6d88..228deca 100644 >> --- a/arch/arm/mach-omap2/pm44xx.c >> +++ b/arch/arm/mach-omap2/pm_omap4plus.c >> @@ -1,7 +1,7 @@ >> /* >> - * OMAP4 Power Management Routines >> + * OMAP4+ Power Management Routines >> * >> - * Copyright (C) 2010-2011 Texas Instruments, Inc. >> + * Copyright (C) 2010-2013 Texas Instruments, Inc. >> * Rajendra Nayak <rnayak@ti.com> >> * Santosh Shilimkar <santosh.shilimkar@ti.com> >> * >> @@ -135,16 +135,16 @@ static void omap_default_idle(void) >> } >> >> /** >> - * omap4_pm_init - Init routine for OMAP4 PM >> + * omap4_init_static_deps - Add OMAP4 static dependencies >> * >> - * Initializes all powerdomain and clockdomain target states >> - * and all PRCM settings. >> + * Add needed static clockdomain dependencies on OMAP4 devices. >> + * Return: 0 on success or 'err' on failures >> */ >> -int __init omap4_pm_init(void) >> +static inline int omap4_init_static_deps(void) > > you can let compiler inline this if it chooses to, right ?? Otherwise, > if you really want to make sure this will be inlined, you could(should?) > be using __always_inline. > Compilers are smart now so that fine.
On Friday 05 April 2013 06:49 PM, Felipe Balbi wrote: > On Fri, Apr 05, 2013 at 06:29:00PM +0530, Santosh Shilimkar wrote: >> OMAP5 and future OMAP based SOCs has backward compatible MPUSS >> IP block with OMAP4. It's programming model is mostly similar. > > s/It's/Its/ > s/mostly // > > (similar already expands to 'almost the same' :-) > >> @@ -355,6 +389,12 @@ int __init omap4_mpuss_init(void) >> >> save_l2x0_context(); >> >> + if (cpu_is_omap44xx()) { >> + omap_pm_ops.finish_suspend = omap4_finish_suspend; >> + omap_pm_ops.resume = omap4_cpu_resume; >> + omap_pm_ops.scu_prepare = scu_pwrst_prepare; >> + } > > why don't you just rename omap4_* into omap_* and add cpu-based checks > there in order to handle differences between omap4 and omap5? > The whole idea is to handle all these SOC specific stuff in init and not sprinkle the checks in runtime code. > If implementation will be almost the same for both, you might be able to > save on some more duplication, no ? > The implementation is not same and hence. If it was same, I wouldn't have introduced function pointers :) Regards, Santosh
On Friday 05 April 2013 07:05 PM, Nishanth Menon wrote: > On 16:19-20130405, Felipe Balbi wrote: >> On Fri, Apr 05, 2013 at 06:29:00PM +0530, Santosh Shilimkar wrote: >>> OMAP5 and future OMAP based SOCs has backward compatible MPUSS >>> IP block with OMAP4. It's programming model is mostly similar. >> >> s/It's/Its/ >> s/mostly // >> >> (similar already expands to 'almost the same' :-) >> >>> @@ -355,6 +389,12 @@ int __init omap4_mpuss_init(void) >>> >>> save_l2x0_context(); >>> >>> + if (cpu_is_omap44xx()) { >>> + omap_pm_ops.finish_suspend = omap4_finish_suspend; >>> + omap_pm_ops.resume = omap4_cpu_resume; >>> + omap_pm_ops.scu_prepare = scu_pwrst_prepare; >>> + } >> >> why don't you just rename omap4_* into omap_* and add cpu-based checks >> there in order to handle differences between omap4 and omap5? >> >> If implementation will be almost the same for both, you might be able to >> save on some more duplication, no ? > Jeez NO! finish_suspend is assembly, further, it is the hottest path in > cpuidle framework - for every WFI we invoke it. we definitely dont want > to add more overhead beyond what is necessary. :-) Our emails crossed. I just said the same thing in other words. Regards, Santosh
Hi, On Fri, Apr 05, 2013 at 08:35:11AM -0500, Nishanth Menon wrote: > On 16:19-20130405, Felipe Balbi wrote: > > On Fri, Apr 05, 2013 at 06:29:00PM +0530, Santosh Shilimkar wrote: > > > OMAP5 and future OMAP based SOCs has backward compatible MPUSS > > > IP block with OMAP4. It's programming model is mostly similar. > > > > s/It's/Its/ > > s/mostly // > > > > (similar already expands to 'almost the same' :-) > > > > > @@ -355,6 +389,12 @@ int __init omap4_mpuss_init(void) > > > > > > save_l2x0_context(); > > > > > > + if (cpu_is_omap44xx()) { > > > + omap_pm_ops.finish_suspend = omap4_finish_suspend; > > > + omap_pm_ops.resume = omap4_cpu_resume; > > > + omap_pm_ops.scu_prepare = scu_pwrst_prepare; > > > + } > > > > why don't you just rename omap4_* into omap_* and add cpu-based checks > > there in order to handle differences between omap4 and omap5? > > > > If implementation will be almost the same for both, you might be able to > > save on some more duplication, no ? > Jeez NO! finish_suspend is assembly, further, it is the hottest path in > cpuidle framework - for every WFI we invoke it. we definitely dont want > to add more overhead beyond what is necessary. alright, settle down ;-) whoever suggested that isn't here anymore
On 17:04-20130405, Felipe Balbi wrote: > Hi, > > On Fri, Apr 05, 2013 at 08:35:11AM -0500, Nishanth Menon wrote: > > On 16:19-20130405, Felipe Balbi wrote: > > > On Fri, Apr 05, 2013 at 06:29:00PM +0530, Santosh Shilimkar wrote: > > > > OMAP5 and future OMAP based SOCs has backward compatible MPUSS > > > > IP block with OMAP4. It's programming model is mostly similar. > > > > > > s/It's/Its/ > > > s/mostly // > > > > > > (similar already expands to 'almost the same' :-) > > > > > > > @@ -355,6 +389,12 @@ int __init omap4_mpuss_init(void) > > > > > > > > save_l2x0_context(); > > > > > > > > + if (cpu_is_omap44xx()) { > > > > + omap_pm_ops.finish_suspend = omap4_finish_suspend; > > > > + omap_pm_ops.resume = omap4_cpu_resume; > > > > + omap_pm_ops.scu_prepare = scu_pwrst_prepare; > > > > + } > > > > > > why don't you just rename omap4_* into omap_* and add cpu-based checks > > > there in order to handle differences between omap4 and omap5? > > > > > > If implementation will be almost the same for both, you might be able to > > > save on some more duplication, no ? > > Jeez NO! finish_suspend is assembly, further, it is the hottest path in > > cpuidle framework - for every WFI we invoke it. we definitely dont want > > to add more overhead beyond what is necessary. > > alright, settle down ;-) whoever suggested that isn't here anymore hehe, Apologies, I was'nt that stressed as the wording might have indicated.. We spend tons of time evaluating with Lauterbach tracing to weed out hot paths - folks who have been bitten by these tend to feel a little defensive I guess and to have surprise regressions are painful to find and fix - esp when around not-so-obvious paths ;)
On Fri, Apr 05, 2013 at 09:18:02AM -0500, Nishanth Menon wrote: > On 17:04-20130405, Felipe Balbi wrote: > > Hi, > > > > On Fri, Apr 05, 2013 at 08:35:11AM -0500, Nishanth Menon wrote: > > > On 16:19-20130405, Felipe Balbi wrote: > > > > On Fri, Apr 05, 2013 at 06:29:00PM +0530, Santosh Shilimkar wrote: > > > > > OMAP5 and future OMAP based SOCs has backward compatible MPUSS > > > > > IP block with OMAP4. It's programming model is mostly similar. > > > > > > > > s/It's/Its/ > > > > s/mostly // > > > > > > > > (similar already expands to 'almost the same' :-) > > > > > > > > > @@ -355,6 +389,12 @@ int __init omap4_mpuss_init(void) > > > > > > > > > > save_l2x0_context(); > > > > > > > > > > + if (cpu_is_omap44xx()) { > > > > > + omap_pm_ops.finish_suspend = omap4_finish_suspend; > > > > > + omap_pm_ops.resume = omap4_cpu_resume; > > > > > + omap_pm_ops.scu_prepare = scu_pwrst_prepare; > > > > > + } > > > > > > > > why don't you just rename omap4_* into omap_* and add cpu-based checks > > > > there in order to handle differences between omap4 and omap5? > > > > > > > > If implementation will be almost the same for both, you might be able to > > > > save on some more duplication, no ? > > > Jeez NO! finish_suspend is assembly, further, it is the hottest path in > > > cpuidle framework - for every WFI we invoke it. we definitely dont want > > > to add more overhead beyond what is necessary. > > > > alright, settle down ;-) whoever suggested that isn't here anymore > hehe, Apologies, I was'nt that stressed as the wording might have > indicated.. We spend tons of time evaluating with Lauterbach tracing to > weed out hot paths - folks who have been bitten by these tend to feel a > little defensive I guess and to have surprise regressions are painful to > find and fix - esp when around not-so-obvious paths ;) understood ;-)
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > The OMAP5 idle driver can re-use most of OMAP4 CPUidle driver > implementation. Also the next derivative SOCs are going to re-use > the MPUSS so, same driver with minor updates can be re-used. > > Prepare the code so that its easier to add CPUidle support for > OMAP5 devices. > > Acked-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> Due to dependencies, I'll add this one to my for_3.10/cleanup/cpuidle branch. Kevin > --- > arch/arm/mach-omap2/cpuidle44xx.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c > index 767f657..ca1551a 100644 > --- a/arch/arm/mach-omap2/cpuidle44xx.c > +++ b/arch/arm/mach-omap2/cpuidle44xx.c > @@ -1,7 +1,7 @@ > /* > - * OMAP4 CPU idle Routines > + * OMAP4+ CPU idle Routines > * > - * Copyright (C) 2011 Texas Instruments, Inc. > + * Copyright (C) 2011-2013 Texas Instruments, Inc. > * Santosh Shilimkar <santosh.shilimkar@ti.com> > * Rajendra Nayak <rnayak@ti.com> > * > @@ -24,13 +24,13 @@ > #include "clockdomain.h" > > /* Machine specific information */ > -struct omap4_idle_statedata { > +struct idle_statedata { > u32 cpu_state; > u32 mpu_logic_state; > u32 mpu_state; > }; > > -static struct omap4_idle_statedata omap4_idle_data[] = { > +static struct idle_statedata omap4_idle_data[] = { > { > .cpu_state = PWRDM_POWER_ON, > .mpu_state = PWRDM_POWER_ON, > @@ -53,11 +53,12 @@ static struct clockdomain *cpu_clkdm[NR_CPUS]; > > static atomic_t abort_barrier; > static bool cpu_done[NR_CPUS]; > +static struct idle_statedata *state_ptr = &omap4_idle_data[0]; > > /* Private functions */ > > /** > - * omap4_enter_idle_coupled_[simple/coupled] - OMAP4 cpuidle entry functions > + * omap_enter_idle_[simple/coupled] - OMAP4PLUS cpuidle entry functions > * @dev: cpuidle device > * @drv: cpuidle driver > * @index: the index of state to be entered > @@ -66,7 +67,7 @@ static bool cpu_done[NR_CPUS]; > * specified low power state selected by the governor. > * Returns the amount of time spent in the low power state. > */ > -static int omap4_enter_idle_simple(struct cpuidle_device *dev, > +static int omap_enter_idle_simple(struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index) > { > @@ -74,11 +75,11 @@ static int omap4_enter_idle_simple(struct cpuidle_device *dev, > return index; > } > > -static int omap4_enter_idle_coupled(struct cpuidle_device *dev, > +static int omap_enter_idle_coupled(struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index) > { > - struct omap4_idle_statedata *cx = &omap4_idle_data[index]; > + struct idle_statedata *cx = state_ptr + index; > int cpu_id = smp_processor_id(); > > /* > @@ -168,7 +169,7 @@ static void omap_setup_broadcast_timer(void *arg) > clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ON, &cpu); > } > > -static DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); > +static DEFINE_PER_CPU(struct cpuidle_device, omap_idle_dev); > > static struct cpuidle_driver omap4_idle_driver = { > .name = "omap4_idle", > @@ -180,7 +181,7 @@ static struct cpuidle_driver omap4_idle_driver = { > .exit_latency = 2 + 2, > .target_residency = 5, > .flags = CPUIDLE_FLAG_TIME_VALID, > - .enter = omap4_enter_idle_simple, > + .enter = omap_enter_idle_simple, > .name = "C1", > .desc = "CPUx ON, MPUSS ON" > }, > @@ -189,7 +190,7 @@ static struct cpuidle_driver omap4_idle_driver = { > .exit_latency = 328 + 440, > .target_residency = 960, > .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_COUPLED, > - .enter = omap4_enter_idle_coupled, > + .enter = omap_enter_idle_coupled, > .name = "C2", > .desc = "CPUx OFF, MPUSS CSWR", > }, > @@ -198,7 +199,7 @@ static struct cpuidle_driver omap4_idle_driver = { > .exit_latency = 460 + 518, > .target_residency = 1100, > .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_COUPLED, > - .enter = omap4_enter_idle_coupled, > + .enter = omap_enter_idle_coupled, > .name = "C3", > .desc = "CPUx OFF, MPUSS OSWR", > }, > @@ -210,9 +211,9 @@ static struct cpuidle_driver omap4_idle_driver = { > /* Public functions */ > > /** > - * omap4_idle_init - Init routine for OMAP4 idle > + * omap4_idle_init - Init routine for OMAP4+ idle > * > - * Registers the OMAP4 specific cpuidle driver to the cpuidle > + * Registers the OMAP4+ specific cpuidle driver to the cpuidle > * framework with the valid set of states. > */ > int __init omap4_idle_init(void) > @@ -240,7 +241,7 @@ int __init omap4_idle_init(void) > } > > for_each_cpu(cpu_id, cpu_online_mask) { > - dev = &per_cpu(omap4_idle_dev, cpu_id); > + dev = &per_cpu(omap_idle_dev, cpu_id); > dev->cpu = cpu_id; > #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED > dev->coupled_cpus = *cpu_online_mask;
* Santosh Shilimkar <santosh.shilimkar@ti.com> [130405 06:01]: > OMAP5 has backward compatible PRCM block and it's programming > model is mostly similar to OMAP4. Same is going to be maintained > for future OMAP4 based SOCs. Hence consolidate the OMAP4 power > management code so that it can be re-used on OMAP5 and later devices. > > While at it, update the kernel-doc for omap4_pm_init(). > > Acked-by: Nishanth Menon <nm@ti.com> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > arch/arm/mach-omap2/Makefile | 9 +-- > arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} | 58 ++++++++++++++++---- > .../mach-omap2/{sleep44xx.S => sleep_omap4plus.S} | 0 > 3 files changed, 53 insertions(+), 14 deletions(-) > rename arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} (83%) > rename arch/arm/mach-omap2/{sleep44xx.S => sleep_omap4plus.S} (100%) I suggest you leave out the rename. That just adds churn and has flame potential. Regards, Tony
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > As discussed on list, I split the v2 [1] series into cleanup and OMAP5 support. Thanks. > This series contains the clean-up patches. I have rebased on top of Tony's > pull request and your 3.10/* branches. Series is tested on OMAP4430 SDP > with CPUIDLE and suspend. OMAP5 PM support I will post later since it has > a dependency with OMAP5 data files which are not pulled in yet. I've added 4/4 to my cpuidle branch and added 1,3/4 to my for_3.10/cleanup/pm branch (now based on Tony's cleanup-v2 branch for dependencies.) Patch 2/4 needs a respin without the rename as per Tony's comments. Thanks, Kevin
On Saturday 06 April 2013 03:04 AM, Tony Lindgren wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [130405 06:01]: >> OMAP5 has backward compatible PRCM block and it's programming >> model is mostly similar to OMAP4. Same is going to be maintained >> for future OMAP4 based SOCs. Hence consolidate the OMAP4 power >> management code so that it can be re-used on OMAP5 and later devices. >> >> While at it, update the kernel-doc for omap4_pm_init(). >> >> Acked-by: Nishanth Menon <nm@ti.com> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> --- >> arch/arm/mach-omap2/Makefile | 9 +-- >> arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} | 58 ++++++++++++++++---- >> .../mach-omap2/{sleep44xx.S => sleep_omap4plus.S} | 0 >> 3 files changed, 53 insertions(+), 14 deletions(-) >> rename arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} (83%) >> rename arch/arm/mach-omap2/{sleep44xx.S => sleep_omap4plus.S} (100%) > > I suggest you leave out the rename. That just adds churn and has > flame potential. > OK. I can leave that mow but have to do renames anyways when I add OMAP5 support. OMAP54XX support inside pm44xx.c and sleep44x.S will be really odd. Let me know if you have concern on renaming it while OMAP5 support gets added ? Regards, Santosh
* Santosh Shilimkar <santosh.shilimkar@ti.com> [130408 03:51]: > On Saturday 06 April 2013 03:04 AM, Tony Lindgren wrote: > > * Santosh Shilimkar <santosh.shilimkar@ti.com> [130405 06:01]: > >> OMAP5 has backward compatible PRCM block and it's programming > >> model is mostly similar to OMAP4. Same is going to be maintained > >> for future OMAP4 based SOCs. Hence consolidate the OMAP4 power > >> management code so that it can be re-used on OMAP5 and later devices. > >> > >> While at it, update the kernel-doc for omap4_pm_init(). > >> > >> Acked-by: Nishanth Menon <nm@ti.com> > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >> --- > >> arch/arm/mach-omap2/Makefile | 9 +-- > >> arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} | 58 ++++++++++++++++---- > >> .../mach-omap2/{sleep44xx.S => sleep_omap4plus.S} | 0 > >> 3 files changed, 53 insertions(+), 14 deletions(-) > >> rename arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} (83%) > >> rename arch/arm/mach-omap2/{sleep44xx.S => sleep_omap4plus.S} (100%) > > > > I suggest you leave out the rename. That just adds churn and has > > flame potential. > > > OK. I can leave that mow but have to do renames anyways when > I add OMAP5 support. OMAP54XX support inside pm44xx.c and sleep44x.S > will be really odd. Well that should be just fine if the hardware is the same. > Let me know if you have concern on renaming it while OMAP5 support > gets added ? If the rename is done, it should have a clear reason to do it in a separate patch. IMHO it's just fine to have omap4 in some name and then assume that at least some follow up SoCs also use that code. In the worst case we may end up renaming things many times: omap-xyz.c -> omap2-xyz.c -> omap2plus-xyz.c -> omap2-to-4-xyz.c -> omap2-to-4-and-am35xx-xyz.c etc :) If we rename something, the description should have a clear reason for doing it like "To avoid confusing between PM code that does not have support for bootrom assisted off-idle on SMP omaps with PM code that's not bootrom assisted, let's rename foo to.." For the naming, the only safe naming convention is to use something actually describing the piece of hardware. Maybe some combination of bootrom-assisted-off-idle + SMP in this case if there's no actual name for this hwblock? Regards, Tony
On Monday 08 April 2013 10:12 PM, Tony Lindgren wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [130408 03:51]: >> On Saturday 06 April 2013 03:04 AM, Tony Lindgren wrote: >>> * Santosh Shilimkar <santosh.shilimkar@ti.com> [130405 06:01]: >>>> OMAP5 has backward compatible PRCM block and it's programming >>>> model is mostly similar to OMAP4. Same is going to be maintained >>>> for future OMAP4 based SOCs. Hence consolidate the OMAP4 power >>>> management code so that it can be re-used on OMAP5 and later devices. >>>> >>>> While at it, update the kernel-doc for omap4_pm_init(). >>>> >>>> Acked-by: Nishanth Menon <nm@ti.com> >>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>> --- >>>> arch/arm/mach-omap2/Makefile | 9 +-- >>>> arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} | 58 ++++++++++++++++---- >>>> .../mach-omap2/{sleep44xx.S => sleep_omap4plus.S} | 0 >>>> 3 files changed, 53 insertions(+), 14 deletions(-) >>>> rename arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} (83%) >>>> rename arch/arm/mach-omap2/{sleep44xx.S => sleep_omap4plus.S} (100%) >>> >>> I suggest you leave out the rename. That just adds churn and has >>> flame potential. >>> >> OK. I can leave that mow but have to do renames anyways when >> I add OMAP5 support. OMAP54XX support inside pm44xx.c and sleep44x.S >> will be really odd. > > Well that should be just fine if the hardware is the same. > >> Let me know if you have concern on renaming it while OMAP5 support >> gets added ? > > If the rename is done, it should have a clear reason to do it in > a separate patch. IMHO it's just fine to have omap4 in some name and > then assume that at least some follow up SoCs also use that code. > In the worst case we may end up renaming things many times: > Agree. omap4_* is just fine and thats why many omap4_* are not renamed. Here the files were calling specific family of OMAP4 i.e OMAP44XX and hence the rename was appropriate. > omap-xyz.c -> omap2-xyz.c -> omap2plus-xyz.c -> omap2-to-4-xyz.c -> > omap2-to-4-and-am35xx-xyz.c etc :) > pm_omap4plus.c and sleep_omap4plus.S was chosen specifically considering the OMAP4+ devices share the PRCM IP. But PRCM_IP itself isn't new so calling by IP wasn't an option. > If we rename something, the description should have a clear reason > for doing it like "To avoid confusing between PM code that does not > have support for bootrom assisted off-idle on SMP omaps with PM code > that's not bootrom assisted, let's rename foo to.." > > For the naming, the only safe naming convention is to use something > actually describing the piece of hardware. Maybe some combination > of bootrom-assisted-off-idle + SMP in this case if there's no actual > name for this hwblock? > As I said, the IP has been there from OMAP2XX days. Here the case that IP version is very similar between OMAP4, OMAP5. DRA(next SOC) and its derivatives. Hence can share most of the code. I thought this was good enough reason considering at least 4 family of SOC's can make use of the code. It has nothing to do with SMP etc specifically and rather the similarity between the PM infrastructure on the mentioned SOCs. Let me know if you can suggested better name than what I chose ? Regards, Santosh
* Santosh Shilimkar <santosh.shilimkar@ti.com> [130408 23:16]: > > > As I said, the IP has been there from OMAP2XX days. Here the case that > IP version is very similar between OMAP4, OMAP5. DRA(next SOC) and its > derivatives. Hence can share most of the code. I thought this was good > enough reason considering at least 4 family of SOC's can make use > of the code. Well at least that might be enough of a reasoning to rename it as it is somewhat futureproof. > It has nothing to do with SMP etc specifically and rather the similarity > between the PM infrastructure on the mentioned SOCs. > > Let me know if you can suggested better name than what I chose ? Not really except something like pm-iprevXXX.[chS] where the rev is the first revision that it works with? But then again if it's touching registers all over the place directly, that naming does not make much sense either :) Regards, Tony
On Tuesday 09 April 2013 10:25 PM, Tony Lindgren wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [130408 23:16]: >>> >> As I said, the IP has been there from OMAP2XX days. Here the case that >> IP version is very similar between OMAP4, OMAP5. DRA(next SOC) and its >> derivatives. Hence can share most of the code. I thought this was good >> enough reason considering at least 4 family of SOC's can make use >> of the code. > > Well at least that might be enough of a reasoning to rename it as > it is somewhat futureproof. > Yep. I can't see to much further in future but there is a *strong* mandate to not break compatibility so hopefully we won't see too much churn for similar IP blocks in future. >> It has nothing to do with SMP etc specifically and rather the similarity >> between the PM infrastructure on the mentioned SOCs. >> >> Let me know if you can suggested better name than what I chose ? > > Not really except something like pm-iprevXXX.[chS] where the rev is > the first revision that it works with? > > But then again if it's touching registers all over the place directly, > that naming does not make much sense either :) > Exactly. Thanks for the discussion. I will go ahead with rename when we add OMAP5 stuff like I did in earlier patch. i.e pm44xx.c --> pm_omap4plus.c sleep44xx.S --> sleep_omap4plus.S Regrads, Santosh
Kevin, As discussed on list, I split the v2 [1] series into cleanup and OMAP5 support. This series contains the clean-up patches. I have rebased on top of Tony's pull request and your 3.10/* branches. Series is tested on OMAP4430 SDP with CPUIDLE and suspend. OMAP5 PM support I will post later since it has a dependency with OMAP5 data files which are not pulled in yet. Regards, Santosh [1] http://www.spinics.net/lists/linux-omap/msg88901.html The following changes since commit 4b9ce8484d8122f2088cedf60265ae86f7822350: Merge branch 'for_3.10/fixes/pm' of git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm into for_3.10/omap_pm-cleanup (2013-04-05 18:10:34 +0530) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/ssantosh/linux.git for you to fetch changes up to 884ef029cb434aad124bf8413211a698597f8f7a: ARM: OMAP4+: CPUidle: Consolidate idle driver for OMAP5 support (2013-04-05 18:11:06 +0530) ---------------------------------------------------------------- Santosh Shilimkar (4): ARM: OMAP4+: PM: Consolidate MPU subsystem PM code for re-use ARM: OMAP4+: PM: Consolidate OMAP4 PM code to re-use it for OMAP5 ARM: OMAP4+: Make secondary_startup function name more consistent ARM: OMAP4+: CPUidle: Consolidate idle driver for OMAP5 support arch/arm/mach-omap2/Makefile | 9 +-- arch/arm/mach-omap2/common.h | 4 +- arch/arm/mach-omap2/cpuidle44xx.c | 31 ++++----- arch/arm/mach-omap2/omap-headsmp.S | 8 +-- arch/arm/mach-omap2/omap-mpuss-lowpower.c | 68 ++++++++++++++++---- arch/arm/mach-omap2/omap-smp.c | 6 +- arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} | 58 ++++++++++++++--- .../mach-omap2/{sleep44xx.S => sleep_omap4plus.S} | 0 8 files changed, 132 insertions(+), 52 deletions(-) rename arch/arm/mach-omap2/{pm44xx.c => pm_omap4plus.c} (83%) rename arch/arm/mach-omap2/{sleep44xx.S => sleep_omap4plus.S} (100%)