[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Rajendra Nayak]

Some functions like _omap4_disable_module() and _omap4_wait_target_disable()
are (will be) used on all OMAPs OMAP4 and beyond which support module level
control. Fix the error checks in these functions to return if called on
any platform pre OMAP4 (i.e OMAP2 and OMAP3) instead of checking for
!cpu_is_omap44xx(). This avoids having to update the error check with a
'&& !cpu_is_omap54xx()' when OMAP5 is introduced and possibly similar updates
when further OMAP generations are added.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 8ac26f2..f2a9afa 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -808,7 +808,7 @@ static void _enable_module(struct omap_hwmod *oh)
  */
 static int _omap4_wait_target_disable(struct omap_hwmod *oh)
 {
-	if (!cpu_is_omap44xx())
+	if (cpu_is_omap24xx() || cpu_is_omap34xx())
 		return 0;
 
 	if (!oh)
@@ -838,7 +838,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
 	int v;
 
 	/* The module mode does not exist prior OMAP4 */
-	if (!cpu_is_omap44xx())
+	if (cpu_is_omap24xx() || cpu_is_omap34xx())
 		return -EINVAL;
 
 	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
-- 
1.7.1

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Jon Hunter]

Hi Rajendra,

On 3/27/2012 4:58, Rajendra Nayak wrote:
> Some functions like _omap4_disable_module() and _omap4_wait_target_disable()
> are (will be) used on all OMAPs OMAP4 and beyond which support module level
> control. Fix the error checks in these functions to return if called on
> any platform pre OMAP4 (i.e OMAP2 and OMAP3) instead of checking for
> !cpu_is_omap44xx(). This avoids having to update the error check with a
> '&&  !cpu_is_omap54xx()' when OMAP5 is introduced and possibly similar updates
> when further OMAP generations are added.
>
> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |    4 ++--
>   1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 8ac26f2..f2a9afa 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -808,7 +808,7 @@ static void _enable_module(struct omap_hwmod *oh)
>    */
>   static int _omap4_wait_target_disable(struct omap_hwmod *oh)
>   {
> -	if (!cpu_is_omap44xx())
> +	if (cpu_is_omap24xx() || cpu_is_omap34xx())
>   		return 0;

What about omap36xx?

>   	if (!oh)
> @@ -838,7 +838,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
>   	int v;
>
>   	/* The module mode does not exist prior OMAP4 */
> -	if (!cpu_is_omap44xx())
> +	if (cpu_is_omap24xx() || cpu_is_omap34xx())
>   		return -EINVAL;

Same here.

>
>   	if (!oh->clkdm || !oh->prcm.omap4.modulemode)

Cheers
Jon

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Paul Walmsley]

Hi Jon,

On Tue, 27 Mar 2012, Jon Hunter wrote:

> On 3/27/2012 4:58, Rajendra Nayak wrote:
>
> > diff --git a/arch/arm/mach-omap2/omap_hwmod.c
> > b/arch/arm/mach-omap2/omap_hwmod.c
> > index 8ac26f2..f2a9afa 100644
> > --- a/arch/arm/mach-omap2/omap_hwmod.c
> > +++ b/arch/arm/mach-omap2/omap_hwmod.c
> > @@ -808,7 +808,7 @@ static void _enable_module(struct omap_hwmod *oh)
> >    */
> >   static int _omap4_wait_target_disable(struct omap_hwmod *oh)
> >   {
> > -	if (!cpu_is_omap44xx())
> > +	if (cpu_is_omap24xx() || cpu_is_omap34xx())
> >   		return 0;
> 
> What about omap36xx?

Unfortunately, cpu_is_omap34xx() also covers OMAP36xx :-(

- Paul

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Hiremath, Vaibhav]

On Tue, Mar 27, 2012 at 15:28:31, Nayak, Rajendra wrote:
> Some functions like _omap4_disable_module() and _omap4_wait_target_disable()
> are (will be) used on all OMAPs OMAP4 and beyond which support module level
> control. Fix the error checks in these functions to return if called on
> any platform pre OMAP4 (i.e OMAP2 and OMAP3) instead of checking for
> !cpu_is_omap44xx(). This avoids having to update the error check with a
> '&& !cpu_is_omap54xx()' when OMAP5 is introduced and possibly similar updates
> when further OMAP generations are added.
> 

Let me add some flavor here :)

