Hi, On Tue, Feb 07, 2012 at 02:38:55PM -0800, Paul Zimmerman wrote: > > -----Original Message----- > > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Felipe Balbi > > Sent: Monday, February 06, 2012 6:26 AM > > > > Those transfer types are generally high bandwidth, so we > > want to optimize transfers with those endpoints. > > > > For that, databook suggests allocating 3 * wMaxPacketSize > > of FIFO. Let's do that. > > > > Signed-off-by: Felipe Balbi <balbi@xxxxxx> > > --- > > > > This will be sent to v3.4 merge window, unless someone > > has any concerns. > > Hi Felipe, > > I have concerns :) cool :-) > > drivers/usb/dwc3/gadget.c | 20 ++++++++++++++++++-- > > 1 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 1f64e7c..0542b96 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -172,6 +172,7 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc) > > for (num = 0; num < DWC3_ENDPOINTS_NUM; num++) { > > struct dwc3_ep *dep = dwc->eps[num]; > > int fifo_number = dep->number >> 1; > > + int mult = 1; > > int tmp; > > > > if (!(dep->number & 1)) > > @@ -180,11 +181,26 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc) > > if (!(dep->flags & DWC3_EP_ENABLED)) > > continue; > > > > - tmp = dep->endpoint.maxpacket; > > - tmp += mdwidth; > > + if (usb_endpoint_xfer_bulk(dep->desc) > > + || usb_endpoint_xfer_isoc(dep->desc)) > > + mult = 3; > > + > > + /* > > + * REVISIT: the following assumes we will always enough space > > + * available on the FIFO RAM for all possible usecases. Make > > + * sure that's true somehow and change FIFO allocation > > + * accordingly. > > + * > > + * If we have Bulk or Isochronous endpoints, we want > > + * them to be able to be very, very fast. So we're giving > > + * those endpoints a fifo_size which is enough for 3 full > > + * packets > > + */ > > This breaks the HAPS platform, and any other platform which doesn't have > enough FIFO RAM to support this. I really think you must implement this > properly (checking for enough RAM) before you add it to the driver. Indeed, I can add such a check. But can you share any debugging messages you might have gotten through console ? That might help understanding what's breaking it. If you look carefully, I'm just using the fifo allocation equation per Databook. > But I really think trying to do things like this automatically in the > driver is misguided. This should be part of platform data or device tree > instead, IMHO. I tend to disagree, actually. Let's say someone has configured the core with 10 IN eps and has enough fifo for 1x 1024 byte for each endpoint. Let's just say he's taking the performance hit ;-) Now, when we're running with anything that uses less than 10 IN eps, we will have a bunch of unused FIFO space, right ? This will allow me to use that space. So, if we're running with mass storage I will have only two IN eps (EP1 and whatever bulk EP gets selected), meaning that I could allocate up to 9x 1024 bytes to the bulk endpoint. -- balbi
Attachment:
signature.asc
Description: Digital signature
