| Message ID | 1249938943-22329-1-git-send-email-jon-hunter@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kevin Hilman |
| Headers | show |
> From: Jon Hunter <jon-hunter@ti.com> > > Depending on the OMAP3 boot mode the MUSB peripheral may or may not be > accessible when the kernel starts. > > The OMAP3 can boot from several devices including USB. The sequence of the > devices the OMAP will attempt to boot from is configured via the sys_boot > pins on the device. If USB is one of the devices the OMAP boot ROM attempts > to boot from on power-on, then the interface clock to the MUSB peripheral will > be enabled by the boot ROM and the MUSB peripheral will be accessible when the > kernel boots. However, if USB is not one of the devices OMAP will attempt to > boot from then the interface clock is not enabled and the MUSB peripheral will > not be accessible on start-up. > > If the MUSB peripheral is not accessible when the kernel boots, then the > kernel will crash when attempting to access the OTG_SYSCONFIG in the function > musb_platform_init(). The actual cause of the crash is the write to the > OTG_SYSCONFIG register in the function usb_musb_pm_init() to reset the MUSB > peripheral which occurs prior to calling musb_platform_init(). The function > usb_musb_pm_init() does not check to see if the interface clock for the MUSB > peripheral is enabled before writing to MUSB register. This write access does > not generate a data-abort at this point, but because this write does not > complete all future accesses to the MUSB controller will generate data-aborts > regardless of whether the interface clock has been enabled at a later stage. > The only way I have found to recover from this is resetting the device. > My understanding is that the interconnect works in this way to prevent a bad > access locking up the system. > > This patch crudely ensures the interface clock for the MUSB in the function > usb_musb_pm_init() is enabled. This patch also ensures that the interface > clock is disabled after the reset is complete. My reasoning for always > disabling the clock rather than maintaing its state is: > > 1). If the MUSB peripheral is not being used then the interface clock should > be disabled. > 2). The musb_set_clock() function uses a static variable called "clk_on" to > determine if the MUSB interface clock is on or off. On boot-up clk_on will be > 0 and so this function assumes that the clock is off by default which may not > be the case in the current code. > > I have also added a while-loop to wait for the reset of the MUSB module to > complete before this function exits and the interface clock it disabled. > > Signed-off-by: Jon Hunter <jon-hunter@ti.com> For what it's worth, Tested-by: Anand Gadiyar <gadiyar@ti.com> on a 3430SDP. > --- > arch/arm/mach-omap2/usb-musb.c | 30 ++++++++++++++++++++++++++++-- > 1 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c > index 3efa19c..7cfe9bb 100644 > --- a/arch/arm/mach-omap2/usb-musb.c > +++ b/arch/arm/mach-omap2/usb-musb.c > @@ -32,25 +32,51 @@ > #include <mach/irqs.h> > #include <mach/mux.h> > #include <mach/usb.h> > +#include "cm.h" > > #define OTG_SYSCONFIG 0x404 > #define OTG_SYSC_SOFTRESET BIT(1) > +#define OTG_SYSSTATUS 0x408 > +#define OTG_SYSS_RESETDONE BIT(0) > > static void __init usb_musb_pm_init(void) > { > - void __iomem *otg_base; > + void __iomem *cm_base, *otg_base; > + unsigned int cm_iclken_core; > > if (!cpu_is_omap34xx()) > return; > > + cm_base = ioremap(OMAP3430_CM_BASE, SZ_4K); > + if (WARN_ON(!cm_base)) > + return; > + > otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K); > - if (WARN_ON(!otg_base)) > + if (WARN_ON(!otg_base)) { > + iounmap(cm_base); > return; > + } > + > + /* Ensure the inferface clock for MUSB is enabled */ > + cm_iclken_core = __raw_readl(OMAP34XX_CM_REGADDR(CORE_MOD, > + CM_ICLKEN1)); > + __raw_writel((cm_iclken_core | > + (1 << OMAP3430_EN_HSOTGUSB_SHIFT)), > + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); > > /* Reset OTG controller. After reset, it will be in > * force-idle, force-standby mode. */ > __raw_writel(OTG_SYSC_SOFTRESET, otg_base + OTG_SYSCONFIG); > > + while (!(OTG_SYSS_RESETDONE & __raw_readl(otg_base + OTG_SYSSTATUS))) > + cpu_relax(); > + > + /* Ensure the interface clock for MUSB is disabled */ > + __raw_writel((cm_iclken_core & > + ~(1 << OMAP3430_EN_HSOTGUSB_SHIFT)), > + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); > + > + iounmap(cm_base); > iounmap(otg_base); > } > > -- > 1.6.0.4 > -- 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
Hi Jon, Jon Hunter <jon-hunter@ti.com> writes: > From: Jon Hunter <jon-hunter@ti.com> > > Depending on the OMAP3 boot mode the MUSB peripheral may or may not be > accessible when the kernel starts. > > The OMAP3 can boot from several devices including USB. The sequence of the > devices the OMAP will attempt to boot from is configured via the sys_boot > pins on the device. If USB is one of the devices the OMAP boot ROM attempts > to boot from on power-on, then the interface clock to the MUSB peripheral will > be enabled by the boot ROM and the MUSB peripheral will be accessible when the > kernel boots. However, if USB is not one of the devices OMAP will attempt to > boot from then the interface clock is not enabled and the MUSB peripheral will > not be accessible on start-up. > > If the MUSB peripheral is not accessible when the kernel boots, then the > kernel will crash when attempting to access the OTG_SYSCONFIG in the function > musb_platform_init(). The actual cause of the crash is the write to the > OTG_SYSCONFIG register in the function usb_musb_pm_init() to reset the MUSB > peripheral which occurs prior to calling musb_platform_init(). The function > usb_musb_pm_init() does not check to see if the interface clock for the MUSB > peripheral is enabled before writing to MUSB register. This write access does > not generate a data-abort at this point, but because this write does not > complete all future accesses to the MUSB controller will generate data-aborts > regardless of whether the interface clock has been enabled at a later stage. > The only way I have found to recover from this is resetting the device. > My understanding is that the interconnect works in this way to prevent a bad > access locking up the system. > > This patch crudely ensures the interface clock for the MUSB in the function > usb_musb_pm_init() is enabled. This patch also ensures that the interface > clock is disabled after the reset is complete. My reasoning for always > disabling the clock rather than maintaing its state is: > > 1). If the MUSB peripheral is not being used then the interface clock should > be disabled. > 2). The musb_set_clock() function uses a static variable called "clk_on" to > determine if the MUSB interface clock is on or off. On boot-up clk_on will be > 0 and so this function assumes that the clock is off by default which may not > be the case in the current code. > > I have also added a while-loop to wait for the reset of the MUSB module to > complete before this function exits and the interface clock it disabled. > > Signed-off-by: Jon Hunter <jon-hunter@ti.com> Thanks for this fix. I'll consider this another vote for switching to omap_hwmod. In general, musb_pm_init() is a hack until the omap_hwmod suport for the OTG module is in place. Using hwmod, this reset stuff will be done by hwmod, and all the clocks will be taken care of. All that to say, for the PM branch, I'd rather see USBOTG module implemented in hwmod than to continue to hack a hack. However, since hwmod doesn't exist in pm-2.6.29, I'm willing to carry this fix in pm-2.6.29. To that end, some comments inlined below... > --- > arch/arm/mach-omap2/usb-musb.c | 30 ++++++++++++++++++++++++++++-- > 1 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c > index 3efa19c..7cfe9bb 100644 > --- a/arch/arm/mach-omap2/usb-musb.c > +++ b/arch/arm/mach-omap2/usb-musb.c > @@ -32,25 +32,51 @@ > #include <mach/irqs.h> > #include <mach/mux.h> > #include <mach/usb.h> > +#include "cm.h" > > #define OTG_SYSCONFIG 0x404 > #define OTG_SYSC_SOFTRESET BIT(1) > +#define OTG_SYSSTATUS 0x408 > +#define OTG_SYSS_RESETDONE BIT(0) > > static void __init usb_musb_pm_init(void) > { > - void __iomem *otg_base; > + void __iomem *cm_base, *otg_base; > + unsigned int cm_iclken_core; > > if (!cpu_is_omap34xx()) > return; > > + cm_base = ioremap(OMAP3430_CM_BASE, SZ_4K); > + if (WARN_ON(!cm_base)) > + return; > + > otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K); > - if (WARN_ON(!otg_base)) > + if (WARN_ON(!otg_base)) { > + iounmap(cm_base); > return; > + } > + > + /* Ensure the inferface clock for MUSB is enabled */ > + cm_iclken_core = __raw_readl(OMAP34XX_CM_REGADDR(CORE_MOD, > + CM_ICLKEN1)); > + __raw_writel((cm_iclken_core | > + (1 << OMAP3430_EN_HSOTGUSB_SHIFT)), > + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); Any reason to no use the clock API for this? otg_clk = clk_get(dev, "ick") clk_enable(otg_clk); You'll have to create a dummy 'struct device' and pass it to clk_get() for this to work right. See commit 917fa280e5e99edcae44a34feab295a59922d16c in linux-omap master for how I did this for MMC (but also note that this is now removed in the current PM branch because hwmod takes care of this.) > /* Reset OTG controller. After reset, it will be in > * force-idle, force-standby mode. */ > __raw_writel(OTG_SYSC_SOFTRESET, otg_base + OTG_SYSCONFIG); > > + while (!(OTG_SYSS_RESETDONE & __raw_readl(otg_base + OTG_SYSSTATUS))) > + cpu_relax(); > + > + /* Ensure the interface clock for MUSB is disabled */ > + __raw_writel((cm_iclken_core & > + ~(1 << OMAP3430_EN_HSOTGUSB_SHIFT)), > + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); > + > + iounmap(cm_base); clk_disable(otg_clk); > iounmap(otg_base); > } > 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
Kevin Hilman wrote: > Thanks for this fix. > > I'll consider this another vote for switching to omap_hwmod. To be honest, I am not too familiar with omap_hwmod. Only what I have seen here: http://patchwork.kernel.org/patch/28171/ Anyway, sounds like a good idea. I am not a fan of the current code as it does appear a little random but I can see that it is necessary. > In general, musb_pm_init() is a hack until the omap_hwmod suport > for the OTG module is in place. Using hwmod, this reset stuff will > be done by hwmod, and all the clocks will be taken care of. > > All that to say, for the PM branch, I'd rather see USBOTG module > implemented in hwmod than to continue to hack a hack. Agree. Although I am concerned that this could cause problems for someone that chooses to use a boot mode that does not attempt to boot from USB before booting from their preferred non-volatile memory. > However, since hwmod doesn't exist in pm-2.6.29, I'm willing to > carry this fix in pm-2.6.29. > > To that end, some comments inlined below... Ok, I will clean up and then should I then rebase the patch of the current pm-2.6.29 branch? >> --- >> arch/arm/mach-omap2/usb-musb.c | 30 ++++++++++++++++++++++++++++-- >> 1 files changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c >> index 3efa19c..7cfe9bb 100644 >> --- a/arch/arm/mach-omap2/usb-musb.c >> +++ b/arch/arm/mach-omap2/usb-musb.c >> @@ -32,25 +32,51 @@ >> #include <mach/irqs.h> >> #include <mach/mux.h> >> #include <mach/usb.h> >> +#include "cm.h" >> >> #define OTG_SYSCONFIG 0x404 >> #define OTG_SYSC_SOFTRESET BIT(1) >> +#define OTG_SYSSTATUS 0x408 >> +#define OTG_SYSS_RESETDONE BIT(0) >> >> static void __init usb_musb_pm_init(void) >> { >> - void __iomem *otg_base; >> + void __iomem *cm_base, *otg_base; >> + unsigned int cm_iclken_core; >> >> if (!cpu_is_omap34xx()) >> return; >> >> + cm_base = ioremap(OMAP3430_CM_BASE, SZ_4K); >> + if (WARN_ON(!cm_base)) >> + return; >> + >> otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K); >> - if (WARN_ON(!otg_base)) >> + if (WARN_ON(!otg_base)) { >> + iounmap(cm_base); >> return; >> + } >> + >> + /* Ensure the inferface clock for MUSB is enabled */ >> + cm_iclken_core = __raw_readl(OMAP34XX_CM_REGADDR(CORE_MOD, >> + CM_ICLKEN1)); >> + __raw_writel((cm_iclken_core | >> + (1 << OMAP3430_EN_HSOTGUSB_SHIFT)), >> + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); > > Any reason to no use the clock API for this? > > otg_clk = clk_get(dev, "ick") > clk_enable(otg_clk); No, I just could not see any easy way to do this as I had assumed I needed to use the musb_device structure which may not be defined depending on the kernel config. Hence, my crude hack. However, I agree that this would be preferred. > You'll have to create a dummy 'struct device' and pass it to clk_get() > for this to work right. > > See commit 917fa280e5e99edcae44a34feab295a59922d16c in linux-omap > master for how I did this for MMC (but also note that this is now > removed in the current PM branch because hwmod takes care of this.) Thanks, this should work! Cheers Jon -- 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
Jon Hunter <jon-hunter@ti.com> writes: > Kevin Hilman wrote: >> Thanks for this fix. >> >> I'll consider this another vote for switching to omap_hwmod. > > To be honest, I am not too familiar with omap_hwmod. Only what I have > seen here: > > http://patchwork.kernel.org/patch/28171/ > > Anyway, sounds like a good idea. I am not a fan of the current code as > it does appear a little random but I can see that it is necessary. Yes, it's a bit random and similar things are needed for other blocks as well. Based on the amount of time spent and headaches had due to debugging init/reset/idle issues of various IP blocks, I will be happy to see these various reset hacks disappear and let hwmod handle them. >> In general, musb_pm_init() is a hack until the omap_hwmod suport >> for the OTG module is in place. Using hwmod, this reset stuff will >> be done by hwmod, and all the clocks will be taken care of. >> >> All that to say, for the PM branch, I'd rather see USBOTG module >> implemented in hwmod than to continue to hack a hack. > > Agree. Although I am concerned that this could cause problems for > someone that chooses to use a boot mode that does not attempt to boot > from USB before booting from their preferred non-volatile memory. > >> However, since hwmod doesn't exist in pm-2.6.29, I'm willing to >> carry this fix in pm-2.6.29. >> >> To that end, some comments inlined below... > > Ok, I will clean up and then should I then rebase the patch of the > current pm-2.6.29 branch? Against the current PM branch is preferred and I'll rebase to pm-2.6.29. I'll also carry in current PM branch until OTG hwmod is in place. Kevin >>> --- >>> arch/arm/mach-omap2/usb-musb.c | 30 ++++++++++++++++++++++++++++-- >>> 1 files changed, 28 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c >>> index 3efa19c..7cfe9bb 100644 >>> --- a/arch/arm/mach-omap2/usb-musb.c >>> +++ b/arch/arm/mach-omap2/usb-musb.c >>> @@ -32,25 +32,51 @@ >>> #include <mach/irqs.h> >>> #include <mach/mux.h> >>> #include <mach/usb.h> >>> +#include "cm.h" >>> #define OTG_SYSCONFIG 0x404 >>> #define OTG_SYSC_SOFTRESET BIT(1) >>> +#define OTG_SYSSTATUS 0x408 >>> +#define OTG_SYSS_RESETDONE BIT(0) >>> static void __init usb_musb_pm_init(void) >>> { >>> - void __iomem *otg_base; >>> + void __iomem *cm_base, *otg_base; >>> + unsigned int cm_iclken_core; >>> if (!cpu_is_omap34xx()) >>> return; >>> + cm_base = ioremap(OMAP3430_CM_BASE, SZ_4K); >>> + if (WARN_ON(!cm_base)) >>> + return; >>> + >>> otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K); >>> - if (WARN_ON(!otg_base)) >>> + if (WARN_ON(!otg_base)) { >>> + iounmap(cm_base); >>> return; >>> + } >>> + >>> + /* Ensure the inferface clock for MUSB is enabled */ >>> + cm_iclken_core = __raw_readl(OMAP34XX_CM_REGADDR(CORE_MOD, >>> + CM_ICLKEN1)); >>> + __raw_writel((cm_iclken_core | >>> + (1 << OMAP3430_EN_HSOTGUSB_SHIFT)), >>> + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); >> >> Any reason to no use the clock API for this? >> >> otg_clk = clk_get(dev, "ick") >> clk_enable(otg_clk); > > No, I just could not see any easy way to do this as I had assumed I > needed to use the musb_device structure which may not be defined > depending on the kernel config. Hence, my crude hack. However, I agree > that this would be preferred. > >> You'll have to create a dummy 'struct device' and pass it to clk_get() >> for this to work right. >> >> See commit 917fa280e5e99edcae44a34feab295a59922d16c in linux-omap >> master for how I did this for MMC (but also note that this is now >> removed in the current PM branch because hwmod takes care of this.) > > Thanks, this should work! > > Cheers > Jon -- 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 --git a/arch/arm/mach-omap2/usb-musb.c b/arch/arm/mach-omap2/usb-musb.c index 3efa19c..7cfe9bb 100644 --- a/arch/arm/mach-omap2/usb-musb.c +++ b/arch/arm/mach-omap2/usb-musb.c @@ -32,25 +32,51 @@ #include <mach/irqs.h> #include <mach/mux.h> #include <mach/usb.h> +#include "cm.h" #define OTG_SYSCONFIG 0x404 #define OTG_SYSC_SOFTRESET BIT(1) +#define OTG_SYSSTATUS 0x408 +#define OTG_SYSS_RESETDONE BIT(0) static void __init usb_musb_pm_init(void) { - void __iomem *otg_base; + void __iomem *cm_base, *otg_base; + unsigned int cm_iclken_core; if (!cpu_is_omap34xx()) return; + cm_base = ioremap(OMAP3430_CM_BASE, SZ_4K); + if (WARN_ON(!cm_base)) + return; + otg_base = ioremap(OMAP34XX_HSUSB_OTG_BASE, SZ_4K); - if (WARN_ON(!otg_base)) + if (WARN_ON(!otg_base)) { + iounmap(cm_base); return; + } + + /* Ensure the inferface clock for MUSB is enabled */ + cm_iclken_core = __raw_readl(OMAP34XX_CM_REGADDR(CORE_MOD, + CM_ICLKEN1)); + __raw_writel((cm_iclken_core | + (1 << OMAP3430_EN_HSOTGUSB_SHIFT)), + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); /* Reset OTG controller. After reset, it will be in * force-idle, force-standby mode. */ __raw_writel(OTG_SYSC_SOFTRESET, otg_base + OTG_SYSCONFIG); + while (!(OTG_SYSS_RESETDONE & __raw_readl(otg_base + OTG_SYSSTATUS))) + cpu_relax(); + + /* Ensure the interface clock for MUSB is disabled */ + __raw_writel((cm_iclken_core & + ~(1 << OMAP3430_EN_HSOTGUSB_SHIFT)), + OMAP34XX_CM_REGADDR(CORE_MOD, CM_ICLKEN1)); + + iounmap(cm_base); iounmap(otg_base); }