[PATCH V2 0/5] fsl_lpuart: improve Idle Line Interrupt and registers handle in .shutdown()

[PATCH V2 0/5] fsl_lpuart: improve Idle Line Interrupt and registers handle in .shutdown()

[Sherry Sun]

The patchset improve the Idle Line Interrupt for lpuart driver, also handle
the registers correctly for lpuart32 when closing the uart port.

Patches have been tested on imx8ulp-evk platform.

Sherry Sun (5):
  tty: serial: fsl_lpuart: only enable Idle Line Interrupt for non-dma
    case
  tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()
  tty: serial: fsl_lpuart: clear UARTMODIR register in
    lpuart32_shutdown()
  tty: serial: fsl_lpuart: disable Rx/Tx DMA in lpuart32_shutdown()
  tty: serial: fsl_lpuart: clear LPUART Status Register in
    lpuart32_shutdown()

 drivers/tty/serial/fsl_lpuart.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH V2 1/5] tty: serial: fsl_lpuart: only enable Idle Line Interrupt for non-dma case

[Sherry Sun]

For the lpuart driver, the Idle Line Interrupt Enable now is only needed
for the CPU mode, so enable the UARTCTRL_ILIE at the correct place, and
clear it when shutdown.

Also need to configure the suitable UARTCTRL_IDLECFG, now the value is
0x7, represent 128 idle characters will trigger the Idle Line Interrupt.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
Changes in V2:
1. Use FIELD_PREP() and GENMASK() for easy access to UARTCTRL_IDLECFG
fields as suggested by Ilpo.
---
 drivers/tty/serial/fsl_lpuart.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index bd685491eead..a8f8e535077a 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -179,7 +179,7 @@
 #define UARTCTRL_SBK		0x00010000
 #define UARTCTRL_MA1IE		0x00008000
 #define UARTCTRL_MA2IE		0x00004000
-#define UARTCTRL_IDLECFG	0x00000100
+#define UARTCTRL_IDLECFG	GENMASK(10, 8)
 #define UARTCTRL_LOOPS		0x00000080
 #define UARTCTRL_DOZEEN		0x00000040
 #define UARTCTRL_RSRC		0x00000020
@@ -1506,7 +1506,7 @@ static void lpuart32_setup_watermark(struct lpuart_port *sport)
 	ctrl = lpuart32_read(&sport->port, UARTCTRL);
 	ctrl_saved = ctrl;
 	ctrl &= ~(UARTCTRL_TIE | UARTCTRL_TCIE | UARTCTRL_TE |
-			UARTCTRL_RIE | UARTCTRL_RE);
+			UARTCTRL_RIE | UARTCTRL_RE | UARTCTRL_ILIE);
 	lpuart32_write(&sport->port, ctrl, UARTCTRL);
 
 	/* enable FIFO mode */
@@ -1530,7 +1530,8 @@ static void lpuart32_setup_watermark_enable(struct lpuart_port *sport)
 	lpuart32_setup_watermark(sport);
 
 	temp = lpuart32_read(&sport->port, UARTCTRL);
-	temp |= UARTCTRL_RE | UARTCTRL_TE | UARTCTRL_ILIE;
+	temp |= UARTCTRL_RE | UARTCTRL_TE;
+	temp |= FIELD_PREP(UARTCTRL_IDLECFG, 0x7);
 	lpuart32_write(&sport->port, temp, UARTCTRL);
 }
 
@@ -1669,7 +1670,7 @@ static void lpuart32_configure(struct lpuart_port *sport)
 	}
 	temp = lpuart32_read(&sport->port, UARTCTRL);
 	if (!sport->lpuart_dma_rx_use)
-		temp |= UARTCTRL_RIE;
+		temp |= UARTCTRL_RIE | UARTCTRL_ILIE;
 	if (!sport->lpuart_dma_tx_use)
 		temp |= UARTCTRL_TIE;
 	lpuart32_write(&sport->port, temp, UARTCTRL);
@@ -1770,7 +1771,7 @@ static void lpuart32_shutdown(struct uart_port *port)
 
 	/* disable Rx/Tx and interrupts */
 	temp = lpuart32_read(port, UARTCTRL);
