[PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx() instead of .shutdown()

[PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx() instead of .shutdown()

[Sherry Sun]

From: Fugang Duan <fugang.duan@nxp.com>

Disable the UCR4_OREN bit in .stop_rx() before the uart receiver is disabled
maybe better than in the .shutdown() function.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
changes in V2:
 - remove the UCR4_OREN clearing in imx_uart_shutdown as this is not needed any
 more.
 - change the commit message.
---
 drivers/tty/serial/imx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index fb75e3e0d828..df8a0c8b8b29 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);
 
 	ucr2 &= ~UCR2_RXEN;
 	imx_uart_writel(sport, ucr2, UCR2);
@@ -1544,7 +1547,7 @@ static void imx_uart_shutdown(struct uart_port *port)
 	imx_uart_writel(sport, ucr1, UCR1);
 
 	ucr4 = imx_uart_readl(sport, UCR4);
-	ucr4 &= ~(UCR4_OREN | UCR4_TCEN);
+	ucr4 &= ~UCR4_TCEN;
 	imx_uart_writel(sport, ucr4, UCR4);
 
 	spin_unlock_irqrestore(&sport->port.lock, flags);
-- 
2.17.1

Re: [PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx() instead of .shutdown()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On Tue, Nov 23, 2021 at 06:51:22PM +0800, Sherry Sun wrote:
> From: Fugang Duan <fugang.duan@nxp.com>
> 
> Disable the UCR4_OREN bit in .stop_rx() before the uart receiver is disabled
> maybe better than in the .shutdown() function.

There should be a better incentive than "maybe it's better to do this".
My expectation is that you verify there is indeed an issue and then fix
that with a descriptive commit log that convinces this is indeed a fix.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx() instead of .shutdown()

[Greg KH]

On Tue, Nov 23, 2021 at 06:51:22PM +0800, Sherry Sun wrote:
> From: Fugang Duan <fugang.duan@nxp.com>
> 
> Disable the UCR4_OREN bit in .stop_rx() before the uart receiver is disabled
> maybe better than in the .shutdown() function.

Why is it better?  What does this "fix"?

thanks,

greg k-h

RE: [PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx() instead of .shutdown()

[Sherry Sun]

Hi Greg,

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 2021年11月24日 17:59
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: jirislaby@kernel.org; u.kleine-koenig@pengutronix.de; linux-
> serial@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx <linux-
> imx@nxp.com>
> Subject: Re: [PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx()
> instead of .shutdown()
> 
> On Tue, Nov 23, 2021 at 06:51:22PM +0800, Sherry Sun wrote:
> > From: Fugang Duan <fugang.duan@nxp.com>
> >
> > Disable the UCR4_OREN bit in .stop_rx() before the uart receiver is
> > disabled maybe better than in the .shutdown() function.
> 
> Why is it better?  What does this "fix"?

Since I am not clear about the history of this patch, so I haven't found any obvious issues without this patch so far.

But after discussing with the IP owner, it is still recommended to disable the receiver-related interrupts like OREN before disabling the receiver.

If not, when we have the overrun error during the receiver disable process, the overrun interrupt will keep trigging until we disable the OREN interrupt in the .shutdown(), because the ORE status can only be cleared when read the rx FIFO or reset the controller.  Although the called time between the receiver disable and OREN disable in .shutdown() is very short, there is still the risk of endless interrupt during this short period of time. So had better to disable OREN before the receiver been disabled in .stop_rx().

Best regards
Sherry

Re: [PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx() instead of .shutdown()

[Greg KH]

On Wed, Nov 24, 2021 at 10:56:43AM +0000, Sherry Sun wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: 2021年11月24日 17:59
> > To: Sherry Sun <sherry.sun@nxp.com>
> > Cc: jirislaby@kernel.org; u.kleine-koenig@pengutronix.de; linux-
> > serial@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx <linux-
> > imx@nxp.com>
> > Subject: Re: [PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx()
> > instead of .shutdown()
> > 
> > On Tue, Nov 23, 2021 at 06:51:22PM +0800, Sherry Sun wrote:
> > > From: Fugang Duan <fugang.duan@nxp.com>
> > >
> > > Disable the UCR4_OREN bit in .stop_rx() before the uart receiver is
> > > disabled maybe better than in the .shutdown() function.
> > 
> > Why is it better?  What does this "fix"?
> 
> Since I am not clear about the history of this patch, so I haven't found any obvious issues without this patch so far.

Then why submit it?

> But after discussing with the IP owner, it is still recommended to disable the receiver-related interrupts like OREN before disabling the receiver.

recommended by what?  The hardware designers?

> If not, when we have the overrun error during the receiver disable process, the overrun interrupt will keep trigging until we disable the OREN interrupt in the .shutdown(), because the ORE status can only be cleared when read the rx FIFO or reset the controller.  Although the called time between the receiver disable and OREN disable in .shutdown() is very short, there is still the risk of endless interrupt during this short period of time. So had better to disable OREN before the receiver been disabled in .stop_rx().

Please document this in the changelog, otherwise we have no idea why
this is needed.

thanks,

greg k-h

RE: [PATCH V2] tty: serial: imx: disable UCR4_OREN in .stop_rx() instead of .shutdown()

[Sherry Sun]

Hi Greg,

> On Wed, Nov 24, 2021 at 10:56:43AM +0000, Sherry Sun wrote:
> > Hi Greg,
> >
> > > -----Original Message-----
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: 2021年11月24日 17:59
> > > To: Sherry Sun <sherry.sun@nxp.com>
> > > Cc: jirislaby@kernel.org; u.kleine-koenig@pengutronix.de; linux-
> > > serial@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> > > <linux- imx@nxp.com>
> > > Subject: Re: [PATCH V2] tty: serial: imx: disable UCR4_OREN in
> > > .stop_rx() instead of .shutdown()
> > >
> > > On Tue, Nov 23, 2021 at 06:51:22PM +0800, Sherry Sun wrote:
> > > > From: Fugang Duan <fugang.duan@nxp.com>
> > > >
> > > > Disable the UCR4_OREN bit in .stop_rx() before the uart receiver
> > > > is disabled maybe better than in the .shutdown() function.
> > >
> > > Why is it better?  What does this "fix"?
> >
> > Since I am not clear about the history of this patch, so I haven't found any
> obvious issues without this patch so far.
> 
> Then why submit it?

Because from the code logic, I think this patch is more reasonable.

> 
> > But after discussing with the IP owner, it is still recommended to disable the
> receiver-related interrupts like OREN before disabling the receiver.
> 
> recommended by what?  The hardware designers?

Yes, the i.MX UART IP designer. 

> 
> > If not, when we have the overrun error during the receiver disable process,
> the overrun interrupt will keep trigging until we disable the OREN interrupt in
> the .shutdown(), because the ORE status can only be cleared when read the
> rx FIFO or reset the controller.  Although the called time between the receiver
> disable and OREN disable in .shutdown() is very short, there is still the risk of
> endless interrupt during this short period of time. So had better to disable
> OREN before the receiver been disabled in .stop_rx().
> 
> Please document this in the changelog, otherwise we have no idea why this is
> needed.

Sure, I will send V3 and add these info into the commit message. Thanks!

Best regards
Sherry