RE: [PATCH 6/7 v2] usb: dwc3: refactor some existing routines for hibernation support

[Paul Zimmerman <Paul.Zimmerman@xxxxxxxxxxxx>]

> From: Felipe Balbi [mailto:balbi@xxxxxx]
> Sent: Thursday, February 23, 2012 2:21 AM
> 
> On Wed, Feb 15, 2012 at 06:57:01PM -0800, Paul Zimmerman wrote:
> > Some existing routines need to be refactored for hibernation. This
> > includes event buffer setup, restoring saved state when an EP is
> > configured, stopping a transfer without the FORCERM bit set, and
> > stopping a transfer on EP0-OUT.
> >
> > In addition, some routines need to be made non-static, so that the
> > hibernation code, which will live in a separate source file, can
> > call them. In particular, this requires splitting dwc3_gadget_start
> > into two functions, a driver-private one that just starts the
> > hardware, and full-function one that is called through the gadget
> > API.
> >
> > Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx>
> > ---
> >  drivers/usb/dwc3/core.c   |    5 ++-
> >  drivers/usb/dwc3/core.h   |    4 ++
> >  drivers/usb/dwc3/gadget.c |  116 ++++++++++++++++++++++++++------------------
> >  drivers/usb/dwc3/gadget.h |    8 +++
> >  4 files changed, 84 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 7124ec0..829c0d2 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -256,7 +256,7 @@ static int __devinit dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned length)
> >   *
> >   * Returns 0 on success otherwise negative errno.
> >   */
> > -static int __devinit dwc3_event_buffers_setup(struct dwc3 *dwc)
> > +int dwc3_event_buffers_setup(struct dwc3 *dwc)
> >  {
> >  	struct dwc3_event_buffer	*evt;
> >  	int				n;
> > @@ -266,6 +266,7 @@ static int __devinit dwc3_event_buffers_setup(struct dwc3 *dwc)
> >  		dev_dbg(dwc->dev, "Event buf %p dma %08llx length %d\n",
> >  				evt->buf, (unsigned long long) evt->dma,
> >  				evt->length);
> > +		evt->lpos = 0;
> 
> Why ? the events are zero-initialized anyway.

For hibernation, this is called during runtime, so the zero-
initialization doesn't help.

> >  		dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(n),
> >  				lower_32_bits(evt->dma));
> > @@ -286,6 +287,8 @@ static void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
> >
> >  	for (n = 0; n < dwc->num_event_buffers; n++) {
> >  		evt = dwc->ev_buffs[n];
> > +		evt->lpos = 0;
> 
> why ? this will only be called right before freeing the event buffer
> memory.

I'll remove this.


> > -static void dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on)
> > +void dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on)
> >  {
> >  	u32			reg;
> >  	u32			timeout = 500;
> > @@ -1451,47 +1457,24 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> >  	return 0;
> >  }
> >
> > -static int dwc3_gadget_start(struct usb_gadget *g,
> > -		struct usb_gadget_driver *driver)
> > +int __dwc3_gadget_start(struct dwc3 *dwc, bool restore)
> 
> I still don't see the need to expose this to another source file. Just
> call this from your runtime_resume method, maybe that's what you're
> doing but runtime_resume was defined on core.c... I will soo figure it
> out, but even in that case, I believe exposing gadget_stat is the wrong
> way to handle things, maybe you need to expose a new API:
> 
> dwc3_gadget_runtime_suspend()
> dwc3_gadget_runtime_resume()
> 
> and call those from core.c's runtime_suspend/runtime_resume
> respectively if core is configured as DRD or Device modes. Similarly, we
> will need a set of hooks for host, I guess.

Would you rather I just put all the suspend/resume code into gadget.c
and core.c? I really wanted to avoid that, but if you're dead set on
keeping all the routines in gadget.c static, then I don't see any
other way.

-- 
Paul

--
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