diff mbox

[2/8] ARM: OMAP2+: hwmod: Cleanup sidle/mstandby programming

Message ID 1361354272-18089-3-git-send-email-santosh.shilimkar@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Santosh Shilimkar Feb. 20, 2013, 9:57 a.m. UTC 
From: Rajendra Nayak <rnayak@ti.com>

_enable_wakeup() and _disable_wakeup() are expected to program the
OCP_SYSCONFIG.ENAWAKEUP bit. Get rid of the additional sidle/mstandby
programming in them, as its confusing (this is expected to be handled
elsewhere as part of _enable_sysc()/__idle_sysc()) and unnecessary.

Well, the _enable_sysc()/_idle_sysc() handled only the mstandby modes
correctly. So fix them so they also handle the midle modes correctly
and program HWMOD_IDLEMODE_SMART_WKUP and HWMOD_IDLEMODE_SMART
respectively.

Tested-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
Tested-by: Sourav Poddar <sourav.poddar@ti.com>
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   48 ++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

Comments

Paul Walmsley March 31, 2013, 1:30 a.m. UTC | #1 
Hi

On Wed, 20 Feb 2013, Santosh Shilimkar wrote:

> From: Rajendra Nayak <rnayak@ti.com>
> 
> _enable_wakeup() and _disable_wakeup() are expected to program the
> OCP_SYSCONFIG.ENAWAKEUP bit.

These functions were originally intended to take care of everything needed 
for the IP block to wake up the chip, including the PRCM WKEN programming.  
ENAWAKEUP is simply one part of that.

> Get rid of the additional sidle/mstandby programming in them, as its 
> confusing (this is expected to be handled elsewhere as part of 
> _enable_sysc()/__idle_sysc())

Sorry, why does the expectation exist for the code to enable and disable 
device wakeup to be part of _enable_sysc()/_idle_sysc(), rather than in 
functions called by _enable_sysc()/_idle_sysc()?

> and unnecessary.

So here's part of the reason why the module wakeup control functions 
should remain separate.  When the kernel boots, all the PM features should 
be disabled.  Then mach-omap2/pm*.c should enables PM features where 
they're needed.

Right now, mach-omap2/pm34xx.c sets module WKEN bits.  (These direct 
register accesses need to be moved as part of the cleanup work, of moving 
the PM/PRM/CM code into drivers.)  But the list of IP blocks that 
should be allowed to wake the system is board-dependent.

So really, what mach-omap2/pm34xx.c should do is to call into the hwmod 
enable-wakeups code to enable MPU wakeups for all the IP blocks that have 
some DT property set, something like 'enable-wakeups'.  Then the hwmod 
code should ensure that the PRM wakeup-enable and GRPSEL bits are 
programmed (by calling into the PRM driver code) and should then either 
set the ENAWAKEUP bits or put the module into smart_wkup MSTANDBY/MIDLE.

Similarly, when the PM driver is unloaded, it should set no-idle on all 
the IP blocks that were marked as wakeup-capable and disable the PRCM 
wakeup control bits, by calling some hwmod disable-wakeups code.

> Well, the _enable_sysc()/_idle_sysc() handled only the mstandby modes
> correctly. So fix them so they also handle the midle modes correctly

If there's a fix here, please split that out into a separate patch.


- Paul
--
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
Rajendra Nayak April 1, 2013, 8:39 a.m. UTC | #2 
Paul,

>> _enable_wakeup() and _disable_wakeup() are expected to program the
>> OCP_SYSCONFIG.ENAWAKEUP bit.
> 
> These functions were originally intended to take care of everything needed 
> for the IP block to wake up the chip, including the PRCM WKEN programming.  
> ENAWAKEUP is simply one part of that.
> 
>> Get rid of the additional sidle/mstandby programming in them, as its 
>> confusing (this is expected to be handled elsewhere as part of 
>> _enable_sysc()/__idle_sysc())
> 
> Sorry, why does the expectation exist for the code to enable and disable 
> device wakeup to be part of _enable_sysc()/_idle_sysc(), rather than in 
> functions called by _enable_sysc()/_idle_sysc()?

