diff mbox

[v2] ARM: OMAP2: hwmod: Fix "register offset NULL check" bug

Message ID 1355835378-23437-1-git-send-email-gururaja.hebbar@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hebbar, Gururaja Dec. 18, 2012, 12:56 p.m. UTC 
From: "Hebbar, Gururaja" <gururaja.hebbar@ti.com>

omap4_cminst_wait_module_ready() checks if register offset is NULL.

int omap4_cminst_wait_module_ready(u8 part, u16 inst, s16 cdoffs,
				   u16 clkctrl_offs)
{
	int i = 0;

	if (!clkctrl_offs)
		return 0;

In case of AM33xx, CLKCTRL register offset for different clock domains
are not uniformly placed. An example of this would be the RTC clock
domain with CLKCTRL offset at 0x00.
In such cases the module ready check is skipped which leads to a data
abort during boot-up when RTC registers is accessed.

Since the actual base address is verified in
omap4_cminst_read_inst_reg(), this check here is not required at all
and hence can be removed.

Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
---
Changes in v2:
	- update commit message to reflect the actual cause. Previous
	message conveyed a wrong/opposite message.

:100644 100644 7f9a464... 40545ff... M	arch/arm/mach-omap2/cminst44xx.c
 arch/arm/mach-omap2/cminst44xx.c |    3 ---
 1 file changed, 3 deletions(-)

Comments

Paul Walmsley Dec. 19, 2012, 2:13 a.m. UTC | #1 
On Tue, 18 Dec 2012, Hebbar Gururaja wrote:

> From: "Hebbar, Gururaja" <gururaja.hebbar@ti.com>
> 
> omap4_cminst_wait_module_ready() checks if register offset is NULL.
> 
> int omap4_cminst_wait_module_ready(u8 part, u16 inst, s16 cdoffs,
> 				   u16 clkctrl_offs)
> {
> 	int i = 0;
> 
> 	if (!clkctrl_offs)
> 		return 0;
> 
> In case of AM33xx, CLKCTRL register offset for different clock domains
> are not uniformly placed. An example of this would be the RTC clock
> domain with CLKCTRL offset at 0x00.
> In such cases the module ready check is skipped which leads to a data
> abort during boot-up when RTC registers is accessed.

We've got a few hwmods on OMAP44xx that don't have clkctrl_offs registers 
listed in the hwmod data, and which are not marked with HWMOD_NO_IDLEST.  
What's going to happen in those cases?


- 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
Hebbar, Gururaja Dec. 20, 2012, 4:20 a.m. UTC | #2 
On Wed, Dec 19, 2012 at 07:43:50, Paul Walmsley wrote:
> On Tue, 18 Dec 2012, Hebbar Gururaja wrote:
> 
> > From: "Hebbar, Gururaja" <gururaja.hebbar@ti.com>
> > 
> > omap4_cminst_wait_module_ready() checks if register offset is NULL.
> > 
> > int omap4_cminst_wait_module_ready(u8 part, u16 inst, s16 cdoffs,
> > 				   u16 clkctrl_offs)
> > {
> > 	int i = 0;
> > 
> > 	if (!clkctrl_offs)
> > 		return 0;
> > 
> > In case of AM33xx, CLKCTRL register offset for different clock domains
> > are not uniformly placed. An example of this would be the RTC clock
> > domain with CLKCTRL offset at 0x00.
> > In such cases the module ready check is skipped which leads to a data
> > abort during boot-up when RTC registers is accessed.
> 
> We've got a few hwmods on OMAP44xx that don't have clkctrl_offs registers 
> listed in the hwmod data, and which are not marked with HWMOD_NO_IDLEST.  
> What's going to happen in those cases?

Hmm. This is a special case to me. Let me go back and do some review.
Will be back with some more details.

> 
> 
> - Paul
> 


Regards, 
Gururaja
--
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 Dec. 20, 2012, 7:35 a.m. UTC | #3 
On Thu, 20 Dec 2012, Hebbar, Gururaja wrote:

> On Wed, Dec 19, 2012 at 07:43:50, Paul Walmsley wrote:
> 
> > We've got a few hwmods on OMAP44xx that don't have clkctrl_offs registers 
> > listed in the hwmod data, and which are not marked with HWMOD_NO_IDLEST.  
> > What's going to happen in those cases?
> 
> Hmm. This is a special case to me. Let me go back and do some review.
> Will be back with some more details.

Probably what needs to happen is that those hwmods need to be marked with 
HWMOD_NO_IDLEST, in a separate patch before yours.  As far as I can tell, 
that's how they should have been marked initially.  Then your patch should 
be fine.

- 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
Hebbar, Gururaja Jan. 30, 2013, 2:07 p.m. UTC | #4 
On Thu, Dec 20, 2012 at 13:05:27, Paul Walmsley wrote:
> On Thu, 20 Dec 2012, Hebbar, Gururaja wrote:
> 
> > On Wed, Dec 19, 2012 at 07:43:50, Paul Walmsley wrote:
> > 
> > > We've got a few hwmods on OMAP44xx that don't have clkctrl_offs registers 
> > > listed in the hwmod data, and which are not marked with HWMOD_NO_IDLEST.  
> > > What's going to happen in those cases?
> > 
> > Hmm. This is a special case to me. Let me go back and do some review.
> > Will be back with some more details.
> 
> Probably what needs to happen is that those hwmods need to be marked with 
> HWMOD_NO_IDLEST, in a separate patch before yours.  As far as I can tell, 
> that's how they should have been marked initially.  Then your patch should 
> be fine.

Looking at latest kernel v3.8-rc5, there is separate cm33xx.c file which
handles module ready checking for am33xx platform. So I will update this 
patch to work on this file instead of touching omap4 related file 
(cminst44xx.c)

> 
> - Paul
> 


Regards, 
Gururaja
--
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 Jan. 30, 2013, 4:39 p.m. UTC | #5 
On Wed, 30 Jan 2013, Hebbar, Gururaja wrote:

> On Thu, Dec 20, 2012 at 13:05:27, Paul Walmsley wrote:
> > On Thu, 20 Dec 2012, Hebbar, Gururaja wrote:
> > 
> > > On Wed, Dec 19, 2012 at 07:43:50, Paul Walmsley wrote:
> > > 
> > > > We've got a few hwmods on OMAP44xx that don't have clkctrl_offs registers 
> > > > listed in the hwmod data, and which are not marked with HWMOD_NO_IDLEST.  
> > > > What's going to happen in those cases?
> > > 
> > > Hmm. This is a special case to me. Let me go back and do some review.
> > > Will be back with some more details.
> > 
> > Probably what needs to happen is that those hwmods need to be marked with 
> > HWMOD_NO_IDLEST, in a separate patch before yours.  As far as I can tell, 
> > that's how they should have been marked initially.  Then your patch should 
> > be fine.
> 
> Looking at latest kernel v3.8-rc5, there is separate cm33xx.c file which
> handles module ready checking for am33xx platform. So I will update this 
> patch to work on this file instead of touching omap4 related file 
> (cminst44xx.c)

The same problem still exists.  According to the TRM, there's at least one
AM335x CLKCTRL register that doesn't have IDLEST bits.  So to avoid 
reading reserved bits, either this patch or a preceding patch should
mark those hwmods with HWMOD_NO_IDLEST.


- 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
Hebbar, Gururaja Jan. 31, 2013, 5:25 a.m. UTC | #6 
On Wed, Jan 30, 2013 at 22:09:51, Paul Walmsley wrote:
> On Wed, 30 Jan 2013, Hebbar, Gururaja wrote:
> 
> > On Thu, Dec 20, 2012 at 13:05:27, Paul Walmsley wrote:
> > > On Thu, 20 Dec 2012, Hebbar, Gururaja wrote:
> > > 
> > > > On Wed, Dec 19, 2012 at 07:43:50, Paul Walmsley wrote:
> > > > 
> > > > > We've got a few hwmods on OMAP44xx that don't have clkctrl_offs registers 
> > > > > listed in the hwmod data, and which are not marked with HWMOD_NO_IDLEST.  
> > > > > What's going to happen in those cases?
> > > > 
> > > > Hmm. This is a special case to me. Let me go back and do some review.
> > > > Will be back with some more details.
> > > 
> > > Probably what needs to happen is that those hwmods need to be marked with 
> > > HWMOD_NO_IDLEST, in a separate patch before yours.  As far as I can tell, 
> > > that's how they should have been marked initially.  Then your patch should 
> > > be fine.
> > 
> > Looking at latest kernel v3.8-rc5, there is separate cm33xx.c file which
> > handles module ready checking for am33xx platform. So I will update this 
> > patch to work on this file instead of touching omap4 related file 
> > (cminst44xx.c)
> 
> The same problem still exists.  According to the TRM, there's at least one
> AM335x CLKCTRL register that doesn't have IDLEST bits.  So to avoid 
> reading reserved bits, either this patch or a preceding patch should
> mark those hwmods with HWMOD_NO_IDLEST.

Ok. I will look into this now.

> 
> 
> - Paul
> 


Regards, 
Gururaja
--
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/cminst44xx.c b/arch/arm/mach-omap2/cminst44xx.c
index 7f9a464..40545ff 100644
--- a/arch/arm/mach-omap2/cminst44xx.c
+++ b/arch/arm/mach-omap2/cminst44xx.c
@@ -271,9 +271,6 @@  int omap4_cminst_wait_module_ready(u8 part, u16 inst, s16 cdoffs,
 {
 	int i = 0;
 
-	if (!clkctrl_offs)
-		return 0;
-
 	omap_test_timeout(_is_module_ready(part, inst, cdoffs, clkctrl_offs),
 			  MAX_MODULE_READY_TIME, i);