Re: [PATCH] serial: imx: also enable Transmit Complete interrupt in rs232 mode

[Rasmus Villemoes]

On 21/11/2023 07.37, Sherry Sun wrote:
> 
> 
>> -----Original Message-----
>> From: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>
>> Sent: 2023年11月20日 21:23
>> To: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby
>> <jirislaby@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Sascha Hauer
>> <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
>> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; dl-linux-
>> imx <linux-imx@xxxxxxx>
>> Cc: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>; linux-
>> kernel@xxxxxxxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; linux-arm-
>> kernel@xxxxxxxxxxxxxxxxxxx
>> Subject: [PATCH] serial: imx: also enable Transmit Complete interrupt in
>> rs232 mode
>>
>> Currently, if one switches to rs232 mode, writes something to the device, and
>> then switches to rs485 mode, the imx_port's ->tx_state is left as SEND. This
>> then prevents a subsequent write in rs485 mode from properly asserting the
>> rts pin (i.e. enabling the transceiver), because imx_uart_start_rx() does not
>> enter the "if (sport->tx_state == OFF)" branch. Hence nothing is actually
>> transmitted.
>>
>> The problem is that in rs232 mode, ->tx_state never gets set to OFF, due to
>>
>> 	usr2 = imx_uart_readl(sport, USR2);
>> 	if (!(usr2 & USR2_TXDC)) {
>> 		/* The shifter is still busy, so retry once TC triggers */
>> 		return;
>> 	}
>>
>> in imx_uart_stop_tx(), and TC never triggers because the Transmit Complete
>> interrupt is not enabled for rs232.
> 
> Hi Rasmus,
> I am afraid this is not right, USR2_TXDC is just a flag, it is not affected by whether UCR4_TCEN is set or not, UCR4_TCEN only determines if USR2_TXDC flag can generate a interrupt or not.

Exactly. And when TCEN is not set, we don't get that interrupt the
comment refers to, so we never retry once TXDC gets set.

> I tried on imx8mp-evk board, for rs232, sport->tx_state can get set to OFF even we don’t set UCR4_TCEN.

I am working on an imx8mp board, so I can definitely tell you that the
code currently has a problem, and this patch fixes it (though, as said,
I do not know if it's the best fix or if it might introduce other problems).

Yes, of course, due to random timing issues, you _might_ see that in
rs232 mode, by the time imx_uart_stop_tx() gets called (most likely from
imx_uart_transmit_buffer()), the transmitter might be done, so we pass
that if (!(usr2 & USR2_TXDC)) test, and get right to the bottom of
imx_uart_stop_tx() where ->tx_state is set to OFF.

But in many cases (it reproduces completely reliably for me), that
imx_uart_stop_tx() hits the early return, and if the Transmit Complete
enable interrupt is not enabled, well, then (referring to the existing
comment) of course TC never triggers, so we never retry, and hence
nothing ever sets ->tx_state back to OFF. And that's not really a
problem as long as you stay in rs232, because that mode doesn't really
use the ->tx_state state machine logic. But switching to rs485 mode, and
the driver is left in a confused state breaking output (input still works).

Rasmus