diff mbox series

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

Message ID 20221110081728.10172-3-sherry.sun@nxp.com
State New
Headers
Series fsl_lpuart: improve Idle Line Interrupt and registers handle in .shutdown() |

Commit Message

Sherry Sun Nov. 10, 2022, 8:17 a.m. UTC 
  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(-)

Comments

Michael Walle Nov. 23, 2022, 10:34 a.m. UTC | #1 
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
Sherry Sun Nov. 23, 2022, 10:58 a.m. UTC | #2 
> -----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
Michael Walle Nov. 23, 2022, 11:09 a.m. UTC | #3 
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
Sherry Sun Nov. 23, 2022, 11:30 a.m. UTC | #4 
> -----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
Michael Walle Nov. 23, 2022, 11:42 a.m. UTC | #5 
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
Sherry Sun Nov. 23, 2022, 1:06 p.m. UTC | #6 
> -----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
diff mbox series

Patch

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);