It all comes down to if SIDLE_SMART_WKUP/MSTANDBY_SMART_WKUP programming
be considered as 'idlemode' programming or 'enwakeup' programming.
If you consider these are being part of 'enwakeup' progrmming, these should
certainly be handled as part of _enable_wakeup() and _disable_wakeup().

Today, in some cases, these are *also* handled as part of _enable_sysc()
and _idle_sysc(). The way _enable_wakeup() is invoked from _enable_sysc()
is also very inconsistent. For instance, for any IP which supports
SYSC_HAS_MIDLEMODE and SYSC_HAS_ENAWAKEUP, we invoke _enable_wakeup()
regardless of the MIDLEMODE programmmed.
While in case of the IP supporting SYSC_HAS_SIDLEMODE, _enable_wakeup() is
invoked only when the SIDLEMODE programmed is SMART or SMART_WKUP.

I understand the cleanups you are suggesting below as part of the movement
of some of these things outside of mach-omap2.
I was more looking at fixing the existing piece so its readable and does
things more consistently.

regards,
Rajendra

> 
>> and unnecessary.
> 
> So here's part of the reason why the module wakeup control functions 
> should remain separate.  When the kernel boots, all the PM features should 
> be disabled.  Then mach-omap2/pm*.c should enables PM features where 
> they're needed.
> 
> Right now, mach-omap2/pm34xx.c sets module WKEN bits.  (These direct 
> register accesses need to be moved as part of the cleanup work, of moving 
> the PM/PRM/CM code into drivers.)  But the list of IP blocks that 
> should be allowed to wake the system is board-dependent.
> 
> So really, what mach-omap2/pm34xx.c should do is to call into the hwmod 
> enable-wakeups code to enable MPU wakeups for all the IP blocks that have 
> some DT property set, something like 'enable-wakeups'.  Then the hwmod 
> code should ensure that the PRM wakeup-enable and GRPSEL bits are 
> programmed (by calling into the PRM driver code) and should then either 
> set the ENAWAKEUP bits or put the module into smart_wkup MSTANDBY/MIDLE.
> 
> Similarly, when the PM driver is unloaded, it should set no-idle on all 
> the IP blocks that were marked as wakeup-capable and disable the PRCM 
> wakeup control bits, by calling some hwmod disable-wakeups code.
> 
>> Well, the _enable_sysc()/_idle_sysc() handled only the mstandby modes
>> correctly. So fix them so they also handle the midle modes correctly
> 
> If there's a fix here, please split that out into a separate patch.
> 
> 
> - Paul
> 

--
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
Rajendra Nayak April 18, 2013, 10:53 a.m. UTC | #3 
Hi Paul,

>>> _enable_wakeup() and _disable_wakeup() are expected to program the
>>> OCP_SYSCONFIG.ENAWAKEUP bit.
>>
>> These functions were originally intended to take care of everything needed 
>> for the IP block to wake up the chip, including the PRCM WKEN programming.  
>> ENAWAKEUP is simply one part of that.
>>
>>> Get rid of the additional sidle/mstandby programming in them, as its 
>>> confusing (this is expected to be handled elsewhere as part of 
>>> _enable_sysc()/__idle_sysc())
>>
>> Sorry, why does the expectation exist for the code to enable and disable 
>> device wakeup to be part of _enable_sysc()/_idle_sysc(), rather than in 
>> functions called by _enable_sysc()/_idle_sysc()?
> 
> It all comes down to if SIDLE_SMART_WKUP/MSTANDBY_SMART_WKUP programming
> be considered as 'idlemode' programming or 'enwakeup' programming.
> If you consider these are being part of 'enwakeup' progrmming, these should
> certainly be handled as part of _enable_wakeup() and _disable_wakeup().
> 
> Today, in some cases, these are *also* handled as part of _enable_sysc()
> and _idle_sysc(). The way _enable_wakeup() is invoked from _enable_sysc()
> is also very inconsistent. For instance, for any IP which supports
> SYSC_HAS_MIDLEMODE and SYSC_HAS_ENAWAKEUP, we invoke _enable_wakeup()
> regardless of the MIDLEMODE programmmed.
> While in case of the IP supporting SYSC_HAS_SIDLEMODE, _enable_wakeup() is
> invoked only when the SIDLEMODE programmed is SMART or SMART_WKUP.
> 
> I understand the cleanups you are suggesting below as part of the movement
> of some of these things outside of mach-omap2.
> I was more looking at fixing the existing piece so its readable and does
> things more consistently.

