hi,
On Wed, Nov 23, 2011 at 12:40:58PM +0100, Sebastian Andrzej Siewior wrote:
> >On Tue, Nov 22, 2011 at 03:38:34PM +0100, Sebastian Andrzej Siewior wrote:
> >> * Felipe Balbi | 2011-11-22 11:42:05 [+0200]:
> >>
> >> >diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >> >index 22c37d2..fd61375 100644
> >> >--- a/include/linux/usb/gadget.h
> >> >+++ b/include/linux/usb/gadget.h
> >> >@@ -102,6 +108,22 @@ struct usb_request {
> >> > unsigned actual;
> >> > };
> >> >
> >> >+struct usb_request_sg {
> >> >+ int status;
> >> >+ unsigned actual;
> >> >+
> >> >+ void (*complete)(struct usb_ep *ep,
> >> >+ struct usb_request_sg *rsg);
> >> >+ void *context;
> >> >+
> >> >+ /*
> >> >+ * Members below are private to the controller. Gadget
> >> >+ * drivers should not touch them.
> >> >+ */
> >> >+ struct usb_request **requests;
> >> >+ int num_entries;
> >> >+};
> >>
> >> Ehm, why do you need this? This seem to be part of something else.
> >
> >what do you mean ? I need one big request which will group all struct
> >usb_requests, it's just the easiest way to know how usb requests you
> >have on this SG list and, with that, how many requests will have CHN bit
> >set (on dwc3).
>
> I think you confuse two things: one usb_request has one buffer contigues
> buffer and therefore one length. So if you as a gadget need pass 1MiB of
> data you can either allocate 1MiB in one piece and deal with the
> consequences _or_ you allocate 256 requests and split buffer in requests
> of 4KiB in size.
> The host should not see a difference. The udc however has the additional
of course not. It will receive the same data anyway.
> 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 ;-)
> 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.
> 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().
> >> >+ 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().
--
balbi
Attachment:
signature.asc
Description: Digital signature
