[PATCH 0/3] serial: samsung: Fix UART status handling and other fixes

[PATCH 0/3] serial: samsung: Fix UART status handling and other fixes

[Robert Baldyga]

Hello,

This patch set contains three patches: two minor fixes and one quite
important bugfix enabling UART status handling in DMA mode, which was
missing so far. It enables, among others, 'break' contition handling,
which is necessary if we want to use Magic SysRq. So this patch fixes
Magic SysRq handling for serial consoles using UART in DMA mode,

Best regards,
Robert Baldyga

Robert Baldyga (3):
  serial: samsung: remove unused 'irq' parameter
  serial: samsung: remove unneded 'ignore_char' label
  serial: samsung: Fix UART status handling in DMA mode

 drivers/tty/serial/samsung.c | 151 ++++++++++++++++++-------------------------
 1 file changed, 64 insertions(+), 87 deletions(-)

-- 
1.9.1

[PATCH 1/3] serial: samsung: remove unused 'irq' parameter

[Robert Baldyga]

This parameter is not used anywhere, so we can get rid of it.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/tty/serial/samsung.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 856686d..fee764c 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -573,7 +573,7 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
 	ourport->rx_mode = S3C24XX_RX_PIO;
 }
 
-static irqreturn_t s3c24xx_serial_rx_chars_dma(int irq, void *dev_id)
+static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
 {
 	unsigned int utrstat, ufstat, received;
 	struct s3c24xx_uart_port *ourport = dev_id;
@@ -621,7 +621,7 @@ finish:
 	return IRQ_HANDLED;
 }
 
-static irqreturn_t s3c24xx_serial_rx_chars_pio(int irq, void *dev_id)
+static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 {
 	struct s3c24xx_uart_port *ourport = dev_id;
 	struct uart_port *port = &ourport->port;
@@ -718,8 +718,8 @@ static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id)
 	struct s3c24xx_uart_port *ourport = dev_id;
 
 	if (ourport->dma && ourport->dma->rx_chan)
-		return s3c24xx_serial_rx_chars_dma(irq, dev_id);
-	return s3c24xx_serial_rx_chars_pio(irq, dev_id);
+		return s3c24xx_serial_rx_chars_dma(dev_id);
+	return s3c24xx_serial_rx_chars_pio(dev_id);
 }
 
 static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
-- 
1.9.1

Re: [PATCH 1/3] serial: samsung: remove unused 'irq' parameter

[Krzysztof Kozlowski]

On 08.09.2015 18:38, Robert Baldyga wrote:
> This parameter is not used anywhere, so we can get rid of it.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/tty/serial/samsung.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

[PATCH 2/3] serial: samsung: remove unneded 'ignore_char' label

[Robert Baldyga]

This label does nothing special and we don't need to have it anymore.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/tty/serial/samsung.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index fee764c..2b05194 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -676,7 +676,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 				dbg("break!\n");
 				port->icount.brk++;
 				if (uart_handle_break(port))
-					goto ignore_char;
+					continue;
 			}
 
 			if (uerstat & S3C2410_UERSTAT_FRAME)
@@ -696,13 +696,10 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 		}
 
 		if (uart_handle_sysrq_char(port, ch))
-			goto ignore_char;
+			continue;
 
 		uart_insert_char(port, uerstat, S3C2410_UERSTAT_OVERRUN,
 				 ch, flag);
-
-ignore_char:
-		continue;
 	}
 
 	spin_unlock_irqrestore(&port->lock, flags);
-- 
1.9.1

Re: [PATCH 2/3] serial: samsung: remove unneded 'ignore_char' label

[Krzysztof Kozlowski]

On 08.09.2015 18:38, Robert Baldyga wrote:
> This label does nothing special and we don't need to have it anymore.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/tty/serial/samsung.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

Label does nothing but it has a meaning. It serves as a comment - ignore
the character. So maybe:

-					goto ignore_char;
+					continue; /* Ignore character */

What do you think?

Best regards,
Krzysztof


> 
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index fee764c..2b05194 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -676,7 +676,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>  				dbg("break!\n");
>  				port->icount.brk++;
>  				if (uart_handle_break(port))
> -					goto ignore_char;
> +					continue;
>  			}
>  
>  			if (uerstat & S3C2410_UERSTAT_FRAME)
> @@ -696,13 +696,10 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>  		}
>  
>  		if (uart_handle_sysrq_char(port, ch))
> -			goto ignore_char;
> +			continue;
>  
>  		uart_insert_char(port, uerstat, S3C2410_UERSTAT_OVERRUN,
>  				 ch, flag);
> -
> -ignore_char:
> -		continue;
>  	}
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
> 

