Message-ID: <20181122151236.GA9611@n2100.armlinux.org.uk>
Date:   Thu, 22 Nov 2018 15:12:36 +0000
From:   Russell King - ARM Linux <linux@...linux.org.uk>
To:     Aaro Koskinen <aaro.koskinen@....fi>
Cc:     Peter Ujfalusi <peter.ujfalusi@...com>, vkoul@...nel.org,
        dan.j.williams@...el.com, dmaengine@...r.kernel.org,
        linux-kernel@...r.kernel.org, tony@...mide.com,
        linux-omap@...r.kernel.org
Subject: Re: [PATCH] dmaengine: ti: omap-dma: Configure LCH_TYPE for OMAP1

On Thu, Nov 22, 2018 at 10:29:48AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 20, 2018 at 11:04:06PM +0200, Aaro Koskinen wrote:
> > I had switched to PIO mode in 2015 since the WARNs about legacy DMA
> > API were too annoying and flooding the console. And now that I tried
> > using DMA again with g_ether, it doesn't work anymore. The device get's
> > recognized on host side, but no traffic goes through. Switching back to
> > PIO makes it to work again.
> 
> A solution to that would be to do what the warning message says, and
> update the driver to the DMAengine API.

Here's a partial conversion (not even build tested) - it only supports
OUT transfers with dmaengine at the moment.

There's at least one thing that doesn't make sense - the driver
apparently can transfer more than req->length bytes, surely if it does
that, it's a serious problem - shouldn't it be noisy about that?

Also we can't deal with the omap_set_dma_dest_burst_mode() setting -
DMAengine always uses a 64 byte burst, but udc wants a smaller burst
setting.  Does this matter?

I've kept the old code for reference (and the driver will fall back if
we can't get a dmaengine channel.)  I haven't been through and checked
that we result in the channel setup largely the same either.

There will be one major difference - UDC uses element sync, where
an element is 16bits, ep->ep.maxpacket/2 in a frame, and "packets"
frames.  DMAengine is using frame sync, with a 16bit element, one
element in a frame, and packets*ep->ep.maxpacket/2 frames.  This
should be functionally equivalent but I'd like confirmation of that.

I'm also not sure about this:

        if (cpu_is_omap15xx())
                end++;

in dma_dest_len() - is that missing from the omap-dma driver?  It looks
like a work-around for some problem on OMAP15xx, but I can't make sense
about why it's in the UDC driver rather than the legacy DMA driver.

I'm also confused by:

        end |= start & (0xffff << 16);

also in dma_dest_len() - omap_get_dma_dst_pos() returns in the high 16
bits the full address:

        if (dma_omap1())
                offset |= (p->dma_read(CDSA, lch) & 0xFFFF0000);

so if the address crosses a 64k physical address boundary, surely orring
in the start address is wrong?  The more I look at dma_dest_len(), the
more I wonder whether that and dma_src_len() are anywhere near correct,
and whether that is a source of breakage for Aaro.

As I've already said, I've no way to test this on hardware.

Please review and let me know whether I missed anything on the OUT
handling path.

Fixing the IN path is going to be a bit more head-scratching.

 drivers/usb/gadget/udc/omap_udc.c | 154 +++++++++++++++++++++++++++++---------
 drivers/usb/gadget/udc/omap_udc.h |   2 +
 2 files changed, 120 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/gadget/udc/omap_udc.c b/drivers/usb/gadget/udc/omap_udc.c
