[PATCH 0/2] tty/serial: atmel: Fix RS485 half duplex operation

[PATCH 0/2] tty/serial: atmel: Fix RS485 half duplex operation

[Razvan Stefanescu]

Using a loopback serial cable with RS485 protocol shows that data is
received:
$ stty -F /dev/ttyS3 raw -echo speed 4800
$ cat /dev/ttyS3 &
$ echo "Hello, world" > /dev/ttyS3
Hello, world

Last line should not be displayed, as it indicates that RX was started
before TX finished.

This happens because driver activates RX when the DMA transfer
completes, but that does not necessarily mean the TX FIFO was emptied.

First patch will add a helper that checks if the transmission is
half-duplex and uses it throughout the driver, replacing multiple lines
of code.

Second patch implements the fix by adding a variable to the port struct.
This is used to indicate that RX needs to be started. When the DMA
transfer completes, the variable is set and the ATMEL_US_TXEMPTY is
reactivated. In the interrupt handler, if the variable is set, RX is
started.

Razvan Stefanescu (2):
  tty/serial: atmel: Add is_half_duplex helper
  tty/serial: atmel: RS485 HD w/DMA: enable RX after TX is stopped

 drivers/tty/serial/atmel_serial.c | 52 +++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 16 deletions(-)

-- 
2.19.1

[PATCH 1/2] tty/serial: atmel: Add is_half_duplex helper

[Razvan Stefanescu]

Use a helper function to check that a port needs to use half duplex
communication, replacing several occurrences of multi-line bit checking.

Signed-off-by: Razvan Stefanescu <razvan.stefanescu@microchip.com>
---
 drivers/tty/serial/atmel_serial.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 05147fe24343..a6577b1c4461 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -231,6 +231,13 @@ static inline void atmel_uart_write_char(struct uart_port *port, u8 value)
 	__raw_writeb(value, port->membase + ATMEL_US_THR);
 }
 