Re: [PATCH 2/3] serial: samsung: remove unneded 'ignore_char' label

[Robert Baldyga]

On 09/10/2015 02:27 AM, Krzysztof Kozlowski wrote:
> On 08.09.2015 18:38, Robert Baldyga wrote:
>> This label does nothing special and we don't need to have it anymore.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/tty/serial/samsung.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> Label does nothing but it has a meaning. It serves as a comment - ignore
> the character. So maybe:
> 
> -					goto ignore_char;
> +					continue; /* Ignore character */
> 
> What do you think?

Look good to me.

Thanks,
Robert

[PATCH 3/3] serial: samsung: Fix UART status handling in DMA mode

[Robert Baldyga]

This patch fixes UART status handling in DMA mode. For this purpose we
introduce s3c24xx_serial_rx_drain_fifo() function, which contains common
RX handling code for both PIO mode and DMA mode (in case of interrupt).

Thanks to this we have, among others, 'break' condition handling needed
for Magic SysRq, which was missing in DMA mode so far. Since we can use
UART in DMA mode as serial console, it's quite important improvement.

This change additionally simplifies RX handling code, as we no longer
need uart_rx_drain_fifo() function, so we can remove it.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/tty/serial/samsung.c | 142 +++++++++++++++++++------------------------
 1 file changed, 61 insertions(+), 81 deletions(-)

diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index 2b05194..007bc5f 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -388,29 +388,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
 static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
 				     unsigned long ufstat);
 
-static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
-{
-	struct uart_port *port = &ourport->port;
-	struct tty_port *tty = &port->state->port;
-	unsigned int ch, ufstat;
-	unsigned int count;
-
-	ufstat = rd_regl(port, S3C2410_UFSTAT);
-	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
-
-	if (!count)
-		return;
-
-	while (count-- > 0) {
-		ch = rd_regb(port, S3C2410_URXH);
-
-		ourport->port.icount.rx++;
-		tty_insert_flip_char(tty, ch, TTY_NORMAL);
-	}
-
-	tty_flip_buffer_push(tty);
-}
-
 static void s3c24xx_serial_stop_rx(struct uart_port *port)
 {
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
@@ -573,64 +550,12 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
 	ourport->rx_mode = S3C24XX_RX_PIO;
 }
 
-static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
+static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 {
-	unsigned int utrstat, ufstat, received;
-	struct s3c24xx_uart_port *ourport = dev_id;
-	struct uart_port *port = &ourport->port;
-	struct s3c24xx_uart_dma *dma = ourport->dma;
-	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
-	struct tty_port *t = &port->state->port;
-	unsigned long flags;
-	struct dma_tx_state state;
-
-	utrstat = rd_regl(port, S3C2410_UTRSTAT);
-	ufstat = rd_regl(port, S3C2410_UFSTAT);
-
-	spin_lock_irqsave(&port->lock, flags);
-
-	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
-		s3c64xx_start_rx_dma(ourport);
-		if (ourport->rx_mode == S3C24XX_RX_PIO)
-			enable_rx_dma(ourport);
-		goto finish;
-	}
-
-	if (ourport->rx_mode == S3C24XX_RX_DMA) {
-		dmaengine_pause(dma->rx_chan);
-		dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
-		dmaengine_terminate_all(dma->rx_chan);
-		received = dma->rx_bytes_requested - state.residue;
-		s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
-
-		enable_rx_pio(ourport);
-	}
-
-	uart_rx_drain_fifo(ourport);
-
-	if (tty) {
-		tty_flip_buffer_push(t);
-		tty_kref_put(tty);
-	}
-
-	wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
-
-finish:
-	spin_unlock_irqrestore(&port->lock, flags);
-
-	return IRQ_HANDLED;
-}
-
-static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
-{
-	struct s3c24xx_uart_port *ourport = dev_id;
 	struct uart_port *port = &ourport->port;
 	unsigned int ufcon, ch, flag, ufstat, uerstat;
-	unsigned long flags;
 	int max_count = port->fifosize;
 
-	spin_lock_irqsave(&port->lock, flags);
-
 	while (max_count-- > 0) {
 		ufcon = rd_regl(port, S3C2410_UFCON);
 		ufstat = rd_regl(port, S3C2410_UFSTAT);
@@ -654,9 +579,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 					ufcon |= S3C2410_UFCON_RESETRX;
 					wr_regl(port, S3C2410_UFCON, ufcon);
 					rx_enabled(port) = 1;
-					spin_unlock_irqrestore(&port->lock,
-							flags);
-					goto out;
+					return;
 				}
 				continue;
 			}
