| Message ID | 20190329140505.23053-1-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc2: gadget: add scatter-gather mode | expand |
Hi Andrzej, On 2019-03-29 15:05, Andrzej Pietrasiewicz wrote: > From: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > > NOT FOR MERGING > > @Minas: can you please test with this patch instead of the original one? > > @Marek: can you please verify if null pointer bug exists in this version? Yes, I've checked it on my test environment and the NULL ptr issue observed in v2 is fixed. I think it is a good idea to send incremental patch to mainline with a 'Fixes:' tag. > This patch adds support for transferring requests, which are > non-contiguous in physical memory, i.e. the data buffer is described by > a scatter-list. This allows transferring large requests without relying > on error-prone contiguous buffer allocations. This way of allocating > requests is already implemented in functionfs and TCM USB functions and > automatically used if UDC driver advertises scatter-gather suppport. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > [mszyprow: fixed null pointer issue, rewrote commit message] > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > [andrzej.p: improved handling zlps] > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/usb/dwc2/gadget.c | 107 +++++++++++++++++++++++++++----------- > 1 file changed, 76 insertions(+), 31 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index e15d8a462085..e76b2e040b4c 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -768,22 +768,13 @@ static u32 dwc2_gadget_get_desc_params(struct dwc2_hsotg_ep *hs_ep, u32 *mask) > return desc_size; > } > > -/* > - * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > - * @hs_ep: The endpoint > - * @dma_buff: DMA address to use > - * @len: Length of the transfer > - * > - * This function will iterate over descriptor chain and fill its entries > - * with corresponding information based on transfer data. > - */ > -static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > +static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, > + struct dwc2_dma_desc **desc, > dma_addr_t dma_buff, > - unsigned int len) > + unsigned int len, > + bool true_last) > { > - struct dwc2_hsotg *hsotg = hs_ep->parent; > int dir_in = hs_ep->dir_in; > - struct dwc2_dma_desc *desc = hs_ep->desc_list; > u32 mps = hs_ep->ep.maxpacket; > u32 maxsize = 0; > u32 offset = 0; > @@ -798,41 +789,82 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > hs_ep->desc_count = 1; > > for (i = 0; i < hs_ep->desc_count; ++i) { > - desc->status = 0; > - desc->status |= (DEV_DMA_BUFF_STS_HBUSY > + (*desc)->status = 0; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HBUSY > << DEV_DMA_BUFF_STS_SHIFT); > > if (len > maxsize) { > if (!hs_ep->index && !dir_in) > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > - desc->status |= (maxsize << > - DEV_DMA_NBYTES_SHIFT & mask); > - desc->buf = dma_buff + offset; > + (*desc)->status |= > + maxsize << DEV_DMA_NBYTES_SHIFT & mask; > + (*desc)->buf = dma_buff + offset; > > len -= maxsize; > offset += maxsize; > } else { > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + if (true_last) > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > if (dir_in) > - desc->status |= (len % mps) ? DEV_DMA_SHORT : > - ((hs_ep->send_zlp) ? DEV_DMA_SHORT : 0); > - if (len > maxsize) > - dev_err(hsotg->dev, "wrong len %d\n", len); > + (*desc)->status |= (len % mps) ? DEV_DMA_SHORT : > + ((hs_ep->send_zlp && true_last) ? > + DEV_DMA_SHORT : 0); > > - desc->status |= > + (*desc)->status |= > len << DEV_DMA_NBYTES_SHIFT & mask; > - desc->buf = dma_buff + offset; > + (*desc)->buf = dma_buff + offset; > } > > - desc->status &= ~DEV_DMA_BUFF_STS_MASK; > - desc->status |= (DEV_DMA_BUFF_STS_HREADY > + (*desc)->status &= ~DEV_DMA_BUFF_STS_MASK; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HREADY > << DEV_DMA_BUFF_STS_SHIFT); > - desc++; > + (*desc)++; > } > } > > +/* > + * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > + * @hs_ep: The endpoint > + * @ureq: Request to transfer > + * @offset: offset in bytes > + * @len: Length of the transfer > + * > + * This function will iterate over descriptor chain and fill its entries > + * with corresponding information based on transfer data. > + */ > +static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > + dma_addr_t dma_buff, > + unsigned int len) > +{ > + struct usb_request *ureq = NULL; > + struct dwc2_dma_desc *desc = hs_ep->desc_list; > + struct scatterlist *sg; > + int i; > + u8 desc_count = 0; > + > + if (hs_ep->req) > + ureq = &hs_ep->req->req; > + > + /* non-DMA sg buffer */ > + if (!ureq || !ureq->num_sgs) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + dma_buff, len, true); > + return; > + } > + > + /* DMA sg buffer */ > + for_each_sg(ureq->sg, sg, ureq->num_sgs, i) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + sg_dma_address(sg) + sg->offset, sg_dma_len(sg), > + sg_is_last(sg)); > + desc_count += hs_ep->desc_count; > + } > + > + hs_ep->desc_count = desc_count; > +} > + > /* > * dwc2_gadget_fill_isoc_desc - fills next isochronous descriptor in chain. > * @hs_ep: The isochronous endpoint. > @@ -944,7 +976,13 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep) > > hs_ep->next_desc = 0; > list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) { > - ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + ret = dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > if (ret) > break; > @@ -1399,7 +1437,13 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, > */ > if (using_desc_dma(hs) && hs_ep->isochronous) { > if (hs_ep->target_frame != TARGET_FRAME_INITIAL) { > - dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > } > return 0; > @@ -4386,6 +4430,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, > hsotg->enabled = 0; > spin_unlock_irqrestore(&hsotg->lock, flags); > > + gadget->sg_supported = using_desc_dma(hsotg); > dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); > > return 0; Best regards
Hi Andrzej, On 3/29/2019 6:05 PM, Andrzej Pietrasiewicz wrote: > From: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > > NOT FOR MERGING > > @Minas: can you please test with this patch instead of the original one? > Tested new version. No issue seen by running USB CV (Ch9, MSC) tests. Could you please elaborate which else tests you performed? > @Marek: can you please verify if null pointer bug exists in this version? > > This patch adds support for transferring requests, which are > non-contiguous in physical memory, i.e. the data buffer is described by > a scatter-list. This allows transferring large requests without relying > on error-prone contiguous buffer allocations. This way of allocating > requests is already implemented in functionfs and TCM USB functions and > automatically used if UDC driver advertises scatter-gather suppport. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > [mszyprow: fixed null pointer issue, rewrote commit message] > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > [andrzej.p: improved handling zlps] > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/usb/dwc2/gadget.c | 107 +++++++++++++++++++++++++++----------- > 1 file changed, 76 insertions(+), 31 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index e15d8a462085..e76b2e040b4c 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -768,22 +768,13 @@ static u32 dwc2_gadget_get_desc_params(struct dwc2_hsotg_ep *hs_ep, u32 *mask) > return desc_size; > } > > -/* > - * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > - * @hs_ep: The endpoint > - * @dma_buff: DMA address to use > - * @len: Length of the transfer > - * > - * This function will iterate over descriptor chain and fill its entries > - * with corresponding information based on transfer data. > - */ > -static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > +static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, > + struct dwc2_dma_desc **desc, > dma_addr_t dma_buff, > - unsigned int len) > + unsigned int len, > + bool true_last) > { > - struct dwc2_hsotg *hsotg = hs_ep->parent; > int dir_in = hs_ep->dir_in; > - struct dwc2_dma_desc *desc = hs_ep->desc_list; > u32 mps = hs_ep->ep.maxpacket; > u32 maxsize = 0; > u32 offset = 0; > @@ -798,41 +789,82 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > hs_ep->desc_count = 1; > > for (i = 0; i < hs_ep->desc_count; ++i) { > - desc->status = 0; > - desc->status |= (DEV_DMA_BUFF_STS_HBUSY > + (*desc)->status = 0; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HBUSY > << DEV_DMA_BUFF_STS_SHIFT); > > if (len > maxsize) { > if (!hs_ep->index && !dir_in) > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > - desc->status |= (maxsize << > - DEV_DMA_NBYTES_SHIFT & mask); > - desc->buf = dma_buff + offset; > + (*desc)->status |= > + maxsize << DEV_DMA_NBYTES_SHIFT & mask; > + (*desc)->buf = dma_buff + offset; > > len -= maxsize; > offset += maxsize; > } else { > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + if (true_last) > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > if (dir_in) > - desc->status |= (len % mps) ? DEV_DMA_SHORT : > - ((hs_ep->send_zlp) ? DEV_DMA_SHORT : 0); > - if (len > maxsize) > - dev_err(hsotg->dev, "wrong len %d\n", len); > + (*desc)->status |= (len % mps) ? DEV_DMA_SHORT : > + ((hs_ep->send_zlp && true_last) ? > + DEV_DMA_SHORT : 0); > > - desc->status |= > + (*desc)->status |= > len << DEV_DMA_NBYTES_SHIFT & mask; > - desc->buf = dma_buff + offset; > + (*desc)->buf = dma_buff + offset; > } > > - desc->status &= ~DEV_DMA_BUFF_STS_MASK; > - desc->status |= (DEV_DMA_BUFF_STS_HREADY > + (*desc)->status &= ~DEV_DMA_BUFF_STS_MASK; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HREADY > << DEV_DMA_BUFF_STS_SHIFT); > - desc++; > + (*desc)++; > } > } > > +/* > + * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > + * @hs_ep: The endpoint > + * @ureq: Request to transfer > + * @offset: offset in bytes > + * @len: Length of the transfer > + * > + * This function will iterate over descriptor chain and fill its entries > + * with corresponding information based on transfer data. > + */ > +static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > + dma_addr_t dma_buff, > + unsigned int len) > +{ > + struct usb_request *ureq = NULL; > + struct dwc2_dma_desc *desc = hs_ep->desc_list; > + struct scatterlist *sg; > + int i; > + u8 desc_count = 0; > + > + if (hs_ep->req) > + ureq = &hs_ep->req->req; > + > + /* non-DMA sg buffer */ > + if (!ureq || !ureq->num_sgs) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + dma_buff, len, true); > + return; > + } > + > + /* DMA sg buffer */ > + for_each_sg(ureq->sg, sg, ureq->num_sgs, i) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + sg_dma_address(sg) + sg->offset, sg_dma_len(sg), > + sg_is_last(sg)); > + desc_count += hs_ep->desc_count; > + } > + > + hs_ep->desc_count = desc_count; > +} > + > /* > * dwc2_gadget_fill_isoc_desc - fills next isochronous descriptor in chain. > * @hs_ep: The isochronous endpoint. > @@ -944,7 +976,13 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep) > > hs_ep->next_desc = 0; > list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) { > - ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + ret = dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > if (ret) > break; > @@ -1399,7 +1437,13 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, > */ > if (using_desc_dma(hs) && hs_ep->isochronous) { > if (hs_ep->target_frame != TARGET_FRAME_INITIAL) { > - dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > } > return 0; > @@ -4386,6 +4430,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, > hsotg->enabled = 0; > spin_unlock_irqrestore(&hsotg->lock, flags); > > + gadget->sg_supported = using_desc_dma(hsotg); > dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); > > return 0; >
Hi Minas, W dniu 01.04.2019 o 12:01, Minas Harutyunyan pisze: > >> @Minas: can you please test with this patch instead of the original one? >> > Tested new version. No issue seen by running USB CV (Ch9, MSC) tests. > Could you please elaborate which else tests you performed? Only those reported by you to be failing, that is MSC. Andrzej
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index e15d8a462085..e76b2e040b4c 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -768,22 +768,13 @@ static u32 dwc2_gadget_get_desc_params(struct dwc2_hsotg_ep *hs_ep, u32 *mask) return desc_size; } -/* - * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. - * @hs_ep: The endpoint - * @dma_buff: DMA address to use - * @len: Length of the transfer - * - * This function will iterate over descriptor chain and fill its entries - * with corresponding information based on transfer data. - */ -static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, +static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, + struct dwc2_dma_desc **desc, dma_addr_t dma_buff, - unsigned int len) + unsigned int len, + bool true_last) { - struct dwc2_hsotg *hsotg = hs_ep->parent; int dir_in = hs_ep->dir_in; - struct dwc2_dma_desc *desc = hs_ep->desc_list; u32 mps = hs_ep->ep.maxpacket; u32 maxsize = 0; u32 offset = 0; @@ -798,41 +789,82 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, hs_ep->desc_count = 1; for (i = 0; i < hs_ep->desc_count; ++i) { - desc->status = 0; - desc->status |= (DEV_DMA_BUFF_STS_HBUSY + (*desc)->status = 0; + (*desc)->status |= (DEV_DMA_BUFF_STS_HBUSY << DEV_DMA_BUFF_STS_SHIFT); if (len > maxsize) { if (!hs_ep->index && !dir_in) - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); - desc->status |= (maxsize << - DEV_DMA_NBYTES_SHIFT & mask); - desc->buf = dma_buff + offset; + (*desc)->status |= + maxsize << DEV_DMA_NBYTES_SHIFT & mask; + (*desc)->buf = dma_buff + offset; len -= maxsize; offset += maxsize; } else { - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); + if (true_last) + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); if (dir_in) - desc->status |= (len % mps) ? DEV_DMA_SHORT : - ((hs_ep->send_zlp) ? DEV_DMA_SHORT : 0); - if (len > maxsize) - dev_err(hsotg->dev, "wrong len %d\n", len); + (*desc)->status |= (len % mps) ? DEV_DMA_SHORT : + ((hs_ep->send_zlp && true_last) ? + DEV_DMA_SHORT : 0); - desc->status |= + (*desc)->status |= len << DEV_DMA_NBYTES_SHIFT & mask; - desc->buf = dma_buff + offset; + (*desc)->buf = dma_buff + offset; } - desc->status &= ~DEV_DMA_BUFF_STS_MASK; - desc->status |= (DEV_DMA_BUFF_STS_HREADY + (*desc)->status &= ~DEV_DMA_BUFF_STS_MASK; + (*desc)->status |= (DEV_DMA_BUFF_STS_HREADY << DEV_DMA_BUFF_STS_SHIFT); - desc++; + (*desc)++; } } +/* + * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. + * @hs_ep: The endpoint + * @ureq: Request to transfer + * @offset: offset in bytes + * @len: Length of the transfer + * + * This function will iterate over descriptor chain and fill its entries + * with corresponding information based on transfer data. + */ +static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, + dma_addr_t dma_buff, + unsigned int len) +{ + struct usb_request *ureq = NULL; + struct dwc2_dma_desc *desc = hs_ep->desc_list; + struct scatterlist *sg; + int i; + u8 desc_count = 0; + + if (hs_ep->req) + ureq = &hs_ep->req->req; + + /* non-DMA sg buffer */ + if (!ureq || !ureq->num_sgs) { + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, + dma_buff, len, true); + return; + } + + /* DMA sg buffer */ + for_each_sg(ureq->sg, sg, ureq->num_sgs, i) { + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, + sg_dma_address(sg) + sg->offset, sg_dma_len(sg), + sg_is_last(sg)); + desc_count += hs_ep->desc_count; + } + + hs_ep->desc_count = desc_count; +} + /* * dwc2_gadget_fill_isoc_desc - fills next isochronous descriptor in chain. * @hs_ep: The isochronous endpoint. @@ -944,7 +976,13 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep) hs_ep->next_desc = 0; list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) { - ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, + dma_addr_t dma_addr = hs_req->req.dma; + + if (hs_req->req.num_sgs) { + WARN_ON(hs_req->req.num_sgs > 1); + dma_addr = sg_dma_address(hs_req->req.sg); + } + ret = dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, hs_req->req.length); if (ret) break; @@ -1399,7 +1437,13 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, */ if (using_desc_dma(hs) && hs_ep->isochronous) { if (hs_ep->target_frame != TARGET_FRAME_INITIAL) { - dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, + dma_addr_t dma_addr = hs_req->req.dma; + + if (hs_req->req.num_sgs) { + WARN_ON(hs_req->req.num_sgs > 1); + dma_addr = sg_dma_address(hs_req->req.sg); + } + dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, hs_req->req.length); } return 0; @@ -4386,6 +4430,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, hsotg->enabled = 0; spin_unlock_irqrestore(&hsotg->lock, flags); + gadget->sg_supported = using_desc_dma(hsotg); dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); return 0;