AM33xx, which has module level control, but falls under OMAP3 family of 
devices. cpu_is_omap34xx() is true for AM33xx device and we have to add 
check in all these functions. And I am sure we will have many of such 
devices in the future.

Can we use some flag based option here, instead of cpu_is_xxx() check?

Thanks,
Vaibhav

> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 8ac26f2..f2a9afa 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -808,7 +808,7 @@ static void _enable_module(struct omap_hwmod *oh)
>   */
>  static int _omap4_wait_target_disable(struct omap_hwmod *oh)
>  {
> -	if (!cpu_is_omap44xx())
> +	if (cpu_is_omap24xx() || cpu_is_omap34xx())
>  		return 0;
>  
>  	if (!oh)
> @@ -838,7 +838,7 @@ static int _omap4_disable_module(struct omap_hwmod *oh)
>  	int v;
>  
>  	/* The module mode does not exist prior OMAP4 */
> -	if (!cpu_is_omap44xx())
> +	if (cpu_is_omap24xx() || cpu_is_omap34xx())
>  		return -EINVAL;
>  
>  	if (!oh->clkdm || !oh->prcm.omap4.modulemode)
> -- 
> 1.7.1
> 
> --
> 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
> 

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Jon Hunter]

Hi Paul,

On 3/27/2012 21:39, Paul Walmsley wrote:
> Hi Jon,
>
> On Tue, 27 Mar 2012, Jon Hunter wrote:
>
>> On 3/27/2012 4:58, Rajendra Nayak wrote:
>>
>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c
>>> b/arch/arm/mach-omap2/omap_hwmod.c
>>> index 8ac26f2..f2a9afa 100644
>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>> @@ -808,7 +808,7 @@ static void _enable_module(struct omap_hwmod *oh)
>>>     */
>>>    static int _omap4_wait_target_disable(struct omap_hwmod *oh)
>>>    {
>>> -	if (!cpu_is_omap44xx())
>>> +	if (cpu_is_omap24xx() || cpu_is_omap34xx())
>>>    		return 0;
>>
>> What about omap36xx?
>
> Unfortunately, cpu_is_omap34xx() also covers OMAP36xx :-(

Thanks. Is that still the case when MULTI_OMAP2 is defined? I can see if 
it is not define then it will always return 1 for all OMAP3, but for 
MULTI_OMAP2 it did not seem to me that it would. May be I should test ...

Cheers
Jon

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Rajendra Nayak]

On Wednesday 28 March 2012 12:02 PM, Hiremath, Vaibhav wrote:
> On Tue, Mar 27, 2012 at 15:28:31, Nayak, Rajendra wrote:
>> Some functions like _omap4_disable_module() and _omap4_wait_target_disable()
>> are (will be) used on all OMAPs OMAP4 and beyond which support module level
>> control. Fix the error checks in these functions to return if called on
>> any platform pre OMAP4 (i.e OMAP2 and OMAP3) instead of checking for
>> !cpu_is_omap44xx(). This avoids having to update the error check with a
>> '&&  !cpu_is_omap54xx()' when OMAP5 is introduced and possibly similar updates
>> when further OMAP generations are added.
>>
>
> Let me add some flavor here :)
>
> AM33xx, which has module level control, but falls under OMAP3 family of
> devices. cpu_is_omap34xx() is true for AM33xx device and we have to add
> check in all these functions. And I am sure we will have many of such
> devices in the future.
>
> Can we use some flag based option here, instead of cpu_is_xxx() check?
>

The intent of this patch was to make the error handling uniform across
all modules control functions in hwmod, and it atleast addresses one
problem of having to update these checks every time a new OMAP gets
added.

The problem that you bring up with AM33xx is regardless of this patch
(you would still need to go update every !cpu_is_omap44xx() check)
and IMHO should be handled separately, with a flag like you are
suggesting or by some other means. If you already have patches on how
this can be done, you should go ahead and post them out.

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Hiremath, Vaibhav]