@@ -702,10 +625,67 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
 				 ch, flag);
 	}
 
-	spin_unlock_irqrestore(&port->lock, flags);
 	tty_flip_buffer_push(&port->state->port);
+}
+
+static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
+{
+	unsigned int utrstat, ufstat, received;
+	struct s3c24xx_uart_port *ourport = dev_id;
+	struct uart_port *port = &ourport->port;
+	struct s3c24xx_uart_dma *dma = ourport->dma;
+	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
+	struct tty_port *t = &port->state->port;
+	unsigned long flags;
+	struct dma_tx_state state;
+
+	utrstat = rd_regl(port, S3C2410_UTRSTAT);
+	ufstat = rd_regl(port, S3C2410_UFSTAT);
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
+		s3c64xx_start_rx_dma(ourport);
+		if (ourport->rx_mode == S3C24XX_RX_PIO)
+			enable_rx_dma(ourport);
+		goto finish;
+	}
+
+	if (ourport->rx_mode == S3C24XX_RX_DMA) {
+		dmaengine_pause(dma->rx_chan);
+		dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
+		dmaengine_terminate_all(dma->rx_chan);
+		received = dma->rx_bytes_requested - state.residue;
+		s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
+
+		enable_rx_pio(ourport);
+	}
+
+	s3c24xx_serial_rx_drain_fifo(ourport);
+
+	if (tty) {
+		tty_flip_buffer_push(t);
+		tty_kref_put(tty);
+	}
+
+	wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
+
+finish:
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
+{
+	struct s3c24xx_uart_port *ourport = dev_id;
+	struct uart_port *port = &ourport->port;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	s3c24xx_serial_rx_drain_fifo(ourport);
+	spin_unlock_irqrestore(&port->lock, flags);
 
-out:
 	return IRQ_HANDLED;
 }
 
-- 
1.9.1

Re: [PATCH 3/3] serial: samsung: Fix UART status handling in DMA mode

[Krzysztof Kozlowski]

On 08.09.2015 18:38, Robert Baldyga wrote:
> This patch fixes UART status handling in DMA mode.

Please describe the issue - what kind of issues are in UART status
handling? I think this fixes a specific bug which can be observed by the
user, so it should be mentioned here. You mention this a little bit
later but I would miss it if I were not aware earlier of the issue.

> For this purpose we
> introduce s3c24xx_serial_rx_drain_fifo() function, which contains common
> RX handling code for both PIO mode and DMA mode (in case of interrupt).
> 
> Thanks to this we have, among others, 'break' condition handling needed
> for Magic SysRq, which was missing in DMA mode so far. Since we can use
> UART in DMA mode as serial console, it's quite important improvement.
> 
> This change additionally simplifies RX handling code, as we no longer
> need uart_rx_drain_fifo() function, so we can remove it.

Maybe it's because I don't know the driver but I have difficulties to
spot the exact fix. There is significant code movement... To spot the
difference I even applied the patch, it helped...

I think it could be split for easier understanding. First introduce
common code (s3c24xx_serial_rx_drain_fifo()) and then fix the issue.
Yeah, I am grumpy but really the fix should be made visible (easier to
spot).

Best regards,
Krzysztof

> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/tty/serial/samsung.c | 142 +++++++++++++++++++------------------------
>  1 file changed, 61 insertions(+), 81 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index 2b05194..007bc5f 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -388,29 +388,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>  static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
>  				     unsigned long ufstat);
>  
> -static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
> -{
> -	struct uart_port *port = &ourport->port;
> -	struct tty_port *tty = &port->state->port;
> -	unsigned int ch, ufstat;
> -	unsigned int count;
> -
> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
> -	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
> -
> -	if (!count)
> -		return;
> -
> -	while (count-- > 0) {
> -		ch = rd_regb(port, S3C2410_URXH);
> -
> -		ourport->port.icount.rx++;
> -		tty_insert_flip_char(tty, ch, TTY_NORMAL);
> -	}
> -
> -	tty_flip_buffer_push(tty);
> -}
> -
>  static void s3c24xx_serial_stop_rx(struct uart_port *port)
>  {
>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
> @@ -573,64 +550,12 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>  	ourport->rx_mode = S3C24XX_RX_PIO;
>  }
>  
> -static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>  {
> -	unsigned int utrstat, ufstat, received;
> -	struct s3c24xx_uart_port *ourport = dev_id;
> -	struct uart_port *port = &ourport->port;
> -	struct s3c24xx_uart_dma *dma = ourport->dma;
> -	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
> -	struct tty_port *t = &port->state->port;
> -	unsigned long flags;
> -	struct dma_tx_state state;
> -
> -	utrstat = rd_regl(port, S3C2410_UTRSTAT);
> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
> -
> -	spin_lock_irqsave(&port->lock, flags);
> -
> -	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
> -		s3c64xx_start_rx_dma(ourport);
> -		if (ourport->rx_mode == S3C24XX_RX_PIO)
> -			enable_rx_dma(ourport);
> -		goto finish;
> -	}
> -
> -	if (ourport->rx_mode == S3C24XX_RX_DMA) {
> -		dmaengine_pause(dma->rx_chan);
> -		dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
> -		dmaengine_terminate_all(dma->rx_chan);
> -		received = dma->rx_bytes_requested - state.residue;
> -		s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
> -
> -		enable_rx_pio(ourport);
> -	}
> -
> -	uart_rx_drain_fifo(ourport);
> -
> -	if (tty) {
> -		tty_flip_buffer_push(t);
> -		tty_kref_put(tty);
> -	}
> -
> -	wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
> -
> -finish:
> -	spin_unlock_irqrestore(&port->lock, flags);
> -
> -	return IRQ_HANDLED;
> -}
> -
> -static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
> -{
> -	struct s3c24xx_uart_port *ourport = dev_id;
>  	struct uart_port *port = &ourport->port;
>  	unsigned int ufcon, ch, flag, ufstat, uerstat;
> -	unsigned long flags;
>  	int max_count = port->fifosize;
>  
> -	spin_lock_irqsave(&port->lock, flags);
> -
>  	while (max_count-- > 0) {
>  		ufcon = rd_regl(port, S3C2410_UFCON);
>  		ufstat = rd_regl(port, S3C2410_UFSTAT);
> @@ -654,9 +579,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>  					ufcon |= S3C2410_UFCON_RESETRX;
>  					wr_regl(port, S3C2410_UFCON, ufcon);
>  					rx_enabled(port) = 1;
> -					spin_unlock_irqrestore(&port->lock,
> -							flags);
> -					goto out;
> +					return;
>  				}
>  				continue;
>  			}
> @@ -702,10 +625,67 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>  				 ch, flag);
>  	}
>  
> -	spin_unlock_irqrestore(&port->lock, flags);
>  	tty_flip_buffer_push(&port->state->port);
> +}
> +
> +static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
> +{
> +	unsigned int utrstat, ufstat, received;
> +	struct s3c24xx_uart_port *ourport = dev_id;
> +	struct uart_port *port = &ourport->port;
> +	struct s3c24xx_uart_dma *dma = ourport->dma;
> +	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
> +	struct tty_port *t = &port->state->port;
> +	unsigned long flags;
> +	struct dma_tx_state state;
> +
> +	utrstat = rd_regl(port, S3C2410_UTRSTAT);
> +	ufstat = rd_regl(port, S3C2410_UFSTAT);
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +
> +	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
> +		s3c64xx_start_rx_dma(ourport);
> +		if (ourport->rx_mode == S3C24XX_RX_PIO)
> +			enable_rx_dma(ourport);
> +		goto finish;
> +	}
> +
> +	if (ourport->rx_mode == S3C24XX_RX_DMA) {
> +		dmaengine_pause(dma->rx_chan);
> +		dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
> +		dmaengine_terminate_all(dma->rx_chan);
> +		received = dma->rx_bytes_requested - state.residue;
> +		s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
> +
> +		enable_rx_pio(ourport);
> +	}
> +
> +	s3c24xx_serial_rx_drain_fifo(ourport);
> +
> +	if (tty) {
> +		tty_flip_buffer_push(t);
> +		tty_kref_put(tty);
> +	}
> +
> +	wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
> +
> +finish:
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
> +{
> +	struct s3c24xx_uart_port *ourport = dev_id;
> +	struct uart_port *port = &ourport->port;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&port->lock, flags);
> +	s3c24xx_serial_rx_drain_fifo(ourport);
> +	spin_unlock_irqrestore(&port->lock, flags);
>  
> -out:
>  	return IRQ_HANDLED;
>  }
>  
> 

