[PATCH 0/3] serial: mxs-auart: flush_buffer hook and interrupt processing

[PATCH 0/3] serial: mxs-auart: flush_buffer hook and interrupt processing

[Hector Palacios]

Hello,

This series implement several fixes to the mxs-auart serial driver.

The first patch implements the flush_buffer hook. Currently the driver
can be shut down and pending data in the FIFO will be lost.

The second patchs adds a check of the BUSY flag in the tx_empty hook
to make sure the UART is not about to fill data into the FIFO.

The third patch moves the interrupt handling from the ISR out to a
tasklet. This was motivated due to different transmission problems
with hardware and software flow control at high baudrates whereby
transmission was interrupted or data lost. 
The problem can be reproduced by setting two AUARTs at 576000/N/8 
with CRTSCTS and sending a large file from one port to the other
 with 'cat'.

Hector Palacios (3):
  serial: mxs-auart: implement flush_buffer hook
  serial: mxs-auart: check BUSY flag on tx_empty hook
  serial: mxs-auart: move irq handling to a tasklet

 drivers/tty/serial/mxs-auart.c | 93 ++++++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 25 deletions(-)

[PATCH 1/3] serial: mxs-auart: implement flush_buffer hook

[Hector Palacios]

Terminate any DMA transfer and verify the TX FIFO is empty.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/tty/serial/mxs-auart.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 9f0461778fc1..d9bf6e103f65 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -782,6 +782,28 @@ static unsigned int mxs_auart_tx_empty(struct uart_port *u)
 		return 0;
 }
 
