bug in lpuart32_setup_watermark - clears all other UARTMODIR fields when setting RTSWATER

[petero5]

Hi

Looks like there is a bug in lpuart32_setup_watermark.
It clears all other UARTMODIR fields when setting the RTSWATER field:

https://github.com/nxp-imx/linux-imx/blob/lf-5.15.y/drivers/tty/serial/fsl_lpuart.c#L1719

/* set RTS watermark */
if (!uart_console(&sport->port)) {
val = lpuart32_read(&sport->port, UARTMODIR);
val = (sport->rxfifo_size >> 1) << UARTMODIR_RTSWATER_S;
lpuart32_write(&sport->port, val, UARTMODIR);
}

It forgets to mask RTSWATER, to preserve the other fields e.g. TXRTSPOL, TXRTSE etc.
Compare with the masking (&= ~ and |=) when handling FIFO mode in the same function.

PS. The issue that we are having is that we have in the device tree:
&lpuart1 {
linux,rs485-enabled-at-boot-time;

TXRTSPOL, TXRTSE have the expected values initially, but after echo or cat /dev/ttyLP1, lpuart32_setup_watermark clears those bits...

Thank you
-- Peter

[Zhiming_Liu]

Hello @petero5 

Do you want to report this bug or you already have solution to fix this ?

Best Regards

Zhiming

[petero5]

Hi Zhiming @Zhiming_Liu 

Yes please, I'd like to report this bug in lpuart32_setup_watermark plus a similar issue in lpuart32_shutdown.
What do we need to do to report these please?
Also need some advice re the lpuart32_shutdown side of this please.

Our transceiver, ST485B, needs RTS to be high when the the lpuart is sending, but low at all other times.
Using linux,rs485-enabled-at-boot-time in the device tree sets RTS low from boot.
The modem register fields, TXRTSE and TXRTSPOL are 1, which is what we need.

However, when anything calls open() on /dev/ttyLP1, the bug in lpuart32_setup_watermark clears those bits, which makes RTS permanently high instead of low...
And even if we workaround that bug on open(), on the close(), lpuart32_shutdown zeros the modem register again, making RTS permanently high again...

I've fixed my copy of lpuart32_setup_watermark, by adding the missing bit masking.
I guess that the author of lpuart32_setup_watermark forgot to add the masking, as the read is there, but the val that was read is overwritten?
Be good if this could be fixed in the official repo(s).

However for lpuart32_shutdown, the issue seems more intentional, and I'm less sure on the correct solution.

The issue in lpuart32_shutdown is:
https://github.com/nxp-imx/linux-imx/blob/lf-5.15.y/drivers/tty/serial/fsl_lpuart.c#L2026
lpuart32_write(port, 0, UARTMODIR)

We do not want this to zero TXRTSE and TXRTSPOL, so we could workaround this, quick and dirty by saving just those two bits.
But for a proper fix, which of all of the modem register fields should be zero'd and which should be preserved on close/shutdown?

Thank you
-- Peter

[Zhiming_Liu]

The software team has fixed this bug in upstream, you can import this in L5.X

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 899066daf16a..238ab0944284 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1722,7 +1722,7 @@ static void lpuart32_setup_watermark(struct lpuart_port *sport)
        /* set RTS watermark */
        if (!uart_console(&sport->port)) {
                val = lpuart32_read(&sport->port, UARTMODIR);
-               val = (sport->rxfifo_size >> 1) << UARTMODIR_RTSWATER_S;
+               val |= (sport->rxfifo_size >> 1) << UARTMODIR_RTSWATER_S;
                lpuart32_write(&sport->port, val, UARTMODIR);
        }

 

@@ -2029,7 +2029,6 @@ static void lpuart32_shutdown(struct uart_port *port)
                UARTCTRL_TCIE | UARTCTRL_RIE | UARTCTRL_ILIE |
                UARTCTRL_LOOPS);
        lpuart32_write(port, temp, UARTCTRL);
-       lpuart32_write(port, 0, UARTMODIR);

 

        if (sport->lpuart_dma_rx_use)
                sport->dma_rx_chan_active = false;

[petero5]

Hi @Zhiming_Liu 

Thank you.
Is that fix to lpuart32_setup_watermark definitely correct?
It looks like it will OR the old RTSWATER bits with the new RTSWATER bits, rather than replace them cleanly.
By comparison, higher up in that function when working with RXIDEN, it first zeros the RXIDEN bits using & and ~ like so:
https://github.com/nxp-imx/linux-imx/blob/lf-5.15.y/drivers/tty/serial/fsl_lpuart.c#L1703
val &= ~(UARTFIFO_RXIDEN_MASK << UARTFIFO_RXIDEN_OFF);
val |= ((rxiden_cnt & UARTFIFO_RXIDEN_MASK) << UARTFIFO_RXIDEN_OFF);

Thank you
-- Peter

[Zhiming_Liu]

This is a simple fix, please refer the last upstream code.

[petero5]

Hi @Zhiming_Liu 

Thank you.
I've spent some time searching for the last upstream code, but am still unsure where to look.
What is the URL or repo and branch for upstream please?

For example, I can still see the bug in lpuart32_setup_watermark in the following places:
https://github.com/nxp-imx/linux-imx/blob/lf-5.15.y/drivers/tty/serial/fsl_lpuart.c#L1719
https://github.com/nxp-imx/linux-imx/blob/lf-6.1.y/drivers/tty/serial/fsl_lpuart.c#L1649
https://github.com/Freescale/linux-fslc/blob/5.15-2.2.x-imx/drivers/tty/serial/fsl_lpuart.c#L1720

Thank you
-- Peter