On Thu, Mar 29, 2012 at 11:42:34, Nayak, Rajendra wrote:
> On Wednesday 28 March 2012 12:02 PM, Hiremath, Vaibhav wrote:
> > On Tue, Mar 27, 2012 at 15:28:31, Nayak, Rajendra wrote:
> >> Some functions like _omap4_disable_module() and _omap4_wait_target_disable()
> >> are (will be) used on all OMAPs OMAP4 and beyond which support module level
> >> control. Fix the error checks in these functions to return if called on
> >> any platform pre OMAP4 (i.e OMAP2 and OMAP3) instead of checking for
> >> !cpu_is_omap44xx(). This avoids having to update the error check with a
> >> '&&  !cpu_is_omap54xx()' when OMAP5 is introduced and possibly similar updates
> >> when further OMAP generations are added.
> >>
> >
> > Let me add some flavor here :)
> >
> > AM33xx, which has module level control, but falls under OMAP3 family of
> > devices. cpu_is_omap34xx() is true for AM33xx device and we have to add
> > check in all these functions. And I am sure we will have many of such
> > devices in the future.
> >
> > Can we use some flag based option here, instead of cpu_is_xxx() check?
> >
> 
> The intent of this patch was to make the error handling uniform across
> all modules control functions in hwmod, and it atleast addresses one
> problem of having to update these checks every time a new OMAP gets
> added.
> 
> The problem that you bring up with AM33xx is regardless of this patch
> (you would still need to go update every !cpu_is_omap44xx() check)

Indeed, in any of cpu_is_xxx() check implementation, I have to add check
for cpu_is_am33xx().

The point I was trying to make here was, cpu_is_xxx() check will become ugly,
as more and more devices gets added to the list, am33xx being the first one 
in omap34xx family.

> and IMHO should be handled separately, with a flag like you are
> suggesting or by some other means. If you already have patches on how
> this can be done, you should go ahead and post them out.
> 

Nope.

But I can work on it, once I finish my AM33xx baseport submission.

Thanks,
Vaibhav

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Rajendra Nayak]

On Thursday 29 March 2012 02:26 PM, Hiremath, Vaibhav wrote:
> The point I was trying to make here was, cpu_is_xxx() check will become ugly,
> as more and more devices gets added to the list, am33xx being the first one
> in omap34xx family.

So are there more, other than AM33xx which are cortex A8 based but
share the OMAP4 PRCM IP block?

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Hiremath, Vaibhav]

On Thu, Mar 29, 2012 at 14:32:34, Nayak, Rajendra wrote:
> On Thursday 29 March 2012 02:26 PM, Hiremath, Vaibhav wrote:
> > The point I was trying to make here was, cpu_is_xxx() check will become ugly,
> > as more and more devices gets added to the list, am33xx being the first one
> > in omap34xx family.
> 
> So are there more, other than AM33xx which are cortex A8 based but
> share the OMAP4 PRCM IP block?
> 

I would say, probably yes. 

But, things are so dynamic and you never know how things will shape up by 
the time you freeze the design.

Thanks,
Vaibhav

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Jon Hunter]

Hi Viabhav,

On 3/29/2012 4:14, Hiremath, Vaibhav wrote:
> On Thu, Mar 29, 2012 at 14:32:34, Nayak, Rajendra wrote:
>> On Thursday 29 March 2012 02:26 PM, Hiremath, Vaibhav wrote:
>>> The point I was trying to make here was, cpu_is_xxx() check will become ugly,
>>> as more and more devices gets added to the list, am33xx being the first one
>>> in omap34xx family.
>>
>> So are there more, other than AM33xx which are cortex A8 based but
>> share the OMAP4 PRCM IP block?
>>
>
> I would say, probably yes.
>
> But, things are so dynamic and you never know how things will shape up by
> the time you freeze the design.

So this begs the question, why does AM33xx return true from 
cpu_is_omap34xx() is the architecture is based upon OMAP4 and not OMAP3?

I understand it is a Cortex-A8 versus Corex-A9, but if the architecture 
is closer to OMAP4, then should it not be classed as OMAP4 and not OMAP3?

Raises another question if we should really have arch_is_omapXXXX and 
not cpu_is_omapXXXX.

Cheers
Jon

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Jon Hunter]

Hi Viabhav,

