[PATCH v2 0/2] Fixes in hwmod reset code

[PATCH v2 0/2] Fixes in hwmod reset code

[Rajendra Nayak]

This fixes a couple of issues around hwmod reset
APIs'.
Patches are based on 'hwmod_enable_remaining_hwmods_devel_3.4'
of git://git.pwsan.com/linux-2.6

Rajendra Nayak (2):
  ARM: omap: hwmod: Restore sysc after a reset
  ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

 arch/arm/mach-omap2/omap_hwmod.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

[PATCH v2 1/2] ARM: omap: hwmod: Restore sysc after a reset

[Rajendra Nayak]

After a softreset, make sure the sysc settings are correctly
restored.

Reported-by: Anand Gadiyar <gadiyar@ti.com>
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 8ac26f2..6c0e7eb 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2767,6 +2767,10 @@ int omap_hwmod_reset(struct omap_hwmod *oh)
 
 	spin_lock_irqsave(&oh->_lock, flags);
 	r = _reset(oh);
+	if (oh->class->sysc) {
+		_update_sysc_cache(oh);
+		_enable_sysc(oh);
+	}
 	spin_unlock_irqrestore(&oh->_lock, flags);
 
 	return r;
-- 
1.7.1

[PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Rajendra Nayak]

omap_hwmod_softreset() does not seem to wait for reset status
after doing a softreset. Make it use _ocp_softreset() instead
which does this correctly.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 6c0e7eb..0c64853 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2423,20 +2423,10 @@ void omap_hwmod_write(u32 v, struct omap_hwmod *oh, u16 reg_offs)
  */
 int omap_hwmod_softreset(struct omap_hwmod *oh)
 {
-	u32 v;
-	int ret;
-
-	if (!oh || !(oh->_sysc_cache))
+	if (!oh)
 		return -EINVAL;
 
-	v = oh->_sysc_cache;
-	ret = _set_softreset(oh, &v);
-	if (ret)
-		goto error;
-	_write_sysconfig(v, oh);
-
-error:
-	return ret;
+	return _ocp_softreset(oh);
 }
 
 /**
-- 
1.7.1

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Paul Walmsley]

Hi Rajendra,

On Tue, 13 Mar 2012, Rajendra Nayak wrote:

> omap_hwmod_softreset() does not seem to wait for reset status
> after doing a softreset. Make it use _ocp_softreset() instead
> which does this correctly.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Anand Gadiyar <gadiyar@ti.com>
> Cc: Shubhrajyoti D <shubhrajyoti@ti.com>

Just noticed that this change results in I2C softreset failed messages on 
OMAP2/3/4 on v3.4-rc2.  For example, on 2420:

[    0.200378] omap_hwmod: i2c1: softreset failed (waited 10000 usec)
[    0.222076] omap_hwmod: i2c2: softreset failed (waited 10000 usec)

Looking more closely at the code, I think the intention of the original 
code was basically correct.  HDQ and I2C both need to execute some custom 
reset code after the SOFTRESET bit is set, but before the RESETDONE bit is 
tested.  After this patch, the internal hwmod code tries to wait for 
RESETDONE to change, before the custom code runs -- and that is going to 
fail.

Just out of curiosity, was the change in this patch prompted by some code 
that needed the change?  Or was this a theoretical problem, driven by a 
code review?  If the latter, we should probably revert this patch, and 
maybe improve the documentation and omap_hwmod_softreset() function name 
to clarify what it actually does.  But if it's the former, it looks like 
we'll need a way to support both the full, standard SOFTRESET sequence 
(perhaps via omap_hwmod_reset() ?) while also supporting the HDQ/I2C 
custom reset code.


regards,


- Paul


> ---
>  arch/arm/mach-omap2/omap_hwmod.c |   14 ++------------
>  1 files changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 6c0e7eb..0c64853 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2423,20 +2423,10 @@ void omap_hwmod_write(u32 v, struct omap_hwmod *oh, u16 reg_offs)
>   */
>  int omap_hwmod_softreset(struct omap_hwmod *oh)
>  {
> -	u32 v;
> -	int ret;
> -
> -	if (!oh || !(oh->_sysc_cache))
> +	if (!oh)
>  		return -EINVAL;
>  
> -	v = oh->_sysc_cache;
> -	ret = _set_softreset(oh, &v);
> -	if (ret)
> -		goto error;
> -	_write_sysconfig(v, oh);
> -
> -error:
> -	return ret;
> +	return _ocp_softreset(oh);
>  }
>  
>  /**
> -- 
> 1.7.1
> 
> --
> 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

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Rajendra Nayak]

Hi Paul,

On Wednesday 11 April 2012 05:41 AM, Paul Walmsley wrote:
> Just noticed that this change results in I2C softreset failed messages on
> OMAP2/3/4 on v3.4-rc2.  For example, on 2420:
>
> [    0.200378] omap_hwmod: i2c1: softreset failed (waited 10000 usec)
> [    0.222076] omap_hwmod: i2c2: softreset failed (waited 10000 usec)
>
> Looking more closely at the code, I think the intention of the original
> code was basically correct.  HDQ and I2C both need to execute some custom
> reset code after the SOFTRESET bit is set, but before the RESETDONE bit is
> tested.  After this patch, the internal hwmod code tries to wait for
> RESETDONE to change, before the custom code runs -- and that is going to
> fail.
>
> Just out of curiosity, was the change in this patch prompted by some code
> that needed the change?  Or was this a theoretical problem, driven by a
> code review?

The changes were done to fix up random L3 interconnect errors that
Anand G was seeing(during i2c reset operation) on some customer 
platforms. I seem to have completely overlooked the I2C_EN programming 
part done in omap_i2c_reset() function when I did the patch.

While the patch did fix the issue for Anand, I guess it
was because of the additional delay post reset, waiting on the
RESETDONE bit and timing out, before accessing the i2c_con register.
So looks like this patch should certainly be reverted but atleast some
platforms/modules (the issue was seen on OMAP4/i2c) will need some
delay between the omap_hwmod_softreset() call and any subsequent module
register accesses.
The patch from Fernando [1] can be quite useful if we can use the
'srst_udelay' field to populate the appropriate delay needed which can
then be used up in omap_hwmod_softreset() function.
I am out sick today, but I can try some on these lines tomorrow once
I am in office, if the approach seems fine to you.
Btw, is [1] queued already by you/Benoit to go upstream or are there
issues with it?

regards,
Rajendra

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/086713.html

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Paul Walmsley]

Hi Rajendra,

On Wed, 11 Apr 2012, Rajendra Nayak wrote:

> The changes were done to fix up random L3 interconnect errors that
> Anand G was seeing(during i2c reset operation) on some customer platforms.

Okay.

> I seem to have completely overlooked the I2C_EN programming part done in 
> omap_i2c_reset() function when I did the patch.

No worries.

> While the patch did fix the issue for Anand, I guess it
> was because of the additional delay post reset, waiting on the
> RESETDONE bit and timing out, before accessing the i2c_con register.
> So looks like this patch should certainly be reverted but atleast some
> platforms/modules (the issue was seen on OMAP4/i2c) will need some
> delay between the omap_hwmod_softreset() call and any subsequent module
> register accesses.
> The patch from Fernando [1] can be quite useful if we can use the
> 'srst_udelay' field to populate the appropriate delay needed which can
> then be used up in omap_hwmod_softreset() function.
> I am out sick today, but I can try some on these lines tomorrow once
> I am in office, if the approach seems fine to you.

Sorry to hear that - I hope you feel better soon.

That approach sounds fine to me, but I don't think Fernando's patch will 
help with I2C..  Since it uses a custom reset function omap_i2c_reset(), 
the delay will actually need to go there.

Another option though is that it might be possible to generalize the 
_ocp_softreset() function to handle the I2C/HDQ cases, so they wouldn't 
need custom reset functions.  I'd guess that's probably 3.5 material, 
though.

> Btw, is [1] queued already by you/Benoit to go upstream or are there
> issues with it?
> 
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-February/086713.html

Tony pulled it:

http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap.git;a=commit;h=b8a633451951d149494076393698880453bf6bfe

Not sure when he plans to send it upstream, but I now think it should go 
up sooner as part of v3.4-rc fixes, to avoid any potential OCP bus errors.


- Paul

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Rajendra Nayak]

Hi Paul,

On Thursday 12 April 2012 12:29 AM, Paul Walmsley wrote:
> That approach sounds fine to me, but I don't think Fernando's patch will
> help with I2C..  Since it uses a custom reset function omap_i2c_reset(),
> the delay will actually need to go there.

While working on fixing this I stumbled upon a couple of other
things which don't seem right.

The i2c custom reset funtion (omap_i2c_reset) uses a hwmod exposed
api to set the SOFTRESET bit, however it then accesses the hwmod
internal structure to poll on RESETDONE status bit. Should there be
another function exposed to poll on RESETDONE too?

_reset() function documents its return value to be -ETIMEDOUT, if the
module fails to reset in time, and hence expects the custom reset
function to return the same in such cases. The omap_i2c_reset() function
seems to return 0 even in cases of timeout. However looking more closely
the return value from _reset() does not seem to be used in the framework
today.

The comment in _ocp_softreset() suggests the intent was to add a new
state to the hwmod state machine.

         /*
          * XXX add _HWMOD_STATE_WEDGED for modules that don't come back 
from
          * _wait_target_ready() or _reset()
          */

