* Re: [PATCH] ARM: PL011: Add support for Rx DMA buffer polling [not found] <50efad8d.84fc440a.589e.ffff9546SMTPIN_ADDED_BROKEN@mx.google.com> @ 2013-01-11 18:34 ` Linus Walleij [not found] ` <50f107aa.894e420a.596a.ffffe3f6SMTPIN_ADDED_BROKEN@mx.google.com> 0 siblings, 1 reply; 19+ messages in thread From: Linus Walleij @ 2013-01-11 18:34 UTC (permalink / raw) To: Chanho Min Cc: Alan Cox, Russell King, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll On Fri, Jan 11, 2013 at 7:13 AM, Chanho Min <chanho.min@lge.com> wrote: > In DMA support, Received data is not triggered until the DMA buffer is filled. Interesting! As you can see in commits: commit ead76f329f777c7301e0a5456a0a1c7a081570bd "ARM: 6763/1: pl011: add optional RX DMA to PL011 v2" commit 5d7b8467e18b14ed44c5781d77993bfdcd8c826b "mach-ux500: config Ux500 PL011 PL022 PL180 for DMA" commit ec8f12533b4a439a8feb0d1a3bf15516781804be "mach-u300: config U300 PL180 PL011 PL022 for DMA" We have enabled RX DMA for Ux500 and U300. It does work on our machines, I'm using it every day and deploying the ux500 systems in the millions. Let's recap how this can work for us. As you can see, when using DMA we turn off UART011_RXIM and instead rely on either: 1) DMA completes and we read in the entire buffer to the TTY or 2) UART011_RTIM (recieve timeout) to happen. This is a timeout that need to be set low enough so that interactivity on a console will not be hampered. Apparently this timeout cannot be configured on the ARM variants while the ST variant has a register for that (which is currently unused.) The amazing thing is that we're using this. We're getting timeouts, so the small set of characters in the FIFO on an interactive console is polled perfectly. This even works on the U300, which has the ARM version of the PL011 and I have once tested it on the RealView PB11MPcore (the only reference design with a working PL08x DMA controller) and it worked on that system as well. As we've discussed before, in order for this to work, the PL011 refman says: ----------8<------------------8<----------------- UARTRXDMASREQ Single character DMA transfer request, asserted by the UART. For receive, one character consists of up to 12 bits. This signal is asserted when the receive FIFO contains at least one character. UARTRXDMABREQ Burst DMA transfer request, asserted by the UART. This signal is asserted when the receive FIFO contains more characters than the programmed watermark level. You can program the watermark level for each FIFO through the UARTIFLS register. UARTRXDMACLR DMA request clear, asserted by the DMA controller to clear the receive request signals. If DMA burst transfer is requested, the clear signal is asserted during the transfer of the last data in the burst. ----------8<------------------8<----------------- We were worried that DMA would suck out characters of the FIFO so quickly that it would never contain the one character necessary to trigger the timeout, so that we would read out the data from the DMA page, due to this part of the refman: ----------8<------------------8<----------------- UARTRTINTR The receive timeout interrupt is asserted when the receive FIFO is not empty, and no further data is received over a 32-bit period. The receive timeout interrupt is cleared either when the FIFO becomes empty through reading all the data (or by reading the holding register), or when a 1 is written to the corresponding bit of the UARTICR register. ----------8<------------------8<----------------- The crucial part is "FIFO is not empty" - when using DMA the FIFO will of course be empty, as DMA is sucking out anything immediately. See this discussion: http://comments.gmane.org/gmane.linux.ports.arm.kernel/92513 But when I tried it out in practice, it must be wired differently or contain some VHDL hacks, because it worked like a charm. I got the timeout IRQs and all. I never managed to get it to fail. Sadly I don't have the VHDL code for the PL011 so I cannot confirm how they (ARMs IP core engineers) solved this. (Paging Pawel on this.) So I suspect that maybe your DMA controller is badly configured, or erroneous, atleast if this is a stock PL011: - What DMA controller are you using with the PL011? - How are the burst lines configured in the DMA controller? (This is often a platform-specific property.) Did you make sure both single and burst request lines are enabled? If only burst is enabled you will get *exactly* the phenomena you are trying to work around above... - If they cannot be software-configured, how are they then configured in hardware? Burst-only? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <50f107aa.894e420a.596a.ffffe3f6SMTPIN_ADDED_BROKEN@mx.google.com>]
* Re: [PATCH] ARM: PL011: Add support for Rx DMA buffer polling [not found] ` <50f107aa.894e420a.596a.ffffe3f6SMTPIN_ADDED_BROKEN@mx.google.com> @ 2013-01-14 0:04 ` Linus Walleij 2013-01-14 0:26 ` Russell King - ARM Linux 0 siblings, 1 reply; 19+ messages in thread From: Linus Walleij @ 2013-01-14 0:04 UTC (permalink / raw) To: Chanho Min Cc: Alan Cox, Russell King, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll, chanho0207 On Sat, Jan 12, 2013 at 7:50 AM, Chanho Min <chanho.min@lge.com> wrote: > We tested as follows: > We set baud rate to 3Mbps and transfer 5000 bytes via /dev/tty at > a time. Then 4096 bytes is received by DMA irq. But, There is no way > to get remaining 904 bytes, unless another 3192 bytes are coming, OK clearly cut phenomena: IRQs seem to appear from the PL080 TC (transfer complete) IRQ only, not from any of the PL011 IRQs. So there will be zero IRQs from the uart in /proc/interrupts but a few of them from the PL080, correct? >>- What DMA controller are you using with the >> PL011? > > PL080. > We don't add any customized configuration from the mainline. OK sorry I was thinking of the old times when we has .cctl configured from the platform. >> - If they cannot be software-configured, how are >> they then configured in hardware? Burst-only? >> Burst single > > As far as I know, single request is not disabled at PL080. > Could you explain what single request has to do with this? At some point we reasoned that we may actually be saved by the reverse phenomena - if single request is *NOT* connected, there will very often be some characters in the FIFO, and that's enough to trigger the RTIM IRQ, and we get a nice flow. So maybe the platforms that actually work, work because they have bursts only. But that would pose a problem when an even burst of characters arrive. Say 16 characters, this will be transferred in two bursts and leave the FIFO empty, but no DMA TC IRQ since the buffer is not full, and we'd get your phenomena. But I could never reproduce the latter theoretical problem, so I think the hardware actually has a fix for this, around the burst signal logic. But it may very well be that the single request can not be enabled for the PL011 for it to work properly. You may not be able to turn this off, and in that case this kind of code is the only way forward indeed. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-14 0:04 ` Linus Walleij @ 2013-01-14 0:26 ` Russell King - ARM Linux 2013-01-14 6:46 ` Linus Walleij 0 siblings, 1 reply; 19+ messages in thread From: Russell King - ARM Linux @ 2013-01-14 0:26 UTC (permalink / raw) To: Linus Walleij Cc: Chanho Min, Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll, chanho0207 On Mon, Jan 14, 2013 at 01:04:58AM +0100, Linus Walleij wrote: > At some point we reasoned that we may actually be saved > by the reverse phenomena - if single request is *NOT* > connected, there will very often be some characters in the > FIFO, and that's enough to trigger the RTIM IRQ, and we get > a nice flow. So maybe the platforms that actually work, work > because they have bursts only. > > But that would pose a problem when an even burst of > characters arrive. Say 16 characters, this will be transferred > in two bursts and leave the FIFO empty, but no DMA TC > IRQ since the buffer is not full, and we'd get your > phenomena. > > But I could never reproduce the latter theoretical problem, > so I think the hardware actually has a fix for this, around > the burst signal logic. > > But it may very well be that the single request can not be > enabled for the PL011 for it to work properly. What you describe above is exactly the problem I see on the Versatile platform with it's PL080 and PL011. I made the comment that this setup can't work properly as it stands. And I don't think that setting a timer which fires every jiffy count is really a solution to the problem. That's an expensive way of having a per-jiffy callback for something that's probably going to remain idle most of the time. Not only that, but a 1 jiffy timeout even for slower baud rates is just utterly silly. The point of using DMA is to move the workload off the CPU onto hardware, so that the CPU can go off and do other stuff. If it's having to poll it once every jiffy, then... > You may not be able to turn this off, and in that case this > kind of code is the only way forward indeed. Unless there's something inbetween the PL080 and PL011 to do that. The peripherals themselves don't have any means of controlling this. The solution I came up with on Versatile was to just not enable receive DMA - at all. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-14 0:26 ` Russell King - ARM Linux @ 2013-01-14 6:46 ` Linus Walleij 2013-01-14 7:27 ` Chanho Min 0 siblings, 1 reply; 19+ messages in thread From: Linus Walleij @ 2013-01-14 6:46 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Chanho Min, Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll, chanho0207 On Mon, Jan 14, 2013 at 1:26 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > [Me] >> But it may very well be that the single request can not be >> enabled for the PL011 for it to work properly. > > What you describe above is exactly the problem I see on the Versatile > platform with it's PL080 and PL011. I made the comment that this setup > can't work properly as it stands. And I don't think that setting a > timer which fires every jiffy count is really a solution to the problem. > That's an expensive way of having a per-jiffy callback for something > that's probably going to remain idle most of the time. OK then that's what we can expect from an unmodified PL011. > Not only that, but a 1 jiffy timeout even for slower baud rates is > just utterly silly. > > The point of using DMA is to move the workload off the CPU onto hardware, > so that the CPU can go off and do other stuff. If it's having to poll > it once every jiffy, then... Agreed. Chanho, have you considered just leaving RX DMA undefined and only use DMA for TX? I know sometimes a programmer will get assigned to "implement DMA for IP block <foo>", and usually they want to deliver, but sometimes you have to come back and say "you know that is not such a good idea". >> You may not be able to turn this off, and in that case this >> kind of code is the only way forward indeed. > > Unless there's something inbetween the PL080 and PL011 to do that. > The peripherals themselves don't have any means of controlling this. We have it working perfectly on U300 and Ux500 but I think it must be thanks to some hardware modifications. On the Ux500 it is strictly required since it is used for megabit rate chips like Bluetooth, and we get successful DMA support on these. > The solution I came up with on Versatile was to just not enable > receive DMA - at all. Then I suspect I can reproduce it with the PB11MPCore and PL081 also given some time. I probably just did the tests the wrong way. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-14 6:46 ` Linus Walleij @ 2013-01-14 7:27 ` Chanho Min 2013-01-14 7:56 ` Linus Walleij 2013-01-14 8:41 ` Chanho Min 0 siblings, 2 replies; 19+ messages in thread From: Chanho Min @ 2013-01-14 7:27 UTC (permalink / raw) To: Linus Walleij Cc: Russell King - ARM Linux, Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll On Mon, Jan 14, 2013 at 3:46 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jan 14, 2013 at 1:26 AM, Russell King - ARM Linux >> What you describe above is exactly the problem I see on the Versatile >> platform with it's PL080 and PL011. I made the comment that this setup >> can't work properly as it stands. And I don't think that setting a >> timer which fires every jiffy count is really a solution to the problem. >> That's an expensive way of having a per-jiffy callback for something >> that's probably going to remain idle most of the time. > > OK then that's what we can expect from an unmodified PL011. > >> Not only that, but a 1 jiffy timeout even for slower baud rates is >> just utterly silly. >> >> The point of using DMA is to move the workload off the CPU onto hardware, >> so that the CPU can go off and do other stuff. If it's having to poll >> it once every jiffy, then... > > Agreed. I agree also. So We hope to suggest a way to reduce such cost. Rx DMA is required for the fast baud only. PIO is sufficient for the slow baud rate. So this feature is optional for the specific port. 1 jiffy timeout is experimental and adjustable. Even 100ms, It will be better than remain in the DMA buffer. Also, the polling interval can be adjusted according to baud rate as Russel mentioned. > Chanho, have you considered just leaving RX DMA undefined > and only use DMA for TX? No, We need RX DMA to reduce data loss rather than TX DMA. >> The solution I came up with on Versatile was to just not enable >> receive DMA - at all. We just want to use this HW function even if SW support is needed. Anyway, It was very useful for megabyte rate chips. Thanks Chanho ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-14 7:27 ` Chanho Min @ 2013-01-14 7:56 ` Linus Walleij 2013-01-14 8:41 ` Chanho Min 1 sibling, 0 replies; 19+ messages in thread From: Linus Walleij @ 2013-01-14 7:56 UTC (permalink / raw) To: Chanho Min Cc: Russell King - ARM Linux, Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll On Mon, Jan 14, 2013 at 8:27 AM, Chanho Min <chanho.min@lge.com> wrote: > We just want to use this HW function even if SW support is needed. > Anyway, It was very useful for megabyte rate chips. So RX DMA will not be enabled for things like console, but accelerators etc. OK that's perfectly valid. Looking back at the patch, do you know if we can get rid of some of the #ifdefs and have this like a runtime switch set in struct amba_pl011_data or Device Tree? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-14 7:27 ` Chanho Min 2013-01-14 7:56 ` Linus Walleij @ 2013-01-14 8:41 ` Chanho Min 2013-01-22 12:45 ` [PATCH v2] " Chanho Min 1 sibling, 1 reply; 19+ messages in thread From: Chanho Min @ 2013-01-14 8:41 UTC (permalink / raw) To: Linus Walleij Cc: Russell King - ARM Linux, Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll, Chanho Min >Looking back at the patch, do you know if we can get rid of >some of the #ifdefs and have this like a runtime switch set in >struct amba_pl011_data or Device Tree? Yes, I know. I'll come back with v2 soon. Thanks, Chanho ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-14 8:41 ` Chanho Min @ 2013-01-22 12:45 ` Chanho Min 2013-01-22 13:18 ` Linus Walleij 0 siblings, 1 reply; 19+ messages in thread From: Chanho Min @ 2013-01-22 12:45 UTC (permalink / raw) To: Linus Walleij, Russell King - ARM Linux Cc: Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll, Chanho Min In DMA support, The received data is not pushed to tty until the DMA buffer is filled. But some megabyte rate chips such as BT expect fast response and data should be pushed immediately. In order to fix this issue, We suggest the use of the timer for polling DMA buffer. In our test, no data loss occurred at high-baudrate as compared with interrupt- driven (We tested with 3Mbps). We changes: - We add timer for polling. If we set poll_timer to 10, every 10ms, timer handler checks the residue in the dma buffer and transfer data to the tty. Also, last_residue is updated for the next polling. - poll_timeout is used to prevent the timer's system cost. If poll_timeout is set to 3000 and no data is received in 3 seconds, we inactivate poll timer and driver falls back to interrupt-driven. When data is received again in FIFO and UART irq is occurred, we switch back to DMA mode and start polling. - If poll timer is activated, We use consistent DMA mappings to avoid from the frequent cache operation of the timer function. sg->coherency is used to check if buffer is coherent. - pl011_dma_rx_chars is modified. the pending size is recalculated because data can be taken by polling. Signed-off-by: Chanho Min <chanho.min@lge.com> --- drivers/tty/serial/amba-pl011.c | 160 ++++++++++++++++++++++++++++++++++----- include/linux/amba/serial.h | 2 + 2 files changed, 145 insertions(+), 17 deletions(-) diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c index 7fca402..0bcd5b5 100644 --- a/drivers/tty/serial/amba-pl011.c +++ b/drivers/tty/serial/amba-pl011.c @@ -107,6 +107,7 @@ static struct uart_amba_port *amba_ports[UART_NR]; struct pl011_sgbuf { struct scatterlist sg; char *buf; + bool coherency; }; struct pl011_dmarx_data { @@ -117,6 +118,11 @@ struct pl011_dmarx_data { struct pl011_sgbuf sgbuf_b; dma_cookie_t cookie; bool running; + struct timer_list timer; + unsigned int last_residue; + unsigned long last_jiffies; + unsigned int poll_rate; + unsigned int poll_timeout; }; struct pl011_dmatx_data { @@ -223,15 +229,29 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap) static int pl011_sgbuf_init(struct dma_chan *chan, struct pl011_sgbuf *sg, enum dma_data_direction dir) { - sg->buf = kmalloc(PL011_DMA_BUFFER_SIZE, GFP_KERNEL); - if (!sg->buf) - return -ENOMEM; + if (sg->coherency) { + dma_addr_t dma_addr; - sg_init_one(&sg->sg, sg->buf, PL011_DMA_BUFFER_SIZE); + sg->buf = dma_alloc_coherent(chan->device->dev, + PL011_DMA_BUFFER_SIZE, &dma_addr, GFP_KERNEL); + if (!sg->buf) + return -ENOMEM; - if (dma_map_sg(chan->device->dev, &sg->sg, 1, dir) != 1) { - kfree(sg->buf); - return -EINVAL; + sg_init_table(&sg->sg, 1); + sg_set_page(&sg->sg, phys_to_page(dma_addr), + PL011_DMA_BUFFER_SIZE, offset_in_page(dma_addr)); + sg_dma_address(&sg->sg) = dma_addr; + } else { + sg->buf = kmalloc(PL011_DMA_BUFFER_SIZE, GFP_KERNEL); + if (!sg->buf) + return -ENOMEM; + + sg_init_one(&sg->sg, sg->buf, PL011_DMA_BUFFER_SIZE); + + if (dma_map_sg(chan->device->dev, &sg->sg, 1, dir) != 1) { + kfree(sg->buf); + return -EINVAL; + } } return 0; } @@ -240,8 +260,14 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg, enum dma_data_direction dir) { if (sg->buf) { - dma_unmap_sg(chan->device->dev, &sg->sg, 1, dir); - kfree(sg->buf); + if (sg->coherency) + dma_free_coherent(chan->device->dev, + PL011_DMA_BUFFER_SIZE, sg->buf, + sg_dma_address(&sg->sg)); + else { + dma_unmap_sg(chan->device->dev, &sg->sg, 1, dir); + kfree(sg->buf); + } } } @@ -300,6 +326,9 @@ static void pl011_dma_probe_initcall(struct uart_amba_port *uap) dmaengine_slave_config(chan, &rx_conf); uap->dmarx.chan = chan; + uap->dmarx.poll_rate = plat->dma_rx_poll_rate; + uap->dmarx.poll_timeout = plat->dma_rx_poll_timeout; + dev_info(uap->port.dev, "DMA channel RX %s\n", dma_chan_name(uap->dmarx.chan)); } @@ -705,10 +734,23 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, int dma_count = 0; u32 fifotaken = 0; /* only used for vdbg() */ - /* Pick everything from the DMA first */ + struct pl011_dmarx_data *dmarx = &uap->dmarx; + int dmataken = 0; + + if (uap->dmarx.poll_rate) { + /* The data can be taken by polling */ + dmataken = sgbuf->sg.length - dmarx->last_residue; + /* Recalculate the pending size */ + if (pending > dmataken) + pending -= dmataken; + } + + /* Pick the remain data from the DMA */ if (pending) { - /* Sync in buffer */ - dma_sync_sg_for_cpu(dev, &sgbuf->sg, 1, DMA_FROM_DEVICE); + /* Sync in buffer for non-coherent DMA */ + if (!sgbuf->coherency) + dma_sync_sg_for_cpu(dev, &sgbuf->sg, 1, + DMA_FROM_DEVICE); /* * First take all chars in the DMA pipe, then look in the FIFO. @@ -716,10 +758,12 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, * as it can. */ dma_count = tty_insert_flip_string(uap->port.state->port.tty, - sgbuf->buf, pending); + sgbuf->buf + dmataken, pending); - /* Return buffer to device */ - dma_sync_sg_for_device(dev, &sgbuf->sg, 1, DMA_FROM_DEVICE); + /* Return buffer to device for non-coherent DMA */ + if (!sgbuf->coherency) + dma_sync_sg_for_device(dev, &sgbuf->sg, 1, + DMA_FROM_DEVICE); uap->port.icount.rx += dma_count; if (dma_count < pending) @@ -727,6 +771,10 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, "couldn't insert all characters (TTY is full?)\n"); } + /* Reset the last_residue for Rx DMA poll */ + if (uap->dmarx.poll_rate) + dmarx->last_residue = sgbuf->sg.length; + /* * Only continue with trying to read the FIFO if all DMA chars have * been taken first. @@ -866,6 +914,59 @@ static inline void pl011_dma_rx_stop(struct uart_amba_port *uap) writew(uap->dmacr, uap->port.membase + UART011_DMACR); } +/* + * Timer handler for Rx DMA polling. + * Every polling, It checks the residue in the dma buffer and transfer + * data to the tty. Also, last_residue is updated for the next polling. + */ +static void pl011_dma_rx_poll(unsigned long args) +{ + struct uart_amba_port *uap = (struct uart_amba_port *)args; + struct pl011_dmarx_data *dmarx = &uap->dmarx; + struct dma_chan *rxchan = uap->dmarx.chan; + unsigned long flags = 0; + unsigned int dmataken = 0; + unsigned int size = 0; + struct pl011_sgbuf *sgbuf; + int dma_count; + struct dma_tx_state state; + + spin_lock(&uap->port.lock); + sgbuf = dmarx->use_buf_b ? &uap->dmarx.sgbuf_b : &uap->dmarx.sgbuf_a; + rxchan->device->device_tx_status(rxchan, dmarx->cookie, &state); + if (likely(state.residue < dmarx->last_residue)) { + dmataken = sgbuf->sg.length - dmarx->last_residue; + size = dmarx->last_residue - state.residue; + dma_count = tty_insert_flip_string(uap->port.state->port.tty, + sgbuf->buf + dmataken, size); + if (dma_count == size) + dmarx->last_residue = state.residue; + dmarx->last_jiffies = jiffies; + } + spin_unlock(&uap->port.lock); + + tty_flip_buffer_push(uap->port.state->port.tty); + + /* + * If no data is received in poll_timeout, the driver will fall back + * to interrupt mode. We will retrigger DMA at the first interrupt. + */ + if (jiffies_to_msecs(jiffies - dmarx->last_jiffies) + > uap->dmarx.poll_timeout) { + + spin_lock_irqsave(&uap->port.lock, flags); + pl011_dma_rx_stop(uap); + spin_unlock_irqrestore(&uap->port.lock, flags); + + uap->dmarx.running = false; + dmaengine_terminate_all(rxchan); + del_timer(&uap->dmarx.timer); + } else { + mod_timer(&uap->dmarx.timer, + jiffies + msecs_to_jiffies(uap->dmarx.poll_rate)); + } +} + static void pl011_dma_startup(struct uart_amba_port *uap) { int ret; @@ -889,6 +990,10 @@ static void pl011_dma_startup(struct uart_amba_port *uap) if (!uap->dmarx.chan) goto skip_rx; + /* Set coherency of the DMA RX buffer */ + uap->dmarx.sgbuf_a.coherency = uap->dmarx.sgbuf_b.coherency + = uap->dmarx.poll_rate ? true : false; + /* Allocate and map DMA RX buffers */ ret = pl011_sgbuf_init(uap->dmarx.chan, &uap->dmarx.sgbuf_a, DMA_FROM_DEVICE); @@ -928,6 +1033,16 @@ skip_rx: if (pl011_dma_rx_trigger_dma(uap)) dev_dbg(uap->port.dev, "could not trigger initial " "RX DMA job, fall back to interrupt mode\n"); + if (uap->dmarx.poll_rate) { + init_timer(&(uap->dmarx.timer)); + uap->dmarx.timer.function = pl011_dma_rx_poll; + uap->dmarx.timer.data = (unsigned long)uap; + mod_timer(&uap->dmarx.timer, + jiffies + + msecs_to_jiffies(uap->dmarx.poll_rate)); + uap->dmarx.last_residue = PL011_DMA_BUFFER_SIZE; + uap->dmarx.last_jiffies = jiffies; + } } } @@ -963,6 +1078,8 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap) /* Clean up the RX DMA */ pl011_sgbuf_free(uap->dmarx.chan, &uap->dmarx.sgbuf_a, DMA_FROM_DEVICE); pl011_sgbuf_free(uap->dmarx.chan, &uap->dmarx.sgbuf_b, DMA_FROM_DEVICE); + if (uap->dmarx.poll_rate) + del_timer_sync(&uap->dmarx.timer); uap->using_rx_dma = false; } } @@ -977,7 +1094,6 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap) return uap->using_rx_dma && uap->dmarx.running; } - #else /* Blank functions if the DMA engine is not available */ static inline void pl011_dma_probe(struct uart_amba_port *uap) @@ -1091,8 +1207,18 @@ static void pl011_rx_chars(struct uart_amba_port *uap) dev_dbg(uap->port.dev, "could not trigger RX DMA job " "fall back to interrupt mode again\n"); uap->im |= UART011_RXIM; - } else + } else { uap->im &= ~UART011_RXIM; + /* Start Rx DMA poll */ + if (uap->dmarx.poll_rate) { + uap->dmarx.last_jiffies = jiffies; + uap->dmarx.last_residue = PL011_DMA_BUFFER_SIZE; + mod_timer(&uap->dmarx.timer, + jiffies + + msecs_to_jiffies(uap->dmarx.poll_rate)); + } + } + writew(uap->im, uap->port.membase + UART011_IMSC); } spin_lock(&uap->port.lock); diff --git a/include/linux/amba/serial.h b/include/linux/amba/serial.h index f612c78..7501a5a 100644 --- a/include/linux/amba/serial.h +++ b/include/linux/amba/serial.h @@ -203,6 +203,8 @@ struct amba_pl011_data { bool (*dma_filter)(struct dma_chan *chan, void *filter_param); void *dma_rx_param; void *dma_tx_param; + unsigned int dma_rx_poll_rate; + unsigned int dma_rx_poll_timeout; void (*init) (void); void (*exit) (void); }; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-22 12:45 ` [PATCH v2] " Chanho Min @ 2013-01-22 13:18 ` Linus Walleij 2013-01-23 5:07 ` Chanho Min 0 siblings, 1 reply; 19+ messages in thread From: Linus Walleij @ 2013-01-22 13:18 UTC (permalink / raw) To: Chanho Min Cc: Russell King - ARM Linux, Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll, Chanho Min On Tue, Jan 22, 2013 at 1:45 PM, Chanho Min <chanho.min@lge.com> wrote: > In DMA support, The received data is not pushed to tty until the DMA buffer > is filled. But some megabyte rate chips such as BT expect fast response and > data should be pushed immediately. In order to fix this issue, We suggest > the use of the timer for polling DMA buffer. > In our test, no data loss occurred at high-baudrate as compared with interrupt- > driven (We tested with 3Mbps). So to enable this you put a poll rate > 0 into the new variable, OK seems like it could work... > - If poll timer is activated, We use consistent DMA mappings to avoid from > the frequent cache operation of the timer function. sg->coherency is used > to check if buffer is coherent. Hm. We can begin like this but maybe we should later patch it to make this the default unless it has performance impacts on our systems. > +++ b/include/linux/amba/serial.h > @@ -203,6 +203,8 @@ struct amba_pl011_data { > bool (*dma_filter)(struct dma_chan *chan, void *filter_param); > void *dma_rx_param; > void *dma_tx_param; > + unsigned int dma_rx_poll_rate; > + unsigned int dma_rx_poll_timeout; > void (*init) (void); > void (*exit) (void); > }; Maybe this is a good opportunity to add some kerneldoc above this struct so as to help platform implementers. It will invariably be reused by Device Tree binding authors later on... Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-22 13:18 ` Linus Walleij @ 2013-01-23 5:07 ` Chanho Min 2013-01-25 20:25 ` Greg Kroah-Hartman 0 siblings, 1 reply; 19+ messages in thread From: Chanho Min @ 2013-01-23 5:07 UTC (permalink / raw) To: Linus Walleij Cc: Russell King - ARM Linux, Alan Cox, Greg Kroah-Hartman, linux-kernel, linux-serial, Pawel Moll On Tue, Jan 22, 2013 at 10:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> - If poll timer is activated, We use consistent DMA mappings to avoid from >> the frequent cache operation of the timer function. sg->coherency is used >> to check if buffer is coherent. > > Hm. We can begin like this but maybe we should later patch it to > make this the default unless it has performance impacts on our > systems. Good, I really hope to make this the default. We expect it has no impact to system even if no polling use. > Maybe this is a good opportunity to add some kerneldoc above > this struct so as to help platform implementers. It will invariably > be reused by Device Tree binding authors later on... Agree, but, frankly I need help about what I didn't modify. So We will leave this work for the next submit. Thanks Chanho ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-23 5:07 ` Chanho Min @ 2013-01-25 20:25 ` Greg Kroah-Hartman 2013-01-25 21:26 ` Russell King - ARM Linux 0 siblings, 1 reply; 19+ messages in thread From: Greg Kroah-Hartman @ 2013-01-25 20:25 UTC (permalink / raw) To: Chanho Min Cc: Linus Walleij, Russell King - ARM Linux, Alan Cox, linux-kernel, linux-serial, Pawel Moll On Wed, Jan 23, 2013 at 02:07:57PM +0900, Chanho Min wrote: > On Tue, Jan 22, 2013 at 10:18 PM, Linus Walleij > <linus.walleij@linaro.org> wrote: > >> - If poll timer is activated, We use consistent DMA mappings to avoid from > >> the frequent cache operation of the timer function. sg->coherency is used > >> to check if buffer is coherent. > > > > Hm. We can begin like this but maybe we should later patch it to > > make this the default unless it has performance impacts on our > > systems. > Good, I really hope to make this the default. > We expect it has no impact to system even if no polling use. > > > Maybe this is a good opportunity to add some kerneldoc above > > this struct so as to help platform implementers. It will invariably > > be reused by Device Tree binding authors later on... > Agree, but, frankly I need help about what I didn't modify. > So We will leave this work for the next submit. So, should this patch be applied? Can I get an ack from someone who knows this driver/platform please? thanks, greg k-h ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-25 20:25 ` Greg Kroah-Hartman @ 2013-01-25 21:26 ` Russell King - ARM Linux 0 siblings, 0 replies; 19+ messages in thread From: Russell King - ARM Linux @ 2013-01-25 21:26 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Chanho Min, Linus Walleij, Alan Cox, linux-kernel, linux-serial, Pawel Moll On Fri, Jan 25, 2013 at 12:25:07PM -0800, Greg Kroah-Hartman wrote: > On Wed, Jan 23, 2013 at 02:07:57PM +0900, Chanho Min wrote: > > On Tue, Jan 22, 2013 at 10:18 PM, Linus Walleij > > <linus.walleij@linaro.org> wrote: > > >> - If poll timer is activated, We use consistent DMA mappings to avoid from > > >> the frequent cache operation of the timer function. sg->coherency is used > > >> to check if buffer is coherent. > > > > > > Hm. We can begin like this but maybe we should later patch it to > > > make this the default unless it has performance impacts on our > > > systems. > > Good, I really hope to make this the default. > > We expect it has no impact to system even if no polling use. > > > > > Maybe this is a good opportunity to add some kerneldoc above > > > this struct so as to help platform implementers. It will invariably > > > be reused by Device Tree binding authors later on... > > Agree, but, frankly I need help about what I didn't modify. > > So We will leave this work for the next submit. > > So, should this patch be applied? Can I get an ack from someone who > knows this driver/platform please? Well, I thought I had explained that I'd prefer to see the poll rate adjusted with the baud rate, but maybe I wasn't explicit enough. Instead, what we seem to have ended up with are two new entries in platform data (which we're trying to get away from with DT): + unsigned int dma_rx_poll_rate; + unsigned int dma_rx_poll_timeout; If this were to be done, then receive DMA could be used on the Versatile PB platforms without having it suck CPU usage unnecessarily at slower baud rates. ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling
@ 2013-01-28 8:15 Chanho Min
2013-01-28 8:28 ` Linus Walleij
0 siblings, 1 reply; 19+ messages in thread
From: Chanho Min @ 2013-01-28 8:15 UTC (permalink / raw)
To: Greg Kroah-Hartman, Russell King - ARM Linux
Cc: Linus Walleij, Alan Cox, linux-kernel, Pawel Moll, linux-serial,
Chanho Min
>Well, I thought I had explained that I'd prefer to see the poll rate
>adjusted with the baud rate, but maybe I wasn't explicit enough.
>Instead, what we seem to have ended up with are two new entries in
>platform data (which we're trying to get away from with DT):
>
>+ unsigned int dma_rx_poll_rate;
>+ unsigned int dma_rx_poll_timeout;
>
>If this were to be done, then receive DMA could be used on the Versatile
>PB platforms without having it suck CPU usage unnecessarily at slower
>baud rates.
This patch is working well on our pl011+ pl080 platform.
If no data is received during time-out, no tick timer works.
Do you want to adjust the poll rate with the baud rate additionally?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-28 8:15 Chanho Min @ 2013-01-28 8:28 ` Linus Walleij 2013-01-29 2:41 ` Chanho Min 0 siblings, 1 reply; 19+ messages in thread From: Linus Walleij @ 2013-01-28 8:28 UTC (permalink / raw) To: Chanho Min Cc: Greg Kroah-Hartman, Russell King - ARM Linux, Alan Cox, linux-kernel, Pawel Moll, linux-serial, Chanho Min On Mon, Jan 28, 2013 at 9:15 AM, Chanho Min <chanho.min@lge.com> wrote: > [Russell] >>Well, I thought I had explained that I'd prefer to see the poll rate >>adjusted with the baud rate, but maybe I wasn't explicit enough. >>Instead, what we seem to have ended up with are two new entries in >>platform data (which we're trying to get away from with DT): >> >>+ unsigned int dma_rx_poll_rate; >>+ unsigned int dma_rx_poll_timeout; >> >>If this were to be done, then receive DMA could be used on the Versatile >>PB platforms without having it suck CPU usage unnecessarily at slower >>baud rates. > This patch is working well on our pl011+ pl080 platform. > If no data is received during time-out, no tick timer works. > Do you want to adjust the poll rate with the baud rate additionally? I'm not following, you wrote earlier in the thread: >>Should we scale the polling interval according to baud >>rate? > > It is also our concern, I will suggest the proper way. We are waiting for you suggestion, i.e. a new patch iteration taking this into account. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-28 8:28 ` Linus Walleij @ 2013-01-29 2:41 ` Chanho Min 2013-01-29 10:57 ` Linus Walleij 0 siblings, 1 reply; 19+ messages in thread From: Chanho Min @ 2013-01-29 2:41 UTC (permalink / raw) To: Linus Walleij, Russell King - ARM Linux Cc: Greg Kroah-Hartman, Alan Cox, linux-kernel, Pawel Moll, linux-serial, Chanho Min On Mon, Jan 28, 2013 at 5:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jan 28, 2013 at 9:15 AM, Chanho Min <chanho.min@lge.com> wrote: >> [Russell] >>>Well, I thought I had explained that I'd prefer to see the poll rate >>>adjusted with the baud rate, but maybe I wasn't explicit enough. >>>Instead, what we seem to have ended up with are two new entries in >>>platform data (which we're trying to get away from with DT): >>> >>>+ unsigned int dma_rx_poll_rate; >>>+ unsigned int dma_rx_poll_timeout; >>>Should we scale the polling interval according to baud >>>rate? >> >> It is also our concern, I will suggest the proper way. It was thought a way to reduce unnecessary cpu usage, but, now we add dma_rx_poll_timeout to stop polling during idle. I thought it is preferred the poll rate is decided by it's user than auto scale. because The required response to tty can be differ from platform to platform. Some platform need fast response even if more cpu usage is needed, but other platform will be enough to handle with slow response. Any opinion will be appreciated. Thanks Chanho ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-29 2:41 ` Chanho Min @ 2013-01-29 10:57 ` Linus Walleij 2013-01-30 0:53 ` Chanho Min 0 siblings, 1 reply; 19+ messages in thread From: Linus Walleij @ 2013-01-29 10:57 UTC (permalink / raw) To: Chanho Min Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox, linux-kernel, Pawel Moll, linux-serial, Chanho Min On Tue, Jan 29, 2013 at 3:41 AM, Chanho Min <chanho.min@lge.com> wrote: > On Mon, Jan 28, 2013 at 5:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Mon, Jan 28, 2013 at 9:15 AM, Chanho Min <chanho.min@lge.com> wrote: >>> [Russell] >>>>Well, I thought I had explained that I'd prefer to see the poll rate >>>>adjusted with the baud rate, but maybe I wasn't explicit enough. >>>>Instead, what we seem to have ended up with are two new entries in >>>>platform data (which we're trying to get away from with DT): >>>> >>>>+ unsigned int dma_rx_poll_rate; >>>>+ unsigned int dma_rx_poll_timeout; > >>>>Should we scale the polling interval according to baud >>>>rate? >>> >>> It is also our concern, I will suggest the proper way. > > It was thought a way to reduce unnecessary cpu usage, > but, now we add dma_rx_poll_timeout to stop polling during idle. During idle I guess any system want to set the UART into IRQ mode? So it will wake up on some event on the UART, whatever it may be. Then it can use DMA polling post-wakeup? > I thought it is preferred the poll rate is decided by it's user than > auto scale. What do you mean by "user" here? Userspace? Or "platform data parameter"? I can se three usecases: - Console or serial TTY - Modem (as in 33k8 modem, needing RTS/CTS HW handshaking) - Peripheral (like high-speed bluetooth chips) needing low-latency The kernel already knows if the port is a console or TTY. What you need to know is if it is one of the other two usecases. > because The required response to tty can be differ from platform to platform. > Some platform need fast response even if more cpu usage is needed, > but other platform will be enough to handle with slow response. Not platform I think. Per usecase. Are you not actually thinking about things like the above? These are usecase requirements, not platform requirements. For example I thing it is magnitude better idea to encode something like an enum for these usecases that some bool is_platform_foo_bt_uart or whatever the alternative would be. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-29 10:57 ` Linus Walleij @ 2013-01-30 0:53 ` Chanho Min 2013-01-30 2:53 ` Chanho Min 0 siblings, 1 reply; 19+ messages in thread From: Chanho Min @ 2013-01-30 0:53 UTC (permalink / raw) To: Linus Walleij Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox, linux-kernel, Pawel Moll, linux-serial > Then it can use DMA polling post-wakeup? Yes, It will be switched back to DMA polling on UART IRQ. > What do you mean by "user" here? Userspace? Or > "platform data parameter"? platform data parameter > Not platform I think. Per usecase. > Are you not actually thinking about things like the above? > These are usecase requirements, not platform requirements. I mean that: Platform knows the requirement of it's ports (It can be usecase above) and can set the poll rate with platform data parameter. Some port is enough to do with slow poll rate, some need fast poll rate for the fast response. Of course some port don't need rx dma and polling, in this case we can set the poll rate to 0. If we fix the poll rate to the scale of baud rate, we don't have such flexibility. Anyway I just want to know we still need auto scale of poll rate. If this become clear, I will prepare v3 of this patch. Thanks Chanho ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-30 0:53 ` Chanho Min @ 2013-01-30 2:53 ` Chanho Min 2013-01-31 20:33 ` Linus Walleij 0 siblings, 1 reply; 19+ messages in thread From: Chanho Min @ 2013-01-30 2:53 UTC (permalink / raw) To: Linus Walleij Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox, linux-kernel, Pawel Moll, linux-serial > Anyway I just want to know we still need auto scale of poll rate. I correct the mistake in this sentence. Anyway I just want to know whether we still need auto scale of poll rate or not. Thanks Chanho ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] ARM: PL011: Add support for Rx DMA buffer polling 2013-01-30 2:53 ` Chanho Min @ 2013-01-31 20:33 ` Linus Walleij 0 siblings, 0 replies; 19+ messages in thread From: Linus Walleij @ 2013-01-31 20:33 UTC (permalink / raw) To: Chanho Min Cc: Russell King - ARM Linux, Greg Kroah-Hartman, Alan Cox, linux-kernel, Pawel Moll, linux-serial On Wed, Jan 30, 2013 at 3:53 AM, Chanho Min <chanho.min@lge.com> wrote: >> Anyway I just want to know we still need auto scale of poll rate. > I correct the mistake in this sentence. > Anyway I just want to know whether we still need auto scale of poll rate or not. For the console/TTY and modem usecase I think it's pretty much necessary don't you think? Else these users (which is by far the most widespread of the kernel users) will be punished by too many wakeups if they use DMA. And they should be able to do that, right? This is a perfect fit for the Versatile and PB11MPcore ARM reference designs. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-01-31 20:33 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <50efad8d.84fc440a.589e.ffff9546SMTPIN_ADDED_BROKEN@mx.google.com> 2013-01-11 18:34 ` [PATCH] ARM: PL011: Add support for Rx DMA buffer polling Linus Walleij [not found] ` <50f107aa.894e420a.596a.ffffe3f6SMTPIN_ADDED_BROKEN@mx.google.com> 2013-01-14 0:04 ` Linus Walleij 2013-01-14 0:26 ` Russell King - ARM Linux 2013-01-14 6:46 ` Linus Walleij 2013-01-14 7:27 ` Chanho Min 2013-01-14 7:56 ` Linus Walleij 2013-01-14 8:41 ` Chanho Min 2013-01-22 12:45 ` [PATCH v2] " Chanho Min 2013-01-22 13:18 ` Linus Walleij 2013-01-23 5:07 ` Chanho Min 2013-01-25 20:25 ` Greg Kroah-Hartman 2013-01-25 21:26 ` Russell King - ARM Linux 2013-01-28 8:15 Chanho Min 2013-01-28 8:28 ` Linus Walleij 2013-01-29 2:41 ` Chanho Min 2013-01-29 10:57 ` Linus Walleij 2013-01-30 0:53 ` Chanho Min 2013-01-30 2:53 ` Chanho Min 2013-01-31 20:33 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).