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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
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.
