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.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
I'm not sure this is the best fix.

At first I considered doing something much more targeted, but
definitely also more hacky: In imx_uart_rs485_config(), if switching
on rs485 mode, simply add "sport->tx_state = OFF;".

If someone has a better suggestion, I'm all ears.

 drivers/tty/serial/imx.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)
  

> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Sent: 2023年11月20日 21:23
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
> <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; dl-linux-
> imx <linux-imx@nxp.com>
> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>; linux-
> kernel@vger.kernel.org; linux-serial@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org
> 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.
I tried on imx8mp-evk board, for rs232, sport->tx_state can get set to OFF even we don’t set UCR4_TCEN.

Best Regards
Sherry

> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> I'm not sure this is the best fix.
> 
> At first I considered doing something much more targeted, but definitely also
> more hacky: In imx_uart_rs485_config(), if switching on rs485 mode, simply
> add "sport->tx_state = OFF;".
> 
> If someone has a better suggestion, I'm all ears.
> 
>  drivers/tty/serial/imx.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> 7341d060f85c..ffee157e13cd 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -708,25 +708,19 @@ static void imx_uart_start_tx(struct uart_port
> *port)
>  		    || sport->tx_state == WAIT_AFTER_RTS) {
> 
>  			hrtimer_try_to_cancel(&sport->trigger_stop_tx);
> -
> -			/*
> -			 * Enable transmitter and shifter empty irq only if
> DMA
> -			 * is off.  In the DMA case this is done in the
> -			 * tx-callback.
> -			 */
> -			if (!sport->dma_is_enabled) {
> -				u32 ucr4 = imx_uart_readl(sport, UCR4);
> -				ucr4 |= UCR4_TCEN;
> -				imx_uart_writel(sport, ucr4, UCR4);
> -			}
> -
> -			sport->tx_state = SEND;
>  		}
> -	} else {
> -		sport->tx_state = SEND;
>  	}
> 
> +	sport->tx_state = SEND;
> +
> +	/*
> +	 * Enable transmitter and shifter empty irq only if DMA is
> +	 * off.  In the DMA case this is done in the tx-callback.
> +	 */
>  	if (!sport->dma_is_enabled) {
> +		u32 ucr4 = imx_uart_readl(sport, UCR4);
> +		ucr4 |= UCR4_TCEN;
> +		imx_uart_writel(sport, ucr4, UCR4);
>  		ucr1 = imx_uart_readl(sport, UCR1);
>  		imx_uart_writel(sport, ucr1 | UCR1_TRDYEN, UCR1);
>  	}
> --
> 2.40.1.1.g1c60b9335d
  

On 21/11/2023 07.37, Sherry Sun wrote:
> 
> 
>> -----Original Message-----
>> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> Sent: 2023年11月20日 21:23
>> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
>> <jirislaby@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha Hauer
>> <s.hauer@pengutronix.de>; Pengutronix Kernel Team
>> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; dl-linux-
>> imx <linux-imx@nxp.com>
>> Cc: Rasmus Villemoes <rasmus.villemoes@prevas.dk>; linux-
>> kernel@vger.kernel.org; linux-serial@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org
>> 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
  

> 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> I'm not sure this is the best fix.
>
> At first I considered doing something much more targeted, but
> definitely also more hacky: In imx_uart_rs485_config(), if switching
> on rs485 mode, simply add "sport->tx_state = OFF;".
>
> If someone has a better suggestion, I'm all ears.


Hello Rasmus,

i can observe a very similar situation, but with a litte different
configuration. This is how i can trigger the situation very quickly:

   1) open the port
   2) send 1 byte out
   3) close the port

Do it in a loop. As faster, the lockup may occur earlier (but not
mandatory, 100ms is sufficient in my setup at 115200 Baud on an
i.mx8mm board).
With this configuration i get the lockup in around 1 minute.

