[PATCH v2 5/8] ARM: OMAP2+: hwmod: ensure that SYSCONFIG bits are reprogrammed after a reset

[Paul Walmsley]

On Thu, 15 Mar 2012, Ramirez Luna, Omar wrote:

> On Thu, Mar 15, 2012 at 1:25 AM, Paul Walmsley <paul at pwsan.com> wrote:
> > On Wed, 14 Mar 2012, Ramirez Luna, Omar wrote:
> >
> >> If we reached here the reset lines will be asserted, and then the code
> >> below touches sysc registers on a module under reset.
> >
> > Do you know of any case where this would be a problem?  Seems like it
> > would only affect IP blocks that have both hard reset lines and SYSCONFIG
> > registers, and as far as I know, we don't have any of those defined?
> 
> MMU (not yet mainlined) has both, a reset line and a sysconfig.

Thanks, I've updated this patch with this fix, included below; rebased 
the series on v3.4-rc2 (the branch is now "hwmod_code_cleanup_3.5"); and 
will ask Tony to re-pull it.

> I have been holding the hwmod for some time, but now that
> rpmsg/remoteproc is going to mainline it could make use of it along
> with omap3isp, however now I need to define functions to handle the
> reset lines (although I was fine with hwmod handling it).

OK.

> AFAIKnew, hwmod handling the reset line was fine (IMHO), the only 2 things were:
> - For the drivers to somehow make use of shutdown/enable if they
> needed they hwmod under reset and out.
> - The annoying: hwmod XX failed to hardreset because of the wrong
> reset sequence but causing no functional issues.

Thanks for the feedback.  Apparently the OMAP4 DSP/ISS people had some 
issues with the old sequence, although I haven't heard anything from them 
directly, so it's unclear what exactly is going on with their code.
 
Anyway, please don't hesitate to let us know if you find any other issues 
with the updated behavior.


- Paul

From: Paul Walmsley <paul at pwsan.com>
Date: Mon, 27 Feb 2012 15:23:56 -0700
Subject: [PATCH] ARM: OMAP2+: hwmod: update OCP_SYSCONFIG registers after a
 custom reset

Since OCP_SYSCONFIG bits are cleared during reset, they should be
reprogrammed unless the IP block is being left in reset.  (The only IP
blocks that are left in reset are IP blocks with hardreset lines and
no custom reset function.)  If the IP block is left in reset, then it
is inaccessible to the MPU, and an access to the OCP_SYSCONFIG
register will cause an abort.

This version incorporates comments from Omar Ramirez Luna
<omar.ramirez at ti.com> to skip the OCP_SYSCONFIG access after asserting
hardreset lines.  This allows the MMU (IOMMU) IP block, which has
both hardreset lines and an OCP_SYSCONFIG register.

Signed-off-by: Paul Walmsley <paul at pwsan.com>
Cc: Benoît Cousson <b-cousson at ti.com>
Cc: Omar Ramirez Luna <omar.ramirez at ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   39 ++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index fed805c..451e865 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1480,10 +1480,10 @@ dis_opt_clks:
  * IVA) have idiosyncratic reset sequences.  So for these relatively
  * rare cases, custom reset code can be supplied in the struct
  * omap_hwmod_class .reset function pointer.  Passes along the return
- * value from either _reset() or the custom reset function - these
- * must return -EINVAL if the hwmod cannot be reset this way or if the
- * hwmod is in the wrong state, -ETIMEDOUT if the module did not reset
- * in time, or 0 upon success.
+ * value from either _ocp_softreset() or the custom reset function -
+ * these must return -EINVAL if the hwmod cannot be reset this way or
+ * if the hwmod is in the wrong state, -ETIMEDOUT if the module did
+ * not reset in time, or 0 upon success.
  */
 static int _reset(struct omap_hwmod *oh)
 {
@@ -1491,22 +1491,29 @@ static int _reset(struct omap_hwmod *oh)
 
 	pr_debug("omap_hwmod: %s: resetting\n", oh->name);
 
-	if (oh->class->reset)
-		return oh->class->reset(oh);
-
-	if (!oh->rst_lines_cnt) {
-		r = _ocp_softreset(oh);
-		if (oh->class->sysc) {
-			_update_sysc_cache(oh);
-			_enable_sysc(oh);
+	if (oh->class->reset) {
+		r = oh->class->reset(oh);
+	} else {
+		if (oh->rst_lines_cnt > 0) {
+			for (i = 0; i < oh->rst_lines_cnt; i++)
+				_assert_hardreset(oh, oh->rst_lines[i].name);
+			return 0;
+		} else {
+			r = _ocp_softreset(oh);
 		}
-		return r;
 	}
 
-	for (i = 0; i < oh->rst_lines_cnt; i++)
-		_assert_hardreset(oh, oh->rst_lines[i].name);
+	/*
+	 * OCP_SYSCONFIG bits need to be reprogrammed after a
+	 * softreset.  The _enable() function should be split to avoid
+	 * the rewrite of the OCP_SYSCONFIG register.
+	 */
+	if (oh->class->sysc) {
+		_update_sysc_cache(oh);
+		_enable_sysc(oh);
+	}
 
-	return 0;
+	return r;
 }
 
 /**
-- 
1.7.9.5