On Thu, May 24, 2012 at 08:11:11PM +0800, Yu Xu wrote:
> 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.
cool. tks
--
balbi
Attachment:
signature.asc
Description: Digital signature
