diff mbox

[v2] OMAP3: PM: reset USB OTG module on boot

Message ID 1245857533-2419-1-git-send-email-khilman@deeprootsystems.com (mailing list archive)
State Superseded
Delegated to: Kevin Hilman
Headers show

Commit Message

Kevin Hilman June 24, 2009, 3:32 p.m. UTC 
Rather than simply setting force-idle mode on boot, do a reset of the
OTG module.  This really ensures that any bootloader/bootstrap code
that leaves it active will not prevent future retention.  After reset,
OTG module will be in force-idle, force-standby mode.

In addition, ensure that the iclk is enabled before attempting a write
to the module SYSCONFIG register.

Problem reported by Mike Chan <mikechan@google.com>

Tested-by: Mike Chan <mikechan@google.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
Updates from v1
- use ioremap()+__raw_write instead of deprecated omap_write()
- Re: defines in omap2430.h, this header is not exported outside
  drivers/usb, so define it locally yere

 arch/arm/mach-omap2/usb-musb.c |   31 +++++++++++++++++++++++++++----
 1 files changed, 27 insertions(+), 4 deletions(-)

Comments

vikram pandita June 24, 2009, 5:02 p.m. UTC | #1 
>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin
>Hilman
>Sent: Wednesday, June 24, 2009 10:32 AM
>To: linux-omap@vger.kernel.org
>Subject: [PATCH v2] OMAP3: PM: reset USB OTG module on boot
>
>Rather than simply setting force-idle mode on boot, do a reset of the
>OTG module.  This really ensures that any bootloader/bootstrap code
>that leaves it active will not prevent future retention.  After reset,
>OTG module will be in force-idle, force-standby mode.
>
>In addition, ensure that the iclk is enabled before attempting a write
>to the module SYSCONFIG register.

This patch solves MUSB issue, but is not generic for all IP blocks on omap.

ROM code or u-boot could leave the module sysconfig in a non-idle state.

I recommend just like CONFIG_OMAP_RESET_CLOCKS for clocks, same could be done generically for all SYSCONFIG registers in some prcm code and not driver specific.

>
>Problem reported by Mike Chan <mikechan@google.com>
>
>Tested-by: Mike Chan <mikechan@google.com>
>Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>---
>Updates from v1
>- use ioremap()+__raw_write instead of deprecated omap_write()
>- Re: defines in omap2430.h, this header is not exported outside
>  drivers/usb, so define it locally yere
>
> arch/arm/mach-omap2/usb-musb.c |   31 +++++++++++++++++++++++++++----
> 1 files changed, 27 insertions(+), 4 deletions(-)
>
>diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
>index d85296d..31adbe6 100644
>--- a/arch/arm/mach-omap2/usb-musb.c
>+++ b/arch/arm/mach-omap2/usb-musb.c
>@@ -26,18 +26,41 @@
>
> #include <linux/usb/musb.h>
>
>+#include <asm/sizes.h>
>+
> #include <mach/hardware.h>
> #include <mach/irqs.h>
> #include <mach/mux.h>
> #include <mach/usb.h>
>
>-#define OTG_SYSCONFIG	(OMAP34XX_HSUSB_OTG_BASE + 0x404)
>+#define OTG_SYSCONFIG	   0x404
>+#define OTG_SYSC_SOFTRESET BIT(1)

This is already defined in drivers/usb/musb/omap2430.h
Could we reuse? 

#if     defined(CONFIG_ARCH_OMAP2430)
#define OMAP_HSOTG_BASE         (OMAP243X_HS_BASE)
#elif   defined(CONFIG_ARCH_OMAP3430)
#define OMAP_HSOTG_BASE         (OMAP34XX_HSUSB_OTG_BASE)
#endif
#define OMAP_HSOTG(offset)      (OMAP_HSOTG_BASE + 0x400 + (offset))
#define OTG_REVISION            OMAP_HSOTG(0x0)
#define OTG_SYSCONFIG           OMAP_HSOTG(0x4)


>
> static void __init usb_musb_pm_init(void)
> {
>-	/* Ensure force-idle mode for OTG controller */
>-	if (cpu_is_omap34xx())
>-		omap_writel(0, OTG_SYSCONFIG);
>+	struct clk *iclk;
>+	void __iomem *otg_base;
>+
>+	if (!cpu_is_omap34xx())
>+		return;

Why is this sys-config reset done only for omap34xx()
This should be valid for any old 24xx() as well as new 44xx() omaps

>+
>+	otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K);
>+	if (WARN_ON(!otg_base))
>+		return;
>+
>+	iclk = clk_get(NULL, "hsotgusb_ick");
>+	if (WARN_ON(!iclk))
>+		return;
>+
>+	clk_enable(iclk);
>+
>+	/* Reset OTG controller.  After reset, it will be in
>+	 * force-idle, force-standby mode. */
>+	__raw_writel(OTG_SYSC_SOFTRESET, otg_base + OTG_SYSCONFIG);
>+
>+	clk_disable(iclk);
>+	clk_put(iclk);
>+	iounmap(otg_base);
> }
>
> #ifdef CONFIG_USB_MUSB_SOC
>--
>1.6.3.2
>
>--
>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