Re: [PATCH 3/3] serial: samsung: Fix UART status handling in DMA mode

[Robert Baldyga]

On 09/10/2015 02:47 AM, Krzysztof Kozlowski wrote:
> On 08.09.2015 18:38, Robert Baldyga wrote:
>> This patch fixes UART status handling in DMA mode.
> 
> Please describe the issue - what kind of issues are in UART status
> handling? I think this fixes a specific bug which can be observed by the
> user, so it should be mentioned here. You mention this a little bit
> later but I would miss it if I were not aware earlier of the issue.
> 
>> For this purpose we
>> introduce s3c24xx_serial_rx_drain_fifo() function, which contains common
>> RX handling code for both PIO mode and DMA mode (in case of interrupt).
>>
>> Thanks to this we have, among others, 'break' condition handling needed
>> for Magic SysRq, which was missing in DMA mode so far. Since we can use
>> UART in DMA mode as serial console, it's quite important improvement.
>>
>> This change additionally simplifies RX handling code, as we no longer
>> need uart_rx_drain_fifo() function, so we can remove it.
> 
> Maybe it's because I don't know the driver but I have difficulties to
> spot the exact fix. There is significant code movement... To spot the
> difference I even applied the patch, it helped...
> 
> I think it could be split for easier understanding. First introduce
> common code (s3c24xx_serial_rx_drain_fifo()) and then fix the issue.
> Yeah, I am grumpy but really the fix should be made visible (easier to
> spot).
> 

Well, what I did is quite simple, but indeed the result diff looks ugly.
The change is: instead of doing simple FIFO drain when interrupt occurs
(which can be caused by simple timeout or by special condition like
'break') I'm calling function which drains FIFO *and checks the special
conditions*. So now we have special condition handling in DMA mode which
was missing. To avoid code duplication I have separated this function
from PIO handling code.

I can try to split this change into some steps. Maybe it will produce
some more readable diff ;)

Thanks,
Robert

