Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
Hi,
On Tue, Feb 12, 2013 at 01:38:52PM +0530, Vivek Gautam wrote:
> Sorry for coming so late on this series :-(, was away actually.
> Just trying with patches in 'dwc3-dev-pm-ops' branch of your tree.
np
> On Mon, Feb 11, 2013 at 3:22 PM, Felipe Balbi wrote:
> > Add support for basic power management on
> > the dwc3 driver. While there is still lots
> > to improve for full PM support, this minimal
> > patch will already make sure that we survive
> > suspend-to-ram and suspend-to-disk without
> > major issues.
> >
> > Cc: Vikas C Sajjan
> > Signed-off-by: Felipe Balbi
> > ---
> > drivers/usb/dwc3/core.c | 108
> > ++
> > drivers/usb/dwc3/core.h | 6 +++
> > drivers/usb/dwc3/gadget.c | 57
> > 3 files changed, 171 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index fb93918..466b894 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -583,11 +583,119 @@ static int dwc3_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> > +#ifdef CONFIG_PM
> > +static int dwc3_prepare(struct device *dev)
> > +{
> > + struct dwc3 *dwc = dev_get_drvdata(dev);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&dwc->lock, flags);
> > +
> > + switch (dwc->mode) {
> > + case DWC3_MODE_DEVICE:
> > + case DWC3_MODE_DRD:
> > + dwc3_gadget_prepare(dwc);
>
> Compiling Host only mode of DWC3 will give linker error :-(
>
> drivers/built-in.o: In function `dwc3_resume':
> drivers/usb/dwc3/core.c:675: undefined reference to `dwc3_gadget_resume'
> drivers/built-in.o: In function `dwc3_suspend':
> drivers/usb/dwc3/core.c:649: undefined reference to `dwc3_gadget_suspend'
> drivers/built-in.o: In function `dwc3_complete':
> drivers/usb/dwc3/core.c:628: undefined reference to `dwc3_gadget_complete'
> drivers/built-in.o: In function `dwc3_prepare':
> drivers/usb/dwc3/core.c:605: undefined reference to `dwc3_gadget_prepare'
>
> we will need to check the "mode" here, since dwc3/gadget.c won't be
> compiling for Host only mode.
nice catch, I'll fix that up :-)
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
Hi Felipe,
Sorry for coming so late on this series :-(, was away actually.
Just trying with patches in 'dwc3-dev-pm-ops' branch of your tree.
On Mon, Feb 11, 2013 at 3:22 PM, Felipe Balbi wrote:
> Add support for basic power management on
> the dwc3 driver. While there is still lots
> to improve for full PM support, this minimal
> patch will already make sure that we survive
> suspend-to-ram and suspend-to-disk without
> major issues.
>
> Cc: Vikas C Sajjan
> Signed-off-by: Felipe Balbi
> ---
> drivers/usb/dwc3/core.c | 108
> ++
> drivers/usb/dwc3/core.h | 6 +++
> drivers/usb/dwc3/gadget.c | 57
> 3 files changed, 171 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index fb93918..466b894 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -583,11 +583,119 @@ static int dwc3_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#ifdef CONFIG_PM
> +static int dwc3_prepare(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> +
> + switch (dwc->mode) {
> + case DWC3_MODE_DEVICE:
> + case DWC3_MODE_DRD:
> + dwc3_gadget_prepare(dwc);
Compiling Host only mode of DWC3 will give linker error :-(
drivers/built-in.o: In function `dwc3_resume':
drivers/usb/dwc3/core.c:675: undefined reference to `dwc3_gadget_resume'
drivers/built-in.o: In function `dwc3_suspend':
drivers/usb/dwc3/core.c:649: undefined reference to `dwc3_gadget_suspend'
drivers/built-in.o: In function `dwc3_complete':
drivers/usb/dwc3/core.c:628: undefined reference to `dwc3_gadget_complete'
drivers/built-in.o: In function `dwc3_prepare':
drivers/usb/dwc3/core.c:605: undefined reference to `dwc3_gadget_prepare'
we will need to check the "mode" here, since dwc3/gadget.c won't be
compiling for Host only mode.
> + /* FALLTHROUGH */
> + case DWC3_MODE_HOST:
> + default:
> + dwc3_event_buffers_cleanup(dwc);
> + break;
> + }
> +
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +
> + return 0;
> +}
> +
> +static void dwc3_complete(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> +
> + switch (dwc->mode) {
> + case DWC3_MODE_DEVICE:
> + case DWC3_MODE_DRD:
> + dwc3_gadget_complete(dwc);
ditto
> + /* FALLTHROUGH */
> + case DWC3_MODE_HOST:
> + default:
> + dwc3_event_buffers_setup(dwc);
> + break;
> + }
> +
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +}
> +
> +static int dwc3_suspend(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> +
> + switch (dwc->mode) {
> + case DWC3_MODE_DEVICE:
> + case DWC3_MODE_DRD:
> + dwc3_gadget_suspend(dwc);
ditto
> + /* FALLTHROUGH */
> + case DWC3_MODE_HOST:
> + default:
> + /* do nothing */
> + break;
> + }
> +
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +
> + return 0;
> +}
> +
> +static int dwc3_resume(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dwc->lock, flags);
> +
> + switch (dwc->mode) {
> + case DWC3_MODE_DEVICE:
> + case DWC3_MODE_DRD:
> + dwc3_gadget_resume(dwc);
ditto
> + /* FALLTHROUGH */
> + case DWC3_MODE_HOST:
> + default:
> + /* do nothing */
> + break;
> + }
> +
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +
> + pm_runtime_disable(dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops dwc3_dev_pm_ops = {
> + .prepare= dwc3_prepare,
> + .complete = dwc3_complete,
> +
> + SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
> +};
> +
> +#define DWC3_PM_OPS&(dwc3_dev_pm_ops)
> +#else
> +#define DWC3_PM_OPSNULL
> +#endif
> +
> static struct platform_driver dwc3_driver = {
> .probe = dwc3_probe,
> .remove = dwc3_remove,
> .driver = {
> .name = "dwc3",
> + .pm = DWC3_PM_OPS,
> },
> };
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e8d74a7..6f4e8ba 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -862,4 +862,10 @@ void dwc3_host_exit(struct dwc3 *dwc);
> int dwc3_gadget_init(struct dwc3 *dwc);
> void dwc3_gadget_exit(str
Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
On Mon, Feb 11, 2013 at 07:36:41PM +0530, kishon wrote: > Hi, > > On Monday 11 February 2013 07:32 PM, Felipe Balbi wrote: > >On Mon, Feb 11, 2013 at 03:58:44PM +0200, Felipe Balbi wrote: > >>On Mon, Feb 11, 2013 at 03:55:06PM +0200, Felipe Balbi wrote: > >>>Hi, > >>> > >>>On Mon, Feb 11, 2013 at 05:26:41PM +0530, kishon wrote: > Hi, > > On Monday 11 February 2013 05:07 PM, Felipe Balbi wrote: > >On Mon, Feb 11, 2013 at 05:02:15PM +0530, kishon wrote: > >>Hi, > >> > >>On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: > >>>Add support for basic power management on > >>>the dwc3 driver. While there is still lots > >>>to improve for full PM support, this minimal > >>>patch will already make sure that we survive > >>>suspend-to-ram and suspend-to-disk without > >>>major issues. > >> > >>It can work only till retention or it can work with off-mode too with > >>this series? Form what I could make out, it'll work till retention. > >>right? > > > >this is not runtime PM and I haven't tested with OMAP. All in all, it > >should work with both, but only testing will tell. Maybe we _do_ need to > >restart the whole IP in that case. At least with PCIe FPGA, it survived > >just fine a suspend-to-ram (echo mem > /sys/power/state). > > > >I'll see if I can test suspend-to-disk later today or tomorrow. > > Ok. Just one thing. I don't see three registers getting restored > properly (needed when suspend-to-disk). One is DWC3_GCTL, which is > being initialized in dwc3_core_init and the other is DWC3_DCFG and > DWC3_DCTL initialized in dwc3_gadget_init(). > >>> > >>>will fix that up, thanks. > >> > >>actually, dctl isn't needed. I'm reconnecting pullups on resume which > >>will set dctl to a correct value. > > > >my bad, I'll loose HIRD threshold if I don't save it, will add to the > >patch. > > No. You are right. commit 2b7583 by Pratyush Anand moves setting HIRD > threshold to conndone interrupt. Initially I was referring to an > older kernel. So DCTL shouldn't be a problem. aaa, good point :-) thanks for noticing it :) -- balbi signature.asc Description: Digital signature
Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
Hi, On Monday 11 February 2013 07:32 PM, Felipe Balbi wrote: On Mon, Feb 11, 2013 at 03:58:44PM +0200, Felipe Balbi wrote: On Mon, Feb 11, 2013 at 03:55:06PM +0200, Felipe Balbi wrote: Hi, On Mon, Feb 11, 2013 at 05:26:41PM +0530, kishon wrote: Hi, On Monday 11 February 2013 05:07 PM, Felipe Balbi wrote: On Mon, Feb 11, 2013 at 05:02:15PM +0530, kishon wrote: Hi, On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: Add support for basic power management on the dwc3 driver. While there is still lots to improve for full PM support, this minimal patch will already make sure that we survive suspend-to-ram and suspend-to-disk without major issues. It can work only till retention or it can work with off-mode too with this series? Form what I could make out, it'll work till retention. right? this is not runtime PM and I haven't tested with OMAP. All in all, it should work with both, but only testing will tell. Maybe we _do_ need to restart the whole IP in that case. At least with PCIe FPGA, it survived just fine a suspend-to-ram (echo mem > /sys/power/state). I'll see if I can test suspend-to-disk later today or tomorrow. Ok. Just one thing. I don't see three registers getting restored properly (needed when suspend-to-disk). One is DWC3_GCTL, which is being initialized in dwc3_core_init and the other is DWC3_DCFG and DWC3_DCTL initialized in dwc3_gadget_init(). will fix that up, thanks. actually, dctl isn't needed. I'm reconnecting pullups on resume which will set dctl to a correct value. my bad, I'll loose HIRD threshold if I don't save it, will add to the patch. No. You are right. commit 2b7583 by Pratyush Anand moves setting HIRD threshold to conndone interrupt. Initially I was referring to an older kernel. So DCTL shouldn't be a problem. Thanks Kishon -- 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 v3 05/11] usb: dwc3: core: add power management support
On Mon, Feb 11, 2013 at 03:58:44PM +0200, Felipe Balbi wrote: > On Mon, Feb 11, 2013 at 03:55:06PM +0200, Felipe Balbi wrote: > > Hi, > > > > On Mon, Feb 11, 2013 at 05:26:41PM +0530, kishon wrote: > > > Hi, > > > > > > On Monday 11 February 2013 05:07 PM, Felipe Balbi wrote: > > > >On Mon, Feb 11, 2013 at 05:02:15PM +0530, kishon wrote: > > > >>Hi, > > > >> > > > >>On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: > > > >>>Add support for basic power management on > > > >>>the dwc3 driver. While there is still lots > > > >>>to improve for full PM support, this minimal > > > >>>patch will already make sure that we survive > > > >>>suspend-to-ram and suspend-to-disk without > > > >>>major issues. > > > >> > > > >>It can work only till retention or it can work with off-mode too with > > > >>this series? Form what I could make out, it'll work till retention. > > > >>right? > > > > > > > >this is not runtime PM and I haven't tested with OMAP. All in all, it > > > >should work with both, but only testing will tell. Maybe we _do_ need to > > > >restart the whole IP in that case. At least with PCIe FPGA, it survived > > > >just fine a suspend-to-ram (echo mem > /sys/power/state). > > > > > > > >I'll see if I can test suspend-to-disk later today or tomorrow. > > > > > > Ok. Just one thing. I don't see three registers getting restored > > > properly (needed when suspend-to-disk). One is DWC3_GCTL, which is > > > being initialized in dwc3_core_init and the other is DWC3_DCFG and > > > DWC3_DCTL initialized in dwc3_gadget_init(). > > > > will fix that up, thanks. > > actually, dctl isn't needed. I'm reconnecting pullups on resume which > will set dctl to a correct value. my bad, I'll loose HIRD threshold if I don't save it, will add to the patch. -- balbi signature.asc Description: Digital signature
Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
On Mon, Feb 11, 2013 at 03:55:06PM +0200, Felipe Balbi wrote: > Hi, > > On Mon, Feb 11, 2013 at 05:26:41PM +0530, kishon wrote: > > Hi, > > > > On Monday 11 February 2013 05:07 PM, Felipe Balbi wrote: > > >On Mon, Feb 11, 2013 at 05:02:15PM +0530, kishon wrote: > > >>Hi, > > >> > > >>On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: > > >>>Add support for basic power management on > > >>>the dwc3 driver. While there is still lots > > >>>to improve for full PM support, this minimal > > >>>patch will already make sure that we survive > > >>>suspend-to-ram and suspend-to-disk without > > >>>major issues. > > >> > > >>It can work only till retention or it can work with off-mode too with > > >>this series? Form what I could make out, it'll work till retention. > > >>right? > > > > > >this is not runtime PM and I haven't tested with OMAP. All in all, it > > >should work with both, but only testing will tell. Maybe we _do_ need to > > >restart the whole IP in that case. At least with PCIe FPGA, it survived > > >just fine a suspend-to-ram (echo mem > /sys/power/state). > > > > > >I'll see if I can test suspend-to-disk later today or tomorrow. > > > > Ok. Just one thing. I don't see three registers getting restored > > properly (needed when suspend-to-disk). One is DWC3_GCTL, which is > > being initialized in dwc3_core_init and the other is DWC3_DCFG and > > DWC3_DCTL initialized in dwc3_gadget_init(). > > will fix that up, thanks. actually, dctl isn't needed. I'm reconnecting pullups on resume which will set dctl to a correct value. -- balbi signature.asc Description: Digital signature
Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
Hi, On Mon, Feb 11, 2013 at 05:26:41PM +0530, kishon wrote: > Hi, > > On Monday 11 February 2013 05:07 PM, Felipe Balbi wrote: > >On Mon, Feb 11, 2013 at 05:02:15PM +0530, kishon wrote: > >>Hi, > >> > >>On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: > >>>Add support for basic power management on > >>>the dwc3 driver. While there is still lots > >>>to improve for full PM support, this minimal > >>>patch will already make sure that we survive > >>>suspend-to-ram and suspend-to-disk without > >>>major issues. > >> > >>It can work only till retention or it can work with off-mode too with > >>this series? Form what I could make out, it'll work till retention. > >>right? > > > >this is not runtime PM and I haven't tested with OMAP. All in all, it > >should work with both, but only testing will tell. Maybe we _do_ need to > >restart the whole IP in that case. At least with PCIe FPGA, it survived > >just fine a suspend-to-ram (echo mem > /sys/power/state). > > > >I'll see if I can test suspend-to-disk later today or tomorrow. > > Ok. Just one thing. I don't see three registers getting restored > properly (needed when suspend-to-disk). One is DWC3_GCTL, which is > being initialized in dwc3_core_init and the other is DWC3_DCFG and > DWC3_DCTL initialized in dwc3_gadget_init(). will fix that up, thanks. -- balbi signature.asc Description: Digital signature
Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
Hi, On Monday 11 February 2013 05:07 PM, Felipe Balbi wrote: On Mon, Feb 11, 2013 at 05:02:15PM +0530, kishon wrote: Hi, On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: Add support for basic power management on the dwc3 driver. While there is still lots to improve for full PM support, this minimal patch will already make sure that we survive suspend-to-ram and suspend-to-disk without major issues. It can work only till retention or it can work with off-mode too with this series? Form what I could make out, it'll work till retention. right? this is not runtime PM and I haven't tested with OMAP. All in all, it should work with both, but only testing will tell. Maybe we _do_ need to restart the whole IP in that case. At least with PCIe FPGA, it survived just fine a suspend-to-ram (echo mem > /sys/power/state). I'll see if I can test suspend-to-disk later today or tomorrow. Ok. Just one thing. I don't see three registers getting restored properly (needed when suspend-to-disk). One is DWC3_GCTL, which is being initialized in dwc3_core_init and the other is DWC3_DCFG and DWC3_DCTL initialized in dwc3_gadget_init(). Thanks Kishon -- 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 v3 05/11] usb: dwc3: core: add power management support
On Mon, Feb 11, 2013 at 05:02:15PM +0530, kishon wrote: > Hi, > > On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: > >Add support for basic power management on > >the dwc3 driver. While there is still lots > >to improve for full PM support, this minimal > >patch will already make sure that we survive > >suspend-to-ram and suspend-to-disk without > >major issues. > > It can work only till retention or it can work with off-mode too with > this series? Form what I could make out, it'll work till retention. > right? this is not runtime PM and I haven't tested with OMAP. All in all, it should work with both, but only testing will tell. Maybe we _do_ need to restart the whole IP in that case. At least with PCIe FPGA, it survived just fine a suspend-to-ram (echo mem > /sys/power/state). I'll see if I can test suspend-to-disk later today or tomorrow. -- balbi signature.asc Description: Digital signature
Re: [PATCH v3 05/11] usb: dwc3: core: add power management support
Hi, On Monday 11 February 2013 03:22 PM, Felipe Balbi wrote: Add support for basic power management on the dwc3 driver. While there is still lots to improve for full PM support, this minimal patch will already make sure that we survive suspend-to-ram and suspend-to-disk without major issues. It can work only till retention or it can work with off-mode too with this series? Form what I could make out, it'll work till retention. right? Thanks Kishon -- 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