--
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
Kevin Hilman June 24, 2009, 5:38 p.m. UTC | #2 
"Pandita, Vikram" <vikram.pandita@ti.com> writes:

>>
>>Rather than simply setting force-idle mode on boot, do a reset of the
>>OTG module.  This really ensures that any bootloader/bootstrap code
>>that leaves it active will not prevent future retention.  After reset,
>>OTG module will be in force-idle, force-standby mode.
>>
>>In addition, ensure that the iclk is enabled before attempting a write
>>to the module SYSCONFIG register.
>
> This patch solves MUSB issue, but is not generic for all IP blocks on omap.
>
> ROM code or u-boot could leave the module sysconfig in a non-idle state.
>
> I recommend just like CONFIG_OMAP_RESET_CLOCKS for clocks, same
> could be done generically for all SYSCONFIG registers in some prcm
> code and not driver specific.

Completely agree.

This feature will come as soon as we have omap_hwmod/omapdev in place.
With that, we will have an easy way to model the SYSCONFIG regs of
each modules, and iterate through them.

Kevin


>>
>>Problem reported by Mike Chan <mikechan@google.com>
>>
>>Tested-by: Mike Chan <mikechan@google.com>
>>Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>>---
>>Updates from v1
>>- use ioremap()+__raw_write instead of deprecated omap_write()
>>- Re: defines in omap2430.h, this header is not exported outside
>>  drivers/usb, so define it locally yere
>>
>> arch/arm/mach-omap2/usb-musb.c |   31 +++++++++++++++++++++++++++----
>> 1 files changed, 27 insertions(+), 4 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
>>index d85296d..31adbe6 100644
>>--- a/arch/arm/mach-omap2/usb-musb.c
>>+++ b/arch/arm/mach-omap2/usb-musb.c
>>@@ -26,18 +26,41 @@
>>
>> #include <linux/usb/musb.h>
>>
>>+#include <asm/sizes.h>
>>+
>> #include <mach/hardware.h>
>> #include <mach/irqs.h>
>> #include <mach/mux.h>
>> #include <mach/usb.h>
>>
>>-#define OTG_SYSCONFIG	(OMAP34XX_HSUSB_OTG_BASE + 0x404)
>>+#define OTG_SYSCONFIG	   0x404
>>+#define OTG_SYSC_SOFTRESET BIT(1)
>
> This is already defined in drivers/usb/musb/omap2430.h
> Could we reuse? 
>
> #if     defined(CONFIG_ARCH_OMAP2430)
> #define OMAP_HSOTG_BASE         (OMAP243X_HS_BASE)
> #elif   defined(CONFIG_ARCH_OMAP3430)
> #define OMAP_HSOTG_BASE         (OMAP34XX_HSUSB_OTG_BASE)
> #endif
> #define OMAP_HSOTG(offset)      (OMAP_HSOTG_BASE + 0x400 + (offset))
> #define OTG_REVISION            OMAP_HSOTG(0x0)
> #define OTG_SYSCONFIG           OMAP_HSOTG(0x4)
>
>
>>
>> static void __init usb_musb_pm_init(void)
>> {
>>-	/* Ensure force-idle mode for OTG controller */
>>-	if (cpu_is_omap34xx())
>>-		omap_writel(0, OTG_SYSCONFIG);
>>+	struct clk *iclk;
>>+	void __iomem *otg_base;
>>+
>>+	if (!cpu_is_omap34xx())
>>+		return;
>
> Why is this sys-config reset done only for omap34xx()
> This should be valid for any old 24xx() as well as new 44xx() omaps
>
>>+
>>+	otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K);
>>+	if (WARN_ON(!otg_base))
>>+		return;
>>+
>>+	iclk = clk_get(NULL, "hsotgusb_ick");
>>+	if (WARN_ON(!iclk))
>>+		return;
>>+
>>+	clk_enable(iclk);
>>+
>>+	/* Reset OTG controller.  After reset, it will be in
>>+	 * force-idle, force-standby mode. */
>>+	__raw_writel(OTG_SYSC_SOFTRESET, otg_base + OTG_SYSCONFIG);
>>+
>>+	clk_disable(iclk);
>>+	clk_put(iclk);
>>+	iounmap(otg_base);
>> }
>>
>> #ifdef CONFIG_USB_MUSB_SOC
>>--
>>1.6.3.2
>>
>>--
>>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
--
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
Kevin Hilman June 24, 2009, 6 p.m. UTC | #3 
"Pandita, Vikram" <vikram.pandita@ti.com> writes:

>>-----Original Message-----
>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin
>>Hilman
>>Sent: Wednesday, June 24, 2009 10:32 AM
>>To: linux-omap@vger.kernel.org
>>Subject: [PATCH v2] OMAP3: PM: reset USB OTG module on boot
>>
>>Rather than simply setting force-idle mode on boot, do a reset of the
>>OTG module.  This really ensures that any bootloader/bootstrap code
>>that leaves it active will not prevent future retention.  After reset,
>>OTG module will be in force-idle, force-standby mode.
>>
>>In addition, ensure that the iclk is enabled before attempting a write
>>to the module SYSCONFIG register.
>
> This patch solves MUSB issue, but is not generic for all IP blocks on omap.
>
> ROM code or u-boot could leave the module sysconfig in a non-idle state.
>
> I recommend just like CONFIG_OMAP_RESET_CLOCKS for clocks, same could be done generically for all SYSCONFIG registers in some prcm code and not driver specific.
>
>>
>>Problem reported by Mike Chan <mikechan@google.com>
>>
>>Tested-by: Mike Chan <mikechan@google.com>
>>Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>>---
>>Updates from v1
>>- use ioremap()+__raw_write instead of deprecated omap_write()
>>- Re: defines in omap2430.h, this header is not exported outside
>>  drivers/usb, so define it locally yere
>>
>> arch/arm/mach-omap2/usb-musb.c |   31 +++++++++++++++++++++++++++----
>> 1 files changed, 27 insertions(+), 4 deletions(-)
>>
>>diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
>>index d85296d..31adbe6 100644
>>--- a/arch/arm/mach-omap2/usb-musb.c
>>+++ b/arch/arm/mach-omap2/usb-musb.c
>>@@ -26,18 +26,41 @@
>>
>> #include <linux/usb/musb.h>
>>
>>+#include <asm/sizes.h>
>>+
>> #include <mach/hardware.h>
>> #include <mach/irqs.h>
>> #include <mach/mux.h>
>> #include <mach/usb.h>
>>
>>-#define OTG_SYSCONFIG	(OMAP34XX_HSUSB_OTG_BASE + 0x404)
>>+#define OTG_SYSCONFIG	   0x404
>>+#define OTG_SYSC_SOFTRESET BIT(1)
>
> This is already defined in drivers/usb/musb/omap2430.h
> Could we reuse? 

See comment above in the 'Updates from v1' section.

