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.
Fixes: e8faff7330a35 ("ARM: 6092/1: atmel_serial: support for RS485 communications")
Signed-off-by: Sergiu Moga <[email protected]>
---
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");
--
2.25.1
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 <[email protected]>
> ---
> 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 <[email protected]>
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.
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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