Re: [PATCH] OMAP2+: omap_hwmod: fix wakeup enable/disable for consistency

2010-12-14 Thread Paul Walmsley
On Thu, 9 Dec 2010, Kevin Hilman wrote:

 In the omap_hwmod core, most of the SYSCONFIG register helper
 functions do not directly write the register, but instead just modify
 a value passed in.
 
 This patch converts the _enable_wakeup() and _disable_wakeup() helper
 functions to take a value argument and only modify it instead of
 actually writing the register.  This makes the wakeup helpers
 consistent with the other helper functions and avoids unintentional
 problems like the following.

Thanks, this looks great.  Queued for 2.6.38 in the 'hwmod_b_2.6.38' 
branch along with the OMAP4 hwmod cleanup patches.

The current integration tag that contains these patches is 
'integration-2.6.38-20101214-012' available from 
git://git.pwsan.com/linux-integration


- Paul
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] OMAP2+: omap_hwmod: fix wakeup enable/disable for consistency

2010-12-09 Thread Kevin Hilman
In the omap_hwmod core, most of the SYSCONFIG register helper
functions do not directly write the register, but instead just modify
a value passed in.

This patch converts the _enable_wakeup() and _disable_wakeup() helper
functions to take a value argument and only modify it instead of
actually writing the register.  This makes the wakeup helpers
consistent with the other helper functions and avoids unintentional
problems like the following.

This problem was found after discovering that GPIO wakeups were no
longer functional.  The root cause was that the ENAWAKEUP bit of the
SYSCONFIG register was being unintentionaly overwritten, leaving
wakeups disabled after the following two commits were combined:

commit: 9980ce53c97392a3dbdc9d1ac3e455d79b4167ed
OMAP: hwmod: Enable module wakeup if in smartidle

commit: 78f26e872f77b6312273216de1a8f836c6f2e143
OMAP: hwmod: Set autoidle after smartidle during _sysc_enable

There resulting in code in _enable_sysc() was this:

/*
 * XXX The clock framework should handle this, by
 * calling into this code.  But this must wait until the
 * clock structures are tagged with omap_hwmod entries
 */
if ((oh-flags  HWMOD_SET_DEFAULT_CLOCKACT) 
(sf  SYSC_HAS_CLOCKACTIVITY))
_set_clockactivity(oh, oh-class-sysc-clockact, v);

_write_sysconfig(v, oh);

so here, 'v' has wakeups disabled.

/* If slave is in SMARTIDLE, also enable wakeup */
if ((sf  SYSC_HAS_SIDLEMODE)  !(oh-flags  HWMOD_SWSUP_SIDLE))
_enable_wakeup(oh);

Here wakeup is enabled in the SYSCONFIG register (but 'v' is not updated)

/*
 * Set the autoidle bit only after setting the smartidle bit
 * Setting this will not have any impact on the other modules.
 */
if (sf  SYSC_HAS_AUTOIDLE) {
idlemode = (oh-flags  HWMOD_NO_OCP_AUTOIDLE) ?
0 : 1;
_set_module_autoidle(oh, idlemode, v);
_write_sysconfig(v, oh);
}

And here, SYSCONFIG is updated again using 'v', which does not have
wakeups enabled, resulting in ENAWAKEUP being cleared.

Special thanks to Benoit Cousson for pointing out that wakeups were
supposed to be automatically enabled when a hwmod is enabled, and thus
helping target the root cause of this problem.

Cc: Paul Walmsley p...@pwsan.com
Cc: Benoit Cousson b-cous...@ti.com
Signed-off-by: Kevin Hilman khil...@deeprootsystems.com
---
Applies on top of Paul's hwmod_a_2.6.38 branch.

 arch/arm/mach-omap2/omap_hwmod.c |   32 +---
 1 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index a039b37..84a03eb 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -388,9 +388,9 @@ static int _set_module_autoidle(struct omap_hwmod *oh, u8 
autoidle,
  * Allow the hardware module @oh to send wakeups.  Returns -EINVAL
  * upon error or 0 upon success.
  */
-static int _enable_wakeup(struct omap_hwmod *oh)
+static int _enable_wakeup(struct omap_hwmod *oh, u32 *v)
 {
-   u32 v, wakeup_mask;
+   u32 wakeup_mask;
 
if (!oh-class-sysc ||
!(oh-class-sysc-sysc_flags  SYSC_HAS_ENAWAKEUP))
@@ -403,9 +403,7 @@ static int _enable_wakeup(struct omap_hwmod *oh)
 
wakeup_mask = (0x1  oh-class-sysc-sysc_fields-enwkup_shift);
 
-   v = oh-_sysc_cache;
-   v |= wakeup_mask;
-   _write_sysconfig(v, oh);
+   *v |= wakeup_mask;
 
/* XXX test pwrdm_get_wken for this hwmod's subsystem */
 
@@ -421,9 +419,9 @@ static int _enable_wakeup(struct omap_hwmod *oh)
  * Prevent the hardware module @oh to send wakeups.  Returns -EINVAL
  * upon error or 0 upon success.
  */
-static int _disable_wakeup(struct omap_hwmod *oh)
+static int _disable_wakeup(struct omap_hwmod *oh, u32 *v)
 {
-   u32 v, wakeup_mask;
+   u32 wakeup_mask;
 
if (!oh-class-sysc ||
!(oh-class-sysc-sysc_flags  SYSC_HAS_ENAWAKEUP))
@@ -436,9 +434,7 @@ static int _disable_wakeup(struct omap_hwmod *oh)
 
wakeup_mask = (0x1  oh-class-sysc-sysc_fields-enwkup_shift);
 
-   v = oh-_sysc_cache;
-   v = ~wakeup_mask;
-   _write_sysconfig(v, oh);
+   *v = ~wakeup_mask;
 
/* XXX test pwrdm_get_wken for this hwmod's subsystem */
 
@@ -786,11 +782,11 @@ static void _enable_sysc(struct omap_hwmod *oh)
(sf  SYSC_HAS_CLOCKACTIVITY))
_set_clockactivity(oh, oh-class-sysc-clockact, v);
 
-   _write_sysconfig(v, oh);
-
/* If slave is in SMARTIDLE, also enable wakeup */
if ((sf  SYSC_HAS_SIDLEMODE)  !(oh-flags  HWMOD_SWSUP_SIDLE))
-   _enable_wakeup(oh);
+   _enable_wakeup(oh, v);
+
+   _write_sysconfig(v, oh);
 
/*
 * Set the autoidle bit only after setting the smartidle bit
@@ -2009,13 +2005,16 @@ int