index 3a16431da321..a37e1d2f0f3e 100644
--- a/drivers/usb/gadget/udc/omap_udc.c
+++ b/drivers/usb/gadget/udc/omap_udc.c
@@ -204,6 +204,7 @@ static int omap_ep_enable(struct usb_ep *_ep,
 	ep->dma_channel = 0;
 	ep->has_dma = 0;
 	ep->lch = -1;
+	ep->dma = NULL;
 	use_ep(ep, UDC_EP_SEL);
 	omap_writew(udc->clr_halt, UDC_CTRL);
 	ep->ackwait = 0;
@@ -576,21 +577,49 @@ static void finish_in_dma(struct omap_ep *ep, struct omap_req *req, int status)
 static void next_out_dma(struct omap_ep *ep, struct omap_req *req)
 {
 	unsigned packets = req->req.length - req->req.actual;
-	int dma_trigger = 0;
+	struct dma_async_tx_descriptor *tx;
+	struct dma_chan *dma = ep->dma;
+	dma_cookie_t cookie;
 	u16 w;
 
-	/* set up this DMA transfer, enable the fifo, start */
-	packets /= ep->ep.maxpacket;
-	packets = min(packets, (unsigned)UDC_RXN_TC + 1);
+	packets = min_t(unsigned, packets / ep->ep.maxpacket, UDC_RXN_TC + 1);
 	req->dma_bytes = packets * ep->ep.maxpacket;
-	omap_set_dma_transfer_params(ep->lch, OMAP_DMA_DATA_TYPE_S16,
-			ep->ep.maxpacket >> 1, packets,
-			OMAP_DMA_SYNC_ELEMENT,
-			dma_trigger, 0);
-	omap_set_dma_dest_params(ep->lch, OMAP_DMA_PORT_EMIFF,
-		OMAP_DMA_AMODE_POST_INC, req->req.dma + req->req.actual,
-		0, 0);
-	ep->dma_counter = omap_get_dma_dst_pos(ep->lch);
+
+	if (dma) {
+		struct dma_slave_config cfg = {
+			.direction = DMA_DEV_TO_MEM,
+			.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTE,
+			/*
+			 * DMAengine uses frame sync mode, setting maxburst=1
+			 * is equivalent to element sync mode.
+			 */
+			.src_maxburst = 1,
+			.src_addr = UDC_DATA_DMA,
+		};
+
+		if (WARN_ON(dmaengine_slave_config(dma, &cfg)))
+			return;
+
+		tx = dmaengine_prep_slave_single(dma,
+						 req->req.dma + req->req.actual,
+						 req->dma_bytes,
+						 DMA_DEV_TO_MEM, 0);
+		if (WARN_ON(!tx))
+			return;
+	} else {
+		int dma_trigger = 0;
+
+		/* set up this DMA transfer, enable the fifo, start */
+		/* dt = S16, cen = ep->ep.maxpacket / 2, cfn = packets */
+		omap_set_dma_transfer_params(ep->lch, OMAP_DMA_DATA_TYPE_S16,
+				ep->ep.maxpacket >> 1, packets,
+				OMAP_DMA_SYNC_ELEMENT,
+				dma_trigger, 0);
+		omap_set_dma_dest_params(ep->lch, OMAP_DMA_PORT_EMIFF,
+			OMAP_DMA_AMODE_POST_INC, req->req.dma + req->req.actual,
+			0, 0);
+		ep->dma_counter = omap_get_dma_dst_pos(ep->lch);
+	}
 
 	omap_writew(UDC_RXN_STOP | (packets - 1), UDC_RXDMA(ep->dma_channel));
 	w = omap_readw(UDC_DMA_IRQ_EN);
@@ -599,7 +628,15 @@ static void next_out_dma(struct omap_ep *ep, struct omap_req *req)
 	omap_writew(ep->bEndpointAddress & 0xf, UDC_EP_NUM);
 	omap_writew(UDC_SET_FIFO_EN, UDC_CTRL);
 
-	omap_start_dma(ep->lch);
+	if (dma) {
+		cookie = dmaengine_submit(tx);
+		if (WARN_ON(dma_submit_error(cookie)))
+			return;
+		ep->dma_cookie = cookie;
+		dma_async_issue_pending(dma);
+	} else {
+		omap_start_dma(ep->lch);
+	}
 }
 
 static void
@@ -607,21 +644,39 @@ finish_out_dma(struct omap_ep *ep, struct omap_req *req, int status, int one)
 {
 	u16	count, w;
 
-	if (status == 0)
-		ep->dma_counter = (u16) (req->req.dma + req->req.actual);
-	count = dma_dest_len(ep, req->req.dma + req->req.actual);
+	if (ep->dma) {
+		struct dma_tx_state state;
+
+		dmaengine_tx_status(ep->dma, ep->dma_cookie, &state);
+
+		count = req->dma_bytes - state.residual;
+	} else {
+		if (status == 0)
+			ep->dma_counter = (u16) (req->req.dma + req->req.actual);
+		count = dma_dest_len(ep, req->req.dma + req->req.actual);
+	}
+
 	count += req->req.actual;
 	if (one)
 		count--;
+
+	/*
+	 * FIXME: Surely if count > req->req.length, something has gone
+	 * seriously wrong and we've scribbled over memory we should not...
+	 * so surely we should be a WARN_ON() at the very least?
+	 */
 	if (count <= req->req.length)
 		req->req.actual = count;
 
-	if (count != req->dma_bytes || status)
-		omap_stop_dma(ep->lch);
-
+	if (count != req->dma_bytes || status) {
+		if (ep->dma)
+			dmaengine_terminate_async(ep->dma);
+		else
+			omap_stop_dma(ep->lch);
 	/* if this wasn't short, request may need another transfer */
-	else if (req->req.actual < req->req.length)
+	} else if (req->req.actual < req->req.length) {
 		return;
+	}
 
 	/* rx completion */
 	w = omap_readw(UDC_DMA_IRQ_EN);
@@ -709,6 +764,7 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
 
 	ep->dma_channel = 0;
 	ep->lch = -1;
+	ep->dma = NULL;
 	if (channel == 0 || channel > 3) {
 		if ((reg & 0x0f00) == 0)
 			channel = 3;
@@ -742,26 +798,44 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
 				0, 0);
 		}
 	} else {
+		struct dma_chan *dma;
+
 		dma_channel = OMAP_DMA_USB_W2FC_RX0 - 1 + channel;
-		status = omap_request_dma(dma_channel,
-			ep->ep.name, dma_error, ep, &ep->lch);
-		if (status == 0) {
+
+		dma = __dma_request_channel(&mask, omap_dma_filter_fn,
+					    (void *)dma_channel);
+		if (dma) {
+			ep->dma = dma;
 			omap_writew(reg, UDC_RXDMA_CFG);
-			/* TIPB */
-			omap_set_dma_src_params(ep->lch,
-				OMAP_DMA_PORT_TIPB,
-				OMAP_DMA_AMODE_CONSTANT,
-				UDC_DATA_DMA,
-				0, 0);
-			/* EMIFF or SDRC */
-			omap_set_dma_dest_burst_mode(ep->lch,
-						OMAP_DMA_DATA_BURST_4);
-			omap_set_dma_dest_data_pack(ep->lch, 1);
+		} else {
+			status = omap_request_dma(dma_channel,
+				ep->ep.name, dma_error, ep, &ep->lch);
+			if (status == 0) {
+				omap_writew(reg, UDC_RXDMA_CFG);
+				/* TIPB */
+				omap_set_dma_src_params(ep->lch,
+					OMAP_DMA_PORT_TIPB,
+					OMAP_DMA_AMODE_CONSTANT,
+					UDC_DATA_DMA,
+					0, 0);
+				/* EMIFF or SDRC */
+				/*
+				 * not ok - CSDP_DST_BURST_64 selected, but this selects
+				 * CSDP_DST_BURST_16 on omap2+ and CSDP_DST_BURST_32 on
+				 * omap1.
+				 */
+				omap_set_dma_dest_burst_mode(ep->lch,
+							OMAP_DMA_DATA_BURST_4);
+				/* ok - CSDP_DST_PACKED set for dmaengine */
+				omap_set_dma_dest_data_pack(ep->lch, 1);
+			}
 		}
 	}
-	if (status)
+	if (d->dma) {
+		ep->has_dma = 1;
+	} else if (status) {
 		ep->dma_channel = 0;
-	else {
+	} else {
 		ep->has_dma = 1;
 		omap_disable_dma_irq(ep->lch, OMAP_DMA_BLOCK_IRQ);
 
@@ -777,6 +851,10 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
 	if (status)
 		DBG("%s no dma channel: %d%s\n", ep->ep.name, status,
 			restart ? " (restart)" : "");
+	else if (d->dma)
+		DBG("%s claimed %cxdma%d dmaengine %s%s\n", ep->ep.name,
+			is_in ? 't' : 'r', ep->dma_channel - 1,
+			dma_chan_name(d->dma), restart ? " (restart)" : "");
 	else
 		DBG("%s claimed %cxdma%d lch %d%s\n", ep->ep.name,
 			is_in ? 't' : 'r',
@@ -850,9 +928,13 @@ static void dma_channel_release(struct omap_ep *ep)
 		if (req)
 			finish_out_dma(ep, req, -ECONNRESET, 0);
 	}
-	omap_free_dma(ep->lch);
+	if (ep->dma)
+		dma_release_channel(ep->dma);
+	else
+		omap_free_dma(ep->lch);
 	ep->dma_channel = 0;
 	ep->lch = -1;
+	ep->dma = NULL;
 	/* has_dma still set, till endpoint is fully quiesced */
 }
 
diff --git a/drivers/usb/gadget/udc/omap_udc.h b/drivers/usb/gadget/udc/omap_udc.h
index 00f9e608e755..68857ae8d763 100644
--- a/drivers/usb/gadget/udc/omap_udc.h
+++ b/drivers/usb/gadget/udc/omap_udc.h
@@ -153,6 +153,8 @@ struct omap_ep {
 	u8				dma_channel;
 	u16				dma_counter;
 	int				lch;
+	struct dma_chan			*dma;
+	dma_cookie_t			dma_cookie;
 	struct omap_udc			*udc;
 	struct timer_list		timer;
 };

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Powered by blists - more mailing lists