Andreas,
On 26/02/18 09:04, Andreas Kemnade wrote:
> Hi,
>
> On Mon, 19 Feb 2018 11:41:36 +0200
> Roger Quadros <rogerq@xxxxxx> wrote:
>
>> Andreas,
>>
>> On 16/02/18 20:35, Andreas Kemnade wrote:
>>> On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
>>> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>>> ---
>>>>> 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
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
