2019-10-16 17:04:18

by Philippe Schenker

Subject: [PATCH v1 1/3] tty: serial: lpuart: Remove unnecessary code from set_mctrl

Currently flow control is not working due to lpuart32_set_mctrl that is
clearing TXCTSE bit in all cases. This bit gets earlier setup by
lpuart32_set_termios.

As I read in Documentation set_mctrl is also not meant for hardware
flow control rather than gpio setting and clearing a RTS signal.
Therefore I guess it is safe to remove the whole code in
lpuart32_set_mctrl.

This was tested with console on a i.MX8QXP SoC.

Signed-off-by: Philippe Schenker <[email protected]>
---

drivers/tty/serial/fsl_lpuart.c | 11 -----------
1 file changed, 11 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 537896c4d887..f3271857621c 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1333,18 +1333,7 @@ static void lpuart_set_mctrl(struct uart_port *port, unsigned int mctrl)

static void lpuart32_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
- unsigned long temp;
-
- temp = lpuart32_read(port, UARTMODIR) &
- ~(UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
-
- if (mctrl & TIOCM_RTS)
- temp |= UARTMODIR_RXRTSE;
-
- if (mctrl & TIOCM_CTS)
- temp |= UARTMODIR_TXCTSE;

- lpuart32_write(port, temp, UARTMODIR);
}

static void lpuart_break_ctl(struct uart_port *port, int break_state)
--
2.23.0

2019-10-16 17:04:21

by Philippe Schenker

Subject: [PATCH v1 2/3] tty: serial: lpuart: Use defines that correspond to correct register

Use UARTMODIR defines instead of UARTMODEM as it is a 32-bit function

Signed-off-by: Philippe Schenker <[email protected]>
---

drivers/tty/serial/fsl_lpuart.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index f3271857621c..346b4a070ce9 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1879,10 +1879,10 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
}

if (termios->c_cflag & CRTSCTS) {
- modem |= UARTMODEM_RXRTSE | UARTMODEM_TXCTSE;
+ modem |= (UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
} else {
termios->c_cflag &= ~CRTSCTS;
- modem &= ~(UARTMODEM_RXRTSE | UARTMODEM_TXCTSE);
+ modem &= ~(UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
}

if (termios->c_cflag & CSTOPB)
--
2.23.0

2019-10-16 17:04:41

by Philippe Schenker

Subject: [PATCH v1 3/3] tty: serial: lpuart: Add RS485 support for 32-bit uart flavour

This commits adds RS485 support for LPUART hardware that uses 32-bit
registers. These are typically found in i.MX8 processors.

Signed-off-by: Philippe Schenker <[email protected]>

---

drivers/tty/serial/fsl_lpuart.c | 65 ++++++++++++++++++++++++++++++++-
1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 346b4a070ce9..22df5f8f48b6 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1280,6 +1280,57 @@ static int lpuart_config_rs485(struct uart_port *port,
return 0;
}

+static int lpuart32_config_rs485(struct uart_port *port,
+ struct serial_rs485 *rs485)
+{
+ struct lpuart_port *sport = container_of(port,
+ struct lpuart_port, port);
+
+ unsigned long modem = lpuart32_read(&sport->port, UARTMODIR)
+ & ~(UARTMODEM_TXRTSPOL | UARTMODEM_TXRTSE);
+ lpuart32_write(&sport->port, modem, UARTMODIR);
+
+ /* clear unsupported configurations */
+ rs485->delay_rts_before_send = 0;
+ rs485->delay_rts_after_send = 0;
+ rs485->flags &= ~SER_RS485_RX_DURING_TX;
+
+ if (rs485->flags & SER_RS485_ENABLED) {
+ /* Enable auto RS-485 RTS mode */
+ modem |= UARTMODEM_TXRTSE;
+
+ /*
+ * RTS needs to be logic HIGH either during transer _or_ after
+ * transfer, other variants are not supported by the hardware.
+ */
+
+ if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
+ SER_RS485_RTS_AFTER_SEND)))
+ rs485->flags |= SER_RS485_RTS_ON_SEND;
+
+ if (rs485->flags & SER_RS485_RTS_ON_SEND &&
+ rs485->flags & SER_RS485_RTS_AFTER_SEND)
+ rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+
+ /*
+ * The hardware defaults to RTS logic HIGH while transfer.
+ * Switch polarity in case RTS shall be logic HIGH
+ * after transfer.
+ * Note: UART is assumed to be active high.
+ */
+ if (rs485->flags & SER_RS485_RTS_ON_SEND)
+ modem &= ~UARTMODEM_TXRTSPOL;
+ else if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
+ modem |= UARTMODEM_TXRTSPOL;
+ }
+
+ /* Store the new configuration */
+ sport->port.rs485 = *rs485;
+
+ lpuart32_write(&sport->port, modem, UARTMODIR);
+ return 0;
+}
+
static unsigned int lpuart_get_mctrl(struct uart_port *port)
{
unsigned int temp = 0;
@@ -1878,6 +1929,13 @@ lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
ctrl |= UARTCTRL_M;
}

