diff mbox

[v3,1/4] ARM: OMAP4+: PM: Consolidate MPU subsystem PM code for re-use

Message ID 1365166743-5940-2-git-send-email-santosh.shilimkar@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Santosh Shilimkar April 5, 2013, 12:59 p.m. UTC 
OMAP5 and future OMAP based SOCs has backward compatible MPUSS
IP block with OMAP4. It's programming model is mostly similar.
Hence consolidate the OMAP MPUSS code so that it can be re-used
on OMAP5 and future SOCs.

No functional change.

Acked-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/mach-omap2/omap-mpuss-lowpower.c |   62 ++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 11 deletions(-)

Comments

Felipe Balbi April 5, 2013, 1:19 p.m. UTC | #1 
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 ?
Nishanth Menon April 5, 2013, 1:35 p.m. UTC | #2 
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
Santosh Shilimkar April 5, 2013, 1:37 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Santosh Shilimkar April 5, 2013, 1:39 p.m. UTC | #4 
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 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi April 5, 2013, 2:04 p.m. UTC | #5 
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
Nishanth Menon April 5, 2013, 2:18 p.m. UTC | #6 
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 ;)
Felipe Balbi April 5, 2013, 2:29 p.m. UTC | #7 
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 ;-)
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
index a27fe72..391bf2d 100644
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -71,10 +71,43 @@  struct omap4_cpu_pm_info {
 	void (*secondary_startup)(void);
 };
 
+/**
+ * struct cpu_pm_ops - CPU pm operations
+ * @finish_suspend:	CPU suspend finisher function pointer
+ * @resume:		CPU resume function pointer
+ * @scu_prepare:	CPU Snoop Control program function pointer
+ *
+ * Structure holds functions pointer for CPU low power operations like
+ * suspend, resume and scu programming.
+ */
+struct cpu_pm_ops {
+	int (*finish_suspend)(unsigned long cpu_state);
+	void (*resume)(void);
+	void (*scu_prepare)(unsigned int cpu_id, unsigned int cpu_state);
+};
+
 static DEFINE_PER_CPU(struct omap4_cpu_pm_info, omap4_pm_info);
 static struct powerdomain *mpuss_pd;
 static void __iomem *sar_base;
 
+static int default_finish_suspend(unsigned long cpu_state)
+{
+	omap_do_wfi();
+	return 0;
+}
+
+static void dummy_cpu_resume(void)
+{}
+
+static void dummy_scu_prepare(unsigned int cpu_id, unsigned int cpu_state)
+{}
+
+struct cpu_pm_ops omap_pm_ops = {
+	.finish_suspend		= default_finish_suspend,
+	.resume			= dummy_cpu_resume,
+	.scu_prepare		= dummy_scu_prepare,
+};
+
 /*
  * Program the wakeup routine address for the CPU0 and CPU1
  * used for OFF or DORMANT wakeup.
@@ -158,11 +191,12 @@  static void save_l2x0_context(void)
 {
 	u32 val;
 	void __iomem *l2x0_base = omap4_get_l2cache_base();
-
-	val = __raw_readl(l2x0_base + L2X0_AUX_CTRL);
-	__raw_writel(val, sar_base + L2X0_AUXCTRL_OFFSET);
-	val = __raw_readl(l2x0_base + L2X0_PREFETCH_CTRL);
-	__raw_writel(val, sar_base + L2X0_PREFETCH_CTRL_OFFSET);
+	if (l2x0_base) {
+		val = __raw_readl(l2x0_base + L2X0_AUX_CTRL);
+		__raw_writel(val, sar_base + L2X0_AUXCTRL_OFFSET);
+		val = __raw_readl(l2x0_base + L2X0_PREFETCH_CTRL);
+		__raw_writel(val, sar_base + L2X0_PREFETCH_CTRL_OFFSET);
+	}
 }
 #else
 static void save_l2x0_context(void)
@@ -225,17 +259,17 @@  int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
 
 	cpu_clear_prev_logic_pwrst(cpu);
 	pwrdm_set_next_pwrst(pm_info->pwrdm, power_state);
-	set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume));
-	scu_pwrst_prepare(cpu, power_state);
+	set_cpu_wakeup_addr(cpu, virt_to_phys(omap_pm_ops.resume));
+	omap_pm_ops.scu_prepare(cpu, power_state);
 	l2x0_pwrst_prepare(cpu, save_state);
 
 	/*
 	 * Call low level function  with targeted low power state.
 	 */
 	if (save_state)
-		cpu_suspend(save_state, omap4_finish_suspend);
+		cpu_suspend(save_state, omap_pm_ops.finish_suspend);
 	else
-		omap4_finish_suspend(save_state);
+		omap_pm_ops.finish_suspend(save_state);
 
 	/*
 	 * Restore the CPUx power state to ON otherwise CPUx
@@ -271,14 +305,14 @@  int __cpuinit omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state)
 	pwrdm_clear_all_prev_pwrst(pm_info->pwrdm);
 	pwrdm_set_next_pwrst(pm_info->pwrdm, power_state);
 	set_cpu_wakeup_addr(cpu, virt_to_phys(pm_info->secondary_startup));
-	scu_pwrst_prepare(cpu, power_state);
+	omap_pm_ops.scu_prepare(cpu, power_state);
 
 	/*
 	 * CPU never retuns back if targeted power state is OFF mode.
 	 * CPU ONLINE follows normal CPU ONLINE ptah via
 	 * omap_secondary_startup().
 	 */
-	omap4_finish_suspend(cpu_state);
+	omap_pm_ops.finish_suspend(cpu_state);
 
 	pwrdm_set_next_pwrst(pm_info->pwrdm, PWRDM_POWER_ON);
 	return 0;
@@ -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;
+	}
+
 	return 0;
 }