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@xxxxxx>
Signed-off-by: Basak, Partha<p-basak2@xxxxxx>
Cc: Felipe Balbi<balbi@xxxxxx>
Cc: Tony Lindgren<tony@xxxxxxxxxxx>
Cc: Kevin Hilman<khilman@xxxxxxxxxxxxxxxxxxx>
Cc: Cousson, Benoit<b-cousson@xxxxxx>
Cc: Paul Walmsley<paul@xxxxxxxxx>
---
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@xxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