is it still the case, or should _reset() just return a void?

regards,
Rajendra

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Paul Walmsley]

Hi Rajendra,

On Thu, 12 Apr 2012, Rajendra Nayak wrote:

> On Thursday 12 April 2012 12:29 AM, Paul Walmsley wrote:
> > That approach sounds fine to me, but I don't think Fernando's patch will
> > help with I2C..  Since it uses a custom reset function omap_i2c_reset(),
> > the delay will actually need to go there.
> 
> While working on fixing this I stumbled upon a couple of other
> things which don't seem right.
> 
> The i2c custom reset funtion (omap_i2c_reset) uses a hwmod exposed
> api to set the SOFTRESET bit, however it then accesses the hwmod
> internal structure to poll on RESETDONE status bit. Should there be
> another function exposed to poll on RESETDONE too?

Sure, something like that would make sense for 3.5.

> _reset() function documents its return value to be -ETIMEDOUT, if the
> module fails to reset in time, and hence expects the custom reset
> function to return the same in such cases. The omap_i2c_reset() function
> seems to return 0 even in cases of timeout. However looking more closely
> the return value from _reset() does not seem to be used in the framework
> today.
> 
> The comment in _ocp_softreset() suggests the intent was to add a new
> state to the hwmod state machine.
> 
>         /*
>          * XXX add _HWMOD_STATE_WEDGED for modules that don't come back from
>          * _wait_target_ready() or _reset()
>          */
> 
> is it still the case, or should _reset() just return a void?