-	temp &= ~(UARTCTRL_TE | UARTCTRL_RE |
+	temp &= ~(UARTCTRL_TE | UARTCTRL_RE | UARTCTRL_ILIE |
 			UARTCTRL_TIE | UARTCTRL_TCIE | UARTCTRL_RIE);
 	lpuart32_write(port, temp, UARTCTRL);
 
-- 
2.17.1

[PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()

[Sherry Sun]

UARTCTRL_LOOPS bit is set in lpuart32_set_mctrl() for loopback mode, but
nowhere clear this bit, it should be cleared when closing the uart port
to avoid the loopback mode been enabled by default when reopening the
uart.

Fixes: 8a0c810d94f0 ("serial: fsl_lpuart: add loopback support")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
Changes in V2:
1. Split one patch into four smaller patches to improve the commit
messages and add Fixes tag as suggested by Ilpo.
---
 drivers/tty/serial/fsl_lpuart.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index a8f8e535077a..dbf8cccea105 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1772,7 +1772,8 @@ static void lpuart32_shutdown(struct uart_port *port)
 	/* disable Rx/Tx and interrupts */
 	temp = lpuart32_read(port, UARTCTRL);
 	temp &= ~(UARTCTRL_TE | UARTCTRL_RE | UARTCTRL_ILIE |
-			UARTCTRL_TIE | UARTCTRL_TCIE | UARTCTRL_RIE);
+			UARTCTRL_TIE | UARTCTRL_TCIE | UARTCTRL_RIE |
+			UARTCTRL_LOOPS);
 	lpuart32_write(port, temp, UARTCTRL);
 
 	spin_unlock_irqrestore(&port->lock, flags);
-- 
2.17.1

[PATCH V2 3/5] tty: serial: fsl_lpuart: clear UARTMODIR register in lpuart32_shutdown()

[Sherry Sun]

UARTMODIR register is configured in lpuart32_set_termios() according to
the termios flags, but nowhere clear it, should clear it when shutdown
the uart port.

Fixes: 380c966c093e ("tty: serial: fsl_lpuart: add 32-bit register interface support")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
Changes in V2:
1. Split one patch into four smaller patches to improve the commit
messages and add Fixes tag as suggested by Ilpo.
---
 drivers/tty/serial/fsl_lpuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index dbf8cccea105..a6f7b056d57b 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1775,6 +1775,7 @@ static void lpuart32_shutdown(struct uart_port *port)
 			UARTCTRL_TIE | UARTCTRL_TCIE | UARTCTRL_RIE |
 			UARTCTRL_LOOPS);
 	lpuart32_write(port, temp, UARTCTRL);
+	lpuart32_write(port, 0, UARTMODIR);
 
 	spin_unlock_irqrestore(&port->lock, flags);
 
-- 
2.17.1

[PATCH V2 4/5] tty: serial: fsl_lpuart: disable Rx/Tx DMA in lpuart32_shutdown()

[Sherry Sun]

UARTBAUD_RDMAE and UARTBAUD_TDMAE are enabled in lpuart32_startup(), but
lpuart32_shutdown() not disable them, only free the dma ring buffer and
release the dma channels, so here disable the Rx/Tx DMA first in
lpuart32_shutdown().

Fixes: 42b68768e51b ("serial: fsl_lpuart: DMA support for 32-bit variant")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
Changes in V2:
1. Split one patch into four smaller patches to improve the commit
messages and add Fixes tag as suggested by Ilpo.
---
 drivers/tty/serial/fsl_lpuart.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index a6f7b056d57b..c297be11422a 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1769,6 +1769,11 @@ static void lpuart32_shutdown(struct uart_port *port)
 
 	spin_lock_irqsave(&port->lock, flags);
 
