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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
>
>
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 <[email protected]>
> ---
> 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);
>
>
> -----Original Message-----
> From: Ilpo Järvinen <[email protected]>
> Sent: 2022年11月9日 20:19
> To: Sherry Sun <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby
> <[email protected]>; linux-serial <[email protected]>; LKML
> <[email protected]>; dl-linux-imx <[email protected]>
> 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 <[email protected]>
> > ---
> > 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);
> >
> >
> -----Original Message-----
> From: Ilpo Järvinen <[email protected]>
> Sent: 2022年11月9日 20:02
> To: Sherry Sun <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Jiri Slaby
> <[email protected]>; linux-serial <[email protected]>; LKML
> <[email protected]>; dl-linux-imx <[email protected]>
> 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 <[email protected]>
> > ---
> > 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);
> >
> >