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

[<Sergiu.Moga@microchip.com>]

On 24.08.2022 15:02, Ilpo Järvinen wrote:
> 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.


Sure, will do :).


>> 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().
>

A quick git blame in an older version of the driver shows this commit:
5dfbd1d734ef5415bc47b034df7433ba21e40e7b

with the following commit message:

```
atmel_serial: fix RTS high after initialization in RS485 mode

When working in RS485 mode, the atmel_serial driver keeps RTS high after 
the initialization of the serial port. It goes low only after the first 
character has been sent.
```

If I am to remove the line in question (delete it entirely) and do a 
simple test, the serial interface seems to continue to work fine in 
RS485 mode. This is for some of the newer IPs, I am not so sure about 
the older ones though, so it is probably a good idea to not risk it.

> --
>   i.


Regards,
     Sergiu