+	/* disable Rx/Tx DMA */
+	temp = lpuart32_read(port, UARTBAUD);
+	temp &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
+	lpuart32_write(port, temp, UARTBAUD);
+
 	/* disable Rx/Tx and interrupts */
 	temp = lpuart32_read(port, UARTCTRL);
 	temp &= ~(UARTCTRL_TE | UARTCTRL_RE | UARTCTRL_ILIE |
-- 
2.17.1

[PATCH V2 5/5] tty: serial: fsl_lpuart: clear LPUART Status Register in lpuart32_shutdown()

[Sherry Sun]

The LPUART Status Register needs to be cleared when closing the uart
port to get a clean environment when reopening the uart.

Fixes: 380c966c093e ("tty: serial: fsl_lpuart: add 32-bit register interface support")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
Changes in V2:
1. Split one patch into four smaller patches to improve the commit
messages and add Fixes tag as suggested by Ilpo.
---
 drivers/tty/serial/fsl_lpuart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index c297be11422a..75162c4784dd 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1769,6 +1769,10 @@ static void lpuart32_shutdown(struct uart_port *port)
 
 	spin_lock_irqsave(&port->lock, flags);
 
+	/* clear statue */
+	temp = lpuart32_read(&sport->port, UARTSTAT);
+	lpuart32_write(&sport->port, temp, UARTSTAT);
+
 	/* disable Rx/Tx DMA */
 	temp = lpuart32_read(port, UARTBAUD);
 	temp &= ~(UARTBAUD_TDMAE | UARTBAUD_RDMAE);
-- 
2.17.1

Re: [PATCH V2 1/5] tty: serial: fsl_lpuart: only enable Idle Line Interrupt for non-dma case

[Greg KH]

On Thu, Nov 10, 2022 at 04:17:24PM +0800, Sherry Sun wrote:
> For the lpuart driver, the Idle Line Interrupt Enable now is only needed
> for the CPU mode, so enable the UARTCTRL_ILIE at the correct place, and
> clear it when shutdown.
> 
> Also need to configure the suitable UARTCTRL_IDLECFG, now the value is
> 0x7, represent 128 idle characters will trigger the Idle Line Interrupt.
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
> Changes in V2:
> 1. Use FIELD_PREP() and GENMASK() for easy access to UARTCTRL_IDLECFG
> fields as suggested by Ilpo.
> ---
>  drivers/tty/serial/fsl_lpuart.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

This commit breaks the build for me as FIELD_PREP() does not seem to be
included properly :(

Please fix up and resend.

thanks,

greg k-h

RE: [PATCH V2 1/5] tty: serial: fsl_lpuart: only enable Idle Line Interrupt for non-dma case

[Sherry Sun]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 2022年11月23日 0:56
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: jirislaby@kernel.org; michael@walle.cc; jingchang.lu@freescale.com;
> tomonori.sakita@sord.co.jp; atsushi.nemoto@sord.co.jp; linux-
> serial@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx <linux-
> imx@nxp.com>
> Subject: Re: [PATCH V2 1/5] tty: serial: fsl_lpuart: only enable Idle Line
> Interrupt for non-dma case
> 
> On Thu, Nov 10, 2022 at 04:17:24PM +0800, Sherry Sun wrote:
> > For the lpuart driver, the Idle Line Interrupt Enable now is only
> > needed for the CPU mode, so enable the UARTCTRL_ILIE at the correct
> > place, and clear it when shutdown.
> >
> > Also need to configure the suitable UARTCTRL_IDLECFG, now the value is
> > 0x7, represent 128 idle characters will trigger the Idle Line Interrupt.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> > Changes in V2:
> > 1. Use FIELD_PREP() and GENMASK() for easy access to UARTCTRL_IDLECFG
> > fields as suggested by Ilpo.
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> This commit breaks the build for me as FIELD_PREP() does not seem to be
> included properly :(
> 
> Please fix up and resend.

Hi Greg,

It is strange that the build is pass at my side. But anyway, I will include the corresponding head files for FIELD_PREP/GENMASK and resend it.
Sorry for the inconvenience.

Best Regards
Sherry

Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()

[Michael Walle]

Am 2022-11-10 09:17, schrieb Sherry Sun:
> UARTCTRL_LOOPS bit is set in lpuart32_set_mctrl() for loopback mode, 
> but
> nowhere clear this bit, it should be cleared when closing the uart port
> to avoid the loopback mode been enabled by default when reopening the
> uart.

It's cleared in set_mctrl(). What is the expectation from the serial
core here?

-michael

Re: [PATCH V2 5/5] tty: serial: fsl_lpuart: clear LPUART Status Register in lpuart32_shutdown()

[Michael Walle]

Am 2022-11-10 09:17, schrieb Sherry Sun:
> The LPUART Status Register needs to be cleared when closing the uart
> port to get a clean environment when reopening the uart.

Shouldn't it be cleared on startup instead? What if there was some
kind of event between shutdown and startup?

-michael

RE: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()

[Sherry Sun]

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: 2022年11月23日 18:34
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in
> lpuart32_shutdown()
> 
> Am 2022-11-10 09:17, schrieb Sherry Sun:
> > UARTCTRL_LOOPS bit is set in lpuart32_set_mctrl() for loopback mode,
> > but nowhere clear this bit, it should be cleared when closing the uart
> > port to avoid the loopback mode been enabled by default when reopening
> > the uart.
> 
> It's cleared in set_mctrl(). What is the expectation from the serial core here?
> 

Hi Michael,

If we call .set_mctrl(TIOCM_LOOP), the UARTCTRL_LOOPS will be set.
Then when we call .shutdown(), serial core won't call .set_mctrl() to clear it, so the UARTCTRL_LOOPS need to be cleared here.
Per my understanding, .shutdown() should clean up all the uart flags, as the transmitter and receiver will been disabled, we will re-configure all the settings needed when re-open the port.

Best Regards
Sherry

Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()

[Michael Walle]

Am 2022-11-23 11:58, schrieb Sherry Sun:
>> -----Original Message-----
>> From: Michael Walle <michael@walle.cc>
>> Sent: 2022年11月23日 18:34
>> To: Sherry Sun <sherry.sun@nxp.com>
>> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
>> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
>> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
>> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear 
>> UARTCTRL_LOOPS in
>> lpuart32_shutdown()
>> 
>> Am 2022-11-10 09:17, schrieb Sherry Sun:
>> > UARTCTRL_LOOPS bit is set in lpuart32_set_mctrl() for loopback mode,
>> > but nowhere clear this bit, it should be cleared when closing the uart
>> > port to avoid the loopback mode been enabled by default when reopening
>> > the uart.
>> 
>> It's cleared in set_mctrl(). What is the expectation from the serial 
>> core here?
>> 
> 
> Hi Michael,
> 
> If we call .set_mctrl(TIOCM_LOOP), the UARTCTRL_LOOPS will be set.
> Then when we call .shutdown(), serial core won't call .set_mctrl() to
> clear it, so the UARTCTRL_LOOPS need to be cleared here.
> Per my understanding, .shutdown() should clean up all the uart flags,
> as the transmitter and receiver will been disabled, we will
> re-configure all the settings needed when re-open the port.

Two things,
(1) should the loopback be cleared on a newly opened serial device?
(2) as mentioned in my other reply, this can also be handled in
     the startup. Eg. the startup can clear the loopback flag.
     (together with possible hardware events).

I'm not that deep into the serial core, thus my question about
the expectations from the serial core. I guess the answer to
(1) is yes, but better to ask.

-michael

RE: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()

[Sherry Sun]

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: 2022年11月23日 19:09
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in
> lpuart32_shutdown()
> 
> Am 2022-11-23 11:58, schrieb Sherry Sun:
> >> -----Original Message-----
> >> From: Michael Walle <michael@walle.cc>
> >> Sent: 2022年11月23日 18:34
> >> To: Sherry Sun <sherry.sun@nxp.com>
> >> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
> >> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
> >> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear
> >> UARTCTRL_LOOPS in
> >> lpuart32_shutdown()
> >>
> >> Am 2022-11-10 09:17, schrieb Sherry Sun:
> >> > UARTCTRL_LOOPS bit is set in lpuart32_set_mctrl() for loopback
> >> > mode, but nowhere clear this bit, it should be cleared when closing
> >> > the uart port to avoid the loopback mode been enabled by default
> >> > when reopening the uart.
> >>
> >> It's cleared in set_mctrl(). What is the expectation from the serial
> >> core here?
> >>
> >
> > Hi Michael,
> >
> > If we call .set_mctrl(TIOCM_LOOP), the UARTCTRL_LOOPS will be set.
> > Then when we call .shutdown(), serial core won't call .set_mctrl() to
> > clear it, so the UARTCTRL_LOOPS need to be cleared here.
> > Per my understanding, .shutdown() should clean up all the uart flags,
> > as the transmitter and receiver will been disabled, we will
> > re-configure all the settings needed when re-open the port.
> 
> Two things,
> (1) should the loopback be cleared on a newly opened serial device?
> (2) as mentioned in my other reply, this can also be handled in
>      the startup. Eg. the startup can clear the loopback flag.
>      (together with possible hardware events).
> 
> I'm not that deep into the serial core, thus my question about the
> expectations from the serial core. I guess the answer to
> (1) is yes, but better to ask.
> 

Hi Michael,

For the (1), I have checked the serial core, seems the answer is no, . startup() won't clean the status, only when the uart device is probed, lpuart will do the global reset to all the registers instead of .startup(). So I think the uart running status cleared in .shutdown() is reasonable.

Best Regards
Sherry

RE: [PATCH V2 5/5] tty: serial: fsl_lpuart: clear LPUART Status Register in lpuart32_shutdown()

[Sherry Sun]

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: 2022年11月23日 18:36
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 5/5] tty: serial: fsl_lpuart: clear LPUART Status
> Register in lpuart32_shutdown()
> 
> Am 2022-11-10 09:17, schrieb Sherry Sun:
> > The LPUART Status Register needs to be cleared when closing the uart
> > port to get a clean environment when reopening the uart.
> 
> Shouldn't it be cleared on startup instead? What if there was some kind of
> event between shutdown and startup?
> 

Hi Michael, same as what we discussed under another patch, unfortunately the .startup() won’t clear the status of the uart port.

Best Regards
Sherry

Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()

[Michael Walle]

Hi Sherry,

Am 2022-11-23 12:30, schrieb Sherry Sun:
>> -----Original Message-----
>> From: Michael Walle <michael@walle.cc>
>> Sent: 2022年11月23日 19:09
>> To: Sherry Sun <sherry.sun@nxp.com>
>> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
>> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
>> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
>> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear 
>> UARTCTRL_LOOPS in
>> lpuart32_shutdown()
>> 
>> Am 2022-11-23 11:58, schrieb Sherry Sun:
>> >> -----Original Message-----
>> >> From: Michael Walle <michael@walle.cc>
>> >> Sent: 2022年11月23日 18:34
>> >> To: Sherry Sun <sherry.sun@nxp.com>
>> >> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
>> >> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
>> >> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
>> >> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
>> >> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear
>> >> UARTCTRL_LOOPS in
>> >> lpuart32_shutdown()
>> >>
>> >> Am 2022-11-10 09:17, schrieb Sherry Sun:
>> >> > UARTCTRL_LOOPS bit is set in lpuart32_set_mctrl() for loopback
>> >> > mode, but nowhere clear this bit, it should be cleared when closing
>> >> > the uart port to avoid the loopback mode been enabled by default
>> >> > when reopening the uart.
>> >>
>> >> It's cleared in set_mctrl(). What is the expectation from the serial
>> >> core here?
>> >>
>> >
>> > Hi Michael,
>> >
>> > If we call .set_mctrl(TIOCM_LOOP), the UARTCTRL_LOOPS will be set.
>> > Then when we call .shutdown(), serial core won't call .set_mctrl() to
>> > clear it, so the UARTCTRL_LOOPS need to be cleared here.
>> > Per my understanding, .shutdown() should clean up all the uart flags,
>> > as the transmitter and receiver will been disabled, we will
>> > re-configure all the settings needed when re-open the port.
>> 
>> Two things,
>> (1) should the loopback be cleared on a newly opened serial device?
>> (2) as mentioned in my other reply, this can also be handled in
>>      the startup. Eg. the startup can clear the loopback flag.
>>      (together with possible hardware events).
>> 
>> I'm not that deep into the serial core, thus my question about the
>> expectations from the serial core. I guess the answer to
>> (1) is yes, but better to ask.
>> 
> 
> Hi Michael,
> 
> For the (1), I have checked the serial core, seems the answer is no, .
> startup() won't clean the status, only when the uart device is probed,
> lpuart will do the global reset to all the registers instead of
> .startup(). So I think the uart running status cleared in .shutdown()
> is reasonable.

That's not what I've meant. Even with this patch as it is right now,
the loopback flag is cleared on a "newly opened serial device". Just
with one difference, you are clearing the flag in shutdown.

My question was rather, should the loopback (or generally any mctrl 
flags)
be persistent across close/open cycles. E.g. looking at omap-serial.c,
this driver doesn't seem to handle the loopback flag at .startup() or
.shutdown(). Same seems to be true for sh-sci.c.

Greg?

-michael

RE: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in lpuart32_shutdown()

[Sherry Sun]

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: 2022年11月23日 19:43
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear UARTCTRL_LOOPS in
> lpuart32_shutdown()
> 
> Hi Sherry,
> 
> Am 2022-11-23 12:30, schrieb Sherry Sun:
> >> -----Original Message-----
> >> From: Michael Walle <michael@walle.cc>
> >> Sent: 2022年11月23日 19:09
> >> To: Sherry Sun <sherry.sun@nxp.com>
> >> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
> >> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
> >> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
> >> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear
> >> UARTCTRL_LOOPS in
> >> lpuart32_shutdown()
> >>
> >> Am 2022-11-23 11:58, schrieb Sherry Sun:
> >> >> -----Original Message-----
> >> >> From: Michael Walle <michael@walle.cc>
> >> >> Sent: 2022年11月23日 18:34
> >> >> To: Sherry Sun <sherry.sun@nxp.com>
> >> >> Cc: gregkh@linuxfoundation.org; jirislaby@kernel.org;
> >> >> jingchang.lu@freescale.com; tomonori.sakita@sord.co.jp;
> >> >> atsushi.nemoto@sord.co.jp; linux-serial@vger.kernel.org; linux-
> >> >> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> >> >> Subject: Re: [PATCH V2 2/5] tty: serial: fsl_lpuart: clear
> >> >> UARTCTRL_LOOPS in
> >> >> lpuart32_shutdown()
> >> >>
> >> >> Am 2022-11-10 09:17, schrieb Sherry Sun:
> >> >> > UARTCTRL_LOOPS bit is set in lpuart32_set_mctrl() for loopback
> >> >> > mode, but nowhere clear this bit, it should be cleared when
> >> >> > closing the uart port to avoid the loopback mode been enabled by
> >> >> > default when reopening the uart.
> >> >>
> >> >> It's cleared in set_mctrl(). What is the expectation from the
> >> >> serial core here?
> >> >>
> >> >
> >> > Hi Michael,
> >> >
> >> > If we call .set_mctrl(TIOCM_LOOP), the UARTCTRL_LOOPS will be set.
> >> > Then when we call .shutdown(), serial core won't call .set_mctrl()
> >> > to clear it, so the UARTCTRL_LOOPS need to be cleared here.
> >> > Per my understanding, .shutdown() should clean up all the uart
> >> > flags, as the transmitter and receiver will been disabled, we will
> >> > re-configure all the settings needed when re-open the port.
> >>
> >> Two things,
> >> (1) should the loopback be cleared on a newly opened serial device?
> >> (2) as mentioned in my other reply, this can also be handled in
> >>      the startup. Eg. the startup can clear the loopback flag.
> >>      (together with possible hardware events).
> >>
> >> I'm not that deep into the serial core, thus my question about the
> >> expectations from the serial core. I guess the answer to
> >> (1) is yes, but better to ask.
> >>
> >
> > Hi Michael,
> >
> > For the (1), I have checked the serial core, seems the answer is no, .
> > startup() won't clean the status, only when the uart device is probed,
> > lpuart will do the global reset to all the registers instead of
> > .startup(). So I think the uart running status cleared in .shutdown()
> > is reasonable.
> 
> That's not what I've meant. Even with this patch as it is right now, the
> loopback flag is cleared on a "newly opened serial device". Just with one
> difference, you are clearing the flag in shutdown.
> 
> My question was rather, should the loopback (or generally any mctrl
> flags)
> be persistent across close/open cycles. E.g. looking at omap-serial.c, this
> driver doesn't seem to handle the loopback flag at .startup() or .shutdown().
> Same seems to be true for sh-sci.c.
> 
> Greg?
> 

Hi Michael,

Now got your point, thanks for the clarification.
I have checked some other drivers, maybe you are right, now I am also confused that if the loopback flags should be persistent across close/open cycles. ☹

Best Regards
Sherry