On Tue, Nov 22, 2011 at 03:54:01PM +0100, Sebastian Andrzej Siewior wrote:
> * Felipe Balbi | 2011-11-22 11:42:06 [+0200]:
>
> >diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >index c04a0d3..c9bdada 100644
> >--- a/drivers/usb/dwc3/gadget.c
> >+++ b/drivers/usb/dwc3/gadget.c
> >@@ -539,6 +539,83 @@ static void dwc3_gadget_ep_free_request(struct usb_ep *ep,
> > kfree(req);
> > }
> >
> >+static int dwc3_gadget_ep_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)
> >+{
> >+ struct dwc3_ep *dep = to_dwc3_ep(ep);
> >+ int ret = -ENOMEM;
> >+ int i;
> >+
> >+ if (!rsg || !sg || num_entries == 0)
> >+ return -EINVAL;
> >+
> >+ rsg->requests = kcalloc(num_entries, sizeof(*rsg->requests), gfp_flags);
> >+ if (!rsg->requests)
> >+ goto err0;
> >+
> >+ for_each_sg(sg, sg, rsg->num_entries, i) {
> >+ struct usb_request *request;
> >+ unsigned len;
> >+
> >+ request = usb_ep_alloc_request(ep, gfp_flags);
> >+ if (!request) {
> >+ rsg->num_entries = i;
> >+ goto err1;
> >+ }
> >+
> >+ len = length;
> >+ if (len == 0) {
> >+ struct scatterlist *sg2;
> >+ int j;
> >+
> >+ for_each_sg(sg, sg2, num_entries, j)
> >+ len += sg2->length;
> >+ }
>
> Why do iterate twice in case length is zero? And we do we have multiple
this is also "borrowed" from host API. If the Length is zero, it means
that we're asking for the entire SG to be sent, so I need to count the
size of all entries before I can assign it to usb_request->length.
> What imagine for sg supportis th follwing: if the udc supports sg we can
> queue sg-based requests which have buf = NULL. A request which we
> receive could look like: req->length = 8192 and req->num_sgs = 3.
> And the sgs are:
> sg 1 starts at mem_page_offset + 2048 len 2048
> sg 2 starts at mem_page_offset + 0 len 4096
> sg 3 starts at mem_page_offset + 0 len 2048
>
> Those three transfers should be part of _one_ request. Therefore you
> don't need this no_interrupt field or anything else like that because
> you call ->complete() once the whole thing is through. With one struct
> usb_request only.
I don't think using a NULL buffer pointer as a check for an SG-based
transfer is really good.
>
> If you queue 2 usb_request struct and the gadget can wait with its
> notification after the second is compled, this is another story.
>
> >+
> >+ request->no_interrupt = true;
> >+ request->buf = NULL;
> >+ request->num_sgs = num_entries;
> >+
> >+ request->length = len;
> >+
> >+ if (usb_endpoint_dir_in(dep->desc))
> >+ request->short_not_ok = true;
>
> Why is this for you to decice? Shouldn't the gadget take care of this?
we shouldn't have shorts on IN transfer, we have created the transfer,
how could it be short ? :-)
>
> >+ rsg->requests[i] = request;
> >+
> >+ /* FIXME allocate a TRB, prepare TRB, set CHN bit, etc */
>
> We have 16 or 32 TRBs waiting. All we need to do prepare then and set
> the CHN bit.
yeah, I know, but we need to re-factor prepare_trbs() a bit.
--
balbi
Attachment:
signature.asc
Description: Digital signature