Do you have any further thoughts on how we should do about this?

regards,
Rajendra

> 
> regards,
> Rajendra
> 
>>
>>> and unnecessary.
>>
>> So here's part of the reason why the module wakeup control functions 
>> should remain separate.  When the kernel boots, all the PM features should 
>> be disabled.  Then mach-omap2/pm*.c should enables PM features where 
>> they're needed.
>>
>> Right now, mach-omap2/pm34xx.c sets module WKEN bits.  (These direct 
>> register accesses need to be moved as part of the cleanup work, of moving 
>> the PM/PRM/CM code into drivers.)  But the list of IP blocks that 
>> should be allowed to wake the system is board-dependent.
>>
>> So really, what mach-omap2/pm34xx.c should do is to call into the hwmod 
>> enable-wakeups code to enable MPU wakeups for all the IP blocks that have 
>> some DT property set, something like 'enable-wakeups'.  Then the hwmod 
>> code should ensure that the PRM wakeup-enable and GRPSEL bits are 
>> programmed (by calling into the PRM driver code) and should then either 
>> set the ENAWAKEUP bits or put the module into smart_wkup MSTANDBY/MIDLE.
>>
>> Similarly, when the PM driver is unloaded, it should set no-idle on all 
>> the IP blocks that were marked as wakeup-capable and disable the PRCM 
>> wakeup control bits, by calling some hwmod disable-wakeups code.
>>
>>> Well, the _enable_sysc()/_idle_sysc() handled only the mstandby modes
>>> correctly. So fix them so they also handle the midle modes correctly
>>
>> If there's a fix here, please split that out into a separate patch.
>>
>>
>> - Paul
>>
> 

--
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
Paul Walmsley April 23, 2013, 8:19 a.m. UTC | #4 
Hi Rajendra,

On Thu, 18 Apr 2013, Rajendra Nayak wrote:

> >>> _enable_wakeup() and _disable_wakeup() are expected to program the
> >>> OCP_SYSCONFIG.ENAWAKEUP bit.
> >>
> >> These functions were originally intended to take care of everything needed 
> >> for the IP block to wake up the chip, including the PRCM WKEN programming.  
> >> ENAWAKEUP is simply one part of that.
> >>
> >>> Get rid of the additional sidle/mstandby programming in them, as its 
> >>> confusing (this is expected to be handled elsewhere as part of 
> >>> _enable_sysc()/__idle_sysc())
> >>
> >> Sorry, why does the expectation exist for the code to enable and disable 
> >> device wakeup to be part of _enable_sysc()/_idle_sysc(), rather than in 
> >> functions called by _enable_sysc()/_idle_sysc()?
> > 
> > It all comes down to if SIDLE_SMART_WKUP/MSTANDBY_SMART_WKUP programming
> > be considered as 'idlemode' programming or 'enwakeup' programming.
> > If you consider these are being part of 'enwakeup' progrmming, these should
> > certainly be handled as part of _enable_wakeup() and _disable_wakeup().
> > 
> > Today, in some cases, these are *also* handled as part of _enable_sysc()
> > and _idle_sysc(). The way _enable_wakeup() is invoked from _enable_sysc()
> > is also very inconsistent. For instance, for any IP which supports
> > SYSC_HAS_MIDLEMODE and SYSC_HAS_ENAWAKEUP, we invoke _enable_wakeup()
> > regardless of the MIDLEMODE programmmed.
> > While in case of the IP supporting SYSC_HAS_SIDLEMODE, _enable_wakeup() is
> > invoked only when the SIDLEMODE programmed is SMART or SMART_WKUP.
> > 
> > I understand the cleanups you are suggesting below as part of the movement
> > of some of these things outside of mach-omap2.
> > I was more looking at fixing the existing piece so its readable and does
> > things more consistently.
> 
> Do you have any further thoughts on how we should do about this?

