[PATCH] serial: imx: Update mctrl old_status on RTSD interrupt

[Marek Vasut]

On 10/2/24 9:49 AM, Uwe Kleine-König wrote:
> On Wed, Oct 02, 2024 at 06:11:16AM +0200, Marek Vasut wrote:
>> When sending data using DMA at high baudrate (4 Mbdps in local test case) to
>> a device with small RX buffer which keeps asserting RTS after every received
>> byte, it is possible that the iMX UART driver would not recognize the falling
>> edge of RTS input signal and get stuck, unable to transmit any more data.
>>
>> This condition happens when the following sequence of events occur:
>> - imx_uart_mctrl_check() is called at some point and takes a snapshot of UART
>>    control signal status into sport->old_status using imx_uart_get_hwmctrl().
>>    The RTSS/TIOCM_CTS bit is of interest here (*).
>> - DMA transfer occurs, the remote device asserts RTS signal after each byte.
>>    The i.MX UART driver recognizes each such RTS signal change, raises an
>>    interrupt with USR1 register RTSD bit set, which leads to invocation of
>>    __imx_uart_rtsint(), which calls uart_handle_cts_change().
>>    - If the RTS signal is deasserted, uart_handle_cts_change() clears
>>      port->hw_stopped and unblocks the port for further data transfers.
>>    - If the RTS is asserted, uart_handle_cts_change() sets port->hw_stopped
>>      and blocks the port for further data transfers. This may occur as the
>>      last interrupt of a transfer, which means port->hw_stopped remains set
>>      and the port remains blocked (**).
>> - Any further data transfer attempts will trigger imx_uart_mctrl_check(),
>>    which will read current status of UART control signals by calling
>>    imx_uart_get_hwmctrl() (***) and compare it with sport->old_status .
>>    - If current status differs from sport->old_status for RTS signal,
>>      uart_handle_cts_change() is called and possibly unblocks the port
>>      by clearing port->hw_stopped .
>>    - If current status does not differ from sport->old_status for RTS
>>      signal, no action occurs. This may occur in case prior snapshot (*)
>>      was taken before any transfer so the RTS is deasserted, current
>>      snapshot (***) was taken after a transfer and therefore RTS is
>>      deasserted again, which means current status and sport->old_status
>>      are identical. In case (**) triggered when RTS got asserted, and
>>      made port->hw_stopped set, the port->hw_stopped will remain set
>>      because no change on RTS line is recognized by this driver and
>>      uart_handle_cts_change() is not called from here to unblock the
>>      port->hw_stopped.
>>
>> Update sport->old_status in __imx_uart_rtsint() accordingly to make
>> imx_uart_mctrl_check() detect such RTS change. Note that TIOCM_CAR
>> and TIOCM_RI bits in sport->old_status do not suffer from this problem.
> 
> Why is that? Just because these don't stop transmission?

If imx_uart_mctrl_check() does not detect the RTS asserted->deasserted 
transition, it will never call uart_handle_cts_change(), which will 
never clear port->hw_stopped and the port will remain stopped 
indefinitely, and never be able to transmit more data AFTER this event 
happens (port close/open will reset the flag too, but that is undesired 
workaround).

>> Fixes: ceca629e0b48 ("[ARM] 2971/1: i.MX uart handle rts irq")
>> Signed-off-by: Marek Vasut <marex at denx.de>
>> ---
>>   drivers/tty/serial/imx.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 67d4a72eda770..3ad7f42790ef9 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -762,6 +762,10 @@ static irqreturn_t __imx_uart_rtsint(int irq, void *dev_id)
>>   
>>   	imx_uart_writel(sport, USR1_RTSD, USR1);
>>   	usr1 = imx_uart_readl(sport, USR1) & USR1_RTSS;
>> +	if (usr1 & USR1_RTSS)
>> +		sport->old_status |= TIOCM_CTS;
>> +	else
>> +		sport->old_status &= ~TIOCM_CTS;
>>   	uart_handle_cts_change(&sport->port, usr1);
>>   	wake_up_interruptible(&sport->port.state->port.delta_msr_wait);
> 
> I didn't grab the whole picture, but I think this deserves a code
> comment.

Added in V2.

> Would it make sense to replace the current code in __imx_uart_rtsint by
> a call to imx_uart_mctrl_check()?
No, the __imx_uart_rtsint() only handles RTS state change interrupt and 
no other interrupts, so calling imx_uart_mctrl_check() would handle 
other unrelated signals for no reason and make the interrupt handler 
slower. I don't think this is an improvement.