* Felipe Balbi | 2011-11-23 13:22:20 [+0200]:
>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.
I see. I assumed the host is supposed to fill this field before
submitting the request.
>> 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.
No. You have num_sgs entry which says how many sg entries it has. 0
means no entry i.e. no sg support at all, one means only one sg entry
for the complete transfers, two means ....
->buffer may be NULL if the memory is not in kernel space but accessible
for the device via DMA. Highmem memory for instance :)
>> 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 ? :-)
I don't see any user of that flag. Anyway, you as a udc should not touch
it by writting. Reading is okay.
>>
>> >+ 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.
Yeah, we need to learn that multiple TRBs belong to one usb_request.
First step of fixing this would be to support usb_requests > 16MiB :)
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
