diff mbox series

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

Message ID 20220430162845.244655-2-marex@denx.de
State New
Headers show
Series [1/2] serial: stm32: Factor out GPIO RTS toggling into separate function | expand

Commit Message

Marek Vasut April 30, 2022, 4:28 p.m. UTC
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.

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(-)

Comments

Erwan LE RAY May 2, 2022, 8:44 a.m. UTC | #1
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.
Marek Vasut May 2, 2022, 11:13 a.m. UTC | #2
On 5/2/22 10:44, Erwan LE RAY wrote:
> Hi Marek,

Hi,

> 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.

The rationale is trivial -- make sure we don't have five copies of the 
same block of code in the driver.

>> 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 ?

Should I also remove stm32_usart_tx_interrupt_enable() / 
stm32_usart_tx_interrupt_disable() , which does the same thing for other 
bits in the interrupt register ?

That sounds to me like making the code harder to read, not easier.

[...]
diff mbox series

Patch

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);
+}
+
 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);
+}
+
 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;