Hello Dirk,
Am Freitag, den 31.07.2015, 10:13 +0200 schrieb Dirk Behme:
> On 31.07.2015 09:40, Lucas Stach wrote:
> > Hi Dirk,
> >
> > Am Freitag, den 31.07.2015, 08:40 +0200 schrieb Dirk Behme:
> >> On 19.05.2015 14:16, Fabio Estevam wrote:
> >>> Hi Philipp,
> >>>
> >>> On Tue, May 19, 2015 at 5:54 AM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
> >>>> The driver configures the IDLE condition to interrupt the SDMA engine.
> >>>> Since the SDMA UART ROM script doesn't clear the IDLE bit itself, this
> >>>> caused repeated 1-byte DMA transfers, regardless of available data in the
> >>>> RX FIFO. Also, when returning due to the IDLE condition, the UART ROM
> >>>> script already increased its counter, causing residue to be off by one.
> >>>>
> >>>> This patch clears the IDLE condition to avoid repeated 1-byte DMA transfers
> >>>> and decreases count by when the DMA transfer was aborted due to the IDLE
> >>>> condition, fixing serial transfers using DMA on i.MX6Q.
> >>>>
> >>>> Reported-by: Peter Seiderer <ps.report@xxxxxxx>
> >>>> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> >>>
> >>> Thanks for the fix. Tested on a imx6sl-warp, where I could not use DMA
> >>> to access a Bluetooth device using the ROM SDMA firmware.
> >>>
> >>> Tested-by: Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx>
> >>>
> >>> Could you please also Cc stable?
> >>
> >>
> >> Just fyi: We got this patch via -stable, and found that it results in
> >> issues for us. So we had to revert it in our local tree.
> >>
> >> We are using the DMA with baud rates > 2Mbit/s. Having a quick look at
> >> the registers in the ARM Reference Manual, USR2_IDLE seems to be a flag
> >> that is set when the RX line has been idle for at least a defined amount
> >> of time.
> >>
> >> For us this commit is dangerous because it fails to check whether the
> >> IDLE CONDITION feature is being used. We don't use the RX IDLE CONDITION
> >> feature, therefore it is likely to cause the last received character to
> >> be discarded causing corruption of data.
> >>
> >>From your description it's not clear to me why this commit is causing
> > issues for you. The idle condition signaling is automatically enabled
> > when using DMA.
> >
> > The idle signaling should only ever fire if the line has been idle for
> > 32byte durations. The aging timer, that expires after data is sitting in
> > the FIFO for 8 byte durations, should have flushed out all data at this
> > point.
> >
> > Do you use a RAM SDMA firmware or do you rely on the internal one.
>
> To my understanding we are using a RAM SDMA firmware. If I read the
> various mailing list thread correctly, the difference between the RAM
> SDMA firmware and the internal one was already often a root cause for
> different behavior.
>
Can you please check that the attached patches fix the problem for your
use-case? They correct the DMA setup to not use the idle condition
detect at all and instead fix the aging timer to work properly.
That's what the reference manual says how things should be done. This
should work both with and without external SDMA firmware, but I only
tested the ROM firmware case so far.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
From 64ae1f6b5c23b61b39ac353d62204eac23ebd877 Mon Sep 17 00:00:00 2001
From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
Date: Mon, 3 Aug 2015 12:06:09 +0200
Subject: [PATCH 1/3] serial: imx: make setup_ufcr more useful
This function is currently only used by a single caller and does not
make use use of it's parameter or return value.
Change prototype to pass in watermark levels, so we can reuse this
function in the DMA setup paths. Also relocate to be near the calling
functions.
Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
---
drivers/tty/serial/imx.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 88250395b0ce..8c16376c487d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -850,20 +850,6 @@ static void imx_break_ctl(struct uart_port *port, int break_state)
spin_unlock_irqrestore(&sport->port.lock, flags);
}
-#define TXTL 2 /* reset default */
-#define RXTL 1 /* reset default */
-
-static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
-{
- unsigned int val;
-
- /* set receiver / transmitter trigger level */
- val = readl(sport->port.membase + UFCR) & (UFCR_RFDIV | UFCR_DCEDTE);
- val |= TXTL << UFCR_TXTL_SHF | RXTL;
- writel(val, sport->port.membase + UFCR);
- return 0;
-}
-
#define RX_BUF_SIZE (PAGE_SIZE)
static void imx_rx_dma_done(struct imx_port *sport)
{
@@ -974,6 +960,20 @@ static int start_rx_dma(struct imx_port *sport)
return 0;
}
+#define TXTL_DEFAULT 2 /* reset default */
+#define RXTL_DEFAULT 1 /* reset default */
+
+static void imx_setup_ufcr(struct imx_port *sport,
+ unsigned char txwl, unsigned char rxwl)
+{
+ unsigned int val;
+
+ /* set receiver / transmitter trigger level */
+ val = readl(sport->port.membase + UFCR) & (UFCR_RFDIV | UFCR_DCEDTE);
+ val |= txwl << UFCR_TXTL_SHF | rxwl;
+ writel(val, sport->port.membase + UFCR);
+}
+
static void imx_uart_dma_exit(struct imx_port *sport)
{
if (sport->dma_chan_rx) {
@@ -1009,7 +1009,7 @@ static int imx_uart_dma_init(struct imx_port *sport)
slave_config.direction = DMA_DEV_TO_MEM;
slave_config.src_addr = sport->port.mapbase + URXD0;
slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- slave_config.src_maxburst = RXTL;
+ slave_config.src_maxburst = RXTL_DEFAULT;
ret = dmaengine_slave_config(sport->dma_chan_rx, &slave_config);
if (ret) {
dev_err(dev, "error in RX dma configuration.\n");
@@ -1033,7 +1033,7 @@ static int imx_uart_dma_init(struct imx_port *sport)
slave_config.direction = DMA_MEM_TO_DEV;
slave_config.dst_addr = sport->port.mapbase + URTX0;
slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- slave_config.dst_maxburst = TXTL;
+ slave_config.dst_maxburst = TXTL_DEFAULT;
ret = dmaengine_slave_config(sport->dma_chan_tx, &slave_config);
if (ret) {
dev_err(dev, "error in TX dma configuration.");
@@ -1109,7 +1109,7 @@ static int imx_startup(struct uart_port *port)
return retval;
}
- imx_setup_ufcr(sport, 0);
+ imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
/* disable the DREN bit (Data Ready interrupt enable) before
* requesting IRQs
@@ -1769,7 +1769,7 @@ imx_console_setup(struct console *co, char *options)
else
imx_console_get_options(sport, &baud, &parity, &bits);
- imx_setup_ufcr(sport, 0);
+ imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
retval = uart_set_options(&sport->port, co, baud, parity, bits, flow);
--
2.1.4
From 124cd2992a1f62cf0107ed2bc6e9d05aa78351e4 Mon Sep 17 00:00:00 2001
From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
Date: Mon, 3 Aug 2015 12:14:18 +0200
Subject: [PATCH 2/3] serial: imx: configure proper DMA burst sizes
Triggering the DMA engine for every byte is horribly inefficient.
Also it doesn't allow to use the aging timer for the RX FIFO as this
requires the DMA engine to leave one byte remaining in the FIFO when
doing a normal burst transfer.
Adjust watermark levels so that the DMA engine can do at least 8 byte
burst transfers. This is a conservative value, as the both TX and RX
FIFOs are able to contain 32 bytes.
Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
---
drivers/tty/serial/imx.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 8c16376c487d..40a2f643d26a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -962,6 +962,8 @@ static int start_rx_dma(struct imx_port *sport)
#define TXTL_DEFAULT 2 /* reset default */
#define RXTL_DEFAULT 1 /* reset default */
+#define TXTL_DMA 8 /* DMA burst setting */
+#define RXTL_DMA 9 /* DMA burst setting */
static void imx_setup_ufcr(struct imx_port *sport,
unsigned char txwl, unsigned char rxwl)
@@ -1009,7 +1011,8 @@ static int imx_uart_dma_init(struct imx_port *sport)
slave_config.direction = DMA_DEV_TO_MEM;
slave_config.src_addr = sport->port.mapbase + URXD0;
slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- slave_config.src_maxburst = RXTL_DEFAULT;
+ /* one byte less than the watermark level to enable the aging timer */
+ slave_config.src_maxburst = RXTL_DMA - 1;
ret = dmaengine_slave_config(sport->dma_chan_rx, &slave_config);
if (ret) {
dev_err(dev, "error in RX dma configuration.\n");
@@ -1033,7 +1036,7 @@ static int imx_uart_dma_init(struct imx_port *sport)
slave_config.direction = DMA_MEM_TO_DEV;
slave_config.dst_addr = sport->port.mapbase + URTX0;
slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
- slave_config.dst_maxburst = TXTL_DEFAULT;
+ slave_config.dst_maxburst = TXTL_DMA;
ret = dmaengine_slave_config(sport->dma_chan_tx, &slave_config);
if (ret) {
dev_err(dev, "error in TX dma configuration.");
@@ -1066,6 +1069,8 @@ static void imx_enable_dma(struct imx_port *sport)
temp |= UCR4_IDDMAEN;
writel(temp, sport->port.membase + UCR4);
+ imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
+
sport->dma_is_enabled = 1;
}
@@ -1088,6 +1093,8 @@ static void imx_disable_dma(struct imx_port *sport)
temp &= ~UCR4_IDDMAEN;
writel(temp, sport->port.membase + UCR4);
+ imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
+
sport->dma_is_enabled = 0;
}
--
2.1.4
From 39ae68be74a2271fb7ed8af4501fc55c4182a6a7 Mon Sep 17 00:00:00 2001
From: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
Date: Mon, 3 Aug 2015 12:21:57 +0200
Subject: [PATCH 3/3] serial: imx: don't use idle condition detect for DMA
transfers
The reference manual states that idle condition detect should not be used
with DMA transfers, as the ROM SDMA scripts don't check those conditions.
The RAM SDMA scripts worked around this, but the change broke compatibility
with the ROM scripts.
The previous commits fixed the DMA burst sizes, so that the aging timer is
now working as described in the reference manual. With this fixed we can
remove the hack of using the idle condition detect to stop the DMA transfer
if there are no new characters incoming.
This should work with both the ROM and RAM SDMA scripts.
Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
---
drivers/tty/serial/imx.c | 31 +++++--------------------------
1 file changed, 5 insertions(+), 26 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 40a2f643d26a..595ff7cabb98 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -873,14 +873,12 @@ static void imx_rx_dma_done(struct imx_port *sport)
}
/*
- * There are three kinds of RX DMA interrupts(such as in the MX6Q):
+ * There are two kinds of RX DMA interrupts(such as in the MX6Q):
* [1] the RX DMA buffer is full.
- * [2] the Aging timer expires(wait for 8 bytes long)
- * [3] the Idle Condition Detect(enabled the UCR4_IDDMAEN).
+ * [2] the aging timer expires
*
- * The [2] is trigger when a character was been sitting in the FIFO
- * meanwhile [3] can wait for 32 bytes long when the RX line is
- * on IDLE state and RxFIFO is empty.
+ * Condition [2] is triggered when a character has been sitting in the FIFO
+ * for at least 8 byte durations.
*/
static void dma_rx_callback(void *data)
{
@@ -898,13 +896,6 @@ static void dma_rx_callback(void *data)
status = dmaengine_tx_status(chan, (dma_cookie_t)0, &state);
count = RX_BUF_SIZE - state.residue;
- if (readl(sport->port.membase + USR2) & USR2_IDLE) {
- /* In condition [3] the SDMA counted up too early */
- count--;
-
- writel(USR2_IDLE, sport->port.membase + USR2);
- }
-
dev_dbg(sport->port.dev, "We get %d bytes.\n", count);
if (count) {
@@ -1059,16 +1050,9 @@ static void imx_enable_dma(struct imx_port *sport)
/* set UCR1 */
temp = readl(sport->port.membase + UCR1);
- temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN |
- /* wait for 32 idle frames for IDDMA interrupt */
- UCR1_ICD_REG(3);
+ temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN;
writel(temp, sport->port.membase + UCR1);
- /* set UCR4 */
- temp = readl(sport->port.membase + UCR4);
- temp |= UCR4_IDDMAEN;
- writel(temp, sport->port.membase + UCR4);
-
imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
sport->dma_is_enabled = 1;
@@ -1088,11 +1072,6 @@ static void imx_disable_dma(struct imx_port *sport)
temp &= ~(UCR2_CTSC | UCR2_CTS);
writel(temp, sport->port.membase + UCR2);
- /* clear UCR4 */
- temp = readl(sport->port.membase + UCR4);
- temp &= ~UCR4_IDDMAEN;
- writel(temp, sport->port.membase + UCR4);
-
imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
sport->dma_is_enabled = 0;
--
2.1.4
