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

[PATCH 0/2] 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 shutdown the uart port.

The patches have been tested on imx8ulp-evk platform.

Sherry Sun (2):
  tty: serial: fsl_lpuart: only enable Idle Line Interrupt for non-dma
    case
  tty: serial: fsl_lpuart: improve lpuart32 registers clearing when
    shutdown

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

-- 
2.17.1

[PATCH 1/2] 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>
---
 drivers/tty/serial/fsl_lpuart.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index bd685491eead..f5a0a14fa366 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_OFF   8
 #define UARTCTRL_LOOPS		0x00000080
 #define UARTCTRL_DOZEEN		0x00000040
 #define UARTCTRL_RSRC		0x00000020
@@ -230,6 +230,8 @@
 #define GLOBAL_RST_MIN_US	20
 #define GLOBAL_RST_MAX_US	40
 
+#define UARTCTRL_IDLECFG	0x7
+
 /* Rx DMA timeout in ms, which is used to calculate Rx ring buffer size */
 #define DMA_RX_TIMEOUT		(10)
 
@@ -1506,7 +1508,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 +1532,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 |= UARTCTRL_IDLECFG << UARTCTRL_IDLECFG_OFF;
 	lpuart32_write(&sport->port, temp, UARTCTRL);
 }
 
@@ -1669,7 +1672,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 +1773,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

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

[Ilpo Järvinen]

On Wed, 9 Nov 2022, 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>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index bd685491eead..f5a0a14fa366 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_OFF   8
>  #define UARTCTRL_LOOPS		0x00000080
>  #define UARTCTRL_DOZEEN		0x00000040
>  #define UARTCTRL_RSRC		0x00000020
> @@ -230,6 +230,8 @@
>  #define GLOBAL_RST_MIN_US	20
>  #define GLOBAL_RST_MAX_US	40
>  
> +#define UARTCTRL_IDLECFG	0x7
> +

GEN_MASK() to the correct bits directly?

>  /* Rx DMA timeout in ms, which is used to calculate Rx ring buffer size */
>  #define DMA_RX_TIMEOUT		(10)
>  
> @@ -1506,7 +1508,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 +1532,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 |= UARTCTRL_IDLECFG << UARTCTRL_IDLECFG_OFF;

FIELD_PREP() would probably be more appropriate for this? Then you can 
also drop the shift offset.

-- 
 i.

>  	lpuart32_write(&sport->port, temp, UARTCTRL);
>  }
>  
> @@ -1669,7 +1672,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 +1773,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);
>  
> 

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

[Sherry Sun]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: 2022年11月9日 20:02
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; linux-serial <linux-serial@vger.kernel.org>; LKML
> <linux-kernel@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 1/2] tty: serial: fsl_lpuart: only enable Idle Line Interrupt
> for non-dma case
> 
> On Wed, 9 Nov 2022, 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>
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index bd685491eead..f5a0a14fa366
> > 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_OFF   8
> >  #define UARTCTRL_LOOPS		0x00000080
> >  #define UARTCTRL_DOZEEN		0x00000040
> >  #define UARTCTRL_RSRC		0x00000020
> > @@ -230,6 +230,8 @@
> >  #define GLOBAL_RST_MIN_US	20
> >  #define GLOBAL_RST_MAX_US	40
> >
> > +#define UARTCTRL_IDLECFG	0x7
> > +
> 
> GEN_MASK() to the correct bits directly?
> 
> >  /* Rx DMA timeout in ms, which is used to calculate Rx ring buffer size */
> >  #define DMA_RX_TIMEOUT		(10)
> >
> > @@ -1506,7 +1508,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 +1532,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 |= UARTCTRL_IDLECFG << UARTCTRL_IDLECFG_OFF;
> 
> FIELD_PREP() would probably be more appropriate for this? Then you can
> also drop the shift offset.

Hi Ilpo, thanks, I will try to use FIELD_PREP() and GENMASK().

Best Regards
Sherry


> 
> --
>  i.
> 
> >  	lpuart32_write(&sport->port, temp, UARTCTRL);  }
> >
> > @@ -1669,7 +1672,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
> +1773,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);
> >
> >

[PATCH 2/2] tty: serial: fsl_lpuart: improve lpuart32 registers clearing when shutdown

[Sherry Sun]

Need to clear the UARTSTAT and UARTMODIR registers when shutdown the
uart port, also clear the Rx/Tx DMA enable bits and loopback
configuration bit.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index f5a0a14fa366..43d9d6a6e94a 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1771,11 +1771,22 @@ 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);
+	lpuart32_write(port, temp, UARTBAUD);
+
 	/* 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);
+	lpuart32_write(port, 0, UARTMODIR);
 
 	spin_unlock_irqrestore(&port->lock, flags);
 
-- 
2.17.1

Re: [PATCH 2/2] tty: serial: fsl_lpuart: improve lpuart32 registers clearing when shutdown

[Ilpo Järvinen]

On Wed, 9 Nov 2022, Sherry Sun wrote:

> Need to clear the UARTSTAT and UARTMODIR registers when shutdown the
> uart port, also clear the Rx/Tx DMA enable bits and loopback
> configuration bit.

This lacks answer to "Why?" question. Think about somebody not as familiar 
with the HW as you are looking back to this very commit message like 5 
years from now and wondering why this change was made.

Preferrably make a separate change out of all these four changes if the 
answers to why question are different.

It would also help in deciding whether Fixes tag is necessary or not 
since you didn't seem to include.

-- 
 i.

> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index f5a0a14fa366..43d9d6a6e94a 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1771,11 +1771,22 @@ 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);
> +	lpuart32_write(port, temp, UARTBAUD);
> +
>  	/* 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);
> +	lpuart32_write(port, 0, UARTMODIR);
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
> 

RE: [PATCH 2/2] tty: serial: fsl_lpuart: improve lpuart32 registers clearing when shutdown

[Sherry Sun]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: 2022年11月9日 20:19
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; linux-serial <linux-serial@vger.kernel.org>; LKML
> <linux-kernel@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: improve lpuart32 registers
> clearing when shutdown
> 
> On Wed, 9 Nov 2022, Sherry Sun wrote:
> 
> > Need to clear the UARTSTAT and UARTMODIR registers when shutdown the
> > uart port, also clear the Rx/Tx DMA enable bits and loopback
> > configuration bit.
> 
> This lacks answer to "Why?" question. Think about somebody not as familiar
> with the HW as you are looking back to this very commit message like 5 years
> from now and wondering why this change was made.
> 
> Preferrably make a separate change out of all these four changes if the
> answers to why question are different.
> 
> It would also help in deciding whether Fixes tag is necessary or not since you
> didn't seem to include.

Hi Ilpo,

Ok, maybe I need to separate the four changes in lpuart32_shutdown() to better describe the "why". Thanks for the comments.

Best Regards
Sherry


> 
> --
>  i.
> 
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index f5a0a14fa366..43d9d6a6e94a
> > 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -1771,11 +1771,22 @@ 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);
> > +	lpuart32_write(port, temp, UARTBAUD);
> > +
> >  	/* 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);
> > +	lpuart32_write(port, 0, UARTMODIR);
> >
> >  	spin_unlock_irqrestore(&port->lock, flags);
> >
> >