There's a patch queued to pass along the return status from _reset():

http://marc.info/?l=linux-omap&m=133417544011862&w=2

As far as the new state goes, if you'd like to add a new state to indicate 
an IP block that fails _reset()/_wait_target_ready() and should be blocked 
from further use, that sounds good to me...


regards,

- Paul

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Rajendra Nayak]

Hi Paul,

>> >  While the patch did fix the issue for Anand, I guess it
>> >  was because of the additional delay post reset, waiting on the
>> >  RESETDONE bit and timing out, before accessing the i2c_con register.

I thought some more on this and the logic of the delay between the
SOFTRESET bit being set and an immediate register access did not make
much sense, because even polling on RESETDONE bit involves an immediate
i2c register access, albeit a different one.

I had a chat again with Anand this morning and realized the patch that
fixed the problems he saw on some customer hardware had changes from
both patch 1/2 and patch 2/2 from this series clubbed into one patch.
I seem to have split them into 2 patches later while posting it out
on the list.

So in all likelihood its the changes in patch 1/2 which made a
difference and the patch 2/2 was a result of my (wrong) analysis based
on code review.

The problems seen were extremely rare and not having access to those
few failing boards is making it difficult to re-validate with these
changes removed. However based on my analysis (hopefully right this time 
:)) it seems safe to revert this patch in mainline.

Should I go ahead and send a revert for the -rc?

regards,
Rajendra

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Paul Walmsley]

Hi Rajendra,

thanks for the summary,

On Fri, 13 Apr 2012, Rajendra Nayak wrote:

> Should I go ahead and send a revert for the -rc?

Thanks for the offer, I don't think it's needed -- I'll just build the 
revert changelog here with your comments from your E-mail.

regards


- Paul

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Paul Walmsley]

Hi Rajendra

here's what I've queued for v3.4-rc; please let me know if you have any 
comments.


- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Fri, 13 Apr 2012 05:08:43 -0600
Subject: [PATCH] ARM: OMAP2+: hwmod: Revert "ARM: OMAP2+: hwmod: Make
 omap_hwmod_softreset wait for reset status"

This reverts commit f9a2f9c3fa76eec55928e8e06f3094c8f01df7cb.  This
commit caused a regression in the I2C hwmod reset on OMAP2/3/4,
logging messages similar to these during boot:

[    0.200378] omap_hwmod: i2c1: softreset failed (waited 10000 usec)
[    0.222076] omap_hwmod: i2c2: softreset failed (waited 10000 usec)

