Re: [PATCH 2/2] serial: stm32: Use TC interrupt to deassert GPIO RTS in RS485 mode

[Erwan LE RAY <erwan.leray@foss.st.com>]

Hi Marek,

On 4/30/22 18:28, Marek Vasut wrote:
> In case the RS485 mode is emulated using GPIO RTS, use the TC interrupt
> to deassert the GPIO RTS, otherwise the GPIO RTS stays asserted after a
> transmission ended and the RS485 cannot work.
> 
Could you please add a cover letter to explain the rational of the first 
patch ? I understood the goal of the first by reading the commit message 
of this second patch.

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Erwan Le Ray <erwan.leray@foss.st.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com>
> Cc: Valentin Caron <valentin.caron@foss.st.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-serial@vger.kernel.org
> ---
>   drivers/tty/serial/stm32-usart.c | 42 ++++++++++++++++++++++++++++++--
>   drivers/tty/serial/stm32-usart.h |  1 +
>   2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 224f359c6051e..764415b8e8f03 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -417,6 +417,14 @@ static void stm32_usart_tx_interrupt_enable(struct uart_port *port)
>   		stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TXEIE);
>   }
>   
> +static void stm32_usart_tc_interrupt_enable(struct uart_port *port)
> +{
> +	struct stm32_port *stm32_port = to_stm32_port(port);
> +	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> +
> +	stm32_usart_set_bits(port, ofs->cr1, USART_CR1_TCIE);
> +}
> +
I don't see the added value of this helper (only 1 instruction used 1 
time), and other Interrupt Enabled bits are already set/unset in others 
functions of this driver.
To keep an homogeneous code in the driver, could you please remove this 
helper and set TCIE directly when you need it ?

>   static void stm32_usart_rx_dma_complete(void *arg)
>   {
>   	struct uart_port *port = arg;
> @@ -442,6 +450,14 @@ static void stm32_usart_tx_interrupt_disable(struct uart_port *port)
>   		stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TXEIE);
>   }
>   
> +static void stm32_usart_tc_interrupt_disable(struct uart_port *port)
> +{
> +	struct stm32_port *stm32_port = to_stm32_port(port);
> +	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> +
> +	stm32_usart_clr_bits(port, ofs->cr1, USART_CR1_TCIE);
> +}
> +
Same comment here.

>   static void stm32_usart_rs485_rts_enable(struct uart_port *port)
>   {
>   	struct stm32_port *stm32_port = to_stm32_port(port);
> @@ -585,6 +601,13 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
>   	u32 isr;
>   	int ret;
>   
> +	if (!stm32_port->hw_flow_control &&
> +	    port->rs485.flags & SER_RS485_ENABLED) {
> +		stm32_port->txdone = false;
> +		stm32_usart_tc_interrupt_disable(port);
> +		stm32_usart_rs485_rts_enable(port);
> +	}
> +
>   	if (port->x_char) {
>   		if (stm32_usart_tx_dma_started(stm32_port) &&
>   		    stm32_usart_tx_dma_enabled(stm32_port))
> @@ -625,8 +648,14 @@ static void stm32_usart_transmit_chars(struct uart_port *port)
>   	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
>   		uart_write_wakeup(port);
>   
> -	if (uart_circ_empty(xmit))
> +	if (uart_circ_empty(xmit)) {
>   		stm32_usart_tx_interrupt_disable(port);
> +		if (!stm32_port->hw_flow_control &&
> +		    port->rs485.flags & SER_RS485_ENABLED) {
> +			stm32_port->txdone = true;
> +			stm32_usart_tc_interrupt_enable(port);
> +		}
> +	}
>   }
>   
>   static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
> @@ -640,6 +669,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>   
>   	sr = readl_relaxed(port->membase + ofs->isr);
>   
> +	if (!stm32_port->hw_flow_control &&
> +	    port->rs485.flags & SER_RS485_ENABLED &&
> +	    (sr & USART_SR_TC)) {
> +		stm32_usart_tc_interrupt_disable(port);
> +		stm32_usart_rs485_rts_disable(port);
> +	}
> +
>   	if ((sr & USART_SR_RTOF) && ofs->icr != UNDEF_REG)
>   		writel_relaxed(USART_ICR_RTOCF,
>   			       port->membase + ofs->icr);
> @@ -763,8 +799,10 @@ static void stm32_usart_start_tx(struct uart_port *port)
>   {
>   	struct circ_buf *xmit = &port->state->xmit;
>   
> -	if (uart_circ_empty(xmit) && !port->x_char)
> +	if (uart_circ_empty(xmit) && !port->x_char) {
> +		stm32_usart_rs485_rts_disable(port);
>   		return;
> +	}
>   
>   	stm32_usart_rs485_rts_enable(port);
>   
> diff --git a/drivers/tty/serial/stm32-usart.h b/drivers/tty/serial/stm32-usart.h
> index d734c4a5fd24c..ee69c203b926d 100644
> --- a/drivers/tty/serial/stm32-usart.h
> +++ b/drivers/tty/serial/stm32-usart.h
> @@ -271,6 +271,7 @@ struct stm32_port {
>   	bool hw_flow_control;
>   	bool swap;		 /* swap RX & TX pins */
>   	bool fifoen;
> +	bool txdone;
>   	int rxftcfg;		/* RX FIFO threshold CFG      */
>   	int txftcfg;		/* TX FIFO threshold CFG      */
>   	bool wakeup_src;

Regards, Erwan.