For my setup it's clear what happens:

   - when the tty is closed imx_uart_shutdown() is called. This calls
     imx_uart_stop_tx()
   - for a lockup, the shifter is still busy and imx_uart_stop_tx()
     returns early (as you explained) without modifying ->tx_state.
   - imx_uart_shutdown() proceeds and finally closes the port. Due to
     imx_uart_stop_tx() is not executed completely tx_state is left in
     state ->tx_state == SEND.
   - When the port is opened again, tx_state is SEND and nothing can
     be transmitted any more. The tx path has locked up!

Setting ->tx_state = SEND in imx_uart_shutdown() helps for my issue
(and should be ok IMHO).

But IMHO there is one next issue with this situation: When the port
operates with WAIT_AFTER_RTS and WAIT_AFTER_SEND then some timers
for callback functions might be active. I did not discover where they
are stopped for the case when the serial port is closed. Maybe stopping
is not required ...

I'd appreciate someone with more experience could review or revise it

Best Regards
Eberhard
  

> 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.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
> I'm not sure this is the best fix.
>
> At first I considered doing something much more targeted, but
> definitely also more hacky: In imx_uart_rs485_config(), if switching
> on rs485 mode, simply add "sport->tx_state = OFF;".
>
> If someone has a better suggestion, I'm all ears.

Hello Rasmus,

i can observe a very similar situation, but with a litte different
configuration. This is how i can trigger the situation very quickly:

   1) open the port
   2) send 1 byte out
   3) close the port

Do it in a loop. As faster, the lockup may occur earlier (but not
mandatory, 100ms is sufficient in my setup at 115200 Baud on an
i.mx8mm board).
With this configuration i get the lockup in around 1 minute.

For my setup it's clear what happens:

   - when the tty is closed imx_uart_shutdown() is called. This calls
     imx_uart_stop_tx()
   - for a lockup, the shifter is still busy and imx_uart_stop_tx()
     returns early (as you explained) without modifying ->tx_state.
   - imx_uart_shutdown() proceeds and finally closes the port. Due to
     imx_uart_stop_tx() is not executed completely tx_state is left in
     state ->tx_state == SEND.
   - When the port is opened again, tx_state is SEND and nothing can
     be transmitted any more. The tx path has locked up!

Setting ->tx_state = SEND in imx_uart_shutdown() helps for my issue
(and should be ok IMHO).

But IMHO there is one next issue with this situation: When the port
operates with WAIT_AFTER_RTS and WAIT_AFTER_SEND then some timers
for callback functions might be active. I did not discover where they
are stopped for the case when the serial port is closed. Maybe stopping
is not required ...

I'd appreciate someone with more experience could review or revise it

Best Regards
Eberhard
  

On 21/11/2023 21.49, Eberhard Stoll wrote:
>> 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.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>> I'm not sure this is the best fix.
>>
>> At first I considered doing something much more targeted, but
>> definitely also more hacky: In imx_uart_rs485_config(), if switching
>> on rs485 mode, simply add "sport->tx_state = OFF;".
>>
>> If someone has a better suggestion, I'm all ears.
> 
> 
> Hello Rasmus,
> 
> i can observe a very similar situation, but with a litte different
> configuration. This is how i can trigger the situation very quickly:
> 
>   1) open the port
>   2) send 1 byte out
>   3) close the port

Hi Eberhard

Thanks for chiming in. I assume this is all in rs485 mode, no switching
to rs232 and back involved?


> Do it in a loop. As faster, the lockup may occur earlier (but not
> mandatory, 100ms is sufficient in my setup at 115200 Baud on an
> i.mx8mm board).
> With this configuration i get the lockup in around 1 minute.
> 
> For my setup it's clear what happens:
> 
>   - when the tty is closed imx_uart_shutdown() is called. This calls
>     imx_uart_stop_tx()
>   - for a lockup, the shifter is still busy and imx_uart_stop_tx()
>     returns early (as you explained) without modifying ->tx_state.
>   - imx_uart_shutdown() proceeds and finally closes the port. Due to
>     imx_uart_stop_tx() is not executed completely tx_state is left in
>     state ->tx_state == SEND.

