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