RE: [PATCH] tty: serial: imx: disable UCR4_OREN in .stop_rx()

Hi Uwe,

-----Original Message-----
From: Uwe Kleine-König <redacted>
Sent: 2021年11月23日 16:43
To: Sherry Sun <redacted>
Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org; linux-
serial@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx <linux-
imx@nxp.com>
Subject: Re: [PATCH] tty: serial: imx: disable UCR4_OREN in .stop_rx()

Hello again,

On Tue, Nov 23, 2021 at 08:51:22AM +0100, Uwe Kleine-König wrote:

quoted

On Tue, Nov 23, 2021 at 03:04:37PM +0800, Sherry Sun wrote:

quoted

From: Fugang Duan <redacted>

Disable UCR4_OREN bit in .stop_rx() to avoid endless interrupt
happen while tty port is closing.

Signed-off-by: Fugang Duan <redacted>
Signed-off-by: Sherry Sun <redacted>
---
 drivers/tty/serial/imx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index fb75e3e0d828..9ecaf051249d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c

@@ -486,18 +486,21 @@ static void imx_uart_stop_tx(struct uart_port

*port)  static void imx_uart_stop_rx(struct uart_port *port)  {
 	struct imx_port *sport = (struct imx_port *)port;
-	u32 ucr1, ucr2;
+	u32 ucr1, ucr2, ucr4;

 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr2 = imx_uart_readl(sport, UCR2);
+	ucr4 = imx_uart_readl(sport, UCR4);

 	if (sport->dma_is_enabled) {
 		ucr1 &= ~(UCR1_RXDMAEN | UCR1_ATDMAEN);
 	} else {
 		ucr1 &= ~UCR1_RRDYEN;
 		ucr2 &= ~UCR2_ATEN;
+		ucr4 &= ~UCR4_OREN;
 	}
 	imx_uart_writel(sport, ucr1, UCR1);
+	imx_uart_writel(sport, ucr4, UCR4);

I don't understand this yet, but I guess this is just a matter of
improving the commit log. How does the endless irq happen?

To be frankly, this patch is an old local patch, I also don't know the history about it.
But I checked the code logic is fine, the UCR4_OREN should be disabled when stop rx.

quoted

... some time later after thinking and reading the driver source ...

Hmm, when the OR event is triggered the handler does

	imx_uart_writel(sport, USR2_ORE, USR2);

and so clears the event. I would expect that this should be good
enough to prevent an irq storm even if the receiver is disabled.
Doesn't acking work with the receiver disabled?

One thing I forgot to mention:

If imx_uart_stop_rx() now clears UCR4_OREN, clearing in
imx_uart_shutdown() should probably be dropped.

You are right, with this patch, clear the UCR4_OREN in imx_uart_shutdown is not necessary any more, I will remove it and send V2. Thank!

Best regards
Sherry

Best regards
Uwe

--
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |