* Felipe Balbi | 2011-11-23 13:59:42 [+0200]: >hi, Hi, >> 255 requests including 255 additional interrupts to notify about the >> completion and not to forget the complete callbacks to the gadget. > >of course not, if no_interrupt is set, it means gadget driver will not >receive the interrupt. BTW, IOC should be set on our driver when >no_interrupt is cleared. Currently we are bypassing that flag and >interrupting always on the last request. That's a mis-use the gadget >framework ;-) Yes :) That was to lower the irq frequency. I still like it. However, if the gadget queues 8 IN requests at once, we might want to give him a head up and inform him after the 4th requests. Something similar to what we do for ISOC. >> Now here is where the sg list comes into the game. You still have your >> 1MiB transfer you want to push through. Instead of 256 usb_requests you >> allocate one. You tell sg_num_entries to be 256 and your sg list will >> have 256 enties (ofcourse you need allocate one of that size but this is >> another story). > >exactly because we have 256 entries, I want to allocate 256 requests, >because the entire code talks usb_requests. Just look at struct >usb_sg_request in <linux/usb.h>. Also, allocating several usb_requests >makes it a lot simpler to allocate on TRB for each request, and we can >use container_of() to fetch our own dwc3_request implementation and so >forth. I don't see the advantage. You have to theoretically deal with the fact that you receveice a requests with a buffer size is > 16MiB. dwc3 can transfer 16MiB in a row, xhci can handle 64KiB only. Other udc may have lower limits. So you should add a pointer within the private part of usb_request where you are atm and not leak this implementation detail into the gadget framework. Still. sg list is designed to be an array. By creating a new structure which contains multiple usb_requests you create the possibility that each one of them has multiple sg entries which in turn may require further splitting due to hw restrictions. Ach, and if you receive a sg list from the io layer you would have to split the the sg list with four entries over 4 usb_requests with one sg entry each. Not good. I know that we don't care about splitting transfers in dwc3 right now because nobody allocates a transfer larger than 16MiB. Anyway once we deal with this "corner case", it should not be complicated to support the sg lisl with only one usb_requests. >> Now, the udc driver gets one usb_request with sg_num_entries = 256 and >> it has to use its sg capabilities which it said it has. Ideally 256 is >> the number of TRBs you need. In the unlikely event where one sg list >> entry exceeds the size of 16MiB you need to split this sg entry into two >> TRBs instead of one. Sarah is forced to do something similar as she has >> 64KiB limit (as you can see in count_sg_trbs_needed()). So once you >> prepared all TRBs (or half of if it if your ring is too small or >> anything) you start the transfer. >> Since the gadget pass one usb_request to the udc it does _not_ care when >> the udc finished the first 20 sg list entries. So you call the callback >> function of the usb_request once all of the 256 sg are through. > >that's the same thing as setting no_interrupt on all first 255 requests >and only calling completion on the last transfer. The difference between >this and the host API is that host SG support is always blocking, it >will sleep on a wait_for_completion(). This is true and I need to change this detail in order to fix UAS support for hcd which don't support sgs as suggested by Alan. The hcd sg interface is blocking but it is no neccesarry a must argument. There is no reason why one could not perform it async. >> >> >+ int (*sg_prepare)(struct usb_ep *ep, struct usb_request_sg *rsg, >> >> >+ struct scatterlist *sg, unsigned int num_entries, >> >> >+ unsigned int length, gfp_t gfp_flags); >> >> >+ int (*sg_queue)(struct usb_ep *ep, struct usb_request_sg *rsg, >> >> >+ gfp_t gfp_flags); >> >> >+ int (*sg_dequeue)(struct usb_ep *ep, struct usb_request_sg *rsg); >> >> >+ >> >> > int (*queue) (struct usb_ep *ep, struct usb_request *req, >> >> > gfp_t gfp_flags); >> >> > int (*dequeue) (struct usb_ep *ep, struct usb_request *req); >> >> >@@ -250,6 +279,45 @@ static inline void usb_ep_free_request(struct usb_ep *ep, >> >> >> >> Why do you need also this extra callbacks? >> > >> >so that scatter/gather API is separate from the other API ?!? don't get >> >your concern. >> >> You don't need to do this. The existing API was enough for the host >> side, should be for the gadget side. > >what are you talking about dude ? host side has separate APIs for SG and >non-SG transfers. See usb_sg_init(), usb_sg_wait() and usb_sg_cancel(). Ach those three. Those are helper functions and not part of the hcd struct / callbacks. As of know I don't see a reason why you should require additional callbacks within the udc. Sebastian -- 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