>
> #if     defined(CONFIG_ARCH_OMAP2430)
> #define OMAP_HSOTG_BASE         (OMAP243X_HS_BASE)
> #elif   defined(CONFIG_ARCH_OMAP3430)
> #define OMAP_HSOTG_BASE         (OMAP34XX_HSUSB_OTG_BASE)
> #endif
> #define OMAP_HSOTG(offset)      (OMAP_HSOTG_BASE + 0x400 + (offset))
> #define OTG_REVISION            OMAP_HSOTG(0x0)
> #define OTG_SYSCONFIG           OMAP_HSOTG(0x4)
>
>
>>
>> static void __init usb_musb_pm_init(void)
>> {
>>-	/* Ensure force-idle mode for OTG controller */
>>-	if (cpu_is_omap34xx())
>>-		omap_writel(0, OTG_SYSCONFIG);
>>+	struct clk *iclk;
>>+	void __iomem *otg_base;
>>+
>>+	if (!cpu_is_omap34xx())
>>+		return;
>
> Why is this sys-config reset done only for omap34xx()
> This should be valid for any old 24xx() as well as new 44xx() omaps

You're correct.  Good catch.

The force-idle "fix" that this is replacing was only being done on
34xx due to the smart-idle errata.  However, this reset should be
done even without any errata.

Thanks for review,  will respin.

Kevin

>>+
>>+	otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K);
>>+	if (WARN_ON(!otg_base))
>>+		return;
>>+
>>+	iclk = clk_get(NULL, "hsotgusb_ick");
>>+	if (WARN_ON(!iclk))
>>+		return;
>>+
>>+	clk_enable(iclk);
>>+
>>+	/* Reset OTG controller.  After reset, it will be in
>>+	 * force-idle, force-standby mode. */
>>+	__raw_writel(OTG_SYSC_SOFTRESET, otg_base + OTG_SYSCONFIG);
>>+
>>+	clk_disable(iclk);
>>+	clk_put(iclk);
>>+	iounmap(otg_base);
>> }
>>
>> #ifdef CONFIG_USB_MUSB_SOC
>>--
>>1.6.3.2
>>
>>--
>>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
--
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
Anand Gadiyar June 24, 2009, 6:11 p.m. UTC | #4 
<snip>
> >
> >>
> >> static void __init usb_musb_pm_init(void)
> >> {
> >>-     /* Ensure force-idle mode for OTG controller */
> >>-     if (cpu_is_omap34xx())
> >>-             omap_writel(0, OTG_SYSCONFIG);
> >>+     struct clk *iclk;
> >>+     void __iomem *otg_base;
> >>+
> >>+     if (!cpu_is_omap34xx())
> >>+             return;
> >
> > Why is this sys-config reset done only for omap34xx()
> > This should be valid for any old 24xx() as well as new 44xx() omaps
> 

Minor correction - it's valid only for any old 243x() only. 242x did not
have this controller, IIRC.


> You're correct.  Good catch.
> 
> The force-idle "fix" that this is replacing was only being done on
> 34xx due to the smart-idle errata.  However, this reset should be
> done even without any errata.
> 
> Thanks for review,  will respin.
> 
> Kevin
> 
--
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
Woodruff, Richard June 24, 2009, 7:27 p.m. UTC | #5 
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kevin Hilman

> > Why is this sys-config reset done only for omap34xx()
> > This should be valid for any old 24xx() as well as new 44xx() omaps
>
> You're correct.  Good catch.
>
> The force-idle "fix" that this is replacing was only being done on
> 34xx due to the smart-idle errata.  However, this reset should be
> done even without any errata.

Minor note, IIRC the different OTG IP used in OMAP2420 did require a similar force at boot time.  That would be a different file if it exists.

Regards,
Richard W.
--
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 June 25, 2009, 6:28 a.m. UTC | #6 
On Wed, 24 Jun 2009, Pandita, Vikram wrote:

> >-----Original Message-----
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Kevin
> >Hilman
> >
> >Rather than simply setting force-idle mode on boot, do a reset of the
> >OTG module.  This really ensures that any bootloader/bootstrap code
> >that leaves it active will not prevent future retention.  After reset,
> >OTG module will be in force-idle, force-standby mode.
> >
> >In addition, ensure that the iclk is enabled before attempting a write
> >to the module SYSCONFIG register.
> 
> This patch solves MUSB issue, but is not generic for all IP blocks on omap.
> 
> ROM code or u-boot could leave the module sysconfig in a non-idle state.
> 
> I recommend just like CONFIG_OMAP_RESET_CLOCKS for clocks, same could be 
> done generically for all SYSCONFIG registers in some prcm code and not 
> driver specific.

I agree.  Please contribute to the development of the omap_hwmod patchset, 
which does this (among other things).


- 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
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c
index d85296d..31adbe6 100644
--- a/arch/arm/mach-omap2/usb-musb.c
+++ b/arch/arm/mach-omap2/usb-musb.c
@@ -26,18 +26,41 @@ 
 
 #include <linux/usb/musb.h>
 
+#include <asm/sizes.h>
+
 #include <mach/hardware.h>
 #include <mach/irqs.h>
 #include <mach/mux.h>
 #include <mach/usb.h>
 
-#define OTG_SYSCONFIG	(OMAP34XX_HSUSB_OTG_BASE + 0x404)
+#define OTG_SYSCONFIG	   0x404
+#define OTG_SYSC_SOFTRESET BIT(1)
 
 static void __init usb_musb_pm_init(void)
 {
-	/* Ensure force-idle mode for OTG controller */
-	if (cpu_is_omap34xx())
-		omap_writel(0, OTG_SYSCONFIG);
+	struct clk *iclk;
+	void __iomem *otg_base;
+
+	if (!cpu_is_omap34xx())
+		return;
+
+	otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K);
+	if (WARN_ON(!otg_base))
+		return;
+
+	iclk = clk_get(NULL, "hsotgusb_ick");
+	if (WARN_ON(!iclk))
+		return;
+
+	clk_enable(iclk);
+
+	/* Reset OTG controller.  After reset, it will be in
+	 * force-idle, force-standby mode. */
+	__raw_writel(OTG_SYSC_SOFTRESET, otg_base + OTG_SYSCONFIG);
+
+	clk_disable(iclk);
+	clk_put(iclk);
+	iounmap(otg_base);
 }
 
 #ifdef CONFIG_USB_MUSB_SOC