+/*
+ * Flush the transmit buffer.
+ * Locking: called with port lock held and IRQs disabled.
+ */
+static void mxs_auart_flush_buffer(struct uart_port *u)
+{
+	struct mxs_auart_port *s = to_auart_port(u);
+	struct dma_chan *channel = s->tx_dma_chan;
+	unsigned int to;
+
+	if (auart_dma_enabled(s)) {
+		/* Avoid deadlock with the DMA engine callback */
+		spin_unlock(&s->port.lock);
+		dmaengine_terminate_all(channel);
+		spin_lock(&s->port.lock);
+	}
+	/* Wait for the FIFO to flush */
+	to = u->timeout;
+	while (!mxs_auart_tx_empty(u) && to--)
+		mdelay(1);
+}
+
 static void mxs_auart_start_tx(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
@@ -828,6 +850,7 @@ static struct uart_ops mxs_auart_ops = {
 	.get_mctrl      = mxs_auart_get_mctrl,
 	.startup	= mxs_auart_startup,
 	.shutdown       = mxs_auart_shutdown,
+	.flush_buffer	= mxs_auart_flush_buffer,
 	.set_termios    = mxs_auart_settermios,
 	.type	   	= mxs_auart_type,
 	.release_port   = mxs_auart_release_port,

[PATCH 2/3] serial: mxs-auart: check BUSY flag on tx_empty hook

[Hector Palacios]

Make sure AUART is not busy before returning tx empty.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/tty/serial/mxs-auart.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index d9bf6e103f65..e5540c89dd48 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -776,10 +776,13 @@ static void mxs_auart_shutdown(struct uart_port *u)
 
 static unsigned int mxs_auart_tx_empty(struct uart_port *u)
 {
-	if (readl(u->membase + AUART_STAT) & AUART_STAT_TXFE)
+	unsigned long stat;
+
+	stat = readl(u->membase + AUART_STAT);
+	if ((stat & (AUART_STAT_BUSY | AUART_STAT_TXFE)) == AUART_STAT_TXFE)
 		return TIOCSER_TEMT;
-	else
-		return 0;
+
+	return 0;
 }
 
 /*

[PATCH 3/3] serial: mxs-auart: move irq handling to a tasklet

[Hector Palacios]

The ISR was handling CTS flag and RX/TX processing.
Occasionally interrupts were coming in the middle of the ISR and
led to blocking transmission (with hw flow) or loss of data.
This patch moves this processing to a tasklet that the ISR schedules.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/tty/serial/mxs-auart.c | 61 +++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index e5540c89dd48..7021269c70b2 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -148,6 +148,10 @@ struct mxs_auart_port {
 	struct clk *clk;
 	struct device *dev;
 
+	struct tasklet_struct tasklet;
+	unsigned int irq_status;
+	unsigned int status;
+
 	/* for DMA */
 	struct scatterlist tx_sgl;
 	struct dma_chan	*tx_dma_chan;
@@ -680,38 +684,48 @@ static void mxs_auart_settermios(struct uart_port *u,
 	}
 }
 
+/*
+ * tasklet handling tty stuff outside the interrupt handler.
+ */
+static void mxs_auart_tasklet_func(unsigned long data)
+{
+	struct uart_port *port = (struct uart_port *)data;
+	struct mxs_auart_port *s = to_auart_port(port);
+
+	/* Handle status irq */
+	if (s->irq_status & AUART_INTR_CTSMIS) {
+		uart_handle_cts_change(&s->port, s->status & AUART_STAT_CTS);
+		writel(AUART_INTR_CTSMIS,
+		       s->port.membase + AUART_INTR_CLR);
+	}
+	/* Handle receive irq */
+	if (s->irq_status & (AUART_INTR_RTIS | AUART_INTR_RXIS)) {
+		if (!auart_dma_enabled(s))
+			mxs_auart_rx_chars(s);
+	}
+	/* Handle transmit irq */
+	if (s->irq_status & AUART_INTR_TXIS)
+		mxs_auart_tx_chars(s);
+}
+
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 {
-	u32 istat;
 	struct mxs_auart_port *s = context;
-	u32 stat = readl(s->port.membase + AUART_STAT);
 
-	istat = readl(s->port.membase + AUART_INTR);
+	s->status = readl(s->port.membase + AUART_STAT);
+	s->irq_status = readl(s->port.membase + AUART_INTR);
 
-	/* ack irq */
-	writel(istat & (AUART_INTR_RTIS
+	/* ack irqs */
+	writel(s->irq_status & (AUART_INTR_RTIS
 		| AUART_INTR_TXIS
 		| AUART_INTR_RXIS
 		| AUART_INTR_CTSMIS),
 			s->port.membase + AUART_INTR_CLR);
 
-	if (istat & AUART_INTR_CTSMIS) {
-		uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
-		writel(AUART_INTR_CTSMIS,
-				s->port.membase + AUART_INTR_CLR);
-		istat &= ~AUART_INTR_CTSMIS;
-	}
-
-	if (istat & (AUART_INTR_RTIS | AUART_INTR_RXIS)) {
-		if (!auart_dma_enabled(s))
-			mxs_auart_rx_chars(s);
-		istat &= ~(AUART_INTR_RTIS | AUART_INTR_RXIS);
-	}
-
-	if (istat & AUART_INTR_TXIS) {
-		mxs_auart_tx_chars(s);
-		istat &= ~AUART_INTR_TXIS;
-	}
+	/* Handle interrupt with tasklet */
+	if (s->irq_status & (AUART_INTR_RTIS | AUART_INTR_RXIS |
+			     AUART_INTR_CTSMIS | AUART_INTR_TXIS))
+		tasklet_schedule(&s->tasklet);
 
 	return IRQ_HANDLED;
 }
@@ -1112,6 +1126,8 @@ static int mxs_auart_probe(struct platform_device *pdev)
 
 	auart_port[s->port.line] = s;
 
+	tasklet_init(&s->tasklet, mxs_auart_tasklet_func, (unsigned long)s);
+
 	mxs_auart_reset(&s->port);
 
 	ret = uart_add_one_port(&auart_driver, &s->port);
@@ -1141,6 +1157,7 @@ static int mxs_auart_remove(struct platform_device *pdev)
 	struct mxs_auart_port *s = platform_get_drvdata(pdev);
 
 	uart_remove_one_port(&auart_driver, &s->port);
+	tasklet_kill(&s->tasklet);
 
 	auart_port[pdev->id] = NULL;
 

[PATCH 0/3] serial: mxs-auart: flush_buffer hook and interrupt processing

[Marek Vasut]

Hi Hector,

> Hello,
> 
> This series implement several fixes to the mxs-auart serial driver.
> 
> The first patch implements the flush_buffer hook. Currently the driver
> can be shut down and pending data in the FIFO will be lost.
> 
> The second patchs adds a check of the BUSY flag in the tx_empty hook
> to make sure the UART is not about to fill data into the FIFO.
> 
> The third patch moves the interrupt handling from the ISR out to a
> tasklet. This was motivated due to different transmission problems
> with hardware and software flow control at high baudrates whereby
> transmission was interrupted or data lost.
> The problem can be reproduced by setting two AUARTs at 576000/N/8
> with CRTSCTS and sending a large file from one port to the other
>  with 'cat'.

57600 is not that high of a baudrate. Or do you really mean 576000 (576 
thousand) ?

Best regards,
Marek Vasut

[PATCH 0/3] serial: mxs-auart: flush_buffer hook and interrupt processing

[Hector Palacios]

On 11/29/2013 05:44 PM, Marek Vasut wrote:
> Hi Hector,
>
>> Hello,
>>
>> This series implement several fixes to the mxs-auart serial driver.
>>
>> The first patch implements the flush_buffer hook. Currently the driver
>> can be shut down and pending data in the FIFO will be lost.
>>
>> The second patchs adds a check of the BUSY flag in the tx_empty hook
>> to make sure the UART is not about to fill data into the FIFO.
>>
>> The third patch moves the interrupt handling from the ISR out to a
>> tasklet. This was motivated due to different transmission problems
>> with hardware and software flow control at high baudrates whereby
>> transmission was interrupted or data lost.
>> The problem can be reproduced by setting two AUARTs at 576000/N/8
>> with CRTSCTS and sending a large file from one port to the other
>>   with 'cat'.
>
> 57600 is not that high of a baudrate. Or do you really mean 576000 (576
> thousand) ?

I wrote and meant 576000, indeed :-)

Best regards,
--
Hector Palacios

[PATCH 1/3] serial: mxs-auart: implement flush_buffer hook

[Marek Vasut]

Hello Hector,

> Terminate any DMA transfer and verify the TX FIFO is empty.
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  drivers/tty/serial/mxs-auart.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/tty/serial/mxs-auart.c
> b/drivers/tty/serial/mxs-auart.c index 9f0461778fc1..d9bf6e103f65 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -782,6 +782,28 @@ static unsigned int mxs_auart_tx_empty(struct
> uart_port *u) return 0;
>  }
> 
> +/*
> + * Flush the transmit buffer.
> + * Locking: called with port lock held and IRQs disabled.
> + */
> +static void mxs_auart_flush_buffer(struct uart_port *u)
> +{
> +	struct mxs_auart_port *s = to_auart_port(u);
> +	struct dma_chan *channel = s->tx_dma_chan;
> +	unsigned int to;
> +
> +	if (auart_dma_enabled(s)) {
> +		/* Avoid deadlock with the DMA engine callback */
> +		spin_unlock(&s->port.lock);
> +		dmaengine_terminate_all(channel);
> +		spin_lock(&s->port.lock);

Can you not maybe just set some flag here to tell the DMA engine callback things 
are shutting down and to don't do anything funny anymore ?

> +	}
> +	/* Wait for the FIFO to flush */
> +	to = u->timeout;
> +	while (!mxs_auart_tx_empty(u) && to--)
> +		mdelay(1);

Maybe you can put some cond_resched() into the loop to avoid wasting too many 
cycles ?

> +}
> +
>  static void mxs_auart_start_tx(struct uart_port *u)
>  {
>  	struct mxs_auart_port *s = to_auart_port(u);
> @@ -828,6 +850,7 @@ static struct uart_ops mxs_auart_ops = {
>  	.get_mctrl      = mxs_auart_get_mctrl,
>  	.startup	= mxs_auart_startup,
>  	.shutdown       = mxs_auart_shutdown,
> +	.flush_buffer	= mxs_auart_flush_buffer,
>  	.set_termios    = mxs_auart_settermios,
>  	.type	   	= mxs_auart_type,
>  	.release_port   = mxs_auart_release_port,

Best regards,
Marek Vasut

[PATCH 3/3] serial: mxs-auart: move irq handling to a tasklet

[Marek Vasut]

Dear Hector Palacios,

> The ISR was handling CTS flag and RX/TX processing.
> Occasionally interrupts were coming in the middle of the ISR and
> led to blocking transmission (with hw flow) or loss of data.
> This patch moves this processing to a tasklet that the ISR schedules.
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  drivers/tty/serial/mxs-auart.c | 61
> +++++++++++++++++++++++++++--------------- 1 file changed, 39
> insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/mxs-auart.c
> b/drivers/tty/serial/mxs-auart.c index e5540c89dd48..7021269c70b2 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -148,6 +148,10 @@ struct mxs_auart_port {
>  	struct clk *clk;
>  	struct device *dev;
> 
> +	struct tasklet_struct tasklet;
> +	unsigned int irq_status;
> +	unsigned int status;
> +
>  	/* for DMA */
>  	struct scatterlist tx_sgl;
>  	struct dma_chan	*tx_dma_chan;
> @@ -680,38 +684,48 @@ static void mxs_auart_settermios(struct uart_port *u,
>  	}
>  }
> 
> +/*
> + * tasklet handling tty stuff outside the interrupt handler.
> + */
> +static void mxs_auart_tasklet_func(unsigned long data)
> +{
> +	struct uart_port *port = (struct uart_port *)data;
> +	struct mxs_auart_port *s = to_auart_port(port);
> +
> +	/* Handle status irq */
> +	if (s->irq_status & AUART_INTR_CTSMIS) {
> +		uart_handle_cts_change(&s->port, s->status & AUART_STAT_CTS);
> +		writel(AUART_INTR_CTSMIS,
> +		       s->port.membase + AUART_INTR_CLR);
> +	}
> +	/* Handle receive irq */
> +	if (s->irq_status & (AUART_INTR_RTIS | AUART_INTR_RXIS)) {
> +		if (!auart_dma_enabled(s))
> +			mxs_auart_rx_chars(s);
> +	}
> +	/* Handle transmit irq */
> +	if (s->irq_status & AUART_INTR_TXIS)
> +		mxs_auart_tx_chars(s);
> +}
> +
>  static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
>  {
> -	u32 istat;
>  	struct mxs_auart_port *s = context;
> -	u32 stat = readl(s->port.membase + AUART_STAT);
> 
> -	istat = readl(s->port.membase + AUART_INTR);
> +	s->status = readl(s->port.membase + AUART_STAT);
> +	s->irq_status = readl(s->port.membase + AUART_INTR);

Aren't you up for a nice race condition here if you get interrupt while checking 
the s->irq_status bits in the tasklet ?

[PATCH 1/3] serial: mxs-auart: implement flush_buffer hook

[Hector Palacios]

Hi Marek,

On 11/29/2013 05:50 PM, Marek Vasut wrote:
> Hello Hector,
>
>> Terminate any DMA transfer and verify the TX FIFO is empty.
>>
>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>> ---
>>   drivers/tty/serial/mxs-auart.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/tty/serial/mxs-auart.c
>> b/drivers/tty/serial/mxs-auart.c index 9f0461778fc1..d9bf6e103f65 100644
>> --- a/drivers/tty/serial/mxs-auart.c
>> +++ b/drivers/tty/serial/mxs-auart.c
>> @@ -782,6 +782,28 @@ static unsigned int mxs_auart_tx_empty(struct
>> uart_port *u) return 0;
>>   }
>>
>> +/*
>> + * Flush the transmit buffer.
>> + * Locking: called with port lock held and IRQs disabled.
>> + */
>> +static void mxs_auart_flush_buffer(struct uart_port *u)
>> +{
>> +	struct mxs_auart_port *s = to_auart_port(u);
>> +	struct dma_chan *channel = s->tx_dma_chan;
>> +	unsigned int to;
>> +
>> +	if (auart_dma_enabled(s)) {
>> +		/* Avoid deadlock with the DMA engine callback */
>> +		spin_unlock(&s->port.lock);
>> +		dmaengine_terminate_all(channel);
>> +		spin_lock(&s->port.lock);
>
> Can you not maybe just set some flag here to tell the DMA engine callback things
> are shutting down and to don't do anything funny anymore ?

I copied these spin_lock protections from the amba-pl011.c driver.
The callbacks in the mxs-auart driver are not taking the lock but I was not sure if 
any of the serial core functions they call are, so I thought it was a good idea to 
take it.

>> +	}
>> +	/* Wait for the FIFO to flush */
>> +	to = u->timeout;
>> +	while (!mxs_auart_tx_empty(u) && to--)
>> +		mdelay(1);
>
> Maybe you can put some cond_resched() into the loop to avoid wasting too many
> cycles ?

Do you mean like this? Ok

	while (!mxs_auart_tx_empty(u) && to--) {
		cond_resched();
		mdelay(1);
	}


Best regards,
--
Hector Palacios

[PATCH 1/3] serial: mxs-auart: implement flush_buffer hook

[Marek Vasut]

Dear Hector Palacios,

> Hi Marek,
> 
> On 11/29/2013 05:50 PM, Marek Vasut wrote:
> > Hello Hector,
> > 
> >> Terminate any DMA transfer and verify the TX FIFO is empty.
> >> 
> >> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> >> ---
> >> 
> >>   drivers/tty/serial/mxs-auart.c | 23 +++++++++++++++++++++++
> >>   1 file changed, 23 insertions(+)
> >> 
> >> diff --git a/drivers/tty/serial/mxs-auart.c
> >> b/drivers/tty/serial/mxs-auart.c index 9f0461778fc1..d9bf6e103f65 100644
> >> --- a/drivers/tty/serial/mxs-auart.c
> >> +++ b/drivers/tty/serial/mxs-auart.c
> >> @@ -782,6 +782,28 @@ static unsigned int mxs_auart_tx_empty(struct
> >> uart_port *u) return 0;
> >> 
> >>   }
> >> 
> >> +/*
> >> + * Flush the transmit buffer.
> >> + * Locking: called with port lock held and IRQs disabled.
> >> + */
> >> +static void mxs_auart_flush_buffer(struct uart_port *u)
> >> +{
> >> +	struct mxs_auart_port *s = to_auart_port(u);
> >> +	struct dma_chan *channel = s->tx_dma_chan;
> >> +	unsigned int to;
> >> +
> >> +	if (auart_dma_enabled(s)) {
> >> +		/* Avoid deadlock with the DMA engine callback */
> >> +		spin_unlock(&s->port.lock);
> >> +		dmaengine_terminate_all(channel);
> >> +		spin_lock(&s->port.lock);
> > 
> > Can you not maybe just set some flag here to tell the DMA engine callback
> > things are shutting down and to don't do anything funny anymore ?
> 
> I copied these spin_lock protections from the amba-pl011.c driver.
> The callbacks in the mxs-auart driver are not taking the lock but I was not
> sure if any of the serial core functions they call are, so I thought it
> was a good idea to take it.

It might be worth investigating this then.

> >> +	}
> >> +	/* Wait for the FIFO to flush */
> >> +	to = u->timeout;
> >> +	while (!mxs_auart_tx_empty(u) && to--)
> >> +		mdelay(1);
> > 
> > Maybe you can put some cond_resched() into the loop to avoid wasting too
> > many cycles ?
> 
> Do you mean like this? Ok
> 
> 	while (!mxs_auart_tx_empty(u) && to--) {
> 		cond_resched();
> 		mdelay(1);
> 	}

Yes, like that.

Best regards,
Marek Vasut

[PATCH 1/3] serial: mxs-auart: implement flush_buffer hook

[Lucas Stach]

Am Freitag, den 29.11.2013, 19:02 +0100 schrieb Marek Vasut:
> Dear Hector Palacios,
> 
> > Hi Marek,
> > 
> > On 11/29/2013 05:50 PM, Marek Vasut wrote:
> > > Hello Hector,
> > > 
> > >> Terminate any DMA transfer and verify the TX FIFO is empty.
> > >> 
> > >> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> > >> ---
> > >> 
> > >>   drivers/tty/serial/mxs-auart.c | 23 +++++++++++++++++++++++
> > >>   1 file changed, 23 insertions(+)
> > >> 
> > >> diff --git a/drivers/tty/serial/mxs-auart.c
> > >> b/drivers/tty/serial/mxs-auart.c index 9f0461778fc1..d9bf6e103f65 100644
> > >> --- a/drivers/tty/serial/mxs-auart.c
> > >> +++ b/drivers/tty/serial/mxs-auart.c
> > >> @@ -782,6 +782,28 @@ static unsigned int mxs_auart_tx_empty(struct
> > >> uart_port *u) return 0;
> > >> 
> > >>   }
> > >> 
> > >> +/*
> > >> + * Flush the transmit buffer.
> > >> + * Locking: called with port lock held and IRQs disabled.
> > >> + */
> > >> +static void mxs_auart_flush_buffer(struct uart_port *u)
> > >> +{
> > >> +	struct mxs_auart_port *s = to_auart_port(u);
> > >> +	struct dma_chan *channel = s->tx_dma_chan;
> > >> +	unsigned int to;
> > >> +
> > >> +	if (auart_dma_enabled(s)) {
> > >> +		/* Avoid deadlock with the DMA engine callback */
> > >> +		spin_unlock(&s->port.lock);
> > >> +		dmaengine_terminate_all(channel);
> > >> +		spin_lock(&s->port.lock);
> > > 
> > > Can you not maybe just set some flag here to tell the DMA engine callback
> > > things are shutting down and to don't do anything funny anymore ?
> > 
> > I copied these spin_lock protections from the amba-pl011.c driver.
> > The callbacks in the mxs-auart driver are not taking the lock but I was not
> > sure if any of the serial core functions they call are, so I thought it
> > was a good idea to take it.
> 
> It might be worth investigating this then.
> 
> > >> +	}
> > >> +	/* Wait for the FIFO to flush */
> > >> +	to = u->timeout;
> > >> +	while (!mxs_auart_tx_empty(u) && to--)
> > >> +		mdelay(1);
> > > 
> > > Maybe you can put some cond_resched() into the loop to avoid wasting too
> > > many cycles ?
> > 
> > Do you mean like this? Ok
> > 
> > 	while (!mxs_auart_tx_empty(u) && to--) {
> > 		cond_resched();
> > 		mdelay(1);
> > 	}
> 
> Yes, like that.
> 
This path isn't timing critical. mdelay still keeps the cpu spinning,
even with a resched. Just use an absolute timeout and sleep instead of
delay.

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 3/3] serial: mxs-auart: move irq handling to a tasklet

[Hector Palacios]

On 11/29/2013 06:00 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> The ISR was handling CTS flag and RX/TX processing.
>> Occasionally interrupts were coming in the middle of the ISR and
>> led to blocking transmission (with hw flow) or loss of data.
>> This patch moves this processing to a tasklet that the ISR schedules.
>>
>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>> ---
>>   drivers/tty/serial/mxs-auart.c | 61
>> +++++++++++++++++++++++++++--------------- 1 file changed, 39
>> insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/tty/serial/mxs-auart.c
>> b/drivers/tty/serial/mxs-auart.c index e5540c89dd48..7021269c70b2 100644
>> --- a/drivers/tty/serial/mxs-auart.c
>> +++ b/drivers/tty/serial/mxs-auart.c
>> @@ -148,6 +148,10 @@ struct mxs_auart_port {
>>   	struct clk *clk;
>>   	struct device *dev;
>>
>> +	struct tasklet_struct tasklet;
>> +	unsigned int irq_status;
>> +	unsigned int status;
>> +
>>   	/* for DMA */
>>   	struct scatterlist tx_sgl;
>>   	struct dma_chan	*tx_dma_chan;
>> @@ -680,38 +684,48 @@ static void mxs_auart_settermios(struct uart_port *u,
>>   	}
>>   }
>>
>> +/*
>> + * tasklet handling tty stuff outside the interrupt handler.
>> + */
>> +static void mxs_auart_tasklet_func(unsigned long data)
>> +{
>> +	struct uart_port *port = (struct uart_port *)data;
>> +	struct mxs_auart_port *s = to_auart_port(port);
>> +
>> +	/* Handle status irq */
>> +	if (s->irq_status & AUART_INTR_CTSMIS) {
>> +		uart_handle_cts_change(&s->port, s->status & AUART_STAT_CTS);
>> +		writel(AUART_INTR_CTSMIS,
>> +		       s->port.membase + AUART_INTR_CLR);
>> +	}
>> +	/* Handle receive irq */
>> +	if (s->irq_status & (AUART_INTR_RTIS | AUART_INTR_RXIS)) {
>> +		if (!auart_dma_enabled(s))
>> +			mxs_auart_rx_chars(s);
>> +	}
>> +	/* Handle transmit irq */
>> +	if (s->irq_status & AUART_INTR_TXIS)
>> +		mxs_auart_tx_chars(s);
>> +}
>> +
>>   static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
>>   {
>> -	u32 istat;
>>   	struct mxs_auart_port *s = context;
>> -	u32 stat = readl(s->port.membase + AUART_STAT);
>>
>> -	istat = readl(s->port.membase + AUART_INTR);
>> +	s->status = readl(s->port.membase + AUART_STAT);
>> +	s->irq_status = readl(s->port.membase + AUART_INTR);
>
> Aren't you up for a nice race condition here if you get interrupt while checking
> the s->irq_status bits in the tasklet ?

Sad, but true. :-(
But if I need to wait on the ISR for the tasklet to process then this patch loses its 
original meaning. Argh... it works so nicely!

Best regards,
--
Hector Palacios