[RFC] ehci-omap: simple suspend implementation

[Andreas Kemnade <andreas@kemnade.info>]

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