RE: [PATCH] tty: serial: fsl_lpuart: disable transmitter before changing RS485 related registers

[Sherry Sun]

> -----Original Message-----
> From: Shenwei Wang <shenwei.wang@xxxxxxx>
> Sent: Saturday, March 8, 2025 3:20 AM
> To: Sherry Sun <sherry.sun@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx;
> jirislaby@xxxxxxxxxx
> Cc: linux-serial@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH] tty: serial: fsl_lpuart: disable transmitter before changing
> RS485 related registers
> 
> 
> 
> > -----Original Message-----
> > From: Sherry Sun <sherry.sun@xxxxxxx>
> > Sent: Thursday, March 6, 2025 8:20 PM
> > To: gregkh@xxxxxxxxxxxxxxxxxxx; jirislaby@xxxxxxxxxx
> > Cc: linux-serial@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > imx@xxxxxxxxxxxxxxx; Shenwei Wang <shenwei.wang@xxxxxxx>
> > Subject: [PATCH] tty: serial: fsl_lpuart: disable transmitter before
> > changing RS485 related registers
> >
> > According to the LPUART reference manual, TXRTSE and TXRTSPOL of
> MODIR
> > register only can be changed when the transmitter is disabled.
> > So disable the transmitter before changing RS485 related registers and
> > re-enable it after the change is done.
> >
> > Fixes: 67b01837861c ("tty: serial: lpuart: Add RS485 support for
> > 32-bit uart
> > flavour")
> > Signed-off-by: Sherry Sun <sherry.sun@xxxxxxx>
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index
> > 91d02c55c470..4dc2f3e2b8e0 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -1484,6 +1484,16 @@ static int lpuart32_config_rs485(struct
> > uart_port *port, struct ktermios *termio
> >
> >  	unsigned long modem = lpuart32_read(&sport->port, UARTMODIR)
> >  				& ~(UARTMODIR_TXRTSPOL |
> > UARTMODIR_TXRTSE);
> > +	u32 ctrl;
> > +
> > +	/* TXRTSE and TXRTSPOL only can be changed when transmitter is
> > disabled. */
> > +	ctrl = lpuart32_read(&sport->port, UARTCTRL);
> > +	if (ctrl & UARTCTRL_TE) {
> > +		/* wait transmit engin complete */
> > +		lpuart32_wait_bit_set(&sport->port, UARTSTAT,
> UARTSTAT_TC);
> > +		lpuart32_write(&sport->port, ctrl & ~UARTCTRL_TE,
> > UARTCTRL);
> 
> Since there may be a delay between writing to the register and the
> UARTCTRL_TE bit actually changing to 0, we should poll the UARTCTRL register
> and verify that UARTCTRL_TE has really become 0 before proceeding.
> Otherwise, subsequent operations would still execute while the UARTCTRL_TE
> bit remains in the status of 1, which is not the intention of this patch.
> 

Hi Shenwei,

Description of TE bit in LPUART RM: "After this field becomes 0, the field reads 1 until the transmitter has completed the current character and the TXD pin is tristated".
I added the UARTSTAT_TC status check to make sure the transmitter has completed, not sure if it is reasonable to add the TE bit poll read, since usually we poll read the status register bits instead of the control bits.

Best Regards
Sherry


> Thanks,
> Shenwei
> 
> > +	}
> > +
> >  	lpuart32_write(&sport->port, modem, UARTMODIR);
> >
> >  	if (rs485->flags & SER_RS485_ENABLED) { @@ -1503,6 +1513,10 @@
> > static int lpuart32_config_rs485(struct uart_port *port, struct ktermios
> *termio
> >  	}
> >
> >  	lpuart32_write(&sport->port, modem, UARTMODIR);
> > +
> > +	if (ctrl & UARTCTRL_TE)
> > +		lpuart32_write(&sport->port, ctrl, UARTCTRL);
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.34.1