Re: [PATCH RFC] ehci-omap: simple suspend implementation
Andreas,
On 26/02/18 09:04, Andreas Kemnade wrote:
> Hi,
>
> On Mon, 19 Feb 2018 11:41:36 +0200
> Roger Quadros wrote:
>
>> Andreas,
>>
>> On 16/02/18 20:35, Andreas Kemnade wrote:
>>> On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
>>> Alan Stern 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
> ---
> 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
Re: [PATCH RFC] ehci-omap: simple suspend implementation
Hi,
On Mon, 19 Feb 2018 11:41:36 +0200
Roger Quadros wrote:
> Andreas,
>
> On 16/02/18 20:35, Andreas Kemnade wrote:
> > On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
> > Alan Stern 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
> >>> ---
> >>> 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
pgpFQLciYD48z.pgp
Description: OpenPGP digital signature
Re: [PATCH RFC] ehci-omap: simple suspend implementation
On Mon, 19 Feb 2018 11:41:36 +0200 Roger Quadros 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 pgpfDFsvdAQ5n.pgp Description: OpenPGP digital signature
Re: [PATCH RFC] ehci-omap: simple suspend implementation
Andreas,
On 16/02/18 20:35, Andreas Kemnade wrote:
> On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
> Alan Stern 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
>>> ---
>>> 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.
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] ehci-omap: simple suspend implementation
On Fri, 16 Feb 2018, Andreas Kemnade wrote:
> On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
> Alan Stern 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
> > > ---
> > > 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-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] ehci-omap: simple suspend implementation
On Fri, 16 Feb 2018 13:13:11 -0500 (EST)
Alan Stern 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
> > ---
> > 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
pgpxAUipTYXaD.pgp
Description: OpenPGP digital signature
Re: [PATCH RFC] ehci-omap: simple suspend implementation
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
> ---
> 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-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC] ehci-omap: simple suspend implementation
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
---
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);
+
+ 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,
}
};
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