Yes, and imx_uart_shutdown() disables the TCEN which would otherwise
cause _stop_tx to get called when the transmitter is no longer busy.

>   - When the port is opened again, tx_state is SEND and nothing can
>     be transmitted any more. The tx path has locked up!
> 
> Setting ->tx_state = SEND in imx_uart_shutdown() helps for my issue
> (and should be ok IMHO).

[I assume you mean tx_state = OFF]. Yes, I suppose doing that would be
ok, but I'm not sure it's a complete fix. In my simple test cast, I have
separate programs invoked to do the I/O and do the mode switch, but in a
real scenario, I'd expect the application itself to just open the device
once, and then do I/O and mode switching as appropriate for the
application logic, and I don't think uart_shutdown would then ever get
called.

> But IMHO there is one next issue with this situation: When the port
> operates with WAIT_AFTER_RTS and WAIT_AFTER_SEND then some timers
> for callback functions might be active. I did not discover where they
> are stopped for the case when the serial port is closed. Maybe stopping
> is not required ...

Indeed, that's an extra complication. Adding two hrtimer_try_to_cancel()
in shutdown would probably not hurt, along with setting tx_state OFF.

I wonder if at least mode switching should simply be disallowed (-EBUSY)
if tx_state is anything but OFF.

Rasmus
  

>> i can observe a very similar situation, but with a litte different
>> configuration. This is how i can trigger the situation very quickly:
>>
>>    1) open the port
>>    2) send 1 byte out
>>    3) close the port
>
> Hi Eberhard
>
> Thanks for chiming in. I assume this is all in rs485 mode, no switching
> to rs232 and back involved?

Yes, no mode switching is involved here. Only rs485 mode.

>> Setting ->tx_state = SEND in imx_uart_shutdown() helps for my issue
>> (and should be ok IMHO).
>
> [I assume you mean tx_state = OFF]. Yes, I suppose doing that would be > ok, but I'm not sure it's a complete fix. In my simple test cast, I have

Oh, yes, sorry my mistake!
I really meant ->tx_state = OFF as you expected in your comment

> separate programs invoked to do the I/O and do the mode switch, but in a
> real scenario, I'd expect the application itself to just open the device
> once, and then do I/O and mode switching as appropriate for the
> application logic, and I don't think uart_shutdown would then ever get
> called.

Switching between rs232 mode and rs485 mode on an open port is a special
use case in my experience. But it's valid and introduces some extra
complexity. As you stated, for this use case setting ->tx_state = OFF in
imx_uart_shutdown() does not really help.

> Indeed, that's an extra complication. Adding two hrtimer_try_to_cancel()
> in shutdown would probably not hurt, along with setting tx_state OFF.
>
> I wonder if at least mode switching should simply be disallowed (-EBUSY)
> if tx_state is anything but OFF.

Seems there are various issues in ->tx_state handling and transmitter
timing in this driver!

Best Regards
Eberhard
  

