Re: [PATCH v4] usb: gadget: mv: Add USB 3.0 device driver for Marvell PXA2128 chip.

[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.

-- 
balbi

Attachment: signature.asc
Description: Digital signature