Is it possible to implement a solution that preserves _enable_wakeup() and 
_disable_wakeup() as distinct functions, that can be called by separate 
wakeup control entry points?

If it makes sense to change _enable_sysc() so that it doesn't call 
_enable_wakeup(), but does similar work itself, that's fine with me, as 
long as there's not too much code duplication.

It will be good to have the inconsistencies fixed.


- Paul
--
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
Rajendra Nayak April 26, 2013, 7:08 a.m. UTC | #5 
On Tuesday 23 April 2013 01:49 PM, Paul Walmsley wrote:
> Hi Rajendra,
> 
> On Thu, 18 Apr 2013, Rajendra Nayak wrote:
> 
>>>>> _enable_wakeup() and _disable_wakeup() are expected to program the
>>>>> OCP_SYSCONFIG.ENAWAKEUP bit.
>>>>
>>>> These functions were originally intended to take care of everything needed 
>>>> for the IP block to wake up the chip, including the PRCM WKEN programming.  
>>>> ENAWAKEUP is simply one part of that.
>>>>
>>>>> Get rid of the additional sidle/mstandby programming in them, as its 
>>>>> confusing (this is expected to be handled elsewhere as part of 
>>>>> _enable_sysc()/__idle_sysc())
>>>>
>>>> Sorry, why does the expectation exist for the code to enable and disable 
>>>> device wakeup to be part of _enable_sysc()/_idle_sysc(), rather than in 
>>>> functions called by _enable_sysc()/_idle_sysc()?
>>>
>>> It all comes down to if SIDLE_SMART_WKUP/MSTANDBY_SMART_WKUP programming
>>> be considered as 'idlemode' programming or 'enwakeup' programming.
>>> If you consider these are being part of 'enwakeup' progrmming, these should
>>> certainly be handled as part of _enable_wakeup() and _disable_wakeup().
>>>
>>> Today, in some cases, these are *also* handled as part of _enable_sysc()
>>> and _idle_sysc(). The way _enable_wakeup() is invoked from _enable_sysc()
>>> is also very inconsistent. For instance, for any IP which supports
>>> SYSC_HAS_MIDLEMODE and SYSC_HAS_ENAWAKEUP, we invoke _enable_wakeup()
>>> regardless of the MIDLEMODE programmmed.
>>> While in case of the IP supporting SYSC_HAS_SIDLEMODE, _enable_wakeup() is
>>> invoked only when the SIDLEMODE programmed is SMART or SMART_WKUP.
>>>
>>> I understand the cleanups you are suggesting below as part of the movement
>>> of some of these things outside of mach-omap2.
>>> I was more looking at fixing the existing piece so its readable and does
>>> things more consistently.
>>
>> Do you have any further thoughts on how we should do about this?
> 
> Is it possible to implement a solution that preserves _enable_wakeup() and 
> _disable_wakeup() as distinct functions, that can be called by separate 
> wakeup control entry points?
> 
> If it makes sense to change _enable_sysc() so that it doesn't call 
> _enable_wakeup(), but does similar work itself, that's fine with me, as 
> long as there's not too much code duplication.
> 
> It will be good to have the inconsistencies fixed.

Sure, I'll post something on those lines shortly.

regards,
Rajendra
> 
> 
> - Paul
> 

