| Message ID | 1380708947-15966-6-git-send-email-csmanjuvijay@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Greg, [Adding ep93xx maintainers as well] On Wed, Oct 2, 2013 at 3:15 AM, Majunath Goudar <csmanjuvijay@gmail.com> wrote: > From: Manjunath Goudar <manjunath.goudar@linaro.org> > > Suspend scenario in case of ohci-ep93xx glue was not > properly handled as it was not suspending generic part > of ohci controller. Alan Stern suggested, properly handle > ohci-ep93xx suspend scenario. > > Calling explicitly the ohci_suspend() routine in > ohci_hcd_ep93xx_drv_suspend() will ensure proper handling of > suspend scenario. > > Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org> > Signed-off-by: Manjunath Goudar <csmanjuvijay@gmail.com> > Acked-by: Alan Stern <stern@rowland.harvard.edu> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Greg KH <greg@kroah.com> > Cc: linux-usb@vger.kernel.org > --- > drivers/usb/host/ohci-ep93xx.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/ohci-ep93xx.c b/drivers/usb/host/ohci-ep93xx.c > index 492f681..08409bf 100644 > --- a/drivers/usb/host/ohci-ep93xx.c > +++ b/drivers/usb/host/ohci-ep93xx.c > @@ -112,13 +112,20 @@ static int ohci_hcd_ep93xx_drv_suspend(struct platform_device *pdev, pm_message_ > { > struct usb_hcd *hcd = platform_get_drvdata(pdev); > struct ohci_hcd *ohci = hcd_to_ohci(hcd); > + bool do_wakeup = device_may_wakeup(&pdev->dev); > + int ret; > > if (time_before(jiffies, ohci->next_statechange)) > msleep(5); > ohci->next_statechange = jiffies; > > - clk_disable(usb_host_clock); > - return 0; > + ret = ohci_suspend(hcd, do_wakeup); > + if (ret) > + return ret; > + > + ep93xx_stop_hc(&pdev->dev); > + > + return ret; > } This patch showed up in -next today (or maybe a while ago and I didn't notice). It's causing a build failure on ep93xx: From http://arm-soc.lixom.net/buildlogs/next-thierry/v3.12-rc5-5366-gba2e8c2/buildall.arm.ep93xx_defconfig.log.failed: drivers/usb/host/ohci-ep93xx.c: In function 'ohci_hcd_ep93xx_drv_suspend': drivers/usb/host/ohci-ep93xx.c:126:2: error: implicit declaration of function 'ep93xx_stop_hc' It's really confusing though, because Majunath has posted this patch three times. The first time it had the ep93xx_stop_hc() call in there, the second patch (labelled V2) did not, and the third(?) version, without version label, also lacked it. No revision log in the patch, and no comments on it. But it does seem like the wrong version was merged based on build results. -Olof
On Mon, 14 Oct 2013, Olof Johansson wrote: > Hi, Greg, > > [Adding ep93xx maintainers as well] > > On Wed, Oct 2, 2013 at 3:15 AM, Majunath Goudar <csmanjuvijay@gmail.com> wrote: > > From: Manjunath Goudar <manjunath.goudar@linaro.org> > > > > Suspend scenario in case of ohci-ep93xx glue was not > > properly handled as it was not suspending generic part > > of ohci controller. Alan Stern suggested, properly handle > > ohci-ep93xx suspend scenario. > > > > Calling explicitly the ohci_suspend() routine in > > ohci_hcd_ep93xx_drv_suspend() will ensure proper handling of > > suspend scenario. > > --- a/drivers/usb/host/ohci-ep93xx.c > > +++ b/drivers/usb/host/ohci-ep93xx.c > > @@ -112,13 +112,20 @@ static int ohci_hcd_ep93xx_drv_suspend(struct platform_device *pdev, pm_message_ > > { > > struct usb_hcd *hcd = platform_get_drvdata(pdev); > > struct ohci_hcd *ohci = hcd_to_ohci(hcd); > > + bool do_wakeup = device_may_wakeup(&pdev->dev); > > + int ret; > > > > if (time_before(jiffies, ohci->next_statechange)) > > msleep(5); > > ohci->next_statechange = jiffies; > > > > - clk_disable(usb_host_clock); > > - return 0; > > + ret = ohci_suspend(hcd, do_wakeup); > > + if (ret) > > + return ret; > > + > > + ep93xx_stop_hc(&pdev->dev); > > + > > + return ret; > > } > > This patch showed up in -next today (or maybe a while ago and I didn't > notice). It's causing a build failure on ep93xx: > > From http://arm-soc.lixom.net/buildlogs/next-thierry/v3.12-rc5-5366-gba2e8c2/buildall.arm.ep93xx_defconfig.log.failed: > > drivers/usb/host/ohci-ep93xx.c: In function 'ohci_hcd_ep93xx_drv_suspend': > drivers/usb/host/ohci-ep93xx.c:126:2: error: implicit declaration of > function 'ep93xx_stop_hc' > > > It's really confusing though, because Majunath has posted this patch > three times. The first time it had the ep93xx_stop_hc() call in there, > the second patch (labelled V2) did not, and the third(?) version, > without version label, also lacked it. No revision log in the patch, > and no comments on it. In fact he has posted it a lot more than 3 times, and the version numbers (or lack thereof) are definitely confusing. The reason for the build failure is that Manjanuth started work on this patch many months ago -- back in the spring. At that time the ep93xx_stop_hc() routine did exist. But commit af3f233fd27b (usb: ohci-ep93xx: tidy up driver (*probe) and (*remove)), dated July 1, removed it. Manjunath never updated his patch in response. > But it does seem like the wrong version was merged based on build results. No, it's a rebasing failure, not a wrong version. Alan Stern
On Mon, Oct 14, 2013 at 04:34:56PM -0400, Alan Stern wrote: > On Mon, 14 Oct 2013, Olof Johansson wrote: > > > Hi, Greg, > > > > [Adding ep93xx maintainers as well] > > > > On Wed, Oct 2, 2013 at 3:15 AM, Majunath Goudar <csmanjuvijay@gmail.com> wrote: > > > From: Manjunath Goudar <manjunath.goudar@linaro.org> > > > > > > Suspend scenario in case of ohci-ep93xx glue was not > > > properly handled as it was not suspending generic part > > > of ohci controller. Alan Stern suggested, properly handle > > > ohci-ep93xx suspend scenario. > > > > > > Calling explicitly the ohci_suspend() routine in > > > ohci_hcd_ep93xx_drv_suspend() will ensure proper handling of > > > suspend scenario. > > > > --- a/drivers/usb/host/ohci-ep93xx.c > > > +++ b/drivers/usb/host/ohci-ep93xx.c > > > @@ -112,13 +112,20 @@ static int ohci_hcd_ep93xx_drv_suspend(struct platform_device *pdev, pm_message_ > > > { > > > struct usb_hcd *hcd = platform_get_drvdata(pdev); > > > struct ohci_hcd *ohci = hcd_to_ohci(hcd); > > > + bool do_wakeup = device_may_wakeup(&pdev->dev); > > > + int ret; > > > > > > if (time_before(jiffies, ohci->next_statechange)) > > > msleep(5); > > > ohci->next_statechange = jiffies; > > > > > > - clk_disable(usb_host_clock); > > > - return 0; > > > + ret = ohci_suspend(hcd, do_wakeup); > > > + if (ret) > > > + return ret; > > > + > > > + ep93xx_stop_hc(&pdev->dev); > > > + > > > + return ret; > > > } > > > > This patch showed up in -next today (or maybe a while ago and I didn't > > notice). It's causing a build failure on ep93xx: > > > > From http://arm-soc.lixom.net/buildlogs/next-thierry/v3.12-rc5-5366-gba2e8c2/buildall.arm.ep93xx_defconfig.log.failed: > > > > drivers/usb/host/ohci-ep93xx.c: In function 'ohci_hcd_ep93xx_drv_suspend': > > drivers/usb/host/ohci-ep93xx.c:126:2: error: implicit declaration of > > function 'ep93xx_stop_hc' > > > > > > It's really confusing though, because Majunath has posted this patch > > three times. The first time it had the ep93xx_stop_hc() call in there, > > the second patch (labelled V2) did not, and the third(?) version, > > without version label, also lacked it. No revision log in the patch, > > and no comments on it. > > In fact he has posted it a lot more than 3 times, and the version > numbers (or lack thereof) are definitely confusing. > > The reason for the build failure is that Manjanuth started work on this > patch many months ago -- back in the spring. At that time the > ep93xx_stop_hc() routine did exist. > > But commit af3f233fd27b (usb: ohci-ep93xx: tidy up driver (*probe) and > (*remove)), dated July 1, removed it. Manjunath never updated his > patch in response. > > > But it does seem like the wrong version was merged based on build results. > > No, it's a rebasing failure, not a wrong version. I've reverted all of these patches now, hopefully from Linaro will step up and fix them and resend them. thanks, greg k-h
On Monday, October 14, 2013 1:35 PM, Alan Stern wrote: > On Mon, 14 Oct 2013, Olof Johansson wrote: > >> Hi, Greg, >> >> [Adding ep93xx maintainers as well] >> >> On Wed, Oct 2, 2013 at 3:15 AM, Majunath Goudar <csmanjuvijay@gmail.com> wrote: >>> From: Manjunath Goudar <manjunath.goudar@linaro.org> >>> >>> Suspend scenario in case of ohci-ep93xx glue was not >>> properly handled as it was not suspending generic part >>> of ohci controller. Alan Stern suggested, properly handle >>> ohci-ep93xx suspend scenario. >>> >>> Calling explicitly the ohci_suspend() routine in >>> ohci_hcd_ep93xx_drv_suspend() will ensure proper handling of >>> suspend scenario. > >>> --- a/drivers/usb/host/ohci-ep93xx.c >>> +++ b/drivers/usb/host/ohci-ep93xx.c >>> @@ -112,13 +112,20 @@ static int ohci_hcd_ep93xx_drv_suspend(struct platform_device *pdev, pm_message_ >>> { >>> struct usb_hcd *hcd = platform_get_drvdata(pdev); >>> struct ohci_hcd *ohci = hcd_to_ohci(hcd); >>> + bool do_wakeup = device_may_wakeup(&pdev->dev); >>> + int ret; >>> >>> if (time_before(jiffies, ohci->next_statechange)) >>> msleep(5); >>> ohci->next_statechange = jiffies; >>> >>> - clk_disable(usb_host_clock); >>> - return 0; >>> + ret = ohci_suspend(hcd, do_wakeup); >>> + if (ret) >>> + return ret; >>> + >>> + ep93xx_stop_hc(&pdev->dev); >>> + >>> + return ret; >>> } >> >> This patch showed up in -next today (or maybe a while ago and I didn't >> notice). It's causing a build failure on ep93xx: >> >> From http://arm-soc.lixom.net/buildlogs/next-thierry/v3.12-rc5-5366-gba2e8c2/buildall.arm.ep93xx_defconfig.log.failed: >> >> drivers/usb/host/ohci-ep93xx.c: In function 'ohci_hcd_ep93xx_drv_suspend': >> drivers/usb/host/ohci-ep93xx.c:126:2: error: implicit declaration of >> function 'ep93xx_stop_hc' >> >> >> It's really confusing though, because Majunath has posted this patch >> three times. The first time it had the ep93xx_stop_hc() call in there, >> the second patch (labelled V2) did not, and the third(?) version, >> without version label, also lacked it. No revision log in the patch, >> and no comments on it. > > In fact he has posted it a lot more than 3 times, and the version > numbers (or lack thereof) are definitely confusing. > > The reason for the build failure is that Manjanuth started work on this > patch many months ago -- back in the spring. At that time the > ep93xx_stop_hc() routine did exist. > > But commit af3f233fd27b (usb: ohci-ep93xx: tidy up driver (*probe) and > (*remove)), dated July 1, removed it. Manjunath never updated his > patch in response. > >> But it does seem like the wrong version was merged based on build results. > > No, it's a rebasing failure, not a wrong version. Alan, As an alternative to this patch, I have successfully used the ohci-platform driver on the ep93xx. This does move a bit of glue code into the ep93xx core (arch/arm/mach-ep93xx/core.c) but it removes the ohci-ep93xx glue driver completely. I can post a patch for this shortly if you would like. Regards, Hartley
On Mon, 14 Oct 2013, Hartley Sweeten wrote: > Alan, > > As an alternative to this patch, I have successfully used the ohci-platform > driver on the ep93xx. This does move a bit of glue code into the ep93xx > core (arch/arm/mach-ep93xx/core.c) but it removes the ohci-ep93xx glue > driver completely. > > I can post a patch for this shortly if you would like. That would be great! It would save me the trouble of fixing Manjanuth's patch and it would remove a source file too! Alan Stern
On Monday, October 14, 2013 1:50 PM, Alan Stern wrote: > On Mon, 14 Oct 2013, Hartley Sweeten wrote: >> As an alternative to this patch, I have successfully used the ohci-platform >> driver on the ep93xx. This does move a bit of glue code into the ep93xx >> core (arch/arm/mach-ep93xx/core.c) but it removes the ohci-ep93xx glue >> driver completely. >> >> I can post a patch for this shortly if you would like. > > That would be great! It would save me the trouble of fixing > Manjanuth's patch and it would remove a source file too! The only thing I'm not sure about use where to enable the USB_OHCI_HCD_PLATFORM driver for ARCH_EP93XX. Right now the ohci-ep93xx glue driver is enabled automatically when USB_OHCI_HCD is enabled due to the ifdefery. I can add a new config option similar to the other users of USB_OHCI_HCD_PLATFORM but I was wondering if there is a cleaner way. Regards, Hartley
On Mon, Oct 14, 2013 at 2:07 PM, Hartley Sweeten <HartleyS@visionengravers.com> wrote: > On Monday, October 14, 2013 1:50 PM, Alan Stern wrote: >> On Mon, 14 Oct 2013, Hartley Sweeten wrote: >>> As an alternative to this patch, I have successfully used the ohci-platform >>> driver on the ep93xx. This does move a bit of glue code into the ep93xx >>> core (arch/arm/mach-ep93xx/core.c) but it removes the ohci-ep93xx glue >>> driver completely. >>> >>> I can post a patch for this shortly if you would like. >> >> That would be great! It would save me the trouble of fixing >> Manjanuth's patch and it would remove a source file too! > > The only thing I'm not sure about use where to enable the > USB_OHCI_HCD_PLATFORM driver for ARCH_EP93XX. > > Right now the ohci-ep93xx glue driver is enabled automatically > when USB_OHCI_HCD is enabled due to the ifdefery. I can add > a new config option similar to the other users of > USB_OHCI_HCD_PLATFORM but I was wondering if there is a > cleaner way. Given that we're trying to reduce code under arch/arm and move it out to driver directories instead, I'm not sure this is a step in the right direction. :-) I guess it depends on how much code we're talking about here, so let's take that discussion once patches are posted. -Olof
On Tue, Oct 15, 2013 at 10:34:53AM +0530, manju goudar wrote: > Alan, > > I am very sorry for this mistake. > I will fix this issue and sending back series. > > Greg, > I am really sorry :(( > Next time I will not make this kind of mistake. > If any issue reported related to my patches I will > fix it and send back to you. > Now I am using my personal id to support my patches. Thanks for continuing to work on this, that's great to hear. Please test your patches on all arches that you are modifying, simple build bugs shouldn't happen. thanks, greg k-h
diff --git a/drivers/usb/host/ohci-ep93xx.c b/drivers/usb/host/ohci-ep93xx.c index 492f681..08409bf 100644 --- a/drivers/usb/host/ohci-ep93xx.c +++ b/drivers/usb/host/ohci-ep93xx.c @@ -112,13 +112,20 @@ static int ohci_hcd_ep93xx_drv_suspend(struct platform_device *pdev, pm_message_ { struct usb_hcd *hcd = platform_get_drvdata(pdev); struct ohci_hcd *ohci = hcd_to_ohci(hcd); + bool do_wakeup = device_may_wakeup(&pdev->dev); + int ret; if (time_before(jiffies, ohci->next_statechange)) msleep(5); ohci->next_statechange = jiffies; - clk_disable(usb_host_clock); - return 0; + ret = ohci_suspend(hcd, do_wakeup); + if (ret) + return ret; + + ep93xx_stop_hc(&pdev->dev); + + return ret; } static int ohci_hcd_ep93xx_drv_resume(struct platform_device *pdev)