| Message ID | 20180216171414.8097-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 16 Feb 2018, Andreas Kemnade wrote: > This powers down the phy and on a gta04 it reduces > suspend current by 13 mA. > For unknown reasons usb does not power on properly. > Also calling usb_phy_shutdown() here feels wrong > apparently the reset line has to be activated. > usb_phy_set_suspend is not enough here. The power > consumption still stays approximately the same as > without any patch. > > With a device connected the device does not enumerate > after resume. A rmmod ehci-omap ; modprobe ehci-omap > does not make it reenumerade. > So there is still something wrong here. > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 57 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c > index 8d8bafc70c1f..0be2ccf8182a 100644 > --- a/drivers/usb/host/ehci-omap.c > +++ b/drivers/usb/host/ehci-omap.c > @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) > return 0; > } > > + > +static int __maybe_unused ehci_omap_suspend(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > + int ret; > + int i; > + > + ret = ehci_suspend(hcd, false); > + if (ret) { > + dev_err(dev, "ehci suspend failed: %d\n", ret); > + return ret; > + } > + for (i = 0; i < omap->nports; i++) { > + if (omap->phy[i]) > + usb_phy_shutdown(omap->phy[i]); > + } > + pm_runtime_put_sync(dev); Why do you include a runtime PM call here, given that the driver doesn't support runtime suspend or resume? > + > + return 0; > +} > + > +static int __maybe_unused ehci_omap_resume(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > + int i; > + > + pm_runtime_get_sync(dev); > + /* > + * An undocumented "feature" in the OMAP3 EHCI controller, > + * causes suspended ports to be taken out of suspend when > + * the USBCMD.Run/Stop bit is cleared (for example when > + * we do ehci_bus_suspend). > + * This breaks suspend-resume if the root-hub is allowed > + * to suspend. Writing 1 to this undocumented register bit > + * disables this feature and restores normal behavior. > + */ > + ehci_write(hcd->regs, EHCI_INSNREG04, > + EHCI_INSNREG04_DISABLE_UNSUSPEND); Doesn't this code belong in ehci_hcd_omap_probe()? I assume you only need to set this undocumented bit once, not every time the controller is suspended. And in any case, according to the comment, you would need to take care of this before the root hub is suspended -- by the time the controller is suspended, it is already too late. Alan Stern > + > + for (i = 0; i < omap->nports; i++) { > + if (omap->phy[i]) { > + usb_phy_init(omap->phy[i]); > + usb_phy_set_suspend(omap->phy[i], false); > + } > + } > + > + ehci_resume(hcd, true); > + return 0; > +} > + > static const struct of_device_id omap_ehci_dt_ids[] = { > { .compatible = "ti,ehci-omap" }, > { } > @@ -273,14 +325,17 @@ static const struct of_device_id omap_ehci_dt_ids[] = { > > MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids); > > +static SIMPLE_DEV_PM_OPS(ehci_omap_pm_ops, ehci_omap_suspend, > + ehci_omap_resume); > + > + > static struct platform_driver ehci_hcd_omap_driver = { > .probe = ehci_hcd_omap_probe, > .remove = ehci_hcd_omap_remove, > .shutdown = usb_hcd_platform_shutdown, > - /*.suspend = ehci_hcd_omap_suspend, */ > - /*.resume = ehci_hcd_omap_resume, */ > .driver = { > .name = hcd_name, > + .pm = &ehci_omap_pm_ops, > .of_match_table = omap_ehci_dt_ids, > } > }; > -- 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
On Fri, 16 Feb 2018 13:13:11 -0500 (EST) Alan Stern <stern@rowland.harvard.edu> wrote: > On Fri, 16 Feb 2018, Andreas Kemnade wrote: > > > This powers down the phy and on a gta04 it reduces > > suspend current by 13 mA. > > For unknown reasons usb does not power on properly. > > Also calling usb_phy_shutdown() here feels wrong > > apparently the reset line has to be activated. > > usb_phy_set_suspend is not enough here. The power > > consumption still stays approximately the same as > > without any patch. > > > > With a device connected the device does not enumerate > > after resume. A rmmod ehci-omap ; modprobe ehci-omap > > does not make it reenumerade. > > So there is still something wrong here. > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > --- > > drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- > > 1 file changed, 57 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c > > index 8d8bafc70c1f..0be2ccf8182a 100644 > > --- a/drivers/usb/host/ehci-omap.c > > +++ b/drivers/usb/host/ehci-omap.c > > @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) > > return 0; > > } > > > > + > > +static int __maybe_unused ehci_omap_suspend(struct device *dev) > > +{ > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > > + int ret; > > + int i; > > + > > + ret = ehci_suspend(hcd, false); > > + if (ret) { > > + dev_err(dev, "ehci suspend failed: %d\n", ret); > > + return ret; > > + } > > + for (i = 0; i < omap->nports; i++) { > > + if (omap->phy[i]) > > + usb_phy_shutdown(omap->phy[i]); > > + } > > + pm_runtime_put_sync(dev); > > Why do you include a runtime PM call here, given that the driver > doesn't support runtime suspend or resume? > Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM calls here in the _probe/_remove functions, so it seems to be sane to do it here, too. > > + > > + return 0; > > +} > > + > > +static int __maybe_unused ehci_omap_resume(struct device *dev) > > +{ > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > > + int i; > > + > > + pm_runtime_get_sync(dev); > > + /* > > + * An undocumented "feature" in the OMAP3 EHCI controller, > > + * causes suspended ports to be taken out of suspend when > > + * the USBCMD.Run/Stop bit is cleared (for example when > > + * we do ehci_bus_suspend). > > + * This breaks suspend-resume if the root-hub is allowed > > + * to suspend. Writing 1 to this undocumented register bit > > + * disables this feature and restores normal behavior. > > + */ > > + ehci_write(hcd->regs, EHCI_INSNREG04, > > + EHCI_INSNREG04_DISABLE_UNSUSPEND); > > Doesn't this code belong in ehci_hcd_omap_probe()? I assume you only > need to set this undocumented bit once, not every time the controller > is suspended. And in any case, according to the comment, you would > need to take care of this before the root hub is suspended -- by the > time the controller is suspended, it is already too late. > I am not really sure about this one. It is set in ehci_hcd_omap_probe() so it is set before suspend. I set it here to ensure it is re-set when the controller is powered down too much to keep register contents. I do not know if that is the case. But I thought it would not harm. Regards, Andreas
On Fri, 16 Feb 2018, Andreas Kemnade wrote: > On Fri, 16 Feb 2018 13:13:11 -0500 (EST) > Alan Stern <stern@rowland.harvard.edu> wrote: > > > On Fri, 16 Feb 2018, Andreas Kemnade wrote: > > > > > This powers down the phy and on a gta04 it reduces > > > suspend current by 13 mA. > > > For unknown reasons usb does not power on properly. > > > Also calling usb_phy_shutdown() here feels wrong > > > apparently the reset line has to be activated. > > > usb_phy_set_suspend is not enough here. The power > > > consumption still stays approximately the same as > > > without any patch. > > > > > > With a device connected the device does not enumerate > > > after resume. A rmmod ehci-omap ; modprobe ehci-omap > > > does not make it reenumerade. > > > So there is still something wrong here. > > > > > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > > > --- > > > drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 57 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c > > > index 8d8bafc70c1f..0be2ccf8182a 100644 > > > --- a/drivers/usb/host/ehci-omap.c > > > +++ b/drivers/usb/host/ehci-omap.c > > > @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) > > > return 0; > > > } > > > > > > + > > > +static int __maybe_unused ehci_omap_suspend(struct device *dev) > > > +{ > > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > > > + int ret; > > > + int i; > > > + > > > + ret = ehci_suspend(hcd, false); > > > + if (ret) { > > > + dev_err(dev, "ehci suspend failed: %d\n", ret); > > > + return ret; > > > + } > > > + for (i = 0; i < omap->nports; i++) { > > > + if (omap->phy[i]) > > > + usb_phy_shutdown(omap->phy[i]); > > > + } > > > + pm_runtime_put_sync(dev); > > > > Why do you include a runtime PM call here, given that the driver > > doesn't support runtime suspend or resume? > > > Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM > calls here in the _probe/_remove functions, so it seems to be sane to do it > here, too. Not really -- when the whole system is going into suspend anyway, there's no point in trying to do a runtime suspend of the EHCI controller. (In fact, the PM subsystem doesn't even allow it.) > > > + > > > + return 0; > > > +} > > > + > > > +static int __maybe_unused ehci_omap_resume(struct device *dev) > > > +{ > > > + struct usb_hcd *hcd = dev_get_drvdata(dev); > > > + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > > > + int i; > > > + > > > + pm_runtime_get_sync(dev); > > > + /* > > > + * An undocumented "feature" in the OMAP3 EHCI controller, > > > + * causes suspended ports to be taken out of suspend when > > > + * the USBCMD.Run/Stop bit is cleared (for example when > > > + * we do ehci_bus_suspend). > > > + * This breaks suspend-resume if the root-hub is allowed > > > + * to suspend. Writing 1 to this undocumented register bit > > > + * disables this feature and restores normal behavior. > > > + */ > > > + ehci_write(hcd->regs, EHCI_INSNREG04, > > > + EHCI_INSNREG04_DISABLE_UNSUSPEND); > > > > Doesn't this code belong in ehci_hcd_omap_probe()? I assume you only > > need to set this undocumented bit once, not every time the controller > > is suspended. And in any case, according to the comment, you would > > need to take care of this before the root hub is suspended -- by the > > time the controller is suspended, it is already too late. > > > > I am not really sure about this one. It is set in ehci_hcd_omap_probe() > so it is set before suspend. I set it here to ensure it is re-set when > the controller is powered down too much to keep register contents. I > do not know if that is the case. But I thought it would not harm. Ah, okay. That's reasonable. But you should change the comment: Refer the reader to the existing comment in ehci_hcd_omap_probe() instead of duplicating it, and then explain that the undocumented bit might not survive a loss of power. Alan Stern -- 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
Andreas, On 16/02/18 20:35, Andreas Kemnade wrote: > On Fri, 16 Feb 2018 13:13:11 -0500 (EST) > Alan Stern <stern@rowland.harvard.edu> wrote: > >> On Fri, 16 Feb 2018, Andreas Kemnade wrote: >> >>> This powers down the phy and on a gta04 it reduces >>> suspend current by 13 mA. >>> For unknown reasons usb does not power on properly. >>> Also calling usb_phy_shutdown() here feels wrong >>> apparently the reset line has to be activated. >>> usb_phy_set_suspend is not enough here. The power >>> consumption still stays approximately the same as >>> without any patch. >>> >>> With a device connected the device does not enumerate >>> after resume. A rmmod ehci-omap ; modprobe ehci-omap >>> does not make it reenumerade. >>> So there is still something wrong here. >>> >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> >>> --- >>> drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 57 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c >>> index 8d8bafc70c1f..0be2ccf8182a 100644 >>> --- a/drivers/usb/host/ehci-omap.c >>> +++ b/drivers/usb/host/ehci-omap.c >>> @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) >>> return 0; >>> } >>> >>> + >>> +static int __maybe_unused ehci_omap_suspend(struct device *dev) >>> +{ >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); >>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; >>> + int ret; >>> + int i; >>> + >>> + ret = ehci_suspend(hcd, false); >>> + if (ret) { >>> + dev_err(dev, "ehci suspend failed: %d\n", ret); >>> + return ret; >>> + } >>> + for (i = 0; i < omap->nports; i++) { >>> + if (omap->phy[i]) >>> + usb_phy_shutdown(omap->phy[i]); >>> + } >>> + pm_runtime_put_sync(dev); >> >> Why do you include a runtime PM call here, given that the driver >> doesn't support runtime suspend or resume? >> > Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM > calls here in the _probe/_remove functions, so it seems to be sane to do it > here, too. > > >>> + >>> + return 0; >>> +} >>> + >>> +static int __maybe_unused ehci_omap_resume(struct device *dev) >>> +{ >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); >>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; >>> + int i; >>> + >>> + pm_runtime_get_sync(dev); >>> + /* >>> + * An undocumented "feature" in the OMAP3 EHCI controller, >>> + * causes suspended ports to be taken out of suspend when >>> + * the USBCMD.Run/Stop bit is cleared (for example when >>> + * we do ehci_bus_suspend). >>> + * This breaks suspend-resume if the root-hub is allowed >>> + * to suspend. Writing 1 to this undocumented register bit >>> + * disables this feature and restores normal behavior. >>> + */ >>> + ehci_write(hcd->regs, EHCI_INSNREG04, >>> + EHCI_INSNREG04_DISABLE_UNSUSPEND); >> >> Doesn't this code belong in ehci_hcd_omap_probe()? I assume you only >> need to set this undocumented bit once, not every time the controller >> is suspended. And in any case, according to the comment, you would >> need to take care of this before the root hub is suspended -- by the >> time the controller is suspended, it is already too late. >> > > I am not really sure about this one. It is set in ehci_hcd_omap_probe() > so it is set before suspend. I set it here to ensure it is re-set when > the controller is powered down too much to keep register contents. I > do not know if that is the case. But I thought it would not harm. > If the Hardware SAR (Save and restore) functionality is enabled then everything will be restored by hardware after a sleep to wake transition. But you will need this patch to enable SAR for the USB power domain. https://lkml.org/lkml/2013/7/10/356 Missing this might be the reason why things break for you after a system suspend/resume.
On Mon, 19 Feb 2018 11:41:36 +0200 Roger Quadros <rogerq@ti.com> wrote: [...] > If the Hardware SAR (Save and restore) functionality is enabled then > everything will be restored by hardware after a sleep to wake transition. > > But you will need this patch to enable SAR for the USB power domain. > https://lkml.org/lkml/2013/7/10/356 > > Missing this might be the reason why things break for you after a system > suspend/resume. > ehci_resume() has a force reset flag which should be enough to bring the system to a known state. But I tried SAR. It did not help. rmmod ehci_omap modprobe ehci_omap is enough to make the device disappear (with and without the patch). rebooting is enough to make the device appear again. That did once work, so I'll first check when did that broke. Regards, Andreas
Hi, On Mon, 19 Feb 2018 11:41:36 +0200 Roger Quadros <rogerq@ti.com> wrote: > Andreas, > > On 16/02/18 20:35, Andreas Kemnade wrote: > > On Fri, 16 Feb 2018 13:13:11 -0500 (EST) > > Alan Stern <stern@rowland.harvard.edu> wrote: > > > >> On Fri, 16 Feb 2018, Andreas Kemnade wrote: > >> > >>> This powers down the phy and on a gta04 it reduces > >>> suspend current by 13 mA. > >>> For unknown reasons usb does not power on properly. > >>> Also calling usb_phy_shutdown() here feels wrong > >>> apparently the reset line has to be activated. > >>> usb_phy_set_suspend is not enough here. The power > >>> consumption still stays approximately the same as > >>> without any patch. > >>> > >>> With a device connected the device does not enumerate > >>> after resume. A rmmod ehci-omap ; modprobe ehci-omap > >>> does not make it reenumerade. > >>> So there is still something wrong here. > >>> > >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > >>> --- > >>> drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- > >>> 1 file changed, 57 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c > >>> index 8d8bafc70c1f..0be2ccf8182a 100644 > >>> --- a/drivers/usb/host/ehci-omap.c > >>> +++ b/drivers/usb/host/ehci-omap.c > >>> @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) > >>> return 0; > >>> } > >>> > >>> + > >>> +static int __maybe_unused ehci_omap_suspend(struct device *dev) > >>> +{ > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); > >>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > >>> + int ret; > >>> + int i; > >>> + > >>> + ret = ehci_suspend(hcd, false); > >>> + if (ret) { > >>> + dev_err(dev, "ehci suspend failed: %d\n", ret); > >>> + return ret; > >>> + } > >>> + for (i = 0; i < omap->nports; i++) { > >>> + if (omap->phy[i]) > >>> + usb_phy_shutdown(omap->phy[i]); > >>> + } > >>> + pm_runtime_put_sync(dev); > >> > >> Why do you include a runtime PM call here, given that the driver > >> doesn't support runtime suspend or resume? > >> > > Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM > > calls here in the _probe/_remove functions, so it seems to be sane to do it > > here, too. > > > > > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int __maybe_unused ehci_omap_resume(struct device *dev) > >>> +{ > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); > >>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; > >>> + int i; > >>> + > >>> + pm_runtime_get_sync(dev); > >>> + /* > >>> + * An undocumented "feature" in the OMAP3 EHCI controller, > >>> + * causes suspended ports to be taken out of suspend when > >>> + * the USBCMD.Run/Stop bit is cleared (for example when > >>> + * we do ehci_bus_suspend). > >>> + * This breaks suspend-resume if the root-hub is allowed > >>> + * to suspend. Writing 1 to this undocumented register bit > >>> + * disables this feature and restores normal behavior. > >>> + */ > >>> + ehci_write(hcd->regs, EHCI_INSNREG04, > >>> + EHCI_INSNREG04_DISABLE_UNSUSPEND); > >> > >> Doesn't this code belong in ehci_hcd_omap_probe()? I assume you only > >> need to set this undocumented bit once, not every time the controller > >> is suspended. And in any case, according to the comment, you would > >> need to take care of this before the root hub is suspended -- by the > >> time the controller is suspended, it is already too late. > >> > > > > I am not really sure about this one. It is set in ehci_hcd_omap_probe() > > so it is set before suspend. I set it here to ensure it is re-set when > > the controller is powered down too much to keep register contents. I > > do not know if that is the case. But I thought it would not harm. > > > > If the Hardware SAR (Save and restore) functionality is enabled then > everything will be restored by hardware after a sleep to wake transition. > > But you will need this patch to enable SAR for the USB power domain. > https://lkml.org/lkml/2013/7/10/356 > > Missing this might be the reason why things break for you after a system > suspend/resume. > Thanks for your hints. There is a full patchset for suspend/resume and runtime suspend support. Is that the latest one? Is it worth to continue on that. I have now a hacky working solution. I enabled offmode and inserted a msleep(50) before ehci_resume, so the phy has more time to come up. What I think is happening is that ehci does not put the phy into lowpower mode via ulpi register access (Register 4, suspendM bit), and if I reset the phy, it will make wrong assumptions about phy state if not put in offmode. I have to enable more debug there to see what is going on there. Regards, Andreas
Andreas, On 26/02/18 09:04, Andreas Kemnade wrote: > Hi, > > On Mon, 19 Feb 2018 11:41:36 +0200 > Roger Quadros <rogerq@ti.com> wrote: > >> Andreas, >> >> On 16/02/18 20:35, Andreas Kemnade wrote: >>> On Fri, 16 Feb 2018 13:13:11 -0500 (EST) >>> Alan Stern <stern@rowland.harvard.edu> wrote: >>> >>>> On Fri, 16 Feb 2018, Andreas Kemnade wrote: >>>> >>>>> This powers down the phy and on a gta04 it reduces >>>>> suspend current by 13 mA. >>>>> For unknown reasons usb does not power on properly. >>>>> Also calling usb_phy_shutdown() here feels wrong >>>>> apparently the reset line has to be activated. >>>>> usb_phy_set_suspend is not enough here. The power >>>>> consumption still stays approximately the same as >>>>> without any patch. >>>>> >>>>> With a device connected the device does not enumerate >>>>> after resume. A rmmod ehci-omap ; modprobe ehci-omap >>>>> does not make it reenumerade. >>>>> So there is still something wrong here. >>>>> >>>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info> >>>>> --- >>>>> drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 57 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c >>>>> index 8d8bafc70c1f..0be2ccf8182a 100644 >>>>> --- a/drivers/usb/host/ehci-omap.c >>>>> +++ b/drivers/usb/host/ehci-omap.c >>>>> @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) >>>>> return 0; >>>>> } >>>>> >>>>> + >>>>> +static int __maybe_unused ehci_omap_suspend(struct device *dev) >>>>> +{ >>>>> + struct usb_hcd *hcd = dev_get_drvdata(dev); >>>>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; >>>>> + int ret; >>>>> + int i; >>>>> + >>>>> + ret = ehci_suspend(hcd, false); >>>>> + if (ret) { >>>>> + dev_err(dev, "ehci suspend failed: %d\n", ret); >>>>> + return ret; >>>>> + } >>>>> + for (i = 0; i < omap->nports; i++) { >>>>> + if (omap->phy[i]) >>>>> + usb_phy_shutdown(omap->phy[i]); >>>>> + } >>>>> + pm_runtime_put_sync(dev); >>>> >>>> Why do you include a runtime PM call here, given that the driver >>>> doesn't support runtime suspend or resume? >>>> >>> Well, the parent (drivers/mfd/omap-usb-host.c) has, and there are runtime PM >>> calls here in the _probe/_remove functions, so it seems to be sane to do it >>> here, too. >>> >>> >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __maybe_unused ehci_omap_resume(struct device *dev) >>>>> +{ >>>>> + struct usb_hcd *hcd = dev_get_drvdata(dev); >>>>> + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; >>>>> + int i; >>>>> + >>>>> + pm_runtime_get_sync(dev); >>>>> + /* >>>>> + * An undocumented "feature" in the OMAP3 EHCI controller, >>>>> + * causes suspended ports to be taken out of suspend when >>>>> + * the USBCMD.Run/Stop bit is cleared (for example when >>>>> + * we do ehci_bus_suspend). >>>>> + * This breaks suspend-resume if the root-hub is allowed >>>>> + * to suspend. Writing 1 to this undocumented register bit >>>>> + * disables this feature and restores normal behavior. >>>>> + */ >>>>> + ehci_write(hcd->regs, EHCI_INSNREG04, >>>>> + EHCI_INSNREG04_DISABLE_UNSUSPEND); >>>> >>>> Doesn't this code belong in ehci_hcd_omap_probe()? I assume you only >>>> need to set this undocumented bit once, not every time the controller >>>> is suspended. And in any case, according to the comment, you would >>>> need to take care of this before the root hub is suspended -- by the >>>> time the controller is suspended, it is already too late. >>>> >>> >>> I am not really sure about this one. It is set in ehci_hcd_omap_probe() >>> so it is set before suspend. I set it here to ensure it is re-set when >>> the controller is powered down too much to keep register contents. I >>> do not know if that is the case. But I thought it would not harm. >>> >> >> If the Hardware SAR (Save and restore) functionality is enabled then >> everything will be restored by hardware after a sleep to wake transition. >> >> But you will need this patch to enable SAR for the USB power domain. >> https://lkml.org/lkml/2013/7/10/356 >> >> Missing this might be the reason why things break for you after a system >> suspend/resume. >> > Thanks for your hints. > There is a full patchset for suspend/resume and > runtime suspend support. Is that the latest one? Is it worth to > continue on that. That is a very old patch series and I haven't worked on it since. But that series should give you all that is needed to get a reliable suspend/resume with remote wakeup as well. > > I have now a hacky working solution. I enabled offmode and inserted > a msleep(50) before ehci_resume, so the phy has more time to come up. > > What I think is happening is that ehci does not put the phy into > lowpower mode via ulpi register access (Register 4, suspendM bit), > and if I reset the phy, it will make wrong assumptions about phy state > if not put in offmode. > I have to enable more debug there to see what is going on there. > > Regards, > Andreas >
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c index 8d8bafc70c1f..0be2ccf8182a 100644 --- a/drivers/usb/host/ehci-omap.c +++ b/drivers/usb/host/ehci-omap.c @@ -266,6 +266,58 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) return 0; } + +static int __maybe_unused ehci_omap_suspend(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; + int ret; + int i; + + ret = ehci_suspend(hcd, false); + if (ret) { + dev_err(dev, "ehci suspend failed: %d\n", ret); + return ret; + } + for (i = 0; i < omap->nports; i++) { + if (omap->phy[i]) + usb_phy_shutdown(omap->phy[i]); + } + pm_runtime_put_sync(dev); + + return 0; +} + +static int __maybe_unused ehci_omap_resume(struct device *dev) +{ + struct usb_hcd *hcd = dev_get_drvdata(dev); + struct omap_hcd *omap = (struct omap_hcd *)hcd_to_ehci(hcd)->priv; + int i; + + pm_runtime_get_sync(dev); + /* + * An undocumented "feature" in the OMAP3 EHCI controller, + * causes suspended ports to be taken out of suspend when + * the USBCMD.Run/Stop bit is cleared (for example when + * we do ehci_bus_suspend). + * This breaks suspend-resume if the root-hub is allowed + * to suspend. Writing 1 to this undocumented register bit + * disables this feature and restores normal behavior. + */ + ehci_write(hcd->regs, EHCI_INSNREG04, + EHCI_INSNREG04_DISABLE_UNSUSPEND); + + for (i = 0; i < omap->nports; i++) { + if (omap->phy[i]) { + usb_phy_init(omap->phy[i]); + usb_phy_set_suspend(omap->phy[i], false); + } + } + + ehci_resume(hcd, true); + return 0; +} + static const struct of_device_id omap_ehci_dt_ids[] = { { .compatible = "ti,ehci-omap" }, { } @@ -273,14 +325,17 @@ static const struct of_device_id omap_ehci_dt_ids[] = { MODULE_DEVICE_TABLE(of, omap_ehci_dt_ids); +static SIMPLE_DEV_PM_OPS(ehci_omap_pm_ops, ehci_omap_suspend, + ehci_omap_resume); + + static struct platform_driver ehci_hcd_omap_driver = { .probe = ehci_hcd_omap_probe, .remove = ehci_hcd_omap_remove, .shutdown = usb_hcd_platform_shutdown, - /*.suspend = ehci_hcd_omap_suspend, */ - /*.resume = ehci_hcd_omap_resume, */ .driver = { .name = hcd_name, + .pm = &ehci_omap_pm_ops, .of_match_table = omap_ehci_dt_ids, } };
This powers down the phy and on a gta04 it reduces suspend current by 13 mA. For unknown reasons usb does not power on properly. Also calling usb_phy_shutdown() here feels wrong apparently the reset line has to be activated. usb_phy_set_suspend is not enough here. The power consumption still stays approximately the same as without any patch. With a device connected the device does not enumerate after resume. A rmmod ehci-omap ; modprobe ehci-omap does not make it reenumerade. So there is still something wrong here. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- drivers/usb/host/ehci-omap.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-)