While the original patch was intended to fix some reset-related timing
issues, it's believed that these problems were actually fixed by
commit 2800852a079504f35f88e44faf5c9c96318c0cca ("ARM: OMAP2+: hwmod:
Restore sysc after a reset"):

    http://marc.info/?l=linux-arm-kernel&m=133410322617245&w=2

Cc: Rajendra Nayak <rnayak@ti.com>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 45f1d9c..7144ae6 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1906,10 +1906,20 @@ void omap_hwmod_write(u32 v, struct omap_hwmod *oh, u16 reg_offs)
  */
 int omap_hwmod_softreset(struct omap_hwmod *oh)
 {
-	if (!oh)
+	u32 v;
+	int ret;
+
+	if (!oh || !(oh->_sysc_cache))
 		return -EINVAL;
 
-	return _ocp_softreset(oh);
+	v = oh->_sysc_cache;
+	ret = _set_softreset(oh, &v);
+	if (ret)
+		goto error;
+	_write_sysconfig(v, oh);
+
+error:
+	return ret;
 }
 
 /**
-- 
1.7.10

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Rajendra Nayak]

On Friday 13 April 2012 04:52 PM, Paul Walmsley wrote:
> Hi Rajendra
>
> here's what I've queued for v3.4-rc; please let me know if you have any
> comments.

Looks good, thanks Paul.

>
>
> - Paul
>
> From: Paul Walmsley<paul@pwsan.com>
> Date: Fri, 13 Apr 2012 05:08:43 -0600
> Subject: [PATCH] ARM: OMAP2+: hwmod: Revert "ARM: OMAP2+: hwmod: Make
>   omap_hwmod_softreset wait for reset status"
>
> This reverts commit f9a2f9c3fa76eec55928e8e06f3094c8f01df7cb.  This
> commit caused a regression in the I2C hwmod reset on OMAP2/3/4,
> logging messages similar to these during boot:
>
> [    0.200378] omap_hwmod: i2c1: softreset failed (waited 10000 usec)
> [    0.222076] omap_hwmod: i2c2: softreset failed (waited 10000 usec)
>
> While the original patch was intended to fix some reset-related timing
> issues, it's believed that these problems were actually fixed by
> commit 2800852a079504f35f88e44faf5c9c96318c0cca ("ARM: OMAP2+: hwmod:
> Restore sysc after a reset"):
>
>      http://marc.info/?l=linux-arm-kernel&m=133410322617245&w=2
>
> Cc: Rajendra Nayak<rnayak@ti.com>
> Signed-off-by: Paul Walmsley<paul@pwsan.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |   14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 45f1d9c..7144ae6 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -1906,10 +1906,20 @@ void omap_hwmod_write(u32 v, struct omap_hwmod *oh, u16 reg_offs)
>    */
>   int omap_hwmod_softreset(struct omap_hwmod *oh)
>   {
> -	if (!oh)
> +	u32 v;
> +	int ret;
> +
> +	if (!oh || !(oh->_sysc_cache))
>   		return -EINVAL;
>
> -	return _ocp_softreset(oh);
> +	v = oh->_sysc_cache;
> +	ret = _set_softreset(oh,&v);
> +	if (ret)
> +		goto error;
> +	_write_sysconfig(v, oh);
> +
> +error:
> +	return ret;
>   }
>
>   /**

[PATCH v2 0/2] Fixes in hwmod reset code

[Rajendra Nayak]

This fixes a couple of issues around hwmod reset
APIs'.
Patches are based on 3.3.-rc7.

Rajendra Nayak (2):
  ARM: omap: hwmod: Restore sysc after a reset
  ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

 arch/arm/mach-omap2/omap_hwmod.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

[PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Rajendra Nayak]

omap_hwmod_softreset() does not seem to wait for reset status
after doing a softreset. Make it use _ocp_softreset() instead
which does this correctly.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Anand Gadiyar <gadiyar@ti.com>
Cc: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index ae56939..489f82d 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1907,20 +1907,10 @@ void omap_hwmod_write(u32 v, struct omap_hwmod *oh, u16 reg_offs)
  */
 int omap_hwmod_softreset(struct omap_hwmod *oh)
 {
-	u32 v;
-	int ret;
-
-	if (!oh || !(oh->_sysc_cache))
+	if (!oh)
 		return -EINVAL;
 
-	v = oh->_sysc_cache;
-	ret = _set_softreset(oh, &v);
-	if (ret)
-		goto error;
-	_write_sysconfig(v, oh);
-
-error:
-	return ret;
+	return _ocp_softreset(oh);
 }
 
 /**
-- 
1.7.1

Re: [PATCH v2 2/2] ARM: omap: hwmod: Make omap_hwmod_softreset wait for reset status

[Paul Walmsley]

On Tue, 13 Mar 2012, Rajendra Nayak wrote:

> omap_hwmod_softreset() does not seem to wait for reset status
> after doing a softreset. Make it use _ocp_softreset() instead
> which does this correctly.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Anand Gadiyar <gadiyar@ti.com>
> Cc: Shubhrajyoti D <shubhrajyoti@ti.com>

Thanks, queued for 3.4-rc.


- Paul