On 3/29/2012 3:56, Hiremath, Vaibhav wrote:
> On Thu, Mar 29, 2012 at 11:42:34, Nayak, Rajendra wrote:
>> On Wednesday 28 March 2012 12:02 PM, Hiremath, Vaibhav wrote:
>>> On Tue, Mar 27, 2012 at 15:28:31, Nayak, Rajendra wrote:
>>>> Some functions like _omap4_disable_module() and _omap4_wait_target_disable()
>>>> are (will be) used on all OMAPs OMAP4 and beyond which support module level
>>>> control. Fix the error checks in these functions to return if called on
>>>> any platform pre OMAP4 (i.e OMAP2 and OMAP3) instead of checking for
>>>> !cpu_is_omap44xx(). This avoids having to update the error check with a
>>>> '&&   !cpu_is_omap54xx()' when OMAP5 is introduced and possibly similar updates
>>>> when further OMAP generations are added.
>>>>
>>>
>>> Let me add some flavor here :)
>>>
>>> AM33xx, which has module level control, but falls under OMAP3 family of
>>> devices. cpu_is_omap34xx() is true for AM33xx device and we have to add
>>> check in all these functions. And I am sure we will have many of such
>>> devices in the future.
>>>
>>> Can we use some flag based option here, instead of cpu_is_xxx() check?
>>>
>>
>> The intent of this patch was to make the error handling uniform across
>> all modules control functions in hwmod, and it atleast addresses one
>> problem of having to update these checks every time a new OMAP gets
>> added.
>>
>> The problem that you bring up with AM33xx is regardless of this patch
>> (you would still need to go update every !cpu_is_omap44xx() check)
>
> Indeed, in any of cpu_is_xxx() check implementation, I have to add check
> for cpu_is_am33xx().

I hope we can avoid adding more cpu_is_amXXXX. That should be a last resort.

Jon

[PATCH] ARM: OMAP: hwmod: Fix error handling in functions used OMAP4 onwards

[Hiremath, Vaibhav]

On Thu, Mar 29, 2012 at 20:33:17, Hunter, Jon wrote:
> Hi Viabhav,
> 
> On 3/29/2012 3:56, Hiremath, Vaibhav wrote:
> > On Thu, Mar 29, 2012 at 11:42:34, Nayak, Rajendra wrote:
> >> On Wednesday 28 March 2012 12:02 PM, Hiremath, Vaibhav wrote:
> >>> On Tue, Mar 27, 2012 at 15:28:31, Nayak, Rajendra wrote:
> >>>> Some functions like _omap4_disable_module() and _omap4_wait_target_disable()
> >>>> are (will be) used on all OMAPs OMAP4 and beyond which support module level
> >>>> control. Fix the error checks in these functions to return if called on
> >>>> any platform pre OMAP4 (i.e OMAP2 and OMAP3) instead of checking for
> >>>> !cpu_is_omap44xx(). This avoids having to update the error check with a
> >>>> '&&   !cpu_is_omap54xx()' when OMAP5 is introduced and possibly similar updates
> >>>> when further OMAP generations are added.
> >>>>
> >>>
> >>> Let me add some flavor here :)
> >>>
> >>> AM33xx, which has module level control, but falls under OMAP3 family of
> >>> devices. cpu_is_omap34xx() is true for AM33xx device and we have to add
> >>> check in all these functions. And I am sure we will have many of such
> >>> devices in the future.
> >>>
> >>> Can we use some flag based option here, instead of cpu_is_xxx() check?
> >>>
> >>
> >> The intent of this patch was to make the error handling uniform across
> >> all modules control functions in hwmod, and it atleast addresses one
> >> problem of having to update these checks every time a new OMAP gets
> >> added.
> >>
> >> The problem that you bring up with AM33xx is regardless of this patch
> >> (you would still need to go update every !cpu_is_omap44xx() check)
> >
> > Indeed, in any of cpu_is_xxx() check implementation, I have to add check
> > for cpu_is_am33xx().
> 
> I hope we can avoid adding more cpu_is_amXXXX. That should be a last resort.
> 

Yes, indeed; adding cpu_is_xxx check would be my last option.

Thanks,
Vaibhav

> Jon
>