Hi, On Thu, Feb 23, 2012 at 11:00:28AM -0800, Paul Zimmerman wrote: > > 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. the thing is that you expose too much detail about the implementation, where you could make the suspend/resume code a lot simpler by hiding a few details. I generally don't to expose internal APIs to any other user (I'm considering core.c as a user) because then core.c would need to know too much about the Gadget API. But the fact is that core.c only needs those because of some PM features, so instead of exposing implementation details to core.c, I would rater see you implement: int dwc3_gadget_runtime_suspend(struct dwc3 *dwc) { /* * save context * go into hibernation * blablabla */ return 0; } void dwc3_gadget_runtime_resume(struct dwc3 *dwc) { /* * get out of hibernation * restore context */ ret = __dwc3_gadget_start(); WARN(ret, "failed to restart gadget\n"); dwc3_gadget_run_stop(dwc, true); } and use those on core.c's runtime_suspend/runtime_resume respectively. That way we hide details about the gadget API implementation, core makes no assumptions about gadget.c and if we decide to change the implementation of any of those functions, core doesn't even need to know as long as the outcome is kept the same. Of course, the event buffers setup will be on core.c: static int dwc3_runtime_suspend(struct device *dev) { struct dwc3 *dwc = dev_get_drvdata(dev); /* blablabla */ return 0; { static void dwc3_runtime_resume(struct device *dev) { struct dwc3 *dev = dev_get_drvdata(dev); unsigned mode; dwc3_event_buffers_setup(dwc); mode = DWC3_MODE(dwc->hwparams.hwparams0); switch (mode) { case DWC3_MODE_DEVICE: dwc3_gadget_runtime_resume(dwc3); break; case DWC3_MODE_HOST: dwc3_host_runtime_resume(dwc3); /* ?? */ break; case DWC3_MODE_DRD: dwc3_gadget_runtime_resume(dwc3); dwc3_host_runtime_resume(dwc3); /* ?? */ break; } } or something similar, but core.c need not know implementation details about device or host and that's why I don't want to expose those functions. -- balbi
Attachment:
signature.asc
Description: Digital signature
