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