Re: [PATCH] tty: serial: atmel: Preserve previous USART mode if RS485 disabled

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3138 bytes --]

On Wed, 24 Aug 2022, Sergiu Moga wrote:

> Whenever the atmel_rs485_config driver method would be called,
> the USART mode is reset to normal mode before even checking if
> RS485 flag is set, thus resulting in losing the previous USART
> mode in the case where the checking fails. Some tools, such as
> `linux-serial-test`, lead to the driver calling this method
> when doing the setup of the serial port: after setting the port
> mode (Hardware Flow Control, Normal Mode, RS485 Mode, etc.),
> `linux-serial-test` tries to enable/disable RS485 depending on
> the commandline arguments passed. If we were to, for example, enable
> Hardware Flow Control through `linux-serial-test`, the tool would
> make the driver set the corresponding bit to 1 (ATMEL_US_USMODE_HWHS
> bit in the ATMEL_US_MR register) through the atmel_set_termios method
> and then proceed to disabling RS485. This, in turn, causes the
> ATMEL_US_USMODE_HWHS bit of the ATMEL_US_MR mode register to be unset
> and, if the checking for RS485 fails, leads to having the mode set
> back to the ATMEL_US_USMODE_NORMAL normal mode. Since in hardware
> flow control mode the meanings of the ATMEL_US_RTSDIS and
> ATMEL_US_RTSEN bits are swapped, this leads to our endpoint leaving
> the RTS line to high when wanting to receive, which is the opposite
> of what the other endpoint is expecting in order to start transmitting.
> This fix ensures that this reset is done only if the checking for RS485
> succeeds.

Could you please try to split this long paragraph to a slightly shorter 
bits such that it would be easier to read.

> Fixes: e8faff7330a35 ("ARM: 6092/1: atmel_serial: support for RS485 communications")
> Signed-off-by: Sergiu Moga <sergiu.moga@microchip.com>
> ---
>  drivers/tty/serial/atmel_serial.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 0a0b46ee0955..c29b1fb48694 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -298,9 +298,6 @@ static int atmel_config_rs485(struct uart_port *port, struct ktermios *termios,
>  
>  	mode = atmel_uart_readl(port, ATMEL_US_MR);
>  
> -	/* Resetting serial mode to RS232 (0x0) */
> -	mode &= ~ATMEL_US_USMODE;
> -
>  	if (rs485conf->flags & SER_RS485_ENABLED) {
>  		dev_dbg(port->dev, "Setting UART to RS485\n");
>  		if (rs485conf->flags & SER_RS485_RX_DURING_TX)
> @@ -310,6 +307,7 @@ static int atmel_config_rs485(struct uart_port *port, struct ktermios *termios,
>  
>  		atmel_uart_writel(port, ATMEL_US_TTGR,
>  				  rs485conf->delay_rts_after_send);
> +		mode &= ~ATMEL_US_USMODE;
>  		mode |= ATMEL_US_USMODE_RS485;
>  	} else {
>  		dev_dbg(port->dev, "Setting UART to RS232\n");
> 

Makes sense.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Unrelated to this patch but I came across it while reviewing yours... Do 
you BTW have any idea why atmel_serial_probe() sets ATMEL_US_USMODE_NORMAL 
inside rs485_enabled block? I'd have expected it wanted to do 
ATMEL_US_USMODE_RS485 there too like is done in atmel_config_rs485().


-- 
 i.