diff mbox series

[OpenWrt-Devel,v4,1/2] serial: ar933x_uart: add rs485 support

Message ID 20200211183357.GA551352@makrotopia.org
State Superseded
Headers show
Series [OpenWrt-Devel,v4,1/2] serial: ar933x_uart: add rs485 support | expand

Commit Message

Daniel Golle Feb. 11, 2020, 6:33 p.m. UTC 
Add support for RS485 tranceiver with transmit/receive switch hooked
to a RTS GPIO pin.
Use the 'rts-gpios' and 'rs485-rts-active-low' properties as described
in devicetree/bindings/serial/rs485.yaml.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 ...61-tty-serial-ar933x-uart-rs485-gpio.patch | 138 ++++++++++++++++++
 1 file changed, 138 insertions(+)
 create mode 100644 target/linux/ath79/patches-4.19/0061-tty-serial-ar933x-uart-rs485-gpio.patch

Comments

Petr Štetiar Feb. 12, 2020, 12:43 p.m. UTC | #1 
Daniel Golle <daniel@makrotopia.org> [2020-02-11 20:33:57]:

Hi,

it really still feels somehow weird, that's mainly why I've suggested to
take this through upstream first in my previous email.

> +@@ -103,10 +106,42 @@ static inline void ar933x_uart_stop_tx_i
> + static inline void ar933x_uart_putc(struct ar933x_uart_port *up, int ch)
> + {
> + 	unsigned int rdata;
> ++	struct serial_rs485 rs485conf = up->port.rs485;
> + 
> + 	rdata = ch & AR933X_UART_DATA_TX_RX_MASK;
> + 	rdata |= AR933X_UART_DATA_TX_CSR;
> +-	ar933x_uart_write(up, AR933X_UART_DATA_REG, rdata);
> ++
> ++	if (rs485conf.flags & SER_RS485_ENABLED) {
> ++		unsigned int timeout = 60000;
> ++		unsigned long flags;
> ++		unsigned int status;
> ++
> ++		/* Disable RX interrupt */
> ++		spin_lock_irqsave(&up->port.lock, flags);

FYI this code path:

 ar933x_uart_interrupt
  ar933x_uart_tx_chars
   ar933x_uart_putc

has acquired spin_lock, disabled TX interrupt, and this codepath:

 ar933x_uart_console_write
  ar933x_uart_console_putchar
   ar933x_uart_putc

has acquired spin_lock and disabled all interrupts already.

> ++		up->ier &= ~AR933X_UART_INT_RX_VALID;
> ++		ar933x_uart_write(up, AR933X_UART_INT_EN_REG, up->ier);

that looks like ar933x_uart_stop_rx() copy&paste

> ++		/* wait for transmission to end */
> ++		do {
> ++			status = ar933x_uart_read(up, AR933X_UART_CS_REG);
> ++			if (--timeout == 0)
> ++				break;
> ++			udelay(1);
> ++		} while ((status & AR933X_UART_CS_TX_BUSY) != 0);

This above looks like ar933x_uart_wait_xmitr() copy&paste but just done
differently, and I miss the point why ar933x_uart_wait_xmitr() cant be reused
here as well.

> ++		ar933x_uart_write(up, AR933X_UART_INT_REG, AR933X_UART_INT_RX_VALID);
> ++		/* remove the character from the FIFO */
> ++		ar933x_uart_write(up, AR933X_UART_DATA_REG, AR933X_UART_DATA_RX_CSR);

I really dont get this part and BTW it possibly breaks `rs485-rx-during-tx`
DTS binding.

-- ynezz
Karl Palsson Feb. 12, 2020, 12:55 p.m. UTC | #2 
Petr  Štetiar  <ynezz@true.cz> wrote:

> 
> I really dont get this part and BTW it possibly breaks
> `rs485-rx-during-tx` DTS binding.

Just as an aside, that's not a required binding, and not used all
that often either. Not all hardware even supports this, even if
you make sure the driver handles it well. The transceivers
normally have separate pins for driver enable and (not) receiver
enable, but they're (very) often wired together, so you _can't_
rx during tx. If your hardware does offer it, it can be used to
do collision detection, but it's definitely an optional binding.
I'd _MUCH_ rather see the basic bindings working _at all_ rather
than asking for this optional one.

Sincerely,
Karl Palsson
Daniel Golle Feb. 12, 2020, 2:34 p.m. UTC | #3 
Hi Petr,

thanks for looking at all that mess I'm extracting from GPL sources...
I've looking at how things are supposed to be done and re-wrote the
RS-485 and half-duplex parts from scratch.

On Wed, Feb 12, 2020 at 01:43:35PM +0100, Petr Štetiar wrote:
> Daniel Golle <daniel@makrotopia.org> [2020-02-11 20:33:57]:
> 
> Hi,
> 
> it really still feels somehow weird, that's mainly why I've suggested to
> take this through upstream first in my previous email.
> 
> > +@@ -103,10 +106,42 @@ static inline void ar933x_uart_stop_tx_i
> > + static inline void ar933x_uart_putc(struct ar933x_uart_port *up, int ch)
> > + {
> > + 	unsigned int rdata;
> > ++	struct serial_rs485 rs485conf = up->port.rs485;
> > + 
> > + 	rdata = ch & AR933X_UART_DATA_TX_RX_MASK;
> > + 	rdata |= AR933X_UART_DATA_TX_CSR;
> > +-	ar933x_uart_write(up, AR933X_UART_DATA_REG, rdata);
> > ++
> > ++	if (rs485conf.flags & SER_RS485_ENABLED) {
> > ++		unsigned int timeout = 60000;
> > ++		unsigned long flags;
> > ++		unsigned int status;
> > ++
> > ++		/* Disable RX interrupt */
> > ++		spin_lock_irqsave(&up->port.lock, flags);
> 
> FYI this code path:
> 
>  ar933x_uart_interrupt
>   ar933x_uart_tx_chars
>    ar933x_uart_putc
> 
> has acquired spin_lock, disabled TX interrupt, and this codepath:
> 
>  ar933x_uart_console_write
>   ar933x_uart_console_putchar
>    ar933x_uart_putc
> 
> has acquired spin_lock and disabled all interrupts already.

I agree, I looked at other drivers and it doesn't make sense to put
that into the putc() function like Teltonika folks did in their SDK.
See my from-scratch re-write following shortly.

> 
> > ++		up->ier &= ~AR933X_UART_INT_RX_VALID;
> > ++		ar933x_uart_write(up, AR933X_UART_INT_EN_REG, up->ier);
> 
> that looks like ar933x_uart_stop_rx() copy&paste

I've abstracted enabling/disabling the RX interrupt in my re-write.

> 
> > ++		/* wait for transmission to end */
> > ++		do {
> > ++			status = ar933x_uart_read(up, AR933X_UART_CS_REG);
> > ++			if (--timeout == 0)
> > ++				break;
> > ++			udelay(1);
> > ++		} while ((status & AR933X_UART_CS_TX_BUSY) != 0);
> 
> This above looks like ar933x_uart_wait_xmitr() copy&paste but just done
> differently, and I miss the point why ar933x_uart_wait_xmitr() cant be reused
> here as well.

There is a slight difference there:
ar933x_uart_wait_xmitr() waits for the output FIFO to allow for new
characters to be put on the FIFO by checking AR933X_UART_DATA_TX_CSR.
This is different from checking whether the send buffer has run
entirely empty and all characters have been sent on the line which is
what AR933X_UART_CS_TX_BUSY checks for and what we want here.

> 
> > ++		ar933x_uart_write(up, AR933X_UART_INT_REG, AR933X_UART_INT_RX_VALID);
> > ++		/* remove the character from the FIFO */
> > ++		ar933x_uart_write(up, AR933X_UART_DATA_REG, AR933X_UART_DATA_RX_CSR);
> 
> I really dont get this part and BTW it possibly breaks `rs485-rx-during-tx`
> DTS binding.

I've abstracted the half-duplex parts similar to how other drivers
did in my rewrite.
diff mbox series

Patch

diff --git a/target/linux/ath79/patches-4.19/0061-tty-serial-ar933x-uart-rs485-gpio.patch b/target/linux/ath79/patches-4.19/0061-tty-serial-ar933x-uart-rs485-gpio.patch
new file mode 100644
index 0000000000..be62087814
--- /dev/null
+++ b/target/linux/ath79/patches-4.19/0061-tty-serial-ar933x-uart-rs485-gpio.patch
@@ -0,0 +1,138 @@ 
+--- a/drivers/tty/serial/Kconfig
++++ b/drivers/tty/serial/Kconfig
+@@ -1296,6 +1296,7 @@ config SERIAL_AR933X
+ 	tristate "AR933X serial port support"
+ 	depends on HAVE_CLK && ATH79
+ 	select SERIAL_CORE
++	select SERIAL_MCTRL_GPIO if GPIOLIB
+ 	help
+ 	  If you have an Atheros AR933X SOC based board and want to use the
+ 	  built-in UART of the SoC, say Y to this option.
+--- a/drivers/tty/serial/ar933x_uart.c
++++ b/drivers/tty/serial/ar933x_uart.c
+@@ -29,6 +29,8 @@
+ 
+ #include <asm/mach-ath79/ar933x_uart.h>
+ 
++#include "serial_mctrl_gpio.h"
++
+ #define DRIVER_NAME "ar933x-uart"
+ 
+ #define AR933X_UART_MAX_SCALE	0xff
+@@ -47,6 +49,7 @@ struct ar933x_uart_port {
+ 	unsigned int		min_baud;
+ 	unsigned int		max_baud;
+ 	struct clk		*clk;
++	struct mctrl_gpios	*gpios;
+ };
+ 
+ static inline unsigned int ar933x_uart_read(struct ar933x_uart_port *up,
+@@ -103,10 +106,42 @@ static inline void ar933x_uart_stop_tx_i
+ static inline void ar933x_uart_putc(struct ar933x_uart_port *up, int ch)
+ {
+ 	unsigned int rdata;
++	struct serial_rs485 rs485conf = up->port.rs485;
+ 
+ 	rdata = ch & AR933X_UART_DATA_TX_RX_MASK;
+ 	rdata |= AR933X_UART_DATA_TX_CSR;
+-	ar933x_uart_write(up, AR933X_UART_DATA_REG, rdata);
++
++	if (rs485conf.flags & SER_RS485_ENABLED) {
++		unsigned int timeout = 60000;
++		unsigned long flags;
++		unsigned int status;
++
++		/* Disable RX interrupt */
++		spin_lock_irqsave(&up->port.lock, flags);
++		up->ier &= ~AR933X_UART_INT_RX_VALID;
++		ar933x_uart_write(up, AR933X_UART_INT_EN_REG, up->ier);
++
++		ar933x_uart_write(up, AR933X_UART_DATA_REG, rdata);
++
++		/* wait for transmission to end */
++		do {
++			status = ar933x_uart_read(up, AR933X_UART_CS_REG);
++			if (--timeout == 0)
++				break;
++			udelay(1);
++		} while ((status & AR933X_UART_CS_TX_BUSY) != 0);
++
++		ar933x_uart_write(up, AR933X_UART_INT_REG, AR933X_UART_INT_RX_VALID);
++		/* remove the character from the FIFO */
++		ar933x_uart_write(up, AR933X_UART_DATA_REG, AR933X_UART_DATA_RX_CSR);
++		/* Enable RX interrupt */
++		up->ier |= AR933X_UART_INT_RX_VALID;
++		ar933x_uart_write(up, AR933X_UART_INT_EN_REG, up->ier);
++
++		spin_unlock_irqrestore(&up->port.lock, flags);
++	} else {
++		ar933x_uart_write(up, AR933X_UART_DATA_REG, rdata);
++	}
+ }
+ 
+ static unsigned int ar933x_uart_tx_empty(struct uart_port *port)
+@@ -125,11 +160,21 @@ static unsigned int ar933x_uart_tx_empty
+ 
+ static unsigned int ar933x_uart_get_mctrl(struct uart_port *port)
+ {
+-	return TIOCM_CAR;
++	struct ar933x_uart_port *up =
++		container_of(port, struct ar933x_uart_port, port);
++	int ret = TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
++
++	mctrl_gpio_get(up->gpios, &ret);
++
++	return ret;
+ }
+ 
+ static void ar933x_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
+ {
++	struct ar933x_uart_port *up =
++		container_of(port, struct ar933x_uart_port, port);
++
++	mctrl_gpio_set(up->gpios, mctrl);
+ }
+ 
+ static void ar933x_uart_start_tx(struct uart_port *port)
+@@ -511,6 +556,13 @@ static const struct uart_ops ar933x_uart
+ 	.verify_port	= ar933x_uart_verify_port,
+ };
+ 
++static int ar933x_config_rs485(struct uart_port *port,
++				struct serial_rs485 *rs485conf)
++{
++	port->rs485 = *rs485conf;
++	return 0;
++}
++
+ #ifdef CONFIG_SERIAL_AR933X_CONSOLE
+ static struct ar933x_uart_port *
+ ar933x_console_ports[CONFIG_SERIAL_AR933X_NR_UARTS];
+@@ -680,6 +732,8 @@ static int ar933x_uart_probe(struct plat
+ 		goto err_disable_clk;
+ 	}
+ 
++	uart_get_rs485_mode(&pdev->dev, &port->rs485);
++
+ 	port->mapbase = mem_res->start;
+ 	port->line = id;
+ 	port->irq = irq_res->start;
+@@ -690,6 +744,7 @@ static int ar933x_uart_probe(struct plat
+ 	port->regshift = 2;
+ 	port->fifosize = AR933X_UART_FIFO_SIZE;
+ 	port->ops = &ar933x_uart_ops;
++	port->rs485_config = ar933x_config_rs485;
+ 
+ 	baud = ar933x_uart_get_baud(port->uartclk, AR933X_UART_MAX_SCALE, 1);
+ 	up->min_baud = max_t(unsigned int, baud, AR933X_UART_MIN_BAUD);
+@@ -697,6 +752,10 @@ static int ar933x_uart_probe(struct plat
+ 	baud = ar933x_uart_get_baud(port->uartclk, 0, AR933X_UART_MAX_STEP);
+ 	up->max_baud = min_t(unsigned int, baud, AR933X_UART_MAX_BAUD);
+ 
++	up->gpios = mctrl_gpio_init(port, 0);
++	if (IS_ERR(up->gpios) && PTR_ERR(up->gpios) != -ENOSYS)
++		return PTR_ERR(up->gpios);
++
+ #ifdef CONFIG_SERIAL_AR933X_CONSOLE
+ 	ar933x_console_ports[up->port.line] = up;
+ #endif