Re: [PATCH 7/9 v3] OMAP: Hwmod api changes

Re: [PATCH 7/9 v3] OMAP: Hwmod api changes

[Sergei Shtylyov]

Hello.

Hema HK wrote:

> OMAP USBOTG modules has a requirement to set the auto idle bit only after
> setting smart idle bit. Modified the _sys_enable api to set the smart idle
> first and then the autoidle bit. Setting this will not have any impact on the
> other modules.

> Signed-off-by: Hema HK <hemahk-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Basak, Partha <p-basak2-l0cyMroinI0@public.gmane.org>
> Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Cc: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Cc: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> Cc: Cousson, Benoit <b-cousson-l0cyMroinI0@public.gmane.org>
> Cc: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

> Index: linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
> ===================================================================
> --- linux-omap-pm.orig/arch/arm/mach-omap2/omap_hwmod.c
> +++ linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
[...]
> @@ -672,6 +666,19 @@ static void _sysc_enable(struct omap_hwm
>  		_set_clockactivity(oh, oh->class->sysc->clockact, &v);
>  
>  	_write_sysconfig(v, oh);
> +
> +	/*
> +	 * Set the auto idle bit only after setting the smartidle bit
> +	 * as this is requirement for some modules like USBOTG

    Need period at the end of sentense, and the next sentense should satrt with 
capital letter. Sorry for the grammar nitpicking. :-)

> +	 * setting this will not have any impact on the other modues.
> +	 */

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/9 v3] OMAP: Hwmod api changes

[Cousson, Benoit]

Hi Hema,

On 9/23/2010 2:30 AM, Kalliguddi, Hema wrote:
> OMAP USBOTG modules has a requirement to set the auto idle bit only after
> setting smart idle bit. Modified the _sys_enable api to set the smart idle
> first and then the autoidle bit. Setting this will not have any impact on the
> other modules.

I think you should change the subject, because it does not reflect 
accurately the patch.

Just a little bit of nitpicking...
For me, an API change is a change in the signature of the API, not a 
change inside an API. Otherwise, since 99% of the code is inside a 
function, we can call most patches like that...
Moreover I don't think _sysc_enable can be considered as an API since it 
is a pure static helper function not exported to the outside.

Something like that seems more accurate for my point of view:

OMAP: hwmod: Set autoidle after smartidle during _sysc_enable

>
> Signed-off-by: Hema HK<hemahk@ti.com>
> Signed-off-by: Basak, Partha<p-basak2@ti.com>
> Cc: Felipe Balbi<balbi@ti.com>
> Cc: Tony Lindgren<tony@atomide.com>
> Cc: Kevin Hilman<khilman@deeprootsystems.com>
> Cc: Cousson, Benoit<b-cousson@ti.com>
> Cc: Paul Walmsley<paul@pwsan.com>
> ---
>   arch/arm/mach-omap2/omap_hwmod.c |   18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> Index: linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
> ===================================================================
> --- linux-omap-pm.orig/arch/arm/mach-omap2/omap_hwmod.c
> +++ linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
> @@ -654,12 +654,6 @@ static void _sysc_enable(struct omap_hwm
>   		_set_master_standbymode(oh, idlemode,&v);
>   	}
>
> -	if (sf&  SYSC_HAS_AUTOIDLE) {
> -		idlemode = (oh->flags&  HWMOD_NO_OCP_AUTOIDLE) ?
> -			0 : 1;
> -		_set_module_autoidle(oh, idlemode,&v);
> -	}
> -
>   	/* XXX OCP ENAWAKEUP bit? */
>
>   	/*
> @@ -672,6 +666,19 @@ static void _sysc_enable(struct omap_hwm
>   		_set_clockactivity(oh, oh->class->sysc->clockact,&v);
>
>   	_write_sysconfig(v, oh);
> +
> +	/*
> +	 * Set the auto idle bit only after setting the smartidle bit

Typo: autoidle without space

> +	 * as this is requirement for some modules like USBOTG

The new official name is usb_otg_hs.

> +	 * setting this will not have any impact on the other modues.

Typo: modules

> +	 */
> +

You can remove that blank line.

> +	if (sf&  SYSC_HAS_AUTOIDLE) {
> +		idlemode = (oh->flags&  HWMOD_NO_OCP_AUTOIDLE) ?
> +			0 : 1;
> +		_set_module_autoidle(oh, idlemode,&v);
> +	}
> +	_write_sysconfig(v, oh);

You should move that inside the if, it will save an useless call in case 
SYSC_HAS_AUTOIDLE is not set.

Beside these minors comments this patch looks fine to me.
Acked-by: Benoit Cousson <b-cousson@ti.com>

I had some concern about the extra overhead added for every IPs that 
does not need that at all, meaning every IPs beside the usb_otg_hs...

But then I realized that autoidle is anyway enabled by default, and 
since the sysconfig value is cached, it should not generate any write 
access to most IPs that will not try to disable the autoidle.

Regards,
Benoit

Re: [PATCH 7/9 v3] OMAP: Hwmod api changes

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 480 bytes --]

Hello Hema,

On Wed, 22 Sep 2010, Hema HK wrote:

> OMAP USBOTG modules has a requirement to set the auto idle bit only after
> setting smart idle bit. Modified the _sys_enable api to set the smart idle
> first and then the autoidle bit. Setting this will not have any impact on the
> other modules.

Please incorporate the comments from Benoît and Sergei and post this patch 
separately from your other MUSB patches, and we'll try to get it in for 
2.6.37.


- Paul

[PATCH 7/9 v3] OMAP: Hwmod api changes

[Hema HK]

OMAP USBOTG modules has a requirement to set the auto idle bit only after
setting smart idle bit. Modified the _sys_enable api to set the smart idle
first and then the autoidle bit. Setting this will not have any impact on the
other modules.

Signed-off-by: Hema HK <hemahk@ti.com>
Signed-off-by: Basak, Partha <p-basak2@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Cousson, Benoit <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/omap_hwmod.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Index: linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
===================================================================
--- linux-omap-pm.orig/arch/arm/mach-omap2/omap_hwmod.c
+++ linux-omap-pm/arch/arm/mach-omap2/omap_hwmod.c
@@ -654,12 +654,6 @@ static void _sysc_enable(struct omap_hwm
 		_set_master_standbymode(oh, idlemode, &v);
 	}
 
-	if (sf & SYSC_HAS_AUTOIDLE) {
-		idlemode = (oh->flags & HWMOD_NO_OCP_AUTOIDLE) ?
-			0 : 1;
-		_set_module_autoidle(oh, idlemode, &v);
-	}
-
 	/* XXX OCP ENAWAKEUP bit? */
 
 	/*
@@ -672,6 +666,19 @@ static void _sysc_enable(struct omap_hwm
 		_set_clockactivity(oh, oh->class->sysc->clockact, &v);
 
 	_write_sysconfig(v, oh);
+
+	/*
+	 * Set the auto idle bit only after setting the smartidle bit
+	 * as this is requirement for some modules like USBOTG
+	 * setting this will not have any impact on the other modues.
+	 */
+
+	if (sf & SYSC_HAS_AUTOIDLE) {
+		idlemode = (oh->flags & HWMOD_NO_OCP_AUTOIDLE) ?
+			0 : 1;
+		_set_module_autoidle(oh, idlemode, &v);
+	}
+	_write_sysconfig(v, oh);
 }
 
 /**

Re: [PATCH 7/9 v3] OMAP: Hwmod api changes

[Felipe Balbi]

On Wed, Sep 22, 2010 at 03:43:14PM -0500, Cousson, Benoit wrote:
>Hi Hema,
>
>On 9/23/2010 2:30 AM, Kalliguddi, Hema wrote:
>> OMAP USBOTG modules has a requirement to set the auto idle bit only after
>> setting smart idle bit. Modified the _sys_enable api to set the smart idle
>> first and then the autoidle bit. Setting this will not have any impact on the
>> other modules.
>
>I think you should change the subject, because it does not reflect
>accurately the patch.
>
>Just a little bit of nitpicking...
>For me, an API change is a change in the signature of the API, not a
>change inside an API. Otherwise, since 99% of the code is inside a
>function, we can call most patches like that...
>Moreover I don't think _sysc_enable can be considered as an API since it
>is a pure static helper function not exported to the outside.
>
>Something like that seems more accurate for my point of view:
>
>OMAP: hwmod: Set autoidle after smartidle during _sysc_enable

+1

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 7/9 v3] OMAP: Hwmod api changes

[Kalliguddi, Hema]

Hi, 

>-----Original Message-----
>From: Balbi, Felipe 
>Sent: Thursday, September 23, 2010 11:53 AM
>To: Cousson, Benoit
>Cc: Kalliguddi, Hema; linux-omap@vger.kernel.org; 
>linux-usb@vger.kernel.org; Basak, Partha; Balbi, Felipe; Tony 
>Lindgren; Kevin Hilman; Paul Walmsley
>Subject: Re: [PATCH 7/9 v3] OMAP: Hwmod api changes
>
>On Wed, Sep 22, 2010 at 03:43:14PM -0500, Cousson, Benoit wrote:
>>Hi Hema,
>>
>>On 9/23/2010 2:30 AM, Kalliguddi, Hema wrote:
>>> OMAP USBOTG modules has a requirement to set the auto idle 
>bit only after
>>> setting smart idle bit. Modified the _sys_enable api to set 
>the smart idle
>>> first and then the autoidle bit. Setting this will not have 
>any impact on the
>>> other modules.
>>
>>I think you should change the subject, because it does not reflect
>>accurately the patch.
>>
>>Just a little bit of nitpicking...
>>For me, an API change is a change in the signature of the API, not a
>>change inside an API. Otherwise, since 99% of the code is inside a
>>function, we can call most patches like that...
>>Moreover I don't think _sysc_enable can be considered as an 
>API since it
>>is a pure static helper function not exported to the outside.
>>
>>Something like that seems more accurate for my point of view:
>>
>>OMAP: hwmod: Set autoidle after smartidle during _sysc_enable
>
>+1
>
I Agree. I will do the changes accordingly and post the patch.
>-- 
>balbi
>