| Message ID | 1304932203-31223-1-git-send-email-keshava_mgowda@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, May 09, 2011 at 02:40:03PM +0530, Keshava Munegowda wrote: > From: Keshava Munegowda <Keshava_mgowda@ti.com> > > the disabling of clocks and freeing GPIO are changed > to fix the occurence of the crash of rmmod of ehci and ohci > drivers > > Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com> NAK > --- > drivers/mfd/omap-usb-host.c | 24 ++++++++++++++++-------- > 1 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c > index 3ab9ffa..cda0797 100644 > --- a/drivers/mfd/omap-usb-host.c > +++ b/drivers/mfd/omap-usb-host.c > @@ -939,6 +939,7 @@ static void usbhs_disable(struct device *dev) > struct usbhs_hcd_omap *omap = dev_get_drvdata(dev); > struct usbhs_omap_platform_data *pdata = &omap->platdata; > unsigned long flags = 0; > + unsigned int halt = 0; you shouldn't need this. > @@ -994,24 +995,31 @@ static void usbhs_disable(struct device *dev) > dev_dbg(dev, "operation timed out\n"); > } > > - if (pdata->ehci_data->phy_reset) { > - if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0])) > - gpio_free(pdata->ehci_data->reset_gpio_port[0]); > - > - if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1])) > - gpio_free(pdata->ehci_data->reset_gpio_port[1]); > + if (is_omap_usbhs_rev2(omap)) { > + if (is_ehci_tll_mode(pdata->port_mode[0])) > + clk_enable(omap->usbtll_p1_fck); > + if (is_ehci_tll_mode(pdata->port_mode[1])) > + clk_enable(omap->usbtll_p2_fck); > + clk_disable(omap->utmi_p2_fck); > + clk_disable(omap->utmi_p1_fck); > } > > - clk_disable(omap->utmi_p2_fck); > - clk_disable(omap->utmi_p1_fck); > clk_disable(omap->usbtll_ick); > clk_disable(omap->usbtll_fck); > clk_disable(omap->usbhost_fs_fck); > clk_disable(omap->usbhost_hs_fck); > clk_disable(omap->usbhost_ick); > + halt = 1; at least from this diff, you're always reaching that part of the code rendering this halt flag useless.
On Mon, May 9, 2011 at 3:06 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Mon, May 09, 2011 at 02:40:03PM +0530, Keshava Munegowda wrote: >> From: Keshava Munegowda <Keshava_mgowda@ti.com> >> >> the disabling of clocks and freeing GPIO are changed >> to fix the occurence of the crash of rmmod of ehci and ohci >> drivers >> >> Signed-off-by: Keshava Munegowda <keshava_mgowda@ti.com> > > NAK > >> --- >> drivers/mfd/omap-usb-host.c | 24 ++++++++++++++++-------- >> 1 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c >> index 3ab9ffa..cda0797 100644 >> --- a/drivers/mfd/omap-usb-host.c >> +++ b/drivers/mfd/omap-usb-host.c >> @@ -939,6 +939,7 @@ static void usbhs_disable(struct device *dev) >> struct usbhs_hcd_omap *omap = dev_get_drvdata(dev); >> struct usbhs_omap_platform_data *pdata = &omap->platdata; >> unsigned long flags = 0; >> + unsigned int halt = 0; > > you shouldn't need this. > >> @@ -994,24 +995,31 @@ static void usbhs_disable(struct device *dev) >> dev_dbg(dev, "operation timed out\n"); >> } >> >> - if (pdata->ehci_data->phy_reset) { >> - if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0])) >> - gpio_free(pdata->ehci_data->reset_gpio_port[0]); >> - >> - if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1])) >> - gpio_free(pdata->ehci_data->reset_gpio_port[1]); >> + if (is_omap_usbhs_rev2(omap)) { >> + if (is_ehci_tll_mode(pdata->port_mode[0])) >> + clk_enable(omap->usbtll_p1_fck); >> + if (is_ehci_tll_mode(pdata->port_mode[1])) >> + clk_enable(omap->usbtll_p2_fck); >> + clk_disable(omap->utmi_p2_fck); >> + clk_disable(omap->utmi_p1_fck); >> } >> >> - clk_disable(omap->utmi_p2_fck); >> - clk_disable(omap->utmi_p1_fck); >> clk_disable(omap->usbtll_ick); >> clk_disable(omap->usbtll_fck); >> clk_disable(omap->usbhost_fs_fck); >> clk_disable(omap->usbhost_hs_fck); >> clk_disable(omap->usbhost_ick); >> + halt = 1; > > at least from this diff, you're always reaching that part of the code > rendering this halt flag useless. I you see only the patch ; its looks like variable halt is not needed; If the code; it will be set only when the clocks are disabled; Then after restoring irq, you will free the gpio based on this value. The complete usbhs_disable code is follows: static void usbhs_disable(struct device *dev) { struct usbhs_hcd_omap *omap = dev_get_drvdata(dev); struct usbhs_omap_platform_data *pdata = &omap->platdata; unsigned long flags = 0; unsigned int halt = 0; unsigned long timeout; dev_dbg(dev, "stopping TI HSUSB Controller\n"); spin_lock_irqsave(&omap->lock, flags); if (omap->count == 0) goto end_disble; omap->count--; if (omap->count != 0) goto end_disble; /* Reset OMAP modules for insmod/rmmod to work */ usbhs_write(omap->uhh_base, OMAP_UHH_SYSCONFIG, is_omap_usbhs_rev2(omap) ? OMAP4_UHH_SYSCONFIG_SOFTRESET : OMAP_UHH_SYSCONFIG_SOFTRESET); timeout = jiffies + msecs_to_jiffies(100); while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS) & (1 << 0))) { cpu_relax(); if (time_after(jiffies, timeout)) dev_dbg(dev, "operation timed out\n"); } while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS) & (1 << 1))) { cpu_relax(); if (time_after(jiffies, timeout)) dev_dbg(dev, "operation timed out\n"); } while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS) & (1 << 2))) { cpu_relax(); if (time_after(jiffies, timeout)) dev_dbg(dev, "operation timed out\n"); } usbhs_write(omap->tll_base, OMAP_USBTLL_SYSCONFIG, (1 << 1)); while (!(usbhs_read(omap->tll_base, OMAP_USBTLL_SYSSTATUS) & (1 << 0))) { cpu_relax(); if (time_after(jiffies, timeout)) dev_dbg(dev, "operation timed out\n"); } if (is_omap_usbhs_rev2(omap)) { if (is_ehci_tll_mode(pdata->port_mode[0])) clk_enable(omap->usbtll_p1_fck); if (is_ehci_tll_mode(pdata->port_mode[1])) clk_enable(omap->usbtll_p2_fck); clk_disable(omap->utmi_p2_fck); clk_disable(omap->utmi_p1_fck); } clk_disable(omap->usbtll_ick); clk_disable(omap->usbtll_fck); clk_disable(omap->usbhost_fs_fck); clk_disable(omap->usbhost_hs_fck); clk_disable(omap->usbhost_ick); halt = 1; end_disble: spin_unlock_irqrestore(&omap->lock, flags); if (halt && pdata->ehci_data->phy_reset) { if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0])) gpio_free(pdata->ehci_data->reset_gpio_port[0]); if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1])) gpio_free(pdata->ehci_data->reset_gpio_port[1]); } } > > -- > balbi > -- 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, On Mon, May 09, 2011 at 03:58:53PM +0530, Munegowda, Keshava wrote: > I you see only the patch ; its looks like variable halt is not needed; > > If the code; it will be set only when the clocks are disabled; > Then after restoring irq, you will free the gpio based on this value. that code is wrong in so many ways that it's difficult to reply, but in short: . get rid of the magic constants, define some macros . instead of using only one error label, use several each one taking care of a different case (that will allow you to remove halt flag) . timeout is never reset, meaning after the first loop, the time_after() will always be true. . this omap->count is ugly. . you shouldn't be accessing pdata on the exit path . that save in spin_lock_irqsave() looks unnecessary. Wouldn't spin_lock_irq() be enough ?
diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c index 3ab9ffa..cda0797 100644 --- a/drivers/mfd/omap-usb-host.c +++ b/drivers/mfd/omap-usb-host.c @@ -939,6 +939,7 @@ static void usbhs_disable(struct device *dev) struct usbhs_hcd_omap *omap = dev_get_drvdata(dev); struct usbhs_omap_platform_data *pdata = &omap->platdata; unsigned long flags = 0; + unsigned int halt = 0; unsigned long timeout; dev_dbg(dev, "stopping TI HSUSB Controller\n"); @@ -994,24 +995,31 @@ static void usbhs_disable(struct device *dev) dev_dbg(dev, "operation timed out\n"); } - if (pdata->ehci_data->phy_reset) { - if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0])) - gpio_free(pdata->ehci_data->reset_gpio_port[0]); - - if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1])) - gpio_free(pdata->ehci_data->reset_gpio_port[1]); + if (is_omap_usbhs_rev2(omap)) { + if (is_ehci_tll_mode(pdata->port_mode[0])) + clk_enable(omap->usbtll_p1_fck); + if (is_ehci_tll_mode(pdata->port_mode[1])) + clk_enable(omap->usbtll_p2_fck); + clk_disable(omap->utmi_p2_fck); + clk_disable(omap->utmi_p1_fck); } - clk_disable(omap->utmi_p2_fck); - clk_disable(omap->utmi_p1_fck); clk_disable(omap->usbtll_ick); clk_disable(omap->usbtll_fck); clk_disable(omap->usbhost_fs_fck); clk_disable(omap->usbhost_hs_fck); clk_disable(omap->usbhost_ick); + halt = 1; end_disble: spin_unlock_irqrestore(&omap->lock, flags); + if (halt && pdata->ehci_data->phy_reset) { + if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0])) + gpio_free(pdata->ehci_data->reset_gpio_port[0]); + + if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1])) + gpio_free(pdata->ehci_data->reset_gpio_port[1]); + } } int omap_usbhs_enable(struct device *dev)