+ /*
+ * When auto RS-485 RTS mode is enabled,
+ * hardware flow control need to be disabled.
+ */
+ if (sport->port.rs485.flags & SER_RS485_ENABLED)
+ termios->c_cflag &= ~CRTSCTS;
+
if (termios->c_cflag & CRTSCTS) {
modem |= (UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
} else {
@@ -2405,7 +2463,10 @@ static int lpuart_probe(struct platform_device *pdev)
sport->port.ops = &lpuart_pops;
sport->port.flags = UPF_BOOT_AUTOCONF;

- sport->port.rs485_config = lpuart_config_rs485;
+ if (lpuart_is_32(sport))
+ sport->port.rs485_config = lpuart32_config_rs485;
+ else
+ sport->port.rs485_config = lpuart_config_rs485;

sport->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
if (IS_ERR(sport->ipg_clk)) {
@@ -2459,7 +2520,7 @@ static int lpuart_probe(struct platform_device *pdev)
sport->port.rs485.delay_rts_after_send)
dev_err(&pdev->dev, "driver doesn't support RTS delays\n");

- lpuart_config_rs485(&sport->port, &sport->port.rs485);
+ sport->port.rs485_config(&sport->port, &sport->port.rs485);

sport->dma_tx_chan = dma_request_slave_channel(sport->port.dev, "tx");
if (!sport->dma_tx_chan)
--
2.23.0

2019-10-17 13:33:42

by Stefan Agner

Subject: Re: [PATCH v1 2/3] tty: serial: lpuart: Use defines that correspond to correct register

On 2019-10-16 17:18, Philippe Schenker wrote:
> Use UARTMODIR defines instead of UARTMODEM as it is a 32-bit function

This reads a bit strange at first. Also it is helpful for later to state
that this does not make a difference in practise, so how about:

Use define from the 32-bit register description UARTMODIR_* instead of
UARTMODEM_*. The value is the same, so there is no functional change.

Otherwise looks good to me:

Reviewed-by: Stefan Agner <[email protected]>

--
Stefan

>
> Signed-off-by: Philippe Schenker <[email protected]>
> ---
>
> drivers/tty/serial/fsl_lpuart.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index f3271857621c..346b4a070ce9 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1879,10 +1879,10 @@ lpuart32_set_termios(struct uart_port *port,
> struct ktermios *termios,
> }
>
> if (termios->c_cflag & CRTSCTS) {
> - modem |= UARTMODEM_RXRTSE | UARTMODEM_TXCTSE;
> + modem |= (UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
> } else {
> termios->c_cflag &= ~CRTSCTS;
> - modem &= ~(UARTMODEM_RXRTSE | UARTMODEM_TXCTSE);
> + modem &= ~(UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
> }
>
> if (termios->c_cflag & CSTOPB)

2019-10-18 09:41:09

by Andy Duan

Subject: RE: [EXT] [PATCH v1 1/3] tty: serial: lpuart: Remove unnecessary code from set_mctrl

From: Philippe Schenker <[email protected]> Sent: Wednesday, October 16, 2019 11:19 PM
> Currently flow control is not working due to lpuart32_set_mctrl that is
> clearing TXCTSE bit in all cases. This bit gets earlier setup by
> lpuart32_set_termios.
>
> As I read in Documentation set_mctrl is also not meant for hardware flow
> control rather than gpio setting and clearing a RTS signal.
> Therefore I guess it is safe to remove the whole code in lpuart32_set_mctrl.
>
> This was tested with console on a i.MX8QXP SoC.
>
> Signed-off-by: Philippe Schenker <[email protected]>

Reviewed-by: Fugang Duan <[email protected]>
> ---
>
> drivers/tty/serial/fsl_lpuart.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index
> 537896c4d887..f3271857621c 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1333,18 +1333,7 @@ static void lpuart_set_mctrl(struct uart_port
> *port, unsigned int mctrl)
>
> static void lpuart32_set_mctrl(struct uart_port *port, unsigned int mctrl) {
> - unsigned long temp;
> -
> - temp = lpuart32_read(port, UARTMODIR) &
> - ~(UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> -
> - if (mctrl & TIOCM_RTS)
> - temp |= UARTMODIR_RXRTSE;
> -
> - if (mctrl & TIOCM_CTS)
> - temp |= UARTMODIR_TXCTSE;
>
> - lpuart32_write(port, temp, UARTMODIR);
> }
>
> static void lpuart_break_ctl(struct uart_port *port, int break_state)
> --
> 2.23.0

2019-10-18 09:41:12

by Andy Duan

Subject: RE: [EXT] [PATCH v1 2/3] tty: serial: lpuart: Use defines that correspond to correct register

From: Philippe Schenker <[email protected]> Sent: Wednesday, October 16, 2019 11:19 PM
> Use UARTMODIR defines instead of UARTMODEM as it is a 32-bit function
>
> Signed-off-by: Philippe Schenker <[email protected]>

Reviewed-by: Fugang Duan <[email protected]>

> ---
>
> drivers/tty/serial/fsl_lpuart.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index
> f3271857621c..346b4a070ce9 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1879,10 +1879,10 @@ lpuart32_set_termios(struct uart_port *port,
> struct ktermios *termios,
> }
>
> if (termios->c_cflag & CRTSCTS) {
> - modem |= UARTMODEM_RXRTSE |
> UARTMODEM_TXCTSE;
> + modem |= (UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> } else {
> termios->c_cflag &= ~CRTSCTS;
> - modem &= ~(UARTMODEM_RXRTSE |
> UARTMODEM_TXCTSE);
> + modem &= ~(UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> }
>
> if (termios->c_cflag & CSTOPB)
> --
> 2.23.0

2019-10-18 09:41:15

by Andy Duan

Subject: RE: [EXT] [PATCH v1 3/3] tty: serial: lpuart: Add RS485 support for 32-bit uart flavour

From: Philippe Schenker <[email protected]> Sent: Wednesday, October 16, 2019 11:19 PM
> This commits adds RS485 support for LPUART hardware that uses 32-bit
> registers. These are typically found in i.MX8 processors.
>
> Signed-off-by: Philippe Schenker <[email protected]>

Reviewed-by: Fugang Duan <[email protected]>
>
> ---
>
> drivers/tty/serial/fsl_lpuart.c | 65 ++++++++++++++++++++++++++++++++-
> 1 file changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index
> 346b4a070ce9..22df5f8f48b6 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1280,6 +1280,57 @@ static int lpuart_config_rs485(struct uart_port
> *port,
> return 0;
> }
>
> +static int lpuart32_config_rs485(struct uart_port *port,
> + struct serial_rs485 *rs485) {
> + struct lpuart_port *sport = container_of(port,
> + struct lpuart_port, port);
> +
> + unsigned long modem = lpuart32_read(&sport->port, UARTMODIR)
> + & ~(UARTMODEM_TXRTSPOL |
> UARTMODEM_TXRTSE);
> + lpuart32_write(&sport->port, modem, UARTMODIR);
> +
> + /* clear unsupported configurations */
> + rs485->delay_rts_before_send = 0;
> + rs485->delay_rts_after_send = 0;
> + rs485->flags &= ~SER_RS485_RX_DURING_TX;
> +
> + if (rs485->flags & SER_RS485_ENABLED) {
> + /* Enable auto RS-485 RTS mode */
> + modem |= UARTMODEM_TXRTSE;
> +
> + /*
> + * RTS needs to be logic HIGH either during transer _or_
> after
> + * transfer, other variants are not supported by the
> hardware.
> + */
> +
> + if (!(rs485->flags & (SER_RS485_RTS_ON_SEND |
> + SER_RS485_RTS_AFTER_SEND)))
> + rs485->flags |= SER_RS485_RTS_ON_SEND;
> +
> + if (rs485->flags & SER_RS485_RTS_ON_SEND &&
> + rs485->flags &
> SER_RS485_RTS_AFTER_SEND)
> + rs485->flags &=
> ~SER_RS485_RTS_AFTER_SEND;
> +
> + /*
> + * The hardware defaults to RTS logic HIGH while
> transfer.
> + * Switch polarity in case RTS shall be logic HIGH
> + * after transfer.
> + * Note: UART is assumed to be active high.
> + */
> + if (rs485->flags & SER_RS485_RTS_ON_SEND)
> + modem &= ~UARTMODEM_TXRTSPOL;
> + else if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
> + modem |= UARTMODEM_TXRTSPOL;
> + }
> +
> + /* Store the new configuration */
> + sport->port.rs485 = *rs485;
> +
> + lpuart32_write(&sport->port, modem, UARTMODIR);
> + return 0;
> +}
> +
> static unsigned int lpuart_get_mctrl(struct uart_port *port) {
> unsigned int temp = 0;
> @@ -1878,6 +1929,13 @@ lpuart32_set_termios(struct uart_port *port,
> struct ktermios *termios,
> ctrl |= UARTCTRL_M;
> }
>
> + /*
> + * When auto RS-485 RTS mode is enabled,
> + * hardware flow control need to be disabled.
> + */
> + if (sport->port.rs485.flags & SER_RS485_ENABLED)
> + termios->c_cflag &= ~CRTSCTS;
> +
> if (termios->c_cflag & CRTSCTS) {
> modem |= (UARTMODIR_RXRTSE |
> UARTMODIR_TXCTSE);
> } else {
> @@ -2405,7 +2463,10 @@ static int lpuart_probe(struct platform_device
> *pdev)
> sport->port.ops = &lpuart_pops;
> sport->port.flags = UPF_BOOT_AUTOCONF;
>
> - sport->port.rs485_config = lpuart_config_rs485;
> + if (lpuart_is_32(sport))
> + sport->port.rs485_config = lpuart32_config_rs485;
> + else
> + sport->port.rs485_config = lpuart_config_rs485;
>
> sport->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> if (IS_ERR(sport->ipg_clk)) {
> @@ -2459,7 +2520,7 @@ static int lpuart_probe(struct platform_device
> *pdev)
> sport->port.rs485.delay_rts_after_send)
> dev_err(&pdev->dev, "driver doesn't support RTS
> delays\n");
>
> - lpuart_config_rs485(&sport->port, &sport->port.rs485);
> + sport->port.rs485_config(&sport->port, &sport->port.rs485);
>
> sport->dma_tx_chan =
> dma_request_slave_channel(sport->port.dev, "tx");
> if (!sport->dma_tx_chan)
> --
> 2.23.0

2019-10-18 10:15:02

by Philippe Schenker

Subject: Re: [PATCH v1 2/3] tty: serial: lpuart: Use defines that correspond to correct register

On Wed, 2019-10-16 at 22:22 +0200, Stefan Agner wrote:
> On 2019-10-16 17:18, Philippe Schenker wrote:
> > Use UARTMODIR defines instead of UARTMODEM as it is a 32-bit
> > function
>
> This reads a bit strange at first. Also it is helpful for later to
> state
> that this does not make a difference in practise, so how about:
>
> Use define from the 32-bit register description UARTMODIR_* instead of
> UARTMODEM_*. The value is the same, so there is no functional change.
>
> Otherwise looks good to me:
>
> Reviewed-by: Stefan Agner <[email protected]>

Thanks for your review and comment. And also thanks to Andy Duan for his
reviews!

You're right, I could have included that so it is clear that nothing is
changed. I will send a v2 today with your suggested commit message

Philippe

>
> --
> Stefan
>
> > Signed-off-by: Philippe Schenker <[email protected]>
> > ---
> >
> > drivers/tty/serial/fsl_lpuart.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c
> > index f3271857621c..346b4a070ce9 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -1879,10 +1879,10 @@ lpuart32_set_termios(struct uart_port *port,
> > struct ktermios *termios,
> > }
> >
> > if (termios->c_cflag & CRTSCTS) {
> > - modem |= UARTMODEM_RXRTSE | UARTMODEM_TXCTSE;
> > + modem |= (UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
> > } else {
> > termios->c_cflag &= ~CRTSCTS;
> > - modem &= ~(UARTMODEM_RXRTSE | UARTMODEM_TXCTSE);
> > + modem &= ~(UARTMODIR_RXRTSE | UARTMODIR_TXCTSE);
> > }
> >
> > if (termios->c_cflag & CSTOPB)