Hi Balbi,
2012/5/24 Felipe Balbi <balbi@xxxxxx>:
> Hi,
>
> On Thu, May 24, 2012 at 06:11:37PM +0800, Yu Xu wrote:
>> Hi Balbi,
>>
>>
>> 2012/5/24 Felipe Balbi <balbi@xxxxxx>:
>> > Hi,
>> >
>> > (seems like you chose not to reply to most of my comments...)
>> >
>> I quite agree and understand these comments, so I did not reply, and
>> will fix in the next patch:)
>
> ok, got it.
>
>> >> > this function doesn't take care of all cases. You should be supporting
>> >> > scatter/gather too, and set the gadget->sg_supported flag.
>> >> >
>> I'm not clear about this. the trb_hw buffer is physical continuous, so
>> I use dma_map_single. You mentioned "this function doesn't take care
>> of all cases", what case else should we handle?
>
> It appears that your controller can easily handle scatter/gather and we
> have support for that on the Gadget API already. You need to let gadget
> drivers queue a request which points to a scatterlist.
>
> At least the new Storage gadget driver written using the Target
> Framework can make good use of this scatter/gather approach to avoid
> memcpy() ;-)
>
> If you look at drivers/usb/dwc3/gadget.c::dwc3_prepare_trbs() you will
> find the following:
>
> static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
> {
> [...]
>
> list_for_each_entry_safe(req, n, &dep->request_list, list) {
> unsigned length;
> dma_addr_t dma;
>
> if (req->request.num_mapped_sgs > 0) {
> struct usb_request *request = &req->request;
> struct scatterlist *sg = request->sg;
> struct scatterlist *s;
> int i;
>
> for_each_sg(sg, s, request->num_mapped_sgs, i) {
> unsigned chain = true;
>
> length = sg_dma_len(s);
> dma = sg_dma_address(s);
>
> if (i == (request->num_mapped_sgs - 1) ||
> sg_is_last(s)) {
> last_one = true;
> chain = false;
> }
>
> trbs_left--;
> if (!trbs_left)
> last_one = true;
>
> if (last_one)
> chain = false;
>
> dwc3_prepare_one_trb(dep, req, dma, length,
> last_one, chain);
>
> if (last_one)
> break;
> }
> } else {
> dma = req->request.dma;
> length = req->request.length;
> trbs_left--;
>
> if (!trbs_left)
> last_one = 1;
>
> /* Is this the last request? */
> if (list_is_last(&req->list, &dep->request_list))
> last_one = 1;
>
> dwc3_prepare_one_trb(dep, req, dma, length,
> last_one, false);
>
> if (last_one)
> break;
> }
> }
> }
>
> You should handle the sg case too ;-) If you wish, it could also be done
> as a separate patch, so you have time to test it properly. In that case,
> you can disregard my comment.
>
Thanks for your explaination. We should implement to handle the sg in our code
to follow the framework.
I'll have a seperate patch for it.
> --
> balbi
Thanks,
Yu Xu
--
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