--
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
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index b45c8fb..fdd55cd 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -588,9 +588,7 @@  static void _set_idle_ioring_wakeup(struct omap_hwmod *oh, bool set_wake)
 static int _enable_wakeup(struct omap_hwmod *oh, u32 *v)
 {
 	if (!oh->class->sysc ||
-	    !((oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP) ||
-	      (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) ||
-	      (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP)))
+	    !(oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP))
 		return -EINVAL;
 
 	if (!oh->class->sysc->sysc_fields) {
@@ -601,11 +599,6 @@  static int _enable_wakeup(struct omap_hwmod *oh, u32 *v)
 	if (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)
 		*v |= 0x1 << oh->class->sysc->sysc_fields->enwkup_shift;
 
-	if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP)
-		_set_slave_idlemode(oh, HWMOD_IDLEMODE_SMART_WKUP, v);
-	if (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP)
-		_set_master_standbymode(oh, HWMOD_IDLEMODE_SMART_WKUP, v);
-
 	/* XXX test pwrdm_get_wken for this hwmod's subsystem */
 
 	return 0;
@@ -621,9 +614,7 @@  static int _enable_wakeup(struct omap_hwmod *oh, u32 *v)
 static int _disable_wakeup(struct omap_hwmod *oh, u32 *v)
 {
 	if (!oh->class->sysc ||
-	    !((oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP) ||
-	      (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP) ||
-	      (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP)))
+	    !(oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP))
 		return -EINVAL;
 
 	if (!oh->class->sysc->sysc_fields) {
@@ -634,11 +625,6 @@  static int _disable_wakeup(struct omap_hwmod *oh, u32 *v)
 	if (oh->class->sysc->sysc_flags & SYSC_HAS_ENAWAKEUP)
 		*v &= ~(0x1 << oh->class->sysc->sysc_fields->enwkup_shift);
 
-	if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP)
-		_set_slave_idlemode(oh, HWMOD_IDLEMODE_SMART, v);
-	if (oh->class->sysc->idlemodes & MSTANDBY_SMART_WKUP)
-		_set_master_standbymode(oh, HWMOD_IDLEMODE_SMART, v);
-
 	/* XXX test pwrdm_get_wken for this hwmod's subsystem */
 
 	return 0;
@@ -1351,13 +1337,24 @@  static void _enable_sysc(struct omap_hwmod *oh)
 
 	clkdm = _get_clkdm(oh);
 	if (sf & SYSC_HAS_SIDLEMODE) {
+		if (oh->flags & HWMOD_SWSUP_SIDLE) {
+			idlemode = HWMOD_IDLEMODE_NO;
+		} else {
+			if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP)
+				idlemode = HWMOD_IDLEMODE_SMART_WKUP;
+			else
+				idlemode = HWMOD_IDLEMODE_SMART;
+		}
+
+		/*
+		 * This is special handling for some IPs like
+		 * 32k sync timer. Force them to idle!
+		 */
 		clkdm_act = (clkdm && clkdm->flags & CLKDM_ACTIVE_WITH_MPU);
 		if (clkdm_act && !(oh->class->sysc->idlemodes &
 				   (SIDLE_SMART | SIDLE_SMART_WKUP)))
 			idlemode = HWMOD_IDLEMODE_FORCE;
-		else
-			idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
-				HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
+
 		_set_slave_idlemode(oh, idlemode, &v);
 	}
 
@@ -1423,13 +1420,14 @@  static void _idle_sysc(struct omap_hwmod *oh)
 	sf = oh->class->sysc->sysc_flags;
 
 	if (sf & SYSC_HAS_SIDLEMODE) {
-		/* XXX What about HWMOD_IDLEMODE_SMART_WKUP? */
-		if (oh->flags & HWMOD_SWSUP_SIDLE ||
-		    !(oh->class->sysc->idlemodes &
-		      (SIDLE_SMART | SIDLE_SMART_WKUP)))
+		if (oh->flags & HWMOD_SWSUP_SIDLE) {
 			idlemode = HWMOD_IDLEMODE_FORCE;
-		else
-			idlemode = HWMOD_IDLEMODE_SMART;
+		} else {
+			if (oh->class->sysc->idlemodes & SIDLE_SMART_WKUP)
+				idlemode = HWMOD_IDLEMODE_SMART_WKUP;
+			else
+				idlemode = HWMOD_IDLEMODE_SMART;
+		}
 		_set_slave_idlemode(oh, idlemode, &v);
 	}