+static inline int atmel_uart_is_half_duplex(struct uart_port *port)
+{
+	return ((port->rs485.flags & SER_RS485_ENABLED) &&
+		!(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+		(port->iso7816.flags & SER_ISO7816_ENABLED);
+}
+
 #ifdef CONFIG_SERIAL_ATMEL_PDC
 static bool atmel_use_pdc_rx(struct uart_port *port)
 {
@@ -608,10 +615,10 @@ static void atmel_stop_tx(struct uart_port *port)
 	/* Disable interrupts */
 	atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
 
-	if (((port->rs485.flags & SER_RS485_ENABLED) &&
-	     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
-	    port->iso7816.flags & SER_ISO7816_ENABLED)
-		atmel_start_rx(port);
+	if (atmel_uart_is_half_duplex(port))
+		if (!atomic_read(&atmel_port->tasklet_shutdown))
+			atmel_start_rx(port);
+
 }
 
 /*
@@ -628,9 +635,7 @@ static void atmel_start_tx(struct uart_port *port)
 		return;
 
 	if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
-		if (((port->rs485.flags & SER_RS485_ENABLED) &&
-		     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
-		    port->iso7816.flags & SER_ISO7816_ENABLED)
+		if (atmel_uart_is_half_duplex(port))
 			atmel_stop_rx(port);
 
 	if (atmel_use_pdc_tx(port))
@@ -928,9 +933,7 @@ static void atmel_complete_tx_dma(void *arg)
 	 */
 	if (!uart_circ_empty(xmit))
 		atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
-	else if (((port->rs485.flags & SER_RS485_ENABLED) &&
-		  !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
-		 port->iso7816.flags & SER_ISO7816_ENABLED) {
+	else if (atmel_uart_is_half_duplex(port)) {
 		/* DMA done, stop TX, start RX for RS485 */
 		atmel_start_rx(port);
 	}
@@ -1508,9 +1511,7 @@ static void atmel_tx_pdc(struct uart_port *port)
 		atmel_uart_writel(port, ATMEL_US_IER,
 				  atmel_port->tx_done_mask);
 	} else {
-		if (((port->rs485.flags & SER_RS485_ENABLED) &&
-		     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
-		    port->iso7816.flags & SER_ISO7816_ENABLED) {
+		if (atmel_uart_is_half_duplex(port)) {
 			/* DMA done, stop TX, start RX for RS485 */
 			atmel_start_rx(port);
 		}
-- 
2.19.1

Re: [PATCH 1/2] tty/serial: atmel: Add is_half_duplex helper

[Richard Genoud]

[ adding Gil Weber in Cc: ]

Le 15/03/2019 à 10:23, Razvan Stefanescu a écrit :
> Use a helper function to check that a port needs to use half duplex
> communication, replacing several occurrences of multi-line bit checking.
> 
> Signed-off-by: Razvan Stefanescu <razvan.stefanescu@microchip.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 05147fe24343..a6577b1c4461 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -231,6 +231,13 @@ static inline void atmel_uart_write_char(struct uart_port *port, u8 value)
>  	__raw_writeb(value, port->membase + ATMEL_US_THR);
>  }
>  
> +static inline int atmel_uart_is_half_duplex(struct uart_port *port)
> +{
> +	return ((port->rs485.flags & SER_RS485_ENABLED) &&
> +		!(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> +		(port->iso7816.flags & SER_ISO7816_ENABLED);
> +}
> +
>  #ifdef CONFIG_SERIAL_ATMEL_PDC
>  static bool atmel_use_pdc_rx(struct uart_port *port)
>  {
> @@ -608,10 +615,10 @@ static void atmel_stop_tx(struct uart_port *port)
>  	/* Disable interrupts */
>  	atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
>  
> -	if (((port->rs485.flags & SER_RS485_ENABLED) &&
> -	     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> -	    port->iso7816.flags & SER_ISO7816_ENABLED)
> -		atmel_start_rx(port);
> +	if (atmel_uart_is_half_duplex(port))
> +		if (!atomic_read(&atmel_port->tasklet_shutdown))
This check wasn't there before.
If it's really needed, please insert it in another patch and explain why
it's needed.

> +			atmel_start_rx(port);
> +
>  }
>  
>  /*
> @@ -628,9 +635,7 @@ static void atmel_start_tx(struct uart_port *port)
>  		return;
>  
>  	if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
> -		if (((port->rs485.flags & SER_RS485_ENABLED) &&
> -		     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> -		    port->iso7816.flags & SER_ISO7816_ENABLED)
> +		if (atmel_uart_is_half_duplex(port))
>  			atmel_stop_rx(port);
>  
>  	if (atmel_use_pdc_tx(port))
> @@ -928,9 +933,7 @@ static void atmel_complete_tx_dma(void *arg)
>  	 */
>  	if (!uart_circ_empty(xmit))
>  		atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
> -	else if (((port->rs485.flags & SER_RS485_ENABLED) &&
> -		  !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> -		 port->iso7816.flags & SER_ISO7816_ENABLED) {
> +	else if (atmel_uart_is_half_duplex(port)) {
>  		/* DMA done, stop TX, start RX for RS485 */
>  		atmel_start_rx(port);
>  	}
> @@ -1508,9 +1511,7 @@ static void atmel_tx_pdc(struct uart_port *port)
>  		atmel_uart_writel(port, ATMEL_US_IER,
>  				  atmel_port->tx_done_mask);
>  	} else {
> -		if (((port->rs485.flags & SER_RS485_ENABLED) &&
> -		     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> -		    port->iso7816.flags & SER_ISO7816_ENABLED) {
> +		if (atmel_uart_is_half_duplex(port)) {
>  			/* DMA done, stop TX, start RX for RS485 */
>  			atmel_start_rx(port);
>  		}
> 

Re: [PATCH 1/2] tty/serial: atmel: Add is_half_duplex helper

[Razvan.Stefanescu]

On 18/03/2019 15:41, Richard Genoud wrote:
> [ adding Gil Weber in Cc: ]
> 
> Le 15/03/2019 à 10:23, Razvan Stefanescu a écrit :
>> Use a helper function to check that a port needs to use half duplex
>> communication, replacing several occurrences of multi-line bit checking.
>>
>> Signed-off-by: Razvan Stefanescu <razvan.stefanescu@microchip.com>
>> ---
>>   drivers/tty/serial/atmel_serial.c | 27 ++++++++++++++-------------
>>   1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
>> index 05147fe24343..a6577b1c4461 100644
>> --- a/drivers/tty/serial/atmel_serial.c
>> +++ b/drivers/tty/serial/atmel_serial.c
>> @@ -231,6 +231,13 @@ static inline void atmel_uart_write_char(struct uart_port *port, u8 value)
>>   	__raw_writeb(value, port->membase + ATMEL_US_THR);
>>   }
>>   
>> +static inline int atmel_uart_is_half_duplex(struct uart_port *port)
>> +{
>> +	return ((port->rs485.flags & SER_RS485_ENABLED) &&
>> +		!(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
>> +		(port->iso7816.flags & SER_ISO7816_ENABLED);
>> +}
>> +
>>   #ifdef CONFIG_SERIAL_ATMEL_PDC
>>   static bool atmel_use_pdc_rx(struct uart_port *port)
>>   {
>> @@ -608,10 +615,10 @@ static void atmel_stop_tx(struct uart_port *port)
>>   	/* Disable interrupts */
>>   	atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
>>   
>> -	if (((port->rs485.flags & SER_RS485_ENABLED) &&
>> -	     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
>> -	    port->iso7816.flags & SER_ISO7816_ENABLED)
>> -		atmel_start_rx(port);
>> +	if (atmel_uart_is_half_duplex(port))
>> +		if (!atomic_read(&atmel_port->tasklet_shutdown))
> This check wasn't there before.
> If it's really needed, please insert it in another patch and explain why
> it's needed.
> 
Thank you for the feedback. It is not needed, will be removed in v2.

Best regards,
Razvan

>> +			atmel_start_rx(port);
>> +
>>   }
>>   
>>   /*
>> @@ -628,9 +635,7 @@ static void atmel_start_tx(struct uart_port *port)
>>   		return;
>>   
>>   	if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
>> -		if (((port->rs485.flags & SER_RS485_ENABLED) &&
>> -		     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
>> -		    port->iso7816.flags & SER_ISO7816_ENABLED)
>> +		if (atmel_uart_is_half_duplex(port))
>>   			atmel_stop_rx(port);
>>   
>>   	if (atmel_use_pdc_tx(port))
>> @@ -928,9 +933,7 @@ static void atmel_complete_tx_dma(void *arg)
>>   	 */
>>   	if (!uart_circ_empty(xmit))
>>   		atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
>> -	else if (((port->rs485.flags & SER_RS485_ENABLED) &&
>> -		  !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
>> -		 port->iso7816.flags & SER_ISO7816_ENABLED) {
>> +	else if (atmel_uart_is_half_duplex(port)) {
>>   		/* DMA done, stop TX, start RX for RS485 */
>>   		atmel_start_rx(port);
>>   	}
>> @@ -1508,9 +1511,7 @@ static void atmel_tx_pdc(struct uart_port *port)
>>   		atmel_uart_writel(port, ATMEL_US_IER,
>>   				  atmel_port->tx_done_mask);
>>   	} else {
>> -		if (((port->rs485.flags & SER_RS485_ENABLED) &&
>> -		     !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
>> -		    port->iso7816.flags & SER_ISO7816_ENABLED) {
>> +		if (atmel_uart_is_half_duplex(port)) {
>>   			/* DMA done, stop TX, start RX for RS485 */
>>   			atmel_start_rx(port);
>>   		}
>>
> 

[PATCH 2/2] tty/serial: atmel: RS485 HD w/DMA: enable RX after TX is stopped

[Razvan Stefanescu]

In half-duplex operation, RX should be started after TX completes.

If DMA is used, there is a case when the DMA transfer completes but the
TX FIFO is not emptied, so the RX cannot be restarted just yet.

Use a boolean variable to store this state and rearm TX interrupt mask
to be signaled again that the transfer finished. In interrupt transmit
handler this variable is used to start RX.

Signed-off-by: Razvan Stefanescu <razvan.stefanescu@microchip.com>
---
 drivers/tty/serial/atmel_serial.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index a6577b1c4461..b0141dcbbb61 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -166,6 +166,8 @@ struct atmel_uart_port {
 	unsigned int		pending_status;
 	spinlock_t		lock_suspended;
 
+	bool			hd_start_rx;	/* can start RX during half-duplex operation */
+
 	/* ISO7816 */
 	unsigned int		fidi_min;
 	unsigned int		fidi_max;
@@ -934,8 +936,13 @@ static void atmel_complete_tx_dma(void *arg)
 	if (!uart_circ_empty(xmit))
 		atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
 	else if (atmel_uart_is_half_duplex(port)) {
-		/* DMA done, stop TX, start RX for RS485 */
-		atmel_start_rx(port);
+		/*
+		 * DMA done, re-enable TXEMPTY and signal that we can stop
+		 * TX and start RX for RS485
+		 */
+		atmel_port->hd_start_rx = true;
+		atmel_uart_writel(port, ATMEL_US_IER,
+				  atmel_port->tx_done_mask);
 	}
 
 	spin_unlock_irqrestore(&port->lock, flags);
@@ -1379,9 +1386,21 @@ atmel_handle_transmit(struct uart_port *port, unsigned int pending)
 	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
 
 	if (pending & atmel_port->tx_done_mask) {
-		/* Either PDC or interrupt transmission */
 		atmel_uart_writel(port, ATMEL_US_IDR,
 				  atmel_port->tx_done_mask);
+
+		/* Start RX if flag was set and FIFO is empty */
+		if (atmel_port->hd_start_rx) {
+			if (atmel_uart_readl(port, ATMEL_US_CSR)
+					& ATMEL_US_TXEMPTY) {
+				atmel_port->hd_start_rx = false;
+				atmel_start_rx(port);
+			} else {
+				dev_warn(port->dev, "Should start RX, but TX fifo is not empty\n");
+			}
+			return;
+		}
+
 		atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
 	}
 }
-- 
2.19.1

Re: [PATCH 2/2] tty/serial: atmel: RS485 HD w/DMA: enable RX after TX is stopped

[Richard Genoud]

[ adding Gil Weber in Cc: ]
Le 15/03/2019 à 10:23, Razvan Stefanescu a écrit :
> In half-duplex operation, RX should be started after TX completes.
> 
> If DMA is used, there is a case when the DMA transfer completes but the
> TX FIFO is not emptied, so the RX cannot be restarted just yet.
> 
> Use a boolean variable to store this state and rearm TX interrupt mask
> to be signaled again that the transfer finished. In interrupt transmit
> handler this variable is used to start RX.

I was sure I've already seen something like that.
Gil, could you give it a test ?

you can add :
Cc: stable@vger.kernel.org
Fixes: b389f173aaa1 ("tty/serial: atmel: RS485 half duplex w/DMA: enable RX after TX is done")

Since patch 1/2 is needed for this one, I think you should add also
Cc:stable / Fixes: to the previous patch.

> Signed-off-by: Razvan Stefanescu <razvan.stefanescu@microchip.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index a6577b1c4461..b0141dcbbb61 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -166,6 +166,8 @@ struct atmel_uart_port {
>  	unsigned int		pending_status;
>  	spinlock_t		lock_suspended;
>  
> +	bool			hd_start_rx;	/* can start RX during half-duplex operation */
> +
>  	/* ISO7816 */
>  	unsigned int		fidi_min;
>  	unsigned int		fidi_max;
> @@ -934,8 +936,13 @@ static void atmel_complete_tx_dma(void *arg)
>  	if (!uart_circ_empty(xmit))
>  		atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
>  	else if (atmel_uart_is_half_duplex(port)) {
> -		/* DMA done, stop TX, start RX for RS485 */
> -		atmel_start_rx(port);
> +		/*
> +		 * DMA done, re-enable TXEMPTY and signal that we can stop
> +		 * TX and start RX for RS485
> +		 */
> +		atmel_port->hd_start_rx = true;
> +		atmel_uart_writel(port, ATMEL_US_IER,
> +				  atmel_port->tx_done_mask);
>  	}
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
> @@ -1379,9 +1386,21 @@ atmel_handle_transmit(struct uart_port *port, unsigned int pending)
>  	struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
>  
>  	if (pending & atmel_port->tx_done_mask) {
> -		/* Either PDC or interrupt transmission */
>  		atmel_uart_writel(port, ATMEL_US_IDR,
>  				  atmel_port->tx_done_mask);
> +
> +		/* Start RX if flag was set and FIFO is empty */
> +		if (atmel_port->hd_start_rx) {
> +			if (atmel_uart_readl(port, ATMEL_US_CSR)
> +					& ATMEL_US_TXEMPTY) {
> +				atmel_port->hd_start_rx = false;
> +				atmel_start_rx(port);
> +			} else {
> +				dev_warn(port->dev, "Should start RX, but TX fifo is not empty\n");
What will happen in this case ?

> +			}
> +			return;
> +		}
> +
>  		atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
>  	}
>  }
> 

Thanks !

Re: [PATCH 2/2] tty/serial: atmel: RS485 HD w/DMA: enable RX after TX is stopped

[Razvan.Stefanescu]

On 18/03/2019 16:30, Richard Genoud wrote:
>> +		/* Start RX if flag was set and FIFO is empty */
>> +		if (atmel_port->hd_start_rx) {
>> +			if (atmel_uart_readl(port, ATMEL_US_CSR)
>> +					& ATMEL_US_TXEMPTY) {
>> +				atmel_port->hd_start_rx = false;
>> +				atmel_start_rx(port);
>> +			} else {
>> +				dev_warn(port->dev, "Should start RX, but TX fifo is not empty\n");
> What will happen in this case ?
> 

RX will not be started. I haven't been able to trigger this error case. 
Would it be better to start RX anyway and just display the error message 
if TX fifo is not empty? But this way it will be like before this fix, 
in case of an error.

Thank you,
Razvan