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

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

[Hebbar Gururaja]

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(-)

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);
 
-- 
1.7.9.5

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

[Felipe Balbi]

On Tue, Dec 18, 2012 at 06:26:18PM +0530, 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.
> 
> 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>

Reviewed-by: Felipe Balbi <balbi@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(-)
> 
> 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);
>  
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121218/415e2a5c/attachment.sig>

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

[Paul Walmsley]

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

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

[Hebbar, Gururaja]

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

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

[Paul Walmsley]

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

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

[Hebbar, Gururaja]

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

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

[Paul Walmsley]

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

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

[Hebbar, Gururaja]

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