>>
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/tty/serial/samsung.c | 142 +++++++++++++++++++------------------------
>>  1 file changed, 61 insertions(+), 81 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index 2b05194..007bc5f 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -388,29 +388,6 @@ static void s3c24xx_uart_copy_rx_to_tty(struct s3c24xx_uart_port *ourport,
>>  static int s3c24xx_serial_rx_fifocnt(struct s3c24xx_uart_port *ourport,
>>  				     unsigned long ufstat);
>>  
>> -static void uart_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>> -{
>> -	struct uart_port *port = &ourport->port;
>> -	struct tty_port *tty = &port->state->port;
>> -	unsigned int ch, ufstat;
>> -	unsigned int count;
>> -
>> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
>> -	count = s3c24xx_serial_rx_fifocnt(ourport, ufstat);
>> -
>> -	if (!count)
>> -		return;
>> -
>> -	while (count-- > 0) {
>> -		ch = rd_regb(port, S3C2410_URXH);
>> -
>> -		ourport->port.icount.rx++;
>> -		tty_insert_flip_char(tty, ch, TTY_NORMAL);
>> -	}
>> -
>> -	tty_flip_buffer_push(tty);
>> -}
>> -
>>  static void s3c24xx_serial_stop_rx(struct uart_port *port)
>>  {
>>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
>> @@ -573,64 +550,12 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
>>  	ourport->rx_mode = S3C24XX_RX_PIO;
>>  }
>>  
>> -static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>> +static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>>  {
>> -	unsigned int utrstat, ufstat, received;
>> -	struct s3c24xx_uart_port *ourport = dev_id;
>> -	struct uart_port *port = &ourport->port;
>> -	struct s3c24xx_uart_dma *dma = ourport->dma;
>> -	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
>> -	struct tty_port *t = &port->state->port;
>> -	unsigned long flags;
>> -	struct dma_tx_state state;
>> -
>> -	utrstat = rd_regl(port, S3C2410_UTRSTAT);
>> -	ufstat = rd_regl(port, S3C2410_UFSTAT);
>> -
>> -	spin_lock_irqsave(&port->lock, flags);
>> -
>> -	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
>> -		s3c64xx_start_rx_dma(ourport);
>> -		if (ourport->rx_mode == S3C24XX_RX_PIO)
>> -			enable_rx_dma(ourport);
>> -		goto finish;
>> -	}
>> -
>> -	if (ourport->rx_mode == S3C24XX_RX_DMA) {
>> -		dmaengine_pause(dma->rx_chan);
>> -		dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
>> -		dmaengine_terminate_all(dma->rx_chan);
>> -		received = dma->rx_bytes_requested - state.residue;
>> -		s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
>> -
>> -		enable_rx_pio(ourport);
>> -	}
>> -
>> -	uart_rx_drain_fifo(ourport);
>> -
>> -	if (tty) {
>> -		tty_flip_buffer_push(t);
>> -		tty_kref_put(tty);
>> -	}
>> -
>> -	wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
>> -
>> -finish:
>> -	spin_unlock_irqrestore(&port->lock, flags);
>> -
>> -	return IRQ_HANDLED;
>> -}
>> -
>> -static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>> -{
>> -	struct s3c24xx_uart_port *ourport = dev_id;
>>  	struct uart_port *port = &ourport->port;
>>  	unsigned int ufcon, ch, flag, ufstat, uerstat;
>> -	unsigned long flags;
>>  	int max_count = port->fifosize;
>>  
>> -	spin_lock_irqsave(&port->lock, flags);
>> -
>>  	while (max_count-- > 0) {
>>  		ufcon = rd_regl(port, S3C2410_UFCON);
>>  		ufstat = rd_regl(port, S3C2410_UFSTAT);
>> @@ -654,9 +579,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>>  					ufcon |= S3C2410_UFCON_RESETRX;
>>  					wr_regl(port, S3C2410_UFCON, ufcon);
>>  					rx_enabled(port) = 1;
>> -					spin_unlock_irqrestore(&port->lock,
>> -							flags);
>> -					goto out;
>> +					return;
>>  				}
>>  				continue;
>>  			}
>> @@ -702,10 +625,67 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>>  				 ch, flag);
>>  	}
>>  
>> -	spin_unlock_irqrestore(&port->lock, flags);
>>  	tty_flip_buffer_push(&port->state->port);
>> +}
>> +
>> +static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
>> +{
>> +	unsigned int utrstat, ufstat, received;
>> +	struct s3c24xx_uart_port *ourport = dev_id;
>> +	struct uart_port *port = &ourport->port;
>> +	struct s3c24xx_uart_dma *dma = ourport->dma;
>> +	struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
>> +	struct tty_port *t = &port->state->port;
>> +	unsigned long flags;
>> +	struct dma_tx_state state;
>> +
>> +	utrstat = rd_regl(port, S3C2410_UTRSTAT);
>> +	ufstat = rd_regl(port, S3C2410_UFSTAT);
>> +
>> +	spin_lock_irqsave(&port->lock, flags);
>> +
>> +	if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
>> +		s3c64xx_start_rx_dma(ourport);
>> +		if (ourport->rx_mode == S3C24XX_RX_PIO)
>> +			enable_rx_dma(ourport);
>> +		goto finish;
>> +	}
>> +
>> +	if (ourport->rx_mode == S3C24XX_RX_DMA) {
>> +		dmaengine_pause(dma->rx_chan);
>> +		dmaengine_tx_status(dma->rx_chan, dma->rx_cookie, &state);
>> +		dmaengine_terminate_all(dma->rx_chan);
>> +		received = dma->rx_bytes_requested - state.residue;
>> +		s3c24xx_uart_copy_rx_to_tty(ourport, t, received);
>> +
>> +		enable_rx_pio(ourport);
>> +	}
>> +
>> +	s3c24xx_serial_rx_drain_fifo(ourport);
>> +
>> +	if (tty) {
>> +		tty_flip_buffer_push(t);
>> +		tty_kref_put(tty);
>> +	}
>> +
>> +	wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
>> +
>> +finish:
>> +	spin_unlock_irqrestore(&port->lock, flags);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
>> +{
>> +	struct s3c24xx_uart_port *ourport = dev_id;
>> +	struct uart_port *port = &ourport->port;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&port->lock, flags);
>> +	s3c24xx_serial_rx_drain_fifo(ourport);
>> +	spin_unlock_irqrestore(&port->lock, flags);
>>  
>> -out:
>>  	return IRQ_HANDLED;
>>  }
>>  
>>
> 
> 