On 22.11.23 09:03, Rasmus Villemoes wrote:
> On 21/11/2023 21.49, Eberhard Stoll wrote:
>>> 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.
>>>
>>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>>> ---
>>> I'm not sure this is the best fix.
>>>
>>> At first I considered doing something much more targeted, but
>>> definitely also more hacky: In imx_uart_rs485_config(), if switching
>>> on rs485 mode, simply add "sport->tx_state = OFF;".
>>>
>>> If someone has a better suggestion, I'm all ears.
>>
>>
>> Hello Rasmus,
>>
>> i can observe a very similar situation, but with a litte different
>> configuration. This is how i can trigger the situation very quickly:
>>
>>   1) open the port
>>   2) send 1 byte out
>>   3) close the port
> 
> Hi Eberhard
> 
> Thanks for chiming in. I assume this is all in rs485 mode, no switching
> to rs232 and back involved?
> 
> 
>> Do it in a loop. As faster, the lockup may occur earlier (but not
>> mandatory, 100ms is sufficient in my setup at 115200 Baud on an
>> i.mx8mm board).
>> With this configuration i get the lockup in around 1 minute.
>>
>> For my setup it's clear what happens:
>>
>>   - when the tty is closed imx_uart_shutdown() is called. This calls
>>     imx_uart_stop_tx()
>>   - for a lockup, the shifter is still busy and imx_uart_stop_tx()
>>     returns early (as you explained) without modifying ->tx_state.
>>   - imx_uart_shutdown() proceeds and finally closes the port. Due to
>>     imx_uart_stop_tx() is not executed completely tx_state is left in
>>     state ->tx_state == SEND.
> 
> Yes, and imx_uart_shutdown() disables the TCEN which would otherwise
> cause _stop_tx to get called when the transmitter is no longer busy.
> 
>>   - When the port is opened again, tx_state is SEND and nothing can
>>     be transmitted any more. The tx path has locked up!
>>
>> Setting ->tx_state = SEND in imx_uart_shutdown() helps for my issue
>> (and should be ok IMHO).
> 
> [I assume you mean tx_state = OFF]. Yes, I suppose doing that would be
> ok, but I'm not sure it's a complete fix. In my simple test cast, I have
> separate programs invoked to do the I/O and do the mode switch, but in a
> real scenario, I'd expect the application itself to just open the device
> once, and then do I/O and mode switching as appropriate for the
> application logic, and I don't think uart_shutdown would then ever get
> called.
> 
>> But IMHO there is one next issue with this situation: When the port
>> operates with WAIT_AFTER_RTS and WAIT_AFTER_SEND then some timers
>> for callback functions might be active. I did not discover where they
>> are stopped for the case when the serial port is closed. Maybe stopping
>> is not required ...
> 
> Indeed, that's an extra complication. Adding two hrtimer_try_to_cancel()
> in shutdown would probably not hurt, along with setting tx_state OFF.
> 
> I wonder if at least mode switching should simply be disallowed (-EBUSY)
> if tx_state is anything but OFF.

Is there a valid use-case for switching the mode while the device is
transmitting? Is this something we need to support for whatever reason?
It sounds rather an obscure thing to do.

If not I would strongly vote for disallowing it in favor of a more
stable and less complex driver.
  

On 22/11/2023 14.01, Frieder Schrempf wrote:
> On 22.11.23 09:03, Rasmus Villemoes wrote:
>> On 21/11/2023 21.49, Eberhard Stoll wrote:

>>> But IMHO there is one next issue with this situation: When the port
>>> operates with WAIT_AFTER_RTS and WAIT_AFTER_SEND then some timers
>>> for callback functions might be active. I did not discover where they
>>> are stopped for the case when the serial port is closed. Maybe stopping
>>> is not required ...
>>
>> Indeed, that's an extra complication. Adding two hrtimer_try_to_cancel()
>> in shutdown would probably not hurt, along with setting tx_state OFF.
>>
>> I wonder if at least mode switching should simply be disallowed (-EBUSY)
>> if tx_state is anything but OFF.
> 
> Is there a valid use-case for switching the mode while the device is
> transmitting? Is this something we need to support for whatever reason?
> It sounds rather an obscure thing to do.

No, I don't think there is, and that's not at all what I'm trying to do.
I was just thinking out loud that maybe we should explicitly disallow it
[further, in fact, if possible, it should be disallowed in the serial
core]. But of course that requires that ->tx_state can actually be
relied upon, which is what my patch attempts to do for the rs232 case -
though it might not be an entirely complete fix, as described by
Eberhard, since we can get to ->shutdown and thus disable the TC
interrupt before it has a chance to fire.

Rasmus