Re: [PATCH 0/3] serial: samsung: Fix UART status handling and other fixes

[Krzysztof Kozlowski]

On 08.09.2015 18:38, Robert Baldyga wrote:
> Hello,
> 
> This patch set contains three patches: two minor fixes and one quite
> important bugfix enabling UART status handling in DMA mode, which was
> missing so far. It enables, among others, 'break' contition handling,
> which is necessary if we want to use Magic SysRq. So this patch fixes
> Magic SysRq handling for serial consoles using UART in DMA mode,

Is it even worth using UART for serial console in DMA mode? How many
benefits it brings? Anyone measured real data?

The patch which enabled DMA for all serial ports on Exynos4 uncovered
some issues. That is one benefit... but was it worth it?

Best regards,
Krzysztof

Re: [PATCH 0/3] serial: samsung: Fix UART status handling and other fixes

[Robert Baldyga]

On 09/09/2015 01:47 AM, Krzysztof Kozlowski wrote:
> On 08.09.2015 18:38, Robert Baldyga wrote:
>> Hello,
>>
>> This patch set contains three patches: two minor fixes and one quite
>> important bugfix enabling UART status handling in DMA mode, which was
>> missing so far. It enables, among others, 'break' contition handling,
>> which is necessary if we want to use Magic SysRq. So this patch fixes
>> Magic SysRq handling for serial consoles using UART in DMA mode,
> 
> Is it even worth using UART for serial console in DMA mode? How many
> benefits it brings? Anyone measured real data?
> 
> The patch which enabled DMA for all serial ports on Exynos4 uncovered
> some issues. That is one benefit... but was it worth it?

Well, UART in DMA mode should work at least as good as in PIO, so there
is nothing wrong in having serial console configured in DMA. Sure, it
rather barely reduces number of interrupts while used as typical serial
console, but in some cases (eg. using zmodem) it can be more profitable.

To be honest, I have no strong feeling about that. I've decided to
utilize new possibility opened by introducing DMA mode in serial driver,
so I've send patch enabling it for each port on Exynos4. But if it
turned out to be unneeded I will not insist on it.

Best regards,
Robert

Re: [PATCH 0/3] serial: samsung: Fix UART status handling and other fixes

[Krzysztof Kozlowski]

On 09.09.2015 18:15, Robert Baldyga wrote:
> On 09/09/2015 01:47 AM, Krzysztof Kozlowski wrote:
>> On 08.09.2015 18:38, Robert Baldyga wrote:
>>> Hello,
>>>
>>> This patch set contains three patches: two minor fixes and one quite
>>> important bugfix enabling UART status handling in DMA mode, which was
>>> missing so far. It enables, among others, 'break' contition handling,
>>> which is necessary if we want to use Magic SysRq. So this patch fixes
>>> Magic SysRq handling for serial consoles using UART in DMA mode,
>>
>> Is it even worth using UART for serial console in DMA mode? How many
>> benefits it brings? Anyone measured real data?
>>
>> The patch which enabled DMA for all serial ports on Exynos4 uncovered
>> some issues. That is one benefit... but was it worth it?
> 
> Well, UART in DMA mode should work at least as good as in PIO, so there
> is nothing wrong in having serial console configured in DMA. Sure, it
> rather barely reduces number of interrupts while used as typical serial
> console, but in some cases (eg. using zmodem) it can be more profitable.

That's some reason... and mentioned earlier testing of DMA which
uncovered some interesting issues. :)

> 
> To be honest, I have no strong feeling about that. I've decided to
> utilize new possibility opened by introducing DMA mode in serial driver,
> so I've send patch enabling it for each port on Exynos4. But if it
> turned out to be unneeded I will not insist on it.

I was just thinking loud... I don't have any data neither so let's go
ahead with serial DMA.

Best regards,
Krzysztof